[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 #130702|review?                     |review+
               Flag|                            |




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

> 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?

> Source/WebCore/css/mediaControlsChromium.css:45
> -    position: absolute;
> +    position: relative;
>      overflow: visible;

This didn't make it into the ChangeLog.

> Source/WebCore/html/shadow/MediaControlElements.cpp:43
>  #include "HTMLMediaElement.h"
> +#include "HTMLVideoElement.h"
>  #include "HTMLNames.h"

Includes should be sorted.

> 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 ;-)

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

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

> Source/WebCore/html/shadow/MediaControlElements.cpp:1256
> +    // Redraw
> +    removeChildren();

This comment a bit terse.

> 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?

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

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

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