[webkit-reviews] review denied: [Bug 122546] Web Inspector: inspector forgets open navigation sidebar when re-opening : [Attachment 213774] v1

[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 122546: Web Inspector: inspector forgets open navigation sidebar when
re-opening
https://bugs.webkit.org/show_bug.cgi?id=122546

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

------- Additional Comments from Timothy Hatcher <timothy at apple.com>
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.ApplicationC
acheManager.Event.FrameManifestAdded,
this._resolveAndShowPendingContentViewCookie, this);
> +   
WebInspector.frameResourceManager.addEventListener(WebInspector.FrameResourceMa
nager.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._pendingContentViewCo
okie, 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",
    ...
};