[webkit-reviews] review granted: [Bug 79746] Main rendering steps for displaying the cue boxes : [Attachment 130702] Updated 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 130702: Updated patch
https://bugs.webkit.org/attachment.cgi?id=130702&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
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.