Bug 174484

Created attachment 316653 [details] Patch

Created attachment 316654 [details] [Image] After Patch is applied - Recording Tab

Created attachment 316656 [details] [Image] After Patch is applied - Canvas ContentView

Created attachment 316657 [details] [Image] After Patch is applied - Settings Experimental

Created attachment 316660 [details] Patch

Comment on attachment 316660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316660&action=review > Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:166 > +localizedStrings["Changes will take effect on the next load"] = "Changes will take effect on the next load"; Maybe "next page load"? > Source/WebInspectorUI/UserInterface/Base/Main.js:914 > + if (!inputElement.files.length) > + return; We should call the callback with null in the case where the user dismisses / cancels the dialogs so that clients can respond to that if needed. We could write a test for this: testRunner.setOpenPanelFiles("resources/data.txt"); WebInspector.loadDataFromFile((data) => { InspectorTest.expectEqual(data, "hello world"); ... }); But its fine if you decide not to. We could add a FileUtilities.js to test save/load at a later time given they aren't included by any Test* code right now. > Source/WebInspectorUI/UserInterface/Base/Utilities.js:1140 > + }, Style: drop the trailing comma here where we don't want other properties. > Source/WebInspectorUI/UserInterface/Models/Recording.js:106 > + consoleMessage.shouldRevealConsole = true; // TODO Is there still something to do here? > Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:143 > +WebInspector.RecordingAction._parameterSwizzleTypeForTypeAtIndex = { > + [WebInspector.Recording.Type.Canvas2D]: { We should have a comment somewhere near this that describes this and what its purpose is! This is the structure that informs the frontend about which properties we need to extract their data from the deduplicated "data" section of the recording payload. So this swizzles the parameter of <action name> when called with <parameter length> at index <parameter index> as type <type>. We could have embedded this information into the recording payload, but you decided not to do that (1) to save space, (2) have a static way to extract data from the frontend that isn't dependent on the JSON because anyone can craft any JSON for import. There is a drawback here in that if WebKit change's one of these functions in a later release, then the old backends will be sending data for a function that may need to be swizzled differently from another frontend. We also "get lucky" that there is no ambiguous case where an action operation of a particular length takes 2 different kinds of data. Another drawback is that if WebKit makes additions to the CanvasRenderingContext2D IDL that takes non-trivial parameters someone will have to remember to update this. It would have been neat to detect this at build time (perhaps generate this at build time from the CanvasRenderingContext2d.idl given it has CallTracingEnabled). This sounds like something we can do in the future. --- How does this handle optional parameters? If there is IDL for an operation with multiple optional parameters without default values: void operation(DOMString a, optional DOMString b, optional DOMString b); What happens if this is called with some of the optional parameters but not all: ctx.operation("a", "b") Will the payload for this unwrap to: name: "operation" parameters: ["a", "b"] Or will it unwrap to something like this (full # of parameters): name: "operation" parameters: ["a", "b", ""] I'm guessing it is the last case but I look too deeply into this. > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:181 > + if (showPath && i >= indexStartVisualizingPath && actionAffectsPath(actions[i])) { > + if (i === indexStartVisualizingPath) > + this._pathContext.setTransform(snapshot.context.getTransform()); I know you said you were going to eliminate a lot of code by exposing a ctx.getPath() to inspector. Before I spend too much time reading through this is the code that will be eliminated in this patch? If so can you point it out so I don't spend too much time on it. > Source/WebInspectorUI/UserInterface/Views/RecordingStateDetailsSidebarPanel.js:125 > + webkitImageSmoothingEnabled: context.webkitImageSmoothingEnabled, > + webkitLineDash: context.webkitLineDash, > + webkitLineDashOffset: context.webkitLineDashOffset, Maybe we should dim / hide these to suggest developers not use them in favor of the standard values? > LayoutTests/inspector/unit-tests/number-utilities-expected.txt:67 > +PASS: 0 should have 1 digit You test positive and negative for everything you might as well for zero as well. Note that -0 is sometimes significant: >>> 1 / 0 < 0 false >>> 1 / -0 < 0 true

Comment on attachment 316660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316660&action=review >> LayoutTests/inspector/unit-tests/number-utilities-expected.txt:67 >> +PASS: 0 should have 1 digit > > You test positive and negative for everything you might as well for zero as well. Note that -0 is sometimes significant: > > >>> 1 / 0 < 0 > false > > >>> 1 / -0 < 0 > true To this end, we show -0 in Web Inspector's console / ObjectTrees (RemoteObject). However `JSON.stringify(-0) === 0` so if the Canvas State has a -0 we probably don't show it because that 0 came through the recording logic. I think thats fine, but something to be aware of.

Comment on attachment 316660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316660&action=review >> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:166 >> +localizedStrings["Changes will take effect on the next load"] = "Changes will take effect on the next load"; > > Maybe "next page load"? I forgot to update LocalizedStrings. This no longer exists. >> Source/WebInspectorUI/UserInterface/Base/Main.js:914 >> + return; > > We should call the callback with null in the case where the user dismisses / cancels the dialogs so that clients can respond to that if needed. > > We could write a test for this: > > testRunner.setOpenPanelFiles("resources/data.txt"); > WebInspector.loadDataFromFile((data) => { > InspectorTest.expectEqual(data, "hello world"); > ... > }); > > But its fine if you decide not to. We could add a FileUtilities.js to test save/load at a later time given they aren't included by any Test* code right now. I'd like to do this, but I get an error that `setOpenPanelFiles` isn't a function on `testRunner`. Do I need to include something, or should I not be doing this inside LayoutTests/Inspector/ ? >> Source/WebInspectorUI/UserInterface/Models/Recording.js:106 >> + consoleMessage.shouldRevealConsole = true; // TODO > > Is there still something to do here? This doesn't really follow the pattern set by ConsoleMessage, as subclasses create a getter for `shouldRevealConsole`. If this is fine, we can leave it, but I think my intention was to add this setter/getter to ConsoleMessage. >> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:143 >> + [WebInspector.Recording.Type.Canvas2D]: { > > We should have a comment somewhere near this that describes this and what its purpose is! > > This is the structure that informs the frontend about which properties we need to extract their data from the deduplicated "data" section of the recording payload. So this swizzles the parameter of <action name> when called with <parameter length> at index <parameter index> as type <type>. We could have embedded this information into the recording payload, but you decided not to do that (1) to save space, (2) have a static way to extract data from the frontend that isn't dependent on the JSON because anyone can craft any JSON for import. > > There is a drawback here in that if WebKit change's one of these functions in a later release, then the old backends will be sending data for a function that may need to be swizzled differently from another frontend. We also "get lucky" that there is no ambiguous case where an action operation of a particular length takes 2 different kinds of data. > > Another drawback is that if WebKit makes additions to the CanvasRenderingContext2D IDL that takes non-trivial parameters someone will have to remember to update this. It would have been neat to detect this at build time (perhaps generate this at build time from the CanvasRenderingContext2d.idl given it has CallTracingEnabled). This sounds like something we can do in the future. > > --- > > How does this handle optional parameters? > > If there is IDL for an operation with multiple optional parameters without default values: > > void operation(DOMString a, optional DOMString b, optional DOMString b); > > What happens if this is called with some of the optional parameters but not all: > > ctx.operation("a", "b") > > Will the payload for this unwrap to: > > name: "operation" > parameters: ["a", "b"] > > Or will it unwrap to something like this (full # of parameters): > > name: "operation" > parameters: ["a", "b", ""] > > I'm guessing it is the last case but I look too deeply into this. It depends on whether the optional parameter has a default value. In the case that it doesn't (setShadow), there will not be an item in the resulting parameters list: name: "setShadow" parameters: [0, 1, 2, ""] // no value for `alpha`, but `color` has a default since its a DOMString (which is "") If an optional parameter that has no default value is not provided, there will be no value for it in the parameters list. >> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:181 >> + this._pathContext.setTransform(snapshot.context.getTransform()); > > I know you said you were going to eliminate a lot of code by exposing a ctx.getPath() to inspector. Before I spend too much time reading through this is the code that will be eliminated in this patch? If so can you point it out so I don't spend too much time on it. I've decided to eliminate the path code altogether and just add it in a single patch later. Thus, this code has been removed. >> Source/WebInspectorUI/UserInterface/Views/RecordingStateDetailsSidebarPanel.js:125 >> + webkitLineDashOffset: context.webkitLineDashOffset, > > Maybe we should dim / hide these to suggest developers not use them in favor of the standard values? I'm in favor of keeping them visible, but having them be dimmed.

Created attachment 316702 [details] Patch

Comment on attachment 316660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316660&action=review >>> Source/WebInspectorUI/UserInterface/Base/Main.js:914 >>> + return; >> >> We should call the callback with null in the case where the user dismisses / cancels the dialogs so that clients can respond to that if needed. >> >> We could write a test for this: >> >> testRunner.setOpenPanelFiles("resources/data.txt"); >> WebInspector.loadDataFromFile((data) => { >> InspectorTest.expectEqual(data, "hello world"); >> ... >> }); >> >> But its fine if you decide not to. We could add a FileUtilities.js to test save/load at a later time given they aren't included by any Test* code right now. > > I'd like to do this, but I get an error that `setOpenPanelFiles` isn't a function on `testRunner`. Do I need to include something, or should I not be doing this inside LayoutTests/Inspector/ ? Shouldn't need to include anything. This should work in both WebKit1 and WebKit2 test runners. >> How does this handle optional parameters? >> >> [...] > > It depends on whether the optional parameter has a default value. In the case that it doesn't (setShadow), there will not be an item in the resulting parameters list: > > name: "setShadow" > parameters: [0, 1, 2, ""] // no value for `alpha`, but `color` has a default since its a DOMString (which is "") > > If an optional parameter that has no default value is not provided, there will be no value for it in the parameters list. Does this then break your swizzling approach since the parameter array list will be shorter than anticipated?

Comment on attachment 316702 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316702&action=review Looks good to me. Because of the list of comments I'd like to see another version / answers to some of my questions but I still think this is good enough to land. > Source/WebInspectorUI/UserInterface/Base/FileUtilities.js:26 > +WebInspector.loadDataFromFile = function(callback) I expected you'd put save in here as well. > Source/WebInspectorUI/UserInterface/Images/Recording.svg:1 > +<?xml version="1.0" encoding="utf-8"?> File a bug on Images/gtk for an image here. > Source/WebInspectorUI/UserInterface/Main.html:167 > + <link rel="stylesheet" href="Views/ScrubberNavigationItem.css"> Style: alphabetical > Source/WebInspectorUI/UserInterface/Main.html:684 > + <script src="Views/ScrubberNavigationItem.js"></script> Style: alphabetical > Source/WebInspectorUI/UserInterface/Models/Recording.js:133 > + if (Array.isArray(data)) Should we throw an error if this type check fails? > Source/WebInspectorUI/UserInterface/Models/Recording.js:138 > + let context = document.createElement("canvas").getContext("2d"); Maybe we can keep 1 scratch canvas around instead of creating a new one every time we swizzle a gradient / pattern? We could destroy it after swizzling. This seems kinda wasteful, but maybe its not a problem. > Source/WebInspectorUI/UserInterface/Models/Recording.js:169 > + if (typeof data === "string") Should we throw an error if this type check fails? > Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:59 > + let forType = WebInspector.RecordingAction._functionNames[type]; > + if (!forType) The `forType` name is poor. Lets go with `functionNames`. > Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:68 > get name() { return this._resolvedName; } > get parameters() { return this._resolvedParameters; } Style: These gutters are non-trivial since they return something not matching their name. Make them non-inlined. Alternatively you could flip `name` / `resolvedName` to `payloadName` and `name` or something like that. > Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:88 > + let forType = WebInspector.RecordingAction._parameterSwizzleTypeForTypeAtIndex[type]; Style: There must be better variable names in here. > Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:122 > + functionForType(type) > + { > + return WebInspector.RecordingAction.functionForType(type, this._resolvedName); > + } > + > + getterForType(type) > + { > + return !this.functionForType(type) && !this._resolvedParameters.length; > + } > + > + visualForType(type) > + { > + let forType = WebInspector.RecordingAction._visualNames[type]; > + if (!forType) > + return false; > + > + return forType.includes(this._resolvedName); > + } > + > + initialStateModifiersForType(type) These are very confusing. They are all returning booleans but their names appear to get something. Secondly these all take a `type` but presumably that doesn't make sense to change. --- How about each of these states gets initialized in swizzle (which I think of as "process") and then they are just accessors: get isFunction get isGetter get visual get stateModifiers > Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:146 > +// make sure that inputs are only considered valid if they conform to the structure defined below. It would be instructive to add a complete example: // For Example: // // IDL: // // void fill(optional CanvasWindingRule winding = "nonzero"); // void fill(DOMPath path, optional CanvasWindingRule winding = "nonzero"); // void fill(float a, float b, float c, float d); // // Swizzle Entries: // // - For the 1 parameter version parameter at index 0 needs to be swizzled as a string // - For the 2 parameter version the parameters need to be swizzled as a Path and String // - For the fake 4 parameter version, numbers don't need to be swizzled, so it is not included // // "fill": { // 1: {0: String} // 2: {0: Path2D, 1: String} // } Something like this goes a much longer way than the opaque paragraph you have. > Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:153 > +WebInspector.RecordingAction._parameterSwizzleTypeForTypeAtIndex = { > + [WebInspector.Recording.Type.Canvas2D]: { > + "clip": { > + 1: {0: WebInspector.Recording.Swizzle.String}, > + 2: {0: WebInspector.Recording.Swizzle.Path2D, 1: WebInspector.Recording.Swizzle.String}, > + }, Since this is code that is being evaluated, you can simplify it a bit if you think it would enhance readability: { let {Array, CanvasStyle, Element, Image, ImageData, Path2D, String, Invalid} = WebInspector.Recording.Swizzle; WebInspector.RecordingAction._parameterSwizzleTypeForTypeAtIndex = { "clip" { 1: {0: String}, 2: {0, Path2D, 1: String}, } ... }; } > Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:261 > +WebInspector.RecordingAction._functionNames = { Style: Wrap or one-per-line. > Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:104 > + if (error) > + return; > + If we don't ever expect an error we can just do: console.assert(!error); > Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:44 > + classNames.push(typeof representedObject.parameters[0]); This is a weird way to infer the type. It seems to work heuristically well here but maybe not always. > Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:83 > + if (representedObject.parameters[i] instanceof CanvasGradient) > + parameterElement.textContent = WebInspector.unlocalizedString("CanvasGradient"); > + else if (representedObject.parameters[i] instanceof CanvasPattern) > + parameterElement.textContent = WebInspector.unlocalizedString("CanvasPattern"); How about just Gradient / Pattern? I don't know if the explicit type adds much value here. > Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:90 > + if (!isFunction) > + break; Is there a non-function version that has more than 1 parameter? > Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:102 > + super(classNames, titleFragment, subtitle, representedObject); This must be the latest call to super I've ever seen. This leave a lot of room for mistakes (any use of `this` above would result in an exception). It sometimes helps to move code above into a static method so it is both clear it is not using `this` and the work before `super()` is visually small. > Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:141 > + contextMenu.appendItem(WebInspector.UIString("Copy Action"), () => { > + InspectorFrontendHost.copyText("context." + this._mainTitleElement.textContent + ";"); > + }); Seems fine to keep, but I'm not sure I would have expected this. > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:100 > + Style: Extra newline > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:110 > + let snapshotIndex = Math.floor(index / WebInspector.RecordingContentView._snapshotInterval); Typically we just capitalize constants like this: WebInspector.RecordingContentView.SnapshotInterval = ...; > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:125 > + if (!(name in snapshot.context)) > + continue; What case does this help with, or is it just a precaution? > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:160 > + while (snapshot.index && actions[snapshot.index].name !== "beginPath") > + --snapshot.index; In the future this will not be required once you can get and apply an entire path? > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:169 > + let lastSnapshot = snapshotIndex; Style: `lastSnapshot` => `lastSnapshotIndex`. > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:234 > + for (let snapshot of this._snapshots) { > + if (snapshot) > + snapshot.element.remove(); > + } Maybe just `this._previewContainer.removeChildren()`? > Source/WebInspectorUI/UserInterface/Views/RecordingNavigationSidebarPanel.css:93 > +.sidebar > .panel.navigation.recording > .content .action.attribute.getter > .icon { > + content: url(../Images/Eye.svg); > +} I still think this UI will be weird but we can tweak later. > Source/WebInspectorUI/UserInterface/Views/RecordingNavigationSidebarPanel.js:58 > + this._recording = recording; Style: Add a newline after this. > Source/WebInspectorUI/UserInterface/Views/RecordingTabContentView.js:161 > + WebInspector.Recording.synthesizeError(WebInspector.UIString("unsupported version")); What does this look like? I'd expect a sentence like "Unsupported version." or better yet "Invalid file format." > Source/WebInspectorUI/UserInterface/Views/ScrubberNavigationItem.js:48 > + set disabled(flag) you might as well include a getter for this. > Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:233 > + if (!window.CanvasAgent) > + return; This would prevent the experimental stylesheet editor down below for older versions of iOS that don't support canvas. This should not be an early return. > LayoutTests/inspector/unit-tests/number-utilities.html:125 > + InspectorTest.expectEqual(Number.countDigits(-1 * num), digits, `${-1 * num} should have ${digits} digits`); `-1 * num` => `-num`. You can use the unary - instead of multiplication!

Created attachment 316893 [details] Patch

Comment on attachment 316702 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316702&action=review >> Source/WebInspectorUI/UserInterface/Images/Recording.svg:1 >> +<?xml version="1.0" encoding="utf-8"?> > > File a bug on Images/gtk for an image here. <https://webkit.org/b/175045> [GTK] Web Inspector: Add Recording.svg >> Source/WebInspectorUI/UserInterface/Models/Recording.js:133 >> + if (Array.isArray(data)) > > Should we throw an error if this type check fails? I'm trying to avoid throwing as much as possible. In the case that this fails, the type is set to Invalid in the if statement right below the catch. >> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:90 >> + break; > > Is there a non-function version that has more than 1 parameter? No, this was more as a sanity check to make sure it wouldn't generate weird UI. >> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:141 >> + }); > > Seems fine to keep, but I'm not sure I would have expected this. I've added another member variable called `_copyText` that contains the full synthesized text of the action. This way, long decimal values are truncated to 2 digits in the UI, but the full value can still be copied. >> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:125 >> + continue; > > What case does this help with, or is it just a precaution? This is a precaution. Actions are already checked when they are created (via Recording's constructor) and will set `valid` to false. InitialState, however, doesn't get checked, so we need to make sure that the user didn't do something weird before changing the context. >> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:160 >> + --snapshot.index; > > In the future this will not be required once you can get and apply an entire path? That's the plan =D >> Source/WebInspectorUI/UserInterface/Views/RecordingNavigationSidebarPanel.css:93 >> +} > > I still think this UI will be weird but we can tweak later. I agree, but I'm not sure how else to go about doing it. There isn't really a good icon for it :( >> Source/WebInspectorUI/UserInterface/Views/RecordingTabContentView.js:161 >> + WebInspector.Recording.synthesizeError(WebInspector.UIString("unsupported version")); > > What does this look like? I'd expect a sentence like "Unsupported version." or better yet "Invalid file format." It would look like this: Recording error: unsupported version I can add a period to the end. I don't think "Invalid file format" is entirely correct here, as the file must be JSON to reach this point (this event handler isn't fired if JSON.parse fails). The only way for `recording` to be falsy is for it to not have a supported version. >> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:233 >> + return; > > This would prevent the experimental stylesheet editor down below for older versions of iOS that don't support canvas. This should not be an early return. Oops.

Created attachment 316894 [details] [Image] After Patch is applied - Recording Tab

Comment on attachment 316893 [details] Patch Clearing flags on attachment: 316893 Committed r220114: <http://trac.webkit.org/changeset/220114>

All reviewed patches have been landed. Closing bug.

<rdar://problem/33664694>