Bug 174481

Created attachment 315409 [details] Patch

Created attachment 315833 [details] Patch

Created attachment 315893 [details] Patch

Created attachment 315913 [details] Patch

Comment on attachment 315913 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315913&action=review > Source/JavaScriptCore/inspector/protocol/Canvas.json:60 > + { "name": "actions", "type": "array", "items": { "$ref": "RecordingAction" }, "description": "JSON data of all canvas API calls." }, Does "JSON data" describing something subtle, or can it be dropped? > Source/JavaScriptCore/inspector/protocol/Canvas.json:128 > + { "name": "canvasId", "$ref": "CanvasId", "description": "Canvas identifier." }, Drop description? > Source/WebCore/ChangeLog:11 > + Currently, a recording doesn't actually "start" until an action is performed on the context. > + This change adds the recording logic, but it does not use it anywhere. This decision was > + made so that if the usage causes performance regressions, rolling back will not remove the > + protocol commands or logic. Don't we want to put InspectorInstrumentation::recordCanvasAction() calls in WebCore if we want to check if this causes performance regressions? This current patch only changes Inspector code so it would be _highly_ unlikely to affect performance. > Source/WebCore/inspector/InspectorCanvas.h:62 > + bool singleFrame() { return m_singleFrame; } Nit: const > Source/WebCore/inspector/InspectorCanvas.h:67 > - ~InspectorCanvas() { } > + ~InspectorCanvas(); Does this need to be public? Can it be moved next to the Private constructor? > Source/WebCore/inspector/InspectorCanvas.h:79 > + long m_availableBufferSpace; Lets give this a default value here. > Source/WebCore/inspector/InspectorCanvas.h:80 > + bool m_singleFrame; Lets { false } this. > Source/WebCore/inspector/InspectorCanvasAgent.h:79 > + void didFinishRecordingCanvasFrame(HTMLCanvasElement&, bool = false); Style: Name the final bool parameter otherwise it is unclear what it is for. > Source/WebCore/inspector/InspectorInstrumentation.h:229 > + static void didFinishRecordingCanvasFrame(HTMLCanvasElement&, bool = false); Style: Name the final bool parameter otherwise it is unclear what it is for. > Source/WebCore/inspector/InspectorInstrumentation.h:388 > + static void didFinishRecordingCanvasFrameImpl(InstrumentingAgents*, HTMLCanvasElement&, bool = false); Style: Name the final bool parameter otherwise it is unclear what it is for. > Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:99 > + recordingFinished(canvasIdentifier, initialState, frames) { Style: Brace on its own line. > Source/WebInspectorUI/UserInterface/Models/Recording.js:36 > + this._version = parseInt(version); > + this._type = String(type); > + this._initialState = typeof initialState === "object" ? initialState : {}; > + this._frames = Array.isArray(frames) ? frames : []; > + this._actions = [new WebInspector.RecordingInitialStateAction].concat(...this._frames.map((frame) => frame.actions)); Normally we assert the types are expected to catch our own programming errors, and then handle optional values. However, I'm guessing since we can import a recording we want to be super type safe here? I think we should verify that the version is reasonable. if (this._version <= 0) throw "Invalid version";

Comment on attachment 315913 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315913&action=review >> Source/WebCore/ChangeLog:11 >> + protocol commands or logic. > > Don't we want to put InspectorInstrumentation::recordCanvasAction() calls in WebCore if we want to check if this causes performance regressions? This current patch only changes Inspector code so it would be _highly_ unlikely to affect performance. That's the idea. This patch shouldn't effect performance, whereas the next patch (which contains all the usages of InspectorInstrumentation::recordCanvasAction) might <https://webkit.org/b/174482>. Since this patch contains all the protocol changes, it would be preferred to not have to roll it back so that the protocol doesn't change. >> Source/WebInspectorUI/UserInterface/Models/Recording.js:36 >> + this._actions = [new WebInspector.RecordingInitialStateAction].concat(...this._frames.map((frame) => frame.actions)); > > Normally we assert the types are expected to catch our own programming errors, and then handle optional values. > > However, I'm guessing since we can import a recording we want to be super type safe here? > > I think we should verify that the version is reasonable. > > if (this._version <= 0) > throw "Invalid version"; That is correct. I currently have the version check inside the function that loads the JSON from a file: <https://bugs.webkit.org/attachment.cgi?id=315914&action=review#line7640>.

Created attachment 316179 [details] Patch

Comment on attachment 316179 [details] Patch Attachment 316179 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4166481 Number of test failures exceeded the failure limit.

Created attachment 316183 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 316179 [details] Patch Attachment 316179 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4166484 Number of test failures exceeded the failure limit.

Created attachment 316185 [details] Archive of layout-test-results from ews114 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 316179 [details] Patch Attachment 316179 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4166503 Number of test failures exceeded the failure limit.

Created attachment 316186 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Created attachment 316342 [details] Patch

Created attachment 316356 [details] Patch

Comment on attachment 316356 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316356&action=review r- because all the bots are red and a few questions below. That said, this looks really great so far to me. > Source/JavaScriptCore/inspector/protocol/Recording.json:12 > + "id": "Recording", Nit: I'd put this type after the other types it uses (InitialState and Frame). Since I'd expect those dependent types to have been defined earlier ("above"). I realize it doesn't matter, but that seems like a pattern we follow. > Source/JavaScriptCore/inspector/protocol/Recording.json:15 > + { "name": "version", "type": "integer" }, Maybe this deserves a comment. This is about future/backwards compatibility? > Source/JavaScriptCore/inspector/protocol/Recording.json:18 > + { "name": "initialState", "$ref": "InitialState", "description": "JSON data of inital state of canvas before recording." }, > + { "name": "frames", "type": "array", "items": { "$ref": "Frame" }, "description": "JSON data of all canvas API calls." }, These both say "canvas" but you've made Recording a general domain. So shouldn't these just say "initial state before recording" and "all API calls." etc. > Source/JavaScriptCore/inspector/protocol/Recording.json:37 > + { "name": "actions", "type": "array", "items": { "type": "any" }, "description": "Information about an action made to the recorded canvas. Follows the structure [name, parameters, trace], where name is a string, parameters is an array, and trace is an array."}, Nit: "canvas again". > Source/JavaScriptCore/inspector/protocol/Recording.json:38 > + { "name": "incomplete", "type": "boolean", "optional": true, "description": "Flag indicating if recording was stopped before frame ended." } Grammar: "if recording" => "if the recording" Grammar: "before frame" => "before this frame" > Source/WebCore/WebCore.xcodeproj/project.pbxproj:3961 > + 952076051F2675FE007D2AAB /* CallTracer.h in Headers */ = {isa = PBXBuildFile; fileRef = 952076011F2675F9007D2AAB /* CallTracer.h */; settings = {ATTRIBUTES = (Private, ); }; }; > + 952076061F2675FE007D2AAB /* CallTracerTypes.h in Headers */ = {isa = PBXBuildFile; fileRef = 952076021F2675F9007D2AAB /* CallTracerTypes.h */; settings = {ATTRIBUTES = (Private, ); }; }; I don't think these need to be exported as Private. Try making them Project in Xcode's right sidebar. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4892 > + push(@$outputArray, " CallTracer::$callTracingCallback(impl, ASCIILiteral(\"" . $attribute->name . "\"), \{ nativeValue \});\n"); Style: Almost no perl escapes '{' and '}' in strings. So I don't think the escapes are needed here. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:6036 > + push(@$outputArray, $indent . " CallTracer::$callTracingCallback(impl, ASCIILiteral(\"" . $operation->name . "\"), \{ " . join(", ", @inspectorRecordingArguments) . " \});\n"); Style: Almost no perl escapes '{' and '}' in strings. So I don't think the escapes are needed here. > Source/WebCore/bindings/scripts/IDLAttributes.json:37 > + "CallTracingCallback": { > + "contextsAllowed": ["interface", "attribute", "operation"], > + "values": ["*"] > + }, Currently we only intend to use this on interface. Are you thinking of cases where we actually would want to do it only on an individual attribute / operation? > Source/WebCore/bindings/scripts/test/JS/JSTestCallTracer.cpp:341 > + auto nodeVariadicArg = convertVariadicArguments<IDLInterface<Node>>(*state, 0); > + RETURN_IF_EXCEPTION(throwScope, encodedJSValue()); > + if (UNLIKELY(impl.callTracingActive())) > + CallTracer::testCallTracerInterface(impl, ASCIILiteral("testOperationWithVariadicArgument"), { nodeVariadicArg }); Hmm, I don't think we want to expose the `nodeVariadicArg` type this way. I think it would be a `Detail::VariadicConverter<IDLType>::Result` type. We probably want to pass along the nodeVariadicArg.arguments optional / Vector. That said, your real use case doesn't have any variadic methods so maybe leave this as it is and tackle it when/if anyone ever needs it (they would have to make their code compile with it when that arrises). I think you can drop it from the Test file. > Source/WebCore/bindings/scripts/test/TestCallTracer.idl:37 > + void testOperationWithArguments(Node nodeArg); > + void testOperationWithNullableArgument(Node? nodeNullableArg); > + void testOperationWithVariadicArgument(Node... nodeVariadicArg); Add a test with multiple arguments: void testOperationWithMultipleArguments(float a, float b, float c); > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:975 > +String CanvasRenderingContext2D::stringForWindingRule(WindingRule windingRule) > +{ > + switch (windingRule) { > + case WindingRule::Nonzero: > + return ASCIILiteral("nonzero"); > + case WindingRule::Evenodd: > + return ASCIILiteral("evenodd"); > + } Aside: It has bugged me that we autogenerate conversions from IDL enums to C++ Enums but don't autogenerate a stringification for them. > Source/WebCore/inspector/InspectorCanvas.cpp:194 > + array->addItem(WTFMove(data)); This one is not deduplicated? So if someone puts a small image to their canvas 100 times in 100 different places we send the full data 100 times? > Source/WebCore/inspector/InspectorCanvas.cpp:202 > + size_t index = m_indexedDuplicateData.find(data); So this is a linear search for each de-duplication? I think we can do better, but it has been performing great for you so maybe this is not a concern. Do you know how much deduplication increased recording time? > Source/WebCore/inspector/InspectorCanvas.cpp:255 > + m_availableBufferSpace -= item->memoryCost(); AvailableBufferSpace is a `long` and memoryCost is a `size_t`. We should at least assert the cost is less than `std::numeric_limits<long>::max()`. Or perhaps just turn this into a couple numbers: size_t m_bufferLimit { 100 * 1024 * 1024 }; size_t m_bufferUsed { 0 }; And then you can check if used > limit. > Source/WebCore/inspector/InspectorCanvas.h:55 > + const HTMLImageElement*, > +#if ENABLE(VIDEO) > + HTMLVideoElement*, > +#endif > + HTMLCanvasElement*, Can these all be const or do you have non-const operations on a few? > Source/WebCore/inspector/InspectorCanvasAgent.cpp:79 > - , m_timer(*this, &InspectorCanvasAgent::canvasDestroyedTimerFired) > + , m_canvasDestroyedTimer(*this, &InspectorCanvasAgent::canvasDestroyedTimerFired) > + , m_canvasRecordingTimer(*this, &InspectorCanvasAgent::canvasRecordingTimerFired) I didn't review the timers very closely since it sounded like you were going to change them. > Source/WebCore/inspector/InspectorCanvasAgent.cpp:248 > + if (inspectorCanvas->canvas().renderingContext()->callTracingActive()) > + return; Lets return an error string in this case because this shouldn't happen. Something like: "Already recording for canvas". > Source/WebCore/inspector/InspectorCanvasAgent.cpp:266 > + if (!inspectorCanvas->canvas().renderingContext()->callTracingActive()) > + return; Lets return an error string in this case because this shouldn't happen. Something like: "No active recording for canvas". > Source/WebCore/inspector/InspectorCanvasAgent.cpp:371 > + if (!inspectorCanvas->hasRecordingData()) { I think it would be worth pulling this out into its own function `buildInitialState()`. Same with the other huge block as something like `buildAction()`. Then the rest of the logic inside recordCanvasAction (looking up the canvas, resetting timers, checking buffer size) can be unencumbered by the logic of building the protocol objects. > Source/WebCore/inspector/InspectorCanvasAgent.cpp:425 > + attributes->setDouble(ASCIILiteral("fillStyle"), static_cast<double>(strokeStyleIndex)); This should be `fillStyleIndex` not `strokeStyleIndex`. What about using setInteger instead of setDouble for these indexForData() results? You'll still need to cast down to int. Vector's length is really `unsigned` despite the API saying it is `size_t`. We can ASSERT that the length of the vector never goes past INT_MAX (if it does we will likely have other problems trying to serialize a vastly >4GB JSON string). > Source/WebCore/inspector/InspectorCanvasAgent.cpp:436 > + if (parameters->length()) > + initialState->setParameters(WTFMove(parameters)); Parameters doesn't appear to be used yet. Is this expected for the future with WebGL? If not maybe we should remove it. > Source/WebCore/inspector/InspectorCanvasAgent.cpp:451 > + [&] (const Element*) { parametersData->addItem(static_cast<double>(inspectorCanvas->indexForData(String("element")))); }, I think this is only for `drawFocusRing` and always sending the string "element" won't be enough for the other side to reproduce the real result. I think that is fine, its probably a rarely used Canvas API. But I think this deserves a comment. > Source/WebCore/inspector/InspectorCanvasAgent.cpp:469 > + buildStringFromPath(value->path(), path); If we have a null/empty path this returns false and we will send an empty string. I assume that is fine? But if that is the case I wonder why buildStringFromPath returns false at all. > Source/WebCore/inspector/InspectorCanvasAgent.cpp:475 > + [&] (double value) { parametersData->addItem(value); }, > + [&] (float value) { parametersData->addItem(value); }, What happens if someone provided NaN for any of our arguments? Do we JSON.stringify that properly? js> JSON.stringify([NaN]) "[null]" > Source/WebCore/inspector/InspectorCanvasAgent.cpp:481 > + [&] (const std::optional<float>& value) { > + if (value) > + parametersData->addItem(value.value()); > + }, Neat! So in case someone calls: void setAlpha(optional unrestricted float alpha = NaN); with no arguments, we will accurately display this as: setAlpha() and not: setAlpha(NaN) Awesome! > Source/WebCore/svg/SVGPathUtilities.cpp:55 > + if (path.isNull() || path.isEmpty()) > + return false; Curious, buildPathFromString returns true for an empty string. Maybe an empty path should produce an empty string? If that is the case and we can always return a valid string for a Path then we could simplify this to just `String buildStringFromPath(const Path&)`. > Source/WebCore/svg/SVGPathUtilities.cpp:61 > + case PathElementMoveToPoint: // The points member will contain 1 value. I don't think these comments add any value. > Source/WebCore/svg/SVGPathUtilities.cpp:63 > + builder.append(String::numberToStringECMAScript(element.points[0].x())); Style: Lets use builder.appendECMAScriptNumber(...) which should be identical.

Comment on attachment 316356 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316356&action=review >> Source/WebCore/bindings/scripts/IDLAttributes.json:37 >> + }, > > Currently we only intend to use this on interface. Are you thinking of cases where we actually would want to do it only on an individual attribute / operation? I currently don't have any cases, but I am not sure how complex the tracing of WebGL calls will be, so I figured it would be simpler to allow all of it in one go. >> Source/WebCore/inspector/InspectorCanvas.cpp:194 >> + array->addItem(WTFMove(data)); > > This one is not deduplicated? So if someone puts a small image to their canvas 100 times in 100 different places we send the full data 100 times? The ImageData object itself is what is deduplicated. If the user decides to create 100 different ImageData objects and use them all, then yes, we could have 100 different sets of data. Since the data is an array, we'd have to iterate over each item in it to check if it matches that of another ImageData object. I was going to do an array deduplication patch as a followup. >> Source/WebCore/inspector/InspectorCanvas.cpp:202 >> + size_t index = m_indexedDuplicateData.find(data); > > So this is a linear search for each de-duplication? I think we can do better, but it has been performing great for you so maybe this is not a concern. > > Do you know how much deduplication increased recording time? I don't have any metrics on this. It really depends on what's being recorded. If the user is continually drawing the same ImageData object, this deduplication is vastly faster than having to serialize the ImageData each time. If the user is drawing a bunch of different strings, then this might perform the same/worse than no deduplication. >> Source/WebCore/inspector/InspectorCanvas.h:55 >> + HTMLCanvasElement*, > > Can these all be const or do you have non-const operations on a few? A few of them have to be non-const. Getting the dataURL of a video/canvas is not a const operation. >> Source/WebCore/inspector/InspectorCanvasAgent.cpp:436 >> + initialState->setParameters(WTFMove(parameters)); > > Parameters doesn't appear to be used yet. Is this expected for the future with WebGL? If not maybe we should remove it. It is needed for WebGL, specifically for creating the context with WebGLAttributes. >> Source/WebCore/inspector/InspectorCanvasAgent.cpp:469 >> + buildStringFromPath(value->path(), path); > > If we have a null/empty path this returns false and we will send an empty string. I assume that is fine? But if that is the case I wonder why buildStringFromPath returns false at all. Sending an empty string is fine. I was following the convention set by the rest of the SVGPathUtilites functions. There is no real reason to return true/false other than that it is a more clear explanation as to whether it succeeded/failed. I will change it to just return a String. >> Source/WebCore/inspector/InspectorCanvasAgent.cpp:475 >> + [&] (float value) { parametersData->addItem(value); }, > > What happens if someone provided NaN for any of our arguments? Do we JSON.stringify that properly? > > js> JSON.stringify([NaN]) > "[null]" Yes, this works correctly. >> Source/WebCore/svg/SVGPathUtilities.cpp:55 >> + return false; > > Curious, buildPathFromString returns true for an empty string. Maybe an empty path should produce an empty string? > > If that is the case and we can always return a valid string for a Path then we could simplify this to just `String buildStringFromPath(const Path&)`. See above.

Created attachment 316436 [details] Patch

Created attachment 316438 [details] Patch

Comment on attachment 316438 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316438&action=review r=me I have minor concerns about `NaN` not being handled properly because it is not JSON serializable. But that is an edge case for canvas right now because we never expect users to use NaN in these cases. I'll expect a test for that with the next patch (the frontend portion). > Source/WebCore/inspector/InspectorCanvas.cpp:147 > + m_bufferLimit = std::min(memoryLimit, static_cast<long>(std::numeric_limits<int>::max())); std::min<long>() I think might make the right side cast not be necessary. > Source/WebCore/inspector/InspectorCanvasAgent.h:79 > + void didFinishRecordingCanvasFrame(HTMLCanvasElement&, bool forceDispatch = false); We prefer enums for boolean parameters so its clear what they mean at the call site. Here you could do something like: ForceDispatch::Yes, ForceDispatch::No. Not sure how important it is for this (I can't think of a great name) so I could go either way. > Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:48 > + static fromPayload(payload) > + { > + if (!Array.isArray(payload)) > + payload = []; > + > + if (isNaN(payload[0])) // Name > + payload[0] = -1; > + > + if (!Array.isArray(payload[1])) // Parameters > + payload[1] = []; > + > + return new WebInspector.RecordingAction(...payload); > + } Maybe just a comment above the fromPayload describing the format. It'll be much easier to read than the code below. // Payload format: [name, parameters] static fromPayload(payload) Alternatively I've just added a comment with the protocol type it conforms to: Models/PropertyPreview.js 47- // Runtime.PropertyPreview. 48: static fromPayload(payload)

Created attachment 316470 [details] Patch

Comment on attachment 316470 [details] Patch Clearing flags on attachment: 316470 Committed r219964: <http://trac.webkit.org/changeset/219964>

All reviewed patches have been landed. Closing bug.