Bug 175282 - Web Inspector: show warning when recorded Canvas action caused no visual change
Summary: Web Inspector: show warning when recorded Canvas action caused no visual change
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 174484
Blocks: WebInspectorCanvasRecording
  Show dependency treegraph
 
Reported: 2017-08-07 13:16 PDT by Devin Rousso
Modified: 2018-05-15 01:23 PDT (History)
8 users (show)

See Also:


Attachments
Patch (9.38 KB, patch)
2017-08-07 17:39 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (527.58 KB, image/png)
2017-08-07 17:40 PDT, Devin Rousso
no flags Details
[Image] Warnings in sidebar - mockup (137.58 KB, image/png)
2017-08-07 18:43 PDT, Matt Baker
no flags Details
Patch (9.13 KB, patch)
2017-08-07 18:59 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (7.24 KB, patch)
2017-08-07 19:41 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (14.04 KB, patch)
2017-10-14 02:13 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (377.70 KB, image/png)
2017-10-14 02:13 PDT, Devin Rousso
no flags Details
Archive of layout-test-results from ews100 for mac-elcapitan (1.04 MB, application/zip)
2017-10-14 03:18 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.18 MB, application/zip)
2017-10-14 03:25 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-elcapitan (1.85 MB, application/zip)
2017-10-14 03:34 PDT, Build Bot
no flags Details
Patch (13.93 KB, patch)
2017-10-14 16:27 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (14.00 KB, patch)
2017-10-15 20:35 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2017-08-07 13:16:03 PDT
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.
Comment 1 Devin Rousso 2017-08-07 17:39:52 PDT
Created attachment 317524 [details]
Patch
Comment 2 Devin Rousso 2017-08-07 17:40:11 PDT
Created attachment 317526 [details]
[Image] After Patch is applied
Comment 3 Matt Baker 2017-08-07 18:43:46 PDT
Created attachment 317530 [details]
[Image] Warnings in sidebar - mockup

Warnings in sidebar
Comment 4 Matt Baker 2017-08-07 18:44:55 PDT
(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.
Comment 5 Devin Rousso 2017-08-07 18:48:33 PDT
(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 :(
Comment 6 Matt Baker 2017-08-07 18:58:02 PDT
(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.
Comment 7 Devin Rousso 2017-08-07 18:59:19 PDT
Created attachment 317531 [details]
Patch
Comment 8 Devin Rousso 2017-08-07 19:41:52 PDT
Created attachment 317533 [details]
Patch
Comment 9 Matt Baker 2017-08-07 21:24:21 PDT
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;
    }
}
Comment 10 Matt Baker 2017-08-07 21:26:02 PDT
(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`.
Comment 11 Radar WebKit Bug Importer 2017-08-23 15:11:35 PDT
<rdar://problem/34045334>
Comment 12 Brian Burg 2017-10-09 11:38:27 PDT
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.
Comment 13 Devin Rousso 2017-10-14 02:13:30 PDT
Created attachment 323803 [details]
Patch
Comment 14 Devin Rousso 2017-10-14 02:13:47 PDT
Created attachment 323804 [details]
[Image] After Patch is applied
Comment 15 Build Bot 2017-10-14 03:18:43 PDT
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
Comment 16 Build Bot 2017-10-14 03:18:44 PDT
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 17 Build Bot 2017-10-14 03:25:04 PDT
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
Comment 18 Build Bot 2017-10-14 03:25:05 PDT
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 19 Build Bot 2017-10-14 03:34:17 PDT
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
Comment 20 Build Bot 2017-10-14 03:34:19 PDT
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
Comment 21 Devin Rousso 2017-10-14 16:27:36 PDT
Created attachment 323830 [details]
Patch
Comment 22 Joseph Pecoraro 2017-10-15 15:46:45 PDT
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 {`
Comment 23 Devin Rousso 2017-10-15 20:35:23 PDT
Created attachment 323867 [details]
Patch
Comment 24 WebKit Commit Bot 2017-10-15 21:46:24 PDT
Comment on attachment 323867 [details]
Patch

Clearing flags on attachment: 323867

Committed r223335: <https://trac.webkit.org/changeset/223335>
Comment 25 WebKit Commit Bot 2017-10-15 21:46:26 PDT
All reviewed patches have been landed.  Closing bug.