[webkit-reviews] review denied: [Bug 175166] Web Inspector: provide method for recording CanvasRenderingContext2D from JavaScript : [Attachment 317219] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has denied Devin Rousso <drousso at apple.com>'s
request for review:
Bug 175166: Web Inspector: provide method for recording
CanvasRenderingContext2D from JavaScript
https://bugs.webkit.org/show_bug.cgi?id=175166

Attachment 317219: Patch

https://bugs.webkit.org/attachment.cgi?id=317219&action=review




--- Comment #3 from Brian Burg <bburg at apple.com> ---
Comment on attachment 317219
  --> https://bugs.webkit.org/attachment.cgi?id=317219
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317219&action=review

Overall the patch is good with a few issues. And it looks like you pulled in
some wrong commits into the diff, please fix that.

> Source/WebInspectorUI/ChangeLog:14
> +	   Open a Recording tab if no CanvasContentView triggered a recording,
as means that the

What will happen if a page samples every 10'th frame? The UI will be unusable.
I think if a recording is already being viewed, then the new ones should show
up in the navigation sidebar list of recordings (that we don't have yet). Let's
remember this when working on that UI.

> Source/WebCore/inspector/InspectorInstrumentation.h:1142
> +inline void
InspectorInstrumentation::didRequestRecordingCanvas(HTMLCanvasElement&
canvasElement)

I don't like this. Let's follow the pattern used for startProfiling /
stopProfiling:

InspectorInstrumentation::{start,stop}CanvasRecording
 -> calls canvasAgent->{start,stop}FromConsole

This is much clearer when reading the canvas agent code because we know exactly
which entry point is being used by console API without digging up 5 other
files.

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:126
> +	   let defaultPrevented =
this.dispatchEventToListeners(WI.CanvasManager.Event.RecordingFinished,
{canvas, recording});

I don't understand why this change is needed.

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:131
> +	       WI.showRepresentedObject(recording, null, {suppressSelection:
true});

Likewise. If this is showing a recording after console.recordEnd, it would be
much easier to grok if the logic was inside
InspectorCanvasAgent::stopRecordingFromConsole.

> Tools/ChangeLog:4
> +	   https://bugs.webkit.org/show_bug.cgi?id=174718

Oops.

> LayoutTests/ChangeLog:3
> +	   Web Inspector: add stack trace information for each RecordingAction

Oops.