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.
<rdar://problem/34192229>
Created attachment 319729 [details] Patch
Created attachment 319869 [details] Patch
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?
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 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 = [¶metersData, &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 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 = [¶metersData, &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
Created attachment 319989 [details] Patch
Comment on attachment 319989 [details] Patch r=me
Comment on attachment 319989 [details] Patch cq- for the build error on Windows.
(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 on attachment 319989 [details] Patch Clearing flags on attachment: 319989 Committed r221695: <http://trac.webkit.org/changeset/221695>
All reviewed patches have been landed. Closing bug.