[Webkit-unassigned] [Bug 122125] Web Inspector: restore navigation panel state across page reloads and reopens

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=122125


Timothy Hatcher <timothy at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #218123|review?                     |review-
               Flag|                            |




--- Comment #4 from Timothy Hatcher <timothy at apple.com>  2013-12-02 13:51:55 PST ---
(From update of attachment 218123)
View in context: https://bugs.webkit.org/attachment.cgi?id=218123&action=review

Nice work! I like this new approach. Some minor things that should be fixed (dontHideByDefault being the main one.)

> Source/WebInspectorUI/UserInterface/Main.js:723
> +WebInspector._provisionalLoadCommitted = function(event)

Where is the addEventListener call for this? Also, IIRC, some navigations do not fire ProvisionalLoadCommitted and only fire MainResourceDidChange. I think about:blank or data URLs are like that.

> Source/WebInspectorUI/UserInterface/Main.js:1106
> +    WebInspector.navigationSidebar.selectedSidebarPanel = sidebarPanel;
> +    var relaxMatchDelay = causedByReload ? matchTypeOnlyDelayForReload : matchTypeOnlyDelayForReopen;

I'd add a newline in-between these. They are disparate actions.

> Source/WebInspectorUI/UserInterface/NavigationSidebarPanel.js:47
> +    this._allContentTreeOutlines = new Set;

Set! Woot!

> Source/WebInspectorUI/UserInterface/NavigationSidebarPanel.js:171
> +        this._allContentTreeOutlines.add(contentTreeOutline);

contentTreeOutline is hidden by default when it is created. If the set is only suppose to contain the visible tree outlines, then this should only add to the set when dontHideByDefault is true.

> Source/WebInspectorUI/UserInterface/NavigationSidebarPanel.js:193
> +        // FIXME: this does not handle selections in the search results tree outline, or folder selections.

But I think it would get the search result tree outline if it is the contentTreeOutline.

> Source/WebInspectorUI/UserInterface/NavigationSidebarPanel.js:199
> +        });
> +        if (!selectedTreeElement)

Newline.

> Source/WebInspectorUI/UserInterface/NavigationSidebarPanel.js:211
> +        this._pendingViewStateCookie = cookie;
> +        // Check if any existing tree elements in any outline match the cookie.

Newline.

> Source/WebInspectorUI/UserInterface/NavigationSidebarPanel.js:550
> +        for (var i = 0; i < workList.length; ++i)
> +            if (workList[i].hasChildren)
> +                workList = workList.concat(workList[i].children);

Needs {} around the for() since it is multiline.

> Source/WebInspectorUI/UserInterface/NavigationSidebarPanel.js:553
> +        // This includes treeOutlines in the elements list, but this is harmless.
> +        return this._checkElementsForPendingViewStateCookie(workList);

I'm not sure what this comment means.

> Source/WebInspectorUI/UserInterface/NavigationSidebarPanel.js:563
> +        function treeElementMatchesCookie(treeElement) {

{ on newline.

> Source/WebInspectorUI/UserInterface/NavigationSidebarPanel.js:580
> +            return Object.keys(candidateObjectCookie).every(function valuesMatchForKey(key) {
> +                return candidateObjectCookie[key] === cookie[key];
> +            });

Object.shallowEqual?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.