[webkit-reviews] review denied: [Bug 97524] Web Inspector: Add Breadcrumb on FileSystemView status bar : [Attachment 165597] Patch

[bugzilla-daemon at webkit.org]

Vsevolod Vlasov <vsevik at chromium.org> has denied Taiju Tsuiki
<tzik at chromium.org>'s request for review:
Bug 97524: Web Inspector: Add Breadcrumb on FileSystemView status bar
https://bugs.webkit.org/show_bug.cgi?id=97524

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

------- Additional Comments from Vsevolod Vlasov <vsevik at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=165597&action=review


> Source/WebCore/inspector/front-end/BreadcrumbList.js:35
> +WebInspector.BreadcrumbList = function()

This doesn't look like a ui component to me.
It looks like a lot of boilerplate (most of it could be omitted if breadcrumb
list implementation is moved to FileSystemView) with no meaningful code.
I think you should either inline this implementation in FileSystemView or
extract something that could be reused by ElementsPanel (e.g. click handling
logic definitely belongs to the generic breadcrumb list implementation).

> Source/WebCore/inspector/front-end/BreadcrumbList.js:46
> +    set children(x)

Please don't use setters/getters. We prefer not to use them as they cause some
closure compilation problems.

> Source/WebCore/inspector/front-end/BreadcrumbList.js:56
> +    get element()

You could make element field public instead.

> Source/WebCore/inspector/front-end/BreadcrumbList.js:78
> +    this._element.wrapper = this;

redundant?

> Source/WebCore/inspector/front-end/FileSystemView.js:31
> +importScript("BreadcrumbList.js");

import scripts implied dependencies should match those in compile-front-end.
You should move add <script> element to inspector.html instead.

> Source/WebCore/inspector/front-end/FileSystemView.js:41
> +    this.registerRequiredCSS("breadcrumbList.css");

This should be moved to BreadcrumbList.js ?
Also there is no such file in the patch.

> Source/WebCore/inspector/front-end/FileSystemView.js:123
> +	   this._crumbs.updateSizes();

This method could be public (to be used in some resize handler) but I think
children setter should call it implicitly.

> Source/WebCore/inspector/front-end/FileSystemView.js:271
> +    this._fileSystemView = fileSystemView;

This one is not used currently.

> Source/WebCore/inspector/front-end/FileSystemView.js:281
> +	   this._treeElement.select();

I don't think crumb element should depend on another ui representation of the
folder (the one you are going to remove soon actually).
I would use fileSystemView and path pair instead.