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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Mar 2 10:59:40 PST 2012


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





--- Comment #2 from Eric Carlson <eric.carlson at apple.com>  2012-03-02 10:59:40 PST ---
(From update of attachment 129769)
View in context: https://bugs.webkit.org/attachment.cgi?id=129769&action=review

> Source/WebCore/ChangeLog:12
> +        * css/mediaControls.css:
> +        (video::-webkit-media-text-track-display): Adjusted text color.

Please say why the color was changed.

> Source/WebCore/html/shadow/MediaControlElements.cpp:1203
> +    if (!toParentMediaElement(this)->isVideo())
> +        return;
> +
> +    // 2. Let video be the media element or other playback mechanism.
> +    HTMLVideoElement* video = static_cast<HTMLVideoElement*>(toParentMediaElement(this));

Nit: you can avoid one of the calls to toParentMediaElement() by changing this slightly.

> Source/WebCore/html/shadow/MediaControlElements.cpp:1214
> +    // FIXME (should be copying the media controls box into a transparent box):
> +    if (video->controls())
> +        output.append(video->mediaControls());

Could you achieve the desired effect, keeping the captions above the default controls, by putting the controls and captions in the same box with appropriate alignment (controls always at the bottom, etc)? This way, we could animate the containing box when the controls visibility is changed and the captions and controls will move in sync.

> Source/WebCore/html/shadow/MediaControlElements.cpp:1259
> +        if (cue->getDisplayState().size()) {
> +            ExceptionCode ec;
> +            appendChild(cue->getDisplayState()[0], ec, true);
> +

Calling getDisplayState() twice is unnecessary, you might as well put it into a local.

Why is only the first element in the vector used?

> Source/WebCore/html/track/TextTrackCue.cpp:409
> +Vector<RefPtr<HTMLDivElement> > TextTrackCue::getDisplayState()

I don't think this is a great name for this method, I would never guess that "getDisplayState" returns the display element(s). Maybe "getDisplayNodes()", or "getDisplayTree()", or ...

I don't understand why this returns a Vector of elements. Couldn't it return a single root node with however many child nodes it needs to display the cue?

> Source/WebCore/html/track/TextTrackCue.cpp:412
> +    if (m_displayBoxes.size() || !m_scriptExecutionContext->isDocument())
> +        return m_displayBoxes;

Don't you need to clear m_displayBoxes in TextTrackCue::cueDidChange() so we update when a cue is modified?

> Source/WebCore/html/track/TextTrackCue.cpp:425
> +    // The document tree is the tree of WebVTT Node Objects rooted at nodes.
> +    simpleBox->appendChild(m_documentFragment, ASSERT_NO_EXCEPTION, true);

Won't this allow a arbitrary markup in a cue? What does tc028-unsupported-markup.vtt look like with this?

> LayoutTests/media/track/track-cue-rendering-expected.txt:33
> -EXPECTED (getComputedStyle(textTrackDisplayElement(video, 'container')).fontSize == '12px') OK
> +EXPECTED (getComputedStyle(mediaControlsElement(internals.shadowRoot(video).firstChild, '-webkit-media-text-track-container')).fontSize == '12px') OK

I would rather have specifics about the shadow hierarchy in a helper function so less will have to change if (when) we change it. Can't you use textTrackDisplayElement() by changing it to work when the "id" parameter is missing and call "textTrackDisplayElement(video)" here?

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



More information about the webkit-unassigned mailing list