Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2701433003:
Hide overlay play button if it can't be shown without clipping (Closed)
|
Created:
3 years, 10 months ago by steimel Modified:
3 years, 9 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, blink-reviews-layout_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, jchaffraix+rendering, leviw+renderwatch, mlamouri+watch-blink_chromium.org, pdr+renderingwatchlist_chromium.org, nessy, Srirama, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHide overlay play button if it can't be shown without clipping.
This CL also refactors MediaControls to listen for size changes via ResizeObserver instead of via notifications from LayoutMedia. Additionally, this CL changes the position of the overlay play button to vertically center on the height of the video minus the height of the media controls panel.
BUG=652951
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2701433003
Cr-Commit-Position: refs/heads/master@{#456724}
Committed: https://chromium.googlesource.com/chromium/src/+/5b069f0bfe05879dc2dfbc32d9b4b171c622d5c3
Patch Set 1 #Patch Set 2 : Refactor to use ResizeObserver #
Total comments: 12
Patch Set 3 : Initialize MediaControls size on first layout. Other code review feedback #Patch Set 4 : rebase #
Total comments: 14
Patch Set 5 : avayvod feedback #
Total comments: 2
Patch Set 6 : Add delay to layout test to allow ResizeObserver to run #
Total comments: 10
Patch Set 7 : mlamouri feedback #
Total comments: 11
Patch Set 8 : Use IntSize instead of ints and add layout test #Patch Set 9 : rebase #Patch Set 10 : Turn on overlay play button in layout test #
Total comments: 1
Patch Set 11 : Create/Destroy ResizeObserver onInsert/onRemove. Add (currently broken) layout test for moving vide… #
Total comments: 4
Patch Set 12 : Vertically center overlay play button disregarding panel. Handle differences in panel height betwee… #
Total comments: 4
Patch Set 13 : Use new runtime flag to enable overlay play button #Patch Set 14 : Utilize MediaControlsPainter to recenter play button instead of hacky css #Patch Set 15 : Fix compile warning for uninitialized variable #
Total comments: 14
Patch Set 16 : mlamouri feedback #Patch Set 17 : Rebase and fix #
Total comments: 2
Patch Set 18 : Add comments for dependent constants #Messages
Total messages: 108 (73 generated)
steimel@chromium.org changed reviewers: + avayvod@chromium.org, mlamouri@chromium.org
PTAL. Not sure if this is the right way to go about this, but I've changed the width-change-notification flow to also send the changed height, and then used experimentally-determined constants to decide whether or not to display the overlay play button
The panel is either visible or hidden and has a fixed height depending on the platform. It seems that could probably find a solution that does not require touching LayoutMedia. Is there something I'm missing?
Just to leave a comment on what we discussed offline: there was a confusion with regards to what the "panel" is here. It would be fine to change the CL and add a video height information in addition to the panel width. A probably more complex but more interesting approach would be to use the ResizeObserver API to know when the video element's size changes. Either way would be okay but the ResizeObserver route would be more interesting IMO ;)
PTAL
The CQ bit was checked by steimel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2701433003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2701433003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:131: ~MediaControlsResizeObserverCallback() override{}; nit: s/{};/ = default;/ https://codereview.chromium.org/2701433003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:135: for (const auto& entry : entries) { Is more than one entry expected? Should we DCHECK() for entries.size == 1? https://codereview.chromium.org/2701433003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:192: m_resizeObserver->observe(m_mediaElement); Would it make sense to initialise the current size in the case the resize observer is set up after the first layout, thus would not notify us of the current size? https://codereview.chromium.org/2701433003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:839: m_panelHeight = newHeight; Is it really still the panel width/height? Shouldn't that be the media element instead? https://codereview.chromium.org/2701433003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right): https://codereview.chromium.org/2701433003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.h:87: void notifyPanelSizeChanged(int newWidth, int newHeight); I guess the name needs to be updated? https://codereview.chromium.org/2701433003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.h:124: class MediaControlsResizeObserverCallback; Do you need to have the class declared here?
The CQ bit was checked by steimel@chromium.org to run a CQ dry run
PTAL :) https://codereview.chromium.org/2701433003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2701433003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:131: ~MediaControlsResizeObserverCallback() override{}; On 2017/02/22 11:48:08, mlamouri wrote: > nit: s/{};/ = default;/ Done. https://codereview.chromium.org/2701433003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:135: for (const auto& entry : entries) { On 2017/02/22 11:48:08, mlamouri wrote: > Is more than one entry expected? Should we DCHECK() for entries.size == 1? Discussed offline. After looking at the spec, there will only be one entry per observed element, and since we only observe one element, yes we should DCHECK https://codereview.chromium.org/2701433003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:192: m_resizeObserver->observe(m_mediaElement); On 2017/02/22 11:48:08, mlamouri wrote: > Would it make sense to initialise the current size in the case the resize > observer is set up after the first layout, thus would not notify us of the > current size? Discussed offline, but re-adding some code in LayoutMedia to inform the mediacontrols of layouts to handle initialization https://codereview.chromium.org/2701433003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:839: m_panelHeight = newHeight; On 2017/02/22 11:48:08, mlamouri wrote: > Is it really still the panel width/height? Shouldn't that be the media element > instead? Done. https://codereview.chromium.org/2701433003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right): https://codereview.chromium.org/2701433003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.h:87: void notifyPanelSizeChanged(int newWidth, int newHeight); On 2017/02/22 11:48:08, mlamouri wrote: > I guess the name needs to be updated? Done. https://codereview.chromium.org/2701433003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.h:124: class MediaControlsResizeObserverCallback; On 2017/02/22 11:48:08, mlamouri wrote: > Do you need to have the class declared here? Discussed offline and leaving here since I'm changing notifyElementSizeChanged to be private and so MediaControlsResizeObserverCallback will need access
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by steimel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2701433003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2701433003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:132: class MediaControls::MediaControlsResizeObserverCallback nit: could you add a comment to the forward declarations in the header that they're needed for private member access? https://codereview.chromium.org/2701433003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:143: DCHECK_EQ(1u, entries.size()); nit: DCHECK entries[0]->target() is m_mediaElement? https://codereview.chromium.org/2701433003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:193: m_elementSizeChangedTimer(TaskRunnerHelper::get(TaskType::UnspecedTimer, rant: I'm sad unspeced is used as a word (however internal search reveals it's being used sometimes). https://codereview.chromium.org/2701433003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:840: // size, since we're called after layout. nit: I'm not sure this comment is up-to-date, I don't see any check for the width/height to match the known ones. https://codereview.chromium.org/2701433003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:853: m_elementWidth = is it still the element's width or panel's width after the computation? is it possible the new value of m_elementWidth is the same as the one used before the resize event after this computation (so we get the event but don't need to do anything since from the panel size perspective nothing changed)? https://codereview.chromium.org/2701433003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:996: if (!m_sizingInitialized) { nit: no need for {} I wish we could somehow request the size in the ctor before registering the ResizeObserver but I guess it's not feasible. https://codereview.chromium.org/2701433003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right): https://codereview.chromium.org/2701433003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.h:133: class MediaControlsResizeObserverCallback; nits: - move the class declaration next to BatchedControlUpdate above - move the member variables definitions down after the existing ones - for new member variables, please, add comments - it seems like the members are all initialized in the ctor so please initialize the bool there too
PTAL https://codereview.chromium.org/2701433003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2701433003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:132: class MediaControls::MediaControlsResizeObserverCallback On 2017/02/23 21:37:31, whywhat wrote: > nit: could you add a comment to the forward declarations in the header that > they're needed for private member access? Done. https://codereview.chromium.org/2701433003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:143: DCHECK_EQ(1u, entries.size()); On 2017/02/23 21:37:31, whywhat wrote: > nit: DCHECK entries[0]->target() is m_mediaElement? Done. https://codereview.chromium.org/2701433003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:193: m_elementSizeChangedTimer(TaskRunnerHelper::get(TaskType::UnspecedTimer, On 2017/02/23 21:37:32, whywhat wrote: > rant: I'm sad unspeced is used as a word (however internal search reveals it's > being used sometimes). Acknowledged :) https://codereview.chromium.org/2701433003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:840: // size, since we're called after layout. On 2017/02/23 21:37:31, whywhat wrote: > nit: I'm not sure this comment is up-to-date, I don't see any check for the > width/height to match the known ones. You're right. At some point, I guess that logic was moved to LayoutMedia, though in this patchset that has been removed to be handled by the ResizeObserver. However, in the next patchset (in order to address your comment below) the logic will be re-added here at the bottom of this method. I'll move this comment there. https://codereview.chromium.org/2701433003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:853: m_elementWidth = On 2017/02/23 21:37:31, whywhat wrote: > is it still the element's width or panel's width after the computation? > is it possible the new value of m_elementWidth is the same as the one used > before the resize event after this computation (so we get the event but don't > need to do anything since from the panel size perspective nothing changed)? Those are both good points. For the first, I've changed the member to be called |m_effective(Width|Height)|. For the second, I've added the logic for not firing the size-change timer if the effectiveWidth hasn't changed. This also brings up whether or not we need to worry about effectiveZoom for height. I assume the height is affected in the same manner as the width, but since the height is only used for the clipping calculation, which (at least currently) depends on fixed-pixel-calculation that may be affected by the zoom as well, not sure if it's necessary. I'm happy to look into it though https://codereview.chromium.org/2701433003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:996: if (!m_sizingInitialized) { On 2017/02/23 21:37:31, whywhat wrote: > nit: no need for {} > > I wish we could somehow request the size in the ctor before registering the > ResizeObserver but I guess it's not feasible. Right. I had tried to initialize in the ctor (and then in initializeControls), however in order to get the size, I had to force a layout, which was breaking a unit test that depended on a layout *not* occurring on initialization (MediaControlsTest.ResetDoesNotTriggerInitialLayout). If not forcing a layout on initialization is *not* a necessary restriction, then I can revisit that https://codereview.chromium.org/2701433003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right): https://codereview.chromium.org/2701433003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.h:133: class MediaControlsResizeObserverCallback; On 2017/02/23 21:37:32, whywhat wrote: > nits: > - move the class declaration next to BatchedControlUpdate above > - move the member variables definitions down after the existing ones > - for new member variables, please, add comments > - it seems like the members are all initialized in the ctor so please initialize > the bool there too Done.
The CQ bit was checked by steimel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, lgtm. I'm not an owner of this part of the code sadly though *staring at Mounir* https://codereview.chromium.org/2701433003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2701433003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:55: constexpr int kAndroidMediaPanelHeight = 48; nit: what does Android mean here? if this is an Android-specific code, how is this compiled on desktop? if it's not, remove the Android part?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2701433003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2701433003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:55: constexpr int kAndroidMediaPanelHeight = 48; On 2017/02/24 00:57:41, whywhat wrote: > nit: what does Android mean here? if this is an Android-specific code, how is > this compiled on desktop? if it's not, remove the Android part? On desktop, the media control panel height is 32px, but on Android, it's 48px. However, we only show the overlay play button on android, so we don't currently have any need for this calculation on desktop
The CQ bit was checked by steimel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I'm mostly concerned about the ResizeObserver usage in addition of LayoutMedia. I would prefer only one thing being used instead. As I said, in the comment, I would be happy to help with this. https://codereview.chromium.org/2701433003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2701433003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:52: constexpr int kOverlayPlayButtonWidth = 48; style: leave empty line above `constexpr` https://codereview.chromium.org/2701433003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:61: } // namespace style: empty line + "anonymous namespace" https://codereview.chromium.org/2701433003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:132: class MediaControls::MediaControlsResizeObserverCallback nit: add `final` https://codereview.chromium.org/2701433003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:999: notifyElementSizeChanged(width, height); I'm a bit concerned by this because you basically could use the `onLayout` callback for everything it seems and we might actually be spending more resources by using both LayoutMedia and the ResizeObserver. I understand that the ResizeObserver doesn't seem to be doing everything we want here and I think we should file a bug or at least find a solution. However, in the context of this change, as I said before, using LayoutMedia would be fine but I don't think there is much benefit is this hybrid solution :( I'm happy to help chase down the ResizeObserver folks to see with them what we can do here. https://codereview.chromium.org/2701433003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right): https://codereview.chromium.org/2701433003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.h:237: int m_effectiveHeight; Could you use IntSize from platform/geometry/?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL https://codereview.chromium.org/2701433003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2701433003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:52: constexpr int kOverlayPlayButtonWidth = 48; On 2017/02/27 18:22:58, mlamouri wrote: > style: leave empty line above `constexpr` Done. https://codereview.chromium.org/2701433003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:61: } // namespace On 2017/02/27 18:22:58, mlamouri wrote: > style: empty line + "anonymous namespace" Done. https://codereview.chromium.org/2701433003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:132: class MediaControls::MediaControlsResizeObserverCallback On 2017/02/27 18:22:58, mlamouri wrote: > nit: add `final` Done. https://codereview.chromium.org/2701433003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:999: notifyElementSizeChanged(width, height); On 2017/02/27 18:22:58, mlamouri wrote: > I'm a bit concerned by this because you basically could use the `onLayout` > callback for everything it seems and we might actually be spending more > resources by using both LayoutMedia and the ResizeObserver. I understand that > the ResizeObserver doesn't seem to be doing everything we want here and I think > we should file a bug or at least find a solution. > > However, in the context of this change, as I said before, using LayoutMedia > would be fine but I don't think there is much benefit is this hybrid solution :( > I'm happy to help chase down the ResizeObserver folks to see with them what we > can do here. Done. https://codereview.chromium.org/2701433003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right): https://codereview.chromium.org/2701433003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.h:237: int m_effectiveHeight; On 2017/02/27 18:22:58, mlamouri wrote: > Could you use IntSize from platform/geometry/? Given the way they're used in computeWhichControlsFit, I think it's simpler to have as basic ints, what do you think?
The CQ bit was checked by steimel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Code lg2m! Thanks for your taking the hard (but better!) path here. You should add tests. I think you can add layout tests in media/controls/. You should be able to access the overaly by looking into the shadow dom. https://codereview.chromium.org/2701433003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2701433003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:199: m_effectiveHeight(0), m_size is set to (0,0) by default https://codereview.chromium.org/2701433003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:848: m_effectiveHeight = newSize->height(); IntSize oldSize = m_size; m_size = newSize; // or some conversion https://codereview.chromium.org/2701433003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:859: if (oldWidth != m_effectiveWidth || oldHeight != m_effectiveHeight) if (oldSize != newSize) https://codereview.chromium.org/2701433003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:973: if (m_effectiveWidth && m_effectiveHeight && m_overlayPlayButton) { `if (!m_size.isEmpty() && m_overlayPlayButton) {` I think `m_size` here makes it clear that both dimensions are set at the same time. https://codereview.chromium.org/2701433003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right): https://codereview.chromium.org/2701433003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.h:229: int m_effectiveHeight; I still think that `IntSize m_size;` would actually make the code simpler, will leave comments below explaining why.
mlamouri@chromium.org changed reviewers: + atotic@chromium.org
+atotic@ FYI as we are using the ResizeObserver for internal code.
The CQ bit was checked by steimel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Hide overlay play button if it can't be shown without clipping BUG=652951 ========== to ========== Hide overlay play button if it can't be shown without clipping. This CL also refactors MediaControls to listen for size changes via ResizeObserver instead of via notifications from LayoutMedia. BUG=652951 ==========
PTAL :) https://codereview.chromium.org/2701433003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2701433003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:199: m_effectiveHeight(0), On 2017/02/28 16:03:14, mlamouri wrote: > m_size is set to (0,0) by default Acknowledged. https://codereview.chromium.org/2701433003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:848: m_effectiveHeight = newSize->height(); On 2017/02/28 16:03:14, mlamouri wrote: > IntSize oldSize = m_size; > m_size = newSize; // or some conversion Done. https://codereview.chromium.org/2701433003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:859: if (oldWidth != m_effectiveWidth || oldHeight != m_effectiveHeight) On 2017/02/28 16:03:14, mlamouri wrote: > if (oldSize != newSize) Done. https://codereview.chromium.org/2701433003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:973: if (m_effectiveWidth && m_effectiveHeight && m_overlayPlayButton) { On 2017/02/28 16:03:14, mlamouri wrote: > `if (!m_size.isEmpty() && m_overlayPlayButton) {` > > I think `m_size` here makes it clear that both dimensions are set at the same > time. Done. https://codereview.chromium.org/2701433003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right): https://codereview.chromium.org/2701433003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.h:229: int m_effectiveHeight; On 2017/02/28 16:03:14, mlamouri wrote: > I still think that `IntSize m_size;` would actually make the code simpler, will > leave comments below explaining why. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by steimel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by steimel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/28 at 16:05:13, mlamouri wrote: > +atotic@ FYI as we are using the ResizeObserver for internal code. My only concern is that ResizeObserver has not shipped yet, and until it ships, I am not 100% sure it'll stay in. What should I do if it needs to be removed?
https://codereview.chromium.org/2701433003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right): https://codereview.chromium.org/2701433003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.h:225: Member<ResizeObserver> m_resizeObserver; I am not an Oilpan expert, but I think we might have circular strong references here: MediaControls => ResizeObserver ResizeObserver => MediaControlsResizeObserverCallback MediaControlsResizeObserverCallback => MediaControls The link can be broken by making one of these a weak reference, probably MediaControlsResizeObserverCallback
On 2017/03/01 at 20:27:35, atotic wrote: > https://codereview.chromium.org/2701433003/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right): > > https://codereview.chromium.org/2701433003/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/shadow/MediaControls.h:225: Member<ResizeObserver> m_resizeObserver; > I am not an Oilpan expert, but I think we might have circular strong references here: > > MediaControls => ResizeObserver > ResizeObserver => MediaControlsResizeObserverCallback > MediaControlsResizeObserverCallback => MediaControls > > The link can be broken by making one of these a weak reference, probably MediaControlsResizeObserverCallback Are any of these Persistent<>? Oilpan handles cycles when they are all marked as Member<>, see https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So... atotic@, one of the reason why I CC'd you here is also so you are aware we are using this. If the Web API were to be removed, we might want to consider keeping it for internal usage or we should have something equivalent.
mlamouri@chromium.org changed reviewers: + avayvod@chromium.orgm - avayvod@chromium.org
https://codereview.chromium.org/2701433003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2701433003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:193: m_resizeObserver(ResizeObserver::create( I've just added a `onInsertedIntoDocument` and `onRemovedFromDocument` callback to MediaControls. Do you mind removing/adding back the observer using this callbacks. It might be useful to make sure we don't break things when changing document. You might want to add a test for this too because I think your change will regress this. You can see an example at LayoutTests/media/autoplay-document-move.html
Okay, so I wanted to get this patchset out today so you could look it over, but the layout test is not yet working. So I rebased to get your new changes, and then tested moving a video between documents, and as you predicted it was broken. I made the changes you suggested to create/destroy the ResizeObserver onInsert/onRemove, and that fixed the issue. However, the layout test currently fails due to a separate issue. In the layout tests, we currently use internals.settings.setMediaControlsOverlayPlayButtonEnabled(true) to have the overlay play button appear on desktop. The MediaControls checks document().settings() to get this setting, and for some reason the javascript-created document's settings aren't affected by our internals.settings call, and therefore the overlay play button doesn't get created. In MediaControls.cpp, I tried commenting out that check (to just always create the overlay play button), and the layout test passed. So I just need to figure out how to get that javascript-created document to listen to that settings change. Any thoughts/knowledge on that would be appreciated. Thanks!
mlamouri@chromium.org changed reviewers: + avayvod@chromium.org
On 2017/03/03 at 01:38:51, steimel wrote: > Okay, so I wanted to get this patchset out today so you could look it over, but the layout test is not yet working. > > So I rebased to get your new changes, and then tested moving a video between documents, and as you predicted it was broken. I made the changes you suggested to create/destroy the ResizeObserver onInsert/onRemove, and that fixed the issue. Happy I didn't slow you down for no good reason here. I was a bit worried :( > However, the layout test currently fails due to a separate issue. In the layout tests, we currently use internals.settings.setMediaControlsOverlayPlayButtonEnabled(true) to have the overlay play button appear on desktop. The MediaControls checks document().settings() to get this setting, and for some reason the javascript-created document's settings aren't affected by our internals.settings call, and therefore the overlay play button doesn't get created. In MediaControls.cpp, I tried commenting out that check (to just always create the overlay play button), and the layout test passed. So I just need to figure out how to get that javascript-created document to listen to that settings change. Any thoughts/knowledge on that would be appreciated. Thanks! Hmm. This is happening because the document you create from Javascript has no settings. I wrote this CL that should fix your problem: https://codereview.chromium.org/2728133002 https://codereview.chromium.org/2701433003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2701433003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:750: } nit: add at the end https://codereview.chromium.org/2701433003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:760: m_resizeObserver.clear(); ditto
Thanks Mounir! Once your CL lands I'll rebase this one and we'll be good to go. Per conversation on crbug.com/652951, I've re-centered the overlay play button to center on the visible part of the video (i.e. ignoring the media panel). The calculation for minimum height has changed accordingly. Also, I've added an OS-check macro to set the min height correctly based on platform https://codereview.chromium.org/2701433003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2701433003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:750: } On 2017/03/03 14:33:56, mlamouri wrote: > nit: add at the end Done. https://codereview.chromium.org/2701433003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:760: m_resizeObserver.clear(); On 2017/03/03 14:33:56, mlamouri wrote: > ditto Done.
ikilpatrick@chromium.org changed reviewers: + ikilpatrick@chromium.org
Drive-by review, I just have some basic questions :) https://codereview.chromium.org/2701433003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2701433003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:893: m_elementSizeChangedTimer.startOneShot(0, BLINK_FROM_HERE); does this need to be a timer now? resizeobserver will keep laying out the page until clean, this could call the callback directly? https://codereview.chromium.org/2701433003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:1009: m_overlayPlayButton->setDoesFit(doesFit); are these controls always provided by the engine? (i.e. they can't be provided by the web developer?)
The CQ bit was checked by steimel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2701433003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2701433003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:893: m_elementSizeChangedTimer.startOneShot(0, BLINK_FROM_HERE); On 2017/03/03 20:52:29, ikilpatrick wrote: > does this need to be a timer now? resizeobserver will keep laying out the page > until clean, this could call the callback directly? I did a quick test, and if this doesn't use the timer then it will hold up the rest of the page layout, so I think we still need the timer https://codereview.chromium.org/2701433003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:1009: m_overlayPlayButton->setDoesFit(doesFit); On 2017/03/03 20:52:29, ikilpatrick wrote: > are these controls always provided by the engine? (i.e. they can't be provided > by the web developer?) I'm not sure that I correctly understand your question, but AFAIK if a web developer wants to use their own controls and not ours, they can not set the "controls" attribute on the video and we won't show any media controls
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== Hide overlay play button if it can't be shown without clipping. This CL also refactors MediaControls to listen for size changes via ResizeObserver instead of via notifications from LayoutMedia. BUG=652951 ========== to ========== Hide overlay play button if it can't be shown without clipping. This CL also refactors MediaControls to listen for size changes via ResizeObserver instead of via notifications from LayoutMedia. BUG=652951 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
PTAL changes to how I'm centering the overlay play button. Thanks :)
The CQ bit was checked by steimel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by steimel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Hide overlay play button if it can't be shown without clipping. This CL also refactors MediaControls to listen for size changes via ResizeObserver instead of via notifications from LayoutMedia. BUG=652951 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Hide overlay play button if it can't be shown without clipping. This CL also refactors MediaControls to listen for size changes via ResizeObserver instead of via notifications from LayoutMedia. Additionally, this CL changes the position of the overlay play button to vertically center on the height of the video minus the height of the media controls panel. BUG=652951 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry I couldn't finish the review there is an issue below. https://codereview.chromium.org/2701433003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2701433003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:61: #endif Can you drop the `#if`? I don't think we even need to worry about non-Android case. Also, you might want to add a TODO that says that we need a better solution that hard coding values here. Maybe point to the css file. https://codereview.chromium.org/2701433003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:64: // LayoutTests/virtual/android/media/controls/overlay-play-button.js. Not sure how useful is this comment as the tests would fail anyway. https://codereview.chromium.org/2701433003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutMedia.cpp (left): https://codereview.chromium.org/2701433003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutMedia.cpp:86: newPanelWidth = width; Here, we set `width` then pass it to `newPanelWidth` which was then sent to the MediaControls. My intuition is that `computePanelWidth()` became irrelevant. Should this logic be moved to the HTMLMediaControls then?
lgtm with comments applied. Thanks a lot :) https://codereview.chromium.org/2701433003/diff/280001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/virtual/android/media/controls/overlay-play-button-document-move.html (right): https://codereview.chromium.org/2701433003/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/virtual/android/media/controls/overlay-play-button-document-move.html:2: <title>media controls overlay play button document move</title> Could you get these tests in media/controls/ but not virtual/android/. I don't think they need to be in virtual/android/. https://codereview.chromium.org/2701433003/diff/280001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/virtual/android/media/controls/overlay-play-button.js (right): https://codereview.chromium.org/2701433003/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/virtual/android/media/controls/overlay-play-button.js:2: // Minimum width is 48px on both desktop and Android. Isn't this actually define in css? https://codereview.chromium.org/2701433003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2701433003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:61: #endif On 2017/03/07 at 19:07:30, mlamouri wrote: > Can you drop the `#if`? I don't think we even need to worry about non-Android case. Also, you add a TODO that says that we need a better solution that hard coding values here. Maybe point to the css file. https://codereview.chromium.org/2701433003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutMedia.cpp (left): https://codereview.chromium.org/2701433003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutMedia.cpp:86: newPanelWidth = width; On 2017/03/07 at 19:07:30, mlamouri wrote: > Here, we set `width` then pass it to `newPanelWidth` which was then sent to the MediaControls. My intuition is that `computePanelWidth()` became irrelevant. Should this logic be moved to the HTMLMediaControls then? Scratch that comment :) https://codereview.chromium.org/2701433003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/MediaControlsPainter.cpp (right): https://codereview.chromium.org/2701433003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/MediaControlsPainter.cpp:197: mediaPanelHeight = panelElement->clientHeight(); I'm a huge fan of ternary operator and I would obviously recommend `int mediaPanelHeight = panelElement ? panelElement->clientHeight() : 0;` but others have different opinions that you can agree with :)
avayvod@chromium.org changed reviewers: - avayvod@chromium.orgm
https://codereview.chromium.org/2701433003/diff/280001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/virtual/android/media/controls/overlay-play-button-document-move.html (right): https://codereview.chromium.org/2701433003/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/virtual/android/media/controls/overlay-play-button-document-move.html:2: <title>media controls overlay play button document move</title> On 2017/03/08 19:13:04, mlamouri wrote: > Could you get these tests in media/controls/ but not virtual/android/. I don't > think they need to be in virtual/android/. Done. https://codereview.chromium.org/2701433003/diff/280001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/virtual/android/media/controls/overlay-play-button.js (right): https://codereview.chromium.org/2701433003/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/virtual/android/media/controls/overlay-play-button.js:2: // Minimum width is 48px on both desktop and Android. On 2017/03/08 19:13:04, mlamouri wrote: > Isn't this actually define in css? Well, the width (and height) of the play button is defined in MediaControlsPainter.cpp. However, the max width (and max height) is defined in MediaControls.cpp. It's just that the max width is the same as the width since we're able to use the whole video width to display the button (unlike the height). https://codereview.chromium.org/2701433003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2701433003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:61: #endif On 2017/03/08 19:13:04, mlamouri wrote: > On 2017/03/07 at 19:07:30, mlamouri wrote: > > Can you drop the `#if`? I don't think we even need to worry about non-Android > case. Also, you add a TODO that says that we need a better solution that hard > coding values here. Maybe point to the css file. > Done. https://codereview.chromium.org/2701433003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:64: // LayoutTests/virtual/android/media/controls/overlay-play-button.js. On 2017/03/07 19:07:30, mlamouri wrote: > Not sure how useful is this comment as the tests would fail anyway. Done. https://codereview.chromium.org/2701433003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutMedia.cpp (left): https://codereview.chromium.org/2701433003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutMedia.cpp:86: newPanelWidth = width; On 2017/03/08 19:13:04, mlamouri wrote: > On 2017/03/07 at 19:07:30, mlamouri wrote: > > Here, we set `width` then pass it to `newPanelWidth` which was then sent to > the MediaControls. My intuition is that `computePanelWidth()` became irrelevant. > Should this logic be moved to the HTMLMediaControls then? > > Scratch that comment :) Acknowledged. https://codereview.chromium.org/2701433003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/MediaControlsPainter.cpp (right): https://codereview.chromium.org/2701433003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/MediaControlsPainter.cpp:197: mediaPanelHeight = panelElement->clientHeight(); On 2017/03/08 19:13:04, mlamouri wrote: > I'm a huge fan of ternary operator and I would obviously recommend `int > mediaPanelHeight = panelElement ? panelElement->clientHeight() : 0;` but others > have different opinions that you can agree with :) Done.
steimel@chromium.org changed reviewers: + foolip@chromium.org
+foolip@ for OWNERS review
The CQ bit was checked by steimel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
The CQ bit was checked by steimel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2701433003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2701433003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:54: // Defined in core/css/mediaControls.css and core/css/mediaControlsAndroid.css. do you also have a comment in these files in case they get changed separately?
The CQ bit was checked by steimel@chromium.org to run a CQ dry run
https://codereview.chromium.org/2701433003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2701433003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:54: // Defined in core/css/mediaControls.css and core/css/mediaControlsAndroid.css. On 2017/03/09 21:48:14, ikilpatrick wrote: > do you also have a comment in these files in case they get changed separately? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
steimel@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng@ for OWNERS review
rs lgtm
The CQ bit was checked by steimel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2701433003/#ps340001 (title: "Add comments for dependent constants")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 340001, "attempt_start_ts": 1489503108262280,
"parent_rev": "255225462a687af278ab3118122638816dd4a212", "commit_rev":
"5b069f0bfe05879dc2dfbc32d9b4b171c622d5c3"}
Message was sent while issue was closed.
Description was changed from ========== Hide overlay play button if it can't be shown without clipping. This CL also refactors MediaControls to listen for size changes via ResizeObserver instead of via notifications from LayoutMedia. Additionally, this CL changes the position of the overlay play button to vertically center on the height of the video minus the height of the media controls panel. BUG=652951 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Hide overlay play button if it can't be shown without clipping. This CL also refactors MediaControls to listen for size changes via ResizeObserver instead of via notifications from LayoutMedia. Additionally, this CL changes the position of the overlay play button to vertically center on the height of the video minus the height of the media controls panel. BUG=652951 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2701433003 Cr-Commit-Position: refs/heads/master@{#456724} Committed: https://chromium.googlesource.com/chromium/src/+/5b069f0bfe05879dc2dfbc32d9b4... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001) as https://chromium.googlesource.com/chromium/src/+/5b069f0bfe05879dc2dfbc32d9b4... |
