Created attachment 254778 [details] Images grid I would like to see a feature in the web inspector, that allows me to click on the Resources tab and then on the Images folder to see all website images in a grid view. Look at the attached image.
<rdar://problem/21353466>
This is a great idea!
Created attachment 287242 [details] Patch
Created attachment 287243 [details] [Image] After Patch is applied
Reviewing now. Really cool. One thought after applying the patch: it would be nice to be able to navigate to a specific resource from the grid view. Not sure about how the UI could look, maybe a go-to arrow appears on hover. It could also be as simple as making the images themselves links to the specific resource.
Comment on attachment 287242 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=287242&action=review Each time the Images folder is selected the items in the grid continue to grow (adds duplicates): 1. Load any page > Resources tab 2. Select Images folder 3. Select any other tree element 4. Select Images folder => All entries in the grid are repeated > Source/WebInspectorUI/UserInterface/Models/ResourceCollection.js:50 > + } Simple getters can be one line: get type() { return this._type; }
Comment on attachment 287242 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=287242&action=review > Source/WebInspectorUI/UserInterface/Models/Resource.js:127 > + static typeViewableAsCollection(type) If this took a Resource.Type instead of a key, it could just be: return type === WebInspector.Resource.Type.Image; > Source/WebInspectorUI/UserInterface/Models/ResourceCollection.js:123 > + if (!this._resourcesTypeMap.has(resource.type)) We should avoid two Map lookups: let resources = this._resourcesTypeMap.get(resource.type); if (resources) resources.push(resource); else this._resourcesTypeMap.set(resource.type, [resource]); > Source/WebInspectorUI/UserInterface/Models/ResourceCollection.js:141 > + if (!this.type && this._resourcesTypeMap.has(resource.type)) We should avoid two Map lookups. This could be: if (!this.type) { let resources = this._resourcesTypeMap.get(resource.type); if (resources) resources.remove(resource); } > Source/WebInspectorUI/UserInterface/Models/ResourceCollection.js:175 > + if (resource.type !== this.type) { Should be be asserting that resource.type !== this.type? If they are the same, that would mean that the type of the collection itself must have changed, which shouldn't happen. > Source/WebInspectorUI/UserInterface/Views/ResourceCollectionContentView.js:28 > + constructor(resource) This should be renamed `resourceCollection`, since ResourceCollection inherits from Object, not Resource. > Source/WebInspectorUI/UserInterface/Views/ResourceCollectionContentView.js:44 > + get resource() This can be a single line. > Source/WebInspectorUI/UserInterface/Views/ResourceCollectionContentView.js:51 > + this._resource.resources.forEach((resource) => { We've been favoring for...of over forEach. > Source/WebInspectorUI/UserInterface/Views/ResourceCollectionContentView.js:64 > + case WebInspector.Resource.Type.Image: Nit: most of our factories are written as a series of if...returns, with a return at the end for the default case (or an assert and null return if no default case exists).
Created attachment 287348 [details] [Image] follow-up idea It could be cool to allow filtering the collection grid by image type (svg, png, etc), and show an item count in the navigation path after the folder name (only when the collection is displayed). Just a thought. Would be best as a follow-up issue (if at all).
(In reply to comment #8) > Created attachment 287348 [details] > [Image] follow-up idea > > It could be cool to allow filtering the collection grid by image type (svg, > png, etc), and show an item count in the navigation path after the folder > name (only when the collection is displayed). > > Just a thought. Would be best as a follow-up issue (if at all). Maybe it could respect the Sidebar's filter? I dunno how I feel about that, but it would match the idea that you click on the Folder and it shows you each of the things visible in the folder if you expand.
Oo, I just saw your attachment. I like that idea.
I like this a lot! I think it would be nice to show resource name right under the image, Finder-style.
(In reply to comment #11) > I like this a lot! > > I think it would be nice to show resource name right under the image, > Finder-style. +1. At the very least it should appear in a tooltip when hovering.
Comment on attachment 287242 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=287242&action=review > Source/WebInspectorUI/UserInterface/Views/FolderTreeElement.js:36 > + this.selectable = !!type; This should be removed, as it breaks keyboard traversal of the tree outline.
Created attachment 287366 [details] Patch (In reply to comment #12) > (In reply to comment #11) > > I like this a lot! > > > > I think it would be nice to show resource name right under the image, > > Finder-style. > > +1. At the very least it should appear in a tooltip when hovering. I would actually recommend against using a visible name in the view, especially for situations where the image is small and the title is very long (like the Apple image in the screenshot). I do think, however, that a tooltip is a good idea. (In reply to comment #13) > Comment on attachment 287242 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=287242&action=review > > > Source/WebInspectorUI/UserInterface/Views/FolderTreeElement.js:36 > > + this.selectable = !!type; > > This should be removed, as it breaks keyboard traversal of the tree outline. I just tried this and it worked just fine. It doesn't allow you to expand/collapse with left/right, so if that's a huge issue then I can remove this (it is just very weird to select "Images", select a different folder, and then still see the "Images" content displayed).
(In reply to comment #14) > Created attachment 287366 [details] > Patch > > (In reply to comment #12) > > (In reply to comment #11) > > > I like this a lot! > > > > > > I think it would be nice to show resource name right under the image, > > > Finder-style. > > > > +1. At the very least it should appear in a tooltip when hovering. > > I would actually recommend against using a visible name in the view, > especially for situations where the image is small and the title is very > long (like the Apple image in the screenshot). I do think, however, that a > tooltip is a good idea. > > (In reply to comment #13) > > Comment on attachment 287242 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=287242&action=review > > > > > Source/WebInspectorUI/UserInterface/Views/FolderTreeElement.js:36 > > > + this.selectable = !!type; > > > > This should be removed, as it breaks keyboard traversal of the tree outline. > > I just tried this and it worked just fine. It doesn't allow you to > expand/collapse with left/right, so if that's a huge issue then I can remove > this (it is just very weird to select "Images", select a different folder, > and then still see the "Images" content displayed). (In reply to comment #14) > Created attachment 287366 [details] > Patch > > (In reply to comment #12) > > (In reply to comment #11) > > > I like this a lot! > > > > > > I think it would be nice to show resource name right under the image, > > > Finder-style. > > > > +1. At the very least it should appear in a tooltip when hovering. > > I would actually recommend against using a visible name in the view, > especially for situations where the image is small and the title is very > long (like the Apple image in the screenshot). I do think, however, that a > tooltip is a good idea. > > (In reply to comment #13) > > Comment on attachment 287242 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=287242&action=review > > > > > Source/WebInspectorUI/UserInterface/Views/FolderTreeElement.js:36 > > > + this.selectable = !!type; > > > > This should be removed, as it breaks keyboard traversal of the tree outline. > > I just tried this and it worked just fine. It doesn't allow you to > expand/collapse with left/right, so if that's a huge issue then I can remove > this (it is just very weird to select "Images", select a different folder, > and then still see the "Images" content displayed). We don't want to break keyboard traversal of the tree. I agree it's a little weird that after clicking a folder tree element the previous content is still showing, but this is nothing new. Let's bring back keyboard navigation and get it landed!
(In reply to comment #15) > We don't want to break keyboard traversal of the tree. I agree it's a little > weird that after clicking a folder tree element the previous content is > still showing, but this is nothing new. > > Let's bring back keyboard navigation and get it landed! I don't think it's a very good experience to select a tree element and have the content not change. How about making it so that when a folder is selected, it will display a blank content view if the type is not viewable as a collection?
(In reply to comment #16) > (In reply to comment #15) > > We don't want to break keyboard traversal of the tree. I agree it's a little > > weird that after clicking a folder tree element the previous content is > > still showing, but this is nothing new. > > > > Let's bring back keyboard navigation and get it landed! > > I don't think it's a very good experience to select a tree element and have > the content not change. How about making it so that when a folder is > selected, it will display a blank content view if the type is not viewable > as a collection? Sounds good! Instead of a blank content view we could do what Chrome does and show the folder name centered in the content view.
Created attachment 287429 [details] Patch
Created attachment 287434 [details] [Image] patch applied, resource error breaks grid content view When resources in the Images folder fail to load, the error message views added to the image grid are still styled to fill the content view. Maybe we should keep these views in the grid, but have special styling for when they appear in this context? A square tile with just an icon could even work, since the tooltip now shows the resource name.
Comment on attachment 287429 [details] Patch Patch looks good, r- for now only because of the issue with resource errors.
(In reply to comment #19) > Created attachment 287434 [details] > [Image] patch applied, resource error breaks grid content view > > When resources in the Images folder fail to load, the error message views > added to the image grid are still styled to fill the content view. > > Maybe we should keep these views in the grid, but have special styling for > when they appear in this context? A square tile with just an icon could even > work, since the tooltip now shows the resource name. So I'd like to keep this as using the content view for the ResourceCollection type because if we decide to support displaying other collections in the future, it requires barely any changes. I think it may be easier to do something like adding an event dispatch when the ResourceContentView fails to load (ResourceContentView.prototype._contentError) and if it is fired, just remove the view entirely from the ResourceCollectionContentView. I don't see a benefit displaying an error message (especially since the user isn't selecting the individual item, but an overview of them all)... Also, do you have any steps to reproduce with the error?
(In reply to comment #21) > (In reply to comment #19) > > Created attachment 287434 [details] > > [Image] patch applied, resource error breaks grid content view > > > > When resources in the Images folder fail to load, the error message views > > added to the image grid are still styled to fill the content view. > > > > Maybe we should keep these views in the grid, but have special styling for > > when they appear in this context? A square tile with just an icon could even > > work, since the tooltip now shows the resource name. > > So I'd like to keep this as using the content view for the > ResourceCollection type because if we decide to support displaying other > collections in the future, it requires barely any changes. > > I think it may be easier to do something like adding an event dispatch when > the ResourceContentView fails to load > (ResourceContentView.prototype._contentError) and if it is fired, just > remove the view entirely from the ResourceCollectionContentView. I don't > see a benefit displaying an error message (especially since the user isn't > selecting the individual item, but an overview of them all)... That makes sense. > Also, do you have any steps to reproduce with the error? 1. Inspect theverge.com 2. Click the images folder => First ContentView spans parent width, can be scrolled to show other images after
Created attachment 287452 [details] Patch
Comment on attachment 287452 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=287452&action=review A few comments, I need to apply the patch and test it out yet. > Source/WebInspectorUI/ChangeLog:39 > + Overrides functions from WebInspector.TreeElement.prototype so that the represented objects This seems like a screwy inversion of model/view. We wouldn't be able to test ResourceCollection directly if the views are maintaining the models. Do you have any ideas for moving the model management into FrameResourceManager, or is it too difficult? We don't really have a separate model of groups of resources, afaik. Maybe we should, so we can catch regressions in folderization (not necessarily in this patch). > Source/WebInspectorUI/UserInterface/Models/Resource.js:127 > + static typeViewableAsCollection(type) Nit: typeIsViewableAsCollection. > Source/WebInspectorUI/UserInterface/Views/ResourceCollectionContentView.js:80 > + let handleContentError = () => { I don't understand why we need a WeakMap of listeners that all do the same thing. We can get the resource from the event as event.target.representedObject, right? Then we can use a single instance method. > Source/WebInspectorUI/UserInterface/Views/TitleView.js:33 > + this.element.textContent = title; This seems somewhat redundant with WebInspector.createMessageTextView. We should probably try to move other code to using this as a placeholder subview.
Comment on attachment 287452 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=287452&action=review >> Source/WebInspectorUI/ChangeLog:39 >> + Overrides functions from WebInspector.TreeElement.prototype so that the represented objects > > This seems like a screwy inversion of model/view. We wouldn't be able to test ResourceCollection directly if the views are maintaining the models. Do you have any ideas for moving the model management into FrameResourceManager, or is it too difficult? We don't really have a separate model of groups of resources, afaik. Maybe we should, so we can catch regressions in folderization (not necessarily in this patch). So I got this working by creating sub-collections from ResourceCollection, but I encountered a bigger issue. We also use FolderTreeElement for models objects that aren't resources (Frames, Flows, Extra Scripts, etc), so using ResourceCollection as the representedObject is not valid. I am working on creating a new Collection model object that uses "instanceof" with constructors instead of directly checking the type. >> Source/WebInspectorUI/UserInterface/Views/ResourceCollectionContentView.js:80 >> + let handleContentError = () => { > > I don't understand why we need a WeakMap of listeners that all do the same thing. We can get the resource from the event as event.target.representedObject, right? Then we can use a single instance method. This is just me being stupid :/ >> Source/WebInspectorUI/UserInterface/Views/TitleView.js:33 >> + this.element.textContent = title; > > This seems somewhat redundant with WebInspector.createMessageTextView. We should probably try to move other code to using this as a placeholder subview. I agree, but WebInspector.createMessageTextView has a bit more logic to it than just displaying text, so I will address this in a separate patch.
Created attachment 287873 [details] Patch
Comment on attachment 287873 [details] Patch Attachment 287873 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2003077 New failing tests: inspector/dom/highlightSelector.html inspector/model/frame-extra-scripts.html inspector/dom/highlightFrame.html inspector/dom/highlightNode.html http/tests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.html http/tests/inspector/console/cross-domain-inspected-node-access.html
Created attachment 287880 [details] Archive of layout-test-results from ews102 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 287873 [details] Patch Attachment 287873 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2003107 New failing tests: inspector/dom/highlightSelector.html inspector/model/frame-extra-scripts.html inspector/dom/highlightFrame.html inspector/dom/highlightNode.html http/tests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.html http/tests/inspector/console/cross-domain-inspected-node-access.html
Created attachment 287881 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 287873 [details] Patch Attachment 287873 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2003088 New failing tests: inspector/dom/highlightSelector.html inspector/model/frame-extra-scripts.html inspector/dom/highlightFrame.html inspector/dom/highlightNode.html http/tests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.html http/tests/inspector/console/cross-domain-inspected-node-access.html
Created attachment 287882 [details] Archive of layout-test-results from ews113 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 287886 [details] Patch
Comment on attachment 287886 [details] Patch Attachment 287886 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2003695 New failing tests: inspector/dom/highlightSelector.html inspector/model/frame-extra-scripts.html inspector/dom/highlightFrame.html inspector/dom/highlightNode.html http/tests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.html http/tests/inspector/console/cross-domain-inspected-node-access.html
Created attachment 287888 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 287886 [details] Patch Attachment 287886 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2003702 New failing tests: inspector/dom/highlightSelector.html inspector/model/frame-extra-scripts.html inspector/dom/highlightFrame.html inspector/dom/highlightNode.html http/tests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.html http/tests/inspector/console/cross-domain-inspected-node-access.html
Created attachment 287889 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Created attachment 287896 [details] Patch
Created attachment 287897 [details] Patch
Comment on attachment 287897 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=287897&action=review This is awesome. Unfortunately, it's too big for any of us to review in a reasonable amount of time, which is why it's been in the review queue for so long. I propose that you split it this way: 1. Collection and ResourceCollection + unit tests to demonstrate how they work 2. Refactor existing folders to use Collection model objects 3. (+ 4?) Add new content views for Resource Collections I'm not going to go over the whole patch, but it looks like an improvement over the previous architecture. I will say that the TypeVerifier stuff looks kind of intrusive, and could be simpler. A Collection could have an elementPredicate, which is just a function that returns true or false. Then, a static method on WI.Collection could create such predicates in the common cases (doing an instanceof check against a WI constructor). In complicated cases, clients could vend their own element predicate (or it could be a static method on WI.Resource or WI.ResourceCollection, for example). > Source/WebInspectorUI/UserInterface/Models/Collection.js:59 > + this.onItemAdded(item); We don't use the this.on* prefix. it should just be this.itemAdded > Source/WebInspectorUI/UserInterface/Models/Collection.js:68 > + Ditto. > Source/WebInspectorUI/UserInterface/Models/Collection.js:80 > + this.onItemsCleared(items); Ditto. > Source/WebInspectorUI/UserInterface/Models/Frame.js:321 > + for (let childFrame of this._childFrames.items) { in cases like this, maybe this._childFramesCollection would make the type of the receiver more obvious. > Source/WebInspectorUI/UserInterface/Models/ResourceCollection.js:31 > + super(items, WebInspector.Resource.verifierForType(resourceType)); This static method is good. Although, it's probably better to put it into the ResourceCollection class since it's only used by this client of Resource. > Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:34 > + this.representedObject.addEventListener(WebInspector.Collection.Event.ItemAdded, this._handleItemAdded, this); We generally name such callbacks _itemAdded and _itemRemoved. Prefixes like 'handle' and 'on' are not used. > Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:44 > + title = WebInspector.UIString("Frames"); It seems awkward to switch on what is essentially an element predicate to arrive at a UIString. I think this should just sniff the type of the collection elements, or use a ResourceCollection.Type that's inferred from its contents or set by the client of the collection.
(In reply to comment #40) > This is awesome. Thanks! > Unfortunately, it's too big for any of us to review in a reasonable amount > of time, which is why it's been in the review queue for so long. > I propose that you split it this way: > > 1. Collection and ResourceCollection + unit tests to demonstrate how they > work > 2. Refactor existing folders to use Collection model objects > 3. (+ 4?) Add new content views for Resource Collections This sounds good. I'll split it once I get a few questions answered (below). > I will say that the TypeVerifier stuff looks kind of intrusive, and could be > simpler. A Collection could have an elementPredicate, which is just a > function that returns true or false. Then, a static method on WI.Collection > could create such predicates in the common cases (doing an instanceof check > against a WI constructor). In complicated cases, clients could vend their > own element predicate (or it could be a static method on WI.Resource or > WI.ResourceCollection, for example). So I feel like this is what TypeVerifier does. The idea is that in situations where we want a collection of a single object type, we need something to verify that any added items are of that type. I originally tried using instanceof checks to verify that they are of a specific type, but the issue there is with WI.Resource, since it can be one of multiple "subtypes". The benefit of having a TypeVerifier style is that it allows for more flexibility when it comes to collection objects (you don't need to have every object be of a certain class, just so long as they all conform to a verifier function). I agree that it is a bit weird, but this seemed to be the best way to future-proof this as well. Also, I needed something that would give me a unique designation for each type AND subtype specifically for the case of WI.Resource.Type.Image, since it's the only collection type that is currently viewable as a collection. > > Source/WebInspectorUI/UserInterface/Models/ResourceCollection.js:31 > > + super(items, WebInspector.Resource.verifierForType(resourceType)); > > This static method is good. Although, it's probably better to put it into > the ResourceCollection class since it's only used by this client of Resource. I put it on WI.Resource because I figured that it only has to do with instances of WI.Resource and it may be used in other cases. > > Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:34 > > + this.representedObject.addEventListener(WebInspector.Collection.Event.ItemAdded, this._handleItemAdded, this); > > We generally name such callbacks _itemAdded and _itemRemoved. Prefixes like > 'handle' and 'on' are not used. Really? I've been using _handle for a long time. I thought we used that for any events (like _handleContextMenu). > > Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:44 > > + title = WebInspector.UIString("Frames"); > > It seems awkward to switch on what is essentially an element predicate to > arrive at a UIString. I think this should just sniff the type of the > collection elements, or use a ResourceCollection.Type that's inferred from > its contents or set by the client of the collection. I was trying to emulate what WI.Resource.displayNameForType considering that the TypeVerifier was the only object it has to distinguish itself. Also, I needed something per unique type AND subtype (specific to WI.Resource) because currently the only supported type that is viewable as a collection is WI.Resource.Type.Image.
Created attachment 292802 [details] Patch
Comment on attachment 292802 [details] Patch Attachment 292802 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2369936 New failing tests: inspector/model/frame-extra-scripts.html
Created attachment 292819 [details] Archive of layout-test-results from ews112 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 292802 [details] Patch Attachment 292802 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2370037 New failing tests: inspector/model/frame-extra-scripts.html
Created attachment 292823 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 292802 [details] Patch Attachment 292802 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2370100 New failing tests: inspector/model/frame-extra-scripts.html transitions/default-timing-function.html
Created attachment 292826 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 292842 [details] Patch