[webkit-reviews] review denied: [Bug 174481] Web Inspector: create protocol for recording Canvas contexts : [Attachment 316356] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jul 25 22:56:12 PDT 2017
Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 174481: Web Inspector: create protocol for recording Canvas contexts
https://bugs.webkit.org/show_bug.cgi?id=174481
Attachment 316356: Patch
https://bugs.webkit.org/attachment.cgi?id=316356&action=review
--- Comment #16 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 316356
--> https://bugs.webkit.org/attachment.cgi?id=316356
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(Strin
g("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.
More information about the webkit-reviews
mailing list