Bug 53020

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

(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 :)

(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 ;-)

Created attachment 82545 [details] WIP: Mostly working.

(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.

Created attachment 83849 [details] WIP: Progress checkpoint.

(In reply to comment #6) > Created an attachment (id=83849) [details] > WIP: Progress checkpoint. Need to implement proper relatedTarget retargeting before going further...

Created attachment 84627 [details] WIP: Another checkpoint.

Comment on attachment 84627 [details] WIP: Another checkpoint. Didn't mean to mark for review.

Created attachment 85053 [details] WIP: Another checkpoint.

Created attachment 85689 [details] Another checkpoint.

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

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

(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.

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

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.

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.

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.

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 :)

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.

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

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

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

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

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

(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

(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.