Bug 190473

Created attachment 352212 [details] Patch

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>