Bug 53020 - [Meta] Convert HTMLMediaElement to use the new shadow DOM
: [Meta] Convert HTMLMediaElement to use the new shadow DOM
Status: RESOLVED FIXED
: WebKit
HTML DOM
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
: 53122 53190 53245 53252 53314 54356 55006 55065 55099 55152 55157 55515 55614 55722 55972 56038 56044 56473 56498 56969 58203 58204 58205
: 48698 49027 56573 57163 58157 58163 58250 58345 58347 58952
  Show dependency treegraph
 
Reported: 2011-01-24 09:51 PST by
Modified: 2011-04-19 18:01 PST (History)


Attachments
WIP: Mostly working. (77.42 KB, patch)
2011-02-15 16:05 PST, Dimitri Glazkov (Google)
no flags Review Patch | Details | Formatted Diff | Diff
WIP: Progress checkpoint. (98.12 KB, patch)
2011-02-25 11:25 PST, Dimitri Glazkov (Google)
no flags Review Patch | Details | Formatted Diff | Diff
WIP: Another checkpoint. (85.24 KB, patch)
2011-03-03 14:21 PST, Dimitri Glazkov (Google)
no flags Review Patch | Details | Formatted Diff | Diff
WIP: Another checkpoint. (83.26 KB, patch)
2011-03-08 09:13 PST, Dimitri Glazkov (Google)
no flags Review Patch | Details | Formatted Diff | Diff
Another checkpoint. (78.58 KB, patch)
2011-03-14 10:16 PST, Dimitri Glazkov (Google)
no flags Review Patch | Details | Formatted Diff | Diff
WIP: Checkpoint after more whittling away. (75.42 KB, patch)
2011-03-22 10:13 PST, Dimitri Glazkov (Google)
no flags Review Patch | Details | Formatted Diff | Diff
Nearly ready for landing. Bake on EWSs. (93.39 KB, patch)
2011-04-07 16:58 PST, Dimitri Glazkov (Google)
no flags Review Patch | Details | Formatted Diff | Diff
Put your seats in the upright position. (93.04 KB, patch)
2011-04-08 15:50 PST, Dimitri Glazkov (Google)
eric.carlson: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-01-24 09:51:49 PST
A master bug to track progress on refactoring audio/video to use the new shadow DOM.
------- Comment #1 From 2011-01-24 22:30:53 PST -------
This looks not to depend, but related to Bug 53001 (what's I'm working now.)
------- Comment #2 From 2011-01-25 07:37:34 PST -------
(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 :)
------- Comment #3 From 2011-01-25 18:53:22 PST -------
(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 ;-)
------- Comment #4 From 2011-02-15 16:05:52 PST -------
Created an attachment (id=82545) [details]
WIP: Mostly working.
------- Comment #5 From 2011-02-15 16:07:21 PST -------
(In reply to comment #4)
> Created an attachment (id=82545) [details] [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.
------- Comment #6 From 2011-02-25 11:25:22 PST -------
Created an attachment (id=83849) [details]
WIP: Progress checkpoint.
------- Comment #7 From 2011-02-25 11:26:04 PST -------
(In reply to comment #6)
> Created an attachment (id=83849) [details] [details]
> WIP: Progress checkpoint.

Need to implement proper relatedTarget retargeting before going further...
------- Comment #8 From 2011-03-03 14:21:10 PST -------
Created an attachment (id=84627) [details]
WIP: Another checkpoint.
------- Comment #9 From 2011-03-03 14:31:23 PST -------
(From update of attachment 84627 [details])
Didn't mean to mark for review.
------- Comment #10 From 2011-03-08 09:13:49 PST -------
Created an attachment (id=85053) [details]
WIP: Another checkpoint.
------- Comment #11 From 2011-03-14 10:16:05 PST -------
Created an attachment (id=85689) [details]
Another checkpoint.
------- Comment #12 From 2011-03-22 10:13:46 PST -------
Created an attachment (id=86469) [details]
WIP: Checkpoint after more whittling away.
------- Comment #13 From 2011-04-07 16:58:34 PST -------
Created an attachment (id=88736) [details]
Nearly ready for landing. Bake on EWSs.
------- Comment #14 From 2011-04-07 17:00:42 PST -------
(In reply to comment #13)
> Created an attachment (id=88736) [details] [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.
------- Comment #15 From 2011-04-07 19:04:17 PST -------
Attachment 88736 [details] did not build on win:
Build output: http://queues.webkit.org/results/8347819
------- Comment #16 From 2011-04-08 09:05:04 PST -------
(From update of attachment 88736 [details])
will fix Win. Also need to plumb hooks for enter/exit full screen.
------- Comment #17 From 2011-04-08 11:08:06 PST -------
(From update of attachment 88736 [details])
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.
------- Comment #18 From 2011-04-08 13:50:18 PST -------
(From update of attachment 88736 [details])
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.
------- Comment #19 From 2011-04-08 14:02:15 PST -------
(From update of attachment 88736 [details])
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 :)
------- Comment #20 From 2011-04-08 15:45:42 PST -------
(From update of attachment 88736 [details])
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.
------- Comment #21 From 2011-04-08 15:50:43 PST -------
Created an attachment (id=88888) [details]
Put your seats in the upright position.
------- Comment #22 From 2011-04-08 15:56:46 PST -------
(From update of attachment 88888 [details])
All bucked in!
------- Comment #23 From 2011-04-09 19:54:59 PST -------
(In reply to comment #22)
> All bucked in!

Can I buy an "L" please?
------- Comment #24 From 2011-04-09 20:06:01 PST -------
(In reply to comment #23)
> (In reply to comment #22)
> > All bucked in!
> 
> Can I buy an "L" please?

L is for lucky! :)
------- Comment #25 From 2011-04-10 07:57:23 PST -------
Committed r83397: <http://trac.webkit.org/changeset/83397>
------- Comment #27 From 2011-04-10 11:32:34 PST -------
(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.