Bug 178045 - Web Inspector: replace TypeVerifier with subclasses of WI.Collection
Summary: Web Inspector: replace TypeVerifier with subclasses of WI.Collection
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-10-06 22:59 PDT by Devin Rousso
Modified: 2017-10-25 16:50 PDT (History)
6 users (show)

See Also:


Attachments
Patch (22.98 KB, patch)
2017-10-06 23:57 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (22.85 KB, patch)
2017-10-25 01:17 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (24.02 KB, patch)
2017-10-25 11:44 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (25.15 KB, patch)
2017-10-25 12:57 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (1.21 MB, application/zip)
2017-10-25 13:37 PDT, Build Bot
no flags Details
Patch (29.52 KB, patch)
2017-10-25 13:55 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 Devin Rousso 2017-10-06 22:59:12 PDT
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.
Comment 1 Devin Rousso 2017-10-06 23:57:45 PDT
Created attachment 323087 [details]
Patch
Comment 2 Matt Baker 2017-10-07 12:52:57 PDT
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).
Comment 3 Devin Rousso 2017-10-07 13:26:59 PDT
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.
Comment 4 Devin Rousso 2017-10-25 01:17:32 PDT
Created attachment 324803 [details]
Patch
Comment 5 Radar WebKit Bug Importer 2017-10-25 08:42:46 PDT
<rdar://problem/35174307>
Comment 6 BJ Burg 2017-10-25 08:46:51 PDT
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.
Comment 7 Devin Rousso 2017-10-25 11:44:19 PDT
Created attachment 324858 [details]
Patch

Not sure how I didn't see that... 🤦
Comment 8 BJ Burg 2017-10-25 11:54:39 PDT
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.

 ;-)
Comment 9 Devin Rousso 2017-10-25 12:57:17 PDT
Created attachment 324871 [details]
Patch
Comment 10 BJ Burg 2017-10-25 13:26:09 PDT
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,
Comment 11 Build Bot 2017-10-25 13:37:28 PDT
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
Comment 12 Build Bot 2017-10-25 13:37:29 PDT
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
Comment 13 Devin Rousso 2017-10-25 13:55:57 PDT
Created attachment 324888 [details]
Patch

Forgot to upload test changes
Comment 14 Devin Rousso 2017-10-25 13:56:46 PDT
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).
Comment 15 Devin Rousso 2017-10-25 16:50:23 PDT
Comment on attachment 324888 [details]
Patch

Clearing flags on attachment: 324888

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