Bug 176178 - Web Inspector: Support overloaded CanvasRenderingContext2D actions with identical parameter counts
Summary: Web Inspector: Support overloaded CanvasRenderingContext2D actions with ident...
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:
Blocks: WebInspectorCanvasRecording
  Show dependency treegraph
 
Reported: 2017-08-31 12:46 PDT by Matt Baker
Modified: 2017-09-06 13:16 PDT (History)
4 users (show)

See Also:


Attachments
Patch (177.17 KB, patch)
2017-09-02 12:35 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (198.54 KB, patch)
2017-09-04 19:10 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] `setFillColor` overload test (368.77 KB, image/png)
2017-09-05 21:09 PDT, Matt Baker
no flags Details
Patch (179.18 KB, patch)
2017-09-05 23:57 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 Matt Baker 2017-08-31 12:46:52 PDT
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.
Comment 1 Radar WebKit Bug Importer 2017-08-31 12:47:26 PDT
<rdar://problem/34192229>
Comment 2 Devin Rousso 2017-09-02 12:35:12 PDT
Created attachment 319729 [details]
Patch
Comment 3 Devin Rousso 2017-09-04 19:10:44 PDT
Created attachment 319869 [details]
Patch
Comment 4 Sam Weinig 2017-09-04 19:52:18 PDT
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?
Comment 5 Matt Baker 2017-09-05 21:09:24 PDT
Created attachment 319977 [details]
[Image] `setFillColor` overload test

Looks good!

Canvas recording now handles the test case that initially brought this to our attention.
Comment 6 Matt Baker 2017-09-05 21:49:58 PDT
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!
Comment 7 Devin Rousso 2017-09-05 22:36:35 PDT
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
Comment 8 Devin Rousso 2017-09-05 23:57:56 PDT
Created attachment 319989 [details]
Patch
Comment 9 Matt Baker 2017-09-06 01:13:55 PDT
Comment on attachment 319989 [details]
Patch

r=me
Comment 10 Matt Baker 2017-09-06 01:17:14 PDT
Comment on attachment 319989 [details]
Patch

cq- for the build error on Windows.
Comment 11 Sam Weinig 2017-09-06 09:19:46 PDT
(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!
Comment 12 WebKit Commit Bot 2017-09-06 13:16:35 PDT
Comment on attachment 319989 [details]
Patch

Clearing flags on attachment: 319989

Committed r221695: <http://trac.webkit.org/changeset/221695>
Comment 13 WebKit Commit Bot 2017-09-06 13:16:37 PDT
All reviewed patches have been landed.  Closing bug.