Bug 178045 – Web Inspector: replace TypeVerifier with subclasses of WI.Collection

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

Instead of using TypeVerifiers as distinguishers between different types of WI.Collection, we can create subclasses that allow for greater distinction between usages of WI.Collection.  This will mainly be used to make determining the placeholder text for WI.CollectionContentView simpler.

[Devin Rousso]

Created attachment 323087 [details]
Patch

[Matt Baker]

Comment on attachment 323087 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323087&action=review

> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:-279
> -            this.debounce(250).addSubview(this._contentPlaceholder);

What was the reason for removing this? This delay shouldn't be that perceptible, and prevents flicker when collection items are added right after the ContentView is shown (which is typical when viewing canvases).

[Devin Rousso]

Comment on attachment 323087 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323087&action=review

>> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:-279
>> -            this.debounce(250).addSubview(this._contentPlaceholder);
> 
> What was the reason for removing this? This delay shouldn't be that perceptible, and prevents flicker when collection items are added right after the ContentView is shown (which is typical when viewing canvases).

I was seeing a noticeable delay before the text appeared, such as viewing resource type folders on <https://www.apple.com> (like Fonts or Scripts).  I wasn't seeing any flickering elsewhere, but I haven't tested extensively.

[Devin Rousso]

Created attachment 324803 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/35174307>

[Blaze Burg]

Comment on attachment 324803 [details]
Patch

This is better, but it still seems more complicated than necessary. If you are going to go all the way to subclassing, then each subclass should override objectHasRequiredType(o) rather than passing in a predicate function to the constructor. I don't see any reason why the predicate needs to be dynamically bound at construction time since it never seems to vary for a particular subclass.

[Devin Rousso]

Created attachment 324858 [details]
Patch

Not sure how I didn't see that... 🤦

[Blaze Burg]

Comment on attachment 324858 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=324858&action=review

r=me please wait for Mac EWS

> Source/WebInspectorUI/UserInterface/Models/Collection.js:70
> +        return true;

If this is required to be overridden, then please 

throw WI.NotImplementedError.subclassMustOverride();

> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:57
>              return WI.UIString("Shader Programs");

I wouldn't be sad if you made this a displayName() that must be overridden, but I won't hold up this patch for it either.

 ;-)

[Devin Rousso]

Created attachment 324871 [details]
Patch

[Blaze Burg]

Comment on attachment 324871 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=324871&action=review

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:364
>  localizedStrings["Enable Layers Tab"] = "Enable Layers Tab";

Is this leftover from another patch? It seems unrelated,

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:-837
> -localizedStrings["Show Contexts in Resources Tab"] = "Show Contexts in Resources Tab";

Is this leftover from another patch? It seems unrelated,

[Build Bot]

Comment on attachment 324871 [details]
Patch

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

New failing tests:
inspector/unit-tests/collection.html

[Build Bot]

Created attachment 324884 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Devin Rousso]

Created attachment 324888 [details]
Patch

Forgot to upload test changes

[Devin Rousso]

Comment on attachment 324871 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=324871&action=review

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:364
>>  localizedStrings["Enable Layers Tab"] = "Enable Layers Tab";
> 
> Is this leftover from another patch? It seems unrelated,

Yeah, I forgot to update localizedStrings.js when I removed the experimental Canvas settings.  Sorry.

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:-837
>> -localizedStrings["Show Contexts in Resources Tab"] = "Show Contexts in Resources Tab";
> 
> Is this leftover from another patch? It seems unrelated,

Ditto (364).

[Devin Rousso]

Comment on attachment 324888 [details]
Patch

Clearing flags on attachment: 324888

Committed r223997: <https://trac.webkit.org/changeset/223997>