refactor: remove base::Value from WebContentsPreferences#30193
Merged
refactor: remove base::Value from WebContentsPreferences#30193
Conversation
This was referenced Jul 19, 2021
Contributor
Author
zcbenz
approved these changes
Jul 26, 2021
Contributor
zcbenz
left a comment
There was a problem hiding this comment.
I think this is a this is much better than saving things in a dictionary 👍 .
Contributor
Author
|
CI failures are unrelated; merging. |
|
No Release Notes |
Contributor
Author
|
Note, this likely breaks @electron/remote, which checks for |
Contributor
|
@nornagon I had vacation last week |
georgexu99
pushed a commit
to georgexu99/electron
that referenced
this pull request
Jul 29, 2021
4 tasks
BlackHole1
pushed a commit
to BlackHole1/electron
that referenced
this pull request
Aug 30, 2021
This was referenced Aug 31, 2021
5 tasks
5 tasks
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of Change
This refactors WebContentsPreferences to store its values directly, rather than
keyed by string in a base::DictionaryValue. It also centralizes logic for
default values; callers no longer have to provide a default value. And finally,
it centralizes all parsing logic in web_contents_preferences.cc, instead of
having logic for fetching keys by string scattered around the codebase.
I think this is not yet the best form of WebContentsPreferences. In particular,
the
Mergemethod makes me uneasy, but I don't yet fully understand why it isarchitected that way currently. It has to do with webviews.
This refactor is motivated by a desire to change the default for
sandboxunder complex circumstances. The straightforward approach to that (shown
here)
turns out to not work, because the "defaults" are applied at
WebContents-creation time, so by the time
Mergeis called, it is not knowablewhether or not sandboxing was disabled by default at creation, or whether it
was unspecified. This refactor allows us to be more explicit about defaulting
logic.
NB, this slightly changes the value of
WebContents.getLastWebPreferences(). Previously it included a full copy of thebase::Valuethat backed the WebContentsPreferences. Now it only includes specifically those values that we reference in our own code.Checklist
npm testpassesRelease Notes
Notes: none