Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2475643004:
Monitor the intersection of video and viewport. (Closed)
|
Created:
4 years, 1 month ago by xjz Modified:
4 years, 1 month ago Reviewers:
szager1, mlamouri (slow - plz ping), esprehn, Zhiqiang Zhang (Slow), Z_DONOTUSE, ojan, xhwang, miu CC:
blink-reviews, blink-reviews-api_chromium.org, blink-reviews-dom_chromium.org, blink-reviews-html_chromium.org, chromium-reviews, chromoting-reviews_chromium.org, dglazkov+blink, eae+blinkwatch, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, miu+watch_chromium.org, mlamouri+watch-blink_chromium.org, rwlbuis, sof, nessy, Srirama, vcarbune.chromium, xjz+watch_chromium.org, wjmaclean, dcheng Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMonitor the intersection of video and viewport.
This CL:
1. Adds ViewportIntersectionObserver to monoitor the intersection of
an element with viewport.
2. Consolidates three ElementVisibilityObservers for HTMLMediaElement
to one observer.
3. Notifies WebMediaPlayer the intersection info when changed.
4. Adds logic to start remoting when video consistently covers most
of the viewport for a certain time.
BUG=643964
Patch Set 1 #Patch Set 2 : Consolidate two ElementVisibilityObserver in AUtoplayUmaHelper. #
Total comments: 24
Patch Set 3 : Observes any video/viewport ratio changes. #Patch Set 4 : Add ElementViewportRatioObserver. #
Total comments: 22
Patch Set 5 : Addressed miu's comments. #
Total comments: 6
Patch Set 6 : Addressed miu's comments from PS#5. #
Total comments: 14
Patch Set 7 : Addressed comments. Observe intersection changing. #
Total comments: 6
Patch Set 8 : Moved the calculation of intersection ratio to RemotingController. #
Total comments: 10
Patch Set 9 : Addressed comments. #
Total comments: 8
Patch Set 10 : Use empty thresholds to indicate observing intersection with viewport. #
Total comments: 2
Patch Set 11 : Subclass IntersectionObserver. #
Total comments: 2
Patch Set 12 : Rebase. #Patch Set 13 : Fix trybots failure. #Messages
Total messages: 64 (25 generated)
Patchset #1 (id:1) has been deleted
miu@chromium.org changed reviewers: + miu@chromium.org
https://codereview.chromium.org/2475643004/diff/40001/media/remoting/remoting... File media/remoting/remoting_controller.cc (right): https://codereview.chromium.org/2475643004/diff/40001/media/remoting/remoting... media/remoting/remoting_controller.cc:148: is_fullscreen_ = ratio > 0.85; Need "debouncing" logic here. Something like "no changes in ratio for 5 seconds" before we attempt to start remoting: void RemotingController::OnVideoViewportRatioChanged(double ratio) { // Reset on any notification, since this indicates the user is scrolling // around in the document, the document is changing layout, etc. viewport_fill_debouncer_timer_.Reset(); // Dropping below the threshold should instantly stop remote rendering. if (ratio < 0.85) { if (is_mostly_filling_viewport_) { is_mostly_filling_viewport_ = false; UpdateAndMaybeSwitch(); } return; } // Meeting/Exceeding the threshold should hold steady for 5 seconds before // starting remote rendering. if (!is_mostly_filling_viewport_) { viewport_fill_debouncer_timer_.Start( ..., 5 seconds, this, &RemotingController::OnViewportMostlyFilledAndUnchanging); } } void RemotingController::OnViewportMostlyFilledAndUnchanging() { is_mostly_filling_viewport_ = true; UpdateAndMaybeSwitch(); } Note: |viewport_fill_debouncer_timer_| is a base::OneShotTimer. I also suggest using a separate flag for this (i.e., don't re-use |is_fullscreen_|). https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right): https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:19: bool shouldReportRootBounds) Did you intend to remove the 3rd argument to this ctor? Otherwise, why are you ignoring it now? https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.h (right): https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:90: // Indicates starting observing whether a muted video autoplaying by play() With better variable naming, I think you should delete this comment. Otherwise, the comment, as it is, needs to be rewritten (grammar issue). https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:92: // The UMA is pending for recording as long as this observer is non-null. It's not an observer anymore. Maybe this should say something like "when this boolean transitions from true to false, a UMA is recorded." https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:93: bool m_mutedVideoPlayMethodVisibilityObserver = false; naming: Wow. The existing code's naming of members and methods is so complicated. If I were changing this code, I'd feel compelled to do massive cleanup here. Regardless, for your change, I'd suggest something much simpler like (IIUC how this boolean is used): m_isObservingVisibilityOfMutedAutoplayVideo; https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:93: bool m_mutedVideoPlayMethodVisibilityObserver = false; To be honest, I'm not sure this boolean is even necessary. If you get events that say the video is visible or not visible, you should be able to tell when a transition occurs. https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:113: bool m_mutedVideoOffscreenDurationVisibilityObserver = false; ditto here: Same problems with code comment, variable naming, etc. https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:489: Vector<float>({std::numeric_limits<float>::min(), 0.85})); 1. Where does 0.85 come from, and how do we change it? This puts logic into Blink that really depends on our feature in Chromium code. We want to keep all control/decision logic in the Chromium code. 2. Our Chromium code will want to know all ratios (including zero), because we will want to "debounce" before deciding to commit to starting or stopping remoting. Thus, I think you don't need the second ElementVisibilityObserver::start(vector) method. https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2356: bool wasPendingAutoplayMuted = m_autoplayWhenVisible && paused() && m_muted && Is the m_autoplayWhenVisible boolean necessary? Why can't this expression be: bool wasPendingAutoplayMuted = !isGestureNeededForPlayback() && isHTMLVideoElement() && RuntimeEnabledFeatures::autoplayMutedVideosEnabled() && paused() && m_muted && isLockedPendingUserGesture(); (Perhaps you might have the previous author of these LOC look over this logic, to be sure it is correct.) https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2383: // have a visibility observer ready to start its playback. Please adjust comment. https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.h:719: bool m_autoplayWhenVisible = false; naming nit: m_shouldAutoplayWhenVisible https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.h:719: bool m_autoplayWhenVisible = false; Is the boolean necessary? (See comment in .cpp file.)
Patchset #3 (id:60001) has been deleted
miu@: Addressed comments in PS#3. PTAL. https://codereview.chromium.org/2475643004/diff/40001/media/remoting/remoting... File media/remoting/remoting_controller.cc (right): https://codereview.chromium.org/2475643004/diff/40001/media/remoting/remoting... media/remoting/remoting_controller.cc:148: is_fullscreen_ = ratio > 0.85; On 2016/11/05 02:47:46, miu wrote: > Need "debouncing" logic here. Something like "no changes in ratio for 5 seconds" > before we attempt to start remoting: > > void RemotingController::OnVideoViewportRatioChanged(double ratio) { > // Reset on any notification, since this indicates the user is scrolling > // around in the document, the document is changing layout, etc. > viewport_fill_debouncer_timer_.Reset(); > > // Dropping below the threshold should instantly stop remote rendering. > if (ratio < 0.85) { > if (is_mostly_filling_viewport_) { > is_mostly_filling_viewport_ = false; > UpdateAndMaybeSwitch(); > } > return; > } > > // Meeting/Exceeding the threshold should hold steady for 5 seconds before > // starting remote rendering. > if (!is_mostly_filling_viewport_) { > viewport_fill_debouncer_timer_.Start( > ..., 5 seconds, this, > &RemotingController::OnViewportMostlyFilledAndUnchanging); > } > } > > void RemotingController::OnViewportMostlyFilledAndUnchanging() { > is_mostly_filling_viewport_ = true; > UpdateAndMaybeSwitch(); > } > > Note: |viewport_fill_debouncer_timer_| is a base::OneShotTimer. I also suggest > using a separate flag for this (i.e., don't re-use |is_fullscreen_|). Done. https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right): https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:19: bool shouldReportRootBounds) On 2016/11/05 02:47:46, miu wrote: > Did you intend to remove the 3rd argument to this ctor? Otherwise, why are you > ignoring it now? Now keep this argument for original purpose. https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.h (right): https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:90: // Indicates starting observing whether a muted video autoplaying by play() On 2016/11/05 02:47:46, miu wrote: > With better variable naming, I think you should delete this comment. Otherwise, > the comment, as it is, needs to be rewritten (grammar issue). Done. https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:92: // The UMA is pending for recording as long as this observer is non-null. On 2016/11/05 02:47:46, miu wrote: > It's not an observer anymore. Maybe this should say something like "when this > boolean transitions from true to false, a UMA is recorded." Done. https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:93: bool m_mutedVideoPlayMethodVisibilityObserver = false; On 2016/11/05 02:47:46, miu wrote: > naming: Wow. The existing code's naming of members and methods is so > complicated. If I were changing this code, I'd feel compelled to do massive > cleanup here. > > Regardless, for your change, I'd suggest something much simpler like (IIUC how > this boolean is used): > > m_isObservingVisibilityOfMutedAutoplayVideo; Done. https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:93: bool m_mutedVideoPlayMethodVisibilityObserver = false; On 2016/11/05 02:47:46, miu wrote: > To be honest, I'm not sure this boolean is even necessary. > > If you get events that say the video is visible or not visible, you should be > able to tell when a transition occurs. I still keep this boolean. The observing may only start when playing events are handled for a muted video, and will immediately stop once it becomes visible. https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:113: bool m_mutedVideoOffscreenDurationVisibilityObserver = false; On 2016/11/05 02:47:46, miu wrote: > ditto here: Same problems with code comment, variable naming, etc. Done. https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:489: Vector<float>({std::numeric_limits<float>::min(), 0.85})); On 2016/11/05 02:47:47, miu wrote: > 1. Where does 0.85 come from, and how do we change it? This puts logic into > Blink that really depends on our feature in Chromium code. We want to keep all > control/decision logic in the Chromium code. > > 2. Our Chromium code will want to know all ratios (including zero), because we > will want to "debounce" before deciding to commit to starting or stopping > remoting. Thus, I think you don't need the second > ElementVisibilityObserver::start(vector) method. Done. Now is able to monitor every changes on video/viewport ratio. https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2356: bool wasPendingAutoplayMuted = m_autoplayWhenVisible && paused() && m_muted && On 2016/11/05 02:47:47, miu wrote: > Is the m_autoplayWhenVisible boolean necessary? Why can't this expression be: > > bool wasPendingAutoplayMuted = !isGestureNeededForPlayback() && > isHTMLVideoElement() && RuntimeEnabledFeatures::autoplayMutedVideosEnabled() && > paused() && m_muted && isLockedPendingUserGesture(); > > (Perhaps you might have the previous author of these LOC look over this logic, > to be sure it is correct.) As replied to the comment in .h file, I keeps the boolean to save a bunch of checks, and also avoid the risk changing the original logic. https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2383: // have a visibility observer ready to start its playback. On 2016/11/05 02:47:46, miu wrote: > Please adjust comment. Done. https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.h:719: bool m_autoplayWhenVisible = false; On 2016/11/05 02:47:47, miu wrote: > naming nit: m_shouldAutoplayWhenVisible Done. https://codereview.chromium.org/2475643004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.h:719: bool m_autoplayWhenVisible = false; On 2016/11/05 02:47:47, miu wrote: > Is the boolean necessary? (See comment in .cpp file.) I keeps this boolean to save a bunch of checks every time when muted video becomes visible: state change logic + tracksAreReady + shouldAutoplay() + !isGestureNeededForPlayback() + isHTMLVideoElement() + muted() + autoplayMutedVideosEnabled.
Patchset #4 (id:100001) has been deleted
PS4: The media/* files all look good. I have a number of suggestions for the blink changes, however: https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ElementVisibilityObserver.h (right): https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ElementVisibilityObserver.h:46: void startObserveElementViewportRatio(); Instead of the separate method, you should make start() a virtual method and override it in the RatioObserver subclass. https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ElementVisibilityObserver.h:70: ElementViewportRatioObserver(Element*, std::unique_ptr<VisibilityCallback>); Reading this subclass's usage comments, it seems like a lot of burden is being placed on the client code that uses this class. Therefore, how about we add: using ChangeCallback = Function<void(float), WTF::SameThreadAffinity>; ...and the constructor becomes: ElementViewportRatioObserver(Element*, std::unique_ptr<ChangeCallback>); ...and that way the client code only has to provide a callback that takes a float argument (the viewport intersection ratio). https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right): https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:217: bool isObservingRatio = m_observer->isObservingElementViewportRatio(); naming/semantics: IMHO, this shouldn't semantically mean "the observer is observing the viewport ratio." Instead, the behavior we want is "the observer wants continuous measurement updates, not just threshold change updates." So, I think you should delete this LOC, and just make a small change to the if-statement near the bottom of this method (see later comments). https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:227: float rootArea = geometry.rootRect.size().width().toFloat() * Doesn't seem like the math should be different. Won't |geometry.targetRect| be equal to |geometry.rootRect| for our use case? https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:240: if (shouldReportChange) { IMHO, this change should be: if (m_lastThresholdIndex != newThresholdIndex || m_observer->isObservingContinuousChanges()) { https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right): https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:196: IntersectionObserver* IntersectionObserver::create( Get rid of the special-case boolean (see comments below). Then, you don't need this extra create() function. (The one above can be called with an empty set of |thresholds|.) https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:294: if (m_observeElementViewportRatio) { Please get rid of this extra boolean. Instead, the caller of this observe() method should provide a |target| that is equal to the rootNode(). https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:118: if (m_isObservingVisibilityOfMutedVideoOffscreenDuration && isVisible) Note: This method is going to be called from HTMLMediaElement repeatedly for the same |isVisible| value. That never happened before. Therefore, I think you should add to the top of this method: if (isVisible == m_isVisible) return; m_isVisible = isVisible; ...and update the code in the last if-statement of this method: It doesn't need the same check, and doesn't need to assign to m_isVisible. https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.h (right): https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:61: void visibilityMightChangedforMutedVideo(bool isVisible); naming: s/Might/Maybe/ https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:537: void onVideoViewportRatioChanged(bool isVisible); Per comments in other file, this callback method should take the float "ratio" argument. https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebMediaPlayer.h:230: virtual void videoViewportRatioChanged(double ratio) {} This value was a float type in the intersection/visibility observer classes. Should this be float here, or do you think we need more precision in the other code modules?
Description was changed from ========== Detect video/viewport ratio change. Modify the ElementVisibilityObserver to also detect the video/viewport ratio change when video is visible. BUG=643964 ========== to ========== Detect video/viewport ratio change. Modify the ElementVisibilityObserver to also detect the video/viewport ratio change when video is visible. BUG=643964 ==========
xjz@chromium.org changed reviewers: + szager@chromium.org
xjz@chromium.org changed reviewers: + ojan@chromium.org - szager@chromium.org
xjz@chromium.org changed reviewers: + xhwang@chromium.org
xjz@chromium.org changed reviewers: + mlamouri@chromium.org - xhwang@chromium.org
xjz@chromium.org changed reviewers: - mlamouri@chromium.org
Description was changed from ========== Detect video/viewport ratio change. Modify the ElementVisibilityObserver to also detect the video/viewport ratio change when video is visible. BUG=643964 ========== to ========== Detect video/viewport ratio change. Main changes in this CL: 1. Adds ElementViewportRatioObserver to monoitor the video/viewport ratio. 2. Consolidates three ElementVisibilityObservers for HTMLMediaElement to one observer. 3. Notifies WebMediaPlayer the video/viewport ratio. 4. Adds logic to start remoting when video covers most of the viewport for a certain time. BUG=643964 ==========
Addressed miu's comments. PTAL. ojan@: PTAL blink changes. Thanks! https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ElementVisibilityObserver.h (right): https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ElementVisibilityObserver.h:46: void startObserveElementViewportRatio(); On 2016/11/09 22:02:07, miu_OOO_until_14_Nov wrote: > Instead of the separate method, you should make start() a virtual method and > override it in the RatioObserver subclass. Done. https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ElementVisibilityObserver.h:70: ElementViewportRatioObserver(Element*, std::unique_ptr<VisibilityCallback>); On 2016/11/09 22:02:07, miu_OOO_until_14_Nov wrote: > Reading this subclass's usage comments, it seems like a lot of burden is being > placed on the client code that uses this class. Therefore, how about we add: > > using ChangeCallback = Function<void(float), WTF::SameThreadAffinity>; > > ...and the constructor becomes: > > ElementViewportRatioObserver(Element*, std::unique_ptr<ChangeCallback>); > > ...and that way the client code only has to provide a callback that takes a > float argument (the viewport intersection ratio). Done. https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right): https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:217: bool isObservingRatio = m_observer->isObservingElementViewportRatio(); On 2016/11/09 22:02:08, miu_OOO_until_14_Nov wrote: > naming/semantics: IMHO, this shouldn't semantically mean "the observer is > observing the viewport ratio." Instead, the behavior we want is "the observer > wants continuous measurement updates, not just threshold change updates." So, I > think you should delete this LOC, and just make a small change to the > if-statement near the bottom of this method (see later comments). Not needed. Added a subclass to observe element/viewport ratio changes. https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:227: float rootArea = geometry.rootRect.size().width().toFloat() * On 2016/11/09 22:02:07, miu_OOO_until_14_Nov wrote: > Doesn't seem like the math should be different. Won't |geometry.targetRect| be > equal to |geometry.rootRect| for our use case? They are different. The targetRect is the element that is observed (video element in our case), not the viewport. https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:240: if (shouldReportChange) { On 2016/11/09 22:02:07, miu_OOO_until_14_Nov wrote: > IMHO, this change should be: > > if (m_lastThresholdIndex != newThresholdIndex || > m_observer->isObservingContinuousChanges()) { I think we only want to report if the ratio is changed, do we? https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right): https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:196: IntersectionObserver* IntersectionObserver::create( On 2016/11/09 22:02:08, miu_OOO_until_14_Nov wrote: > Get rid of the special-case boolean (see comments below). Then, you don't need > this extra create() function. (The one above can be called with an empty set of > |thresholds|.) Keeps the boolean to indicate observing element/viewport ratio. I am not sure if this create() can be saved though. If the IntersectionObserver always asking for a non-empty thresholds vector, we can use an empty thresholds vector to indicate observing element/viewport ratio, and saved this create(). +ojan https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:294: if (m_observeElementViewportRatio) { On 2016/11/09 22:02:08, miu_OOO_until_14_Nov wrote: > Please get rid of this extra boolean. Instead, the caller of this observe() > method should provide a |target| that is equal to the rootNode(). As discussed offline, the |target| is the video element, and the root is the viewport. keeps the boolean. https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:118: if (m_isObservingVisibilityOfMutedVideoOffscreenDuration && isVisible) On 2016/11/09 22:02:08, miu_OOO_until_14_Nov wrote: > Note: This method is going to be called from HTMLMediaElement repeatedly for the > same |isVisible| value. That never happened before. Therefore, I think you > should add to the top of this method: > > if (isVisible == m_isVisible) > return; > m_isVisible = isVisible; > > ...and update the code in the last if-statement of this method: It doesn't need > the same check, and doesn't need to assign to m_isVisible. Done. https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.h (right): https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:61: void visibilityMightChangedforMutedVideo(bool isVisible); On 2016/11/09 22:02:08, miu_OOO_until_14_Nov wrote: > naming: s/Might/Maybe/ Done. https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:537: void onVideoViewportRatioChanged(bool isVisible); On 2016/11/09 22:02:08, miu_OOO_until_14_Nov wrote: > Per comments in other file, this callback method should take the float "ratio" > argument. Done. https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/2475643004/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebMediaPlayer.h:230: virtual void videoViewportRatioChanged(double ratio) {} On 2016/11/09 22:02:08, miu_OOO_until_14_Nov wrote: > This value was a float type in the intersection/visibility observer classes. > Should this be float here, or do you think we need more precision in the other > code modules? Done. Changed to float.
xjz@chromium.org changed reviewers: + szager@chromium.org
lgtm % some small things: https://codereview.chromium.org/2475643004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ElementVisibilityObserver.h (right): https://codereview.chromium.org/2475643004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ElementVisibilityObserver.h:23: VisibilityObserverBase(Element*); Add 'explicit' keyword. https://codereview.chromium.org/2475643004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ElementVisibilityObserver.h:53: ~ElementVisibilityObserver() override; Since these subclasses are final, should all the 'override' keywords be 'final' instead? https://codereview.chromium.org/2475643004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/IntersectionObserver.h (right): https://codereview.chromium.org/2475643004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObserver.h:115: bool m_observeElementViewportRatio = false; Since this is initialized in the ctor, you don't need the " = false" here.
xjz@chromium.org changed reviewers: + xhwang@chromium.org
Thanks miu@! Addressed comments in PS#6. +xhwang@ for changes on media/base/* and media/blink/*. Thanks! https://codereview.chromium.org/2475643004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ElementVisibilityObserver.h (right): https://codereview.chromium.org/2475643004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ElementVisibilityObserver.h:23: VisibilityObserverBase(Element*); On 2016/11/11 08:18:33, miu_OOO_until_14_Nov wrote: > Add 'explicit' keyword. Done. https://codereview.chromium.org/2475643004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ElementVisibilityObserver.h:53: ~ElementVisibilityObserver() override; On 2016/11/11 08:18:33, miu_OOO_until_14_Nov wrote: > Since these subclasses are final, should all the 'override' keywords be 'final' > instead? Done. https://codereview.chromium.org/2475643004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/IntersectionObserver.h (right): https://codereview.chromium.org/2475643004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObserver.h:115: bool m_observeElementViewportRatio = false; On 2016/11/11 08:18:33, miu_OOO_until_14_Nov wrote: > Since this is initialized in the ctor, you don't need the " = false" here. Done.
Chromium media changes look good. Please get Blink approval first, then I'll take another look. Thanks! https://codereview.chromium.org/2475643004/diff/160001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/2475643004/diff/160001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebMediaPlayer.h:229: // Inform WebMediaPlayer when the video/viewport ratio changed. Please provide more details on what this ratio is.
I had a few new thoughts on this. Some more comments for consideration: https://codereview.chromium.org/2475643004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right): https://codereview.chromium.org/2475643004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:274: if (!geometry.targetRect.isEmpty() && geometry.doesIntersect) { This should check that |rootRect| is not empty instead of |targetRect| (to prevent possible divide-by-zero crashes). https://codereview.chromium.org/2475643004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:282: if (newVisibleRatio != m_lastVisibleRatio) { A second thought on this: Maybe we should send the ratio, even if it has not changed? Our control logic wants to know about any changes in position as well so we don't activate while the video is re-positioning. For example, let's say the video is fully contained within the viewport and fills 90% of it. If the video moves up or down (e.g., user scroll or layout), but stays contained within the viewport, we should not activate remoting until the movement stops. So, consider making this: if (geometry.intersectionRect != m_lastIntersectionRect) { ... Also, thinking aloud: I wonder if the public interface should expose how the video is moving when the ratio is the same? Perhaps, instead of the ratio, we should be sending the intersection rect and the root rect? Or, we could send the relative position and size, computed as: float relativeX = geometry.intersectionRect.x().toFloat() / geometry.rootRect.width().toFloat(); float relativeY = geometry.intersectionRect.y().toFloat() / geometry.rootRect.height().toFloat(); float relativeWidth = geometry.intersectionRect.width().toFloat() / geometry.rootRect.width().toFloat(); float relativeHeight = geometry.intersectionRect.height().toFloat() / geometry.rootRect.height().toFloat(); This would allow us to be more intelligent about the activation decisions in downstream code, if we find out we need to be. What do you think?
mlamouri@chromium.org changed reviewers: + mlamouri@chromium.org, zqzhang@chromium.org
I would zqzhang@ to review the media changes.
AutoplayUmaHelper and HTMLMediaElement lgtm % nits I'm a bit worried whether we are observing too frequently, but szager@ is the right person to answer that https://codereview.chromium.org/2475643004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): https://codereview.chromium.org/2475643004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:8: #include "core/dom/ElementVisibilityObserver.h" nit: remove this include https://codereview.chromium.org/2475643004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.h (right): https://codereview.chromium.org/2475643004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:40: class ElementVisibilityObserver; nit: remove this forward decl https://codereview.chromium.org/2475643004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:93: bool m_isObservingVisibilityOfMutedAutoplayVideo = false; nit: Currently, this is describing more than what we use it for. You can rename it `m_isObservingMutedVideoPlayMethodBecomeVisible`, and change the comment accordingly. https://codereview.chromium.org/2475643004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2475643004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:39: #include "core/dom/ElementVisibilityObserver.h" nit: remove this unused include
xjz@chromium.org changed reviewers: + esprehn@chromium.org
Thanks zqzhang@ and miu@. Addressed your comments in PS#7. PTAL. +esprehn@ for changes on third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp and third_party/WebKit/public/platform/WebMediaPlayer.h. https://codereview.chromium.org/2475643004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right): https://codereview.chromium.org/2475643004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:274: if (!geometry.targetRect.isEmpty() && geometry.doesIntersect) { On 2016/11/14 21:03:33, miu wrote: > This should check that |rootRect| is not empty instead of |targetRect| (to > prevent possible divide-by-zero crashes). Done. https://codereview.chromium.org/2475643004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:282: if (newVisibleRatio != m_lastVisibleRatio) { On 2016/11/14 21:03:33, miu wrote: > A second thought on this: Maybe we should send the ratio, even if it has not > changed? Our control logic wants to know about any changes in position as well > so we don't activate while the video is re-positioning. For example, let's say > the video is fully contained within the viewport and fills 90% of it. If the > video moves up or down (e.g., user scroll or layout), but stays contained within > the viewport, we should not activate remoting until the movement stops. > > So, consider making this: > > if (geometry.intersectionRect != m_lastIntersectionRect) { > ... > > Also, thinking aloud: I wonder if the public interface should expose how the > video is moving when the ratio is the same? Perhaps, instead of the ratio, we > should be sending the intersection rect and the root rect? Or, we could send the > relative position and size, computed as: > > float relativeX = geometry.intersectionRect.x().toFloat() / > geometry.rootRect.width().toFloat(); > float relativeY = geometry.intersectionRect.y().toFloat() / > geometry.rootRect.height().toFloat(); > float relativeWidth = geometry.intersectionRect.width().toFloat() / > geometry.rootRect.width().toFloat(); > float relativeHeight = geometry.intersectionRect.height().toFloat() / > geometry.rootRect.height().toFloat(); > > This would allow us to be more intelligent about the activation decisions in > downstream code, if we find out we need to be. What do you think? Since we don't know the more intelligent logic for now, I changed to report both rootRect and intersectRect. And did a bunch of renaming accordingly. https://codereview.chromium.org/2475643004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): https://codereview.chromium.org/2475643004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:8: #include "core/dom/ElementVisibilityObserver.h" On 2016/11/15 11:29:27, Zhiqiang Zhang wrote: > nit: remove this include Done. https://codereview.chromium.org/2475643004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.h (right): https://codereview.chromium.org/2475643004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:40: class ElementVisibilityObserver; On 2016/11/15 11:29:27, Zhiqiang Zhang wrote: > nit: remove this forward decl Done. https://codereview.chromium.org/2475643004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:93: bool m_isObservingVisibilityOfMutedAutoplayVideo = false; On 2016/11/15 11:29:27, Zhiqiang Zhang wrote: > nit: Currently, this is describing more than what we use it for. > You can rename it `m_isObservingMutedVideoPlayMethodBecomeVisible`, and change > the comment accordingly. Done. https://codereview.chromium.org/2475643004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2475643004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:39: #include "core/dom/ElementVisibilityObserver.h" On 2016/11/15 11:29:27, Zhiqiang Zhang wrote: > nit: remove this unused include This header file is needed. https://codereview.chromium.org/2475643004/diff/160001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/2475643004/diff/160001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebMediaPlayer.h:229: // Inform WebMediaPlayer when the video/viewport ratio changed. On 2016/11/14 18:32:03, xhwang wrote: > Please provide more details on what this ratio is. Done.
Comments on PS7: https://codereview.chromium.org/2475643004/diff/180001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2475643004/diff/180001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:359: intersect_info.root_rect = I think the types have implicit conversion defined. Meaning, you can just: intersect_info.root_rect = info.rootRect; intersect_info.intersect_rect = info.intersectRect; https://codereview.chromium.org/2475643004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right): https://codereview.chromium.org/2475643004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:277: float newVisibleRatio = 0; Please remove |newVisibleRatio| and move the ratio computation to remoting_controller.cc where it is needed. https://codereview.chromium.org/2475643004/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/2475643004/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebMediaPlayer.h:112: float ratio = 0; Please remove |ratio|. This can be computed in remoting_controller.cc where it is needed. (This is what we were talking about earlier today, face-to-face.) For all other cases, where you use ratio > 0.0f to determine visibility, just use !intersectRect.empty() instead.
Addressed comments. PTAL https://codereview.chromium.org/2475643004/diff/180001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2475643004/diff/180001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:359: intersect_info.root_rect = On 2016/11/15 23:29:26, miu wrote: > I think the types have implicit conversion defined. Meaning, you can just: > > intersect_info.root_rect = info.rootRect; > intersect_info.intersect_rect = info.intersectRect; Done. https://codereview.chromium.org/2475643004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right): https://codereview.chromium.org/2475643004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:277: float newVisibleRatio = 0; On 2016/11/15 23:29:26, miu wrote: > Please remove |newVisibleRatio| and move the ratio computation to > remoting_controller.cc where it is needed. Done. https://codereview.chromium.org/2475643004/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/2475643004/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebMediaPlayer.h:112: float ratio = 0; On 2016/11/15 23:29:26, miu wrote: > Please remove |ratio|. This can be computed in remoting_controller.cc where it > is needed. (This is what we were talking about earlier today, face-to-face.) > > For all other cases, where you use ratio > 0.0f to determine visibility, just > use !intersectRect.empty() instead. Done.
PS8 lgtm % small things: https://codereview.chromium.org/2475643004/diff/200001/media/remoting/remotin... File media/remoting/remoting_controller.h (right): https://codereview.chromium.org/2475643004/diff/200001/media/remoting/remotin... media/remoting/remoting_controller.h:119: // Video/Viewport ratio meeting/exceeding the threshold should hold steady for nit: Please add a blank line above this line. https://codereview.chromium.org/2475643004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ElementVisibilityObserver.cpp (right): https://codereview.chromium.org/2475643004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ElementVisibilityObserver.cpp:122: WebRect(entry->rootBounds()->top(), entry->rootBounds()->left(), Please fix: top() and left() are reversed. https://codereview.chromium.org/2475643004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ElementVisibilityObserver.cpp:127: intersectRect = WebRect(entry->intersectionRect()->top(), ditto: top() and left() are reversed. https://codereview.chromium.org/2475643004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/IntersectionObserver.h (right): https://codereview.chromium.org/2475643004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObserver.h:102: bool isObservingIntersection); naming: This is weird because the class name is IntersectionObserver and the last argument is named |isObservingIntersection|. Maybe you meant something like |observeViewportIntersection|? https://codereview.chromium.org/2475643004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/2475643004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:312: WebMediaPlayer::ViewportIntersectionInfo viewportIntersectInfo() const { naming suggestion: How about something more descriptive like currentViewportIntersection() and |m_currentViewportIntersection|?
Thanks miu@! Addressed comments in PS #9. https://codereview.chromium.org/2475643004/diff/200001/media/remoting/remotin... File media/remoting/remoting_controller.h (right): https://codereview.chromium.org/2475643004/diff/200001/media/remoting/remotin... media/remoting/remoting_controller.h:119: // Video/Viewport ratio meeting/exceeding the threshold should hold steady for On 2016/11/16 02:40:26, miu wrote: > nit: Please add a blank line above this line. Done. https://codereview.chromium.org/2475643004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ElementVisibilityObserver.cpp (right): https://codereview.chromium.org/2475643004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ElementVisibilityObserver.cpp:122: WebRect(entry->rootBounds()->top(), entry->rootBounds()->left(), On 2016/11/16 02:40:26, miu wrote: > Please fix: top() and left() are reversed. Done. Thanks for pointing this out! https://codereview.chromium.org/2475643004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ElementVisibilityObserver.cpp:127: intersectRect = WebRect(entry->intersectionRect()->top(), On 2016/11/16 02:40:26, miu wrote: > ditto: top() and left() are reversed. Done. https://codereview.chromium.org/2475643004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/IntersectionObserver.h (right): https://codereview.chromium.org/2475643004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObserver.h:102: bool isObservingIntersection); On 2016/11/16 02:40:26, miu wrote: > naming: This is weird because the class name is IntersectionObserver and the > last argument is named |isObservingIntersection|. Maybe you meant something like > |observeViewportIntersection|? Done. https://codereview.chromium.org/2475643004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/2475643004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:312: WebMediaPlayer::ViewportIntersectionInfo viewportIntersectInfo() const { On 2016/11/16 02:40:26, miu wrote: > naming suggestion: How about something more descriptive like > currentViewportIntersection() and |m_currentViewportIntersection|? Done.
Description was changed from ========== Detect video/viewport ratio change. Main changes in this CL: 1. Adds ElementViewportRatioObserver to monoitor the video/viewport ratio. 2. Consolidates three ElementVisibilityObservers for HTMLMediaElement to one observer. 3. Notifies WebMediaPlayer the video/viewport ratio. 4. Adds logic to start remoting when video covers most of the viewport for a certain time. BUG=643964 ========== to ========== Monitor the intersection of video and viewport. This CL: 1. Adds ViewportIntersectionObserver to monoitor the intersection of an element with viewport. 2. Consolidates three ElementVisibilityObservers for HTMLMediaElement to one observer. 3. Notifies WebMediaPlayer the intersection info when changed. 4. Adds logic to start remoting when video consistently covers most of the viewport for a certain time. BUG=643964 ==========
https://codereview.chromium.org/2475643004/diff/220001/media/remoting/remotin... File media/remoting/remoting_controller.cc (right): https://codereview.chromium.org/2475643004/diff/220001/media/remoting/remotin... media/remoting/remoting_controller.cc:171: if (ratio < 0.85) { Magic number! Please declare a double kViewportRatioVideoRenderingThreshold, or some such thing. https://codereview.chromium.org/2475643004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right): https://codereview.chromium.org/2475643004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:296: } else if (target->document() == rootNode()->document()) { Please push the logic for determining shouldReportRootBounds out to all callers. It's very weird to have logic here that overrides the constructor argument. Maybe add a static method to IntersectionObserver: static bool hierarchyAllowsRootBoundsReporting(Node& root, Element& target); ... which contains all of the previous logic for calculating shouldReportRootBounds. Then, change all of the callers to: IntersectionObserver::create(..., IntersectionObserver::hierarchyAllowsRootBoundsReporting(root, target)); https://codereview.chromium.org/2475643004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:307: if (m_observeViewportIntersection) { Why is this 'if' clause necessary? The value of shouldReportRootBounds should be accurate in all cases. https://codereview.chromium.org/2475643004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/IntersectionObserver.h (right): https://codereview.chromium.org/2475643004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObserver.h:54: static IntersectionObserver* create(const Vector<Length>& rootMargin, This constructor is unnecessary. Just pass an empty thresholds vector to the existing ::create().
Addressed szager@'s comments in PS #10. PTAL. Thanks! https://codereview.chromium.org/2475643004/diff/220001/media/remoting/remotin... File media/remoting/remoting_controller.cc (right): https://codereview.chromium.org/2475643004/diff/220001/media/remoting/remotin... media/remoting/remoting_controller.cc:171: if (ratio < 0.85) { On 2016/11/16 17:53:54, szager1 wrote: > Magic number! Please declare a double kViewportRatioVideoRenderingThreshold, or > some such thing. Done. https://codereview.chromium.org/2475643004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right): https://codereview.chromium.org/2475643004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:296: } else if (target->document() == rootNode()->document()) { On 2016/11/16 17:53:54, szager1 wrote: > Please push the logic for determining shouldReportRootBounds out to all callers. > It's very weird to have logic here that overrides the constructor argument. > > Maybe add a static method to IntersectionObserver: > > static bool hierarchyAllowsRootBoundsReporting(Node& root, Element& target); > > ... which contains all of the previous logic for calculating > shouldReportRootBounds. Then, change all of the callers to: > > IntersectionObserver::create(..., > IntersectionObserver::hierarchyAllowsRootBoundsReporting(root, target)); Sorry, I didn't quite get your comment "It's very weird to have logic here that overrides the constructor argument". IIUC, |shouldReportRootBounds| is a parameter for |IntersectionObservation|, which is created below. And the parameter is for some optimization. In the case that observing the intersection with viewport, we always need this info, so we set it true. IMHO, it is totally fine to put the logic here just before creating the |IntersectionObservation|. WDYT? https://codereview.chromium.org/2475643004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:307: if (m_observeViewportIntersection) { On 2016/11/16 17:53:54, szager1 wrote: > Why is this 'if' clause necessary? The value of shouldReportRootBounds should > be accurate in all cases. This 'if' clause here is to create the ViewportIntersectionObservation if observing the intersection with the viewport. Two difference with the normal(orginal) IntersectionObservation. 1) No ratio is computed. Therefore IntersectionObserverEntry::intersectionRatio() is meaningless. 2) No thresholds is used. Any change on intersection rect will be reported through IntersectionObserverEntry::rootBounds() and IntersectionObserverEntry::intersectionRect(). https://codereview.chromium.org/2475643004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/IntersectionObserver.h (right): https://codereview.chromium.org/2475643004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObserver.h:54: static IntersectionObserver* create(const Vector<Length>& rootMargin, On 2016/11/16 17:53:54, szager1 wrote: > This constructor is unnecessary. Just pass an empty thresholds vector to the > existing ::create(). Done.
Why is this path changing any of the code for intersection observer? How does JS use this feature? What spec change is being proposed?
On 2016/11/16 20:47:18, esprehn wrote: > Why is this path changing any of the code for intersection observer? How does JS > use this feature? What spec change is being proposed? This CL adds a new ViewportIntersectionObserver to monitor the intersection of video with viewport. It doesn't change any original logic in InterSectionObserver, but just reuse some codes for this purpose, as suggested by ojan@ and szager@. +ojan@ +szager@ The change does not affect JS using the IntersectionObserver.
https://codereview.chromium.org/2475643004/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right): https://codereview.chromium.org/2475643004/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:273: if (m_observeViewportIntersection) { Hmm... you're using m_observerViewportIntersection as a kind of hacky alternative to sub-classing IntersectionObserver. I think you should just sub-class IntersectionObserver. Something like: class IntersectionObserver { ... protected: virtual IntersectionObservation* createObservation(Element& target, bool shouldReportRootBounds) final { return new IntersectionObservation(*this, target, shouldReportRootBounds); } } class ViewportIntersectionObserver : public IntersectionObserver { protected: IntersectionObservation* createObservation(Element& target, bool) { return new ViewportIntersectionObservation(*this, target); } } void IntersectionObserver::observe(...) { ... IntersectionObservation* observation = createObservation(*target, shouldReportRootBounds); ... }
On 2016/11/16 21:37:56, szager1 wrote: > https://codereview.chromium.org/2475643004/diff/240001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right): > > https://codereview.chromium.org/2475643004/diff/240001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:273: if > (m_observeViewportIntersection) { > Hmm... you're using m_observerViewportIntersection as a kind of hacky > alternative to sub-classing IntersectionObserver. I think you should just > sub-class IntersectionObserver. Something like: > > class IntersectionObserver { > ... > protected: > virtual IntersectionObservation* createObservation(Element& target, bool > shouldReportRootBounds) final { > return new IntersectionObservation(*this, target, shouldReportRootBounds); > } > } > > class ViewportIntersectionObserver : public IntersectionObserver { > protected: > IntersectionObservation* createObservation(Element& target, bool) { > return new ViewportIntersectionObservation(*this, target); > } > } > > void IntersectionObserver::observe(...) { > ... > IntersectionObservation* observation = createObservation(*target, > shouldReportRootBounds); > ... > } I put "final" in the wrong place, but hopefully this is clear.
Please don't subclass it, that's not something that a JS developer would do either. You want a wrapper just like ElementVisibilityObserver. Think about yourself like a web developer. What would you do?
On 2016/11/16 22:04:24, esprehn wrote: > Please don't subclass it, that's not something that a JS developer would do > either. You want a wrapper just like ElementVisibilityObserver. > > Think about yourself like a web developer. What would you do? I advised subclassing IntersectionObservation because they need custom logic in IntersectionObservation::computeIntersectionObservations. Without the custom logic, they probably couldn't use IntersectionObserver at all.
On 2016/11/16 at 22:15:12, szager wrote: > On 2016/11/16 22:04:24, esprehn wrote: > > Please don't subclass it, that's not something that a JS developer would do > > either. You want a wrapper just like ElementVisibilityObserver. > > > > Think about yourself like a web developer. What would you do? > > I advised subclassing IntersectionObservation because they need custom logic in IntersectionObservation::computeIntersectionObservations. Without the custom logic, they probably couldn't use IntersectionObserver at all. That sounds like we need to change the spec then? I dont think we should be adding back doors and it's not clear why this feature is different than what a web developer would want to do themselves.
On 2016/11/16 22:19:21, esprehn wrote: > On 2016/11/16 at 22:15:12, szager wrote: > > On 2016/11/16 22:04:24, esprehn wrote: > > > Please don't subclass it, that's not something that a JS developer would do > > > either. You want a wrapper just like ElementVisibilityObserver. > > > > > > Think about yourself like a web developer. What would you do? > > > > I advised subclassing IntersectionObservation because they need custom logic > in IntersectionObservation::computeIntersectionObservations. Without the custom > logic, they probably couldn't use IntersectionObserver at all. > > That sounds like we need to change the spec then? I dont think we should be > adding back doors and it's not clear why this feature is different than what a > web developer would want to do themselves. esprehn: You're mistaken. None of this is exposed to JS or the rest of the web platform. This new information is only being passed back to the embedder, to make control decisions.
On 2016/11/16 22:19:21, esprehn wrote: > On 2016/11/16 at 22:15:12, szager wrote: > > On 2016/11/16 22:04:24, esprehn wrote: > > > Please don't subclass it, that's not something that a JS developer would do > > > either. You want a wrapper just like ElementVisibilityObserver. > > > > > > Think about yourself like a web developer. What would you do? > > > > I advised subclassing IntersectionObservation because they need custom logic > in IntersectionObservation::computeIntersectionObservations. Without the custom > logic, they probably couldn't use IntersectionObserver at all. > > That sounds like we need to change the spec then? I dont think we should be > adding back doors and it's not clear why this feature is different than what a > web developer would want to do themselves. I don't agree with this. This code runs on every frame; if we opened it up to be customized in js, then we'd have to run js on every frame. It's also not clear to me why this feature ought to adhere to what is possible for web developers to do.
esprehn: I just forwarded you the e-mail discussion that led to this, so you have the background info. :)
Thanks szager@! Addressed your comments in PS #11. PTAL https://codereview.chromium.org/2475643004/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right): https://codereview.chromium.org/2475643004/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:273: if (m_observeViewportIntersection) { On 2016/11/16 21:37:55, szager1 wrote: > Hmm... you're using m_observerViewportIntersection as a kind of hacky > alternative to sub-classing IntersectionObserver. I think you should just > sub-class IntersectionObserver. Something like: > > class IntersectionObserver { > ... > protected: > virtual IntersectionObservation* createObservation(Element& target, bool > shouldReportRootBounds) final { > return new IntersectionObservation(*this, target, shouldReportRootBounds); > } > } > > class ViewportIntersectionObserver : public IntersectionObserver { > protected: > IntersectionObservation* createObservation(Element& target, bool) { > return new ViewportIntersectionObservation(*this, target); > } > } > > void IntersectionObserver::observe(...) { > ... > IntersectionObservation* observation = createObservation(*target, > shouldReportRootBounds); > ... > } Done.
ojan@google.com changed reviewers: + ojan@google.com
So sorry for my delay getting back to this. This patch looks great overall. Thanks especially for cleaning up the multiple instances of ElementVisibilityObserver so we only have one per video. I think we're good to go if we can resolve the subclassing/modifying of IntersectionObserver issue. I think we can meet your needs without doing so. Specifically, as you noted, the only thing you don't get from the ElementVisibilityObserver that you need is the intersection rect and the root rect. I think you can get what you need with roughly the following: 1. Change ElementVisibilityObserver::onVisibilityChanged to pass the intersectionRect off the IntersectionObserverEntry as well as the isVisible bool to the callback on line 78: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Eleme... 2. In HTMLMediaElement::onVideoViewportIntersectionChanged, do what you're doing in this patch, except compute the rootRect there instead of getting it from the IntersectionObserver callback. I think HTMLMediaElement has access to the rootRect via document().frameHost()->visualViewport().visibleRect(). That last bit of code is a little new, so I'm not 100% sure that's right. kenrb@ or wjmaclean@ can you comment on whether visibleRect is the right way to get at the rect for the root viewport for the tab? Again, sorry for the delayed response. I'm in meetings all day tomorrow, but will try my best to be responsive. https://codereview.chromium.org/2475643004/diff/260001/media/remoting/remotin... File media/remoting/remoting_controller.cc (right): https://codereview.chromium.org/2475643004/diff/260001/media/remoting/remotin... media/remoting/remoting_controller.cc:194: is_mostly_filling_viewport_ = true; If the video is mostly filling the viewport, then 2 seconds later it's not, we'll set is_mostly_filling_viewport_ to false on line 177, but then hit this callback and set it to true. I think that's not what you're intending? Or am I reading the code wrong or is that how it's supposed to work? I think you want to set is_mostly_filling_viewport_ to true when you schedule the callback (i.e. on line 186). Then this function would do: if (is_mostly_filling_viewport) UpdateAndMaybeSwitch();
https://codereview.chromium.org/2475643004/diff/260001/media/remoting/remotin... File media/remoting/remoting_controller.cc (right): https://codereview.chromium.org/2475643004/diff/260001/media/remoting/remotin... media/remoting/remoting_controller.cc:194: is_mostly_filling_viewport_ = true; On 2016/11/17 06:19:05, Z_DONOTUSE wrote: > If the video is mostly filling the viewport, then 2 seconds later it's not, > we'll set is_mostly_filling_viewport_ to false on line 177, but then hit this > callback and set it to true. I think that's not what you're intending? Or am I > reading the code wrong or is that how it's supposed to work? > > I think you want to set is_mostly_filling_viewport_ to true when you schedule > the callback (i.e. on line 186). Then this function would do: > > if (is_mostly_filling_viewport) > UpdateAndMaybeSwitch(); Before setting |is_mostly_filling_viewport_| to false on Line 177, the timer is stopped in Line 171, and this callback will not be hit.
Thanks ojan! I agree with you that we could get intersectionRect/rootRect without modifying the intersectionObserver if we can get the notification. However, as miu@ pointed out in the email thread, we can not directly use ElementVisibilityObserver/IntersectionObserver because the following two issues: 1. ElementVisibilityObserver/IntersectionObserver monitors the fraction of the video that is visible. For example, even if the video only covers 1/4 of the viewport, it will report 1 if the whole video is visible. So we will not get notified when video is enlarged to fill in most of the viewport since the ratio is not changed. 2. ElementVisibilityObserver/IntersectionObserver is per-threshold notification, but we want continuous updates to use some "debouncing logic" to be sure that size/layout changes are not in-progress. On 2016/11/17 06:19:06, Z_DONOTUSE wrote: > So sorry for my delay getting back to this. > > This patch looks great overall. Thanks especially for cleaning up the multiple > instances of ElementVisibilityObserver so we only have one per video. > > I think we're good to go if we can resolve the subclassing/modifying of > IntersectionObserver issue. I think we can meet your needs without doing so. > Specifically, as you noted, the only thing you don't get from the > ElementVisibilityObserver that you need is the intersection rect and the root > rect. > > I think you can get what you need with roughly the following: > > 1. Change ElementVisibilityObserver::onVisibilityChanged to pass the > intersectionRect off the IntersectionObserverEntry as well as the isVisible bool > to the callback on line 78: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Eleme... > > > 2. In HTMLMediaElement::onVideoViewportIntersectionChanged, do what you're doing > in this patch, except compute the rootRect there instead of getting it from the > IntersectionObserver callback. I think HTMLMediaElement has access to the > rootRect via document().frameHost()->visualViewport().visibleRect(). That last > bit of code is a little new, so I'm not 100% sure that's right. > > kenrb@ or wjmaclean@ can you comment on whether visibleRect is the right way to > get at the rect for the root viewport for the tab? > > Again, sorry for the delayed response. I'm in meetings all day tomorrow, but > will try my best to be responsive. > > https://codereview.chromium.org/2475643004/diff/260001/media/remoting/remotin... > File media/remoting/remoting_controller.cc (right): > > https://codereview.chromium.org/2475643004/diff/260001/media/remoting/remotin... > media/remoting/remoting_controller.cc:194: is_mostly_filling_viewport_ = true; > If the video is mostly filling the viewport, then 2 seconds later it's not, > we'll set is_mostly_filling_viewport_ to false on line 177, but then hit this > callback and set it to true. I think that's not what you're intending? Or am I > reading the code wrong or is that how it's supposed to work? > > I think you want to set is_mostly_filling_viewport_ to true when you schedule > the callback (i.e. on line 186). Then this function would do: > > if (is_mostly_filling_viewport) > UpdateAndMaybeSwitch();
The CQ bit was checked by xjz@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...
That sounds like maybe you want to use a ResizeObserver too? Maybe we need an ElementResizeObserver in C++ that wraps a ResizeObserver and you want to compose those two features. Note that I wasn't saying your change is observable to JS, what I'm saying is that IntersectionObserver itself is a DOM API. It shouldn't implement backdoors for C++. If this feature needs to know something new, then we should discuss what's missing from the IntersectionObserver spec. It's very important that we avoid adding back doors and make sure that whatever tools we're using internally we're exposing to web developers. A web developer or extension author building a video player needs the same tools you do. :)
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_...)
On 2016/11/17 07:38:29, esprehn wrote: > That sounds like maybe you want to use a ResizeObserver too? Maybe we need an > ElementResizeObserver in C++ that wraps a ResizeObserver and you want to compose > those two features. This probably wouldn't work because it doesn't track changes to the size of the viewport. > Note that I wasn't saying your change is observable to JS, what I'm saying is > that IntersectionObserver itself is a DOM API. It shouldn't implement backdoors > for C++. If this feature needs to know something new, then we should discuss > what's missing from the IntersectionObserver spec. It's very important that we > avoid adding back doors and make sure that whatever tools we're using internally > we're exposing to web developers. A web developer or extension author building a > video player needs the same tools you do. :) One way that we could avoid sub-classing IO is to add an option to IO which interprets "threshold" as the intersetion/root ratio rather than the intersection/target ratio. If that were the case, then this patch could use a regular IO instance with thresholds=[0.85]. This feels like a pretty obscure option; it has never been raised on the spec issue tracker for IntersectionObserver, and it might get a cold reception from other browsers who have not yet shipped IO. However, we could only expose an option like that in IntersectionObserver if the root and target are same-origin. For the feature being implemented here, the same-origin requirement would have to be bypassed. So if we accept that this feature requires capabilities that are not -- and should not -- be web-exposed, then I would argue that sub-classing IO is better than duplicating all of its logic.
The CQ bit was checked by xjz@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...
On 2016/11/17 15:43:51, szager1 wrote: > On 2016/11/17 07:38:29, esprehn wrote: > > That sounds like maybe you want to use a ResizeObserver too? Maybe we need an > > ElementResizeObserver in C++ that wraps a ResizeObserver and you want to > compose > > those two features. > > This probably wouldn't work because it doesn't track changes to the size of the > viewport. > > > Note that I wasn't saying your change is observable to JS, what I'm saying is > > that IntersectionObserver itself is a DOM API. It shouldn't implement > backdoors > > for C++. If this feature needs to know something new, then we should discuss > > what's missing from the IntersectionObserver spec. It's very important that we > > avoid adding back doors and make sure that whatever tools we're using > internally > > we're exposing to web developers. A web developer or extension author building > a > > video player needs the same tools you do. :) > > One way that we could avoid sub-classing IO is to add an option to IO which > interprets "threshold" as the intersetion/root ratio rather than the > intersection/target ratio. If that were the case, then this patch could use a > regular IO instance with thresholds=[0.85]. This feels like a pretty obscure > option; it has never been raised on the spec issue tracker for > IntersectionObserver, and it might get a cold reception from other browsers who > have not yet shipped IO. Even this works, we still can't get continuous updates, but just per-threshold notification. +miu@ > > However, we could only expose an option like that in IntersectionObserver if the > root and target are same-origin. For the feature being implemented here, the > same-origin requirement would have to be bypassed. > > So if we accept that this feature requires capabilities that are not -- and > should not -- be web-exposed, then I would argue that sub-classing IO is better > than duplicating all of its logic. +1 ojan@ and esprehn@, WDYT?
On 2016/11/17 at 18:55:42, xjz wrote: > On 2016/11/17 15:43:51, szager1 wrote: > > On 2016/11/17 07:38:29, esprehn wrote: > > > That sounds like maybe you want to use a ResizeObserver too? Maybe we need an > > > ElementResizeObserver in C++ that wraps a ResizeObserver and you want to > > compose > > > those two features. > > > > This probably wouldn't work because it doesn't track changes to the size of the > > viewport. > > > > > Note that I wasn't saying your change is observable to JS, what I'm saying is > > > that IntersectionObserver itself is a DOM API. It shouldn't implement > > backdoors > > > for C++. If this feature needs to know something new, then we should discuss > > > what's missing from the IntersectionObserver spec. It's very important that we > > > avoid adding back doors and make sure that whatever tools we're using > > internally > > > we're exposing to web developers. A web developer or extension author building > > a > > > video player needs the same tools you do. :) > > > > One way that we could avoid sub-classing IO is to add an option to IO which > > interprets "threshold" as the intersetion/root ratio rather than the > > intersection/target ratio. If that were the case, then this patch could use a > > regular IO instance with thresholds=[0.85]. This feels like a pretty obscure > > option; it has never been raised on the spec issue tracker for > > IntersectionObserver, and it might get a cold reception from other browsers who > > have not yet shipped IO. > > Even this works, we still can't get continuous updates, but just per-threshold notification. Do you need continuous updates? If I'm reading the code right, what you care about is if the video has stayed past the 0.85 viewport threshold for the last 5 seconds. So, assuming you know the sizes of the video and the viewport, you only need a single threshold and to monitor when it exceeds or doesn't. szager, esprehn, and I just had a quick hallway discussion and I have a proposal that I think works. Please let me know if it doesn't. The important thing is that it relies on the same things we'd eventually like to expose to web authors. That makes it so that the code is self-maintaining because we *have* to support, and thoroughly-test, the web exposed API regardless of what we do with other code. So it's less maintenance work for us, but also your feature won't unexpectedly break out from under you. I think the missing thing here is the ability to dynamically modify the threshold list. This is a feature we've gotten requests for from web authors as well. If you know the size of the video and the size of the viewport, then you can pick the right threshold value to know when the video covers 85% of the viewport, right? So, concretely: 1. Add the ability to dynamically modify the thresholds list to IntersectionObserver and ElementVisibilityObserver. There's two choices for how you could do this: a) We will eventually expose this to web authors, but of course you don't need to do that work. For this patch, you can stick to just the C++ classes. The implementation would be to add a setThresholds method that takes the new thresholds and calls scheduleAnimation to get the rendering pipeline to run again. b) If option A sounds like too much work, you could also instead just add the ability to pass a threshold to ElementVisibilityObserver and recreate the ElementVisbilityObserver whenever the threshold changes. 2. You can already get the size of the viewport via the visualViewport as I suggested above. There's also mainFrameDidChangeSize() so you can see when the size changed. Note: We need to verify that these APIs work as I think they do with the out of process iframe folks I CC'ed above. There's certainly a way to get the root viewport's size, I'm just not up to date on it since out of process iframes changed things. I'll IM them to get them to pay attention to this review. :) 3. You can get notified when the video changes size by setting up a ResizeObserver. I don't think we have other examples of doing this from C++ yet, but I don't think this should be particularly hard. It will be similar in spirit to ElementVisibilityObserver. Hopefully this isn't asking too much. If you have concerns with this approach, it might be easiest to just have a VC meeting so we can avoid delaying you with lots of email back-and-forth.
On 2016/11/17 at 20:21:08, ojan wrote: > On 2016/11/17 at 18:55:42, xjz wrote: > > On 2016/11/17 15:43:51, szager1 wrote: > > > On 2016/11/17 07:38:29, esprehn wrote: > > > > That sounds like maybe you want to use a ResizeObserver too? Maybe we need an > > > > ElementResizeObserver in C++ that wraps a ResizeObserver and you want to > > > compose > > > > those two features. > > > > > > This probably wouldn't work because it doesn't track changes to the size of the > > > viewport. > > > > > > > Note that I wasn't saying your change is observable to JS, what I'm saying is > > > > that IntersectionObserver itself is a DOM API. It shouldn't implement > > > backdoors > > > > for C++. If this feature needs to know something new, then we should discuss > > > > what's missing from the IntersectionObserver spec. It's very important that we > > > > avoid adding back doors and make sure that whatever tools we're using > > > internally > > > > we're exposing to web developers. A web developer or extension author building > > > a > > > > video player needs the same tools you do. :) > > > > > > One way that we could avoid sub-classing IO is to add an option to IO which > > > interprets "threshold" as the intersetion/root ratio rather than the > > > intersection/target ratio. If that were the case, then this patch could use a > > > regular IO instance with thresholds=[0.85]. This feels like a pretty obscure > > > option; it has never been raised on the spec issue tracker for > > > IntersectionObserver, and it might get a cold reception from other browsers who > > > have not yet shipped IO. > > > > Even this works, we still can't get continuous updates, but just per-threshold notification. > > Do you need continuous updates? If I'm reading the code right, what you care about is if the video has stayed past the 0.85 viewport threshold for the last 5 seconds. So, assuming you know the sizes of the video and the viewport, you only need a single threshold and to monitor when it exceeds or doesn't. > > szager, esprehn, and I just had a quick hallway discussion and I have a proposal that I think works. Please let me know if it doesn't. The important thing is that it relies on the same things we'd eventually like to expose to web authors. That makes it so that the code is self-maintaining because we *have* to support, and thoroughly-test, the web exposed API regardless of what we do with other code. So it's less maintenance work for us, but also your feature won't unexpectedly break out from under you. > > I think the missing thing here is the ability to dynamically modify the threshold list. This is a feature we've gotten requests for from web authors as well. If you know the size of the video and the size of the viewport, then you can pick the right threshold value to know when the video covers 85% of the viewport, right? > > So, concretely: > 1. Add the ability to dynamically modify the thresholds list to IntersectionObserver and ElementVisibilityObserver. There's two choices for how you could do this: > a) We will eventually expose this to web authors, but of course you don't need to do that work. For this patch, you can stick to just the C++ classes. The implementation would be to add a setThresholds method that takes the new thresholds and calls scheduleAnimation to get the rendering pipeline to run again. > b) If option A sounds like too much work, you could also instead just add the ability to pass a threshold to ElementVisibilityObserver and recreate the ElementVisbilityObserver whenever the threshold changes. > > 2. You can already get the size of the viewport via the visualViewport as I suggested above. There's also mainFrameDidChangeSize() so you can see when the size changed. Note: We need to verify that these APIs work as I think they do with the out of process iframe folks I CC'ed above. There's certainly a way to get the root viewport's size, I'm just not up to date on it since out of process iframes changed things. I'll IM them to get them to pay attention to this review. :) Incidentally, this step isn't new to this proposal. Your existing patch also won't work with out of process iframes because it'll get the wrong root size. kenrb is taking a look now at what's exposed to use. > 3. You can get notified when the video changes size by setting up a ResizeObserver. I don't think we have other examples of doing this from C++ yet, but I don't think this should be particularly hard. It will be similar in spirit to ElementVisibilityObserver. > > Hopefully this isn't asking too much. If you have concerns with this approach, it might be easiest to just have a VC meeting so we can avoid delaying you with lots of email back-and-forth.
On 2016/11/17 06:19:06, Z_DONOTUSE wrote: > 2. In HTMLMediaElement::onVideoViewportIntersectionChanged, do what you're doing > in this patch, except compute the rootRect there instead of getting it from the > IntersectionObserver callback. I think HTMLMediaElement has access to the > rootRect via document().frameHost()->visualViewport().visibleRect(). That last > bit of code is a little new, so I'm not 100% sure that's right. > > kenrb@ or wjmaclean@ can you comment on whether visibleRect is the right way to > get at the rect for the root viewport for the tab? > There isn't a lot of information about the main frame in subframe processes, but the size on visualViewport should correctly contain the main frame's size. I just verified in a debugger, since relevant code has been subject to refactoring as of late. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebFrameWi... The scroll offset is always (0, 0), so there is no location info there, but it sounds like that is okay. We had to populate the VisualViewport::m_size in subframe processes because of cases where a remote main frame gets swapped to local, it needs to know its viewport size for layout and doesn't necessarily get a resize from the browser at that time.
On 2016/11/17 20:21:08, ojan wrote: > On 2016/11/17 at 18:55:42, xjz wrote: > > Even this works, we still can't get continuous updates, but just per-threshold > notification. > > Do you need continuous updates? If I'm reading the code right, what you care > about is if the video has stayed past the 0.85 viewport threshold for the last 5 > seconds. So, assuming you know the sizes of the video and the viewport, you only > need a single threshold and to monitor when it exceeds or doesn't. That's not the behavior we want. We're not just interested in an 85% threshold, but also whether the video is still somehow moving/sizing at all. We do not want to activate until the position/size of the video w.r.t. the viewport is unchanging for 5 seconds. To be clear, if we are consistently above the 85% threshold while the video is still moving/sizing, we do not want to activate. That's why we want all updates, not just threshold updates. Another concern we have is whether we will want to adjust this logic in the future, after UX testing and user feedback. We feel it makes the most sense for the decision-making code to be closer to the control logic for this feature, rather than it existing in Blink where others may start to depend on it, making it hard to change in the future. That said, if you feel any part of this would be useful for the web platform as a whole, then I agree it would be reasonable to make this available to the web platform. The only issue, then, is to formulate a sufficiently generic solution, with all the right "knobs" available for behavior tuning.
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_...)
On 2016/11/17 21:07:17, miu wrote: > On 2016/11/17 20:21:08, ojan wrote: > > On 2016/11/17 at 18:55:42, xjz wrote: > > > Even this works, we still can't get continuous updates, but just > per-threshold > > notification. > > > > Do you need continuous updates? If I'm reading the code right, what you care > > about is if the video has stayed past the 0.85 viewport threshold for the last > 5 > > seconds. So, assuming you know the sizes of the video and the viewport, you > only > > need a single threshold and to monitor when it exceeds or doesn't. > > That's not the behavior we want. We're not just interested in an 85% threshold, > but also whether the video is still somehow moving/sizing at all. We do not want > to activate until the position/size of the video w.r.t. the viewport is > unchanging for 5 seconds. To be clear, if we are consistently above the 85% > threshold while the video is still moving/sizing, we do not want to activate. > That's why we want all updates, not just threshold updates. > > Another concern we have is whether we will want to adjust this logic in the > future, after UX testing and user feedback. We feel it makes the most sense for > the decision-making code to be closer to the control logic for this feature, > rather than it existing in Blink where others may start to depend on it, making > it hard to change in the future. > > That said, if you feel any part of this would be useful for the web platform as > a whole, then I agree it would be reasonable to make this available to the web > platform. The only issue, then, is to formulate a sufficiently generic solution, > with all the right "knobs" available for behavior tuning. Close this issue now as discussed on VC. Opened https://codereview.chromium.org/2511143006/ for new approach. |
