Bug 176178 – Web Inspector: Support overloaded CanvasRenderingContext2D actions with identical parameter counts

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

[Matt Baker]

Summary:
Support overloaded CanvasRenderingContext2D actions with identical parameter counts.

For example, the following actions have two parameters, but they differ on type:

    void setFillColor(DOMString color, optional unrestricted float alpha);
    void setFillColor(unrestricted float grayLevel, optional unrestricted float alpha = 1);

RecordingAction swizzles types based on the number of parameters:

    WI.RecordingAction._parameterSwizzleTypeForTypeAtIndex = {
        [WI.Recording.Type.Canvas2D]: {
            "setFillColor": {
                1: {0: String},
                2: {0: String},
            }
        }
    }

Since the first two overloads have the same number of parameters, the first parameter is interpreted as an index, causing the swizzle to fail. Worse, if the value happens to be a valid index, the result is a subtle logic error.

[Radar WebKit Bug Importer]

<rdar://problem/34192229>

[Devin Rousso]

Created attachment 319729 [details]
Patch

[Devin Rousso]

Created attachment 319869 [details]
Patch

[Sam Weinig]

Comment on attachment 319869 [details]
Patch

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

> LayoutTests/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=176178#add_comment

You got an extraneous #add_comment in there.

> LayoutTests/inspector/canvas/recording-2d-expected.txt:61
>              0
>            ],

Is there any way to make the output of this test a bit more understandable? How are you determining that the out is correct?

[Matt Baker]

Created attachment 319977 [details]
[Image] `setFillColor` overload test

Looks good!

Canvas recording now handles the test case that initially brought this to our attention.

[Matt Baker]

Comment on attachment 319869 [details]
Patch

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

Looking good! Just a few changes and questions. Also, it looks like Source/WebCore/WebCore.xcodeproj/project.pbxproj needs to be rebaselined.

> Source/WebCore/inspector/InspectorCanvas.cpp:384
> +    for (T item : vector)

This should be T&, or better yet auto&.

> Source/WebCore/inspector/InspectorCanvas.cpp:489
> +    auto addParameter = [&parametersData, &swizzleTypes] (auto value, RecordingSwizzleTypes swizzleType) {

I think this is a case where auto reduces readability. To find out the type of `value`, I need to find a place where the lambda is used, then lookup `indexForData`.

> Source/WebInspectorUI/UserInterface/Models/Recording.js:196
> +                case WI.Recording.Swizzle.Invalid:

Does Invalid really mean "pass through and don't swizzle"? If so another name may be better, like WI.Recording.Swizzle.None.

> Source/WebInspectorUI/UserInterface/Models/Recording.js:-189
> -                case WI.Recording.Swizzle.TexImageSource:

What happened to TexImageSource?

> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:105
> +            let swizzled = recording.swizzle(item, this._payloadSwizzleTypes[i]);

swizzled -> swizzledItem

> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:114
> +                let array = recording.swizzle(item, WI.Recording.Swizzle.Invalid);

Why is the swizzle type Invalid?

> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:-529
> -

Nice!

[Devin Rousso]

Comment on attachment 319869 [details]
Patch

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

>> Source/WebCore/inspector/InspectorCanvas.cpp:489
>> +    auto addParameter = [&parametersData, &swizzleTypes] (auto value, RecordingSwizzleTypes swizzleType) {
> 
> I think this is a case where auto reduces readability. To find out the type of `value`, I need to find a place where the lambda is used, then lookup `indexForData`.

The reason behind this was that I don't really care about the type as that is handled by the overloaded `addItem`.  The point of this lambda is to avoid duplicated code, not necessarily to do anything fancy.

>> Source/WebInspectorUI/UserInterface/Models/Recording.js:-189
>> -                case WI.Recording.Swizzle.TexImageSource:
> 
> What happened to TexImageSource?

TexImageSource is really just an alias for WTF::Variant<RefPtr<ImageData>, RefPtr<HTMLImageElement>, RefPtr<HTMLCanvasElement>, RefPtr<HTMLVideoElement>>.  As such, it is actually handled by each of those individual cases, which go to ImageData and Image.

>> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:114
>> +                let array = recording.swizzle(item, WI.Recording.Swizzle.Invalid);
> 
> Why is the swizzle type Invalid?

The issue with this case is that Arrays aren't deduplicated right now, but the object holding the data for each CallFrame is deduplicated since it's expected that many actions will have a similar stack trace.  So, until it makes sense to deduplicate arrays, the only time where we have to do this is for CallFrame objects.  In this case, since WI.Recording.Swizzle.Array has a different effect (where it returns the exact value that was sent in the JSON), we need to use another type that will still reach into the deduplicated data but not perform any modifications.  Based on your comment above, I've changed this to None, as that makes more sense.

>> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:-529
>> -
> 
> Nice!

It felt sooooooo good to delete this :P

>> LayoutTests/inspector/canvas/recording-2d-expected.txt:61
>>            ],
> 
> Is there any way to make the output of this test a bit more understandable? How are you determining that the out is correct?

<https://webkit.org/b/176441> Web Inspector: make Canvas recording tests more human readable

[Devin Rousso]

Created attachment 319989 [details]
Patch

[Matt Baker]

Comment on attachment 319989 [details]
Patch

r=me

[Matt Baker]

Comment on attachment 319989 [details]
Patch

cq- for the build error on Windows.

[Sam Weinig]

(In reply to Devin Rousso from comment #7)
> Comment on attachment 319869 [details]
> Patch
> 
> >> LayoutTests/inspector/canvas/recording-2d-expected.txt:61
> >>            ],
> > 
> > Is there any way to make the output of this test a bit more understandable? How are you determining that the out is correct?
> 
> <https://webkit.org/b/176441> Web Inspector: make Canvas recording tests
> more human readable

Thanks!

[WebKit Commit Bot]

Comment on attachment 319989 [details]
Patch

Clearing flags on attachment: 319989

Committed r221695: <http://trac.webkit.org/changeset/221695>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.