[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