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);
Created attachment 352212 [details] Patch
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.
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
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
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
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
Created attachment 352233 [details] Patch Fix model object test
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.
Created attachment 352466 [details] Patch
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.
Created attachment 352480 [details] Patch
Comment on attachment 352480 [details] Patch Clearing flags on attachment: 352480 Committed r237198: <https://trac.webkit.org/changeset/237198>
All reviewed patches have been landed. Closing bug.
<rdar://problem/45314325>