[webkit-reviews] review granted: [Bug 190473] Web Inspector: Canvas: capture previously saved states and add them to the recording payload : [Attachment 352233] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 190473: Web Inspector: Canvas: capture previously saved states and add them
to the recording payload
https://bugs.webkit.org/show_bug.cgi?id=190473

Attachment 352233: Patch

https://bugs.webkit.org/attachment.cgi?id=352233&action=review




--- Comment #8 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 352233
  --> https://bugs.webkit.org/attachment.cgi?id=352233
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352233&action=review

r=me

> Source/WebCore/inspector/InspectorCanvas.cpp:409
> +String InspectorCanvas::indexForKey(const String& key)

Nit: The name of this looks like it will be returning a number. I'd suggest a
name like `stringIndexForKey`.

> Source/WebInspectorUI/UserInterface/Models/Recording.js:77
> +	   if (!Array.isArray(payload.initialState.states) ||
payload.initialState.states.some((item) => typeof item !== "object" || item ===
null)){

Style: Space before brace in this if statement.

> Source/WebInspectorUI/UserInterface/Models/Recording.js:81
> +	       if (!isEmptyObject(payload.initialState.attributes)) {

Nit: We'd normally just put "iOS 12.0" here not 12.1.

I think the COMPATIBILITY comment go above the above if statement? It seems
like all of this code, including the `...states = []` is handling the case
where states didn't exist.

> Source/WebInspectorUI/UserInterface/Models/Recording.js:465
> +	       // COMPATIBILITY (iOS 12.1): Recording.types.InitialState.states
did not exist yet

Nit: "12.0". Also I'd probably do Recording.InitialState.states or something
like that without the ".types" in the middle. But that's just me, I haven't
seen that terminology before.

> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:268
> +	       if (lastAction) {
> +		   let lastState = lastAction.states.lastValue;
> +		   for (let key in currentState) {
> +		       if (!(key in lastState) || (currentState[key] !==
lastState[key] && !Object.shallowEqual(currentState[key], lastState[key])))
> +			   this._stateModifiers.add(key);
> +		   }
> +	       }

This seems new, it should be explained in the ChangeLog somewhere.

> LayoutTests/inspector/canvas/recording-2d.html:528
> +		  
InspectorTest.expectEqual(recording.initialState.states.length, 4, "There
should be four existing states.");

Nit: We normally don't type out numbers like `four`, unless there is something
specific I'd just use `4` here.