Bug 97524 - Web Inspector: Add Breadcrumb on FileSystemView status bar
Summary: Web Inspector: Add Breadcrumb on FileSystemView status bar
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Taiju Tsuiki
URL:
Keywords:
Depends on:
Blocks: 68203 91709
  Show dependency treegraph
 
Reported: 2012-09-24 22:15 PDT by Taiju Tsuiki
Modified: 2014-08-03 18:31 PDT (History)
12 users (show)

See Also:


Attachments
Patch (11.18 KB, patch)
2012-09-25 06:18 PDT, Taiju Tsuiki
vsevik: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Taiju Tsuiki 2012-09-24 22:15:54 PDT
Before removing its directory tree, we need other navigation UI to parent directory. I'm adding breadcrumb in this patch.
This UI might not scale for deep directory tree. So, we should add crumb collapsion like ElementPanel does, before expose it by default.
Comment 1 Taiju Tsuiki 2012-09-25 06:18:40 PDT
Created attachment 165597 [details]
Patch
Comment 2 WebKit Review Bot 2012-09-25 07:03:50 PDT
Comment on attachment 165597 [details]
Patch

Attachment 165597 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14036034

New failing tests:
http/tests/inspector/resource-tree/resource-tree-frame-add.html
http/tests/inspector/resource-tree/resource-tree-reload.html
http/tests/inspector/appcache/appcache-iframe-manifests.html
http/tests/inspector/resource-tree/resource-tree-non-unique-url.html
http/tests/inspector/appcache/appcache-swap.html
inspector/console/command-line-api-inspect.html
inspector/debugger/source-frame.html
inspector/storage-panel-dom-storage-update.html
inspector/database-table-name-excaping.html
inspector/storage-panel-dom-storage.html
http/tests/inspector/search/resources-search-match-index.html
http/tests/inspector/appcache/appcache-manifest-with-non-existing-file.html
http/tests/inspector/filesystem/directory-tree.html
http/tests/inspector/indexeddb/resources-panel.html
Comment 3 Build Bot 2012-09-25 07:37:37 PDT
Comment on attachment 165597 [details]
Patch

Attachment 165597 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14030095

New failing tests:
http/tests/inspector/appcache/appcache-iframe-manifests.html
http/tests/inspector/appcache/appcache-swap.html
inspector/console/command-line-api-inspect.html
inspector/debugger/source-frame.html
inspector/storage-panel-dom-storage-update.html
inspector/database-table-name-excaping.html
inspector/storage-panel-dom-storage.html
http/tests/inspector/search/resources-search-match-index.html
http/tests/inspector/appcache/appcache-manifest-with-non-existing-file.html
http/tests/inspector/resource-tree/resource-tree-non-unique-url.html
Comment 4 Vsevolod Vlasov 2012-10-03 01:37:55 PDT
Comment on attachment 165597 [details]
Patch

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.
Comment 5 Brian Burg 2014-08-03 18:31:05 PDT
This feature is no longer in WebKit.