We would like to enable access to local storage from Web Inspector. However, there are two problems: 1. privateBrowsingEnabled setting is true for web inspector pages (see WebInspectorClient.mm), this prevents web inspector from modifying local storage (see StorageAreaImpl::setItem). According to comment, this is done to keep inspector out of history. This can be solved by introducing new setting "excludeFromHistory" and then using it instead of "privateBrowsingEnabled" in WebInspectorClient. 2. chrome registers "chrome:" schema as no access (see RenderThread::EnsureWebKitInitialized), this prevents web inspector from accessing the local storage. We can add "allowLocalStorageAccess" setting like it's already done for "allowUniversalAccessFromFileURLs" (see Document::initSecurityContext). But I don't feel like it's a good solution. Any thoughts?
Chris, do you have thoughts on #2? #1 sounds good to me.
Created attachment 71064 [details] Use localStorage to persist front-end settings.
I have 2 concerns: 1) LocalStorage is racy if you use it from 2 threads/processes. Are you OK with this? 2) It doesn't seem as though there's any code to migrate forward settings from the old system to the new. Is that OK?
(In reply to comment #3) > I have 2 concerns: > 1) LocalStorage is racy if you use it from 2 threads/processes. Are you OK with this? That could be a problem, we should probably use Web SQL DB instead. > 2) It doesn't seem as though there's any code to migrate forward settings from the old system to the new. Is that OK? There's very few settings currently. So it's ok to forget the old settings.
(In reply to comment #4) > (In reply to comment #3) > > I have 2 concerns: > > 1) LocalStorage is racy if you use it from 2 threads/processes. Are you OK with this? > That could be a problem, we should probably use Web SQL DB instead. We just need a dictionary for our settings, and SQL is much more than that. And since we aren't concerned about atomicity, localStorage should work pretty well for us.
Created attachment 71564 [details] Use shorter keys (no "application" prefix).
Comment on attachment 71564 [details] Use shorter keys (no "application" prefix). View in context: https://bugs.webkit.org/attachment.cgi?id=71564&action=review > WebCore/inspector/front-end/Panel.js:37 > + WebInspector.applicationSettings.installSetting(this._sidebarWidthSettingName(), "application", undefined); It should be WebInspector.settings.installApplicationSetting and WebInspector.settings.installProjectSetting > WebCore/inspector/front-end/Settings.js:97 > + return key + "." + this._mainResourceURL; You might want to ignore fragment here.
Created attachment 71734 [details] Comments addressed.
Comment on attachment 71734 [details] Comments addressed. View in context: https://bugs.webkit.org/attachment.cgi?id=71734&action=review > WebCore/inspector/front-end/inspector.js:604 > + WebInspector.applicationSettings.dispatchEventToListeners("loaded"); Now that we load settings synchronously, we no longer need the listener. We should always expect them to be available. You can remove this notification and all the places that listen to settings load. Unless we go the DB way... What about reading from multiple threads / removing keys. fishd had some reservations on that front...
Committed r70622: <http://trac.webkit.org/changeset/70622>
(In reply to comment #9) > (From update of attachment 71734 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=71734&action=review > > > WebCore/inspector/front-end/inspector.js:604 > > + WebInspector.applicationSettings.dispatchEventToListeners("loaded"); > > Now that we load settings synchronously, we no longer need the listener. We should always expect them to be available. You can remove this notification and all the places that listen to settings load. Unless we go the DB way... done > What about reading from multiple threads / removing keys. fishd had some reservations on that front... Issue with reading from multiple threads is that localStorage.foo = localStorage.foo + 1 is not atomic, but that's ok for us.