Bug 19207 - Inspector should remember the size of sidebars set by the user
Summary: Inspector should remember the size of sidebars set by the user
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Yury Semikhatsky
URL:
Keywords:
Depends on: 41687
Blocks:
  Show dependency treegraph
 
Reported: 2008-05-23 02:54 PDT by Anthony Ricaud
Modified: 2010-07-07 03:10 PDT (History)
7 users (show)

See Also:


Attachments
Patch (15.82 KB, patch)
2010-07-06 23:43 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (16.57 KB, patch)
2010-07-07 01:53 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (18.63 KB, patch)
2010-07-07 02:15 PDT, Yury Semikhatsky
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anthony Ricaud 2008-05-23 02:54:04 PDT
It's a user preference, it should be remembered over instances.
Comment 1 Anthony Ricaud 2008-07-16 11:16:02 PDT
I'd like to work on the "remember" bugs (bug 19208 and bug 19209). I need a way to store these preferences. I've thought about Database Storage and localStorage. Maybe there's also a way to do this in WebKit.
Comment 2 Timothy Hatcher 2008-07-16 12:04:17 PDT
I would say localStorage is the best bet. But it would be lumped into all file URL's localStorage. So Database with a unique name for the inspector might be better. 
Comment 3 Matt Lilek 2008-08-02 15:23:15 PDT
(In reply to comment #2)
> I would say localStorage is the best bet. But it would be lumped into all file
> URL's localStorage. So Database with a unique name for the inspector might be
> better. 
> 

Database is async, which is a problem when retrieving the information, so unless I'm missing a really obvious (or sneaky) way of making the database stuff synchronous, it's probably going to have to be localStorage.
Comment 4 Timothy Hatcher 2008-08-20 14:44:15 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > I would say localStorage is the best bet. But it would be lumped into all file
> > URL's localStorage. So Database with a unique name for the inspector might be
> > better. 
> > 
> 
> Database is async, which is a problem when retrieving the information, so
> unless I'm missing a really obvious (or sneaky) way of making the database
> stuff synchronous, it's probably going to have to be localStorage.
> 

Yes asynchronous makes it annoying. But you can still use it.

When the Inspector opens, read all the preferences from the database into a Preferences object. Then when a preferences needs to change store the preference in the database and in the Preferences object. Any clients of the preferences will read only from the Preferences object, but the values do still persist.

This is basically how localStorage works behind the scenes.

Maybe we could consider a way to tell the Web Inspector's WebKit to use a special backing database for it's localStorage, so it does not conflict with other file URLs?
Comment 5 Alexei Masterov 2010-05-24 11:16:49 PDT
Web Inspector: [REGRESSION]: Styles Sidebar (Panel) would NOT re-open with the width set by the user in the previous session.

To reproduce:
==============
1. Open Elements Panel.
2. Change the layout (width for example) of the sidebar
3. Close Inspector
4. Open Inspector
5. Observe sidebar return to the default layout.

NOTE: previous version of WebKit had this working.
Comment 6 Alexei Masterov 2010-07-06 07:14:07 PDT
This bug is more general than 41687, so 41687 is set as a dependency
Comment 7 Yury Semikhatsky 2010-07-06 23:43:00 PDT
Created attachment 60691 [details]
Patch
Comment 8 Pavel Feldman 2010-07-06 23:56:39 PDT
Comment on attachment 60691 [details]
Patch

WebCore/inspector/front-end/Panel.js:35
 +      this._name = name;
Nit: it is easy for ancestors to hide it. I'd rename it to something more specific (_panelName)

WebCore/inspector/front-end/Panel.js:42
 +      get toolbarItemClass()
No need to expose it, just use the name in get toolbarItem below.

WebCore/inspector/front-end/Panel.js:327
 +      get _sidebarWidthSettingName()
I'd not use getters for private fields (use function instead).

WebCore/inspector/front-end/Panel.js:392
 +          WebInspector.log("saving " + this._sidebarWidthSettingName + " = " + this.sidebarElement.offsetWidth);
Remove logging.

WebCore/inspector/front-end/Settings.js:61
 +      WebInspector.applicationSettings._installSetting("elementsSidebarWidth", "elements-sidebar-width", undefined);
I think panel constructors should register theses instead - otherwise hack. r- for this. I'd make installSetting public and allow its invocation before the load takes place. I'd do the same thing as now + initialize store with default values. Then I'd populate existing settings from JSON.parse(settingsString) in _load.
Comment 9 Yury Semikhatsky 2010-07-07 01:53:21 PDT
Created attachment 60706 [details]
Patch
Comment 10 Yury Semikhatsky 2010-07-07 02:15:22 PDT
Created attachment 60709 [details]
Patch
Comment 11 Yury Semikhatsky 2010-07-07 02:15:56 PDT
Addressed all comments. Please take a look.
Comment 12 Yury Semikhatsky 2010-07-07 03:10:46 PDT
Committed r62647