| Summary: | Web Inspector: Add Breadcrumb on FileSystemView status bar | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Taiju Tsuiki <tzik> | ||||
| Component: | Web Inspector (Deprecated) | Assignee: | Taiju Tsuiki <tzik> | ||||
| Status: | RESOLVED INVALID | ||||||
| Severity: | Normal | CC: | apavlov, burg, bweinstein, dglazkov, joepeck, keishi, loislo, pfeldman, pmuellr, rik, webkit.review.bot, yurys | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 68203, 91709 | ||||||
| Attachments: |
|
||||||
|
Description
Taiju Tsuiki
2012-09-24 22:15:54 PDT
Created attachment 165597 [details]
Patch
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 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 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. This feature is no longer in WebKit. |