feat: allow defining RequestContext preferences #4

jodett · 2023-01-23T07:48:42Z

Thanks for your great work.

For our case we would like to set some RequestContext properties for our browser instances.
We have the feeling others could use that feature too and would be happy to share if you agree.
It would be great if you could review it and if it meets your standards merge it.
If you have any suggestions, let us know so we can improve this PR.

Best Regards

amaitland

Thanks for the PR 👍

In future I'd ask that you perform your own review before submitting your Pull Request.
Make sure code formatting is consistent with current formatting, remove any formatting only or unrelated code changes (there are multiple in this case).

In terms of functionality it looks like you've only added to the WPF implementation, The WinForms implementation should also be updated to support the same functionality (no mention of this being WPF only in your comments or title).

amaitland · 2023-01-23T23:54:54Z

CefSharp.OutOfProcess.BrowserProcess/BrowserProcessHandler.cs

@@ -44,7 +44,7 @@ protected override void Dispose(bool disposing)
        {
            base.Dispose(disposing);

-            if(disposing)
+            if (disposing)


In future please avoid formatting only changes.

forgot to disable the autoformatter, pardon, should now be cleaned, i resolved all other formatting comments and left this one open for you to confirm it's ok now

CefSharp.OutOfProcess.Wpf.HwndHost/ChromiumWebBrowser.cs

amaitland · 2023-01-24T00:06:05Z

CefSharp.OutOfProcess.Interface/IOutOfProcessClientRpc.cs

+        /// Modify Global RequestContext-Preferences
+        /// </summary>
+        /// /// <param name="requestContextPreferences">Request Context Preferences to set.</param>
+        void UpdateGlobalRequestContextPreferences(IDictionary<string, object> requestContextPreferences);


What's the use case for this method exactly?

we don't have one atm, just added it because CEF allows setting global requestContext preferences.
should i remove it?

amaitland · 2023-01-24T00:07:02Z

CefSharp.OutOfProcess.Interface/IOutOfProcessClientRpc.cs

+        /// </summary>
+        /// <param name="browserId">browser id</param>
+        /// <param name="requestContextPreferences">Request Context Preferences to set.</param>
+        void UpdateRequestContextPreferences(int browserId, IDictionary<string, object> requestContextPreferences);


Please rename method to SetRequestContextPreferences so it matches RequestContext.SetPreference naming.

CefSharp.OutOfProcess.BrowserProcess/BrowserProcessHandler.cs

amaitland · 2023-01-24T00:09:02Z

CefSharp.OutOfProcess.BrowserProcess/BrowserProcessHandler.cs

+                {
+                    foreach (KeyValuePair<string, object> pref in preferences)
+                    {
+                        requestContext?.SetPreference(pref.Key, pref.Value, out _);


There's no point looping through all the kvps in the dict if the request context is null.

i removed the null-conditional operator, because the "is"-clause makes sure we only proceed with a non-null requestContext

amaitland · 2023-01-24T00:10:21Z

CefSharp.OutOfProcess.BrowserProcess/BrowserProcessHandler.cs

+                IRequestContext requestContext = null;
+                if (requestContextPreferences != null)
+                {
+                    requestContext = new RequestContext();


Assigning a new RequestContext here dramatically changes the behaviour, you've not provided any way to manage the RequestContextSettings, so you'll always end up with an in memory cache.

This needs a rethink.

would it use the previous configured cefsettings if i use the globalrequestcontext values?
and therefore use the cachePath set there

…text per browser

jodett · 2023-01-31T08:31:55Z

Thanks for your reply.

Excuse my first attempt, i didn't expect such a fast response and should have opened it as a draft first.
I removed all styling-only changes.
I hope i have honored all your suggestions and would just need a decision if the methods manipulating the GlobalRequestContext should be removed.
We don't have a usecase for these but thought it would make the PR more feature-complete.
I also added the implementation for WinForms.

Awaiting your thoughts with Best Regards
Jo

amaitland

See comments inline

amaitland · 2023-02-01T00:35:19Z

CefSharp.OutOfProcess.Interface/IOutOfProcessClientRpc.cs

@@ -35,8 +36,22 @@ public interface IOutOfProcessClientRpc
        /// <param name="parentHwnd">parent Hwnd</param>
        /// <param name="url">start url</param>
        /// <param name="browserId">browser id</param>
+        /// <param name="requestContextPreferences">Request Context Preferences to set.</param>


The comment needs to be updated to describe the actual behaviour, a new RequestContext sharing the same settings as the Global.

amaitland · 2023-02-01T00:36:34Z

CefSharp.OutOfProcess.Core/OutOfProcessHost.cs

@@ -85,11 +86,12 @@ public string ChromiumVersion
        /// <param name="handle">handle used to host the control</param>
        /// <param name="url"></param>
        /// <param name="id"></param>
+        /// <param name="requestContextPreferences">request context preference.</param>


The comment needs to be updated to describe the actual behaviour, a new RequestContext sharing the same settings as the Global.

amaitland · 2023-02-01T00:36:55Z

CefSharp.OutOfProcess.WinForms/ChromiumWebBrowser.cs

@@ -64,13 +70,15 @@ public class ChromiumWebBrowser : Control, IChromiumWebBrowserInternal
        /// </summary>
        /// <param name="host">Out of process host</param>
        /// <param name="initialAddress">address that will be initially loaded in the browser</param>
-        public ChromiumWebBrowser(OutOfProcessHost host, string initialAddress)
+        /// <param name="requestContextPreferences">requestContextPreferences to set</param>


The comment needs to be updated to describe the actual behaviour, a new RequestContext sharing the same settings as the Global.

amaitland · 2023-02-01T00:37:09Z

CefSharp.OutOfProcess.Wpf.HwndHost/ChromiumWebBrowser.cs

@@ -279,13 +285,15 @@ public bool IsDisposed
        /// </summary>
        /// <param name="host">Out of process host</param>
        /// <param name="initialAddress">address to load initially</param>
-        public ChromiumWebBrowser(OutOfProcessHost host, string initialAddress = null)
+        /// <param name="requestContextPreferences">requestContextPreferences to set</param>


The comment needs to be updated to describe the actual behaviour, a new RequestContext sharing the same settings as the Global.

amaitland · 2023-02-01T00:42:03Z

CefSharp.OutOfProcess.Interface/IOutOfProcessClientRpc.cs

+        /// </summary>
+        /// <param name="browserId">browser id</param>
+        /// <param name="requestContextPreferences">Request Context Preferences to set.</param>
+        void SetRequestContextPreferences(int browserId, IDictionary<string, object> requestContextPreferences);


How often do you need to set multiple preferences at once? If I'm honest I don't love this. There's no error reporting, absolutely no idea if the preferences were set.

Personally I'd prefer to see something like the following where you get the bool and error string returned from the SetPreference call.

Task<SetRequestContextPreferenceResponse> SetRequestContextPreferenceAsync(int browserId, string name, object value);

jodett force-pushed the feat/requestContext branch from d9e5da9 to b0ab809 Compare January 23, 2023 14:12

amaitland changed the title ~~feat: allow defining RequestContext params~~ feat: allow defining RequestContext preferences Jan 23, 2023

amaitland requested changes Jan 24, 2023

View reviewed changes

jodett force-pushed the feat/requestContext branch 2 times, most recently from 0ecbeb9 to 87ca41d Compare January 24, 2023 07:21

feat: allow defining RequestContext params

1ff7d8a

jodett force-pushed the feat/requestContext branch from 87ca41d to 1ff7d8a Compare January 24, 2023 07:34

fix: use globalRequestcontext as template for configurable requestcon…

43b6af6

…text per browser

jodett requested a review from amaitland January 25, 2023 06:30

cleanup code

28d7c61

amaitland requested changes Feb 1, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow defining RequestContext preferences #4

feat: allow defining RequestContext preferences #4

jodett commented Jan 23, 2023

amaitland left a comment

amaitland Jan 23, 2023

jodett Jan 24, 2023 •

edited

amaitland Jan 24, 2023

jodett Jan 24, 2023

amaitland Jan 24, 2023

jodett Jan 24, 2023

amaitland Jan 24, 2023

jodett Jan 24, 2023

amaitland Jan 24, 2023

jodett Jan 24, 2023

jodett commented Jan 31, 2023 •

edited

amaitland left a comment

amaitland Feb 1, 2023

amaitland Feb 1, 2023

amaitland Feb 1, 2023

amaitland Feb 1, 2023

amaitland Feb 1, 2023

feat: allow defining RequestContext preferences #4

Are you sure you want to change the base?

feat: allow defining RequestContext preferences #4

Conversation

jodett commented Jan 23, 2023

amaitland left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodett Jan 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodett commented Jan 31, 2023 • edited

amaitland left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodett Jan 24, 2023 •

edited

jodett commented Jan 31, 2023 •

edited