Bug 122546 - Web Inspector: inspector forgets open navigation sidebar when re-opening
Summary: Web Inspector: inspector forgets open navigation sidebar when re-opening
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-10-09 05:15 PDT by Brian Burg
Modified: 2013-10-10 16:19 PDT (History)
5 users (show)

See Also:


Attachments
v1 (52.92 KB, patch)
2013-10-09 07:07 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
v2 (64.21 KB, patch)
2013-10-10 10:02 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
v2.1 (style nits) (64.24 KB, patch)
2013-10-10 15:36 PDT, Brian Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2013-10-09 05:15:41 PDT
Always restoring through sidebars causes the inspector to lose track of the previously-opened sidebar. The current architecture of save/restore from cookie also makes it awkward to restore view state that is independent of the represented object (i.e., scroll positions or subview selections).
Comment 1 Radar WebKit Bug Importer 2013-10-09 05:16:02 PDT
<rdar://problem/15185630>
Comment 2 Brian Burg 2013-10-09 07:07:23 PDT
Created attachment 213774 [details]
v1
Comment 3 Timothy Hatcher 2013-10-09 14:33:54 PDT
Comment on attachment 213774 [details]
v1

View in context: https://bugs.webkit.org/attachment.cgi?id=213774&action=review

Looks good. Just fix up the layering violations.

> Source/WebInspectorUI/UserInterface/ApplicationCacheManager.js:115
> +        return (matchOnTypeAlone && this._applicationCacheObjects.length) ? this._applicationCacheObjects[0] : null;

No need for the ( ).

> Source/WebInspectorUI/UserInterface/ContentBrowser.js:78
> +    // These listeners are for events that could resolve a pending content view cookie.
> +    WebInspector.applicationCacheManager.addEventListener(WebInspector.ApplicationCacheManager.Event.FrameManifestAdded, this._resolveAndShowPendingContentViewCookie, this);
> +    WebInspector.frameResourceManager.addEventListener(WebInspector.FrameResourceManager.Event.MainFrameDidChange, this._resolveAndShowPendingContentViewCookie, this);
> +    WebInspector.storageManager.addEventListener(WebInspector.StorageManager.Event.DatabaseWasAdded, this._resolveAndShowPendingContentViewCookie, this);
> +    WebInspector.storageManager.addEventListener(WebInspector.StorageManager.Event.CookieStorageObjectWasAdded, this._resolveAndShowPendingContentViewCookie, this);
> +    WebInspector.storageManager.addEventListener(WebInspector.StorageManager.Event.DOMStorageObjectWasAdded, this._resolveAndShowPendingContentViewCookie, this);

This is a layering violation. Moving this to Main.js and possibly calling into a public function on ContentBrowser for generic logic would be best.

> Source/WebInspectorUI/UserInterface/ContentBrowser.js:577
> +    _resolveAndShowPendingContentViewCookie: function(matchOnTypeAlone)
> +    {
> +        var cookie = this._pendingContentViewCookie;
> +        if (!cookie)
> +            return false;
> +
> +        var representedObject = null;
> +
> +        if (cookie.type === WebInspector.ContentView.ResourceContentViewCookieType)
> +            representedObject = WebInspector.frameResourceManager.objectForCookie(cookie);
> +
> +        if (cookie.type === WebInspector.ContentView.TimelinesContentViewCookieType)
> +            representedObject = WebInspector.timelineManager.objectForCookie(cookie);
> +
> +        if (cookie.type === WebInspector.ContentView.CookieStorageContentViewCookieType || cookie.type === WebInspector.ContentView.DatabaseContentViewCookieType  || cookie.type === WebInspector.ContentView.DatabaseTableContentViewCookieType || cookie.type === WebInspector.ContentView.DOMStorageContentViewCookieType)
> +            representedObject = WebInspector.storageManager.objectForCookie(this._pendingContentViewCookie, matchOnTypeAlone);
> +
> +        if (cookie.type === WebInspector.ContentView.ApplicationCacheContentViewCookieType)
> +            representedObject = WebInspector.applicationCacheManager.objectForCookie(this._pendingContentViewCookie, matchOnTypeAlone);
> +
> +        if (!representedObject)
> +            return false;
> +
> +        // If we reached this point, then we should be able to create and/or display a content view based on the cookie.
> +        delete this._pendingContentViewCookie;
> +        if (this._lastAttemptCookieCheckingTimeout)
> +            clearTimeout(this._lastAttemptCookieCheckingTimeout);
> +
> +        // Delay this work because other listeners of the originating event might not have fired yet.
> +        // So displaying the content view before those listeners do their work might cause the
> +        // dependent view states (navigation sidebar tree elements, path components) to be wrong.
> +        function delayedWork()
> +        {
> +            this.showContentViewForRepresentedObject(representedObject, cookie);
> +        }
> +        setTimeout(delayedWork.bind(this), 0);
> +        return true;
> +    },

This should also move to Main.js since it touches many high level classes that ContentBrowser should not know about.

> Source/WebInspectorUI/UserInterface/ContentView.js:105
> +WebInspector.ContentView.ApplicationCacheContentViewCookieType = "application-cache";
> +WebInspector.ContentView.CookieStorageContentViewCookieType = "cookie-storage";
> +WebInspector.ContentView.DatabaseContentViewCookieType = "database";
> +WebInspector.ContentView.DatabaseTableContentViewCookieType = "database-table";
> +WebInspector.ContentView.DOMStorageContentViewCookieType = "dom-storage";
> +WebInspector.ContentView.TimelinesContentViewCookieType = "timeline";
> +WebInspector.ContentView.ResourceContentViewCookieType = "resource"; // includes Frame too.

This is also a layering violation. Main.js would be best. Also this format would be better and more like an enum:

WebInspector.ContentVIewCookieType = {
    ApplicationCache: "application-cache",
    ...
};
Comment 4 Brian Burg 2013-10-10 10:02:27 PDT
Created attachment 213897 [details]
v2
Comment 5 Timothy Hatcher 2013-10-10 11:46:41 PDT
Comment on attachment 213897 [details]
v2

View in context: https://bugs.webkit.org/attachment.cgi?id=213897&action=review

Looks good. Just some style nits.

> Source/WebInspectorUI/UserInterface/FrameResourceManager.js:86
> +        var representedObject = (cookie.url) ? this.resourceForURL(cookie.url) : this.mainFrame;

No need for ( ).

> Source/WebInspectorUI/UserInterface/StorageManager.js:159
> +            return (matchOnTypeAlone && array.length) ? array[0] : null;

No need for ( ).

> Source/WebInspectorUI/UserInterface/StorageManager.js:179
> +        if (cookie.type === WebInspector.ContentViewCookieType.DOMStorage)
> +            return findMatchingObjectInArray(this._domStorageObjects, function(object) {
> +                return object.host === cookie.host && object.isLocalStorage() === cookie.isLocalStorage;
> +            });

This needs { } since it is multi-line.

> Source/WebInspectorUI/UserInterface/StorageManager.js:184
> +        if (cookie.type === WebInspector.ContentViewCookieType.Database)
> +            return findMatchingObjectInArray(this._databaseObjects, function(object) {
> +                return object.host === cookie.host && object.name === cookie.name;
> +            });

Ditto.

> Source/WebInspectorUI/UserInterface/StorageManager.js:192
> +        if (cookie.type === WebInspector.ContentViewCookieType.DatabaseTable)
> +            return findMatchingObjectInArray(this._databaseObjects, function(object) {
> +                return object.host === cookie.host && object.database === cookie.name;
> +            });

Ditto.
Comment 6 Brian Burg 2013-10-10 15:36:20 PDT
Created attachment 213935 [details]
v2.1 (style nits)
Comment 7 WebKit Commit Bot 2013-10-10 16:19:26 PDT
Comment on attachment 213935 [details]
v2.1 (style nits)

Clearing flags on attachment: 213935

Committed r157269: <http://trac.webkit.org/changeset/157269>
Comment 8 WebKit Commit Bot 2013-10-10 16:19:29 PDT
All reviewed patches have been landed.  Closing bug.