[Webkit-unassigned] [Bug 79746] Main rendering steps for displaying the cue boxes

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=79746


Eric Carlson <eric.carlson at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #131107|review?, commit-queue?      |review+, commit-queue-
               Flag|                            |




--- Comment #10 from Eric Carlson <eric.carlson at apple.com>  2012-03-10 13:38:58 PST ---
(From update of attachment 131107)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.