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.

Sign up for GitHub

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

Jump to bottom

feat: add enableWebSQL webpreference #23311

Merged
merged 2 commits into from
May 6, 2020
Merged

feat: add enableWebSQL webpreference #23311

merged 2 commits into from
May 6, 2020

Conversation

deepak1556
Copy link
Member

@deepak1556 deepak1556 commented Apr 28, 2020 •
edited
Loading

Description of Change

Allows to disable websql api in a backward compatible way

Checklist

Release Notes

Notes: enableWebSQL is a new webpreference option to enable/disable websql api

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Apr 28, 2020
nornagon
nornagon reviewed Apr 28, 2020
View reviewed changes
docs/api/browser-window.md
@@ -385,6 +385,8 @@ It creates a new `BrowserWindow` with native properties as set by the `options`.
visible to users.
* `spellcheck` Boolean (optional) - Whether to enable the builtin spellchecker.
Default is `true`.
* `enableWebSQL` Boolean (optional) - Whether to enable the [WebSQL api](https://www.w3.org/TR/webdatabase/).
Default is `true`.
Copy link
Member

Choose a reason for hiding this comment

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

Do we plan to set the default to false in a followup?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thats up for discussion, I didn't know if we wanted to keep it enabled by default upto a certain electron version and then disable it ?

shell/renderer/content_settings_observer.cc
Comment on lines +24 to +27
if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
switches::kEnableWebSQL)) {
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this a purely renderer-side check? Is there any way we can disable this browser-side as well?

Copy link
Member Author

@deepak1556 deepak1556 Apr 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We can't disable on browser side in an isolated way, it only manages quota for storage and establishes connection to SQLite https://source.chromium.org/chromium/chromium/src/+/master:storage/browser/database/

The renderer side check is pretty consistent and powerful.

There is flag --disable-databases https://source.chromium.org/chromium/chromium/src/+/master:content/public/common/content_switches.cc;l=116 but it would disable both indexedDB and webSQL

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Apr 29, 2020
@zcbenz zcbenz requested a review from nornagon May 1, 2020 01:54
nornagon
nornagon approved these changes May 5, 2020
View reviewed changes
Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

If this is a renderer-side check only, then I think this being a process-wide switch rather than a per-RenderFrame switch is wrong. However, since most of our other web preferences have the same issue, I won't block this PR on the issue.

@deepak1556
Copy link
Member Author

Thanks @nornagon , but we don't currently expose a way to set per RenderFrame flags from the browser process. We have to expose the RenderFrameHost as a property on the webContents to achieve it or some other similar mechanism.

Is there another approach you have in mind ?

@nornagon
Copy link
Member

nornagon commented May 6, 2020

Not currently, I think this is something @loc is exploring.

@deepak1556
Copy link
Member Author

Cool, thanks!

Failing tests are unrelated, merging.

@deepak1556 deepak1556 merged commit a707a3e into master May 6, 2020
@release-clerk
Copy link

release-clerk bot commented May 6, 2020

Release Notes Persisted

enableWebSQL is a new webpreference option to enable/disable websql api

@deepak1556 deepak1556 deleted the robo/disable_websql branch May 6, 2020 19:53
@deepak1556
Copy link
Member Author

/trop run backport-to 9-x-y

@trop
Copy link
Contributor

trop bot commented May 13, 2020

The backport process for this PR has been manually initiated -
sending your commits to "9-x-y"!

@trop trop bot mentioned this pull request May 13, 2020
Merged
@deepak1556
Copy link
Member Author

/trop run backport-to 8-x-y

@trop
Copy link
Contributor

trop bot commented May 13, 2020

I have automatically backported this PR to "9-x-y", please check out #23580

@trop trop bot added the in-flight/9-x-y label May 13, 2020
@trop
Copy link
Contributor

trop bot commented May 13, 2020

The backport process for this PR has been manually initiated -
sending your commits to "8-x-y"!

@trop
Copy link
Contributor

trop bot commented May 13, 2020

I was unable to backport this PR to "8-x-y" cleanly;
you will need to perform this backport manually.

deepak1556 added a commit that referenced this pull request May 13, 2020
@deepak1556
feat: add enableWebSQL webpreference (#23311)
33e4ca5
deepak1556 added a commit that referenced this pull request May 13, 2020
@deepak1556
feat: add enableWebSQL webpreference (#23311)
966c363
@deepak1556 deepak1556 mentioned this pull request May 13, 2020
Merged
@trop
Copy link
Contributor

trop bot commented May 13, 2020

@deepak1556 has manually backported this PR to "8-x-y", please check out #23581

@deepak1556 deepak1556 mentioned this pull request May 13, 2020
Merged
@trop
Copy link
Contributor

trop bot commented May 13, 2020

@deepak1556 has manually backported this PR to "7-2-x", please check out #23582

@trop trop bot added the in-flight/7-2-x label May 13, 2020
deepak1556 added a commit that referenced this pull request May 14, 2020
@deepak1556
feat: add enableWebSQL webpreference (#23311)
4a5b224
@trop trop bot removed the in-flight/7-2-x label May 14, 2020
jkleinsc pushed a commit that referenced this pull request May 14, 2020
@deepak1556
feat: add enableWebSQL webpreference (#23582)
ac7ec60 
* feat: add enableWebSQL webpreference (#23311)

* fix tests
jkleinsc pushed a commit that referenced this pull request May 15, 2020
@deepak1556
feat: add enableWebSQL webpreference (#23311) (#23581)
2a44b1a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zcbenz zcbenz zcbenz approved these changes

@nornagon nornagon nornagon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@deepak1556 @nornagon @zcbenz