Bug 178188 – Web Inspector: Canvas tab: recordings should have a unique name

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

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.

[Radar WebKit Bug Importer]

<rdar://problem/34943364>

[Matt Baker]

Created attachment 323472 [details]
Patch

[Build Bot]

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]

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]

Created attachment 323498 [details]
Patch

[Devin Rousso]

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 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]

Created attachment 323788 [details]
Patch

[Matt Baker]

Created attachment 323793 [details]
Patch

[Devin Rousso]

Comment on attachment 323793 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 323793 [details]
Patch

Clearing flags on attachment: 323793

Committed r223322: <https://trac.webkit.org/changeset/223322>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.