[webkit-reviews] review granted: [Bug 46142] Built-in HTML5 <audio> (and sometimes <video>) UI doesn't update playhead location or time displays : [Attachment 80717] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jer Noble <jer.noble at apple.com>'s
request for review:
Bug 46142: Built-in HTML5 <audio> (and sometimes <video>) UI doesn't update
playhead location or time displays
https://bugs.webkit.org/show_bug.cgi?id=46142

Attachment 80717: Patch
https://bugs.webkit.org/attachment.cgi?id=80717&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=80717&action=review

> Source/WebCore/rendering/RenderMedia.cpp:92
> +    // Make sure to push a new LayoutState onto the LayoutState stack, so
that subsequent queries of 
> +    // view()->layoutState() return valid values for m_paintOffset and
m_layoutOffset.  These values
> +    // are queried by our children, from within
RenderBox::computeRectForRepaint(), which is called 
> +    // in turn by RenderObject::repaintRectangle(), which is called in turn
by controlsRenderer->layout().
> +    // If we do not push our LayoutState onto the stack, repainting
operations be attempted at incorrect 
> +    // coordinates.

Aggh! I have pushed you into writing a bad comment! I suggest this comment:

    // We are calling the layout function directly on controlsRenderer, so need
to set up state with LayoutStateMaintainer.

Or something along those lines. I still don’t understand exactly under what
circumstances use of LayoutStateMaintainer is needed, so I’m not entirely
satisfied, but you must not let my feedback cause you to check in a giant
comment!

> Source/WebCore/rendering/RenderMedia.cpp:103
> +    statePusher.pop();

I suggest a blank line before this to match the declaration above.