Bug 47715

Summary: Web Inspector: move frontend settings to local storage
Product: WebKit Reporter: Pavel Podivilov <podivilov>
Component: Web Inspector (Deprecated)Assignee: Pavel Podivilov <podivilov>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, beidson, bweinstein, cevans, fishd, joepeck, jorlow, keishi, loislo, pfeldman, pmuellr, rik, timothy, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Use localStorage to persist front-end settings.
none
Use shorter keys (no "application" prefix).
pfeldman: review-
Comments addressed. pfeldman: review+, pfeldman: commit-queue-

Description Pavel Podivilov 2010-10-15 02:25:14 PDT
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?
Comment 1 Jeremy Orlow 2010-10-15 02:35:31 PDT
Chris, do you have thoughts on #2?

#1 sounds good to me.
Comment 2 Pavel Podivilov 2010-10-18 11:29:09 PDT
Created attachment 71064 [details]
Use localStorage to persist front-end settings.
Comment 3 Jeremy Orlow 2010-10-19 02:42:41 PDT
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?
Comment 4 Pavel Podivilov 2010-10-19 05:03:40 PDT
(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.
Comment 5 Pavel Podivilov 2010-10-19 06:12:26 PDT
(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.
Comment 6 Pavel Podivilov 2010-10-22 08:14:36 PDT
Created attachment 71564 [details]
Use shorter keys (no "application" prefix).
Comment 7 Pavel Feldman 2010-10-22 08:44:24 PDT
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.
Comment 8 Pavel Podivilov 2010-10-25 05:02:09 PDT
Created attachment 71734 [details]
Comments addressed.
Comment 9 Pavel Feldman 2010-10-26 13:47:59 PDT
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...
Comment 10 Pavel Podivilov 2010-10-27 01:52:51 PDT
Committed r70622: <http://trac.webkit.org/changeset/70622>
Comment 11 Pavel Podivilov 2010-10-27 01:54:40 PDT
(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.