Bug 47715 – Web Inspector: move frontend settings to local storage

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Pavel Podivilov]

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?

[Jeremy Orlow]

Chris, do you have thoughts on #2?

#1 sounds good to me.

[Pavel Podivilov]

Created attachment 71064 [details]
Use localStorage to persist front-end settings.

[Jeremy Orlow]

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?

[Pavel Podivilov]

(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.

[Pavel Podivilov]

(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.

[Pavel Podivilov]

Created attachment 71564 [details]
Use shorter keys (no "application" prefix).

[Pavel Feldman]

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.

[Pavel Podivilov]

Created attachment 71734 [details]
Comments addressed.

[Pavel Feldman]

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...

[Pavel Podivilov]

Committed r70622: <http://trac.webkit.org/changeset/70622>

[Pavel Podivilov]

(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.