Summary: | Web Inspector: Canvas tab: recordings should have a unique name | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Baker <mattbaker> | ||||||||||||
Component: | Web Inspector | Assignee: | Matt Baker <mattbaker> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | buildbot, commit-queue, hi, inspector-bugzilla-changes, rniwa, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 173807, 175485 | ||||||||||||||
Attachments: |
|
Description
Matt Baker
2017-10-11 15:26:19 PDT
Created attachment 323472 [details]
Patch
Comment on attachment 323472 [details] Patch Attachment 323472 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4829132 New failing tests: imported/w3c/web-platform-tests/XMLHttpRequest/open-url-worker-origin.htm Created attachment 323486 [details]
Archive of layout-test-results from ews101 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 323498 [details]
Patch
Comment on attachment 323498 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323498&action=review This looks fine to me, but I'd like to either discuss WI.Recording.setUniqueName or see it changed to non-static before r+. > Source/WebInspectorUI/UserInterface/Models/Recording.js:68 > + static setUniqueName(recording, suggestedName) Having this as static seems a bit weird. I would imagine that you could redo this akin to WI.Canvas.prototype.displayName. I understand that it uses static objects on WI.Recording (e.g. `_importedRecordingNames`), but I think we are fine to use those in non-static functions too. > Source/WebInspectorUI/UserInterface/Models/Recording.js:70 > + let recordingNames; NIT: I'd rename this as `recordingNameSet`, as `recordingNames` suggests to me that it's an array. > Source/WebInspectorUI/UserInterface/Models/Recording.js:75 > + recordingNames = new Set; > + recording.source[WI.Recording.CanvasRecordingNamesSymbol] = recordingNames; I usually put all of these on one line. recordingNames = recording.source[WI.Recording.CanvasRecordingNamesSymbol] = new Set; > Source/WebInspectorUI/UserInterface/Models/Recording.js:197 > + get name() { return this._name; } Please add `this._name` to the constructor with some default value. Comment on attachment 323498 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323498&action=review >> Source/WebInspectorUI/UserInterface/Models/Recording.js:68 >> + static setUniqueName(recording, suggestedName) > > Having this as static seems a bit weird. I would imagine that you could redo this akin to WI.Canvas.prototype.displayName. I understand that it uses static objects on WI.Recording (e.g. `_importedRecordingNames`), but I think we are fine to use those in non-static functions too. Since it takes an optional suggested name, which might not even be used, this wouldn't make sense as a setter. A non-static method would be okay. Created attachment 323788 [details]
Patch
Created attachment 323793 [details]
Patch
Comment on attachment 323793 [details]
Patch
r=me
Comment on attachment 323793 [details] Patch Clearing flags on attachment: 323793 Committed r223322: <https://trac.webkit.org/changeset/223322> All reviewed patches have been landed. Closing bug. |