Bug 145906 - Web Inspector: [META] images in a grid view
Summary: Web Inspector: [META] images in a grid view
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.10
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 163995 164098 164349 164407
Blocks: 195630
  Show dependency treegraph
 
Reported: 2015-06-11 23:04 PDT by Alexander Deplov
Modified: 2019-03-12 10:53 PDT (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Deplov 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.
Comment 1 Radar WebKit Bug Importer 2015-06-11 23:04:24 PDT
<rdar://problem/21353466>
Comment 2 Joseph Pecoraro 2015-06-12 15:22:34 PDT
This is a great idea!
Comment 3 Devin Rousso 2016-08-28 17:00:14 PDT
Created attachment 287242 [details]
Patch
Comment 4 Devin Rousso 2016-08-28 17:00:50 PDT
Created attachment 287243 [details]
[Image] After Patch is applied
Comment 5 Matt Baker 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.
Comment 6 Matt Baker 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; }
Comment 7 Matt Baker 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).
Comment 8 Matt Baker 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).
Comment 9 Joseph Pecoraro 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.
Comment 10 Joseph Pecoraro 2016-08-29 16:49:03 PDT
Oo, I just saw your attachment. I like that idea.
Comment 11 Nikita Vasilyev 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.
Comment 12 Matt Baker 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.
Comment 13 Matt Baker 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.
Comment 14 Devin Rousso 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).
Comment 15 Matt Baker 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!
Comment 16 Devin Rousso 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?
Comment 17 Matt Baker 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.
Comment 18 Devin Rousso 2016-08-30 14:19:33 PDT
Created attachment 287429 [details]
Patch
Comment 19 Matt Baker 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.
Comment 20 Matt Baker 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.
Comment 21 Devin Rousso 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?
Comment 22 Matt Baker 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
Comment 23 Devin Rousso 2016-08-30 16:42:39 PDT
Created attachment 287452 [details]
Patch
Comment 24 Brian Burg 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.
Comment 25 Devin Rousso 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.
Comment 26 Devin Rousso 2016-09-03 16:55:44 PDT
Created attachment 287873 [details]
Patch
Comment 27 Build Bot 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
Comment 28 Build Bot 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
Comment 29 Build Bot 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
Comment 30 Build Bot 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
Comment 31 Build Bot 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
Comment 32 Build Bot 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
Comment 33 Devin Rousso 2016-09-03 19:20:54 PDT
Created attachment 287886 [details]
Patch
Comment 34 Build Bot 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
Comment 35 Build Bot 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
Comment 36 Build Bot 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
Comment 37 Build Bot 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
Comment 38 Devin Rousso 2016-09-03 21:56:33 PDT
Created attachment 287896 [details]
Patch
Comment 39 Devin Rousso 2016-09-03 22:10:35 PDT
Created attachment 287897 [details]
Patch
Comment 40 Brian Burg 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.
Comment 41 Devin Rousso 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.
Comment 42 Devin Rousso 2016-10-25 12:44:53 PDT
Created attachment 292802 [details]
Patch
Comment 43 Build Bot 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
Comment 44 Build Bot 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
Comment 45 Build Bot 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
Comment 46 Build Bot 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
Comment 47 Build Bot 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
Comment 48 Build Bot 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
Comment 49 Devin Rousso 2016-10-25 16:10:51 PDT
Created attachment 292842 [details]
Patch