Bug 178188

Summary: Web Inspector: Canvas tab: recordings should have a unique name
Product: WebKit Reporter: Matt Baker <mattbaker>
Component: Web InspectorAssignee: 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 Flags
Patch
none
Archive of layout-test-results from ews101 for mac-elcapitan
none
Patch
none
Patch
none
Patch none

Matt Baker
Reported 2017-10-11 15:26:19 PDT
Summary: Recordings should have a unique name. New recordings should get an auto-incrememnted name like Recording 1, Recording 2, etc. Imported recordings should use the filename with the extension removed. Resolve conflicts by appending a suffix: "My Recording", "My Recording (2)", etc. Note: Recording names only need to be unique per-canvas. This is needed for the Canvas tab, which lists recordings per-canvas.
Attachments
Patch (12.58 KB, patch)
2017-10-11 15:37 PDT, Matt Baker 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews101 for mac-elcapitan (990.80 KB, application/zip)
2017-10-11 16:42 PDT, Build Bot 		no flags
Details
Patch (12.58 KB, patch)
2017-10-11 17:55 PDT, Matt Baker 		no flags
DetailsFormatted DiffDiff
Patch (13.31 KB, patch)
2017-10-13 18:55 PDT, Matt Baker 		no flags
DetailsFormatted DiffDiff
Patch (13.39 KB, patch)
2017-10-13 20:23 PDT, Matt Baker 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2017-10-11 15:29:54 PDT
<rdar://problem/34943364>
Matt Baker
Comment 2 2017-10-11 15:37:41 PDT
Created attachment 323472 [details] Patch
Build Bot
Comment 3 2017-10-11 16:42:36 PDT
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
Build Bot
Comment 4 2017-10-11 16:42:38 PDT
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
Matt Baker
Comment 5 2017-10-11 17:55:24 PDT
Created attachment 323498 [details] Patch
Devin Rousso
Comment 6 2017-10-12 14:48:23 PDT
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.
Matt Baker
Comment 7 2017-10-12 15:54:08 PDT
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.
Matt Baker
Comment 8 2017-10-13 18:55:03 PDT
Created attachment 323788 [details] Patch
Matt Baker
Comment 9 2017-10-13 20:23:57 PDT
Created attachment 323793 [details] Patch
Devin Rousso
Comment 10 2017-10-14 11:37:07 PDT
Comment on attachment 323793 [details] Patch r=me
WebKit Commit Bot
Comment 11 2017-10-14 12:05:19 PDT
Comment on attachment 323793 [details] Patch Clearing flags on attachment: 323793 Committed r223322: <https://trac.webkit.org/changeset/223322>
WebKit Commit Bot
Comment 12 2017-10-14 12:05:21 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.