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
Pavel Podivilov
2010-10-15 02:25:14 PDT
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. |