Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1156993013:
New media playback UI. (Closed)
|
Created:
5 years, 6 months ago by liberato (no reviews please) Modified:
5 years, 4 months ago CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-html_chromium.org, blink-reviews-paint_chromium.org, blink-reviews-rendering, blink-reviews-style_chromium.org, dglazkov+blink, dshwang, eae+blinkwatch, eric.carlson_apple.com, feature-media-reviews_chromium.org, gasubic, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, rwlbuis, nessy, slimming-paint-reviews_chromium.org, vcarbune.chromium, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionOptionally replace media playback UI (Off by default)
This CL adds a new media playback UI (crbug.com/446450), hidden
behind RuntimeEnabledFeatures::newMediaPlaybackUi . By default,
the behavior should be unchanged.
https://codereview.chromium.org/1266813002 enables this UI as the
default for clank only. To see the new clank UI on other platforms,
run with --enable-blink-features=newMediaPlaybackUi . The desktop
UI is still being revised.
This CL does not rebaseline the layout tests since the feature is
off by default. Once the desktop UI is finalized in a future CL,
the runtime feature and the code for the old UI will be removed.
BUG=446350, 488625, 487344
Screenshots can be found here: https://code.google.com/p/chromium/issues/detail?id=446350#c74
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200116
Patch Set 1 #Patch Set 2 : Cleaned up comments, etc. #Patch Set 3 : Rebased. #Patch Set 4 : Turned off New UI as default #Patch Set 5 : Turned off debug unless new ui is enabled. #Patch Set 6 : Fixed volume slider in current UI case. #Patch Set 7 : Fixed clobbered assets. #Patch Set 8 : Updated slider paint #Patch Set 9 : updated button paint #Patch Set 10 : Rebased. #Patch Set 11 : Updated for new GraphicsContext changes. #Patch Set 12 : Removed debug rectangles. #Patch Set 13 : Fixed disabled player state. #Patch Set 14 : Fixed display of zoom slider and narrow players. #Patch Set 15 : Fixed tests (thanks, trybots!) and duration display. #
Total comments: 6
Patch Set 16 : New volume icons. #Patch Set 17 : Rebased. #
Total comments: 34
Patch Set 18 : manual rebaseline. #
Total comments: 62
Patch Set 19 : addressed (some) cl feedback. #Patch Set 20 : fixed some tests from previous CL. #
Total comments: 111
Patch Set 21 : lots of cl feedback, turned feature to off by default. #Patch Set 22 : fixed broken unit test, rebaselined another. #Patch Set 23 : fixed two test failures -- need tryOverlayCast after all :) #Patch Set 24 : cleaned up volume / mute hiding logic. #
Total comments: 14
Patch Set 25 : Moved allow* to Internals.idl. #Patch Set 26 : cl feedback, tried to fix weird linker error. #Patch Set 27 : added websettings, added white background to overlay cast, updated overlay cast behavior. #Patch Set 28 : git rebase-no-matic. #Patch Set 29 : rebased, again. #Patch Set 30 : Fixed my own broken unit test. They really work! #
Total comments: 5
Patch Set 31 : cl feedback. #Patch Set 32 : minor decrufting. #
Total comments: 53
Patch Set 33 : added batched updates for control visibility, cl feedback #Patch Set 34 : Added batched updates for controls visibility, more cl feedback. #Patch Set 35 : fixed some failing tests. #Patch Set 36 : fixed more unit tests, rebased. #Patch Set 37 : only call clientWidth() from reset to try to fix layout tests. #Patch Set 38 : switched to click special case in slider thumb. #Patch Set 39 : removed slider click handling, since deferring desktop. #Patch Set 40 : removed panelWidth check and unused method declaration. #Patch Set 41 : Added WebRuntimeFeature, updated CL description #Patch Set 42 : actually got the other CL number right... #
Total comments: 11
Patch Set 43 : cl feedback, rebased. #
Total comments: 3
Patch Set 44 : made batch counter static, moved NON_COPYABLE, made miniumWidth 48 in all cases. #Patch Set 45 : ...because i compiled release. #
Total comments: 10
Patch Set 46 : deflaked some tests. #Patch Set 47 : re-added a few deflaked tests. #Patch Set 48 : Rebased to land.... Of course i'm serious, and don't call me Shirley. #Patch Set 49 : Modified css. #
Messages
Total messages: 97 (27 generated)
liberato@chromium.org changed reviewers: + philipj@opera.com
liberato@chromium.org changed reviewers: + renganathan@chromium.org, tkent@chromium.org
Hello This CL provides an new media playback UI (see BUG). It is off by default. There will be some more iteration on it, but the code is able to stand on its own now. Layout tests are not yet rebased. I appreciate any feedback that you provide. Thanks Frank
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
Nice work! Can you post some screen shots to the bugs of the new controls? Ditto for linking to that comment from the CL description here.
On 2015/06/08 at 21:39:17, dalecurtis wrote: > Nice work! Can you post some screen shots to the bugs of the new controls? Ditto for linking to that comment from the CL description here. Please make sure to optimize the new pngs otherwise it will cause a big bump in binary size. You can use src/tools/resources/optimize-png-files.sh
On 2015/06/08 21:39:17, DaleCurtis wrote: > Nice work! Can you post some screen shots to the bugs of the new controls? Ditto > for linking to that comment from the CL description here. Done, thanks for pointing out the omission. -fl
On 2015/06/08 21:59:29, aurimas wrote: > On 2015/06/08 at 21:39:17, dalecurtis wrote: > > Nice work! Can you post some screen shots to the bugs of the new controls? > Ditto for linking to that comment from the CL description here. > > Please make sure to optimize the new pngs otherwise it will cause a big bump in > binary size. You can use src/tools/resources/optimize-png-files.sh Thanks for the feedback. I'll double check this before landing. I did try that script earlier, but it did some weird stuff. Seems some dep or other is broken on my ubuntu box. I ended up running pngcrush manually. One of them is still ~10K, so maybe i missed it. thanks -fl
Looks really nice! I do worry about all the magic values everywhere, is there any way we can avoid that? Or is the answer we should be using blink-in-js? https://codereview.chromium.org/1156993013/diff/270001/Source/core/css/CSSDef... File Source/core/css/CSSDefaultStyleSheets.cpp (right): https://codereview.chromium.org/1156993013/diff/270001/Source/core/css/CSSDef... Source/core/css/CSSDefaultStyleSheets.cpp:160: String mediaRules = loadResourceAsASCIIString( Are you using clang-format? This formatting seems off. https://codereview.chromium.org/1156993013/diff/270001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/270001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:151: #if OS(ANDROID) Seems like this should be more a function of viewport size? https://codereview.chromium.org/1156993013/diff/270001/Source/core/paint/Medi... File Source/core/paint/MediaControlsPainter.cpp (right): https://codereview.chromium.org/1156993013/diff/270001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:47: // Current UI slider thumbs sizes. Do we want to hard code all these sizes or can we get them from the assets?
thanks for the feedback. overarching reply: blink-in-js looks better all the time. -fl https://codereview.chromium.org/1156993013/diff/270001/Source/core/css/CSSDef... File Source/core/css/CSSDefaultStyleSheets.cpp (right): https://codereview.chromium.org/1156993013/diff/270001/Source/core/css/CSSDef... Source/core/css/CSSDefaultStyleSheets.cpp:160: String mediaRules = loadResourceAsASCIIString( On 2015/06/11 00:54:28, DaleCurtis wrote: > Are you using clang-format? This formatting seems off. i didn't. i will, thanks. https://codereview.chromium.org/1156993013/diff/270001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/270001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:151: #if OS(ANDROID) On 2015/06/11 00:54:29, DaleCurtis wrote: > Seems like this should be more a function of viewport size? it's part of the spec -- hide this on android, since the volume buttons are readily available. now, maybe we want to keep the mute button -- that's a good question. https://codereview.chromium.org/1156993013/diff/270001/Source/core/paint/Medi... File Source/core/paint/MediaControlsPainter.cpp (right): https://codereview.chromium.org/1156993013/diff/270001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:47: // Current UI slider thumbs sizes. On 2015/06/11 00:54:29, DaleCurtis wrote: > Do we want to hard code all these sizes or can we get them from the assets? unfortunately, we can't get them from the assets that i know of. they're big enough to look nice on higher dpi devices. so maybe we could just hardcode the conversion factor, which is the same for all of them, but not sure that's better. it's fewer constants but more dependencies.
thanks for the feedback. overarching reply: blink-in-js looks better all the time. -fl
jochen@chromium.org changed reviewers: + jochen@chromium.org
could you please send an intent to implement mail for this feature? also, there should be a regular launch bug, can you please include the bug # in the CL description?
I've made a build with this and it looks quite nice! Is the plan to now have identical controls for Desktop and Android? If that's the long-term plan, then the empty Source/core/css/mediaControlsAndroidNew.css will serve no purpose. My biggest concern with all of this is having both systems side-by-side. The new CSS is based on the old CSS, so there will be cases where both have to be updated. This was a pain before I merged multiple CSS files into the current state. So, what's the timeline for shipping this? I could give feedback on the look and feel (a bit much padding?) but I presume this was designed by someone in a UX team at Google? https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... File Source/core/css/mediaControlsNew.css (left): https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:30: audio:not([controls]) { Oops, when all of this is done make sure to base it off a fresh copy of mediaControls.css https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... File Source/core/css/mediaControlsNew.css (right): https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:49: max-width: 100%; Simply removing max-width should have the same effect. https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:50: height: 48.00px; Why the extra precision? https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:75: border-radius: 0px; Simply removing any mention of border-radius should do the trick. https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:141: fargin: 0; fargin? https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:145: width: 100%; This may result in a problem I had previously. If the clickable area for the overlay play button goes right up to the controls panel, trying to click stuff in the controls panel may result in a disambiguation zoom, which isn't great. Is the intention of this change to allow clicking anywhere to play? https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:188: margin: 0 0 0 0; Just margin: 0 means the same thing. https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:208: font-family: Noto Sans, Serif; Is Noto Sans part of Chrome or will this only work on Android? Serif (should be "serif"?) as a fallback for Noto Sans seems a bit strange. https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:210: font-weight: normal; I think normal is the default. https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:218: text-shadow: none; Isn't this the default? https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:233: * the left to pad the leading "/" */ This seems like the kind of thing that might result in a 1px error that typography geeks will notice. Did you look into simply having a single text box for the time and just having spaces around the slash? https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:238: font-family: Noto Sans; No fallback here? Maybe the font-family should be set at a higher level? https://codereview.chromium.org/1156993013/diff/310001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/310001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:151: #if OS(ANDROID) This could be done in CSS instead. https://codereview.chromium.org/1156993013/diff/310001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:566: // Hide all controls that don't fit. This seems useful, but will hideControlsForSpace() be called when the size of the video element is changed by CSS? I don't know, but it seems like there should be ways of doing this same thing in CSS, probably flexbox. Maybe you can ask Tab Atkins if this is already possible? https://codereview.chromium.org/1156993013/diff/310001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:593: // TODO(liberato): get confirmation from spec on these values. Do you mean the HTML spec? That doesn't say what the size of anything in the controls should be. https://codereview.chromium.org/1156993013/diff/310001/Source/core/paint/Medi... File Source/core/paint/MediaControlsPainter.h (right): https://codereview.chromium.org/1156993013/diff/310001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.h:54: static void adjustMediaSliderThumbPaintSize(const IntRect&, const ComputedStyle&, IntRect& rectOut); Can't this simply return an IntRect by value? Even with out C++11 move semantics there's a good chance it would fall under https://en.wikipedia.org/wiki/Return_value_optimization
philipj: thanks for the feedback. side by side css: the plan is for the *new.css files to replace the originals, so it should be as it started once the dust settles. empty android css: yes, the goal is for them to be the same. if we actually achieve that, then i'll deprecate the android UA style sheets. timeline: we were aiming for M45, but looking at the calendar, it is likely too late at this point. branch is next week, i think. i thought there was another week in there :( look and feel: feedback is welcome. i'll get you in touch with the UX designer. jochen: thanks for the feedback. i'll take care of both of these. https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... File Source/core/css/mediaControlsNew.css (left): https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:30: audio:not([controls]) { On 2015/06/17 12:52:24, philipj wrote: > Oops, when all of this is done make sure to base it off a fresh copy of > mediaControls.css Acknowledged. https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... File Source/core/css/mediaControlsNew.css (right): https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:49: max-width: 100%; On 2015/06/17 12:52:24, philipj wrote: > Simply removing max-width should have the same effect. Done. https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:50: height: 48.00px; On 2015/06/17 12:52:24, philipj wrote: > Why the extra precision? generated automatically. i'll remove. https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:75: border-radius: 0px; On 2015/06/17 12:52:24, philipj wrote: > Simply removing any mention of border-radius should do the trick. Done. https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:141: fargin: 0; On 2015/06/17 12:52:24, philipj wrote: > fargin? "far margin" - sets margin on elements in your neighbor's browser. it's new. fixed. https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:145: width: 100%; On 2015/06/17 12:52:24, philipj wrote: > This may result in a problem I had previously. If the clickable area for the > overlay play button goes right up to the controls panel, trying to click stuff > in the controls panel may result in a disambiguation zoom, which isn't great. Is > the intention of this change to allow clicking anywhere to play? good point. that's the intention. i can shrink it a bit and top align. is there a better way? https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:188: margin: 0 0 0 0; On 2015/06/17 12:52:24, philipj wrote: > Just margin: 0 means the same thing. left over from tinkering. fixed. https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:208: font-family: Noto Sans, Serif; On 2015/06/17 12:52:24, philipj wrote: > Is Noto Sans part of Chrome or will this only work on Android? Serif (should be > "serif"?) as a fallback for Noto Sans seems a bit strange. i thought it was part of chrome, but upon checking again, it seems less clear. i'll check. https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:210: font-weight: normal; On 2015/06/17 12:52:24, philipj wrote: > I think normal is the default. Done. https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:218: text-shadow: none; On 2015/06/17 12:52:24, philipj wrote: > Isn't this the default? yeah. not sure why it was specified before. i'll see if i can prune it down. https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:233: * the left to pad the leading "/" */ On 2015/06/17 12:52:24, philipj wrote: > This seems like the kind of thing that might result in a 1px error that > typography geeks will notice. Did you look into simply having a single text box > for the time and just having spaces around the slash? i'd prefer that. but, the / is supposed to be anchored, and the font is variable width. i couldn't figure out how to do that without left aligning it. https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:238: font-family: Noto Sans; On 2015/06/17 12:52:24, philipj wrote: > No fallback here? Maybe the font-family should be set at a higher level? added to my todo list. https://codereview.chromium.org/1156993013/diff/310001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/310001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:151: #if OS(ANDROID) On 2015/06/17 12:52:24, philipj wrote: > This could be done in CSS instead. good point. https://codereview.chromium.org/1156993013/diff/310001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:566: // Hide all controls that don't fit. On 2015/06/17 12:52:25, philipj wrote: > This seems useful, but will hideControlsForSpace() be called when the size of > the video element is changed by CSS? I don't know, but it seems like there > should be ways of doing this same thing in CSS, probably flexbox. Maybe you can > ask Tab Atkins if this is already possible? thanks. i've cc'd you on the email. resizing the video player probably won't call this again. https://codereview.chromium.org/1156993013/diff/310001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:593: // TODO(liberato): get confirmation from spec on these values. On 2015/06/17 12:52:25, philipj wrote: > Do you mean the HTML spec? That doesn't say what the size of anything in the > controls should be. confusing comment on my part. i meant the new ui spec. will fix. https://codereview.chromium.org/1156993013/diff/310001/Source/core/paint/Medi... File Source/core/paint/MediaControlsPainter.h (right): https://codereview.chromium.org/1156993013/diff/310001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.h:54: static void adjustMediaSliderThumbPaintSize(const IntRect&, const ComputedStyle&, IntRect& rectOut); On 2015/06/17 12:52:25, philipj wrote: > Can't this simply return an IntRect by value? Even with out C++11 move semantics > there's a good chance it would fall under > https://en.wikipedia.org/wiki/Return_value_optimization possibly, but it will end up being functionally the same thing in the best case. to be sure, i fiddled around with clang. when it doesn't have the method body to inline, it seems to do better with the explicit out param. when it does have the method body, as it will in this case, it is more aggressive and they end up being almost identical.
Stumbled upon this, might be relevant to the font question: https://codereview.chromium.org/1130613002
this new patch addresses some of the CL feedback. Still need to diff against most recent mediaControls.css, and still trying to ensure that Noto Sans is available. Changes to element width probably still don't work right. Overlay play button now has a 10px bottom margin. Fonts have better fallbacks.
On 2015/06/19 21:52:41, liberato wrote: > this new patch addresses some of the CL feedback. Still need to diff against > most recent mediaControls.css, and still trying to ensure that Noto Sans is > available. Changes to element width probably still don't work right. > > Overlay play button now has a 10px bottom margin. Fonts have better fallbacks. changes to element width dynamically via JS definitely do not hide / unhide controls properly. will fix.
This version gets a resize event for the HTMLMediaElement, and updates controls visibility based on player size. I considered modifying a layout for this, but that seemed like a step further away from JS. this way, it's still event driven.
hi all i've merged mediaControls.css from ToT (one block was added, one was subsumed into existing changes). the diff looks weird (FontData in the css file?) but i think that it's just tool weirdness. The UX team has also given me a list of fonts that doesn't require us to bundle Noto Sans. So, that should also be good. i'm going to check to be sure that there's no outstanding feedback, but i believe that it's all addressed. If anybody has any more comments, I'd appreciate them. especially the magic word variety :) thanks -fl
after more thought, there will be another small update to this. It will convert the RuntimeEnabledFlag to "enable old UI" with status=stable. it will make life a lot simpler to land this, and rebaseline the tests. three CLs instead of four, i think. thanks -fl
fixed some broken tests, and decided against the runtime features polarity switch. it was just too weird.
here's the almost most recent stuff. there are changes in-flight to simplify the control hiding logic at the end of MediaControls.cpp -- it ended up being a simple function, via a long journey. i'm also removing some of the test changes - the setTimeout() calls can be removed once i get property updates to happen synchronously again. philipj -- any suggestions on the repaint / relayout are most welcome. there are no layout tests for the control hiding logic yet. https://codereview.chromium.org/1156993013/diff/530001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/530001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:639: void MediaControls::updateControlsVisibilityForSpace() I'll simplify this. it's the same as "add things that fit greedily starting from the most important". i've also moved some of this state into MediaControlElement, so that this is all much easier.
here's the almost most recent stuff. there are changes in-flight to simplify the control hiding logic at the end of MediaControls.cpp -- it ended up being a simple function, via a long journey. i'm also removing some of the test changes - the setTimeout() calls can be removed once i get property updates to happen synchronously again. philipj -- any suggestions on the repaint / relayout are most welcome. there are no layout tests for the control hiding logic yet. https://codereview.chromium.org/1156993013/diff/530001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/530001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:639: void MediaControls::updateControlsVisibilityForSpace() I'll simplify this. it's the same as "add things that fit greedily starting from the most important". i've also moved some of this state into MediaControlElement, so that this is all much easier.
here's the almost most recent stuff. there are changes in-flight to simplify the control hiding logic at the end of MediaControls.cpp -- it ended up being a simple function, via a long journey. i'm also removing some of the test changes - the setTimeout() calls can be removed once i get property updates to happen synchronously again. philipj -- any suggestions on the repaint / relayout are most welcome. there are no layout tests for the control hiding logic yet.
simplified the control hiding logic. added a sync update to show / hide controls rather than always being event-based. this lets us update properties immediately, and is closer to what MediaControls used to do. i restored some of the tests to their previous state. some still include setTimeout(..., 0). i am working through these.
This version fixes a test crash in virtual/android/fullscreen/video-scrolled-iframe.html . While stopping DOM objects, HTMLMediaElement tries to refresh cast button visibility. in the new UI, this triggers a relayout, which crashes. unsure why yet. reverted the behavior to match the old UI -- show / hide controls but don't trigger a relayout (via clientWidth() in updateControlsVisibilityForSpace()). local repro goes away when i do this. i've also posted a blank CL to check the flakes against it -- seems like 'without patch' doesn't report the flakes, but 'with patch' on a blank CL does. weird. thanks -fl
minor updates, added a unit test to make sure that control drop order doesn't change silently from what it is now. thanks -fl
there is no work in-flight for this CL. everything is pending review feedback and the whim of the trybots. thanks -fl
this addresses the two test failures in the previous revision. the mac failure was due to LayoutThemeMac.cpp missing an 'else' (which previously worked anyway). the windows failure was just an incorrect TestExpectations line. except for test failures and review feedback, i have no updates planned. thanks -fl
fixed the unit test i added -- i replaced console.log with consoleWrite(). failed on mac due to nondeterministic output. no planned updates, unless it fails again. thanks -fl
Patchset #18 (id:330001) has been deleted
Patchset #18 (id:350001) has been deleted
Patchset #18 (id:370001) has been deleted
Patchset #18 (id:390001) has been deleted
Patchset #31 (id:670001) has been deleted
Patchset #30 (id:650001) has been deleted
Patchset #29 (id:630001) has been deleted
Patchset #28 (id:610001) has been deleted
Patchset #27 (id:590001) has been deleted
Patchset #26 (id:570001) has been deleted
Patchset #25 (id:550001) has been deleted
Patchset #24 (id:530001) has been deleted
Patchset #23 (id:510001) has been deleted
Patchset #22 (id:490001) has been deleted
Patchset #21 (id:470001) has been deleted
Patchset #20 (id:450001) has been deleted
Patchset #19 (id:430001) has been deleted
Patchset #18 (id:410001) has been deleted
Patchset #18 (id:690001) has been deleted
fs@opera.com changed reviewers: + fs@opera.com
Some higher-level comments mostly revolving around the "selective visibility" stuff. https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:244: scheduleUpdateControlsVisibilityForSpace(); First refreshCastButtonVisibility will call updateControlsVisibilityForSpace, and then scheduleUpdateControlsVisibilityForSpace will call it again as well as schedule another call, making for 2+1 calls in total... https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:620: updateControlsVisibilityForSpace(); // So that queries work Having a schedule<operation> that also performs <operation> immediately is, well, slightly unconventional. https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.h (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControls.h:113: void scheduleUpdateControlsVisibilityForSpace(); I don't know what the "ForSpace" suffix is supposed to mean, but I don't think it adds much. https://codereview.chromium.org/1156993013/diff/710001/Source/core/layout/Lay... File Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/layout/Lay... Source/core/layout/LayoutObject.cpp:1829: if (m_bitfields.sendResizeEvent() && m_node) { I think it would be better just put this mechanism within MediaControls (using a zero-delay Timer). The actual size-changes for MediaControls will be constrained to the setWidth/setHeight calls in LayoutMedia::layout.
There will be many comments, so I'll send them in batches. In addition to the inline comments, I wonder if anything was intentionally changed around the timeout to hide the controls? When testing this in content_shell on Linux, sometimes dragging the mouse off the bottom edge of the video will cause the controls to stay around for much longer than usual. Perhaps this is because the controls now align with the bottom of the video and there's some bug around the mouseout handling that now triggers more easily? https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/TestExpect... File LayoutTests/TestExpectations (right): https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/TestExpect... LayoutTests/TestExpectations:1574: # Was specific to linux with Pass / ImageOnlyFailure. Removing modifier Perhaps mention this kind of thing in the commit message instead, or add a TODO if you should add [Linux] back after the rebaseline. https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/TestExpect... LayoutTests/TestExpectations:1689: crbug.com/1156993013 compositing/geometry/video-fixed-scrolling.html [ NeedsRebaseline ] I guess this should be 446350? For this and all the tests in this block, could you check if they're actually testing something that depends on the controls, or if the controls attribute can't be removed from the test instead? It's a nuisance to update this many tests for any change to the controls... https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/cont... File LayoutTests/media/controls-volume-slider.html (right): https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/cont... LayoutTests/media/controls-volume-slider.html:17: // Click at 25% of the volume slider, to avoid hitting Perhaps this isn't a new bug, but now that the area is bigger, it's quite noticeable and annoying that one can't click even slightly outside the blue circle to seek. Clicking and then dragging a pixel does work, so it has a glitchy feeling to it. Can the behavior be changed instead of the test? https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/trac... File LayoutTests/media/track/track-cue-rendering-after-controls-added-expected.html (right): https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/trac... LayoutTests/media/track/track-cue-rendering-after-controls-added-expected.html:8: <body> I don't think this change is needed to pass the test? (Many tests omit unnecessary markup in order to bring attention to the bits that are being tested.) https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/trac... File LayoutTests/media/track/track-cue-rendering-after-controls-added.html (right): https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/trac... LayoutTests/media/track/track-cue-rendering-after-controls-added.html:25: // Add the controls. The cue should move to avoid overlap. Defer this Was this test flaky before as well, or what has changed? https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/trac... File LayoutTests/media/track/track-cue-rendering-wider-than-controls.html (right): https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/trac... LayoutTests/media/track/track-cue-rendering-wider-than-controls.html:9: video::-webkit-media-controls-enclosure { A bit odd, but I guess this test can just be removed when ::-webkit-media-* is removed and the max-width in mediaControls.css is no more. https://codereview.chromium.org/1156993013/diff/710001/Source/core/css/CSSDef... File Source/core/css/CSSDefaultStyleSheets.cpp (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/css/CSSDef... Source/core/css/CSSDefaultStyleSheets.cpp:40: #include "core/page/Page.h" Seems to compile without this include. https://codereview.chromium.org/1156993013/diff/710001/Source/core/css/mediaC... File Source/core/css/mediaControlsAndroidNew.css (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/css/mediaC... Source/core/css/mediaControlsAndroidNew.css:3: * Copyright (C) 2009 Apple Inc. All rights reserved. I would keep the copyright header from which this was copied, or use the new smaller one if it wasn't copied. Changing years and adding lines over time is tedious :) https://codereview.chromium.org/1156993013/diff/710001/Source/core/css/mediaC... Source/core/css/mediaControlsAndroidNew.css:31: audio::-webkit-media-controls-mute-button, video::-webkit-media-controls-mute-button { This is contrary to https://codereview.chromium.org/1045103002/ I happen to agree that and we have a patch in Opera for Android to get rid of the mute button, but was this discussed internally? https://codereview.chromium.org/1156993013/diff/710001/Source/core/css/mediaC... File Source/core/css/mediaControlsNew.css (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:58: margin: 0; This was added, does it make a difference? In general there seems to be a whole lot of cruft in this CSS, can you tried to minimize it to just the stuff that is actually needed? Like box-sizing:border-box just below here doesn't seem to make sense, and such is the case every 5-10 lines in this file I think. https://codereview.chromium.org/1156993013/diff/710001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:120: padding: 12px; The amount of whitespace around the mute button looks really excessive, and was pointed out by some people I showed the new controls to internally. It makes it a bit non-obvious that the second slider is related to the volume button at all. Does this match what was intended in the mockups? I'm no designer, but something closer to the amount of space between the play button and the duration text would seem better. https://codereview.chromium.org/1156993013/diff/710001/Source/core/layout/Lay... File Source/core/layout/LayoutThemeMac.mm (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/layout/Lay... Source/core/layout/LayoutThemeMac.mm:1019: adjustMediaSliderThumbSize(style); Why was this change needed? https://codereview.chromium.org/1156993013/diff/710001/Source/core/paint/Medi... File Source/core/paint/MediaControlsPainter.cpp (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:117: // The new UI uses "muted" and "not muted" only. Is this by design? I was expecting the "waves" to follow along when dragging the slider, but they don't. https://codereview.chromium.org/1156993013/diff/710001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:503: return paintMediaButton(paintInfo.context, rect, mediaExitFullscreenButton, !isEnabled); Is there a test that would fail if the enter fullscreen button were always drawn? There have been a few bugs around invalidation for the different button states, and although I think this will work by virtue of MediaControlElement::setDisplayType invalidating, a test to lock it down would be nice. https://codereview.chromium.org/1156993013/diff/710001/Source/platform/Runtim... File Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/1156993013/diff/710001/Source/platform/Runtim... Source/platform/RuntimeEnabledFeatures.in:101: NewMediaPlaybackUi status=stable Since it's enabled by default I don't think the command line switch in https://codereview.chromium.org/1145053006/ is needed, nor should it be necessary to expose this in WebRuntimeFeatures. If there's a problem, it should suffice to flip this flag.
Some comments on the new test. It also looks like more tests and more code to handle video resizing is needed. When I simply resize a page with a video with max-width:100% in content_shell for Linux, the controls aren't updated immediately, and it's possible to get into states where just reloading or hovering will change the layout. https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/vide... File LayoutTests/media/video-controls-drop-order.html (right): https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/vide... LayoutTests/media/video-controls-drop-order.html:1: <html> https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-... https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-... To get rid of <body> you can simply put the test script at the end and not have an init() at all. https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/vide... LayoutTests/media/video-controls-drop-order.html:24: videos=[]; videos is already set to [] https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/vide... LayoutTests/media/video-controls-drop-order.html:26: for(var width = 50; width < 550; width+=50) { Spaces around operators. https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/vide... LayoutTests/media/video-controls-drop-order.html:26: for(var width = 50; width < 550; width+=50) { This will load quite a few videos in parallel. Can you be sure to run this test 1000 times or so locally to make sure it isn't flaky? I think there have been flaky video tests of this sort. https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/vide... LayoutTests/media/video-controls-drop-order.html:55: for (var idx in controlNames) { Since these are plain arrays you could use "for (x of y)" here, seems like that's supported now. (forEach() would also have been an option) https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/vide... LayoutTests/media/video-controls-drop-order.html:59: var state = (control.style['display'] != 'none') ? "seen" : "gone"; Just control.style.display would be be idiomatic, or getComputedStyle() to be more robust against what it is that causes the control be display:none. https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/vide... LayoutTests/media/video-controls-drop-order.html:64: testRunner.notifyDone(); Seems just as well to let the test finish running, so that all of the failures can be seen if it fails on the bots. https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/vide... LayoutTests/media/video-controls-drop-order.html:82: <div id="videoDiv"/> </div> if you want to close the div here, or someone might think that this is XHTML at a glance. You could also do without this by simply appending the videos to document.body
Reviewed somewhat carefully up to MediaControls.cpp https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... File Source/core/html/shadow/MediaControlElementTypes.cpp (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControlElementTypes.cpp:120: int minWidthInPixels = 48; Not sure the local variable explains much in such a short method, just returning the values seems OK. https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControlElementTypes.cpp:123: // we've been shown. Instead, we just memorize 48 except for the Not sure what "memorize" means here, maybe "assume"? https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... File Source/core/html/shadow/MediaControlElementTypes.h (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControlElementTypes.h:83: // These hold the state about whether this control should be shown if OK, I take it the problem is that there are now two different reasons that something can be hidden: 1. Because it makes no sense, e.g. the CC button where there are no captions. 2. Because it won't fit. Keeping show() and hide() exposed and having want() and dontWant() call those internally seems like it might invite mistakes, by code calling show() or hide() directly, and just confusion about how this works. Could the new model be made explicit by having exposed ways of saying that the element isn't needed for the two different reasons, and then handling the actual display state internally based on those two bits? Alternatively, if there's any way for the elements to not have any knowledge about the reason they're being shown or hidden, that would also be cleaner, but I don't know where one would keep track of the state to avoid showing the element again when just one of the bits flips to the "can show" state. https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControlElementTypes.h:91: // this control. Ideally, this would just be shorthand for the css s/css attributes/computed style/ https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:38: #include "public/platform/Platform.h" It doesn't look like this include is needed. https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:95: // +-MediaControlTimelineElement (-webkit-media-controls-timeline) Can you keep it in both places and annotated with {if (!)RuntimeSomethingOrAnother} until the flag is gone? https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:446: void MediaControls::refreshCastButtonVisibilityWithoutLayout() This name doesn't seem right, getBoundingClientRect() may trigger layout. If it really is guaranteed, can it be asserted somehow to calm the skeptical reader? https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.h (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControls.h:72: // can e called while stopping DOM objects, during which a layout s/e/be/ but also see the comment about this not seeming to actually be layout free.
The Chromium-side CL https://codereview.chromium.org/1145053006/ has been removed, is there a new CL? Reviewing this really requires testing some things locally, but fortunately I still have the now-gone CL applied.
Finished reviewing MediaControls.cpp https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... File Source/core/html/shadow/MediaControlElementTypes.h (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControlElementTypes.h:78: private: Move these things to the private section at the bottom instead. If you can remove show() and hide() too, better to just have a single way (public or private) to update the inline style. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControlElementTypes.h:119: bool m_isShown; Does this always equal m_isWanted && m_doesFit? If so remove it. Also append ": 1" to consecutive bools to make them a bitfield. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... File Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControlElements.cpp:311: if (mediaElement().shouldShowControls() && mediaElement().togglePlayStateWillPlay()) Transform this into a single call to setIsWanted, potentially with a local bool isWanted if that makes it more readable. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:94: // +-MediaControlTimelineElement (-webkit-media-controls-timeline) (if !RTE::newMediaPlaybackUi()) Can you use the same style as the above {if mediaControlsOverlayPlayButtonEnabled}? https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:132: if (!RuntimeEnabledFeatures::newMediaPlaybackUiEnabled()) Break out this into a local variable like in ::reset() https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:138: if (!RuntimeEnabledFeatures::newMediaPlaybackUiEnabled()) Transform this into a single setIsWanted() call. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:156: #if OS(ANDROID) Is this actually needed? Should work the same if done in mediaControlsAndroidNew.css I think. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:190: const bool useNewUi = RuntimeEnabledFeatures::newMediaPlaybackUiEnabled(); Also make duration const if you prefer that. (If you happen to know if const on local variable ever makes a difference to the generated code I'd also be curious to know, seems to me like the compiler should be able to tell that it isn't modified with or without it.) https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:216: m_volumeSlider->setIsWanted(false); Ditto. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:220: #if OS(ANDROID) Should also be possible to do with just CSS. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:223: m_volumeSlider->setIsWanted(false); Ditto. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:243: m_fullScreenButton->setIsWanted(true); Ditto. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:400: // If the element is muted, then we want the unmute button. Otherwise, I think that if we're going to change anything about this again, we should just unmute and set volume to 1 when playing using the built-in controls. We don't show the volume slider even though volume can be set to 0, so the current logic seems strange to me, and a waste of pixels in the vast majority of cases. Alternatively, show the mute button only for videos that are actually muted, but of course leave it in after that point as well. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:423: m_toggleClosedCaptionsButton->setIsWanted(true); Ditto. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:468: // without wanting the bar cast button. We depend on the fact s/bar/panel/ or "the button in the controls panel" I'm also not sure what the "We depend on ..." bit is saying, updateControlsVisibilityForSpace() isn't called here. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:472: m_castButton->setIsWanted(false); The mismatch between showOverlayCastButton() and m_castButton->setIsWanted(false) looks odd. Any idea why we can't just call m_overlayCastButton->setIsWanted(true) here? It seems strange to reset the hide timer in a function like this that one should expect to be called to potentially invalidate state, but have no side effect if everything is already in the correct state. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:489: // do NOT updateControls...() here. This comment isn't needed IMHO. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:614: if (panelWidth == 0 || panelWidth == m_panelWidth) Shouldn't m_panelWidth still be updated? Seems simpler to just always have it in sync with layout? https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:618: m_panelWidthChangedTimer.startOneShot(0, FROM_HERE); Given that this is called from LayoutMedia::layout() I take it the reason for this is to avoid some asserts about style invalidation during layout? Still, it seems like doing it on a timer could cause one bad frame to be rendered. fs? My hunch is that the only way to really make this work might be to move the logic to layout somehow. It's true that JS controls couldn't do that, but if the controls were in an iframe and listened to the resize event, I presume that updating the layout in the resize event would not cause one bad frame. At least I hope there's actually a way for web content to do this kind of thing correctly, otherwise our web platform sucks! :) https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:623: int panelWidth = m_panel->clientWidth(); This may trigger a layout, is that intentional? It seems odd to both have LayoutMedia notify about the panel width and to get it like this. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:641: void MediaControls::computeWhichControlsFit(int panelWidth) Seems like this doesn't need an argument, use m_panelWidth? https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:664: for (int i = 0; elements[i]; i++) { for (MediaControlElement* element : elements) at least compiles, and I assume that you could get rid of the trailing 0 in the array using it. That would also make it explicit that none of the elements can be null, as that would just silently terminate the loop with the current code. There are elements that can be null, like m_overlayPlayButton, so it's not really obvious. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:678: if (!m_castButton->isShown()) I think this and above are the only places where isWanted() and isShown() are used, and it looks like it could be replaced by just remembering some state from the loop. It would be nice if the price of ugliness for the cast button hack were localized and we don't have exposed state of the buttons that invites more special casing in other contexts. Also merge into a single setIsWanted() call. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:682: } // else do not change overlay cast button state. Don't need this comment. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.h (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.h:70: void hideCastButtons(); Ought this not be equivalent to refreshCastButtonVisibilityWithoutUpdate() in the context it's called, i.e. when !mediaElement().hasRemoteRoutes()? It seems like in principle it should be enough with just refreshCastButtonVisibility() public, but cleaning up showOverlayCastButton() isn't your job if you don't want it ;) https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.h:116: // For the new UI, hide elements that don't fit, and show those things Remove "For the new UI" or it's sure to be left in long after there's only one UI.
https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:614: if (panelWidth == 0 || panelWidth == m_panelWidth) On 2015/07/09 08:56:10, philipj wrote: > Shouldn't m_panelWidth still be updated? Seems simpler to just always have it in > sync with layout? Yeah, if checking this at all, it's probably better to do it just before the actual controls "layout". https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:618: m_panelWidthChangedTimer.startOneShot(0, FROM_HERE); On 2015/07/09 08:56:10, philipj wrote: > Given that this is called from LayoutMedia::layout() I take it the reason for > this is to avoid some asserts about style invalidation during layout? Still, it > seems like doing it on a timer could cause one bad frame to be rendered. fs? Correct on both accounts AFAIK. > My hunch is that the only way to really make this work might be to move the > logic to layout somehow. Agreed. > It's true that JS controls couldn't do that, but if the > controls were in an iframe and listened to the resize event, I presume that > updating the layout in the resize event would not cause one bad frame. At least > I hope there's actually a way for web content to do this kind of thing > correctly, otherwise our web platform sucks! :) Our web platform sucks =). AFAIK, 'resize' is detected after layout and the event dispatched from the rAF callback (ScriptedAnimationController). Technically that's an "implementation detail" though of course - i.e. nothing preventing layout -> resize -> re-layout -> ... https://codereview.chromium.org/1156993013/diff/750001/Source/core/layout/Lay... File Source/core/layout/LayoutMedia.cpp (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/layout/Lay... Source/core/layout/LayoutMedia.cpp:93: if (mediaElement() && mediaElement()->mediaControls()) if (newSize.width() != oldSize.width() && ... or this will notify on any layout. (clientWidth as used elsewhere will be pixel-snapped though.)
thanks for all the feedback. i've not addressed all of the most recent, but only a few points. the most important question i pose in the comments is: > to be clear: i'm about to go spelunking into layout code. is this what you both intend for me to do? thanks -fl https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/TestExpect... File LayoutTests/TestExpectations (right): https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/TestExpect... LayoutTests/TestExpectations:1121: crbug.com/401902 virtual/gpu/fast/canvas/canvas-lose-restore-googol-size.html [ Pass Failure ] not sure what's up with this. will check. https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/TestExpect... LayoutTests/TestExpectations:1574: # Was specific to linux with Pass / ImageOnlyFailure. Removing modifier On 2015/07/08 10:31:51, philipj wrote: > Perhaps mention this kind of thing in the commit message instead, or add a TODO > if you should add [Linux] back after the rebaseline. done, TODO. https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/TestExpect... LayoutTests/TestExpectations:1689: crbug.com/1156993013 compositing/geometry/video-fixed-scrolling.html [ NeedsRebaseline ] On 2015/07/08 10:31:51, philipj wrote: > I guess this should be 446350? > > For this and all the tests in this block, could you check if they're actually > testing something that depends on the controls, or if the controls attribute > can't be removed from the test instead? It's a nuisance to update this many > tests for any change to the controls... crbug: good catch. i have a script that updates this file automatically, and i pasted the CL number in accidentally on the command line. video controls: good idea. this will take some time, and will be separate from the next CL update. https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/cont... File LayoutTests/media/controls-volume-slider.html (right): https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/cont... LayoutTests/media/controls-volume-slider.html:17: // Click at 25% of the volume slider, to avoid hitting On 2015/07/08 10:31:51, philipj wrote: > Perhaps this isn't a new bug, but now that the area is bigger, it's quite > noticeable and annoying that one can't click even slightly outside the blue > circle to seek. Clicking and then dragging a pixel does work, so it has a > glitchy feeling to it. Can the behavior be changed instead of the test? sounds good. https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/trac... File LayoutTests/media/track/track-cue-rendering-after-controls-added-expected.html (right): https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/trac... LayoutTests/media/track/track-cue-rendering-after-controls-added-expected.html:8: <body> On 2015/07/08 10:31:51, philipj wrote: > I don't think this change is needed to pass the test? (Many tests omit > unnecessary markup in order to bring attention to the bits that are being > tested.) Done. https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/trac... File LayoutTests/media/track/track-cue-rendering-after-controls-added.html (right): https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/trac... LayoutTests/media/track/track-cue-rendering-after-controls-added.html:25: // Add the controls. The cue should move to avoid overlap. Defer this On 2015/07/08 10:31:51, philipj wrote: > Was this test flaky before as well, or what has changed? this wasn't flaky. this change gives the scheduled update a chance to run. however, based on your second round of comments (100% width video / mouse hovering / new layout), i need to take another look. https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/trac... File LayoutTests/media/track/track-cue-rendering-wider-than-controls.html (right): https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/trac... LayoutTests/media/track/track-cue-rendering-wider-than-controls.html:9: video::-webkit-media-controls-enclosure { On 2015/07/08 10:31:51, philipj wrote: > A bit odd, but I guess this test can just be removed when ::-webkit-media-* is > removed and the max-width in mediaControls.css is no more. yeah, i was unsure whether to remove it now or wait until the dust settles. i added it to my TODO list to be sure that it doesn't get lost. https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/vide... File LayoutTests/media/video-controls-drop-order.html (right): https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/vide... LayoutTests/media/video-controls-drop-order.html:1: <html> On 2015/07/08 14:37:34, philipj wrote: > https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-... > https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-... > > To get rid of <body> you can simply put the test script at the end and not have > an init() at all. Acknowledged. https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/vide... LayoutTests/media/video-controls-drop-order.html:24: videos=[]; On 2015/07/08 14:37:34, philipj wrote: > videos is already set to [] Done. https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/vide... LayoutTests/media/video-controls-drop-order.html:26: for(var width = 50; width < 550; width+=50) { On 2015/07/08 14:37:34, philipj wrote: > Spaces around operators. Done. https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/vide... LayoutTests/media/video-controls-drop-order.html:26: for(var width = 50; width < 550; width+=50) { On 2015/07/08 14:37:34, philipj wrote: > Spaces around operators. Done. https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/vide... LayoutTests/media/video-controls-drop-order.html:26: for(var width = 50; width < 550; width+=50) { On 2015/07/08 14:37:34, philipj wrote: > Spaces around operators. Done. https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/vide... LayoutTests/media/video-controls-drop-order.html:55: for (var idx in controlNames) { On 2015/07/08 14:37:34, philipj wrote: > Since these are plain arrays you could use "for (x of y)" here, seems like > that's supported now. (forEach() would also have been an option) Done. https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/vide... LayoutTests/media/video-controls-drop-order.html:59: var state = (control.style['display'] != 'none') ? "seen" : "gone"; On 2015/07/08 14:37:34, philipj wrote: > Just control.style.display would be be idiomatic, or getComputedStyle() to be > more robust against what it is that causes the control be display:none. Done. https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/vide... LayoutTests/media/video-controls-drop-order.html:64: testRunner.notifyDone(); On 2015/07/08 14:37:34, philipj wrote: > Seems just as well to let the test finish running, so that all of the failures > can be seen if it fails on the bots. Done. https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/vide... LayoutTests/media/video-controls-drop-order.html:82: <div id="videoDiv"/> On 2015/07/08 14:37:34, philipj wrote: > </div> if you want to close the div here, or someone might think that this is > XHTML at a glance. You could also do without this by simply appending the videos > to document.body Done. https://codereview.chromium.org/1156993013/diff/710001/Source/core/css/CSSDef... File Source/core/css/CSSDefaultStyleSheets.cpp (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/css/CSSDef... Source/core/css/CSSDefaultStyleSheets.cpp:40: #include "core/page/Page.h" On 2015/07/08 10:31:51, philipj wrote: > Seems to compile without this include. thanks, removed. https://codereview.chromium.org/1156993013/diff/710001/Source/core/css/mediaC... File Source/core/css/mediaControlsAndroidNew.css (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/css/mediaC... Source/core/css/mediaControlsAndroidNew.css:3: * Copyright (C) 2009 Apple Inc. All rights reserved. On 2015/07/08 10:31:52, philipj wrote: > I would keep the copyright header from which this was copied, or use the new > smaller one if it wasn't copied. Changing years and adding lines over time is > tedious :) i'll update this with the de-crufting to match mediaControlsAndroid.css, from which it came. https://codereview.chromium.org/1156993013/diff/710001/Source/core/css/mediaC... Source/core/css/mediaControlsAndroidNew.css:31: audio::-webkit-media-controls-mute-button, video::-webkit-media-controls-mute-button { On 2015/07/08 10:31:51, philipj wrote: > This is contrary to https://codereview.chromium.org/1045103002/ > > I happen to agree that and we have a patch in Opera for Android to get rid of > the mute button, but was this discussed internally? it was, but i was unaware of this bug. i'll follow up. maybe we should show the unmute button if it's muted. that's what the most recent CL does. i also added a test to be sure that muted media shows an unmute button. how do you handle this case in opera? https://codereview.chromium.org/1156993013/diff/710001/Source/core/css/mediaC... File Source/core/css/mediaControlsNew.css (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:58: margin: 0; On 2015/07/08 10:31:52, philipj wrote: > This was added, does it make a difference? In general there seems to be a whole > lot of cruft in this CSS, can you tried to minimize it to just the stuff that is > actually needed? Like box-sizing:border-box just below here doesn't seem to make > sense, and such is the case every 5-10 lines in this file I think. de-cruft is still pending, probably in PS20. https://codereview.chromium.org/1156993013/diff/710001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:120: padding: 12px; On 2015/07/08 10:31:52, philipj wrote: > The amount of whitespace around the mute button looks really excessive, and was > pointed out by some people I showed the new controls to internally. It makes it > a bit non-obvious that the second slider is related to the volume button at all. > Does this match what was intended in the mockups? I'm no designer, but something > closer to the amount of space between the play button and the duration text > would seem better. i've forwarded this feedback to the design team. thanks. https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... File Source/core/html/shadow/MediaControlElementTypes.cpp (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControlElementTypes.cpp:120: int minWidthInPixels = 48; On 2015/07/08 15:06:38, philipj wrote: > Not sure the local variable explains much in such a short method, just returning > the values seems OK. Done. https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControlElementTypes.cpp:123: // we've been shown. Instead, we just memorize 48 except for the On 2015/07/08 15:06:38, philipj wrote: > Not sure what "memorize" means here, maybe "assume"? Done. https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... File Source/core/html/shadow/MediaControlElementTypes.h (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControlElementTypes.h:83: // These hold the state about whether this control should be shown if On 2015/07/08 15:06:38, philipj wrote: > OK, I take it the problem is that there are now two different reasons that > something can be hidden: > 1. Because it makes no sense, e.g. the CC button where there are no captions. > 2. Because it won't fit. > > Keeping show() and hide() exposed and having want() and dontWant() call those > internally seems like it might invite mistakes, by code calling show() or hide() > directly, and just confusion about how this works. > > Could the new model be made explicit by having exposed ways of saying that the > element isn't needed for the two different reasons, and then handling the actual > display state internally based on those two bits? > > Alternatively, if there's any way for the elements to not have any knowledge > about the reason they're being shown or hidden, that would also be cleaner, but > I don't know where one would keep track of the state to avoid showing the > element again when just one of the bits flips to the "can show" state. originally, i tried to keep the state in MediaControls, since i prefer for the elements not to know anything about this. however, it was fairly cumbersome. i made fits/doesnt fit properties of the element, and made hide/show private. https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControlElementTypes.h:91: // this control. Ideally, this would just be shorthand for the css On 2015/07/08 15:06:38, philipj wrote: > s/css attributes/computed style/ Done. https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:38: #include "public/platform/Platform.h" On 2015/07/08 15:06:38, philipj wrote: > It doesn't look like this include is needed. Done. https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:95: // +-MediaControlTimelineElement (-webkit-media-controls-timeline) On 2015/07/08 15:06:38, philipj wrote: > Can you keep it in both places and annotated with {if > (!)RuntimeSomethingOrAnother} until the flag is gone? Done. https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:244: scheduleUpdateControlsVisibilityForSpace(); On 2015/07/08 09:31:00, fs wrote: > First refreshCastButtonVisibility will call updateControlsVisibilityForSpace, > and then scheduleUpdateControlsVisibilityForSpace will call it again as well as > schedule another call, making for 2+1 calls in total... updated to ...WithoutLayout(), which is now called ...WithoutUpdate(). https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:446: void MediaControls::refreshCastButtonVisibilityWithoutLayout() On 2015/07/08 15:06:38, philipj wrote: > This name doesn't seem right, getBoundingClientRect() may trigger layout. If it > really is guaranteed, can it be asserted somehow to calm the skeptical reader? "causes no more layouts than before". i've renamed to ...WithoutUpdate(). https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:620: updateControlsVisibilityForSpace(); // So that queries work On 2015/07/08 09:31:00, fs wrote: > Having a schedule<operation> that also performs <operation> immediately is, > well, slightly unconventional. true. it no longer does this. https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.h (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControls.h:72: // can e called while stopping DOM objects, during which a layout On 2015/07/08 15:06:38, philipj wrote: > s/e/be/ but also see the comment about this not seeming to actually be layout > free. this method is now refreshCastButtonVisibilityWithoutUpdate(). https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControls.h:113: void scheduleUpdateControlsVisibilityForSpace(); On 2015/07/08 09:31:00, fs wrote: > I don't know what the "ForSpace" suffix is supposed to mean, but I don't think > it adds much. it was intended to mean "based on what fits". i'll think of a new name. https://codereview.chromium.org/1156993013/diff/710001/Source/core/layout/Lay... File Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/layout/Lay... Source/core/layout/LayoutObject.cpp:1829: if (m_bitfields.sendResizeEvent() && m_node) { On 2015/07/08 09:31:00, fs wrote: > I think it would be better just put this mechanism within MediaControls (using a > zero-delay Timer). The actual size-changes for MediaControls will be constrained > to the setWidth/setHeight calls in LayoutMedia::layout. thanks! I added a call to LayoutMedia::layout() to notify MediaControls. MediaControl sets a zero-delay timer, as you suggest. https://codereview.chromium.org/1156993013/diff/710001/Source/core/layout/Lay... File Source/core/layout/LayoutThemeMac.mm (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/layout/Lay... Source/core/layout/LayoutThemeMac.mm:1019: adjustMediaSliderThumbSize(style); On 2015/07/08 10:31:52, philipj wrote: > Why was this change needed? the original adjust..Size() did nothing unless the slider type was one of the two media sliders. the new one does, since it does the same thing in both cases. i considered keeping the old behavior there, and not adding the 'else'. however, the else seemed less flaky and matches what ThemeDefault does already. https://codereview.chromium.org/1156993013/diff/710001/Source/core/paint/Medi... File Source/core/paint/MediaControlsPainter.cpp (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:117: // The new UI uses "muted" and "not muted" only. On 2015/07/08 10:31:52, philipj wrote: > Is this by design? I was expecting the "waves" to follow along when dragging the > slider, but they don't. i'll verify that this wasn't overlooked, but i believe that it is intentional. https://codereview.chromium.org/1156993013/diff/710001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:503: return paintMediaButton(paintInfo.context, rect, mediaExitFullscreenButton, !isEnabled); On 2015/07/08 10:31:52, philipj wrote: > Is there a test that would fail if the enter fullscreen button were always > drawn? There have been a few bugs around invalidation for the different button > states, and although I think this will work by virtue of > MediaControlElement::setDisplayType invalidating, a test to lock it down would > be nice. good point. will add. https://codereview.chromium.org/1156993013/diff/710001/Source/platform/Runtim... File Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/1156993013/diff/710001/Source/platform/Runtim... Source/platform/RuntimeEnabledFeatures.in:101: NewMediaPlaybackUi status=stable On 2015/07/08 10:31:52, philipj wrote: > Since it's enabled by default I don't think the command line switch in > https://codereview.chromium.org/1145053006/ is needed, nor should it be > necessary to expose this in WebRuntimeFeatures. If there's a problem, it should > suffice to flip this flag. indeed, i forgot to close that CL. thanks. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:156: #if OS(ANDROID) On 2015/07/09 08:56:10, philipj wrote: > Is this actually needed? Should work the same if done in > mediaControlsAndroidNew.css I think. since we unhide mute to handle the 'media starts muted' bug, i wanted to treat it the same here. else MediaContolElement can get out of sync. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:190: const bool useNewUi = RuntimeEnabledFeatures::newMediaPlaybackUiEnabled(); On 2015/07/09 08:56:10, philipj wrote: > Also make duration const if you prefer that. (If you happen to know if const on > local variable ever makes a difference to the generated code I'd also be curious > to know, seems to me like the compiler should be able to tell that it isn't > modified with or without it.) i've always assumed that it's a note to myself more than to the compiler :) https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:614: if (panelWidth == 0 || panelWidth == m_panelWidth) On 2015/07/09 09:31:17, fs wrote: > On 2015/07/09 08:56:10, philipj wrote: > > Shouldn't m_panelWidth still be updated? Seems simpler to just always have it > in > > sync with layout? > > Yeah, if checking this at all, it's probably better to do it just before the > actual controls "layout". i can remove the check here safely, but i can't add it to checkWhichControlsFit(). i'll remove. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:618: m_panelWidthChangedTimer.startOneShot(0, FROM_HERE); On 2015/07/09 09:31:17, fs wrote: > On 2015/07/09 08:56:10, philipj wrote: > > Given that this is called from LayoutMedia::layout() I take it the reason for > > this is to avoid some asserts about style invalidation during layout? Still, > it > > seems like doing it on a timer could cause one bad frame to be rendered. fs? > > Correct on both accounts AFAIK. > > > My hunch is that the only way to really make this work might be to move the > > logic to layout somehow. > > Agreed. > > > It's true that JS controls couldn't do that, but if the > > controls were in an iframe and listened to the resize event, I presume that > > updating the layout in the resize event would not cause one bad frame. At > least > > I hope there's actually a way for web content to do this kind of thing > > correctly, otherwise our web platform sucks! :) > > Our web platform sucks =). AFAIK, 'resize' is detected after layout and the > event dispatched from the rAF callback (ScriptedAnimationController). > Technically that's an "implementation detail" though of course - i.e. nothing > preventing layout -> resize -> re-layout -> ... to be clear: i'm about to go spelunking into layout code. is this what you both intend for me to do?
thanks for all the feedback. i've not addressed all of the most recent, but only a few points. the most important question i pose in the comments is: > to be clear: i'm about to go spelunking into layout code. is this what you both intend for me to do? thanks -fl https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/vide... File LayoutTests/media/video-controls-drop-order.html (right): https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/vide... LayoutTests/media/video-controls-drop-order.html:59: var state = (control.style['display'] != 'none') ? "seen" : "gone"; On 2015/07/08 14:37:34, philipj wrote: > Just control.style.display would be be idiomatic, or getComputedStyle() to be > more robust against what it is that causes the control be display:none. Done. https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/vide... LayoutTests/media/video-controls-drop-order.html:64: testRunner.notifyDone(); On 2015/07/08 14:37:34, philipj wrote: > Seems just as well to let the test finish running, so that all of the failures > can be seen if it fails on the bots. Done. https://codereview.chromium.org/1156993013/diff/710001/Source/core/css/CSSDef... File Source/core/css/CSSDefaultStyleSheets.cpp (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/css/CSSDef... Source/core/css/CSSDefaultStyleSheets.cpp:40: #include "core/page/Page.h" On 2015/07/08 10:31:51, philipj wrote: > Seems to compile without this include. thanks, removed. https://codereview.chromium.org/1156993013/diff/710001/Source/core/css/mediaC... File Source/core/css/mediaControlsAndroidNew.css (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/css/mediaC... Source/core/css/mediaControlsAndroidNew.css:3: * Copyright (C) 2009 Apple Inc. All rights reserved. On 2015/07/08 10:31:52, philipj wrote: > I would keep the copyright header from which this was copied, or use the new > smaller one if it wasn't copied. Changing years and adding lines over time is > tedious :) i'll update this with the de-crufting to match mediaControlsAndroid.css, from which it came. https://codereview.chromium.org/1156993013/diff/710001/Source/core/css/mediaC... Source/core/css/mediaControlsAndroidNew.css:31: audio::-webkit-media-controls-mute-button, video::-webkit-media-controls-mute-button { On 2015/07/08 10:31:51, philipj wrote: > This is contrary to https://codereview.chromium.org/1045103002/ > > I happen to agree that and we have a patch in Opera for Android to get rid of > the mute button, but was this discussed internally? it was, but i was unaware of this bug. i'll follow up. maybe we should show the unmute button if it's muted. that's what the most recent CL does. i also added a test to be sure that muted media shows an unmute button. how do you handle this case in opera? https://codereview.chromium.org/1156993013/diff/710001/Source/core/css/mediaC... File Source/core/css/mediaControlsNew.css (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:58: margin: 0; On 2015/07/08 10:31:52, philipj wrote: > This was added, does it make a difference? In general there seems to be a whole > lot of cruft in this CSS, can you tried to minimize it to just the stuff that is > actually needed? Like box-sizing:border-box just below here doesn't seem to make > sense, and such is the case every 5-10 lines in this file I think. de-cruft is still pending, probably in PS20. https://codereview.chromium.org/1156993013/diff/710001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:120: padding: 12px; On 2015/07/08 10:31:52, philipj wrote: > The amount of whitespace around the mute button looks really excessive, and was > pointed out by some people I showed the new controls to internally. It makes it > a bit non-obvious that the second slider is related to the volume button at all. > Does this match what was intended in the mockups? I'm no designer, but something > closer to the amount of space between the play button and the duration text > would seem better. i've forwarded this feedback to the design team. thanks. https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... File Source/core/html/shadow/MediaControlElementTypes.cpp (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControlElementTypes.cpp:120: int minWidthInPixels = 48; On 2015/07/08 15:06:38, philipj wrote: > Not sure the local variable explains much in such a short method, just returning > the values seems OK. Done. https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControlElementTypes.cpp:123: // we've been shown. Instead, we just memorize 48 except for the On 2015/07/08 15:06:38, philipj wrote: > Not sure what "memorize" means here, maybe "assume"? Done. https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... File Source/core/html/shadow/MediaControlElementTypes.h (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControlElementTypes.h:83: // These hold the state about whether this control should be shown if On 2015/07/08 15:06:38, philipj wrote: > OK, I take it the problem is that there are now two different reasons that > something can be hidden: > 1. Because it makes no sense, e.g. the CC button where there are no captions. > 2. Because it won't fit. > > Keeping show() and hide() exposed and having want() and dontWant() call those > internally seems like it might invite mistakes, by code calling show() or hide() > directly, and just confusion about how this works. > > Could the new model be made explicit by having exposed ways of saying that the > element isn't needed for the two different reasons, and then handling the actual > display state internally based on those two bits? > > Alternatively, if there's any way for the elements to not have any knowledge > about the reason they're being shown or hidden, that would also be cleaner, but > I don't know where one would keep track of the state to avoid showing the > element again when just one of the bits flips to the "can show" state. originally, i tried to keep the state in MediaControls, since i prefer for the elements not to know anything about this. however, it was fairly cumbersome. i made fits/doesnt fit properties of the element, and made hide/show private. https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControlElementTypes.h:91: // this control. Ideally, this would just be shorthand for the css On 2015/07/08 15:06:38, philipj wrote: > s/css attributes/computed style/ Done. https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:38: #include "public/platform/Platform.h" On 2015/07/08 15:06:38, philipj wrote: > It doesn't look like this include is needed. Done. https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:95: // +-MediaControlTimelineElement (-webkit-media-controls-timeline) On 2015/07/08 15:06:38, philipj wrote: > Can you keep it in both places and annotated with {if > (!)RuntimeSomethingOrAnother} until the flag is gone? Done. https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:244: scheduleUpdateControlsVisibilityForSpace(); On 2015/07/08 09:31:00, fs wrote: > First refreshCastButtonVisibility will call updateControlsVisibilityForSpace, > and then scheduleUpdateControlsVisibilityForSpace will call it again as well as > schedule another call, making for 2+1 calls in total... updated to ...WithoutLayout(), which is now called ...WithoutUpdate(). https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:446: void MediaControls::refreshCastButtonVisibilityWithoutLayout() On 2015/07/08 15:06:38, philipj wrote: > This name doesn't seem right, getBoundingClientRect() may trigger layout. If it > really is guaranteed, can it be asserted somehow to calm the skeptical reader? "causes no more layouts than before". i've renamed to ...WithoutUpdate(). https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:620: updateControlsVisibilityForSpace(); // So that queries work On 2015/07/08 09:31:00, fs wrote: > Having a schedule<operation> that also performs <operation> immediately is, > well, slightly unconventional. true. it no longer does this. https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.h (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControls.h:72: // can e called while stopping DOM objects, during which a layout On 2015/07/08 15:06:38, philipj wrote: > s/e/be/ but also see the comment about this not seeming to actually be layout > free. this method is now refreshCastButtonVisibilityWithoutUpdate(). https://codereview.chromium.org/1156993013/diff/710001/Source/core/html/shado... Source/core/html/shadow/MediaControls.h:113: void scheduleUpdateControlsVisibilityForSpace(); On 2015/07/08 09:31:00, fs wrote: > I don't know what the "ForSpace" suffix is supposed to mean, but I don't think > it adds much. it was intended to mean "based on what fits". i'll think of a new name. https://codereview.chromium.org/1156993013/diff/710001/Source/core/layout/Lay... File Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/layout/Lay... Source/core/layout/LayoutObject.cpp:1829: if (m_bitfields.sendResizeEvent() && m_node) { On 2015/07/08 09:31:00, fs wrote: > I think it would be better just put this mechanism within MediaControls (using a > zero-delay Timer). The actual size-changes for MediaControls will be constrained > to the setWidth/setHeight calls in LayoutMedia::layout. thanks! I added a call to LayoutMedia::layout() to notify MediaControls. MediaControl sets a zero-delay timer, as you suggest. https://codereview.chromium.org/1156993013/diff/710001/Source/core/layout/Lay... File Source/core/layout/LayoutThemeMac.mm (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/layout/Lay... Source/core/layout/LayoutThemeMac.mm:1019: adjustMediaSliderThumbSize(style); On 2015/07/08 10:31:52, philipj wrote: > Why was this change needed? the original adjust..Size() did nothing unless the slider type was one of the two media sliders. the new one does, since it does the same thing in both cases. i considered keeping the old behavior there, and not adding the 'else'. however, the else seemed less flaky and matches what ThemeDefault does already. https://codereview.chromium.org/1156993013/diff/710001/Source/core/paint/Medi... File Source/core/paint/MediaControlsPainter.cpp (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:117: // The new UI uses "muted" and "not muted" only. On 2015/07/08 10:31:52, philipj wrote: > Is this by design? I was expecting the "waves" to follow along when dragging the > slider, but they don't. i'll verify that this wasn't overlooked, but i believe that it is intentional. https://codereview.chromium.org/1156993013/diff/710001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:503: return paintMediaButton(paintInfo.context, rect, mediaExitFullscreenButton, !isEnabled); On 2015/07/08 10:31:52, philipj wrote: > Is there a test that would fail if the enter fullscreen button were always > drawn? There have been a few bugs around invalidation for the different button > states, and although I think this will work by virtue of > MediaControlElement::setDisplayType invalidating, a test to lock it down would > be nice. good point. will add. https://codereview.chromium.org/1156993013/diff/710001/Source/platform/Runtim... File Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/1156993013/diff/710001/Source/platform/Runtim... Source/platform/RuntimeEnabledFeatures.in:101: NewMediaPlaybackUi status=stable On 2015/07/08 10:31:52, philipj wrote: > Since it's enabled by default I don't think the command line switch in > https://codereview.chromium.org/1145053006/ is needed, nor should it be > necessary to expose this in WebRuntimeFeatures. If there's a problem, it should > suffice to flip this flag. indeed, i forgot to close that CL. thanks. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:156: #if OS(ANDROID) On 2015/07/09 08:56:10, philipj wrote: > Is this actually needed? Should work the same if done in > mediaControlsAndroidNew.css I think. since we unhide mute to handle the 'media starts muted' bug, i wanted to treat it the same here. else MediaContolElement can get out of sync. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:190: const bool useNewUi = RuntimeEnabledFeatures::newMediaPlaybackUiEnabled(); On 2015/07/09 08:56:10, philipj wrote: > Also make duration const if you prefer that. (If you happen to know if const on > local variable ever makes a difference to the generated code I'd also be curious > to know, seems to me like the compiler should be able to tell that it isn't > modified with or without it.) i've always assumed that it's a note to myself more than to the compiler :) https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:614: if (panelWidth == 0 || panelWidth == m_panelWidth) On 2015/07/09 09:31:17, fs wrote: > On 2015/07/09 08:56:10, philipj wrote: > > Shouldn't m_panelWidth still be updated? Seems simpler to just always have it > in > > sync with layout? > > Yeah, if checking this at all, it's probably better to do it just before the > actual controls "layout". i can remove the check here safely, but i can't add it to checkWhichControlsFit(). i'll remove. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:618: m_panelWidthChangedTimer.startOneShot(0, FROM_HERE); On 2015/07/09 09:31:17, fs wrote: > On 2015/07/09 08:56:10, philipj wrote: > > Given that this is called from LayoutMedia::layout() I take it the reason for > > this is to avoid some asserts about style invalidation during layout? Still, > it > > seems like doing it on a timer could cause one bad frame to be rendered. fs? > > Correct on both accounts AFAIK. > > > My hunch is that the only way to really make this work might be to move the > > logic to layout somehow. > > Agreed. > > > It's true that JS controls couldn't do that, but if the > > controls were in an iframe and listened to the resize event, I presume that > > updating the layout in the resize event would not cause one bad frame. At > least > > I hope there's actually a way for web content to do this kind of thing > > correctly, otherwise our web platform sucks! :) > > Our web platform sucks =). AFAIK, 'resize' is detected after layout and the > event dispatched from the rAF callback (ScriptedAnimationController). > Technically that's an "implementation detail" though of course - i.e. nothing > preventing layout -> resize -> re-layout -> ... to be clear: i'm about to go spelunking into layout code. is this what you both intend for me to do?
https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:618: m_panelWidthChangedTimer.startOneShot(0, FROM_HERE); On 2015/07/09 12:10:57, liberato wrote: > On 2015/07/09 09:31:17, fs wrote: > > On 2015/07/09 08:56:10, philipj wrote: > > > Given that this is called from LayoutMedia::layout() I take it the reason > for > > > this is to avoid some asserts about style invalidation during layout? Still, > > it > > > seems like doing it on a timer could cause one bad frame to be rendered. fs? > > > > Correct on both accounts AFAIK. > > > > > My hunch is that the only way to really make this work might be to move the > > > logic to layout somehow. > > > > Agreed. > > > > > It's true that JS controls couldn't do that, but if the > > > controls were in an iframe and listened to the resize event, I presume that > > > updating the layout in the resize event would not cause one bad frame. At > > least > > > I hope there's actually a way for web content to do this kind of thing > > > correctly, otherwise our web platform sucks! :) > > > > Our web platform sucks =). AFAIK, 'resize' is detected after layout and the > > event dispatched from the rAF callback (ScriptedAnimationController). > > Technically that's an "implementation detail" though of course - i.e. nothing > > preventing layout -> resize -> re-layout -> ... > > to be clear: i'm about to go spelunking into layout code. is this what you both > intend for me to do? Well, it's essentially a QoI issue - there could (will) be a single-frame glitch when the "what fits?" algorithm decides to hide/unhide a control. Depending on various factors (width-delta, padding, etc.) it'll end up looking less pretty than it maybe could. I'm pretty sure that you - having seen this is action - have a better feel for how "glitchy" it actually appears in practice though, so as far as I'm concerned I'd leave it up to you to decided. philipj on the other hand is perhaps a bit more adamant than I am ;-)
On 2015/07/09 12:10:57, liberato wrote: > thanks for all the feedback. i've not addressed all of the most recent, but > only a few points. the most important question i pose in the comments is: > > > to be clear: i'm about to go spelunking into layout code. is this what you > both intend for me to do? A very important question, indeed. I suspect that when you go down that path things will become non-trivial quickly, and as you say that approach wouldn't translate to a JS implementation. So, is there any way that JS could implement this logic without rendering bad frames? The resize event is synchronized with animation frames, but I don't know what happens if the event handler triggers a new layout, which will be necessary here. In manual testing, is the problem with a timeout observable? It would be worth testing both small incremental changes to the size driven by some animation and larger changes where the problem ought to be more pronounced, if it's there. If some layout integration is necessary, one possible approach would be to override layoutObjectIsNeeded, and somehow ensure that all layout objects are destroyed and recreated for a resize. fs probably has other ideas :)
On 2015/07/09 12:26:42, philipj wrote: > On 2015/07/09 12:10:57, liberato wrote: > > thanks for all the feedback. i've not addressed all of the most recent, but > > only a few points. the most important question i pose in the comments is: > > > > > to be clear: i'm about to go spelunking into layout code. is this what you > > both intend for me to do? > > A very important question, indeed. I suspect that when you go down that path > things will become non-trivial quickly, and as you say that approach wouldn't > translate to a JS implementation. > > So, is there any way that JS could implement this logic without rendering bad > frames? The resize event is synchronized with animation frames, but I don't know > what happens if the event handler triggers a new layout, which will be necessary > here. If it does, then the document will be layouted before the next paint (as required by the lifecycle). > In manual testing, is the problem with a timeout observable? It would be worth > testing both small incremental changes to the size driven by some animation and > larger changes where the problem ought to be more pronounced, if it's there. > > If some layout integration is necessary, one possible approach would be to > override layoutObjectIsNeeded, and somehow ensure that all layout objects are > destroyed and recreated for a resize. fs probably has other ideas :) That would be too early. Doing this during layout I guess would be easiest by just mutating the ComputedStyle of the relevant boxes (as little as possible - 'visibility' might be enough.) Setting an override content width could be one other way, but I suspect that won't play too well with the flex-container.
Phew, all done, for this pass at least :) https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... File Source/core/html/shadow/SliderThumbElement.cpp (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/SliderThumbElement.cpp:231: if (m_inDragMode && !m_didDragAnywhere && isMediaSlider()) { I think what I would expect is that mousedown actually causes the slider to be centered under the pointer/finger. This seems to be how the YouTube Android app works. Should simplify the changes in this class by a lot. Also, what are the other kinds of sliders, does this fix make sense for them too or is the failure to budge a feature in some contexts? It seems somewhat unlikely that the behavior is intentional and tested. https://codereview.chromium.org/1156993013/diff/750001/Source/core/layout/Lay... File Source/core/layout/LayoutTheme.cpp (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/layout/Lay... Source/core/layout/LayoutTheme.cpp:241: minutes %= 60; This makes it somewhat hard to follow the differences, I think it may be more clear to simply calculate the seconds and minutes in the shared code and then do the hours in the old UI code path. https://codereview.chromium.org/1156993013/diff/750001/Source/core/layout/Lay... Source/core/layout/LayoutTheme.cpp:261: const char* separator = (includeSeparator ? "/ " : ""); I don't think the () are required here. https://codereview.chromium.org/1156993013/diff/750001/Source/core/layout/Lay... Source/core/layout/LayoutTheme.cpp:264: // 10-60 minutes duration is 00:00 Shouldn't this be 10-99? A 61 minute movie shouldn't show as 061:00 should it? https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... File Source/core/paint/MediaControlsPainter.cpp (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:47: // Current UI slider thumbs sizes. Could these be turned into static functions to hide away the runtime checks? Same for sliderBackgroundColor and anything else where it would result in fewer runtime checks in total. https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:59: // New UI overlay play button size. I don't think I understand this bit, can't the size just be set in CSS? https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:101: context->beginLayer(kDisabledAlpha); Don't need {} here, but more importantly, the naming here had me confused. I read tranparent as alpha 0, but this is about rendering some thing with alpha 0.4. Perhaps a bool isEnabled argument to match most of the call sites? https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:177: // Overlay play button is full-screen, so center. We don't know here if we're in fullscreen or not, do we? If this is to implement the logic that the button should be centered on screen in fullscreen but centered excluding the controls panel while inline, that's possible to implement in CSS, we actually have a patch for this in Opera for Android, assuming the old sizes: /* In fullscreen, center the button on the video, ignoring the panel height. */ video:-webkit-full-screen::-webkit-media-controls-overlay-play-button { margin-top: -14px; /* 40/2 - 34 */ } https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:196: // If we're using the new UI, create a short, vertically centered rect. The current height is 8px but it still works fine to click outside of it to seek. What happens is this is set to 2px using CSS? The slider uses Shadow DOM internally so I'm guessing that the height of the slider rect is separate from the height of the clickable area already? https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:206: adjustedRect.setHeight(zoomedHeight); I guess this will be rounded to zero if the zoom level of the page is <25%? If FloatRect is returned then perhaps a blurry line will be rendered instead. (Not really a new problem, just that 2 is a smaller number than 8.) https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:287: bool drawNewUiGrayed = !hasSource(mediaElement) && RuntimeEnabledFeatures::newMediaPlaybackUiEnabled(); Can probably drop "new" here lest it be left when the flag is gone. https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:288: if (drawNewUiGrayed) { Extra {} https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:308: context->endLayer(); Could this be refactored so that there's a paint() does beginLayer()+reallyPaint()+endLayer() instead of multiple endLayer()+returns? When in a single function, it would be nicer with an on-stack this with a destructor, but that seems way overkill compared to splitting it up. https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:338: Color endColor; So no end color at all for the new UI? Are we then drawing first a background and then blue on top of it up to the slider? Could it be changed to just not draw a background and instead paint a before and after bit in different colors? https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:454: if (RuntimeEnabledFeatures::newMediaPlaybackUiEnabled()) { {} https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:491: // with the new player UI, we have separate assets for enter / exit s/with/With/ https://codereview.chromium.org/1156993013/diff/750001/Source/platform/Runtim... File Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/1156993013/diff/750001/Source/platform/Runtim... Source/platform/RuntimeEnabledFeatures.in:100: NewMediaPlaybackUi status=stable I think we should first land this with no status to ensure that all of the existing tests work as they should with the old UI code in branches, then have a separate CL that flips the flag and updates the test.
> > In manual testing, is the problem with a timeout observable? if i construct the test carefully, then i can see it. under normal usage, i haven't. but, i might not have the fastest eyes on the planet. it was something that i was looking out for, since i realized that the current method does have the potential for bad frames. the most visible i can make it is if it jumps by about >50px from some width that's >150 (i.e., sliders are visible before the jump), while the video is bottom aligned so that the controls don't move. in that case, i can see the sliders repaint if a control is added that shortens them. it's less visible in the other direction, and not visible to me with smooth animation. fs's suggestion of modifying visibility sounds fairly straightforward (thanks!). i will look into this today. however, i'd really like not to integrate the media player significantly further into the cpp code with this UI change, because that really seems like going in the wrong direction. i'd prefer to put the work into getting the bad frame out of a JS implementation, since that will benefit other uses of JS-based layout too. so, *my proposal is this*: if the layout change is relatively superficial, then i'll go for it as part of this new UI. if the layout change turns out to be labrinth-like or potentially destabilizing, then let's stick with the current method and put the work into a better JS solution. thanks -fl On Thu, Jul 9, 2015 at 5:43 AM, <fs@opera.com> wrote: > On 2015/07/09 12:26:42, philipj wrote: > >> On 2015/07/09 12:10:57, liberato wrote: >> > thanks for all the feedback. i've not addressed all of the most >> recent, but >> > only a few points. the most important question i pose in the comments >> is: >> > >> > > to be clear: i'm about to go spelunking into layout code. is this >> what >> > you > >> > both intend for me to do? >> > > A very important question, indeed. I suspect that when you go down that >> path >> things will become non-trivial quickly, and as you say that approach >> wouldn't >> translate to a JS implementation. >> > > So, is there any way that JS could implement this logic without rendering >> bad >> frames? The resize event is synchronized with animation frames, but I >> don't >> > know > >> what happens if the event handler triggers a new layout, which will be >> > necessary > >> here. >> > > If it does, then the document will be layouted before the next paint (as > required by the lifecycle). > > In manual testing, is the problem with a timeout observable? It would be >> worth >> testing both small incremental changes to the size driven by some >> animation >> > and > >> larger changes where the problem ought to be more pronounced, if it's >> there. >> > > If some layout integration is necessary, one possible approach would be to >> override layoutObjectIsNeeded, and somehow ensure that all layout objects >> are >> destroyed and recreated for a resize. fs probably has other ideas :) >> > > That would be too early. Doing this during layout I guess would be easiest > by > just mutating the ComputedStyle of the relevant boxes (as little as > possible - > 'visibility' might be enough.) Setting an override content width could be > one > other way, but I suspect that won't play too well with the flex-container. > > https://codereview.chromium.org/1156993013/ > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2015/07/09 14:42:07, blink-reviews wrote: > > > > In manual testing, is the problem with a timeout observable? > > > if i construct the test carefully, then i can see it. under normal usage, > i haven't. but, i might not have the fastest eyes on the planet. it was > something that i was looking out for, since i realized that the current > method does have the potential for bad frames. > > the most visible i can make it is if it jumps by about >50px from some > width that's >150 (i.e., sliders are visible before the jump), while the > video is bottom aligned so that the controls don't move. in that case, i > can see the sliders repaint if a control is added that shortens them. it's > less visible in the other direction, and not visible to me with smooth > animation. > > fs's suggestion of modifying visibility sounds fairly straightforward > (thanks!). i will look into this today. however, i'd really like not to > integrate the media player significantly further into the cpp code with > this UI change, because that really seems like going in the wrong > direction. i'd prefer to put the work into getting the bad frame out of a > JS implementation, since that will benefit other uses of JS-based layout > too. > > so, *my proposal is this*: if the layout change is relatively superficial, > then i'll go for it as part of this new UI. if the layout change turns out > to be labrinth-like or potentially destabilizing, then let's stick with the > current method and put the work into a better JS solution. Sounds like a good plan. I think resizing videos with controls can't be so common that this should be a blocker. The most common case ought to be in the fullscreen transition, but if you try that you'll notice that the fullscreen transition itself is just terrible, so it would drown in the flicker: https://archive.org/details/chromium-fullscreen-flicker I also agree that making it harder to port the controls to JS would be sad, so if this really can't be done using JS I'm more inclined to let it be broken to give us an incentive to fix the problem for web developers as well.
On 2015/07/09 15:00:44, philipj wrote: > On 2015/07/09 14:42:07, blink-reviews wrote: > > > > > > In manual testing, is the problem with a timeout observable? > > > > > > if i construct the test carefully, then i can see it. under normal usage, > > i haven't. but, i might not have the fastest eyes on the planet. it was > > something that i was looking out for, since i realized that the current > > method does have the potential for bad frames. > > > > the most visible i can make it is if it jumps by about >50px from some > > width that's >150 (i.e., sliders are visible before the jump), while the > > video is bottom aligned so that the controls don't move. in that case, i > > can see the sliders repaint if a control is added that shortens them. it's > > less visible in the other direction, and not visible to me with smooth > > animation. > > > > fs's suggestion of modifying visibility sounds fairly straightforward > > (thanks!). i will look into this today. however, i'd really like not to > > integrate the media player significantly further into the cpp code with > > this UI change, because that really seems like going in the wrong > > direction. i'd prefer to put the work into getting the bad frame out of a > > JS implementation, since that will benefit other uses of JS-based layout > > too. > > > > so, *my proposal is this*: if the layout change is relatively superficial, > > then i'll go for it as part of this new UI. if the layout change turns out > > to be labrinth-like or potentially destabilizing, then let's stick with the > > current method and put the work into a better JS solution. > > Sounds like a good plan. I think resizing videos with controls can't be so > common that this should be a blocker. The most common case ought to be in the > fullscreen transition, but if you try that you'll notice that the fullscreen > transition itself is just terrible, so it would drown in the flicker: > https://archive.org/details/chromium-fullscreen-flicker > > I also agree that making it harder to port the controls to JS would be sad, so > if this really can't be done using JS I'm more inclined to let it be broken to > give us an incentive to fix the problem for web developers as well. after a prototype, the cope of the layout changes look more scary than i was hoping. the flex layout currently ignores visibility. so, i added support for it, following the example of the deprecated flex layout. i can now cause elements to show / collapse during LayoutMedia::layout, but it's far from working correctly. nested elements, like sliders and text, even those in flex boxes, don't know what to do. The sliders, for example, can draw when they're collapsed, and sometimes in the wrong spot. the obvious stuff (recursively set visibility and/or mark for layout) doesn't help. i looked into modifying LayoutBlock / BlockFlow / Slider, wasn't clearly safe. so, unless there are objections, i'm going to proceed with the current timer-based approach. thanks -fl
this CL addresses all outstanding feedback. please let me know if i've missed anything. Big changes: * this CL turns the new UI off by default, per philipj. for some reason that i'm still chasing, one test (compositing/geometry/video-fixed-scrolling.html) fails due to an extra element in the output. this CL includes a rebaseline for that test. i'm still chasing the root cause. probably some missing check for the new UI since the last time i verified these tests with the switch off. * stays with timer-based layout, after a prototype of layout integration uncovered additional trouble with that route. details in other comments. * changedControlSelections() still calls clientWidth(). i believe that it's safe to remove that and rely on m_panelWidth, but i haven't been able to verify it. will revisit. thanks -fl https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... File Source/core/html/shadow/MediaControlElementTypes.h (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControlElementTypes.h:78: private: On 2015/07/09 08:56:09, philipj wrote: > Move these things to the private section at the bottom instead. If you can > remove show() and hide() too, better to just have a single way (public or > private) to update the inline style. Done. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControlElementTypes.h:119: bool m_isShown; On 2015/07/09 08:56:09, philipj wrote: > Does this always equal m_isWanted && m_doesFit? If so remove it. > > Also append ": 1" to consecutive bools to make them a bitfield. Done. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... File Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControlElements.cpp:311: if (mediaElement().shouldShowControls() && mediaElement().togglePlayStateWillPlay()) On 2015/07/09 08:56:10, philipj wrote: > Transform this into a single call to setIsWanted, potentially with a local bool > isWanted if that makes it more readable. Done. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:94: // +-MediaControlTimelineElement (-webkit-media-controls-timeline) (if !RTE::newMediaPlaybackUi()) On 2015/07/09 08:56:10, philipj wrote: > Can you use the same style as the above {if > mediaControlsOverlayPlayButtonEnabled}? done. didn't realize that's what it meant. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:132: if (!RuntimeEnabledFeatures::newMediaPlaybackUiEnabled()) On 2015/07/09 08:56:10, philipj wrote: > Break out this into a local variable like in ::reset() Done. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:138: if (!RuntimeEnabledFeatures::newMediaPlaybackUiEnabled()) On 2015/07/09 08:56:10, philipj wrote: > Transform this into a single setIsWanted() call. Done. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:216: m_volumeSlider->setIsWanted(false); On 2015/07/09 08:56:10, philipj wrote: > Ditto. this one always ends up worse with the #ifdef. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:220: #if OS(ANDROID) On 2015/07/09 08:56:10, philipj wrote: > Should also be possible to do with just CSS. true, but this way it's all in one spot. otherwise, we have to remember not to touch volume slider state in the case of android. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:223: m_volumeSlider->setIsWanted(false); On 2015/07/09 08:56:10, philipj wrote: > Ditto. Done. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:243: m_fullScreenButton->setIsWanted(true); On 2015/07/09 08:56:10, philipj wrote: > Ditto. done. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:400: // If the element is muted, then we want the unmute button. Otherwise, On 2015/07/09 08:56:11, philipj wrote: > I think that if we're going to change anything about this again, we should just > unmute and set volume to 1 when playing using the built-in controls. We don't > show the volume slider even though volume can be set to 0, so the current logic > seems strange to me, and a waste of pixels in the vast majority of cases. > > Alternatively, show the mute button only for videos that are actually muted, but > of course leave it in after that point as well. the latter one seems almost exactly what the code does. if we have audio and we're muted, then add the mute button. it's also very similar to what the original patch for crbug.com/467242 did, which seemed to fix it. so, i suspect that the problems were about the mute tag rather than volume==0. i agree that doing something with the volume is a good idea, but not sure what. i'm not sure that i understand the first suggestion -- unmuting automatically seems like it might not go so well. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:423: m_toggleClosedCaptionsButton->setIsWanted(true); On 2015/07/09 08:56:11, philipj wrote: > Ditto. Done. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:468: // without wanting the bar cast button. We depend on the fact On 2015/07/09 08:56:10, philipj wrote: > s/bar/panel/ or "the button in the controls panel" > > I'm also not sure what the "We depend on ..." bit is saying, > updateControlsVisibilityForSpace() isn't called here. expanded comment. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:472: m_castButton->setIsWanted(false); On 2015/07/09 08:56:10, philipj wrote: > The mismatch between showOverlayCastButton() and > m_castButton->setIsWanted(false) looks odd. Any idea why we can't just call > m_overlayCastButton->setIsWanted(true) here? It seems strange to reset the hide > timer in a function like this that one should expect to be called to potentially > invalidate state, but have no side effect if everything is already in the > correct state. yeah, i agree. changed. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:489: // do NOT updateControls...() here. On 2015/07/09 08:56:11, philipj wrote: > This comment isn't needed IMHO. Done. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:623: int panelWidth = m_panel->clientWidth(); On 2015/07/09 08:56:11, philipj wrote: > This may trigger a layout, is that intentional? It seems odd to both have > LayoutMedia notify about the panel width and to get it like this. i expected that it would avoid a bad frame, but truthfully i never tested that. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:641: void MediaControls::computeWhichControlsFit(int panelWidth) On 2015/07/09 08:56:10, philipj wrote: > Seems like this doesn't need an argument, use m_panelWidth? yeah, i had the same thought. i did it this way so that one doesn't accidentally call computeWhichControlsFit() when one really wants to get the most current width first. however, i'll try removing all of changedControlSelections(), and just get the width via notifyPanelWidthChanged(), then it'll be much simpler. hopefully that works. i'm just not convinced yet that it won't introduce an extra bad frame. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:664: for (int i = 0; elements[i]; i++) { On 2015/07/09 08:56:10, philipj wrote: > for (MediaControlElement* element : elements) at least compiles, and I assume > that you could get rid of the trailing 0 in the array using it. > > That would also make it explicit that none of the elements can be null, as that > would just silently terminate the loop with the current code. There are elements > that can be null, like m_overlayPlayButton, so it's not really obvious. done, cool. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:678: if (!m_castButton->isShown()) On 2015/07/09 08:56:10, philipj wrote: > I think this and above are the only places where isWanted() and isShown() are > used, and it looks like it could be replaced by just remembering some state from > the loop. It would be nice if the price of ugliness for the cast button hack > were localized and we don't have exposed state of the buttons that invites more > special casing in other contexts. > > Also merge into a single setIsWanted() call. i've removed isShown(), but i don't see how to remove isWanted. it's a consequence of moving the state to the MediaElement. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:682: } // else do not change overlay cast button state. On 2015/07/09 08:56:11, philipj wrote: > Don't need this comment. Done. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.h (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.h:70: void hideCastButtons(); On 2015/07/09 08:56:11, philipj wrote: > Ought this not be equivalent to refreshCastButtonVisibilityWithoutUpdate() in > the context it's called, i.e. when !mediaElement().hasRemoteRoutes()? It seems > like in principle it should be enough with just refreshCastButtonVisibility() > public, but cleaning up showOverlayCastButton() isn't your job if you don't want > it ;) i originally had it that way, but decided that it was confusing in HTMLMediaElement since it wasn't clear what an 'Update' was. naming is one of the three hardest problems in computer science, after all. i've removed hide...() and replaced with refresh...Update(). https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.h:116: // For the new UI, hide elements that don't fit, and show those things On 2015/07/09 08:56:11, philipj wrote: > Remove "For the new UI" or it's sure to be left in long after there's only one > UI. Done. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... File Source/core/html/shadow/SliderThumbElement.cpp (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/SliderThumbElement.cpp:231: if (m_inDragMode && !m_didDragAnywhere && isMediaSlider()) { On 2015/07/09 13:40:56, philipj wrote: > I think what I would expect is that mousedown actually causes the slider to be > centered under the pointer/finger. This seems to be how the YouTube Android app > works. Should simplify the changes in this class by a lot. > > Also, what are the other kinds of sliders, does this fix make sense for them too > or is the failure to budge a feature in some contexts? It seems somewhat > unlikely that the behavior is intentional and tested. mousedown move: i considered that, but having the start of a drag also move the the slider didn't feel right. drags are easy because they're relative to the touch down. small movement => small adjustment. recentering means that getting a small adjustment also requires a precisely located touch. other sliders: i expect that the behavior can be broadened, but i didn't want to do it as part of this change. https://codereview.chromium.org/1156993013/diff/750001/Source/core/layout/Lay... File Source/core/layout/LayoutMedia.cpp (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/layout/Lay... Source/core/layout/LayoutMedia.cpp:93: if (mediaElement() && mediaElement()->mediaControls()) On 2015/07/09 09:31:17, fs wrote: > if (newSize.width() != oldSize.width() && ... > > or this will notify on any layout. (clientWidth as used elsewhere will be > pixel-snapped though.) Done. https://codereview.chromium.org/1156993013/diff/750001/Source/core/layout/Lay... File Source/core/layout/LayoutTheme.cpp (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/layout/Lay... Source/core/layout/LayoutTheme.cpp:241: minutes %= 60; On 2015/07/09 13:40:56, philipj wrote: > This makes it somewhat hard to follow the differences, I think it may be more > clear to simply calculate the seconds and minutes in the shared code and then do > the hours in the old UI code path. Done. https://codereview.chromium.org/1156993013/diff/750001/Source/core/layout/Lay... Source/core/layout/LayoutTheme.cpp:261: const char* separator = (includeSeparator ? "/ " : ""); On 2015/07/09 13:40:56, philipj wrote: > I don't think the () are required here. Done. https://codereview.chromium.org/1156993013/diff/750001/Source/core/layout/Lay... Source/core/layout/LayoutTheme.cpp:264: // 10-60 minutes duration is 00:00 On 2015/07/09 13:40:56, philipj wrote: > Shouldn't this be 10-99? A 61 minute movie shouldn't show as 061:00 should it? i've asked the UX designers to verify the intention. https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... File Source/core/paint/MediaControlsPainter.cpp (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:47: // Current UI slider thumbs sizes. On 2015/07/09 13:40:56, philipj wrote: > Could these be turned into static functions to hide away the runtime checks? > Same for sliderBackgroundColor and anything else where it would result in fewer > runtime checks in total. not sure that i understand. declaring these as const inlines all references and doesn't emit any initialized data. i can declare these 'static const', just to be sure. i checked this, and the assembly was identical on a test program with clang++. https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:101: context->beginLayer(kDisabledAlpha); On 2015/07/09 13:40:56, philipj wrote: > Don't need {} here, but more importantly, the naming here had me confused. I > read tranparent as alpha 0, but this is about rendering some thing with alpha > 0.4. Perhaps a bool isEnabled argument to match most of the call sites? Done. https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:288: if (drawNewUiGrayed) { On 2015/07/09 13:40:56, philipj wrote: > Extra {} Done. https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:308: context->endLayer(); On 2015/07/09 13:40:56, philipj wrote: > Could this be refactored so that there's a paint() does > beginLayer()+reallyPaint()+endLayer() instead of multiple endLayer()+returns? > When in a single function, it would be nicer with an on-stack this with a > destructor, but that seems way overkill compared to splitting it up. indeed! https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:338: Color endColor; On 2015/07/09 13:40:56, philipj wrote: > So no end color at all for the new UI? Are we then drawing first a background > and then blue on top of it up to the slider? Could it be changed to just not > draw a background and instead paint a before and after bit in different colors? it depends on where the buffered range is. it doesn't need to start at the left edge of the slider bar. https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:454: if (RuntimeEnabledFeatures::newMediaPlaybackUiEnabled()) { On 2015/07/09 13:40:56, philipj wrote: > {} Done. https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:491: // with the new player UI, we have separate assets for enter / exit On 2015/07/09 13:40:56, philipj wrote: > s/with/With/ Done. https://codereview.chromium.org/1156993013/diff/750001/Source/platform/Runtim... File Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/1156993013/diff/750001/Source/platform/Runtim... Source/platform/RuntimeEnabledFeatures.in:100: NewMediaPlaybackUi status=stable On 2015/07/09 13:40:56, philipj wrote: > I think we should first land this with no status to ensure that all of the > existing tests work as they should with the old UI code in branches, then have a > separate CL that flips the flag and updates the test. Acknowledged. https://codereview.chromium.org/1156993013/diff/750001/public/web/WebRuntimeF... File public/web/WebRuntimeFeatures.h (right): https://codereview.chromium.org/1156993013/diff/750001/public/web/WebRuntimeF... public/web/WebRuntimeFeatures.h:163: BLINK_EXPORT static void enableNewMediaPlaybackUi(bool); i think that this can be removed.
https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:94: // +-MediaControlTimelineElement (-webkit-media-controls-timeline) (if !RTE::newMediaPlaybackUi()) On 2015/07/09 22:35:34, liberato wrote: > On 2015/07/09 08:56:10, philipj wrote: > > Can you use the same style as the above {if > > mediaControlsOverlayPlayButtonEnabled}? > > done. didn't realize that's what it meant. fs pulled the syntax out of a hat, and I just like consistency :) https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:216: m_volumeSlider->setIsWanted(false); On 2015/07/09 22:35:34, liberato wrote: > On 2015/07/09 08:56:10, philipj wrote: > > Ditto. > > this one always ends up worse with the #ifdef. Oh, here it wasn't actually m_volumeSlider and m_muteButton, not the same, never mind me. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:220: #if OS(ANDROID) On 2015/07/09 22:35:34, liberato wrote: > On 2015/07/09 08:56:10, philipj wrote: > > Should also be possible to do with just CSS. > > true, but this way it's all in one spot. otherwise, we have to remember not to > touch volume slider state in the case of android. Oh, so before it was always an option to hide using CSS, and it didn't matter what you did to the element. Now that doesn't work because of the hide logic that doesn't look at the computed style... Still don't want a OS ifdef here. I got rid of the last one by making the overlay fullscreen button a setting. Would that be an option? Something like settings()->mediaControlsVolumeSliderEnabled()? Possibly combined with the muted thing depending on how we decide on that. A setting makes it possible to write tests that won't have to run on Android too. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:400: // If the element is muted, then we want the unmute button. Otherwise, On 2015/07/09 22:35:34, liberato wrote: > On 2015/07/09 08:56:11, philipj wrote: > > I think that if we're going to change anything about this again, we should > just > > unmute and set volume to 1 when playing using the built-in controls. We don't > > show the volume slider even though volume can be set to 0, so the current > logic > > seems strange to me, and a waste of pixels in the vast majority of cases. > > > > Alternatively, show the mute button only for videos that are actually muted, > but > > of course leave it in after that point as well. > > the latter one seems almost exactly what the code does. if we have audio and > we're muted, then add the mute button. it's also very similar to what the > original patch for crbug.com/467242 did, which seemed to fix it. so, i suspect > that the problems were about the mute tag rather than volume==0. > > i agree that doing something with the volume is a good idea, but not sure what. > > i'm not sure that i understand the first suggestion -- unmuting automatically > seems like it might not go so well. FWIW, we unmute when pressing the overlay fullscreen button for the first time in Opera for Android as a way to remove the mute button, but somewhere where's possible/easy to play while muted would be nicer. I actually wasn't paying attention to the surrounding code here, I just assumed this was preserving the existing behavior. Not so. Can you write some tests for this (easier with a setting) to verify the conditions under which the button should be shown and not? A problem here is that ::reset() also fiddles with this, so I think the logic for when it's shown should be in one place. crbug.com/467242 doesn't look like the right bug. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:678: if (!m_castButton->isShown()) On 2015/07/09 22:35:33, liberato wrote: > On 2015/07/09 08:56:10, philipj wrote: > > I think this and above are the only places where isWanted() and isShown() are > > used, and it looks like it could be replaced by just remembering some state > from > > the loop. It would be nice if the price of ugliness for the cast button hack > > were localized and we don't have exposed state of the buttons that invites > more > > special casing in other contexts. > > > > Also merge into a single setIsWanted() call. > > i've removed isShown(), but i don't see how to remove isWanted. it's a > consequence of moving the state to the MediaElement. Uh right, the loop above is also using isWanted(), for good reasons, I searched badly. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... File Source/core/html/shadow/SliderThumbElement.cpp (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/SliderThumbElement.cpp:231: if (m_inDragMode && !m_didDragAnywhere && isMediaSlider()) { On 2015/07/09 22:35:35, liberato wrote: > On 2015/07/09 13:40:56, philipj wrote: > > I think what I would expect is that mousedown actually causes the slider to be > > centered under the pointer/finger. This seems to be how the YouTube Android > app > > works. Should simplify the changes in this class by a lot. > > > > Also, what are the other kinds of sliders, does this fix make sense for them > too > > or is the failure to budge a feature in some contexts? It seems somewhat > > unlikely that the behavior is intentional and tested. > > mousedown move: i considered that, but having the start of a drag also move the > the slider didn't feel right. drags are easy because they're relative to the > touch down. small movement => small adjustment. recentering means that getting > a small adjustment also requires a precisely located touch. > > other sliders: i expect that the behavior can be broadened, but i didn't want to > do it as part of this change. You're right, <input type=range> with a mouse would be weird if you couldn't just grab the handle and drag it. However, if you try that in a Desktop browser you'll see it's actually broken, just clicking does nothing, but as soon as you move the mouse it centers the handle under the mouse, so you do need to click very precisely to move it just a little. Not sure what the "right" behavior is, maybe discuss with your UX people? Having something different happen for <input type=range> and this slider doesn't seem justified, I think. https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... File Source/core/paint/MediaControlsPainter.cpp (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:47: // Current UI slider thumbs sizes. On 2015/07/09 22:35:36, liberato wrote: > On 2015/07/09 13:40:56, philipj wrote: > > Could these be turned into static functions to hide away the runtime checks? > > Same for sliderBackgroundColor and anything else where it would result in > fewer > > runtime checks in total. > > not sure that i understand. declaring these as const inlines all references and > doesn't emit any initialized data. i can declare these 'static const', just to > be sure. i checked this, and the assembly was identical on a test program with > clang++. I mean something like static int mediaSliderThumbWidth() { if (REF::newUI()) return 36; return 32; } for everything that depends on the runtime flag. https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:338: Color endColor; On 2015/07/09 22:35:35, liberato wrote: > On 2015/07/09 13:40:56, philipj wrote: > > So no end color at all for the new UI? Are we then drawing first a background > > and then blue on top of it up to the slider? Could it be changed to just not > > draw a background and instead paint a before and after bit in different > colors? > > it depends on where the buffered range is. it doesn't need to start at the left > edge of the slider bar. Oh, right, there are three colors, I was confused. https://codereview.chromium.org/1156993013/diff/750001/public/web/WebRuntimeF... File public/web/WebRuntimeFeatures.h (right): https://codereview.chromium.org/1156993013/diff/750001/public/web/WebRuntimeF... public/web/WebRuntimeFeatures.h:163: BLINK_EXPORT static void enableNewMediaPlaybackUi(bool); On 2015/07/09 22:35:36, liberato wrote: > i think that this can be removed. Yep.
refactored the show / hide volume / mute controls logic. added tests for it. it carefully avoids requiring knowledge of whether the new or old UI is in place, which makes everything easier. NOTE: this doesn't enable the new UI by default, nor does it hide the volume / mute button on android. it looks like the right place to add that is in RenderViewImpl::ApplyWebPreferences() inside existing ifdefs. i'll put out a separate CL for that after this lands. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:220: #if OS(ANDROID) On 2015/07/09 23:18:37, philipj wrote: > On 2015/07/09 22:35:34, liberato wrote: > > On 2015/07/09 08:56:10, philipj wrote: > > > Should also be possible to do with just CSS. > > > > true, but this way it's all in one spot. otherwise, we have to remember not > to > > touch volume slider state in the case of android. > > Oh, so before it was always an option to hide using CSS, and it didn't matter > what you did to the element. Now that doesn't work because of the hide logic > that doesn't look at the computed style... > > Still don't want a OS ifdef here. I got rid of the last one by making the > overlay fullscreen button a setting. Would that be an option? Something like > settings()->mediaControlsVolumeSliderEnabled()? Possibly combined with the muted > thing depending on how we decide on that. A setting makes it possible to write > tests that won't have to run on Android too. i've added settings()->preferHidden{VolumeSlider, MuteButton}(). i did it this way rather than ...Enabled() since the mute button can show up even if it's not enabled in that case. this way, they're symmetric. i've also added a flag to allow this behavior, since the old ui ignores it. the test works in both UIs that way.
https://codereview.chromium.org/1156993013/diff/830001/Source/core/frame/Sett... File Source/core/frame/Settings.in (right): https://codereview.chromium.org/1156993013/diff/830001/Source/core/frame/Sett... Source/core/frame/Settings.in:369: forceAllowHiddenAudioElements initial=false Setting that are for testing only can be put in Source/core/testing/InternalSettings.* testing/Internals.idl is also an option, you could use that to reach into MediaPlayer and tickle it directly, but it doesn't matter which way it's done I think.
https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:618: m_panelWidthChangedTimer.startOneShot(0, FROM_HERE); On 2015/07/09 12:25:39, fs wrote: > On 2015/07/09 12:10:57, liberato wrote: > > On 2015/07/09 09:31:17, fs wrote: > > > On 2015/07/09 08:56:10, philipj wrote: > > > > Given that this is called from LayoutMedia::layout() I take it the reason > > for > > > > this is to avoid some asserts about style invalidation during layout? > Still, > > > it > > > > seems like doing it on a timer could cause one bad frame to be rendered. > fs? > > > > > > Correct on both accounts AFAIK. > > > > > > > My hunch is that the only way to really make this work might be to move > the > > > > logic to layout somehow. > > > > > > Agreed. > > > > > > > It's true that JS controls couldn't do that, but if the > > > > controls were in an iframe and listened to the resize event, I presume > that > > > > updating the layout in the resize event would not cause one bad frame. At > > > least > > > > I hope there's actually a way for web content to do this kind of thing > > > > correctly, otherwise our web platform sucks! :) > > > > > > Our web platform sucks =). AFAIK, 'resize' is detected after layout and the > > > event dispatched from the rAF callback (ScriptedAnimationController). > > > Technically that's an "implementation detail" though of course - i.e. > nothing > > > preventing layout -> resize -> re-layout -> ... > > > > to be clear: i'm about to go spelunking into layout code. is this what you > both > > intend for me to do? > > Well, it's essentially a QoI issue - there could (will) be a single-frame glitch > when the "what fits?" algorithm decides to hide/unhide a control. Depending on > various factors (width-delta, padding, etc.) it'll end up looking less pretty > than it maybe could. I'm pretty sure that you - having seen this is action - > have a better feel for how "glitchy" it actually appears in practice though, so > as far as I'm concerned I'd leave it up to you to decided. philipj on the other > hand is perhaps a bit more adamant than I am ;-) So let's add a TODO that acknowledges that there's a problem here, in cast someone comes along and sees this code and either gets suspicious or wants to do something similar elsewhere. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.h (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.h:70: void hideCastButtons(); On 2015/07/09 22:35:35, liberato wrote: > On 2015/07/09 08:56:11, philipj wrote: > > Ought this not be equivalent to refreshCastButtonVisibilityWithoutUpdate() in > > the context it's called, i.e. when !mediaElement().hasRemoteRoutes()? It seems > > like in principle it should be enough with just refreshCastButtonVisibility() > > public, but cleaning up showOverlayCastButton() isn't your job if you don't > want > > it ;) > > i originally had it that way, but decided that it was confusing in > HTMLMediaElement since it wasn't clear what an 'Update' was. naming is one of > the three hardest problems in computer science, after all. > > i've removed hide...() and replaced with refresh...Update(). Acknowledged. https://codereview.chromium.org/1156993013/diff/830001/Source/core/frame/Sett... File Source/core/frame/Settings.in (right): https://codereview.chromium.org/1156993013/diff/830001/Source/core/frame/Sett... Source/core/frame/Settings.in:358: preferHiddenVolumeSlider initial=false Could these be collapsed into a single setting, after all I suppose they will always be in sync. https://codereview.chromium.org/1156993013/diff/830001/Source/core/html/shado... File Source/core/html/shadow/MediaControlElementTypes.h (right): https://codereview.chromium.org/1156993013/diff/830001/Source/core/html/shado... Source/core/html/shadow/MediaControlElementTypes.h:107: // if an only if we're wanted and we fit. s/an/and/ https://codereview.chromium.org/1156993013/diff/830001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/830001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:410: // sets it; it lets CSS choose. The new UI does. If we're allowing I would be nice if these descriptions were in terms of the settings and not old/new UI, or they'll have to be rewritten later. Commenting on this line because I couldn't parse "sets it". I think all of this would be simpler if the question of when to show/hide the mute button is decoupled from the old/new UI. To ship the new UI, first enable the runtime flag, then fiddle with the settings on Android. https://codereview.chromium.org/1156993013/diff/830001/Source/core/layout/Lay... File Source/core/layout/LayoutTheme.cpp (right): https://codereview.chromium.org/1156993013/diff/830001/Source/core/layout/Lay... Source/core/layout/LayoutTheme.cpp:238: int minutes = (seconds / 60); Don't need () https://codereview.chromium.org/1156993013/diff/830001/Source/core/layout/Lay... Source/core/layout/LayoutTheme.cpp:244: int durationMins = (durationSecs / 60); Ditto. https://codereview.chromium.org/1156993013/diff/830001/Source/core/paint/Medi... File Source/core/paint/MediaControlsPainter.cpp (right): https://codereview.chromium.org/1156993013/diff/830001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:498: // full screen mode. s/full screen/fullscreen/ (I obsess about this: http://blog.foolip.org/2014/07/21/history-of-the-fullscreen-api/)
moved test-only flags to Internals.idl. there's a patch at: https://drive.google.com/a/chromium.org/file/d/0B7ROFckpOJrgQzh2aHlDbmswRGM/v... thanks -fl https://codereview.chromium.org/1156993013/diff/830001/Source/core/frame/Sett... File Source/core/frame/Settings.in (right): https://codereview.chromium.org/1156993013/diff/830001/Source/core/frame/Sett... Source/core/frame/Settings.in:358: preferHiddenVolumeSlider initial=false On 2015/07/10 14:51:58, philipj wrote: > Could these be collapsed into a single setting, after all I suppose they will > always be in sync. i did that originally, but separated them for reasons that are no longer clear to me after a few hours sleep. i'll merge. https://codereview.chromium.org/1156993013/diff/830001/Source/core/frame/Sett... Source/core/frame/Settings.in:369: forceAllowHiddenAudioElements initial=false On 2015/07/10 14:21:35, philipj wrote: > Setting that are for testing only can be put in > Source/core/testing/InternalSettings.* > > testing/Internals.idl is also an option, you could use that to reach into > MediaPlayer and tickle it directly, but it doesn't matter which way it's done I > think. that's cool! I moved it to Internals.idl, since that's ten shades of awesome. https://codereview.chromium.org/1156993013/diff/830001/Source/core/html/shado... File Source/core/html/shadow/MediaControlElementTypes.h (right): https://codereview.chromium.org/1156993013/diff/830001/Source/core/html/shado... Source/core/html/shadow/MediaControlElementTypes.h:107: // if an only if we're wanted and we fit. On 2015/07/10 14:51:59, philipj wrote: > s/an/and/ done. https://codereview.chromium.org/1156993013/diff/830001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/830001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:410: // sets it; it lets CSS choose. The new UI does. If we're allowing On 2015/07/10 14:51:59, philipj wrote: > I would be nice if these descriptions were in terms of the settings and not > old/new UI, or they'll have to be rewritten later. Commenting on this line > because I couldn't parse "sets it". > > I think all of this would be simpler if the question of when to show/hide the > mute button is decoupled from the old/new UI. To ship the new UI, first enable > the runtime flag, then fiddle with the settings on Android. i agree that 'old' and 'new' are awful names. i've tried to improve it in the most recent CL, with mixed success. since most of the references are immediately around the REF checks. when the flag goes away, the comments should get changed at the same time. at least, that's the best i can come up with. :( https://codereview.chromium.org/1156993013/diff/830001/Source/core/layout/Lay... File Source/core/layout/LayoutTheme.cpp (right): https://codereview.chromium.org/1156993013/diff/830001/Source/core/layout/Lay... Source/core/layout/LayoutTheme.cpp:238: int minutes = (seconds / 60); On 2015/07/10 14:51:59, philipj wrote: > Don't need () Done. https://codereview.chromium.org/1156993013/diff/830001/Source/core/layout/Lay... Source/core/layout/LayoutTheme.cpp:244: int durationMins = (durationSecs / 60); On 2015/07/10 14:51:59, philipj wrote: > Ditto. Done. https://codereview.chromium.org/1156993013/diff/830001/Source/core/paint/Medi... File Source/core/paint/MediaControlsPainter.cpp (right): https://codereview.chromium.org/1156993013/diff/830001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:498: // full screen mode. On 2015/07/10 14:51:59, philipj wrote: > s/full screen/fullscreen/ > > (I obsess about this: > http://blog.foolip.org/2014/07/21/history-of-the-fullscreen-api/) :)
https://codereview.chromium.org/1156993013/diff/950001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1156993013/diff/950001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3589: if (mediaControls()) ensureMediaControls() instead of the null check would probably be good, so that the caller doesn't need to take care to add the controls attribute before using this API. https://codereview.chromium.org/1156993013/diff/950001/public/web/WebSettings.h File public/web/WebSettings.h (right): https://codereview.chromium.org/1156993013/diff/950001/public/web/WebSettings... public/web/WebSettings.h:208: virtual void setPreferHiddenAudioElements(bool) = 0; Can now be removed?
a link to the patch: https://drive.google.com/a/chromium.org/file/d/0B7ROFckpOJrgSlA2cGNtOW5ZNEk/v... this one: https://codereview.chromium.org/1226063007/ sets the feature to stable. thanks -fl https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... File Source/core/css/mediaControlsNew.css (right): https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:145: width: 100%; On 2015/06/17 17:07:25, liberato wrote: > On 2015/06/17 12:52:24, philipj wrote: > > This may result in a problem I had previously. If the clickable area for the > > overlay play button goes right up to the controls panel, trying to click stuff > > in the controls panel may result in a disambiguation zoom, which isn't great. > Is > > the intention of this change to allow clicking anywhere to play? > > good point. that's the intention. > > i can shrink it a bit and top align. is there a better way? i ended up shrinking this a bit. https://codereview.chromium.org/1156993013/diff/310001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:238: font-family: Noto Sans; On 2015/06/17 17:07:25, liberato wrote: > On 2015/06/17 12:52:24, philipj wrote: > > No fallback here? Maybe the font-family should be set at a higher level? > > added to my todo list. ux designers provided a fallback list. https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/cont... File LayoutTests/media/controls-volume-slider.html (right): https://codereview.chromium.org/1156993013/diff/710001/LayoutTests/media/cont... LayoutTests/media/controls-volume-slider.html:17: // Click at 25% of the volume slider, to avoid hitting On 2015/07/09 12:10:53, liberato wrote: > On 2015/07/08 10:31:51, philipj wrote: > > Perhaps this isn't a new bug, but now that the area is bigger, it's quite > > noticeable and annoying that one can't click even slightly outside the blue > > circle to seek. Clicking and then dragging a pixel does work, so it has a > > glitchy feeling to it. Can the behavior be changed instead of the test? > > sounds good. done. https://codereview.chromium.org/1156993013/diff/710001/Source/core/css/mediaC... File Source/core/css/mediaControlsAndroidNew.css (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/css/mediaC... Source/core/css/mediaControlsAndroidNew.css:3: * Copyright (C) 2009 Apple Inc. All rights reserved. On 2015/07/09 12:10:56, liberato wrote: > On 2015/07/08 10:31:52, philipj wrote: > > I would keep the copyright header from which this was copied, or use the new > > smaller one if it wasn't copied. Changing years and adding lines over time is > > tedious :) > > i'll update this with the de-crufting to match mediaControlsAndroid.css, from > which it came. copied from mediaControlsAndroid.css . https://codereview.chromium.org/1156993013/diff/710001/Source/core/css/mediaC... File Source/core/css/mediaControlsNew.css (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/css/mediaC... Source/core/css/mediaControlsNew.css:120: padding: 12px; On 2015/07/09 12:10:56, liberato wrote: > On 2015/07/08 10:31:52, philipj wrote: > > The amount of whitespace around the mute button looks really excessive, and > was > > pointed out by some people I showed the new controls to internally. It makes > it > > a bit non-obvious that the second slider is related to the volume button at > all. > > Does this match what was intended in the mockups? I'm no designer, but > something > > closer to the amount of space between the play button and the duration text > > would seem better. > > i've forwarded this feedback to the design team. thanks. the new whitespace is from the style guide, and is intended. https://codereview.chromium.org/1156993013/diff/710001/Source/core/paint/Medi... File Source/core/paint/MediaControlsPainter.cpp (right): https://codereview.chromium.org/1156993013/diff/710001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:117: // The new UI uses "muted" and "not muted" only. On 2015/07/09 12:10:56, liberato wrote: > On 2015/07/08 10:31:52, philipj wrote: > > Is this by design? I was expecting the "waves" to follow along when dragging > the > > slider, but they don't. > > i'll verify that this wasn't overlooked, but i believe that it is intentional. it is intentional. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:400: // If the element is muted, then we want the unmute button. Otherwise, On 2015/07/09 23:18:36, philipj wrote: > On 2015/07/09 22:35:34, liberato wrote: > > On 2015/07/09 08:56:11, philipj wrote: > > > I think that if we're going to change anything about this again, we should > > just > > > unmute and set volume to 1 when playing using the built-in controls. We > don't > > > show the volume slider even though volume can be set to 0, so the current > > logic > > > seems strange to me, and a waste of pixels in the vast majority of cases. > > > > > > Alternatively, show the mute button only for videos that are actually muted, > > but > > > of course leave it in after that point as well. > > > > the latter one seems almost exactly what the code does. if we have audio and > > we're muted, then add the mute button. it's also very similar to what the > > original patch for crbug.com/467242 did, which seemed to fix it. so, i > suspect > > that the problems were about the mute tag rather than volume==0. > > > > i agree that doing something with the volume is a good idea, but not sure > what. > > > > i'm not sure that i understand the first suggestion -- unmuting automatically > > seems like it might not go so well. > > FWIW, we unmute when pressing the overlay fullscreen button for the first time > in Opera for Android as a way to remove the mute button, but somewhere where's > possible/easy to play while muted would be nicer. > > I actually wasn't paying attention to the surrounding code here, I just assumed > this was preserving the existing behavior. Not so. Can you write some tests for > this (easier with a setting) to verify the conditions under which the button > should be shown and not? A problem here is that ::reset() also fiddles with > this, so I think the logic for when it's shown should be in one place. > > crbug.com/467242 doesn't look like the right bug. added LayoutTests/media/video-controls-hidden-audio.html . https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:472: m_castButton->setIsWanted(false); On 2015/07/09 22:35:34, liberato wrote: > On 2015/07/09 08:56:10, philipj wrote: > > The mismatch between showOverlayCastButton() and > > m_castButton->setIsWanted(false) looks odd. Any idea why we can't just call > > m_overlayCastButton->setIsWanted(true) here? It seems strange to reset the > hide > > timer in a function like this that one should expect to be called to > potentially > > invalidate state, but have no side effect if everything is already in the > > correct state. > > yeah, i agree. changed. it turns out that the right answer is tryShowOverlayCast, else it doesn't notice if there is an element over the player. that should cause cast not to show up. luckily, it also failed a test, which is the only reason i realized the difference. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:618: m_panelWidthChangedTimer.startOneShot(0, FROM_HERE); On 2015/07/10 14:51:58, philipj wrote: > On 2015/07/09 12:25:39, fs wrote: > > On 2015/07/09 12:10:57, liberato wrote: > > > On 2015/07/09 09:31:17, fs wrote: > > > > On 2015/07/09 08:56:10, philipj wrote: > > > > > Given that this is called from LayoutMedia::layout() I take it the > reason > > > for > > > > > this is to avoid some asserts about style invalidation during layout? > > Still, > > > > it > > > > > seems like doing it on a timer could cause one bad frame to be rendered. > > fs? > > > > > > > > Correct on both accounts AFAIK. > > > > > > > > > My hunch is that the only way to really make this work might be to move > > the > > > > > logic to layout somehow. > > > > > > > > Agreed. > > > > > > > > > It's true that JS controls couldn't do that, but if the > > > > > controls were in an iframe and listened to the resize event, I presume > > that > > > > > updating the layout in the resize event would not cause one bad frame. > At > > > > least > > > > > I hope there's actually a way for web content to do this kind of thing > > > > > correctly, otherwise our web platform sucks! :) > > > > > > > > Our web platform sucks =). AFAIK, 'resize' is detected after layout and > the > > > > event dispatched from the rAF callback (ScriptedAnimationController). > > > > Technically that's an "implementation detail" though of course - i.e. > > nothing > > > > preventing layout -> resize -> re-layout -> ... > > > > > > to be clear: i'm about to go spelunking into layout code. is this what you > > both > > > intend for me to do? > > > > Well, it's essentially a QoI issue - there could (will) be a single-frame > glitch > > when the "what fits?" algorithm decides to hide/unhide a control. Depending on > > various factors (width-delta, padding, etc.) it'll end up looking less pretty > > than it maybe could. I'm pretty sure that you - having seen this is action - > > have a better feel for how "glitchy" it actually appears in practice though, > so > > as far as I'm concerned I'd leave it up to you to decided. philipj on the > other > > hand is perhaps a bit more adamant than I am ;-) > > So let's add a TODO that acknowledges that there's a problem here, in cast > someone comes along and sees this code and either gets suspicious or wants to do > something similar elsewhere. Done. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:641: void MediaControls::computeWhichControlsFit(int panelWidth) On 2015/07/09 22:35:33, liberato wrote: > On 2015/07/09 08:56:10, philipj wrote: > > Seems like this doesn't need an argument, use m_panelWidth? > > yeah, i had the same thought. i did it this way so that one doesn't > accidentally call computeWhichControlsFit() when one really wants to get the > most current width first. > > however, i'll try removing all of changedControlSelections(), and just get the > width via notifyPanelWidthChanged(), then it'll be much simpler. hopefully that > works. > > i'm just not convinced yet that it won't introduce an extra bad frame. this does cause an extra bad frame, so i've left it as-is. https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... File Source/core/html/shadow/SliderThumbElement.cpp (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/html/shado... Source/core/html/shadow/SliderThumbElement.cpp:231: if (m_inDragMode && !m_didDragAnywhere && isMediaSlider()) { On 2015/07/09 23:18:37, philipj wrote: > On 2015/07/09 22:35:35, liberato wrote: > > On 2015/07/09 13:40:56, philipj wrote: > > > I think what I would expect is that mousedown actually causes the slider to > be > > > centered under the pointer/finger. This seems to be how the YouTube Android > > app > > > works. Should simplify the changes in this class by a lot. > > > > > > Also, what are the other kinds of sliders, does this fix make sense for them > > too > > > or is the failure to budge a feature in some contexts? It seems somewhat > > > unlikely that the behavior is intentional and tested. > > > > mousedown move: i considered that, but having the start of a drag also move > the > > the slider didn't feel right. drags are easy because they're relative to the > > touch down. small movement => small adjustment. recentering means that > getting > > a small adjustment also requires a precisely located touch. > > > > other sliders: i expect that the behavior can be broadened, but i didn't want > to > > do it as part of this change. > > You're right, <input type=range> with a mouse would be weird if you couldn't > just grab the handle and drag it. However, if you try that in a Desktop browser > you'll see it's actually broken, just clicking does nothing, but as soon as you > move the mouse it centers the handle under the mouse, so you do need to click > very precisely to move it just a little. > > Not sure what the "right" behavior is, maybe discuss with your UX people? Having > something different happen for <input type=range> and this slider doesn't seem > justified, I think. i'll open a discussion, but i'd prefer not to connect it with this change. the other sliders aren't causing problems, but the media sliders are. https://codereview.chromium.org/1156993013/diff/750001/Source/core/layout/Lay... File Source/core/layout/LayoutTheme.cpp (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/layout/Lay... Source/core/layout/LayoutTheme.cpp:264: // 10-60 minutes duration is 00:00 On 2015/07/09 22:35:35, liberato wrote: > On 2015/07/09 13:40:56, philipj wrote: > > Shouldn't this be 10-99? A 61 minute movie shouldn't show as 061:00 should it? > > i've asked the UX designers to verify the intention. sorry, forgot to update this one. the 61:00 is intended. https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... File Source/core/paint/MediaControlsPainter.cpp (right): https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:47: // Current UI slider thumbs sizes. On 2015/07/09 23:18:37, philipj wrote: > On 2015/07/09 22:35:36, liberato wrote: > > On 2015/07/09 13:40:56, philipj wrote: > > > Could these be turned into static functions to hide away the runtime checks? > > > Same for sliderBackgroundColor and anything else where it would result in > > fewer > > > runtime checks in total. > > > > not sure that i understand. declaring these as const inlines all references > and > > doesn't emit any initialized data. i can declare these 'static const', just > to > > be sure. i checked this, and the assembly was identical on a test program > with > > clang++. > > I mean something like static int mediaSliderThumbWidth() { if (REF::newUI()) > return 36; return 32; } for everything that depends on the runtime flag. tried this -- ended up being harder to follow. the problem is that old:new don't have the same cases, so the logic just gets more spread out. for example, both old and new paths have duplicated constants, but it's unclear that they're actually they should change together. https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:59: // New UI overlay play button size. On 2015/07/09 13:40:56, philipj wrote: > I don't think I understand this bit, can't the size just be set in CSS? unfortunately, not that i can see. i arrived here after failing to get the behavior with css alone. in particular, the touch area should be full screen, but the painter draws the entire content + padding by default. margin doesn't seem to be touchable. so, i made it draw a correctly sized icon in the middle of the touch target. might have been able to do it with a pseudo-element, but some special painting would still be required. https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:177: // Overlay play button is full-screen, so center. On 2015/07/09 13:40:56, philipj wrote: > We don't know here if we're in fullscreen or not, do we? If this is to implement > the logic that the button should be centered on screen in fullscreen but > centered excluding the controls panel while inline, that's possible to implement > in CSS, we actually have a patch for this in Opera for Android, assuming the old > sizes: > > /* In fullscreen, center the button on the video, ignoring the panel height. */ > video:-webkit-full-screen::-webkit-media-controls-overlay-play-button { > margin-top: -14px; /* 40/2 - 34 */ > } it's not trying to adjust for the control bar. that's already done in CSS, if i remember. the comment here is wrong, shouldn't say "full screen". will update to "entire player". however, that element covers the whole player so that it's touch target is large. here, we just draw the icon with the original size. https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:196: // If we're using the new UI, create a short, vertically centered rect. On 2015/07/09 13:40:56, philipj wrote: > The current height is 8px but it still works fine to click outside of it to > seek. What happens is this is set to 2px using CSS? The slider uses Shadow DOM > internally so I'm guessing that the height of the slider rect is separate from > the height of the clickable area already? yes, it seems to be. weird. that was the first thing i tried, and the thing was untouchable. i must have broken something else inadvertently. anyway, i removed adjustSliderRectIfNewUi(). thanks! https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:206: adjustedRect.setHeight(zoomedHeight); On 2015/07/09 13:40:56, philipj wrote: > I guess this will be rounded to zero if the zoom level of the page is <25%? If > FloatRect is returned then perhaps a blurry line will be rendered instead. (Not > really a new problem, just that 2 is a smaller number than 8.) it does disappear. i switched to float, and it does show gray bars. also uncovered a bug in zoom with the new layout code -- we need to adjust for effective zoom when a layout is triggered by resize now. https://codereview.chromium.org/1156993013/diff/750001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:287: bool drawNewUiGrayed = !hasSource(mediaElement) && RuntimeEnabledFeatures::newMediaPlaybackUiEnabled(); On 2015/07/09 13:40:56, philipj wrote: > Can probably drop "new" here lest it be left when the flag is gone. Done. https://codereview.chromium.org/1156993013/diff/950001/Source/core/html/HTMLM... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1156993013/diff/950001/Source/core/html/HTMLM... Source/core/html/HTMLMediaElement.cpp:3589: if (mediaControls()) On 2015/07/14 11:16:22, philipj wrote: > ensureMediaControls() instead of the null check would probably be good, so that > the caller doesn't need to take care to add the controls attribute before using > this API. done. i chose the null check because it's unclear what this function should do if there are no controls. i considered an assert, since it's a test hook. -fl https://codereview.chromium.org/1156993013/diff/950001/Source/core/paint/Medi... File Source/core/paint/MediaControlsPainter.cpp (right): https://codereview.chromium.org/1156993013/diff/950001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:51: static const int mediaVolumeSliderThumbWidth = 24; i didn't move these into functions. the result was more difficult to follow, since the constants aren't 1:1 between old:new. the weirdness just got spread out more. deprecating the old ui is the best way to make it simpler, that i can see. https://codereview.chromium.org/1156993013/diff/950001/public/web/WebSettings.h File public/web/WebSettings.h (right): https://codereview.chromium.org/1156993013/diff/950001/public/web/WebSettings... public/web/WebSettings.h:208: virtual void setPreferHiddenAudioElements(bool) = 0; On 2015/07/14 11:16:22, philipj wrote: > Can now be removed? thanks!
Took another pass. Sorry if some issues have been discussed once or twice already, it's very hard to keep track of issues over time in this review. https://codereview.chromium.org/1156993013/diff/990001/Source/core/css/mediaC... File Source/core/css/mediaControlsAndroidNew.css (right): https://codereview.chromium.org/1156993013/diff/990001/Source/core/css/mediaC... Source/core/css/mediaControlsAndroidNew.css:27: /* WARNING: This css file can only style <audio> and <video> elements */ Is there still a plan for this file to contain something? If not it can just be removed. https://codereview.chromium.org/1156993013/diff/990001/Source/core/frame/Sett... File Source/core/frame/Settings.in (right): https://codereview.chromium.org/1156993013/diff/990001/Source/core/frame/Sett... Source/core/frame/Settings.in:361: preferHiddenAudioElements initial=false An audio element is <audio>, perhaps preferHiddenVolumeControls? https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:149: Revert extra blank line https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:204: m_durationDisplay->setIsWanted(true); Can fold this into a single setIsWanted call. https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:382: if (mediaElement().muted()) { Drop the extra {}, presumably left as part of a now-reverted change. https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:398: // instead let it up to the CSS. s/let/leave/ https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:612: if (panelWidth == 0) Repeat of earlier issue, why not update m_panelWidth first? https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:630: int panelWidth = m_panel->clientWidth(); When is this needed, is it not redundant with notifyPanelWidthChanged? It seems like what ought to happen is that any time a control changes its isWanted state, computeWhichControlsFit() should be called, right? This is currently called in reset(), but there ought to be many cases where a control is removed without recomputing. An easy case to test with is the CC button, where removing all tracks should cause it to go away without a reset() call. https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:678: const int minimumWidth = element->minimumWidth(); Move this inside the branch where it is used. https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:701: m_keepMuteButton = false; Do you also want it to be reset when loading a new media resource? MediaControls so far does not have any persistent state, so that calling reset() will always (modulo bugs) have the same effect. https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... File Source/core/html/shadow/SliderThumbElement.cpp (right): https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/SliderThumbElement.cpp:232: // We got a down / up with no drag in between, and we're a media I don't think this is quite right. When you click anywhere outside the slider, the position is updated on mousedown and then you can drag with the "thumb element" centered under the mouse/touch point. This also seems to be the behavior of YouTube. I think the desired behavior can be implemented by removing the |targetNode != element()| bit from RangeInputType::handleMouseDownEvent and thus not have any changes to this file. If you would like to limit the change to the media controls, please add a TODO and a bug to do it after a Blink roll or two, so that it can be separated out in a bisect, but to leave the difference indefinitely seems strange. https://codereview.chromium.org/1156993013/diff/990001/Source/core/layout/Lay... File Source/core/layout/LayoutMedia.cpp (right): https://codereview.chromium.org/1156993013/diff/990001/Source/core/layout/Lay... Source/core/layout/LayoutMedia.cpp:94: mediaElement()->mediaControls()->notifyPanelWidthChanged(newSize.width().toInt()); Is there any particular reason to convert to int here instead of passing a LayoutUnit? It wouldn't make much of a difference, but would make it more explicit in the media controls code that this comes from layout, and make it harder to update it from offsetWidth() or similar. https://codereview.chromium.org/1156993013/diff/990001/Source/core/paint/Medi... File Source/core/paint/MediaControlsPainter.cpp (right): https://codereview.chromium.org/1156993013/diff/990001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:174: // Overlay play button covers the entire player, so center and draw a Unresolved earlier issue: this ought to lead to a disambiguation zoom when clicking close to the edge of the controls, which caused a similar earlier change to be reverted: https://codereview.chromium.org/867063003 Also, this will get in the way of implemented tap to show/hide controls: https://codereview.chromium.org/914043002/ https://codereview.chromium.org/1156993013/diff/990001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:369: int zoomedPaintHeight = mediaSliderThumbPaintHeightNew * zoomLevel; Should this also use float sizes? https://codereview.chromium.org/1156993013/diff/990001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:482: bool isEnabled = hasSource(mediaElement); Is a change in the fullscreen button visibility and enabled state intended? The combination of hasSource(mediaElement) here with mediaElement().hasVideo() in MediaControls::reset() seems strange, because hasVideo() implies hasSource(), so isEnabled ought to always be true here? https://codereview.chromium.org/1156993013/diff/990001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:501: bool isEnabled = hasSource(mediaElement); This is also new. The entire panel is grayed out if !hasSource() for the new UI and the enabled flag is ignored for the old, so will this change be observable? It seems to me that if any button is to be disabled, it should be based on the relevant state (presence of text tracks in this case) and not the top-level hasSource() condition. https://codereview.chromium.org/1156993013/diff/990001/public/web/WebRuntimeF... File public/web/WebRuntimeFeatures.h (right): https://codereview.chromium.org/1156993013/diff/990001/public/web/WebRuntimeF... public/web/WebRuntimeFeatures.h:163: BLINK_EXPORT static void enableNewMediaPlaybackUi(bool); Can be removed?
The problem of inconsistent hiding of the controls when dragging the mouse outside of the bottom edge of the video also remains.
added batched updates for controls visibility, to control how often we update it. some inner function during init are also called externally. could have done a refactor, but it's hard enough to keep rebased as it is. also fixed the "mouse out" bug, where controls don't fade. was present in the original ui, just harder to trigger. thanks -fl https://codereview.chromium.org/1156993013/diff/990001/Source/core/css/mediaC... File Source/core/css/mediaControlsAndroidNew.css (right): https://codereview.chromium.org/1156993013/diff/990001/Source/core/css/mediaC... Source/core/css/mediaControlsAndroidNew.css:27: /* WARNING: This css file can only style <audio> and <video> elements */ On 2015/07/21 12:02:21, philipj wrote: > Is there still a plan for this file to contain something? If not it can just be > removed. it now looks like we're going to separate desktop and android slightly, so i'd like to keep this around. i'm going to enable the new UI only for android first, and address desktop in some future CL after the spec has been updated. i do not want to move everything into the android CSS. keeping it available (hidden) for desktop will help with usability testing. https://codereview.chromium.org/1156993013/diff/990001/Source/core/frame/Sett... File Source/core/frame/Settings.in (right): https://codereview.chromium.org/1156993013/diff/990001/Source/core/frame/Sett... Source/core/frame/Settings.in:361: preferHiddenAudioElements initial=false On 2015/07/21 12:02:21, philipj wrote: > An audio element is <audio>, perhaps preferHiddenVolumeControls? Done. https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... File Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/MediaControlElements.cpp:76: || type == EventTypeNames::mouseout this is the cause of the "takes a while to hide the panel after mouseout" bug that you mentioned. it only happens if one triggers a mouseout over one of the two sliders. it wasn't as visible in the old UI since there was a small gap between the bottom of the panel and the bottom of the media element, which usually triggered another mouseout. i modified this check to include mouse{over,out,move} only if the scroller is in drag mode. however, i didn't see any difference in behavior if one simply never checks for them. when the slider captures the event, the mouseouts get eaten somewhere anyway. https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:149: On 2015/07/21 12:02:21, philipj wrote: > Revert extra blank line Done. https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:204: m_durationDisplay->setIsWanted(true); On 2015/07/21 12:02:21, philipj wrote: > Can fold this into a single setIsWanted call. done. don't know why this wasn't one line to begin with -- i kept many existing cases separate originally to keep the diff more readable. easier to find changes in old ui behavior that way. https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:382: if (mediaElement().muted()) { On 2015/07/21 12:02:21, philipj wrote: > Drop the extra {}, presumably left as part of a now-reverted change. Done. https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:398: // instead let it up to the CSS. On 2015/07/21 12:02:21, philipj wrote: > s/let/leave/ Done. https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:612: if (panelWidth == 0) On 2015/07/21 12:02:21, philipj wrote: > Repeat of earlier issue, why not update m_panelWidth first? this isn't the same. this is the callback that notfies about new width. the old issues was with computeWhichControlsFit(), which no longer takes panelWidth as an argument. https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:630: int panelWidth = m_panel->clientWidth(); On 2015/07/21 12:02:21, philipj wrote: > When is this needed, is it not redundant with notifyPanelWidthChanged? > > It seems like what ought to happen is that any time a control changes its > isWanted state, computeWhichControlsFit() should be called, right? This is > currently called in reset(), but there ought to be many cases where a control is > removed without recomputing. An easy case to test with is the CC button, where > removing all tracks should cause it to go away without a reset() call. when visibility changes, one should call changedControlSelections(). It grabs the clientWidth to update m_panelWidth. especially during initial construction, this is helpful. letting the panel width show up via notifyPanelWidthChanged() caused an extra bad frame, unfortunately. i'll verify why it no longer calls this more often -- it did before, and i removed it. ... not sure. Added calls more often, and included a simple batching mechanism so that only the outermost call actually calls changedControlSelections(). there were a few routines that were called both as top-level entry points and as part of some other top-level entry. factoring into top-level and internal calls wouldn't have helped, since some setIsWanted() calls are conditional. https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:678: const int minimumWidth = element->minimumWidth(); On 2015/07/21 12:02:21, philipj wrote: > Move this inside the branch where it is used. Done. https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:701: m_keepMuteButton = false; On 2015/07/21 12:02:21, philipj wrote: > Do you also want it to be reset when loading a new media resource? MediaControls > so far does not have any persistent state, so that calling reset() will always > (modulo bugs) have the same effect. good point, will reset in reset(). https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... File Source/core/html/shadow/SliderThumbElement.cpp (right): https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/SliderThumbElement.cpp:232: // We got a down / up with no drag in between, and we're a media On 2015/07/21 12:02:21, philipj wrote: > I don't think this is quite right. When you click anywhere outside the slider, > the position is updated on mousedown and then you can drag with the "thumb > element" centered under the mouse/touch point. This also seems to be the > behavior of YouTube. > > I think the desired behavior can be implemented by removing the |targetNode != > element()| bit from RangeInputType::handleMouseDownEvent and thus not have any > changes to this file. > > If you would like to limit the change to the media controls, please add a TODO > and a bug to do it after a Blink roll or two, so that it can be separated out in > a bisect, but to leave the difference indefinitely seems strange. i agree in principle, but i don't have enough experience to say whether the broader change is okay. since you do, and since you seem fairly confident that it is okay, i'll do it now. https://codereview.chromium.org/1156993013/diff/990001/Source/core/layout/Lay... File Source/core/layout/LayoutMedia.cpp (right): https://codereview.chromium.org/1156993013/diff/990001/Source/core/layout/Lay... Source/core/layout/LayoutMedia.cpp:94: mediaElement()->mediaControls()->notifyPanelWidthChanged(newSize.width().toInt()); On 2015/07/21 12:02:21, philipj wrote: > Is there any particular reason to convert to int here instead of passing a > LayoutUnit? It wouldn't make much of a difference, but would make it more > explicit in the media controls code that this comes from layout, and make it > harder to update it from offsetWidth() or similar. Done. https://codereview.chromium.org/1156993013/diff/990001/Source/core/paint/Medi... File Source/core/paint/MediaControlsPainter.cpp (right): https://codereview.chromium.org/1156993013/diff/990001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:174: // Overlay play button covers the entire player, so center and draw a On 2015/07/21 12:02:21, philipj wrote: > Unresolved earlier issue: this ought to lead to a disambiguation zoom when > clicking close to the edge of the controls, which caused a similar earlier > change to be reverted: > https://codereview.chromium.org/867063003 > > Also, this will get in the way of implemented tap to show/hide controls: > https://codereview.chromium.org/914043002/ disambiguation: there's a 10px border between the panel and the overlay in the css. i've never seen a disambiguation zoom. the paint does need to be adjusted. show/hide: thanks for pointing this out. based on this, i prototyped a similar approach in MediaControls::defaultEventHandler. it seems to work fine in the new UI. i'm not going to include it in this CL, but might put it out as a follow-up for both old and new. https://codereview.chromium.org/1156993013/diff/990001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:369: int zoomedPaintHeight = mediaSliderThumbPaintHeightNew * zoomLevel; On 2015/07/21 12:02:21, philipj wrote: > Should this also use float sizes? Done. https://codereview.chromium.org/1156993013/diff/990001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:482: bool isEnabled = hasSource(mediaElement); On 2015/07/21 12:02:21, philipj wrote: > Is a change in the fullscreen button visibility and enabled state intended? The > combination of hasSource(mediaElement) here with mediaElement().hasVideo() in > MediaControls::reset() seems strange, because hasVideo() implies hasSource(), so > isEnabled ought to always be true here? yes, it should be true always. didn't assume that here just because it seemed weird to make this one different. https://codereview.chromium.org/1156993013/diff/990001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:501: bool isEnabled = hasSource(mediaElement); On 2015/07/21 12:02:21, philipj wrote: > This is also new. The entire panel is grayed out if !hasSource() for the new UI > and the enabled flag is ignored for the old, so will this change be observable? > It seems to me that if any button is to be disabled, it should be based on the > relevant state (presence of text tracks in this case) and not the top-level > hasSource() condition. there's no case where !hasSource() and we have a CC button. however, i didn't want to assume that here. so it's not observable, but leaving out the check seems weird, as above. the goal was to have uniform checks to gray out the UI, without memorizing how MediaControls works. anyway, i modified this and the cast check to be more specific. the fullscreen button is left as-is, since the check for that is more complicated. https://codereview.chromium.org/1156993013/diff/990001/public/web/WebRuntimeF... File public/web/WebRuntimeFeatures.h (right): https://codereview.chromium.org/1156993013/diff/990001/public/web/WebRuntimeF... public/web/WebRuntimeFeatures.h:163: BLINK_EXPORT static void enableNewMediaPlaybackUi(bool); On 2015/07/21 12:02:21, philipj wrote: > Can be removed? Done.
Gone through the comments, haven't looked much at the code changes yet. https://codereview.chromium.org/1156993013/diff/990001/Source/core/css/mediaC... File Source/core/css/mediaControlsAndroidNew.css (right): https://codereview.chromium.org/1156993013/diff/990001/Source/core/css/mediaC... Source/core/css/mediaControlsAndroidNew.css:27: /* WARNING: This css file can only style <audio> and <video> elements */ On 2015/07/27 20:26:08, liberato wrote: > On 2015/07/21 12:02:21, philipj wrote: > > Is there still a plan for this file to contain something? If not it can just > be > > removed. > > it now looks like we're going to separate desktop and android slightly, so i'd > like to keep this around. i'm going to enable the new UI only for android > first, and address desktop in some future CL after the spec has been updated. > > i do not want to move everything into the android CSS. keeping it available > (hidden) for desktop will help with usability testing. Acknowledged. https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... File Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/MediaControlElements.cpp:76: || type == EventTypeNames::mouseout On 2015/07/27 20:26:08, liberato wrote: > this is the cause of the "takes a while to hide the panel after mouseout" bug > that you mentioned. it only happens if one triggers a mouseout over one of the > two sliders. it wasn't as visible in the old UI since there was a small gap > between the bottom of the panel and the bottom of the media element, which > usually triggered another mouseout. > > i modified this check to include mouse{over,out,move} only if the scroller is in > drag mode. > > however, i didn't see any difference in behavior if one simply never checks for > them. when the slider captures the event, the mouseouts get eaten somewhere > anyway. That makes sense. I made a demo for myself to see how this works: http://software.hixie.ch/utilities/js/live-dom-viewer/saved/3574 (Add the gap class to the inner div to see the old behavior.) From this, I gather that the mouseover and mouseout logic in MediaControls::defaultEventHandler could actually just use mouseenter and mouseleave and be simplified somewhat as a consequence. (In this CL only if it's a cleaner fix.) Like you, I can't actually see that these checks are needed, while one is dragging a slider it doesn't seem like any of the events are fired anyway. The origin of this is https://codereview.chromium.org/406213002 but it's possible that more than necessary was done to fix the "the click event isn't being blocked on the time and volume sliders" problem. Could try what happens if one simply replaces isUserInteractionEventForSlider() with isUserInteractionEvent()? It's a rare thing when things can be fixed by just removing code, but perhaps this is such an occasion. https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:612: if (panelWidth == 0) On 2015/07/27 20:26:08, liberato wrote: > On 2015/07/21 12:02:21, philipj wrote: > > Repeat of earlier issue, why not update m_panelWidth first? > > this isn't the same. this is the callback that notfies about new width. the > old issues was with computeWhichControlsFit(), which no longer takes panelWidth > as an argument. Oh, OK. In this case, would it break anything if m_panelWidth were first set and then the check was done? It's nice if the state is always in sync, if it isn't one might start to look for a reason. https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:630: int panelWidth = m_panel->clientWidth(); On 2015/07/27 20:26:08, liberato wrote: > On 2015/07/21 12:02:21, philipj wrote: > > When is this needed, is it not redundant with notifyPanelWidthChanged? > > > > It seems like what ought to happen is that any time a control changes its > > isWanted state, computeWhichControlsFit() should be called, right? This is > > currently called in reset(), but there ought to be many cases where a control > is > > removed without recomputing. An easy case to test with is the CC button, where > > removing all tracks should cause it to go away without a reset() call. > > when visibility changes, one should call changedControlSelections(). It grabs > the clientWidth to update m_panelWidth. especially during initial construction, > this is helpful. > > letting the panel width show up via notifyPanelWidthChanged() caused an extra > bad frame, unfortunately. > > i'll verify why it no longer calls this more often -- it did before, and i > removed it. > > ... not sure. Added calls more often, and included a simple batching mechanism > so that only the outermost call actually calls changedControlSelections(). > there were a few routines that were called both as top-level entry points and as > part of some other top-level entry. factoring into top-level and internal calls > wouldn't have helped, since some setIsWanted() calls are conditional. Hmm, so it seems that if this helps avoid a bad frame, it's because clientWidth() is forcing a layout instead of waiting for the callback from layout. The new documentation of changedControlSelections() is helpful, but can calling setIsWanted() ever actually change the size of the controls (I think no) or does this forced layout help us learn the size a frame earlier in some scenario? Put differently, when would an ASSERT(m_panelWidth == m_panel->clientWidth()) at the top of this function fail? An explicit document().updateLayoutIgnorePendingStylesheets() and clearing the timeout might help the responsibilities more clear, although so might more documentation. https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... File Source/core/html/shadow/SliderThumbElement.cpp (right): https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/SliderThumbElement.cpp:232: // We got a down / up with no drag in between, and we're a media On 2015/07/27 20:26:09, liberato wrote: > On 2015/07/21 12:02:21, philipj wrote: > > I don't think this is quite right. When you click anywhere outside the slider, > > the position is updated on mousedown and then you can drag with the "thumb > > element" centered under the mouse/touch point. This also seems to be the > > behavior of YouTube. > > > > I think the desired behavior can be implemented by removing the |targetNode != > > element()| bit from RangeInputType::handleMouseDownEvent and thus not have any > > changes to this file. > > > > If you would like to limit the change to the media controls, please add a TODO > > and a bug to do it after a Blink roll or two, so that it can be separated out > in > > a bisect, but to leave the difference indefinitely seems strange. > > i agree in principle, but i don't have enough experience to say whether the > broader change is okay. since you do, and since you seem fairly confident that > it is okay, i'll do it now. It definitely is possible that the change will result in bug reports, and if that happens let's hope we remember that this was part of the CL. Perhaps mention it in the description. If you're more paranoid, you could also do it in a separate CL before or after this. https://codereview.chromium.org/1156993013/diff/990001/Source/core/paint/Medi... File Source/core/paint/MediaControlsPainter.cpp (right): https://codereview.chromium.org/1156993013/diff/990001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:174: // Overlay play button covers the entire player, so center and draw a On 2015/07/27 20:26:09, liberato wrote: > On 2015/07/21 12:02:21, philipj wrote: > > Unresolved earlier issue: this ought to lead to a disambiguation zoom when > > clicking close to the edge of the controls, which caused a similar earlier > > change to be reverted: > > https://codereview.chromium.org/867063003 > > > > Also, this will get in the way of implemented tap to show/hide controls: > > https://codereview.chromium.org/914043002/ > > disambiguation: there's a 10px border between the panel and the overlay in the > css. i've never seen a disambiguation zoom. the paint does need to be > adjusted. > > show/hide: thanks for pointing this out. based on this, i prototyped a similar > approach in MediaControls::defaultEventHandler. it seems to work fine in the > new UI. i'm not going to include it in this CL, but might put it out as a > follow-up for both old and new. Sounds good. Do you have that WIP CL somewhere? I agree that this CL is big enough as it is, but the interesting bit is what the size of the fullscreen button element will be. When I've looked at this, and in fact implemented it hackily for Opera for Android, I was treating clicks directly on the MediaControls element (not any child) as the trigger for show/hide. https://codereview.chromium.org/1156993013/diff/990001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:482: bool isEnabled = hasSource(mediaElement); On 2015/07/27 20:26:09, liberato wrote: > On 2015/07/21 12:02:21, philipj wrote: > > Is a change in the fullscreen button visibility and enabled state intended? > The > > combination of hasSource(mediaElement) here with mediaElement().hasVideo() in > > MediaControls::reset() seems strange, because hasVideo() implies hasSource(), > so > > isEnabled ought to always be true here? > > yes, it should be true always. didn't assume that here just because it seemed > weird to make this one different. Right, many other elements have this same check, so doing it here seems OK even if redundant. https://codereview.chromium.org/1156993013/diff/990001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:501: bool isEnabled = hasSource(mediaElement); On 2015/07/27 20:26:09, liberato wrote: > On 2015/07/21 12:02:21, philipj wrote: > > This is also new. The entire panel is grayed out if !hasSource() for the new > UI > > and the enabled flag is ignored for the old, so will this change be > observable? > > It seems to me that if any button is to be disabled, it should be based on the > > relevant state (presence of text tracks in this case) and not the top-level > > hasSource() condition. > > there's no case where !hasSource() and we have a CC button. however, i didn't > want to assume that here. so it's not observable, but leaving out the check > seems weird, as above. the goal was to have uniform checks to gray out the UI, > without memorizing how MediaControls works. > > anyway, i modified this and the cast check to be more specific. the fullscreen > button is left as-is, since the check for that is more complicated. Acknowledged.
https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:612: if (panelWidth == 0) On 2015/07/28 15:23:49, philipj wrote: > On 2015/07/27 20:26:08, liberato wrote: > > On 2015/07/21 12:02:21, philipj wrote: > > > Repeat of earlier issue, why not update m_panelWidth first? > > > > this isn't the same. this is the callback that notfies about new width. the > > old issues was with computeWhichControlsFit(), which no longer takes > panelWidth > > as an argument. > > Oh, OK. In this case, would it break anything if m_panelWidth were first set and > then the check was done? It's nice if the state is always in sync, if it isn't > one might start to look for a reason. sure, i'll set it here unconditionally and let changedControlSelections() worry about it, since it checks too. i don't think that the case ever happens anyway. https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:630: int panelWidth = m_panel->clientWidth(); On 2015/07/28 15:23:49, philipj wrote: > On 2015/07/27 20:26:08, liberato wrote: > > On 2015/07/21 12:02:21, philipj wrote: > > > When is this needed, is it not redundant with notifyPanelWidthChanged? > > > > > > It seems like what ought to happen is that any time a control changes its > > > isWanted state, computeWhichControlsFit() should be called, right? This is > > > currently called in reset(), but there ought to be many cases where a > control > > is > > > removed without recomputing. An easy case to test with is the CC button, > where > > > removing all tracks should cause it to go away without a reset() call. > > > > when visibility changes, one should call changedControlSelections(). It grabs > > the clientWidth to update m_panelWidth. especially during initial > construction, > > this is helpful. > > > > letting the panel width show up via notifyPanelWidthChanged() caused an extra > > bad frame, unfortunately. > > > > i'll verify why it no longer calls this more often -- it did before, and i > > removed it. > > > > ... not sure. Added calls more often, and included a simple batching > mechanism > > so that only the outermost call actually calls changedControlSelections(). > > there were a few routines that were called both as top-level entry points and > as > > part of some other top-level entry. factoring into top-level and internal > calls > > wouldn't have helped, since some setIsWanted() calls are conditional. > > Hmm, so it seems that if this helps avoid a bad frame, it's because > clientWidth() is forcing a layout instead of waiting for the callback from > layout. The new documentation of changedControlSelections() is helpful, but can > calling setIsWanted() ever actually change the size of the controls (I think no) > or does this forced layout help us learn the size a frame earlier in some > scenario? > > Put differently, when would an ASSERT(m_panelWidth == m_panel->clientWidth()) at > the top of this function fail? > > An explicit document().updateLayoutIgnorePendingStylesheets() and clearing the > timeout might help the responsibilities more clear, although so might more > documentation. i removed this function. the only case where the clientWidth() check helped avoid a bad frame was in reset(). it just sets m_panelWidth there. everybody that called changedControlSelections() before now calls computeWhichControlsFit(). it's all much clearer. as for the assert, it would crash if called at the wrong time. now that we only get panelWidth from notify*() or reset, though, this isn't an issue. i'd still like to understand it better: STDOUT: #CRASHED - renderer (pid 41184) STDERR: ASSERTION FAILED: !node.needsStyleRecalc() STDERR: ../../third_party/WebKit/Source/core/dom/Document.cpp(1692) : void blink::assertLayoutTreeUpdated(blink::Node &) STDERR: 1 0x10d39b0ef blink::Document::updateLayoutTree(blink::StyleRecalcChange) STDERR: 2 0x10d39c1ac blink::Document::updateLayout() STDERR: 3 0x10d39c5b6 blink::Document::updateLayoutIgnorePendingStylesheets(blink::Document::RunPostLayoutTasks) STDERR: 4 0x10d3c9f45 blink::Element::clientWidth() STDERR: 5 0x10d65a276 blink::MediaControls::refreshClosedCaptionsButtonVisibility() STDERR: 6 0x10d586e69 blink::HTMLMediaElement::addTextTrack(blink::TextTrack*) STDERR: 7 0x10d5871f7 blink::HTMLMediaElement::didAddTrackElement(blink::HTMLTrackElement*) STDERR: 8 0x10d5b9f1d blink::HTMLTrackElement::insertedInto(blink::ContainerNode*) STDERR: 9 0x10d37af80 blink::ContainerNode::notifyNodeInsertedInternal(blink::Node&, WTF::Vector<WTF::RefPtr<blink::Node>, 11ul, WTF::DefaultAllocator>&) STDERR: 10 0x10d378d9a blink::ContainerNode::notifyNodeInserted(blink::Node&, blink::ContainerNode::ChildrenChangeSource) STDERR: 11 0x10d3783a9 blink::ContainerNode::updateTreeAfterInsertion(blink::Node&) STDERR: 12 0x10d377bdb blink::ContainerNode::appendChild(WTF::PassRefPtr<blink::Node>, blink::ExceptionState&) STDERR: 13 0x10d3f974c blink::Node::appendChild(WTF::PassRefPtr<blink::Node>, blink::ExceptionState&) STDERR: 14 0x10dfd2c2a blink::NodeV8Internal::appendChildMethodCallbackForMainWorld(v8::FunctionCallbackInfo<v8::Value> const&) STDERR: 15 0x10c8019a8 v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) STDERR: 16 0x10c8321db v8::internal::MaybeHandle<v8::internal::Object> v8::internal::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::(anonymous namespace)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>&) STDERR: 17 0x10c8358c4 v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... File Source/core/html/shadow/SliderThumbElement.cpp (right): https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/SliderThumbElement.cpp:232: // We got a down / up with no drag in between, and we're a media On 2015/07/28 15:23:49, philipj wrote: > On 2015/07/27 20:26:09, liberato wrote: > > On 2015/07/21 12:02:21, philipj wrote: > > > I don't think this is quite right. When you click anywhere outside the > slider, > > > the position is updated on mousedown and then you can drag with the "thumb > > > element" centered under the mouse/touch point. This also seems to be the > > > behavior of YouTube. > > > > > > I think the desired behavior can be implemented by removing the |targetNode > != > > > element()| bit from RangeInputType::handleMouseDownEvent and thus not have > any > > > changes to this file. > > > > > > If you would like to limit the change to the media controls, please add a > TODO > > > and a bug to do it after a Blink roll or two, so that it can be separated > out > > in > > > a bisect, but to leave the difference indefinitely seems strange. > > > > i agree in principle, but i don't have enough experience to say whether the > > broader change is okay. since you do, and since you seem fairly confident > that > > it is okay, i'll do it now. > > It definitely is possible that the change will result in bug reports, and if > that happens let's hope we remember that this was part of the CL. Perhaps > mention it in the description. If you're more paranoid, you could also do it in > a separate CL before or after this. i've reverted all of this, since we're now shipping android only. desktop is deferred. android doesn't have the issue, since touch events in RangeInputType don't care about the thumb. i can put together a separate CL to change the slider behavior, which will also make me feel better. :) https://codereview.chromium.org/1156993013/diff/990001/Source/core/paint/Medi... File Source/core/paint/MediaControlsPainter.cpp (right): https://codereview.chromium.org/1156993013/diff/990001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:174: // Overlay play button covers the entire player, so center and draw a On 2015/07/28 15:23:49, philipj wrote: > On 2015/07/27 20:26:09, liberato wrote: > > On 2015/07/21 12:02:21, philipj wrote: > > > Unresolved earlier issue: this ought to lead to a disambiguation zoom when > > > clicking close to the edge of the controls, which caused a similar earlier > > > change to be reverted: > > > https://codereview.chromium.org/867063003 > > > > > > Also, this will get in the way of implemented tap to show/hide controls: > > > https://codereview.chromium.org/914043002/ > > > > disambiguation: there's a 10px border between the panel and the overlay in the > > css. i've never seen a disambiguation zoom. the paint does need to be > > adjusted. > > > > show/hide: thanks for pointing this out. based on this, i prototyped a > similar > > approach in MediaControls::defaultEventHandler. it seems to work fine in the > > new UI. i'm not going to include it in this CL, but might put it out as a > > follow-up for both old and new. > > Sounds good. Do you have that WIP CL somewhere? I agree that this CL is big > enough as it is, but the interesting bit is what the size of the fullscreen > button element will be. When I've looked at this, and in fact implemented it > hackily for Opera for Android, I was treating clicks directly on the > MediaControls element (not any child) as the trigger for show/hide. that sounds like exactly what i did... no, i don't have a WIP CL, but i will put one together. i didn't test it super thoroughly, just enough to convince myself that an answer was somewhere in the general area.
https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:612: if (panelWidth == 0) On 2015/07/30 05:54:23, liberato wrote: > On 2015/07/28 15:23:49, philipj wrote: > > On 2015/07/27 20:26:08, liberato wrote: > > > On 2015/07/21 12:02:21, philipj wrote: > > > > Repeat of earlier issue, why not update m_panelWidth first? > > > > > > this isn't the same. this is the callback that notfies about new width. > the > > > old issues was with computeWhichControlsFit(), which no longer takes > > panelWidth > > > as an argument. > > > > Oh, OK. In this case, would it break anything if m_panelWidth were first set > and > > then the check was done? It's nice if the state is always in sync, if it isn't > > one might start to look for a reason. > > sure, i'll set it here unconditionally and let changedControlSelections() worry > about it, since it checks too. i don't think that the case ever happens anyway. Acknowledged. https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:630: int panelWidth = m_panel->clientWidth(); On 2015/07/30 05:54:23, liberato wrote: > On 2015/07/28 15:23:49, philipj wrote: > > On 2015/07/27 20:26:08, liberato wrote: > > > On 2015/07/21 12:02:21, philipj wrote: > > > > When is this needed, is it not redundant with notifyPanelWidthChanged? > > > > > > > > It seems like what ought to happen is that any time a control changes its > > > > isWanted state, computeWhichControlsFit() should be called, right? This is > > > > currently called in reset(), but there ought to be many cases where a > > control > > > is > > > > removed without recomputing. An easy case to test with is the CC button, > > where > > > > removing all tracks should cause it to go away without a reset() call. > > > > > > when visibility changes, one should call changedControlSelections(). It > grabs > > > the clientWidth to update m_panelWidth. especially during initial > > construction, > > > this is helpful. > > > > > > letting the panel width show up via notifyPanelWidthChanged() caused an > extra > > > bad frame, unfortunately. > > > > > > i'll verify why it no longer calls this more often -- it did before, and i > > > removed it. > > > > > > ... not sure. Added calls more often, and included a simple batching > > mechanism > > > so that only the outermost call actually calls changedControlSelections(). > > > there were a few routines that were called both as top-level entry points > and > > as > > > part of some other top-level entry. factoring into top-level and internal > > calls > > > wouldn't have helped, since some setIsWanted() calls are conditional. > > > > Hmm, so it seems that if this helps avoid a bad frame, it's because > > clientWidth() is forcing a layout instead of waiting for the callback from > > layout. The new documentation of changedControlSelections() is helpful, but > can > > calling setIsWanted() ever actually change the size of the controls (I think > no) > > or does this forced layout help us learn the size a frame earlier in some > > scenario? > > > > Put differently, when would an ASSERT(m_panelWidth == m_panel->clientWidth()) > at > > the top of this function fail? > > > > An explicit document().updateLayoutIgnorePendingStylesheets() and clearing the > > timeout might help the responsibilities more clear, although so might more > > documentation. > > i removed this function. the only case where the clientWidth() check helped > avoid a bad frame was in reset(). it just sets m_panelWidth there. everybody > that called changedControlSelections() before now calls > computeWhichControlsFit(). it's all much clearer. > > as for the assert, it would crash if called at the wrong time. now that we only > get panelWidth from notify*() or reset, though, this isn't an issue. i'd still > like to understand it better: > STDOUT: #CRASHED - renderer (pid 41184) > STDERR: ASSERTION FAILED: !node.needsStyleRecalc() > STDERR: ../../third_party/WebKit/Source/core/dom/Document.cpp(1692) : void > blink::assertLayoutTreeUpdated(blink::Node &) > STDERR: 1 0x10d39b0ef > blink::Document::updateLayoutTree(blink::StyleRecalcChange) > STDERR: 2 0x10d39c1ac blink::Document::updateLayout() > STDERR: 3 0x10d39c5b6 > blink::Document::updateLayoutIgnorePendingStylesheets(blink::Document::RunPostLayoutTasks) > STDERR: 4 0x10d3c9f45 blink::Element::clientWidth() > STDERR: 5 0x10d65a276 > blink::MediaControls::refreshClosedCaptionsButtonVisibility() > STDERR: 6 0x10d586e69 blink::HTMLMediaElement::addTextTrack(blink::TextTrack*) > STDERR: 7 0x10d5871f7 > blink::HTMLMediaElement::didAddTrackElement(blink::HTMLTrackElement*) > STDERR: 8 0x10d5b9f1d > blink::HTMLTrackElement::insertedInto(blink::ContainerNode*) > STDERR: 9 0x10d37af80 > blink::ContainerNode::notifyNodeInsertedInternal(blink::Node&, > WTF::Vector<WTF::RefPtr<blink::Node>, 11ul, WTF::DefaultAllocator>&) > STDERR: 10 0x10d378d9a blink::ContainerNode::notifyNodeInserted(blink::Node&, > blink::ContainerNode::ChildrenChangeSource) > STDERR: 11 0x10d3783a9 > blink::ContainerNode::updateTreeAfterInsertion(blink::Node&) > STDERR: 12 0x10d377bdb > blink::ContainerNode::appendChild(WTF::PassRefPtr<blink::Node>, > blink::ExceptionState&) > STDERR: 13 0x10d3f974c blink::Node::appendChild(WTF::PassRefPtr<blink::Node>, > blink::ExceptionState&) > STDERR: 14 0x10dfd2c2a > blink::NodeV8Internal::appendChildMethodCallbackForMainWorld(v8::FunctionCallbackInfo<v8::Value> > const&) > STDERR: 15 0x10c8019a8 v8::internal::FunctionCallbackArguments::Call(void > (*)(v8::FunctionCallbackInfo<v8::Value> const&)) > STDERR: 16 0x10c8321db v8::internal::MaybeHandle<v8::internal::Object> > v8::internal::HandleApiCallHelper<false>(v8::internal::Isolate*, > v8::internal::(anonymous > namespace)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>&) > STDERR: 17 0x10c8358c4 v8::internal::Builtin_HandleApiCall(int, > v8::internal::Object**, v8::internal::Isolate*) What was the source of the appendChild() call here? The assert doesn't look familiar, but I guess it isn't allowed to attempt to update the layout during a tree modification, presumably that will be done after the whole modification is done. https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/MediaControls.cpp:701: m_keepMuteButton = false; On 2015/07/27 20:26:08, liberato wrote: > On 2015/07/21 12:02:21, philipj wrote: > > Do you also want it to be reset when loading a new media resource? > MediaControls > > so far does not have any persistent state, so that calling reset() will always > > (modulo bugs) have the same effect. > > good point, will reset in reset(). Acknowledged.
OK, quickly approaching (or have we passed?) the point where landing this and iterating is easier than trying to keep track of changes to a big CL. Other than the things I commented on, is there anything else outstanding that I've forgotten about? I guess the slider behavior on mousedown/touchstart will be left untouched and you'll do it in a separate CL? https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... File Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/MediaControlElements.cpp:76: || type == EventTypeNames::mouseout On 2015/07/28 15:23:48, philipj wrote: > On 2015/07/27 20:26:08, liberato wrote: > > this is the cause of the "takes a while to hide the panel after mouseout" bug > > that you mentioned. it only happens if one triggers a mouseout over one of > the > > two sliders. it wasn't as visible in the old UI since there was a small gap > > between the bottom of the panel and the bottom of the media element, which > > usually triggered another mouseout. > > > > i modified this check to include mouse{over,out,move} only if the scroller is > in > > drag mode. > > > > however, i didn't see any difference in behavior if one simply never checks > for > > them. when the slider captures the event, the mouseouts get eaten somewhere > > anyway. > > That makes sense. I made a demo for myself to see how this works: > http://software.hixie.ch/utilities/js/live-dom-viewer/saved/3574 > > (Add the gap class to the inner div to see the old behavior.) > > From this, I gather that the mouseover and mouseout logic in > MediaControls::defaultEventHandler could actually just use mouseenter and > mouseleave and be simplified somewhat as a consequence. (In this CL only if it's > a cleaner fix.) > > Like you, I can't actually see that these checks are needed, while one is > dragging a slider it doesn't seem like any of the events are fired anyway. The > origin of this is https://codereview.chromium.org/406213002 but it's possible > that more than necessary was done to fix the "the click event isn't being > blocked on the time and volume sliders" problem. > > Could try what happens if one simply replaces isUserInteractionEventForSlider() > with isUserInteractionEvent()? It's a rare thing when things can be fixed by > just removing code, but perhaps this is such an occasion. Were you able to figure out if the change is needed? https://codereview.chromium.org/1156993013/diff/990001/Source/core/paint/Medi... File Source/core/paint/MediaControlsPainter.cpp (right): https://codereview.chromium.org/1156993013/diff/990001/Source/core/paint/Medi... Source/core/paint/MediaControlsPainter.cpp:174: // Overlay play button covers the entire player, so center and draw a On 2015/07/30 05:54:23, liberato wrote: > On 2015/07/28 15:23:49, philipj wrote: > > On 2015/07/27 20:26:09, liberato wrote: > > > On 2015/07/21 12:02:21, philipj wrote: > > > > Unresolved earlier issue: this ought to lead to a disambiguation zoom when > > > > clicking close to the edge of the controls, which caused a similar earlier > > > > change to be reverted: > > > > https://codereview.chromium.org/867063003 > > > > > > > > Also, this will get in the way of implemented tap to show/hide controls: > > > > https://codereview.chromium.org/914043002/ > > > > > > disambiguation: there's a 10px border between the panel and the overlay in > the > > > css. i've never seen a disambiguation zoom. the paint does need to be > > > adjusted. > > > > > > show/hide: thanks for pointing this out. based on this, i prototyped a > > similar > > > approach in MediaControls::defaultEventHandler. it seems to work fine in > the > > > new UI. i'm not going to include it in this CL, but might put it out as a > > > follow-up for both old and new. > > > > Sounds good. Do you have that WIP CL somewhere? I agree that this CL is big > > enough as it is, but the interesting bit is what the size of the fullscreen > > button element will be. When I've looked at this, and in fact implemented it > > hackily for Opera for Android, I was treating clicks directly on the > > MediaControls element (not any child) as the trigger for show/hide. > > that sounds like exactly what i did... > > no, i don't have a WIP CL, but i will put one together. i didn't test it super > thoroughly, just enough to convince myself that an answer was somewhere in the > general area. Could you reduce the size of the overlay play button in this CL, so that we don't accidentally end up with "click anywhere to play/pause" and being accused of regressions when it instead shows/hides? https://codereview.chromium.org/1156993013/diff/1190001/Source/core/html/shad... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/1190001/Source/core/html/shad... Source/core/html/shadow/MediaControls.cpp:70: class MediaControls::BatchedControlUpdate { WTF_MAKE_NONCOPYABLE seems to be the norm for stack-only classes like this. https://codereview.chromium.org/1156993013/diff/1190001/Source/core/html/shad... Source/core/html/shadow/MediaControls.cpp:725: m_batchDepth++; I guess also ASSERT >= 0 before incrementing and after decrementing. https://codereview.chromium.org/1156993013/diff/1190001/Source/core/html/shad... File Source/core/html/shadow/MediaControls.h (right): https://codereview.chromium.org/1156993013/diff/1190001/Source/core/html/shad... Source/core/html/shadow/MediaControls.h:164: int m_batchDepth; Rather than keeping the state on the MediaControls object, could BatchedControlUpdate have a static member to count the number of instances of the stack, and when that reaches zero, call computeWhichControlsFit()? (Globals are ugly, but so are flags for "I'm now inside this method" and the like.) https://codereview.chromium.org/1156993013/diff/1190001/Source/core/layout/La... File Source/core/layout/LayoutTheme.cpp (right): https://codereview.chromium.org/1156993013/diff/1190001/Source/core/layout/La... Source/core/layout/LayoutTheme.cpp:264: // >60 minutes duration is 000:00 Previously you said "the 61:00 is intended" but this would still result in "061:00".
Just one issue overlooked it seems. https://codereview.chromium.org/1156993013/diff/1190001/Source/core/html/shad... File Source/core/html/shadow/MediaControls.h (right): https://codereview.chromium.org/1156993013/diff/1190001/Source/core/html/shad... Source/core/html/shadow/MediaControls.h:164: int m_batchDepth; On 2015/08/03 13:52:34, philipj wrote: > Rather than keeping the state on the MediaControls object, could > BatchedControlUpdate have a static member to count the number of instances of > the stack, and when that reaches zero, call computeWhichControlsFit()? > > (Globals are ugly, but so are flags for "I'm now inside this method" and the > like.) Ping. https://codereview.chromium.org/1156993013/diff/1210001/Source/core/html/shad... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/1210001/Source/core/html/shad... Source/core/html/shadow/MediaControls.cpp:83: WTF_MAKE_NONCOPYABLE(BatchedControlUpdate); This is usually before the public: bit, for some reason.
Patchset #44 (id:1230001) has been deleted
i forgot to publish comments yesterday. that explains it! anyway, only other change besides feedback here is making pseudo-layout use 48 for all controls, including sliders. https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... File Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/1156993013/diff/990001/Source/core/html/shado... Source/core/html/shadow/MediaControlElements.cpp:76: || type == EventTypeNames::mouseout On 2015/08/03 13:52:34, philipj wrote: > On 2015/07/28 15:23:48, philipj wrote: > > On 2015/07/27 20:26:08, liberato wrote: > > > this is the cause of the "takes a while to hide the panel after mouseout" > bug > > > that you mentioned. it only happens if one triggers a mouseout over one of > > the > > > two sliders. it wasn't as visible in the old UI since there was a small gap > > > between the bottom of the panel and the bottom of the media element, which > > > usually triggered another mouseout. > > > > > > i modified this check to include mouse{over,out,move} only if the scroller > is > > in > > > drag mode. > > > > > > however, i didn't see any difference in behavior if one simply never checks > > for > > > them. when the slider captures the event, the mouseouts get eaten somewhere > > > anyway. > > > > That makes sense. I made a demo for myself to see how this works: > > http://software.hixie.ch/utilities/js/live-dom-viewer/saved/3574 > > > > (Add the gap class to the inner div to see the old behavior.) > > > > From this, I gather that the mouseover and mouseout logic in > > MediaControls::defaultEventHandler could actually just use mouseenter and > > mouseleave and be simplified somewhat as a consequence. (In this CL only if > it's > > a cleaner fix.) > > > > Like you, I can't actually see that these checks are needed, while one is > > dragging a slider it doesn't seem like any of the events are fired anyway. The > > origin of this is https://codereview.chromium.org/406213002 but it's possible > > that more than necessary was done to fix the "the click event isn't being > > blocked on the time and volume sliders" problem. > > > > Could try what happens if one simply replaces > isUserInteractionEventForSlider() > > with isUserInteractionEvent()? It's a rare thing when things can be fixed by > > just removing code, but perhaps this is such an occasion. > > Were you able to figure out if the change is needed? i'm unable to convince myself. rather than breaking something. i opened crbug.com/516416 and assigned it to me. https://codereview.chromium.org/1156993013/diff/1190001/Source/core/html/shad... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/1190001/Source/core/html/shad... Source/core/html/shadow/MediaControls.cpp:70: class MediaControls::BatchedControlUpdate { On 2015/08/03 13:52:34, philipj wrote: > WTF_MAKE_NONCOPYABLE seems to be the norm for stack-only classes like this. good point, done. https://codereview.chromium.org/1156993013/diff/1190001/Source/core/html/shad... Source/core/html/shadow/MediaControls.cpp:725: m_batchDepth++; On 2015/08/03 13:52:34, philipj wrote: > I guess also ASSERT >= 0 before incrementing and after decrementing. Done. https://codereview.chromium.org/1156993013/diff/1190001/Source/core/html/shad... File Source/core/html/shadow/MediaControls.h (right): https://codereview.chromium.org/1156993013/diff/1190001/Source/core/html/shad... Source/core/html/shadow/MediaControls.h:164: int m_batchDepth; On 2015/08/03 13:52:34, philipj wrote: > Rather than keeping the state on the MediaControls object, could > BatchedControlUpdate have a static member to count the number of instances of > the stack, and when that reaches zero, call computeWhichControlsFit()? > > (Globals are ugly, but so are flags for "I'm now inside this method" and the > like.) if two tabs have media controls, might they not get confused? https://codereview.chromium.org/1156993013/diff/1190001/Source/core/layout/La... File Source/core/layout/LayoutTheme.cpp (right): https://codereview.chromium.org/1156993013/diff/1190001/Source/core/layout/La... Source/core/layout/LayoutTheme.cpp:264: // >60 minutes duration is 000:00 On 2015/08/03 13:52:35, philipj wrote: > Previously you said "the 61:00 is intended" but this would still result in > "061:00". Done. https://codereview.chromium.org/1156993013/diff/1210001/Source/core/html/shad... File Source/core/html/shadow/MediaControlElementTypes.h (right): https://codereview.chromium.org/1156993013/diff/1210001/Source/core/html/shad... Source/core/html/shadow/MediaControlElementTypes.h:91: int minimumWidth(); in cases where the bar had between 48 and 54 pixel remaining, having the numeric time display in place of the slider was just weird. switched it so they stay strictly in order.
https://codereview.chromium.org/1156993013/diff/1190001/Source/core/html/shad... File Source/core/html/shadow/MediaControls.h (right): https://codereview.chromium.org/1156993013/diff/1190001/Source/core/html/shad... Source/core/html/shadow/MediaControls.h:164: int m_batchDepth; On 2015/08/04 15:07:00, liberato wrote: > On 2015/08/03 13:52:34, philipj wrote: > > Rather than keeping the state on the MediaControls object, could > > BatchedControlUpdate have a static member to count the number of instances of > > the stack, and when that reaches zero, call computeWhichControlsFit()? > > > > (Globals are ugly, but so are flags for "I'm now inside this method" and the > > like.) > > if two tabs have media controls, might they not get confused? A grep for 'static unsigned s_' will find the pattern I had in mind, StyleAttributeMutationScope is probably the most similar case. Since there is only one main thread per process and BatchedControlUpdate will only exist on the stack of the main thread, there shouldn't be a problem. Unless, of course, you could have two different MediaControls objects with these scope objects on the stack at the same time?
i believe that this is up to date with all CL feedback. thanks -fl https://codereview.chromium.org/1156993013/diff/1190001/Source/core/html/shad... File Source/core/html/shadow/MediaControls.h (right): https://codereview.chromium.org/1156993013/diff/1190001/Source/core/html/shad... Source/core/html/shadow/MediaControls.h:164: int m_batchDepth; On 2015/08/04 15:15:59, philipj wrote: > On 2015/08/04 15:07:00, liberato wrote: > > On 2015/08/03 13:52:34, philipj wrote: > > > Rather than keeping the state on the MediaControls object, could > > > BatchedControlUpdate have a static member to count the number of instances > of > > > the stack, and when that reaches zero, call computeWhichControlsFit()? > > > > > > (Globals are ugly, but so are flags for "I'm now inside this method" and the > > > like.) > > > > if two tabs have media controls, might they not get confused? > > A grep for 'static unsigned s_' will find the pattern I had in mind, > StyleAttributeMutationScope is probably the most similar case. Since there is > only one main thread per process and BatchedControlUpdate will only exist on the > stack of the main thread, there shouldn't be a problem. > > Unless, of course, you could have two different MediaControls objects with these > scope objects on the stack at the same time? Done. https://codereview.chromium.org/1156993013/diff/1210001/Source/core/html/shad... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/1210001/Source/core/html/shad... Source/core/html/shadow/MediaControls.cpp:83: WTF_MAKE_NONCOPYABLE(BatchedControlUpdate); On 2015/08/04 11:54:53, philipj wrote: > This is usually before the public: bit, for some reason. Done.
LGTM with minor final nits. THANK YOU for having patience, I haven't been as responsive as I strive to be, but now let's move on to incremental improvements :) https://codereview.chromium.org/1156993013/diff/1270001/LayoutTests/TestExpec... File LayoutTests/TestExpectations (right): https://codereview.chromium.org/1156993013/diff/1270001/LayoutTests/TestExpec... LayoutTests/TestExpectations:1504: crbug.com/446350 compositing/geometry/video-fixed-scrolling.html [ NeedsRebaseline ] So I ran this test and the failure is: --- /home/philip/src/chromium/src/webkit/Debug/layout-test-results/compositing/geometry/video-fixed-scrolling-expected.txt +++ /home/philip/src/chromium/src/webkit/Debug/layout-test-results/compositing/geometry/video-fixed-scrolling-actual.txt @@ -8,13 +8,15 @@ LayoutBlockFlow {P} at (0,401) size 769x20 LayoutText {#text} at (0,0) size 425x19 text run at (0,0) width 425: "The blue bar should be in front of the video, and at the top of the page." + LayoutBlockFlow (anonymous) at (0,437) size 769x0 + LayoutText {#text} at (0,0) size 0x0 layer at (8,88) size 400x300 LayoutVideo {VIDEO} at (0,80) size 400x300 layer at (8,88) size 400x300 LayoutFlexibleBox (relative positioned) {DIV} at (0,0) size 400x300 - LayoutBlockFlow {DIV} at (0,265) size 400x35 -layer at (8,88) size 400x265 - LayoutFlexibleBox (relative positioned) {DIV} at (0,0) size 400x265 + LayoutBlockFlow {DIV} at (0,252) size 400x48 +layer at (8,88) size 400x242 + LayoutFlexibleBox (relative positioned) {DIV} at (0,0) size 400x242 layer at (0,50) size 500x60 LayoutBlockFlow (positioned) {DIV} at (0,0) size 500x60 [bgcolor=#0000FFCC] scrolled to 0,50 What is this change, and why is it happening before the flag is flipped? https://codereview.chromium.org/1156993013/diff/1270001/LayoutTests/media/tra... File LayoutTests/media/track/track-cue-rendering-after-controls-added.html (right): https://codereview.chromium.org/1156993013/diff/1270001/LayoutTests/media/tra... LayoutTests/media/track/track-cue-rendering-after-controls-added.html:29: setTimeout(function() This is likely to be flaky. https://docs.google.com/document/d/1Yl4SnTLBWmY1O99_BTtQvuoffP8YM9HZx2YPkEsad... suggests alternatives, and that whole document is just great reading if you haven't seen it before. (Found it via https://www.chromium.org/developers/testing/webkit-layout-tests#TOC-Writing-r... after much searching.) https://codereview.chromium.org/1156993013/diff/1270001/LayoutTests/media/vid... File LayoutTests/media/video-controls-mouse-events-captured.html (right): https://codereview.chromium.org/1156993013/diff/1270001/LayoutTests/media/vid... LayoutTests/media/video-controls-mouse-events-captured.html:50: eventSender.mouseDown(); Do you think it's worthwhile to test both with and without dragging? If it can be done within this test it seems worth it to me, otherwise maybe not. https://codereview.chromium.org/1156993013/diff/1270001/Source/core/html/shad... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/1270001/Source/core/html/shad... Source/core/html/shadow/MediaControls.cpp:47: static int s_batchDepth = 0; This can be a static member of BatchedControlUpdate, as in e.g. StyleAttributeMutationScope
do you need a rubberstamp from me, or does philip already cover everything?
An extra look at platform/ and web/ wouldn't hurt.
lgtm with comment https://codereview.chromium.org/1156993013/diff/1270001/public/web/WebRuntime... File public/web/WebRuntimeFeatures.h (right): https://codereview.chromium.org/1156993013/diff/1270001/public/web/WebRuntime... public/web/WebRuntimeFeatures.h:161: BLINK_EXPORT static void enableNewMediaPlaybackUi(bool); this shouldn't be needed now, right?
yay! philipj: thanks for all your help! i appreciate the large amount of the work you put into this review! jochen: please let me know if you'd prefer a different way of enabling the new UI only for android., rather than WebRuntimeFeatures. that CL should be in your inbox momentarily. thanks -fl https://codereview.chromium.org/1156993013/diff/1270001/LayoutTests/TestExpec... File LayoutTests/TestExpectations (right): https://codereview.chromium.org/1156993013/diff/1270001/LayoutTests/TestExpec... LayoutTests/TestExpectations:1504: crbug.com/446350 compositing/geometry/video-fixed-scrolling.html [ NeedsRebaseline ] On 2015/08/05 08:41:05, philipj wrote: > So I ran this test and the failure is: > > --- > /home/philip/src/chromium/src/webkit/Debug/layout-test-results/compositing/geometry/video-fixed-scrolling-expected.txt > +++ > /home/philip/src/chromium/src/webkit/Debug/layout-test-results/compositing/geometry/video-fixed-scrolling-actual.txt > @@ -8,13 +8,15 @@ > LayoutBlockFlow {P} at (0,401) size 769x20 > LayoutText {#text} at (0,0) size 425x19 > text run at (0,0) width 425: "The blue bar should be in front of the > video, and at the top of the page." > + LayoutBlockFlow (anonymous) at (0,437) size 769x0 > + LayoutText {#text} at (0,0) size 0x0 > layer at (8,88) size 400x300 > LayoutVideo {VIDEO} at (0,80) size 400x300 > layer at (8,88) size 400x300 > LayoutFlexibleBox (relative positioned) {DIV} at (0,0) size 400x300 > - LayoutBlockFlow {DIV} at (0,265) size 400x35 > -layer at (8,88) size 400x265 > - LayoutFlexibleBox (relative positioned) {DIV} at (0,0) size 400x265 > + LayoutBlockFlow {DIV} at (0,252) size 400x48 > +layer at (8,88) size 400x242 > + LayoutFlexibleBox (relative positioned) {DIV} at (0,0) size 400x242 > layer at (0,50) size 500x60 > LayoutBlockFlow (positioned) {DIV} at (0,0) size 500x60 [bgcolor=#0000FFCC] > scrolled to 0,50 > > What is this change, and why is it happening before the flag is flipped? hrm, this is not what i get. the LayoutBlockFlow with height 48 looks like the controls were still enabled when this test is run. when i run them off, i see only the first two added lines in the diff (LayoutBlockFlow 769x0 and LayoutText 0x0). The x0 lines show up with the call to clientWidth() at the end of MediaControls::reset() to set m_panelWidth. I'll make it conditional on the flag, then the test passes. i have absolutely no idea why asking for the width causes the render tree to change. i added a comment in reset() that describes it. https://codereview.chromium.org/1156993013/diff/1270001/LayoutTests/media/tra... File LayoutTests/media/track/track-cue-rendering-after-controls-added.html (right): https://codereview.chromium.org/1156993013/diff/1270001/LayoutTests/media/tra... LayoutTests/media/track/track-cue-rendering-after-controls-added.html:29: setTimeout(function() On 2015/08/05 08:41:05, philipj wrote: > This is likely to be flaky. > https://docs.google.com/document/d/1Yl4SnTLBWmY1O99_BTtQvuoffP8YM9HZx2YPkEsad... > suggests alternatives, and that whole document is just great reading if you > haven't seen it before. (Found it via > https://www.chromium.org/developers/testing/webkit-layout-tests#TOC-Writing-r... > after much searching.) thanks! that's a great reference. https://codereview.chromium.org/1156993013/diff/1270001/LayoutTests/media/vid... File LayoutTests/media/video-controls-mouse-events-captured.html (right): https://codereview.chromium.org/1156993013/diff/1270001/LayoutTests/media/vid... LayoutTests/media/video-controls-mouse-events-captured.html:50: eventSender.mouseDown(); On 2015/08/05 08:41:05, philipj wrote: > Do you think it's worthwhile to test both with and without dragging? If it can > be done within this test it seems worth it to me, otherwise maybe not. good idea. done. https://codereview.chromium.org/1156993013/diff/1270001/Source/core/html/shad... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1156993013/diff/1270001/Source/core/html/shad... Source/core/html/shadow/MediaControls.cpp:47: static int s_batchDepth = 0; On 2015/08/05 08:41:05, philipj wrote: > This can be a static member of BatchedControlUpdate, as in e.g. > StyleAttributeMutationScope that was my preference as well. i patterned after https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... which used static global. changed. https://codereview.chromium.org/1156993013/diff/1270001/public/web/WebRuntime... File public/web/WebRuntimeFeatures.h (right): https://codereview.chromium.org/1156993013/diff/1270001/public/web/WebRuntime... public/web/WebRuntimeFeatures.h:161: BLINK_EXPORT static void enableNewMediaPlaybackUi(bool); On 2015/08/05 10:58:44, jochen wrote: > this shouldn't be needed now, right? it is needed temporarily, unfortunately. the desktop UI spec came under revision, so i will enable the new UI only for android. the WRF is consumed by https://codereview.chromium.org/1266813002, since we already do a bunch of android-specific setup there for chrome. i've added you as a reviewer to that CL, in case you have suggestions about how to better do what i'm trying to do. I'll hold off on landing this one in case there are any changes there.
using the web runtime feature for enabling the UI on android only is ok
The CQ bit was checked by liberato@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, philipj@opera.com Link to the patchset: https://codereview.chromium.org/1156993013/#ps1330001 (title: "Rebased to land.... Of course i'm serious, and don't call me Shirley.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1156993013/1330001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1156993013/1330001
Message was sent while issue was closed.
Committed patchset #48 (id:1330001) as https://src.chromium.org/viewvc/blink?view=rev&revision=200116 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
