Bug 174967 - Web Inspector: Preview Canvas path when viewing a recording
Summary: Web Inspector: Preview Canvas path when viewing a recording
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-07-28 22:14 PDT by Devin Rousso
Modified: 2017-08-07 16:56 PDT (History)
15 users (show)

See Also:


Attachments
Patch (86.36 KB, patch)
2017-08-02 00:11 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (87.88 KB, patch)
2017-08-02 15:27 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (711.60 KB, image/png)
2017-08-02 15:27 PDT, Devin Rousso
no flags Details
Archive of layout-test-results from ews102 for mac-elcapitan (1.17 MB, application/zip)
2017-08-02 16:30 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (1.34 MB, application/zip)
2017-08-02 17:03 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (1.27 MB, application/zip)
2017-08-02 18:02 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-elcapitan (2.08 MB, application/zip)
2017-08-02 18:07 PDT, Build Bot
no flags Details
Patch (88.05 KB, patch)
2017-08-02 20:48 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (137.74 KB, patch)
2017-08-03 23:16 PDT, Devin Rousso
bburg: review+
Details | Formatted Diff | Diff
Patch (138.33 KB, patch)
2017-08-04 17:47 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (137.72 KB, patch)
2017-08-07 12:24 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (137.72 KB, patch)
2017-08-07 14:55 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (137.72 KB, patch)
2017-08-07 15: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-07-28 22:14:25 PDT
Similar to "Show Grid", a "Show Path" navigation item should be added to enable/disable the rendering of the current path for the recorded canvas.
Comment 1 Devin Rousso 2017-08-02 00:11:35 PDT
Created attachment 316939 [details]
Patch
Comment 2 Devin Rousso 2017-08-02 00:12:02 PDT
Comment on attachment 316939 [details]
Patch

Oops.  Wrong bug.
Comment 3 Devin Rousso 2017-08-02 15:27:37 PDT
Created attachment 317013 [details]
Patch
Comment 4 Devin Rousso 2017-08-02 15:27:48 PDT
Created attachment 317014 [details]
[Image] After Patch is applied
Comment 5 Matt Baker 2017-08-02 15:45:14 PDT
(In reply to Devin Rousso from comment #4)
> Created attachment 317014 [details]
> [Image] After Patch is applied

Cool! I like the dashed lines and solid points. What does red indicate?
Comment 6 Devin Rousso 2017-08-02 16:16:15 PDT
(In reply to Matt Baker from comment #5)
> (In reply to Devin Rousso from comment #4)
> > Created attachment 317014 [details]
> > [Image] After Patch is applied
> 
> Cool! I like the dashed lines and solid points. What does red indicate?
Red is used for modifications made to the path by the current action.  In the screenshot, `moveTo` is selected, so the dashed line corresponding to that is drawn in red.
Comment 7 Build Bot 2017-08-02 16:30:27 PDT
Comment on attachment 317013 [details]
Patch

Attachment 317013 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4242419

New failing tests:
fast/canvas/2d.setPath.html
Comment 8 Build Bot 2017-08-02 16:30:29 PDT
Created attachment 317028 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 9 Build Bot 2017-08-02 17:03:15 PDT
Comment on attachment 317013 [details]
Patch

Attachment 317013 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4242572

New failing tests:
fast/canvas/2d.setPath.html
Comment 10 Build Bot 2017-08-02 17:03:17 PDT
Created attachment 317034 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 11 Build Bot 2017-08-02 18:02:11 PDT
Comment on attachment 317013 [details]
Patch

Attachment 317013 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4242817

New failing tests:
fast/canvas/2d.setPath.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-template-element/template-element/template-descendant-head.html
Comment 12 Build Bot 2017-08-02 18:02:12 PDT
Created attachment 317049 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 13 Build Bot 2017-08-02 18:07:33 PDT
Comment on attachment 317013 [details]
Patch

Attachment 317013 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4242889

New failing tests:
fast/canvas/2d.setPath.html
fast/dom/Window/HTMLBodyElement-window-eventListener-attributes.html
Comment 14 Build Bot 2017-08-02 18:07:35 PDT
Created attachment 317052 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 15 Devin Rousso 2017-08-02 20:48:01 PDT
Created attachment 317084 [details]
Patch
Comment 16 Joseph Pecoraro 2017-08-03 19:37:04 PDT
Comment on attachment 317084 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317084&action=review

This looks good to me. We should get some nod of approval from Rendering folks (like smfr).

I think adding an InspectorAdditions RuntimeEnabledFeature to get a few inspector specific Web APIs is a great idea though. We might want to use that in more places.=!

> Source/WebCore/ChangeLog:29
> +        Add runtime flag for added IDL items above so that they are only usable within the inspector
> +        page. The runtime flag is not enabled from anywhere else as of now.

"within the inspector page" => "within the inspector process". This is significant because RuntimeEnabledFeatures is a shared singleton per-process.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:194
> +    [EnabledAtRuntime=InspectorAdditions] void setPath(DOMPath path);
> +    [EnabledAtRuntime=InspectorAdditions, NewObject] DOMPath getPath();

I believe you were going to propose this in standards. Could you add a link to that in the bugzilla bug?

> Source/WebInspectorUI/UserInterface/Base/Setting.js:124
> +    showCanvasPath: new WI.Setting("show-canvas-path", false),

Should we make this on by default?

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:43
> +            this._showPathButtonNavigationItem = new WI.ActivateButtonNavigationItem("show-path", WI.UIString("Show Path"), WI.UIString("Hide Path"), "Images/PaintFlashing.svg", 13, 13);

I think we should only include this button if we have the InspectorExtras support? This would help (1) those using Web Inspector frontend outside of WebKit (2) WebKitLegacy inspector where this is won't work:

    static supportsPathDebugging()
    {
        return !!CanvasRenderingContext2D.prototype.setPath;
    }

I think we should have a different icon for this. File a follow-up (and maybe message Jon Davis). Maybe just a few lines /\/ with one being dashed?

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:121
> +        function actionAffectsPath(action) {
> +            const pathActionNames = [
> +                "arc",
> +                "arcTo",
> +                "beginPath",
> +                "bezierCurveTo",
> +                "closePath",
> +                "ellipse",
> +                "lineTo",
> +                "moveTo",
> +                "quadraticCurveTo",
> +                "rect",
> +                "resetTransform",
> +                "rotate",
> +                "scale",
> +                "setTransform",
> +                "transform",
> +                "translate",
> +            ];
> +            return pathActionNames.includes(action.name);
> +        }

Change this to be a switch statement / Set. You can probably make this a member function as well instead of an inline function.

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:187
> +            let lastPath = [];
> +            let subPathStart = [];

These name really threw me. This is a point [x, y]. Maybe call them: `lastPathXY`, `lastPathPoint`, `lastPathPosition`. Or even just splitting it up `lastPathX` and `lastPathY` which will eliminate the confusing `...lastPath` below.

Likewise `subPathStart` is poorly named. I don't have a good name, but there must be a clearer one. Maybe `expectedClosePositionX` and `expectedClosePositionY`.

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:271
>                      if (key === "strokeStyle" || key === "fillStyle")
>                          value = this.representedObject.swizzle(value, WI.Recording.Swizzle.CanvasStyle);
> +                    else if (key === "globalCompositeOperation"
> +                        || key === "lineCap"
> +                        || key === "lineJoin"

You could make all of this a switch I think it would read better:

    switch (key) {
    case "strokeStyle":
    case "fillStyle":
        value = swizzle(CanvasStyle);
        break;
    case "..."
        value = swizzle(String);
        break;
    ...
    }

> Source/WebKit/Shared/WebPreferencesDefinitions.h:295
> +    macro(InspectorAdditionsEnabled, inspectorAdditionsEnabled, Bool, bool, false, "Inspector Additions", "Additional features enabled for WebInspector") \

This could use a better description. Perhaps: "Enable additional page APIs used by the Web Inspector frontend page".

> Source/WebKitLegacy/mac/WebView/WebPreferencesPrivate.h:586
> +@property (nonatomic) BOOL inspectorAdditionsEnabled;

The order of these is all over the place. You should be consistent. If you put this after allowMediaContentTypesrequiringHardwareSupport it should be before that in all 4 places above. Currently it is above, below, or nowhere near.

> LayoutTests/fast/canvas/2d.getPath.modification.html:41
> +

Style: extra newline.

> LayoutTests/fast/canvas/2d.getPath.newobject.html:22
> +

Style: extra newline.

> LayoutTests/inspector/canvas/recording-2d.html:41
> +    ctx.save(); // This will be matched with the `restore` call below.

(Devin says he will fix this)
Comment 17 Joseph Pecoraro 2017-08-03 19:42:29 PDT
Dean / Simon, what are your thoughts on Web Inspector folks using a RuntimeEnabledFeature only for our InspectorProcess to enable some extra Web APIs to get some extra functionality? Here we add a few on CanvasRenderingContext2D that only the Web Inspector page will be able to use. I think its a great idea, especially for APIs like this which we may want to standardize and we can easily write tests for.
Comment 18 Simon Fraser (smfr) 2017-08-03 19:57:51 PDT
I don't think REF are the right way to toggle behavior for a web-facing API like this. I think you want some native code that just causes the new functionality to be exposed.
Comment 19 Joseph Pecoraro 2017-08-03 20:18:18 PDT
(In reply to Simon Fraser (smfr) from comment #18)
> I don't think REF are the right way to toggle behavior for a web-facing API
> like this. I think you want some native code that just causes the new
> functionality to be exposed.

We aren't toggling behavior, we are toggling the existence of extra APIs. In this case CanvasRenderingContext2d.prototype.getPath/setPath.

Is there a way to do this without REF? I suggested REF because it already exists and works as we would expect (expose APIs based on the setting).

Alternatively we could create a InspectorFrontendHost.getCanvasPathFromCanvasContext(ctx) like function, but that seems much poorer for readability and maintainability given how far removed it is from the actual JavaScript object's API.
Comment 20 Devin Rousso 2017-08-03 22:26:19 PDT
Comment on attachment 317084 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317084&action=review

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:194
>> +    [EnabledAtRuntime=InspectorAdditions, NewObject] DOMPath getPath();
> 
> I believe you were going to propose this in standards. Could you add a link to that in the bugzilla bug?

I'll file a bug tomorrow.  =D

>> Source/WebInspectorUI/UserInterface/Base/Setting.js:124
>> +    showCanvasPath: new WI.Setting("show-canvas-path", false),
> 
> Should we make this on by default?

I don't think we should.  It might be weird for people to see a white overlay on their canvas by default, as it wouldn't match what's visible on the page.

>> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:43
>> +            this._showPathButtonNavigationItem = new WI.ActivateButtonNavigationItem("show-path", WI.UIString("Show Path"), WI.UIString("Hide Path"), "Images/PaintFlashing.svg", 13, 13);
> 
> I think we should only include this button if we have the InspectorExtras support? This would help (1) those using Web Inspector frontend outside of WebKit (2) WebKitLegacy inspector where this is won't work:
> 
>     static supportsPathDebugging()
>     {
>         return !!CanvasRenderingContext2D.prototype.setPath;
>     }
> 
> I think we should have a different icon for this. File a follow-up (and maybe message Jon Davis). Maybe just a few lines /\/ with one being dashed?

👍

>> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:187
>> +            let subPathStart = [];
> 
> These name really threw me. This is a point [x, y]. Maybe call them: `lastPathXY`, `lastPathPoint`, `lastPathPosition`. Or even just splitting it up `lastPathX` and `lastPathY` which will eliminate the confusing `...lastPath` below.
> 
> Likewise `subPathStart` is poorly named. I don't have a good name, but there must be a clearer one. Maybe `expectedClosePositionX` and `expectedClosePositionY`.

I actually think that subPathStart is an accurate name.  closePath is supposed to add a lineTo the start of the current sub-path.

https://html.spec.whatwg.org/multipage/canvas.html#dom-context-2d-closepath
Comment 21 Devin Rousso 2017-08-03 23:16:14 PDT
Created attachment 317215 [details]
Patch
Comment 22 Brian Burg 2017-08-04 09:55:03 PDT
Comment on attachment 317215 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317215&action=review

r=me but let Joe and a rendering person take a final glance.

> Source/WebCore/page/RuntimeEnabledFeatures.h:213
> +    void setInspectorAdditionsEnabled(bool isEnabled) { m_inspectorAdditionsEnabled = isEnabled; }

You'll need to rebase this as I just removed ENABLE(WEB_SOCKETS) above.

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:134
> +        let indexStartVisualizingPath = Infinity;

Nit: weird variable name

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:160
> +            let drawCanvasPath = showCanvasPath && indexStartVisualizingPath <= to;

Nit: shouldDrawCanvasPath

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:350
> +    _actionModifiesPath(recordingAction)

This could be static.

> Source/WebKit/Shared/WebPreferencesDefinitions.h:294
> +    macro(InspectorAdditionsEnabled, inspectorAdditionsEnabled, Bool, bool, false, "Inspector Additions", "Enable additional page APIs used by the Web Inspector frontend page") \

Nit: "Web Inspector Additions"

> Source/WebKit/UIProcess/API/C/WKPreferencesRefPrivate.h:514
> +WK_EXPORT bool WKPreferencesGetInspectorAdditionsEnabled(WKPreferencesRef);

I don't think there is any point exposing this through the WK2 C API. We don't expect it to be used this way, as the RuntimeEnabledFeature is turned on using the internal methods rather than public SPI/API.


EDIT: ah I guess this is needed for WKTR? Gross.

> Source/WebKitLegacy/mac/WebView/WebPreferencesPrivate.h:586
> +@property (nonatomic) BOOL inspectorAdditionsEnabled;

I don't think there is any point exposing this preference for WebKit1.

EDIT: ah I guess this is needed for DRT?

> Source/WebKitLegacy/mac/WebView/WebView.mm:3042
> +    RuntimeEnabledFeatures::sharedFeatures().setInspectorAdditionsEnabled(preferences.inspectorAdditionsEnabled);

Ditto.

> LayoutTests/fast/canvas/2d.currentPoint.html:12
> +function shouldHaveCurrentPointEquality(x, y) {

Nit: shouldHaveCurrentPointEqualTo(x,y)
Comment 23 Devin Rousso 2017-08-04 17:47:15 PDT
Created attachment 317319 [details]
Patch
Comment 24 Devin Rousso 2017-08-07 12:20:24 PDT
Comment on attachment 317084 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317084&action=review

>>> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:194
>>> +    [EnabledAtRuntime=InspectorAdditions, NewObject] DOMPath getPath();
>> 
>> I believe you were going to propose this in standards. Could you add a link to that in the bugzilla bug?
> 
> I'll file a bug tomorrow.  =D

<https://github.com/whatwg/html/issues/2897> Add getPath()/setPath() to CanvasRenderingContext2D
<https://github.com/whatwg/html/issues/2896> Add currentX/currentY attributes to CanvasPath

>>> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:43
>>> +            this._showPathButtonNavigationItem = new WI.ActivateButtonNavigationItem("show-path", WI.UIString("Show Path"), WI.UIString("Hide Path"), "Images/PaintFlashing.svg", 13, 13);
>> 
>> I think we should only include this button if we have the InspectorExtras support? This would help (1) those using Web Inspector frontend outside of WebKit (2) WebKitLegacy inspector where this is won't work:
>> 
>>     static supportsPathDebugging()
>>     {
>>         return !!CanvasRenderingContext2D.prototype.setPath;
>>     }
>> 
>> I think we should have a different icon for this. File a follow-up (and maybe message Jon Davis). Maybe just a few lines /\/ with one being dashed?
> 
> 👍

<https://webkit.org/b/175274> Web Inspector: create better icon for Canvas Path navigation item
Comment 25 Devin Rousso 2017-08-07 12:24:07 PDT
Created attachment 317451 [details]
Patch
Comment 26 Devin Rousso 2017-08-07 14:55:43 PDT
Created attachment 317473 [details]
Patch

Lets try this again :|
Comment 27 Devin Rousso 2017-08-07 15:35:27 PDT
Created attachment 317487 [details]
Patch

Lets see if JSC shows up this time.
Comment 28 WebKit Commit Bot 2017-08-07 16:55:41 PDT
Comment on attachment 317487 [details]
Patch

Clearing flags on attachment: 317487

Committed r220370: <http://trac.webkit.org/changeset/220370>
Comment 29 WebKit Commit Bot 2017-08-07 16:55:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 30 Radar WebKit Bug Importer 2017-08-07 16:56:38 PDT
<rdar://problem/33765490>