Bug 190473 – Web Inspector: Canvas: capture previously saved states and add them to the recording payload

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

[Devin Rousso]

We should be able to support recording in between `save()` and `restore()` properly.

context.fillStyle = "red";
context.save();
console.record(context);
context.fillStyle = "blue";
context.restore();
context.fillRect(0, 0, 10, 10);
console.recordEnd(context);

[Devin Rousso]

Created attachment 352212 [details]
Patch

[EWS Watchlist]

Attachment 352212 [details] did not pass style-queue:


ERROR: Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2066:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 352212 [details]
Patch

Attachment 352212 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9557712

New failing tests:
inspector/model/recording.html

[EWS Watchlist]

Created attachment 352228 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 352212 [details]
Patch

Attachment 352212 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9557683

New failing tests:
inspector/model/recording.html

[EWS Watchlist]

Created attachment 352230 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Devin Rousso]

Created attachment 352233 [details]
Patch

Fix model object test

[Joseph Pecoraro]

Comment on attachment 352233 [details]
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.

[Devin Rousso]

Created attachment 352466 [details]
Patch

[Devin Rousso]

Comment on attachment 352233 [details]
Patch

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

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

This code does handle the case that it doesn't exist, but the compatibility part only involves using `attributes` as a state.  The only reason we do these checks is for the case where the user imports a recording, since it's arbitrary JSON and could theoretically be in any format.

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

I think it's a bit clearer, since it directly follows the path in the protocol, but I don't think we've used it before.

[Devin Rousso]

Created attachment 352480 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 352480 [details]
Patch

Clearing flags on attachment: 352480

Committed r237198: <https://trac.webkit.org/changeset/237198>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45314325>