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.
<rdar://problem/34943364>
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.