Bug 175282 – Web Inspector: show warning when recorded Canvas action caused no visual change

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 317524 [details]
Patch

[Devin Rousso]

Created attachment 317526 [details]
[Image] After Patch is applied

[Matt Baker]

Created attachment 317530 [details]
[Image] Warnings in sidebar - mockup

Warnings in sidebar

[Matt Baker]

(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.

[Devin Rousso]

(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 :(

[Matt Baker]

(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.

[Devin Rousso]

Created attachment 317531 [details]
Patch

[Devin Rousso]

Created attachment 317533 [details]
Patch

[Matt Baker]

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;
    }
}

[Matt Baker]

(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`.

[Radar WebKit Bug Importer]

<rdar://problem/34045334>

[Blaze Burg]

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.

[Devin Rousso]

Created attachment 323803 [details]
Patch

[Devin Rousso]

Created attachment 323804 [details]
[Image] After Patch is applied

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Devin Rousso]

Created attachment 323830 [details]
Patch

[Joseph Pecoraro]

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 {`

[Devin Rousso]

Created attachment 323867 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 323867 [details]
Patch

Clearing flags on attachment: 323867

Committed r223335: <https://trac.webkit.org/changeset/223335>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.