[webkit-reviews] review granted: [Bug 79746] Main rendering steps for displaying the cue boxes : [Attachment 131107] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Victor Carbune
<vcarbune at adobe.com>'s request for review:
Bug 79746: Main rendering steps for displaying the cue boxes
https://bugs.webkit.org/show_bug.cgi?id=79746

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

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=131107&action=review


This is a *terrific* patch, thank you! I am marking this cq- only because of
the duplicate entry in the layout test ChangeLog.

> Source/WebCore/html/shadow/MediaControlElements.cpp:1224
> +    LOG(Media, "MediaControlTextTrackContainerElement::updateDisplay()");

Nit: This function is called frequently enough that this logging generates a
*lot* of output. We should remove it in a future check in.

> Source/WebCore/html/shadow/MediaControlElements.cpp:1285
> +	   if (!contains(displayTree.get()) && displayTree->hasChildNodes())
> +	       appendChild(displayTree, ASSERT_NO_EXCEPTION, true);

Nit: I think the "hasChildNodes()" test will be faster than "contains()", so it
should be the first test in the expression. This would be good to fix the next
time this function is modified.

> Source/WebCore/html/track/TextTrackCue.cpp:424
> +    if (!m_displayTreeShouldChange ||
!m_scriptExecutionContext->isDocument())

Nit: unless a scriptExecutionContext can change type, we should
"ASSERT(m_scriptExecutionContext->isDocument())" in the constructor instead of
checking it every time through here. Again, this can be changed the next time
this file is modified.

> Source/WebCore/html/track/TextTrackCue.cpp:441
> +    m_displayTree->appendChild(getCueAsHTML(), ASSERT_NO_EXCEPTION, true);

Why not use m_documentFragment directly, getCueAsHTML creates a new fragment
every time? If it isn't possible to use m_documentFragment, why do we cache it
in getCueAsHTML at all?

Either way, this is an optimization and can be done in a follow up patch.

> LayoutTests/ChangeLog:3175
> +2012-03-08  Victor Carbune  <vcarbune at adobe.com>
> +
> +	   Updated LayoutTests for basic rendering of cues.
> +	   https://bugs.webkit.org/show_bug.cgi?id=79746
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   * media/media-controls.js:
> +	   (mediaControlsElement):
> +	   (textTrackDisplayElement):
> +	   * media/track/track-cue-mutable-text-expected.txt:
> +	   * media/track/track-cue-nothing-to-render-expected.txt:
> +	   * media/track/track-cue-rendering-expected.txt:
> +	   * media/track/track-cue-rendering.html:
> +

Oops, the ChangeLog should not have two entries.