Bug 174967 – Web Inspector: Preview Canvas path when viewing a recording

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 316939 [details]
Patch

[Devin Rousso]

Comment on attachment 316939 [details]
Patch

Oops.  Wrong bug.

[Devin Rousso]

Created attachment 317013 [details]
Patch

[Devin Rousso]

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

[Matt Baker]

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

[Devin Rousso]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Devin Rousso]

Created attachment 317084 [details]
Patch

[Joseph Pecoraro]

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)

[Joseph Pecoraro]

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.

[Simon Fraser (smfr)]

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.

[Joseph Pecoraro]

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

[Devin Rousso]

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

[Devin Rousso]

Created attachment 317215 [details]
Patch

[Blaze Burg]

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)

[Devin Rousso]

Created attachment 317319 [details]
Patch

[Devin Rousso]

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

[Devin Rousso]

Created attachment 317451 [details]
Patch

[Devin Rousso]

Created attachment 317473 [details]
Patch

Lets try this again :|

[Devin Rousso]

Created attachment 317487 [details]
Patch

Lets see if JSC shows up this time.

[WebKit Commit Bot]

Comment on attachment 317487 [details]
Patch

Clearing flags on attachment: 317487

Committed r220370: <http://trac.webkit.org/changeset/220370>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33765490>