Whenever a visual action is applied (e.g. fill, stroke, etc.), a warning should be shown if that action had no visual change on the canvas. As an example, calling `fill` with an empty path will have no effect.
Created attachment 317524 [details] Patch
Created attachment 317526 [details] [Image] After Patch is applied
Created attachment 317530 [details] [Image] Warnings in sidebar - mockup Warnings in sidebar
(In reply to Devin Rousso from comment #2) > Created attachment 317526 [details] > [Image] After Patch is applied Very cool! How about showing warnings in the sidebar too? Right now it would be difficult to discover this feature. Better yet, an additional filter button to the left of the filter bar to only show those operations with warnings.
(In reply to Matt Baker from comment #4) > (In reply to Devin Rousso from comment #2) > > Created attachment 317526 [details] > > [Image] After Patch is applied > > Very cool! How about showing warnings in the sidebar too? Right now it would > be difficult to discover this feature. Better yet, an additional filter > button to the left of the filter bar to only show those operations with > warnings. So I thought about doing something like that, but the issue is that we currently don't have a way of determining if an action has an effect until after we apply it to the preview canvas. This is because we don't want to apply each action until it's necessary. If we are OK with the overhead of applying every action and getting/comparing ImageData for each visual one, then it's doable. I don't see it being very performant for large recordings though :(
(In reply to Devin Rousso from comment #5) > (In reply to Matt Baker from comment #4) > > (In reply to Devin Rousso from comment #2) > > > Created attachment 317526 [details] > > > [Image] After Patch is applied > > > > Very cool! How about showing warnings in the sidebar too? Right now it would > > be difficult to discover this feature. Better yet, an additional filter > > button to the left of the filter bar to only show those operations with > > warnings. > So I thought about doing something like that, but the issue is that we > currently don't have a way of determining if an action has an effect until > after we apply it to the preview canvas. This is because we don't want to > apply each action until it's necessary. If we are OK with the overhead of > applying every action and getting/comparing ImageData for each visual one, > then it's doable. I don't see it being very performant for large recordings > though :( That makes sense. Showing the icon in the sidebar once the action has been applied might be a bad idea too, since it would imply that the items without an icon had no issues.
Created attachment 317531 [details] Patch
Created attachment 317533 [details] Patch
Comment on attachment 317533 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317533&action=review > Source/WebInspectorUI/ChangeLog:13 > + If the selected action is visual, save a copy of the preview canvas' dataURL before Wording: "dataURL" is not a proper name. HTMLCanvasElement.prototype.toDataURL returns a data URI. Use that, or "data URL" in your comments. > Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:44 > + this._hasEffect = undefined; Maybe a more descriptive name? What about `causesVisibleChange`? > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:232 > + If RecordingAction was responsible for applying itself to the context, you could remove the `hasEffect` setter, and the variable ``. It should simplify the logic here, and should have minimal ripple effect (this._applyAction calls will need to be changed): // RecordingContentView let checkForChange = i === to && actions[I].isVisual; actions[I].apply(snapshot.context, {checkForChange}); if (checkForChange && actions[i].hasEffect) { let warningIcon = this._messageElement.appendChild(useSVGSymbol("Images/Warning.svg", "glyph")); warningIcon.classList.add("icon"); this._messageElement.appendChild(document.createTextNode(WI.UIString("This action causes no visual change"))); } // RecordingAction _apply(context, options = {}) { if (!this.valid) return; if (this.parameters.includes(WI.Recording.Swizzle.Invalid)) return; let contentBefore; let shouldCheckForChange = options.checkForChange && this._hasEffect === undefined; if (shouldCheckForChange) contentBefore = context.canvas.toDataURL(); try { let name = options.nameOverride || this.name; if (this.isFunction) context[name](...this.parameters); else { if (this.isGetter) context[name]; else context[name] = action.parameters[0]; } if (shouldCheckForChange) this._hasEffect = contentBefore !== context.canvas.toDataURL(); } catch (e) { WI.Recording.synthesizeError(WI.UIString("“%s” threw an error.").format(action.name)); action.valid = false; } }
(In reply to Matt Baker from comment #9) > Comment on attachment 317533 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=317533&action=review ... > If RecordingAction was responsible for applying itself to the context, you > could remove the `hasEffect` setter, and the variable ``. It should simplify the variable `alreadyCheckedHasEffect`.
<rdar://problem/34045334>
Comment on attachment 317533 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317533&action=review r- based on Matt's comments, which I think are a nicer direction. > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.css:65 > + margin-right: 4px; This needs RTL treatment.
Created attachment 323803 [details] Patch
Created attachment 323804 [details] [Image] After Patch is applied
Comment on attachment 323803 [details] Patch Attachment 323803 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4855386 New failing tests: inspector/canvas/recording-2d.html
Created attachment 323806 [details] Archive of layout-test-results from ews100 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 323803 [details] Patch Attachment 323803 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4855392 New failing tests: inspector/canvas/recording-2d.html
Created attachment 323807 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 323803 [details] Patch Attachment 323803 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4855398 New failing tests: inspector/canvas/recording-2d.html
Created attachment 323808 [details] Archive of layout-test-results from ews116 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 323830 [details] Patch
Comment on attachment 323830 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323830&action=review r=me > Source/WebInspectorUI/UserInterface/Models/Recording.js:58 > - WI.Recording.synthesizeError(WI.UIString("â%sâ is invalid.").format(action.name)); > + WI.Recording.synthesizeError(WI.UIString("â%sâ is invalid.").format(this._name)); Nit: This is not in the ChangeLog. > Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:144 > + } catch (e) { Style: Could drop the (e) variable since it is not used: `} catch {`
Created attachment 323867 [details] Patch
Comment on attachment 323867 [details] Patch Clearing flags on attachment: 323867 Committed r223335: <https://trac.webkit.org/changeset/223335>
All reviewed patches have been landed. Closing bug.