[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





--- Comment #12 from Victor Carbune <vcarbune at adobe.com>  2012-03-11 16:17:20 PST ---
(In reply to comment #10)
> (From update of attachment 131107 [details])
> 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.
Fixed.

> > 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.
Fixed.

> > 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.
Fixed.

> > 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.
Here's my approach:
* m_documentFragment is cached in getCueAsHTML() because it's faster to clone it, rather than re-parse & re-create it (re-parsing is required only when text contents have changed).
* m_displayTree is the cached 'internal version' of m_documentFragment for the shadow dom (it gets reprocessed only when m_displayTreeShouldChange is true), which later will have additional styling or other nodes we need for the cue rendering rules.

This implies that m_documentFragment can't be used directly in m_displayTree because calling getCueAsHTML() needs to have a clean version of it.

> > 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.
Sorry about this, it seems I'm constantly having issues merging ChangeLogs ;)

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