WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
145906
Web Inspector: [META] images in a grid view
https://bugs.webkit.org/show_bug.cgi?id=145906
Summary
Web Inspector: [META] images in a grid view
Alexander Deplov
Reported
2015-06-11 23:04:08 PDT
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.
Attachments
Images grid
(50.87 KB, image/jpeg)
2015-06-11 23:04 PDT
,
Alexander Deplov
no flags
Details
Patch
(23.08 KB, patch)
2016-08-28 17:00 PDT
,
Devin Rousso
mattbaker
: review-
Details
Formatted Diff
Diff
[Image] After Patch is applied
(514.82 KB, image/png)
2016-08-28 17:00 PDT
,
Devin Rousso
no flags
Details
[Image] follow-up idea
(575.41 KB, image/png)
2016-08-29 16:32 PDT
,
Matt Baker
no flags
Details
Patch
(24.19 KB, patch)
2016-08-29 19:59 PDT
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Patch
(30.30 KB, patch)
2016-08-30 14:19 PDT
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
[Image] patch applied, resource error breaks grid content view
(502.95 KB, image/png)
2016-08-30 14:48 PDT
,
Matt Baker
no flags
Details
Patch
(32.29 KB, patch)
2016-08-30 16:42 PDT
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Patch
(82.83 KB, patch)
2016-09-03 16:55 PDT
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews102 for mac-yosemite
(891.48 KB, application/zip)
2016-09-03 17:46 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2
(1.11 MB, application/zip)
2016-09-03 17:51 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews113 for mac-yosemite
(1.53 MB, application/zip)
2016-09-03 17:53 PDT
,
Build Bot
no flags
Details
Patch
(82.90 KB, patch)
2016-09-03 19:20 PDT
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews101 for mac-yosemite
(892.16 KB, application/zip)
2016-09-03 20:11 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews104 for mac-yosemite-wk2
(1.07 MB, application/zip)
2016-09-03 20:16 PDT
,
Build Bot
no flags
Details
Patch
(110.59 KB, patch)
2016-09-03 21:56 PDT
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Patch
(109.82 KB, patch)
2016-09-03 22:10 PDT
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Patch
(103.32 KB, patch)
2016-10-25 12:44 PDT
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews112 for mac-yosemite
(1.71 MB, application/zip)
2016-10-25 14:24 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews107 for mac-yosemite-wk2
(1.47 MB, application/zip)
2016-10-25 14:39 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews101 for mac-yosemite
(1.11 MB, application/zip)
2016-10-25 14:50 PDT
,
Build Bot
no flags
Details
Patch
(103.95 KB, patch)
2016-10-25 16:10 PDT
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Show Obsolete
(18)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2015-06-11 23:04:24 PDT
<
rdar://problem/21353466
>
Joseph Pecoraro
Comment 2
2015-06-12 15:22:34 PDT
This is a great idea!
Devin Rousso
Comment 3
2016-08-28 17:00:14 PDT
Created
attachment 287242
[details]
Patch
Devin Rousso
Comment 4
2016-08-28 17:00:50 PDT
Created
attachment 287243
[details]
[Image] After Patch is applied
Matt Baker
Comment 5
2016-08-29 15:28:26 PDT
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.
Matt Baker
Comment 6
2016-08-29 15:48:25 PDT
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; }
Matt Baker
Comment 7
2016-08-29 16:18:00 PDT
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).
Matt Baker
Comment 8
2016-08-29 16:32:29 PDT
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).
Joseph Pecoraro
Comment 9
2016-08-29 16:48:00 PDT
(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.
Joseph Pecoraro
Comment 10
2016-08-29 16:49:03 PDT
Oo, I just saw your attachment. I like that idea.
Nikita Vasilyev
Comment 11
2016-08-29 16:58:34 PDT
I like this a lot! I think it would be nice to show resource name right under the image, Finder-style.
Matt Baker
Comment 12
2016-08-29 17:07:31 PDT
(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.
Matt Baker
Comment 13
2016-08-29 17:08:45 PDT
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.
Devin Rousso
Comment 14
2016-08-29 19:59:24 PDT
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).
Matt Baker
Comment 15
2016-08-30 12:23:57 PDT
(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!
Devin Rousso
Comment 16
2016-08-30 13:18:31 PDT
(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?
Matt Baker
Comment 17
2016-08-30 13:45:34 PDT
(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.
Devin Rousso
Comment 18
2016-08-30 14:19:33 PDT
Created
attachment 287429
[details]
Patch
Matt Baker
Comment 19
2016-08-30 14:48:04 PDT
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.
Matt Baker
Comment 20
2016-08-30 14:49:07 PDT
Comment on
attachment 287429
[details]
Patch Patch looks good, r- for now only because of the issue with resource errors.
Devin Rousso
Comment 21
2016-08-30 16:06:15 PDT
(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?
Matt Baker
Comment 22
2016-08-30 16:13:35 PDT
(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
Devin Rousso
Comment 23
2016-08-30 16:42:39 PDT
Created
attachment 287452
[details]
Patch
Blaze Burg
Comment 24
2016-09-01 11:55:44 PDT
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.
Devin Rousso
Comment 25
2016-09-03 10:09:07 PDT
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.
Devin Rousso
Comment 26
2016-09-03 16:55:44 PDT
Created
attachment 287873
[details]
Patch
Build Bot
Comment 27
2016-09-03 17:46:09 PDT
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
Build Bot
Comment 28
2016-09-03 17:46:13 PDT
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
Build Bot
Comment 29
2016-09-03 17:51:19 PDT
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
Build Bot
Comment 30
2016-09-03 17:51:22 PDT
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
Build Bot
Comment 31
2016-09-03 17:53:43 PDT
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
Build Bot
Comment 32
2016-09-03 17:53:47 PDT
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
Devin Rousso
Comment 33
2016-09-03 19:20:54 PDT
Created
attachment 287886
[details]
Patch
Build Bot
Comment 34
2016-09-03 20:11:16 PDT
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
Build Bot
Comment 35
2016-09-03 20:11:19 PDT
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
Build Bot
Comment 36
2016-09-03 20:16:10 PDT
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
Build Bot
Comment 37
2016-09-03 20:16:14 PDT
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
Devin Rousso
Comment 38
2016-09-03 21:56:33 PDT
Created
attachment 287896
[details]
Patch
Devin Rousso
Comment 39
2016-09-03 22:10:35 PDT
Created
attachment 287897
[details]
Patch
Blaze Burg
Comment 40
2016-09-06 17:57:10 PDT
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.
Devin Rousso
Comment 41
2016-09-07 00:25:12 PDT
(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.
Devin Rousso
Comment 42
2016-10-25 12:44:53 PDT
Created
attachment 292802
[details]
Patch
Build Bot
Comment 43
2016-10-25 14:24:14 PDT
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
Build Bot
Comment 44
2016-10-25 14:24:18 PDT
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
Build Bot
Comment 45
2016-10-25 14:39:27 PDT
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
Build Bot
Comment 46
2016-10-25 14:39:31 PDT
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
Build Bot
Comment 47
2016-10-25 14:50:02 PDT
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
Build Bot
Comment 48
2016-10-25 14:50:06 PDT
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
Devin Rousso
Comment 49
2016-10-25 16:10:51 PDT
Created
attachment 292842
[details]
Patch
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug