Summary: | Web Inspector: replace TypeVerifier with subclasses of WI.Collection | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | bburg, buildbot, inspector-bugzilla-changes, mattbaker, rniwa, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Attachments: |
|
Description
Devin Rousso
2017-10-06 22:59:12 PDT
Created attachment 323087 [details]
Patch
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 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. Created attachment 324803 [details]
Patch
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.
Created attachment 324858 [details]
Patch
Not sure how I didn't see that... 🤦
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. ;-) Created attachment 324871 [details]
Patch
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 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 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
Created attachment 324888 [details]
Patch
Forgot to upload test changes
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 on attachment 324888 [details] Patch Clearing flags on attachment: 324888 Committed r223997: <https://trac.webkit.org/changeset/223997> |