Bug 190473

Summary: Web Inspector: Canvas: capture previously saved states and add them to the recording payload
Product: WebKit Reporter: Devin Rousso <drousso>
Component: Web InspectorAssignee: Devin Rousso <drousso>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, esprehn+autocc, ews-watchlist, gyuyoung.kim, inspector-bugzilla-changes, joepeck, keith_miller, mark.lam, msaboff, rniwa, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 173807, 175283    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews103 for mac-sierra
none
Archive of layout-test-results from ews106 for mac-sierra-wk2
none
Patch
none
Patch
none
Patch none

Description Devin Rousso 2018-10-11 09:03:22 PDT
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);
Comment 1 Devin Rousso 2018-10-12 15:04:17 PDT
Created attachment 352212 [details]
Patch
Comment 2 EWS Watchlist 2018-10-12 15:10:38 PDT Comment hidden (obsolete)
Comment 3 EWS Watchlist 2018-10-12 16:08:16 PDT Comment hidden (obsolete)
Comment 4 EWS Watchlist 2018-10-12 16:08:18 PDT Comment hidden (obsolete)
Comment 5 EWS Watchlist 2018-10-12 16:14:19 PDT Comment hidden (obsolete)
Comment 6 EWS Watchlist 2018-10-12 16:14:20 PDT Comment hidden (obsolete)
Comment 7 Devin Rousso 2018-10-12 16:27:27 PDT
Created attachment 352233 [details]
Patch

Fix model object test
Comment 8 Joseph Pecoraro 2018-10-15 19:54:23 PDT
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.
Comment 9 Devin Rousso 2018-10-16 09:59:29 PDT
Created attachment 352466 [details]
Patch
Comment 10 Devin Rousso 2018-10-16 11:45:58 PDT
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.
Comment 11 Devin Rousso 2018-10-16 11:46:51 PDT
Created attachment 352480 [details]
Patch
Comment 12 WebKit Commit Bot 2018-10-16 12:23:30 PDT
Comment on attachment 352480 [details]
Patch

Clearing flags on attachment: 352480

Committed r237198: <https://trac.webkit.org/changeset/237198>
Comment 13 WebKit Commit Bot 2018-10-16 12:23:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2018-10-16 12:24:34 PDT
<rdar://problem/45314325>