[Webkit-unassigned] [Bug 53020] [Meta] Convert HTMLMediaElement to use the new shadow DOM

[bugzilla-daemon at webkit.org]

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





--- Comment #17 from Eric Carlson <eric.carlson at apple.com>  2011-04-08 11:08:06 PST ---
(From update of attachment 88736)
Nice cleanup!

View in context: https://bugs.webkit.org/attachment.cgi?id=88736&action=review

> Source/WebCore/ChangeLog:16
> +        are now using a set of higher fidelity set of hooks that notify MediaControls

Nit: you probably only want one "set" here

> Source/WebCore/html/shadow/MediaControls.cpp:138
> +    // FIXME: ONLY APPEND WHEN SUPPORTS SEEKBACK

Bug number (http://webkit.org/b/57163 maybe)?

> Source/WebCore/html/shadow/MediaControls.cpp:143
> +    // FIXME: ONLY APPEND WHEN SUPPORTS SEEKFORWARD

Ditto.

> Source/WebCore/html/shadow/MediaControls.cpp:158
> +    RefPtr<MediaControlFullscreenButtonElement> fullScreenButton = MediaControlFullscreenButtonElement::create(mediaElement);
> +    controls->m_fullScreenButton = fullScreenButton.get();
> +    panel->appendChild(fullScreenButton.release(), ec, true);

Not all movies/themes support fullscreen, so it is probably worth a "FIXME:" and a bug number here too.

> Source/WebCore/html/shadow/MediaControls.cpp:264
> +    // FIXME: Make more efficient.

Might as well write up a bug now and include the number here.

> Source/WebCore/html/shadow/MediaControls.cpp:366
> +void MediaControls::reportedError()
>  {
> -    return m_controlsShadowRoot ? m_controlsShadowRoot->renderBox() : 0;
> -}
> -
> -void MediaControls::updateControlVisibility()
> -{
    SNIP
> +    if (m_volumeSliderContainer)
> +        m_volumeSliderContainer->hide();
> +    if (m_toggleClosedCaptionsButton && !page->theme()->hasOwnDisabledStateHandlingFor(MediaToggleClosedCaptionsButtonPart))
> +        m_toggleClosedCaptionsButton->hide();
> +}

Is there any reason to not hide the fullscreen button too?

> Source/WebCore/html/shadow/MediaControls.cpp:373
> +    if (m_mediaElement->supportsFullscreen())
> +        m_fullScreenButton->show();
> +    else
> +        m_fullScreenButton->hide();

Why do this when the network state changes? The availability of state-based controls (eg. fullscreen, captions, volume) is based on media features so it might be better to do this when the readyState changes.

> Source/WebCore/html/shadow/MediaControls.cpp:385
> +void MediaControls::loadedMetadata()
> +{
> +    if (m_statusDisplay)
> +        m_statusDisplay->hide();
>  
> -    m_opacityAnimationFrom = animateFrom;
> -    m_opacityAnimationTo = animateTo;
> +    reset();
> +}

It might be more useful to have a generic "readyStateChanged" function.

> Source/WebCore/rendering/MediaControlElements.cpp:87
> +    // FIXME: Make more efficient.

Bug number?

> Source/WebCore/rendering/MediaControlElements.cpp:94
> +    // FIXME: Make more efficient.

Ditto.

> Source/WebCore/rendering/MediaControlElements.cpp:305
> +    style()->setProperty(displayString(), "none", ec);

Is there any reason to not use a static String for "none"?

> Source/WebCore/rendering/MediaControlElements.cpp:633
> +MediaControlTimelineElement::MediaControlTimelineElement(HTMLMediaElement* mediaElement, MediaControls* controls)
>      : MediaControlInputElement(mediaElement, MediaSlider)
> +    , m_controls(controls)
>  {
> +    setAttribute(precisionAttr, "float");
>  }

It’s not a good idea to call functions like this from inside the constructor when the object hasn’t been adopted by its first owner. It can lead to RefCounted assertions if anything ref/derefs. We used to do this throughout the media controls classes, but Darin fixed them a while ago. 

It looks like you already do this in the create() function anyway.

> Source/WebCore/rendering/MediaControlElements.cpp:664
> +        // FIXME: This is fired 3 times on every click. We should not be doing that.

Bug number?

> Source/WebCore/rendering/MediaControlElements.cpp:701
>  inline MediaControlVolumeSliderElement::MediaControlVolumeSliderElement(HTMLMediaElement* mediaElement)
>      : MediaControlInputElement(mediaElement, MediaVolumeSlider)
>  {
> +    setAttribute(precisionAttr, "float");
> +    setAttribute(maxAttr, "1");
>  }

Shouldn't do this in the constructor.

> Source/WebCore/rendering/MediaControlElements.h:79
> +    // FIXME: These should be part of some InlineStylableElement.

Bug number?

> Source/WebCore/rendering/MediaControlElements.h:157
> +    // FIXME: These should be part of some InlineStylableElement.

Ditto.

> Source/WebCore/rendering/MediaControlElements.h:229
> +    // FIXME: PERHAPS MAKE EXPLICIT PLAY/PAUSE CALLS.

Ditto.

> Source/WebCore/rendering/RenderMedia.cpp:54
> +// FIXME: Move to RenderVideo
>  HTMLMediaElement* RenderMedia::mediaElement() const

Does this really need to move to RenderVideo? Bug number of so please.

> Source/WebCore/rendering/RenderMedia.cpp:91
> +void RenderMedia::paint(PaintInfo& paintInfo, int tx, int ty)
>  {
> -    m_controls->update();
> +    RenderImage::paint(paintInfo, tx, ty);
>  }

What does this do?

> Source/WebCore/rendering/RenderThemeMac.mm:-1998
> -    case MediaRewindButtonPart:
> -        if (mediaElement->isFullscreen())
> -            return mediaElement->movieLoadType() == MediaPlayer::LiveStream 
> -                || mediaElement->movieLoadType() == MediaPlayer::StoredStream;
> -    case MediaSeekForwardButtonPart:
> -    case MediaSeekBackButtonPart:
> -        if (mediaElement->isFullscreen())
> -            return mediaElement->movieLoadType() != MediaPlayer::StoredStream 
> -                && mediaElement->movieLoadType() != MediaPlayer::LiveStream;

This needs to be handled somewhere.

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