[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 #9 from Victor Carbune <vcarbune at adobe.com>  2012-03-09 14:28:15 PST ---
(In reply to comment #7)
> (From update of attachment 130702 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130702&action=review
> 
> I made a few minor suggestions, but this looks great overall! It is probably worth regenerating the ChangeLogs to make sure all changes are documented.
> 
> > Source/WebCore/ChangeLog:13
> > +        * css/mediaControls.css:
> > +        (video::-webkit-media-text-track-display): Adjusted text color to match
> > +        the color required in the WebVTT spec (section 3.5.1 'color' property)
> 
> It is worth mentioning the changes you made to position the controls.
Done.

> > Source/WebCore/css/mediaControls.css:44
> > -    display: block;
> > +    display: -webkit-box;
> >      direction: ltr;
> > +    -webkit-box-align: start;
> > +    -webkit-box-pack: end;
> > +    -webkit-box-orient: vertical;
> 
> I expect this is the source of the chromium test failures?
Yes, just these lines and functional visual changes.

> > Source/WebCore/css/mediaControlsChromium.css:45
> > -    position: absolute;
> > +    position: relative;
> >      overflow: visible;
> 
> This didn't make it into the ChangeLog.
Regenerated changelogs.

> > Source/WebCore/html/shadow/MediaControlElements.cpp:43
> >  #include "HTMLMediaElement.h"
> > +#include "HTMLVideoElement.h"
> >  #include "HTMLNames.h"
> 
> Includes should be sorted.
Done.

> > Source/WebCore/html/shadow/MediaControlElements.cpp:184
> > +    stopTimer();
> > +
> > +    double duration = document()->page() ? document()->page()->theme()->mediaControlsFadeOutDuration() : 0;
> > +    m_transitionTimer.startOneShot(duration);
> 
> It would be good to have a brief comment here about why we need to use a timer for this. As you know, this confused at least me ;-)
Added.

> The timer functions also didn't make it into the ChangeLog.
Regenerated ChangeLogs.

> > Source/WebCore/html/shadow/MediaControlElements.cpp:1244
> > +    // 4. If the user agent is exposing a user interface for video, add to
> > +    // output one or more completely transparent positioned CSS block boxes that
> > +    // cover the same region as the user interface.
> > +
> > +    // 5. If the last time these rules were run, the user agent was not exposing
> > +    // a user interface for video, but now it is, let reset be true. Otherwise,
> > +    // let reset be false.
> 
> A brief comment about why you don't have to do anything explicit for these steps would be helpful.
Done.

> > Source/WebCore/html/shadow/MediaControlElements.cpp:1256
> > +    // Redraw
> > +    removeChildren();
> 
> This comment a bit terse.
Changed.

> > Source/WebCore/html/shadow/MediaControlElements.cpp:1260
> > +    // 9. If reset is false, then, for each text track cue cue in cues: if cue's
> > +    // text track cue display state has a set of CSS boxes, then add those boxes
> > +    // to output, and remove cue from cues.
> 
> A brief comment about why you don't have to do anything explicit for these steps would be helpful.
> 
> > Source/WebCore/html/track/TextTrackCue.cpp:426
> > +    // 10.11. Apply the terms of the CSS specifications to nodes within the
> > +    // following constraints, thus obtaining a set of CSS boxes positioned
> > +    // relative to an initial containing block:
> > +    RefPtr<HTMLDivElement> m_displayTree = HTMLDivElement::create(document);
> 
> Could you empty the div if it already exists instead of deleting and allocating it every time a cue changes?
Yes, it indeed makes a lot of sense not adding and deleting nodes all the time.

However, we certainly don't want to end up with a lot of empty divs (for each cue in the track), so they should be removed even if they're empty.

I have changed the methods to keep the div appended only while the cue has the active flag set, and called remove() on the displayTree when the flag is unset (within TextTrackCue::setIsActive).

> > Source/WebCore/html/track/TextTrackCue.cpp:429
> > +    // The document tree is the tree of WebVTT Node Objects rooted at nodes.
> > +    RefPtr<DocumentFragment> documentTree = WebVTTParser::create(0, m_scriptExecutionContext)->createDocumentFragmentFromCueText(m_content);
> 
> Why not use TextTrackCue::getCueAsHTML() instead? That way we will use the fragment if it has already been created (always the case for a cue that has not been modified by script).
Done.

> > LayoutTests/media/media-controls.js:35
> > -function textTrackDisplayElement(parentElement, id)
> > +function textTrackDisplayElement(parentElement)
> 
> Nit: I think this would be easier to understand if it kept the "id" parameter.
Done.

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