Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: remove base::Value from WebContentsPreferences #30193

Merged
merged 4 commits into from
Jul 26, 2021

Conversation

nornagon
Copy link
Member

@nornagon nornagon commented Jul 19, 2021

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 Merge method makes me uneasy, but I don't yet fully understand why it is
architected that way currently. It has to do with webviews.

This refactor is motivated by a desire to change the default for sandbox
under 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 Merge is called, it is not knowable
whether 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 the base::Value that backed the WebContentsPreferences. Now it only includes specifically those values that we reference in our own code.

Checklist

  • PR description included and stakeholders cc'd
  • npm test passes

Release Notes

Notes: none

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jul 20, 2021
@nornagon
Copy link
Member Author

ping @zcbenz @miniak , would be great if you could take a look at this!

Copy link
Member

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a this is much better than saving things in a dictionary 👍 .

@nornagon
Copy link
Member Author

CI failures are unrelated; merging.

@nornagon nornagon merged commit 385d0f5 into main Jul 26, 2021
@nornagon nornagon deleted the webpreferences-refactor branch July 26, 2021 16:04
@release-clerk
Copy link

release-clerk bot commented Jul 26, 2021

No Release Notes

@nornagon
Copy link
Member Author

Note, this likely breaks @electron/remote, which checks for enableRemoteModule in getLastWebPreferences: https://github.com/electron/remote/blob/c80f8533abb8279c322779ad56ff1336c3eb181f/src/main/server.ts#L291-L292

@miniak
Copy link
Contributor

miniak commented Jul 26, 2021

@nornagon I had vacation last week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants