Bug 122125 – Web Inspector: restore navigation panel state across page reloads and reopens

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Brian Burg]

When a page is reloaded with the same main frame, we lose all navigation history.

I think this bug is already in in Radar, but filing a public bug in case I fix it.

[Radar WebKit Bug Importer]

<rdar://problem/15115189>

[Brian Burg]

This has been fixed by a more comprehensive refactoring in my own branch. https://github.com/burg/timelapse/commit/943de47644bcf1efeb8fe019e4f5daf2fa5f51d5 I will try to extract the changes and submit a patch here.

[Brian Burg]

Created attachment 218123 [details]
v1 patch

[Timothy Hatcher]

Comment on attachment 218123 [details]
v1 patch

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?

[Brian Burg]

Comment on attachment 218123 [details]
v1 patch

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

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

Oops, the addEventListener got lost in the rebase somehow.

For navigations without ProvisionalLoadCommitted, is there another event we can use as a time to save the view state? We can't use MainResourceDidChange, as some of the state is torn down by the time that event's handlers have all run.

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

Oh, I missed that. I will rename the set and add a conditional around this statement.

With this change, are there other places where I should be adding or removing tree outlines from the set of visible outlines?

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

This comment is from a prior approach, I'll rewrite it to mention folder selections only.

>> Source/WebInspectorUI/UserInterface/NavigationSidebarPanel.js:553
>> +        return this._checkElementsForPendingViewStateCookie(workList);
> 
> I'm not sure what this comment means.

Oh, the work list included the tree outlines as well as elements. I'll change this to prune the tree outlines (which should all be at the front, thus easy to remove).

>> Source/WebInspectorUI/UserInterface/NavigationSidebarPanel.js:580
>> +            });
> 
> Object.shallowEqual?

This is asymmetric on purpose, because the saved cookie may contain other things (such as selected sidebar) besides the representedObject's serialized identity.

[Brian Burg]

Created attachment 218246 [details]
v2

[Timothy Hatcher]

Comment on attachment 218246 [details]
v2

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

> Source/WebInspectorUI/UserInterface/NavigationSidebarPanel.js:557
> +        var workList = [];
> +        this._visibleContentTreeOutlines.forEach(function(outline) { workList.push(outline); });
> +        for (var i = 0; i < workList.length; ++i) {
> +            if (workList[i].hasChildren)
> +                workList = workList.concat(workList[i].children);
> +        }
> +
> +        // Remove TreeOutline elements from the front of the work list.
> +        workList.splice(0, this._visibleContentTreeOutlines.size);

I think this can be simplified (and likely in a performant way). Can this just iterate over the TreeOutlines and then use traverseNextTreeElement to append to workList?

[Brian Burg]

(In reply to comment #7)
> (From update of attachment 218246 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218246&action=review
> 
> > Source/WebInspectorUI/UserInterface/NavigationSidebarPanel.js:557
> > +        var workList = [];
> > +        this._visibleContentTreeOutlines.forEach(function(outline) { workList.push(outline); });
> > +        for (var i = 0; i < workList.length; ++i) {
> > +            if (workList[i].hasChildren)
> > +                workList = workList.concat(workList[i].children);
> > +        }
> > +
> > +        // Remove TreeOutline elements from the front of the work list.
> > +        workList.splice(0, this._visibleContentTreeOutlines.size);
> 
> I think this can be simplified (and likely in a performant way). Can this just iterate over the TreeOutlines and then use traverseNextTreeElement to append to workList?

I couldn't figure out traverseNextTreeElement or find an example where it seems to iterate the entire tree. If it works, it would definitely be faster.

[Timothy Hatcher]

Comment on attachment 218246 [details]
v2

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

>>> Source/WebInspectorUI/UserInterface/NavigationSidebarPanel.js:557
>>> +        workList.splice(0, this._visibleContentTreeOutlines.size);
>> 
>> I think this can be simplified (and likely in a performant way). Can this just iterate over the TreeOutlines and then use traverseNextTreeElement to append to workList?
> 
> I couldn't figure out traverseNextTreeElement or find an example where it seems to iterate the entire tree. If it works, it would definitely be faster.

http://pastie.org/private/iqgn2kypobwj5eapfk1za is an example from NavigationSidebarPanel.

[Brian Burg]

Created attachment 218304 [details]
v3

[Timothy Hatcher]

Comment on attachment 218304 [details]
v3

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

> Source/WebInspectorUI/UserInterface/NavigationSidebarPanel.js:551
> +            var currentTreeElement = outline.hasChildren ? outline.children[0] : null;

outline.children[0] alone works, since it will be undefined if there are no children. But this works.

[WebKit Commit Bot]

Comment on attachment 218304 [details]
v3

Clearing flags on attachment: 218304

Committed r160025: <http://trac.webkit.org/changeset/160025>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.