|
|
Created:
3 years, 10 months ago by Zhiqiang Zhang (Slow) Modified:
3 years, 9 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, mlamouri+watch-blink_chromium.org, nessy, Srirama Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Blink>Media] Add heuristic for dominant video detection for Android
This CL adds the heuristic for dominant video detection for
Android, in which we use the fullscreen state of the document and
the viewport intersection to determine if a video is the dominant
content in fullscreen mode (as the page may fullscreen the
embedding div instead of the video element).
BUG=695462
Review-Url: https://codereview.chromium.org/2696893002
Cr-Commit-Position: refs/heads/master@{#454272}
Committed: https://chromium.googlesource.com/chromium/src/+/abc082487c58110e72e8d8e9ff6fa0b7abc9dd12
Patch Set 1 #
Total comments: 4
Patch Set 2 : fixed nits #Patch Set 3 : tweaked the logic and merged tests #Patch Set 4 : rework done #Patch Set 5 : . #Patch Set 6 : . #Patch Set 7 : s/CustomControlsFullscreenDetector/MediaCustomControlsFullscreenDetector #
Total comments: 10
Patch Set 8 : addressed mlamouri's comments #
Total comments: 11
Patch Set 9 : addressed comments #Patch Set 10 : nits #Patch Set 11 : fixing logic in WMPI #
Total comments: 6
Patch Set 12 : addressed nits #
Total comments: 14
Patch Set 13 : addressed mlamouri@'s comments #Patch Set 14 : fixed event listener leak #
Total comments: 3
Patch Set 15 : nits #Messages
Total messages: 78 (41 generated)
Description was changed from ========== [Blink>Media] Improve the heuristic for dominant video detection Previously, the heuristic for determining the dominant video is pretty desktop-orientated, which does not fit on mobile as most screens are portrait but most videos are landscape. This CL improves the heuristics so that it no longer relies on screen/video orientation. Now two heuristics are used: * The proportion of a video that is visible must be greater than a threshold. * One edge of the video must occupy most of that dimension of the viewport. BUG=692098 ========== to ========== [Blink>Media] Improve the heuristic for dominant video detection Previously, the heuristic for determining the dominant video is pretty desktop-orientated, which does not fit on mobile as most screens are portrait but most videos are landscape. This CL improves the heuristics so that it no longer relies on screen/video orientation. Now two heuristics are used: * The proportion of a video that is visible must be greater than a threshold. * One edge of the video must occupy most of that dimension of the viewport. BUG=692098 ==========
zqzhang@chromium.org changed reviewers: + mlamouri@chromium.org
zqzhang@chromium.org changed reviewers: + miu@chromium.org
+miu: PTAL
This sgtm but I would like miu@ to give some feedback. https://codereview.chromium.org/2696893002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2696893002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:4149: float yOccupationRatio = 1.0f * intersectionRect.height() / rootRect.height(); nit: I guess those can be `const float` https://codereview.chromium.org/2696893002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElementTest.cpp (right): https://codereview.chromium.org/2696893002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElementTest.cpp:154: ASSERT_TRUE(HTMLMediaElementTest::computeIsMostlyFillingViewport( nit: here and below: use EXPECT_*
The CQ bit was checked by zqzhang@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/2696893002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2696893002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:4149: float yOccupationRatio = 1.0f * intersectionRect.height() / rootRect.height(); On 2017/02/15 11:08:04, mlamouri wrote: > nit: I guess those can be `const float` Done. https://codereview.chromium.org/2696893002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElementTest.cpp (right): https://codereview.chromium.org/2696893002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElementTest.cpp:154: ASSERT_TRUE(HTMLMediaElementTest::computeIsMostlyFillingViewport( On 2017/02/15 11:08:04, mlamouri wrote: > nit: here and below: use EXPECT_* Oops. Why did I do that :/ Done.
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_...)
PTAL * Tweaked the logic a bit (as I missed the case where a part of zoomed video is occupying most of the viewport). * Merged the tests into MediaElementFillingViewportTest.
miu@chromium.org changed reviewers: + xjz@chromium.org
+xjz
not lgtm On desktop, the "fills one dimension" will cause the wrong behavior for Media Remoting on some sites where the videos fill the X dimension mostly, but not the Y dimension (e.g., sort of like: https://vimeo.com/168812053). Your new heuristic doesn't seem to be accurately described as "dominant video in viewport." Perhaps you should be implementing a separate signal? Also, could you please explain how you intend to use this signal and what the user experience stories will be?
I just chatted with mlamouri@ about this. It seems the main issue is that the "fullscreen" status of the element isn't being taken into account. Based on our discussion, I'd propose the following: 1. If the HTMLMediaElement or an ancestor element is NOT the document's fullscreen element, use the existing "portion of viewport area" logic. 2. If the element or an ancestor IS the document's fullscreen element, then use the "one dimension extends to edge" logic. I think this would result in the desired behavior for both desktop and mobile, and for all use cases. WDYT?
On 2017/02/15 21:51:51, miu wrote: > I just chatted with mlamouri@ about this. It seems the main issue is that the > "fullscreen" status of the element isn't being taken into account. Based on our > discussion, I'd propose the following: > > 1. If the HTMLMediaElement or an ancestor element is NOT the document's > fullscreen element, use the existing "portion of viewport area" logic. > > 2. If the element or an ancestor IS the document's fullscreen element, then use > the "one dimension extends to edge" logic. > > I think this would result in the desired behavior for both desktop and mobile, > and for all use cases. WDYT? Agree. Though this doesn't work for the edge case that has multiple video elements that have one dimension dominant in viewport. Can we use different logic for desktop and mobile?
I'm reworking on it after talking to mlamouri@. Will let you know when it's done.
The CQ bit was checked by zqzhang@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 zqzhang@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...
PTAL The latest changes: * Using a helper class listening to fullscreen events and compute isDominantVideo, which decouples from HTMLMediaElement. * Existing logic for MediaRemote is preserved.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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 zqzhang@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 ========== [Blink>Media] Improve the heuristic for dominant video detection Previously, the heuristic for determining the dominant video is pretty desktop-orientated, which does not fit on mobile as most screens are portrait but most videos are landscape. This CL improves the heuristics so that it no longer relies on screen/video orientation. Now two heuristics are used: * The proportion of a video that is visible must be greater than a threshold. * One edge of the video must occupy most of that dimension of the viewport. BUG=692098 ========== to ========== [Blink>Media] Add heuristic for dominant video detection for Android This CL adds the heuristic for dominant video detection for Android, in which we use the fullscreen state of the document and the viewport intersection to determine if a video is the dominant content in fullscreen mode (as the page may fullscreen the embedding div instead of the video element). BUG=692098 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Just had a quick look. Didn't check the tests and algorithm much. miu@, can you PTAL? https://codereview.chromium.org/2696893002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp (right): https://codereview.chromium.org/2696893002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:47: oldDocument.removeEventListener(EventTypeNames::fullscreenchange, this, true); We should call addEventListener on the new document, right? Maybe this should be tested? :) https://codereview.chromium.org/2696893002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:56: return false; Can this happen? We should be fullscreen, right? https://codereview.chromium.org/2696893002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:95: false); We should probably cancel the timer too so we guarantee the callback will not be called unless we are effectively fullscreen. https://codereview.chromium.org/2696893002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:116: isDominant); What happens if the video goes fullscreen before playing or loading? Can we end up with no webMediaPlayer? Should we listen to playing or loadedmetadata event to guarantee that we properly notify the WMP? https://codereview.chromium.org/2696893002/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/2696893002/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebMediaPlayer.h:275: virtual void becameDominantVisibleContentInFullscreen(bool isDominant) {} I would call this `setEffectivelyFullscreen` but it's really bikeshedding :)
miu@, xjz@: PTAL https://codereview.chromium.org/2696893002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp (right): https://codereview.chromium.org/2696893002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:47: oldDocument.removeEventListener(EventTypeNames::fullscreenchange, this, true); On 2017/02/21 at 21:58:28, mlamouri wrote: > We should call addEventListener on the new document, right? > > Maybe this should be tested? :) Done. https://codereview.chromium.org/2696893002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:56: return false; On 2017/02/21 at 21:58:28, mlamouri wrote: > Can this happen? We should be fullscreen, right? Done, removed this parameter as we should be in fullscreen when calling this method. https://codereview.chromium.org/2696893002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:95: false); On 2017/02/21 at 21:58:28, mlamouri wrote: > We should probably cancel the timer too so we guarantee the callback will not be called unless we are effectively fullscreen. Done. https://codereview.chromium.org/2696893002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:116: isDominant); On 2017/02/21 at 21:58:28, mlamouri wrote: > What happens if the video goes fullscreen before playing or loading? Should we listen to playing or loadedmetadata event to guarantee that we properly notify the WMP? Now I also listen to loadedmetadata. When any event is received, we check the media element readyState, and early return if it's less thank HAVE_METADATA. > Can we end up with no webMediaPlayer? Yes, the trybots complained somehow. https://codereview.chromium.org/2696893002/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/2696893002/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebMediaPlayer.h:275: virtual void becameDominantVisibleContentInFullscreen(bool isDominant) {} On 2017/02/21 at 21:58:28, mlamouri wrote: > I would call this `setEffectivelyFullscreen` but it's really bikeshedding :) Done.
https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp (right): https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:62: 1.0f * intersectionRect.width() / rootRect.width(); Can rootRect be empty? If not, maybe add DCHECK. https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:131: return DocumentFullscreen::fullscreenElement(videoElement().document()); Can the fullscreen element be the child of the video element? Maybe call this "hasFullscreenElement"? ooc, what's the difference between fullscreenElement() and currentFullscreenElement()? Which one should be used here? https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebMediaPlayer.h:276: // fullscreen video, i.e. being fullscreened or in a fullscreened node, and s/in a fullscreened node/in a fullscreened document? s/the dominant video in fullscreen/the dominant video in viewport?
PTAL Addressed xjz@'s comments. https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp (right): https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:62: 1.0f * intersectionRect.width() / rootRect.width(); On 2017/02/22 at 18:48:34, xjz wrote: > Can rootRect be empty? If not, maybe add DCHECK. Thanks. Now I return false if targetRect or rootRect is empty. https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:131: return DocumentFullscreen::fullscreenElement(videoElement().document()); On 2017/02/22 at 18:48:34, xjz wrote: > Can the fullscreen element be the child of the video element? This should be rare and we are focusing on the typical use cases at the moment. We can always iterate if there are some edge cases. > Maybe call this "hasFullscreenElement"? A more proper name is "isDocumentFullscreen". mlamouri@: should this logic be sufficient? In the very beginning I wanted to look up in the dom tree and see if any element is the fullscreen element. Now I just check if the document has fullscreen element. > ooc, what's the difference between fullscreenElement() and currentFullscreenElement()? Which one should be used here? The other one is asynchronous, but at this point, they should be the same. Also calling the Fullscreen:: version instead of DocumentFullscreen:: version https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebMediaPlayer.h:276: // fullscreen video, i.e. being fullscreened or in a fullscreened node, and On 2017/02/22 at 18:48:34, xjz wrote: > s/in a fullscreened node/in a fullscreened document? > > s/the dominant video in fullscreen/the dominant video in viewport? Done.
lgtm % https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp (right): https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:62: 1.0f * intersectionRect.width() / rootRect.width(); On 2017/02/23 21:50:55, Zhiqiang Zhang wrote: > On 2017/02/22 at 18:48:34, xjz wrote: > > Can rootRect be empty? If not, maybe add DCHECK. > > Thanks. Now I return false if targetRect or rootRect is empty. I don't see any changes though. :)
https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp (right): https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:62: 1.0f * intersectionRect.width() / rootRect.width(); On 2017/02/23 at 22:49:46, xjz wrote: > On 2017/02/23 21:50:55, Zhiqiang Zhang wrote: > > On 2017/02/22 at 18:48:34, xjz wrote: > > > Can rootRect be empty? If not, maybe add DCHECK. > > > > Thanks. Now I return false if targetRect or rootRect is empty. > > I don't see any changes though. :) Sorry, it's a mistake (committed to another checkout). Done. https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:131: return DocumentFullscreen::fullscreenElement(videoElement().document()); I reverted back to isVideoOrParentFullscreen(). Now I'm not just checking the document has fullscreen element, but also check if the fullscreen element is the video element itself or its ancestor.
The CQ bit was checked by zqzhang@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 ========== [Blink>Media] Add heuristic for dominant video detection for Android This CL adds the heuristic for dominant video detection for Android, in which we use the fullscreen state of the document and the viewport intersection to determine if a video is the dominant content in fullscreen mode (as the page may fullscreen the embedding div instead of the video element). BUG=692098 ========== to ========== [Blink>Media] Add heuristic for dominant video detection for Android This CL adds the heuristic for dominant video detection for Android, in which we use the fullscreen state of the document and the viewport intersection to determine if a video is the dominant content in fullscreen mode (as the page may fullscreen the embedding div instead of the video element). BUG=695462 ==========
https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp (right): https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:131: return DocumentFullscreen::fullscreenElement(videoElement().document()); On 2017/02/24 12:03:40, Zhiqiang Zhang wrote: > I reverted back to isVideoOrParentFullscreen(). Now I'm not just checking the > document has fullscreen element, but also check if the fullscreen element is the > video element itself or its ancestor. The logic sgtm. Just ooc, if the video is the current fullscreen element, do you still need to start timer and check whether it is dominant? The fullscreen might already be a strong indicator. WDYT?
lgtm % a couple small things: https://codereview.chromium.org/2696893002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp (right): https://codereview.chromium.org/2696893002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:139: Element* currentElement = &videoElement(); IIRC, there's some kind of isAncestorOf() utility method you should use instead of rolling your own loop here. https://codereview.chromium.org/2696893002/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/2696893002/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebMediaPlayer.h:273: virtual void becameDominantVisibleContent(bool isDominant) {} Can the old heuristic be removed? It seems the new one should work for our use case as well as yours. :) If you want to address that in a separate change, please put a TODO comment here and file a crbug for tracking (cc'ing me and xjz). Also, feel free to send us a patch and we can test it out on our end to make sure all's well.
https://codereview.chromium.org/2696893002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp (right): https://codereview.chromium.org/2696893002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:98: event->type() == EventTypeNames::loadedmetadata); Do you also want to listen to the resize event of the video element since it might be resized while one of its parent is in fullscreen?
The CQ bit was checked by zqzhang@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/2696893002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp (right): https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:131: return DocumentFullscreen::fullscreenElement(videoElement().document()); On 2017/02/24 at 18:30:10, xjz wrote: > On 2017/02/24 12:03:40, Zhiqiang Zhang wrote: > > I reverted back to isVideoOrParentFullscreen(). Now I'm not just checking the > > document has fullscreen element, but also check if the fullscreen element is the > > video element itself or its ancestor. > > The logic sgtm. Just ooc, if the video is the current fullscreen element, do you still need to start timer and check whether it is dominant? The fullscreen might already be a strong indicator. WDYT? That would bring in several more if-else's. I'd prefer keep the logic simpler. https://codereview.chromium.org/2696893002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp (right): https://codereview.chromium.org/2696893002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:98: event->type() == EventTypeNames::loadedmetadata); On 2017/02/25 at 00:49:12, xjz wrote: > Do you also want to listen to the resize event of the video element since it might be resized while one of its parent is in fullscreen? Not for now. We are counting on the page to resize the video element properly. That's why we have a 1 second delay for it. Any layout change after that will be ignored. https://codereview.chromium.org/2696893002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:139: Element* currentElement = &videoElement(); On 2017/02/24 at 22:44:39, miu wrote: > IIRC, there's some kind of isAncestorOf() utility method you should use instead of rolling your own loop here. Yes, Node.contains(). Done :) https://codereview.chromium.org/2696893002/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/2696893002/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebMediaPlayer.h:273: virtual void becameDominantVisibleContent(bool isDominant) {} On 2017/02/24 at 22:44:39, miu wrote: > Can the old heuristic be removed? It seems the new one should work for our use case as well as yours. :) > > If you want to address that in a separate change, please put a TODO comment here and file a crbug for tracking (cc'ing me and xjz). > > Also, feel free to send us a patch and we can test it out on our end to make sure all's well. Filed a bug and added TODO :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mlamouri: PTAL
lgtm with comments applied. Two follow-ups to keep in mind: - have this behind a runtime enabled flags so we only use these CPU cycles when we need to - we should handle removal from document but that's a general issue with controls https://codereview.chromium.org/2696893002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLVideoElement.cpp (right): https://codereview.chromium.org/2696893002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLVideoElement.cpp:284: if (m_customControlsFullscreenDetector) Why do you do a null-check here? https://codereview.chromium.org/2696893002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp (right): https://codereview.chromium.org/2696893002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:109: } style: remove { } https://codereview.chromium.org/2696893002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:130: } style: remove { } https://codereview.chromium.org/2696893002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.h (right): https://codereview.chromium.org/2696893002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.h:21: public: Add WTF_MAKE_NONCOPYABLE() above `public:`? https://codereview.chromium.org/2696893002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.h:27: HTMLVideoElement& videoElement() { return *m_videoElement; } could this be private? https://codereview.chromium.org/2696893002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.h:40: bool onExitFullscreen(); these are not implemented it looks like https://codereview.chromium.org/2696893002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.h:46: static bool computeIsDominantVideo(const IntRect& targetRect, Maybe leave a note that this is visible for tests? You could even have "computeIsDominantVideoForTests()" that call an anonymous function inside the cpp file if we want very nit-picky :)
Patchset #13 (id:240001) has been deleted
zqzhang@chromium.org changed reviewers: + foolip@chromium.org, hubbe@chromium.org
+foolip: third_party/WebKit/ +hubbe: media/ https://codereview.chromium.org/2696893002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLVideoElement.cpp (right): https://codereview.chromium.org/2696893002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLVideoElement.cpp:284: if (m_customControlsFullscreenDetector) On 2017/02/27 at 18:04:55, mlamouri wrote: > Why do you do a null-check here? Got mislead by the if-statement above. Removed. https://codereview.chromium.org/2696893002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp (right): https://codereview.chromium.org/2696893002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:109: } On 2017/02/27 at 18:04:55, mlamouri wrote: > style: remove { } Done. https://codereview.chromium.org/2696893002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:130: } On 2017/02/27 at 18:04:55, mlamouri wrote: > style: remove { } Done. https://codereview.chromium.org/2696893002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.h (right): https://codereview.chromium.org/2696893002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.h:21: public: On 2017/02/27 at 18:04:55, mlamouri wrote: > Add WTF_MAKE_NONCOPYABLE() above `public:`? Done. https://codereview.chromium.org/2696893002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.h:27: HTMLVideoElement& videoElement() { return *m_videoElement; } On 2017/02/27 at 18:04:55, mlamouri wrote: > could this be private? Done. https://codereview.chromium.org/2696893002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.h:40: bool onExitFullscreen(); On 2017/02/27 at 18:04:55, mlamouri wrote: > these are not implemented it looks like Removed. https://codereview.chromium.org/2696893002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.h:46: static bool computeIsDominantVideo(const IntRect& targetRect, On 2017/02/27 at 18:04:55, mlamouri wrote: > Maybe leave a note that this is visible for tests? You could even have "computeIsDominantVideoForTests()" that call an anonymous function inside the cpp file if we want very nit-picky :) Using name `computeIsDominantVideoForTests()`. Calling an anonymous function seems a bit overkill.
media/* LGTM
zqzhang@chromium.org changed reviewers: + rbyers@chromium.org
+rbyers for rubber-stamping third_party/WebKit/
zqzhang@chromium.org changed reviewers: + haraken@chromium.org
+haraken for rubber-stamping third_party/WebKit in APAC time
Implementation-wise looks good, but do we have tests?
On 2017/03/02 01:26:07, haraken wrote: > Implementation-wise looks good, but do we have tests? Sorry, I was not opening the *Test.cpp :) LGTM.
The CQ bit was checked by zqzhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xjz@chromium.org, miu@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2696893002/#ps260001 (title: "addressed mlamouri@'s comments")
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
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_...)
Description was changed from ========== [Blink>Media] Add heuristic for dominant video detection for Android This CL adds the heuristic for dominant video detection for Android, in which we use the fullscreen state of the document and the viewport intersection to determine if a video is the dominant content in fullscreen mode (as the page may fullscreen the embedding div instead of the video element). BUG=695462 ========== to ========== [Blink>Media] Add heuristic for dominant video detection for Android This CL adds the heuristic for dominant video detection for Android, in which we use the fullscreen state of the document and the viewport intersection to determine if a video is the dominant content in fullscreen mode (as the page may fullscreen the embedding div instead of the video element). BUG=695462 ==========
rbyers@chromium.org changed reviewers: - rbyers@chromium.org
mlamouri: PTAL diff #13..#14
The CQ bit was checked by zqzhang@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...
lgtm with comment applied https://codereview.chromium.org/2696893002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp (right): https://codereview.chromium.org/2696893002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:102: } What about: if (event->type() == ...) { attach(); return; } if (event-type() == ...) { detach(); return; } DCHECK(... event must be one of the expected ones ...);
https://codereview.chromium.org/2696893002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp (right): https://codereview.chromium.org/2696893002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:102: } On 2017/03/02 at 14:23:36, mlamouri wrote: > What about: > if (event->type() == ...) { > attach(); > return; > } > if (event-type() == ...) { > detach(); > return; > } > > DCHECK(... event must be one of the expected ones ...); Zhiqiang told me how wrong I was. Second time today :)
The CQ bit was checked by zqzhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xjz@chromium.org, hubbe@chromium.org, miu@chromium.org, haraken@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2696893002/#ps300001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2696893002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp (right): https://codereview.chromium.org/2696893002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:102: } On 2017/03/02 at 14:27:18, mlamouri wrote: > On 2017/03/02 at 14:23:36, mlamouri wrote: > > What about: > > if (event->type() == ...) { > > attach(); > > return; > > } > > if (event-type() == ...) { > > detach(); > > return; > > } > > > > DCHECK(... event must be one of the expected ones ...); > > Zhiqiang told me how wrong I was. Second time today :) Added DCHECK() and comments explaining why
CQ is committing da patch. Bot data: {"patchset_id": 300001, "attempt_start_ts": 1488465169195270, "parent_rev": "37a76369eb0ad7176c613023f29a16fde58f8b97", "commit_rev": "abc082487c58110e72e8d8e9ff6fa0b7abc9dd12"}
Message was sent while issue was closed.
Description was changed from ========== [Blink>Media] Add heuristic for dominant video detection for Android This CL adds the heuristic for dominant video detection for Android, in which we use the fullscreen state of the document and the viewport intersection to determine if a video is the dominant content in fullscreen mode (as the page may fullscreen the embedding div instead of the video element). BUG=695462 ========== to ========== [Blink>Media] Add heuristic for dominant video detection for Android This CL adds the heuristic for dominant video detection for Android, in which we use the fullscreen state of the document and the viewport intersection to determine if a video is the dominant content in fullscreen mode (as the page may fullscreen the embedding div instead of the video element). BUG=695462 Review-Url: https://codereview.chromium.org/2696893002 Cr-Commit-Position: refs/heads/master@{#454272} Committed: https://chromium.googlesource.com/chromium/src/+/abc082487c58110e72e8d8e9ff6f... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:300001) as https://chromium.googlesource.com/chromium/src/+/abc082487c58110e72e8d8e9ff6f... |