[webkit-reviews] review granted: [Bug 178302] Web Inspector: Canvas2D Profiling: highlight expensive context commands in the captured command log : [Attachment 324088] Alternative

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has granted Devin Rousso
<webkit at devinrousso.com>'s request for review:
Bug 178302: Web Inspector: Canvas2D Profiling: highlight expensive context
commands in the captured command log
https://bugs.webkit.org/show_bug.cgi?id=178302

Attachment 324088: Alternative

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




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

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

r=me with some naming nits. Please re-EWS since this patch is old.

> Source/WebCore/inspector/InspectorCanvas.cpp:174
>  void InspectorCanvas::markNewFrame()

Please rename to "finalizeFrame" or something, now that it's called at the end
of a frame.

> Source/WebCore/inspector/InspectorCanvas.cpp:177
> +	  
static_cast<Inspector::Protocol::Recording::Frame*>(m_frames->get(m_frames->len
gth() - 1).get())->setTime(monotonicallyIncreasingTimeMS() -
m_currentFrameStartTime);

Please break up this long line. How are you sure that m_frames->length() is
nonzero?

> Source/WebInspectorUI/UserInterface/Models/RecordingFrame.js:55
> +    get time() { return this._time; }

I'd prefer we call this duration throughout. Does 'time' refer to duration,
start time, or end time? I don't know.