Bug 53020 – [Meta] Convert HTMLMediaElement to use the new shadow DOM

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Dimitri Glazkov (Google)]

A master bug to track progress on refactoring audio/video to use the new shadow DOM.

[Hajime Morrita]

This looks not to depend, but related to Bug 53001 (what's I'm working now.)

[Dimitri Glazkov (Google)]

(In reply to comment #1)
> This looks not to depend, but related to Bug 53001 (what's I'm working now.)

I was going to keep m_mediaElement and let you get rid of it :)

[Hajime Morrita]

(In reply to comment #2)
> (In reply to comment #1)
> > This looks not to depend, but related to Bug 53001 (what's I'm working now.)
> 
> I was going to keep m_mediaElement and let you get rid of it :)
wow, thanks ;-)

[Dimitri Glazkov (Google)]

Created attachment 82545 [details]
WIP: Mostly working.

[Dimitri Glazkov (Google)]

(In reply to comment #4)
> Created an attachment (id=82545) [details]
> WIP: Mostly working.

This is more of a rewrite rather than a refactoring, I'll probably have to land it in chunks, but first I need to understand how to do it. This patch is part of this understanding.

[Dimitri Glazkov (Google)]

Created attachment 83849 [details]
WIP: Progress checkpoint.

[Dimitri Glazkov (Google)]

(In reply to comment #6)
> Created an attachment (id=83849) [details]
> WIP: Progress checkpoint.

Need to implement proper relatedTarget retargeting before going further...

[Dimitri Glazkov (Google)]

Created attachment 84627 [details]
WIP: Another checkpoint.

[Dimitri Glazkov (Google)]

Comment on attachment 84627 [details]
WIP: Another checkpoint.

Didn't mean to mark for review.

[Dimitri Glazkov (Google)]

Created attachment 85053 [details]
WIP: Another checkpoint.

[Dimitri Glazkov (Google)]

Created attachment 85689 [details]
Another checkpoint.

[Dimitri Glazkov (Google)]

Created attachment 86469 [details]
WIP: Checkpoint after more whittling away.

[Dimitri Glazkov (Google)]

Created attachment 88736 [details]
Nearly ready for landing. Bake on EWSs.

[Dimitri Glazkov (Google)]

(In reply to comment #13)
> Created an attachment (id=88736) [details]
> Nearly ready for landing. Bake on EWSs.

This is not a patch for review -- still cleaning up various bits. Just want to let the EWS to get a whiff of it.

Yes, it's a whopper. And I apologize, I attempted to whittle it down as much as I could. Eric, I would really like some of your time tomorrow to go over it and hopefully decorate the tree with it.

[Build Bot]

Attachment 88736 [details] did not build on win:
Build output: http://queues.webkit.org/results/8347819

[Dimitri Glazkov (Google)]

Comment on attachment 88736 [details]
Nearly ready for landing. Bake on EWSs.

will fix Win. Also need to plumb hooks for enter/exit full screen.

[Eric Carlson]

Comment on attachment 88736 [details]
Nearly ready for landing. Bake on EWSs.

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.

[Dimitri Glazkov (Google)]

Comment on attachment 88736 [details]
Nearly ready for landing. Bake on EWSs.

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

Done.

>> Source/WebCore/html/shadow/MediaControls.cpp:138
>> +    // FIXME: ONLY APPEND WHEN SUPPORTS SEEKBACK
> 
> Bug number (http://webkit.org/b/57163 maybe)?

Done.

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

Done.

>> Source/WebCore/html/shadow/MediaControls.cpp:158
>> +    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.

Done.

>> Source/WebCore/html/shadow/MediaControls.cpp:264
>> +    // FIXME: Make more efficient.
> 
> Might as well write up a bug now and include the number here.

Done. Bug 58157 filed.

>> Source/WebCore/html/shadow/MediaControls.cpp:366

> 
> SNIP

That is a good idea. Hid the fullscreen button.

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

Done.

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

Done.

>> Source/WebCore/rendering/MediaControlElements.cpp:305
>> +    style()->setProperty(displayString(), "none", ec);
> 
> Is there any reason to not use a static String for "none"?

Fixed.

>> Source/WebCore/rendering/MediaControlElements.cpp:633
>>  }
> 
> 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.

DOH! I totally thought I fixed it. Fixed now.

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

Done.

>> Source/WebCore/rendering/MediaControlElements.cpp:701
>>  }
> 
> Shouldn't do this in the constructor.

Sorry. Removed.

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

FIXME removed, this is obsolete now.

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

Done.

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

This was an old idea, comment obsolete now.

>> Source/WebCore/rendering/RenderMedia.cpp:54
>>  HTMLMediaElement* RenderMedia::mediaElement() const
> 
> Does this really need to move to RenderVideo? Bug number of so please.

No, not really -- early ideas. Comment obsolete now, removed :)

>> Source/WebCore/rendering/RenderMedia.cpp:91
>>  }
> 
> What does this do?

This is my debugging code! :) Removed.

[Dimitri Glazkov (Google)]

Comment on attachment 88736 [details]
Nearly ready for landing. Bake on EWSs.

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

>> Source/WebCore/html/shadow/MediaControls.cpp:373
>> +        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.

I am going to investigate this after landing. Feeling skittish about changing the logic now :)

[Dimitri Glazkov (Google)]

Comment on attachment 88736 [details]
Nearly ready for landing. Bake on EWSs.

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

>> Source/WebCore/rendering/RenderThemeMac.mm:-1998
>> -                && mediaElement->movieLoadType() != MediaPlayer::LiveStream;
> 
> This needs to be handled somewhere.

I added enteredFullscreen/exitedFullscreen callbacks and moved the magic there.

[Dimitri Glazkov (Google)]

Created attachment 88888 [details]
Put your seats in the upright position.

[Eric Carlson]

Comment on attachment 88888 [details]
Put your seats in the upright position.

All bucked in!

[Eric Carlson]

(In reply to comment #22)
> All bucked in!

Can I buy an "L" please?

[Dimitri Glazkov (Google)]

(In reply to comment #23)
> (In reply to comment #22)
> > All bucked in!
> 
> Can I buy an "L" please?

L is for lucky! :)

[Dimitri Glazkov (Google)]

Committed r83397: <http://trac.webkit.org/changeset/83397>

[Jessie Berlin]

(In reply to comment #25)
> Committed r83397: <http://trac.webkit.org/changeset/83397>

It looks like this change broke /fast/layers/video-layer.html on Windows:

http://build.webkit.org/builders/Windows%207%20Release%20%28Tests%29/builds/11438
http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r83397%20(11439)/fast/layers/video-layer-pretty-diff.html

[Dimitri Glazkov (Google)]

(In reply to comment #26)
> (In reply to comment #25)
> > Committed r83397: <http://trac.webkit.org/changeset/83397>
> 
> It looks like this change broke /fast/layers/video-layer.html on Windows:
> 
> http://build.webkit.org/builders/Windows%207%20Release%20%28Tests%29/builds/11438
> http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r83397%20(11439)/fast/layers/video-layer-pretty-diff.html

yup, working on fixing.