[webkit-reviews] review denied: [Bug 122125] Web Inspector: restore navigation panel state across page reloads and reopens : [Attachment 218123] v1 patch

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at apple.com> has denied Brian Burg
<burg at cs.washington.edu>'s request for review:
Bug 122125: Web Inspector: restore navigation panel state across page reloads
and reopens
https://bugs.webkit.org/show_bug.cgi?id=122125

Attachment 218123: v1 patch
https://bugs.webkit.org/attachment.cgi?id=218123&action=review

------- Additional Comments from Timothy Hatcher <timothy at apple.com>
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?