Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2511143006:
Detect change on the intersection of video and viewport. (Closed)
|
Created:
4 years, 1 month ago by xjz Modified:
4 years ago CC:
apacible+watch_chromium.org, 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, kinuko+watch, miu+watch_chromium.org, mlamouri+watch-blink_chromium.org, rwlbuis, sof, nessy, Srirama, vcarbune.chromium, xjz+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDetect change on the intersection of video and viewport.
This Cl:
1. Abstracts the calculation of intersection geometry from
IntersectionObserver. Calculating algorithms keep unchanged.
2. When a media is playing, periodically checks whether the
intersection with viewport has changed. If changed, notifies
WebMediaPlayerImpl.
3. Adds logic to start remoting when video consistently covers most
of the viewport for a certain time.
BUG=643964
Committed: https://crrev.com/cdbbe7359047bf173cff5bff708b59efb889862c
Cr-Commit-Position: refs/heads/master@{#436166}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Addressed miu's comments. #
Total comments: 16
Patch Set 3 : Addressed comments from PS#2. #
Total comments: 16
Patch Set 4 : Addressed ojan's comments. #
Total comments: 4
Patch Set 5 : Addressed ojan's comments. Added tests. #Patch Set 6 : Addressed comments. #
Total comments: 12
Patch Set 7 : Addressed comments. Added two more tests. #Patch Set 8 : Rename. #Patch Set 9 : Rebased only. #Patch Set 10 : Bug fix. #Patch Set 11 : Rebase again. #Messages
Total messages: 66 (29 generated)
Description was changed from ========== Detect change on the intersection of video and viewport. This Cl: 1. Abstracts the calculation of intersection geometry from IntersectionObserver. Calculating algorithms keep unchanged. 2. When a media is playing, periodically checks whether the intersection with viewport has changed. If changed, notifies WebMediaPlayerImpl. 3. Adds logic to start remoting when video consistently covers most of the viewport for a certain time. BUG=643964 ========== to ========== Detect change on the intersection of video and viewport. This Cl: 1. Abstracts the calculation of intersection geometry from IntersectionObserver. Calculating algorithms keep unchanged. 2. When a media is playing, periodically checks whether the intersection with viewport has changed. If changed, notifies WebMediaPlayerImpl. 3. Adds logic to start remoting when video consistently covers most of the viewport for a certain time. BUG=643964 ==========
xjz@chromium.org changed reviewers: + esprehn@chromium.org, miu@chromium.org, ojan@chromium.org, szager@chromium.org
xjz@chromium.org changed reviewers: + xhwang@chromium.org
PTAL. Thanks! media/* is not changed from original CL: https://codereview.chromium.org/2475643004/.
I focused on media/*, HTMLMediaElement.cpp and FrameLoaderClientImpl.cpp. I only glanced at the other changes (szager1@ et. al. will take a close look there). Comments on PS1: https://codereview.chromium.org/2511143006/diff/1/media/base/media_observer.h File media/base/media_observer.h (right): https://codereview.chromium.org/2511143006/diff/1/media/base/media_observer.h... media/base/media_observer.h:33: virtual void OnVideoViewportIntersectionChanged( naming nit: Just OnViewportIntersectionChanged (no "video" at front) because this could be called by any type of media element (not just video). (and please adjust code comment) https://codereview.chromium.org/2511143006/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2511143006/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.h:186: void videoViewportIntersectionChanged( ditto: remove "video" from name. https://codereview.chromium.org/2511143006/diff/1/media/remoting/remoting_ren... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2511143006/diff/1/media/remoting/remoting_ren... media/remoting/remoting_renderer_controller.cc:16: constexpr float kViewportRatioVideoRenderingThreshold = 0.85f; naming accuracy: How about kViewportFillActivationThreshold? https://codereview.chromium.org/2511143006/diff/1/media/remoting/remoting_ren... media/remoting/remoting_renderer_controller.cc:23: float intersect_area = static_cast<float>(intersect_rect.width()) * To simplify: Instead of width*height, you can call GetArea() on these rects. https://codereview.chromium.org/2511143006/diff/1/media/remoting/remoting_ren... media/remoting/remoting_renderer_controller.cc:93: if (ratio < kViewportRatioVideoRenderingThreshold) { To simplify: You can avoid both the floating-point math and the separate helper needed for a divide-by-zero check by doing a little algebra here (to devise a new conditional expression): ratio < 0.85 --> (intersect_rect.GetArea() / root_rect.GetArea()) < (85 / 100) --> (100 * intersect_rect.GetArea()) < (85 * root_rect.GetArea()) Now, even if either area is zero, the expression evaluates to the right answer: constexpr int64_t kOneHundredPercent = 100; constexpr int64_t kFillActivationThresholdPercent = 85; if ((kOneHundredPercent * info.intersect_rect.GetArea()) < (kFillActivationThresholdPercent * info.root_rect.GetArea()) { ... https://codereview.chromium.org/2511143006/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/IntersectionGeometry.cpp (right): https://codereview.chromium.org/2511143006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/IntersectionGeometry.cpp:1: // Copyright 2016 The Chromium Authors. All rights reserved. FWICT, most of this code is moved from IntersectionObservation.cpp, and is not new code. Can you adjust your git similarity setting and re-upload so we can see what actually has changed? (`git cl upload --similarity=20` might do the trick, but you can test this with `git diff -M20% --stat origin/master`) https://codereview.chromium.org/2511143006/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2511143006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2531: if (m_webMediaPlayer) { There's an early return from this method about 6-8 lines above. But we want this to always execute. So, please move this up (perhaps to the very top of this method?). https://codereview.chromium.org/2511143006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2539: WebRect(geometry.intersectionRect().x().rawValue(), This translation between rectangle data types is a functional detail that belongs in IntersectionGeometry more than it does here in HTMLMediaElement. Can you change intersectionRect() and rootRect() to return a WebRect? If not, maybe you could create two accessor methods in IntersectionGeometry: intersectionWebRect() and rootWebRect()? https://codereview.chromium.org/2511143006/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right): https://codereview.chromium.org/2511143006/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:783: WebMediaPlayer::ViewportIntersectionInfo intersectInfo = It seems that this should be unconditional. Just: mediaPlayer->viewportIntersectionChanged( htmlMediaElement.currentViewportIntersection());
Thanks miu@! Addressed your comments in PS# 2. PTAL. szager@/ojan@: Can you PTAL the changes on IntersectionGeometry? There should be no logic change, but just abstracts related codes from IntersectionObservation and IntersectionObserver. Thanks! https://codereview.chromium.org/2511143006/diff/1/media/base/media_observer.h File media/base/media_observer.h (right): https://codereview.chromium.org/2511143006/diff/1/media/base/media_observer.h... media/base/media_observer.h:33: virtual void OnVideoViewportIntersectionChanged( On 2016/11/21 21:07:03, miu wrote: > naming nit: Just OnViewportIntersectionChanged (no "video" at front) because > this could be called by any type of media element (not just video). (and please > adjust code comment) Done. https://codereview.chromium.org/2511143006/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2511143006/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.h:186: void videoViewportIntersectionChanged( On 2016/11/21 21:07:03, miu wrote: > ditto: remove "video" from name. Done. https://codereview.chromium.org/2511143006/diff/1/media/remoting/remoting_ren... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2511143006/diff/1/media/remoting/remoting_ren... media/remoting/remoting_renderer_controller.cc:16: constexpr float kViewportRatioVideoRenderingThreshold = 0.85f; On 2016/11/21 21:07:03, miu wrote: > naming accuracy: How about kViewportFillActivationThreshold? Done. https://codereview.chromium.org/2511143006/diff/1/media/remoting/remoting_ren... media/remoting/remoting_renderer_controller.cc:23: float intersect_area = static_cast<float>(intersect_rect.width()) * On 2016/11/21 21:07:03, miu wrote: > To simplify: Instead of width*height, you can call GetArea() on these rects. Removed this helper as suggested in the below comment. https://codereview.chromium.org/2511143006/diff/1/media/remoting/remoting_ren... media/remoting/remoting_renderer_controller.cc:93: if (ratio < kViewportRatioVideoRenderingThreshold) { On 2016/11/21 21:07:03, miu wrote: > To simplify: You can avoid both the floating-point math and the separate helper > needed for a divide-by-zero check by doing a little algebra here (to devise a > new conditional expression): > > ratio < 0.85 > > --> (intersect_rect.GetArea() / root_rect.GetArea()) < (85 / 100) > > --> (100 * intersect_rect.GetArea()) < (85 * root_rect.GetArea()) > > Now, even if either area is zero, the expression evaluates to the right answer: > > constexpr int64_t kOneHundredPercent = 100; > constexpr int64_t kFillActivationThresholdPercent = 85; > > if ((kOneHundredPercent * info.intersect_rect.GetArea()) < > (kFillActivationThresholdPercent * info.root_rect.GetArea()) { > ... > Done. https://codereview.chromium.org/2511143006/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/IntersectionGeometry.cpp (right): https://codereview.chromium.org/2511143006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/IntersectionGeometry.cpp:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/11/21 21:07:03, miu wrote: > FWICT, most of this code is moved from IntersectionObservation.cpp, and is not > new code. Can you adjust your git similarity setting and re-upload so we can see > what actually has changed? (`git cl upload --similarity=20` might do the trick, > but you can test this with `git diff -M20% --stat origin/master`) Done. Most of the codes are originally from InterSectionObservation.cpp and IntersectionObserver.cpp. https://codereview.chromium.org/2511143006/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2511143006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2531: if (m_webMediaPlayer) { On 2016/11/21 21:07:03, miu wrote: > There's an early return from this method about 6-8 lines above. But we want this > to always execute. So, please move this up (perhaps to the very top of this > method?). Done. Moved this to the very top of the method. https://codereview.chromium.org/2511143006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2539: WebRect(geometry.intersectionRect().x().rawValue(), On 2016/11/21 21:07:04, miu wrote: > This translation between rectangle data types is a functional detail that > belongs in IntersectionGeometry more than it does here in HTMLMediaElement. Can > you change intersectionRect() and rootRect() to return a WebRect? If not, maybe > you could create two accessor methods in IntersectionGeometry: > intersectionWebRect() and rootWebRect()? Done. Add accessor methods in IntersectionGeometry. https://codereview.chromium.org/2511143006/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right): https://codereview.chromium.org/2511143006/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:783: WebMediaPlayer::ViewportIntersectionInfo intersectInfo = On 2016/11/21 21:07:04, miu wrote: > It seems that this should be unconditional. Just: > > mediaPlayer->viewportIntersectionChanged( > htmlMediaElement.currentViewportIntersection()); Done.
media/*, HTMLMediaElement.cpp and FrameLoaderClientImpl.cpp lgtm https://codereview.chromium.org/2511143006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right): https://codereview.chromium.org/2511143006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:783: WebMediaPlayer::ViewportIntersectionInfo intersectInfo = nit: Is this local variable (intersectInfo) needed? You can eliminate a copy of the struct by: mediaPlayer->viewportIntersectionChanged( htmlMediaElement.currentViewportIntersection());
https://codereview.chromium.org/2511143006/diff/20001/media/base/media_observ... File media/base/media_observer.h (right): https://codereview.chromium.org/2511143006/diff/20001/media/base/media_observ... media/base/media_observer.h:20: struct ViewportIntersectionInfo { As we mentioned in the meeting, we'd prefer not to ship layout information from blink out to the browser. Instead, I suggest you move the logic in RemotingMediaController that analyzes ViewportIntersectionInfo (including the de-bounce timer) into a blink class, and instead export something that doesn't directly reveal layout information. https://codereview.chromium.org/2511143006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/IntersectionGeometry.h (right): https://codereview.chromium.org/2511143006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionGeometry.h:51: LayoutObject* getRootLayoutObject() const; If there are methods on IntersectionObserver that are no longer being used (because you copied them into IntersectionGeometry), then please remove them from IntersectionObserver. https://codereview.chromium.org/2511143006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right): https://codereview.chromium.org/2511143006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:23: if (!m_target || !m_observer->rootNode()) The rootNode() check is a behavior change; previously, if the root node went away, a notification would be generated. We should keep the old behavior. https://codereview.chromium.org/2511143006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:63: IntRect snappedRootBounds = pixelSnappedIntRect(geometry.rootRect()); geometry.rootIntRect() https://codereview.chromium.org/2511143006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:67: timestamp, newVisibleRatio, pixelSnappedIntRect(geometry.targetRect()), geometry.targetIntRect() https://codereview.chromium.org/2511143006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:68: rootBoundsPointer, pixelSnappedIntRect(geometry.intersectionRect()), geometry.intersectionIntRect()
https://codereview.chromium.org/2511143006/diff/20001/media/base/media_observ... File media/base/media_observer.h (right): https://codereview.chromium.org/2511143006/diff/20001/media/base/media_observ... media/base/media_observer.h:20: struct ViewportIntersectionInfo { On 2016/11/23 at 17:44:58, szager1 wrote: > As we mentioned in the meeting, we'd prefer not to ship layout information from blink out to the browser. Instead, I suggest you move the logic in RemotingMediaController that analyzes ViewportIntersectionInfo (including the de-bounce timer) into a blink class, and instead export something that doesn't directly reveal layout information. To expand on this a bit, my mental model is that we want followup patches that change the heuristics to not need extra plumbing through WebMediaPlayer. You should be able to make a change that just touches a couple files in third_party/WebKit. One possible API here is to a callback that requests the video to be remote playbacked. Actually, now that I think about it, is the existing WebMediaPlayer::requestRemotePlayback callback sufficient? HTMLMediaElement can call that when the video is in a state that it should remote playback.
Also, FYI, you'll get faster and better reviews in the future if you break up your patches into pieces. For example, this one could have been broken up into: 1. Something that didn't worry about intersection information and just looked at the video rect and the root rect. With a TODO to extract the proper clipping/intersection methods from IntersectionObserver. 2. A followup patch with the IntersectionObserver changes. That makes the patches much easier to review for correctness, which in turn gets faster turnaround time on reviews.
On 2016/11/23 18:49:13, ojan wrote: > Also, FYI, you'll get faster and better reviews in the future if you break up > your patches into pieces. For example, this one could have been broken up into: > 1. Something that didn't worry about intersection information and just looked at > the video rect and the root rect. With a TODO to extract the proper > clipping/intersection methods from IntersectionObserver. > 2. A followup patch with the IntersectionObserver changes. > > That makes the patches much easier to review for correctness, which in turn gets > faster turnaround time on reviews. Thanks for the suggestion. Will do this in future. :)
szager1, ojan, and miu: Thanks for reviewing! Addressed all comments in PS#2 in PS#3. PTAL. https://codereview.chromium.org/2511143006/diff/20001/media/base/media_observ... File media/base/media_observer.h (right): https://codereview.chromium.org/2511143006/diff/20001/media/base/media_observ... media/base/media_observer.h:20: struct ViewportIntersectionInfo { On 2016/11/23 17:44:58, szager1 wrote: > As we mentioned in the meeting, we'd prefer not to ship layout information from > blink out to the browser. Instead, I suggest you move the logic in > RemotingMediaController that analyzes ViewportIntersectionInfo (including the > de-bounce timer) into a blink class, and instead export something that doesn't > directly reveal layout information. Done. https://codereview.chromium.org/2511143006/diff/20001/media/base/media_observ... media/base/media_observer.h:20: struct ViewportIntersectionInfo { On 2016/11/23 18:43:43, ojan wrote: > On 2016/11/23 at 17:44:58, szager1 wrote: > > As we mentioned in the meeting, we'd prefer not to ship layout information > from blink out to the browser. Instead, I suggest you move the logic in > RemotingMediaController that analyzes ViewportIntersectionInfo (including the > de-bounce timer) into a blink class, and instead export something that doesn't > directly reveal layout information. > > To expand on this a bit, my mental model is that we want followup patches that > change the heuristics to not need extra plumbing through WebMediaPlayer. You > should be able to make a change that just touches a couple files in > third_party/WebKit. > > One possible API here is to a callback that requests the video to be remote > playbacked. Actually, now that I think about it, is the existing > WebMediaPlayer::requestRemotePlayback callback sufficient? HTMLMediaElement can > call that when the video is in a state that it should remote playback. As szager1 suggested, moved the logic determining whether media is filling most of the viewport (with the debouncer timer) into blink. But I don't think we can use WebMediaPlayer::requestRemotePlayback callback here. IIUC, WebMediaPlayer::requestRemotePlayback callback is for Remote Playback API, which is different from our Media Remoting application. Also, starting media remoting is not completely controlled by filling most of the viewport signal. Other criteria need to be checked in our control logic. So I added WebMediaPlayer::stablyFilledMostViewport(), which will be called when media element starts/stops stably filling most of the viewport. https://codereview.chromium.org/2511143006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/IntersectionGeometry.h (right): https://codereview.chromium.org/2511143006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionGeometry.h:51: LayoutObject* getRootLayoutObject() const; On 2016/11/23 17:44:58, szager1 wrote: > If there are methods on IntersectionObserver that are no longer being used > (because you copied them into IntersectionGeometry), then please remove them > from IntersectionObserver. Done. https://codereview.chromium.org/2511143006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right): https://codereview.chromium.org/2511143006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:23: if (!m_target || !m_observer->rootNode()) On 2016/11/23 17:44:58, szager1 wrote: > The rootNode() check is a behavior change; previously, if the root node went > away, a notification would be generated. We should keep the old behavior. Done. https://codereview.chromium.org/2511143006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:63: IntRect snappedRootBounds = pixelSnappedIntRect(geometry.rootRect()); On 2016/11/23 17:44:58, szager1 wrote: > geometry.rootIntRect() Done. https://codereview.chromium.org/2511143006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:67: timestamp, newVisibleRatio, pixelSnappedIntRect(geometry.targetRect()), On 2016/11/23 17:44:58, szager1 wrote: > geometry.targetIntRect() Done. https://codereview.chromium.org/2511143006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:68: rootBoundsPointer, pixelSnappedIntRect(geometry.intersectionRect()), On 2016/11/23 17:44:58, szager1 wrote: > geometry.intersectionIntRect() Done. https://codereview.chromium.org/2511143006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right): https://codereview.chromium.org/2511143006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:783: WebMediaPlayer::ViewportIntersectionInfo intersectInfo = On 2016/11/22 00:14:30, miu wrote: > nit: Is this local variable (intersectInfo) needed? You can eliminate a copy of > the struct by: > > mediaPlayer->viewportIntersectionChanged( > htmlMediaElement.currentViewportIntersection()); Not applicable. I find that we don't need make change here. Instead, this can be done in HTMLMediaElement.
One last round of review and I think we're good to go. Thanks for being patient. If you disagree with the nits, feel free to say so and not do them. https://codereview.chromium.org/2511143006/diff/40001/media/base/media_observ... File media/base/media_observer.h (right): https://codereview.chromium.org/2511143006/diff/40001/media/base/media_observ... media/base/media_observer.h:25: virtual void OnStablyFilledMostViewportChanged( Nit: These methods would be easier to understand if you s/MostViewport/Viewport/. The argument name makes clear that it's mostly filling. Feel free to ignore since this is just bikeshedding. https://codereview.chromium.org/2511143006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2511143006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:132: constexpr double kMostlyFillViewportBecomeStableTime = 5; Nit: I generally prefer time values to say the unit of time to be extra clear. So, I'd make this kMostlyFillViewportBecomeStableSeconds. https://codereview.chromium.org/2511143006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:4208: void HTMLMediaElement::checkViewportIntersectionChanged() { Can we early return if we're not tab casting so we only need to do this work when tab casting? The intersection work isn't *very* expensive, but it's not cheap either. Looks like we don't plumb that data in today, but I think it should be possible to via RemoteRenderingController? In either case, putting a TODO and doing so a followup patch would be fine. https://codereview.chromium.org/2511143006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:4212: IntersectionGeometry geometry(&document, this, Vector<Length>(), true); Now that you're in the HTMLElement world, you can get to the document more directly with document(). https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Node.... Also, we generally try to avoid boolean arguments to functions unless it's obvious what the bool is because it makes it hard to know what they are. From the C++ style guide "Consider changing the function signature to replace a bool argument with an enum argument. This will make the argument values self-describing." The typical solution here is to create an enum: enum class IntersectionGeometryBounds { kShouldReportRootBounds, kShouldNotReportRootBounds, } https://codereview.chromium.org/2511143006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:4224: kMostlyFillViewportThresholdPercent * Nit: If you make this kMostlyFillViewportThreshold and have the value be 0.85 instead of 85, then you can avoid needing the kOneHundredPercent value at all. https://codereview.chromium.org/2511143006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:4232: m_webMediaPlayer->stablyFilledMostViewport(false); Nit: It took me a bit to understand these few lines. Partially it's because you use false here and isMostlyFillingViewport on line 4230. Either way is fine, but it would make the code easier to read if you either use false in both places or isMostlyFillingViewport in both places. https://codereview.chromium.org/2511143006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:4237: BLINK_FROM_HERE); Indentation is off here. FWIW, I haven't used it much, but a lot of people use "git cl format" to have git cl fix these sorts of issues for them instead of doing it manually. https://codereview.chromium.org/2511143006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/2511143006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.h:739: bool m_mostlyFillingViewport = false; Put the bool up in line 653 instead with the ": 1" syntax so that it can tightly pack with the other bools. Otherwise, this line will use 32-64 bits of memory instead of 1. :)
Thanks ojan! Addressed your comments in PS#4. PTAL. https://codereview.chromium.org/2511143006/diff/40001/media/base/media_observ... File media/base/media_observer.h (right): https://codereview.chromium.org/2511143006/diff/40001/media/base/media_observ... media/base/media_observer.h:25: virtual void OnStablyFilledMostViewportChanged( On 2016/11/24 00:48:33, ojan wrote: > Nit: These methods would be easier to understand if you > s/MostViewport/Viewport/. The argument name makes clear that it's mostly > filling. > > Feel free to ignore since this is just bikeshedding. Done. https://codereview.chromium.org/2511143006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2511143006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:132: constexpr double kMostlyFillViewportBecomeStableTime = 5; On 2016/11/24 00:48:33, ojan wrote: > Nit: I generally prefer time values to say the unit of time to be extra clear. > So, I'd make this kMostlyFillViewportBecomeStableSeconds. Done. https://codereview.chromium.org/2511143006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:4208: void HTMLMediaElement::checkViewportIntersectionChanged() { On 2016/11/24 00:48:33, ojan wrote: > Can we early return if we're not tab casting so we only need to do this work > when tab casting? The intersection work isn't *very* expensive, but it's not > cheap either. > > Looks like we don't plumb that data in today, but I think it should be possible > to via RemoteRenderingController? In either case, putting a TODO and doing so a > followup patch would be fine. Add TODO comment for now. Will see if this can be resolved in a follow-up CL. https://codereview.chromium.org/2511143006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:4212: IntersectionGeometry geometry(&document, this, Vector<Length>(), true); On 2016/11/24 00:48:33, ojan wrote: > Now that you're in the HTMLElement world, you can get to the document more > directly with document(). > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Node.... > > Also, we generally try to avoid boolean arguments to functions unless it's > obvious what the bool is because it makes it hard to know what they are. From > the C++ style guide "Consider changing the function signature to replace a bool > argument with an enum argument. This will make the argument values > self-describing." > > The typical solution here is to create an enum: > enum class IntersectionGeometryBounds { > kShouldReportRootBounds, > kShouldNotReportRootBounds, > } Done. Name the enum as ReportRootBounds. https://codereview.chromium.org/2511143006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:4224: kMostlyFillViewportThresholdPercent * On 2016/11/24 00:48:33, ojan wrote: > Nit: If you make this kMostlyFillViewportThreshold and have the value be 0.85 > instead of 85, then you can avoid needing the kOneHundredPercent value at all. Done. https://codereview.chromium.org/2511143006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:4232: m_webMediaPlayer->stablyFilledMostViewport(false); On 2016/11/24 00:48:33, ojan wrote: > Nit: It took me a bit to understand these few lines. Partially it's because you > use false here and isMostlyFillingViewport on line 4230. Either way is fine, but > it would make the code easier to read if you either use false in both places or > isMostlyFillingViewport in both places. Done. https://codereview.chromium.org/2511143006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:4237: BLINK_FROM_HERE); On 2016/11/24 00:48:33, ojan wrote: > Indentation is off here. FWIW, I haven't used it much, but a lot of people use > "git cl format" to have git cl fix these sorts of issues for them instead of > doing it manually. I always do "git cl format". Is the "indentation" here not correct? Anyway, after renaming |kMostlyFillViewportBecomeStableTime|, this is not applicable. https://codereview.chromium.org/2511143006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/2511143006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.h:739: bool m_mostlyFillingViewport = false; On 2016/11/24 00:48:33, ojan wrote: > Put the bool up in line 653 instead with the ": 1" syntax so that it can tightly > pack with the other bools. Otherwise, this line will use 32-64 bits of memory > instead of 1. :) Done.
I was about to approve this and realized that there's no tests. So sorry I didn't catch this earlier. Probably the easiest way to write a test for this will be as a SimTest. Here's an example you can work from: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/tests/Link... If you did something like that but with a video element, then you could query the mostlyFillsViewport bool on the video element. Unfortunately, you'll need to friend class HTMLMediaElement with your test to get at the private state of HTMLMediaElement. I can't think of a better way to test this at the moment though. esprehn, might have better ideas. https://codereview.chromium.org/2511143006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/IntersectionObservation.h (right): https://codereview.chromium.org/2511143006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObservation.h:40: unsigned m_lastThresholdIndex; Please keep the : 1 and the : 30. That should work fine even though you're now using an enum. Did it not? As it is, this will take up 64/128 bits where it could instead take up 32/64 (the two numbers are for 32-bit and 64-bit builds).
On 2016/11/24 03:06:06, ojan wrote: > I was about to approve this and realized that there's no tests. So sorry I > didn't catch this earlier. Probably the easiest way to write a test for this > will be as a SimTest. Here's an example you can work from: > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/tests/Link... > > If you did something like that but with a video element, then you could query > the mostlyFillsViewport bool on the video element. Unfortunately, you'll need to Good idea, Ojan. I chatted with xjz@ a bit about this. One issue is: The update on that bool happens from a timer that only starts once the video element plays video. I see a couple of possibilities: 1. Write test code that has a video element play one of the test/data/web/foo.webm files and then runs the MessageLoop (what's the Blink equivalent?) until the boolean changes. This has the downside of higher test run-time overhead and requires the test take at least 250ms to run. Unless we can mock-out the clock the timers use somehow? 2. Fake the timer firing by having the test code directly invoke the timer's callback method. WDYT? Advice?
On 2016/11/24 03:06:06, ojan wrote: > I was about to approve this and realized that there's no tests. So sorry I > didn't catch this earlier. Probably the easiest way to write a test for this > will be as a SimTest. Here's an example you can work from: > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/tests/Link... > > If you did something like that but with a video element, then you could query > the mostlyFillsViewport bool on the video element. Unfortunately, you'll need to Good idea, Ojan. I chatted with xjz@ a bit about this. One issue is: The update on that bool happens from a timer that only starts once the video element plays video. I see a couple of possibilities: 1. Write test code that has a video element play one of the test/data/web/foo.webm files and then runs the MessageLoop (what's the Blink equivalent?) until the boolean changes. This has the downside of higher test run-time overhead and requires the test take at least 250ms to run. Unless we can mock-out the clock the timers use somehow? 2. Fake the timer firing by having the test code directly invoke the timer's callback method. WDYT? Advice?
modulo tests, the code lg. nit (with my user hat on): the current implementation will always buffer video to the browser for at least five seconds before remoting kicks in. Would be nice to allow remoting right from the start if the player was mostly filling the viewport when the play button was pressed.
Yeah, I think there's follow-up patches to do for hooking into start/stop of playing, but they don't need to go into this patch. On Tue, Nov 29, 2016, 3:06 PM <szager@chromium.org> wrote: > modulo tests, the code lg. > > nit (with my user hat on): the current implementation will always buffer > video > to the browser for at least five seconds before remoting kicks in. Would be > nice to allow remoting right from the start if the player was mostly > filling the > viewport when the play button was pressed. > > https://codereview.chromium.org/2511143006/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Yeah, I think there's follow-up patches to do for hooking into start/stop of playing, but they don't need to go into this patch. On Tue, Nov 29, 2016, 3:06 PM <szager@chromium.org> wrote: > modulo tests, the code lg. > > nit (with my user hat on): the current implementation will always buffer > video > to the browser for at least five seconds before remoting kicks in. Would be > nice to allow remoting right from the start if the player was mostly > filling the > viewport when the play button was pressed. > > https://codereview.chromium.org/2511143006/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #5 (id:80001) has been deleted
Added tests. PTAL https://codereview.chromium.org/2511143006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/IntersectionObservation.h (right): https://codereview.chromium.org/2511143006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObservation.h:40: unsigned m_lastThresholdIndex; On 2016/11/24 03:06:06, ojan wrote: > Please keep the : 1 and the : 30. That should work fine even though you're now > using an enum. Did it not? > > As it is, this will take up 64/128 bits where it could instead take up 32/64 > (the two numbers are for 32-bit and 64-bit builds). Yes, it still only takes 1 bit. The only reason I removed that is because it will cause the following pre-submit error: check-webkit-style failed third_party/WebKit/Source/core/dom/IntersectionObservation.h:39: Member m_shouldReportRootBounds of class IntersectionObservation defined as a bitfield of type IntersectionGeometry::ReportRootBounds. Please declare all bitfields as unsigned. [runtime/bitfields] [4] In order to keep this, I added converters to convert between bool and the enum.
https://codereview.chromium.org/2511143006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/IntersectionObservation.h (right): https://codereview.chromium.org/2511143006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObservation.h:40: unsigned m_lastThresholdIndex; On 2016/11/30 01:58:44, xjz wrote: > In order to keep this, I added converters to convert between bool and the enum. You can make it unsigned. Just initialize it in the ctor as: ..., m_shouldReportRootBounds(shouldReportRootBounds == kShouldReportRootBounds), ..., and this member should be const, right?
Addressed comments. PTAL. https://codereview.chromium.org/2511143006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/IntersectionObservation.h (right): https://codereview.chromium.org/2511143006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObservation.h:40: unsigned m_lastThresholdIndex; On 2016/11/30 03:40:36, miu wrote: > On 2016/11/30 01:58:44, xjz wrote: > > In order to keep this, I added converters to convert between bool and the > enum. > > You can make it unsigned. Just initialize it in the ctor as: > > ..., > m_shouldReportRootBounds(shouldReportRootBounds == kShouldReportRootBounds), > ..., > > and this member should be const, right? Yes, that is what toShouldReportRootBounds() does. And we need to convert it back to the enum when create IntersectionGeometry. It seems the helpers are not necessary, since both are only called once. Removed them and instead put codes where they are called. Also marked |m_shouldReportRootBounds| as const.
lgtm! Please remove the runPendingTasks line and inline the beginFrame call instead of having a wrapper function if turns out to not be needed. If it is actually needed, then lgtm as is. If you're willing to, I think it'd be worth adding the couple extra test cases I measured, but it doesn't need another round of review. Unfortunately, I think I'm not an OWNER in Source/web, so you'll need to find someone to rubberstamp. https://codereview.chromium.org/2511143006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/MediaElementFillingViewportTest.cpp (right): https://codereview.chromium.org/2511143006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/MediaElementFillingViewportTest.cpp:34: testing::runPendingTasks(); This would be for running tasks that are scheduled during the frame pumping (e.g. if you were expecting the debounce timer callback to actually get called), but I don't think there are any callbacks that need calling for these tests. Am I wrong? If there are, then this line is correct. If not, it's best to keep tests to the minimal of what's needed. https://codereview.chromium.org/2511143006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/MediaElementFillingViewportTest.cpp:85: TEST_F(MediaElementFillingViewportTest, FillingViewportChanged) { For good measure, you might also want to test the following cases: -A video that's large enough to fill the viewport, but is scrolled half offscreen, so doesn't. -A video that's larger than the viewport. This was the case that would have not worked if we just used vanilla IntersectionObserver.
media/ looking good. Just a few nits for discussion. https://codereview.chromium.org/2511143006/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/2511143006/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebMediaPlayer.h:231: // Inform WebmediaPlayer when the element starts/stops stably filling most of nit: WebMediaPlayer https://codereview.chromium.org/2511143006/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebMediaPlayer.h:233: virtual void stablyFilledViewport(bool isMostlyFillingViewport) {} nit: Could you please clarify what "stably" and "mostly" exactly mean? https://codereview.chromium.org/2511143006/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebMediaPlayer.h:233: virtual void stablyFilledViewport(bool isMostlyFillingViewport) {} nit: IMHO this is a bit hard to read, especially since "stably" is in the API name, but "mostly" is in the boolean parameter. For example, what's the difference b/w: stablyFilledViewport(bool isMostlyFillingViewport) and mostlyFilledViewport(bool isStablyFillingViewport) With that, how about something like: stablyAndMostlyFilledViewport(bool filled) or anything that could make the interface cleaner?
Patchset #7 (id:140001) has been deleted
ojan@google.com changed reviewers: + ojan@google.com
https://codereview.chromium.org/2511143006/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/2511143006/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebMediaPlayer.h:233: virtual void stablyFilledViewport(bool isMostlyFillingViewport) {} On 2016/11/30 at 18:13:09, xhwang wrote: > nit: Could you please clarify what "stably" and "mostly" exactly mean? How about just filledViewport(bool isMostlyFillingViewport)? As we tweak heuristics, the "stably" bit might change or get dropped entirely. We might also add new things.
Patchset #7 (id:160001) has been deleted
Thanks ojan and xhwang. Addressed your comments and added two more tests. PTAL. esprehn: need your rubberstamp on third_party/WebKit/Source/web. Thanks! https://codereview.chromium.org/2511143006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/MediaElementFillingViewportTest.cpp (right): https://codereview.chromium.org/2511143006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/MediaElementFillingViewportTest.cpp:34: testing::runPendingTasks(); On 2016/11/30 07:09:40, ojan wrote: > This would be for running tasks that are scheduled during the frame pumping > (e.g. if you were expecting the debounce timer callback to actually get called), > but I don't think there are any callbacks that need calling for these tests. Am > I wrong? > > If there are, then this line is correct. If not, it's best to keep tests to the > minimal of what's needed. Done. https://codereview.chromium.org/2511143006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/MediaElementFillingViewportTest.cpp:85: TEST_F(MediaElementFillingViewportTest, FillingViewportChanged) { On 2016/11/30 07:09:40, ojan wrote: > For good measure, you might also want to test the following cases: > -A video that's large enough to fill the viewport, but is scrolled half > offscreen, so doesn't. > -A video that's larger than the viewport. This was the case that would have not > worked if we just used vanilla IntersectionObserver. Done. https://codereview.chromium.org/2511143006/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/2511143006/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebMediaPlayer.h:231: // Inform WebmediaPlayer when the element starts/stops stably filling most of On 2016/11/30 18:13:09, xhwang wrote: > nit: WebMediaPlayer Done. https://codereview.chromium.org/2511143006/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebMediaPlayer.h:233: virtual void stablyFilledViewport(bool isMostlyFillingViewport) {} On 2016/11/30 18:48:23, Z_DONOTUSE wrote: > On 2016/11/30 at 18:13:09, xhwang wrote: > > nit: Could you please clarify what "stably" and "mostly" exactly mean? > > How about just filledViewport(bool isMostlyFillingViewport)? As we tweak > heuristics, the "stably" bit might change or get dropped entirely. We might also > add new things. I am fine with either way, but think this one is simpler. xhwang: WDYT?
https://codereview.chromium.org/2511143006/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/2511143006/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebMediaPlayer.h:233: virtual void stablyFilledViewport(bool isMostlyFillingViewport) {} On 2016/11/30 18:58:16, xjz wrote: > On 2016/11/30 18:48:23, Z_DONOTUSE wrote: > > On 2016/11/30 at 18:13:09, xhwang wrote: > > > nit: Could you please clarify what "stably" and "mostly" exactly mean? > > > > How about just filledViewport(bool isMostlyFillingViewport)? As we tweak > > heuristics, the "stably" bit might change or get dropped entirely. We might > also > > add new things. > > I am fine with either way, but think this one is simpler. > xhwang: WDYT? filledViewport(bool isMostlyFillingViewport) l-g-t-m :)
https://codereview.chromium.org/2511143006/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/2511143006/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebMediaPlayer.h:233: virtual void stablyFilledViewport(bool isMostlyFillingViewport) {} On 2016/11/30 19:06:21, xhwang wrote: > On 2016/11/30 18:58:16, xjz wrote: > > On 2016/11/30 18:48:23, Z_DONOTUSE wrote: > > > On 2016/11/30 at 18:13:09, xhwang wrote: > > > > nit: Could you please clarify what "stably" and "mostly" exactly mean? > > > > > > How about just filledViewport(bool isMostlyFillingViewport)? As we tweak > > > heuristics, the "stably" bit might change or get dropped entirely. We might > > also > > > add new things. > > > > I am fine with either way, but think this one is simpler. > > xhwang: WDYT? > > filledViewport(bool isMostlyFillingViewport) l-g-t-m :) I don't like filledViewport, for multiple reasons: 1. "filled" can read as an adjective, not a verb. That suggests a naming like "didFillViewport" but that brings me to reason #2: 2. The naming of this function is exposing the internal details of the heuristics, but we already know these details will be changing somewhat in a soon-upcoming CL. So, the "filledViewport" naming locks in the current implementation details, rather than exposing a callback interface for a specific purpose. So, let's step back and think about what this is being used for and what information it is meant to communicate: that the video has been positioned and sized in such a way to indicate the user is meant to view or engage with it immersively. Therefore, I propose: onShowingImmersively(bool) <-- my favorite or becameDominantVisibleContent(bool) or becamePrimaryEngagingElement(bool) Which one do you guys like?
On 2016/11/30 19:56:56, miu wrote: > https://codereview.chromium.org/2511143006/diff/120001/third_party/WebKit/pub... > File third_party/WebKit/public/platform/WebMediaPlayer.h (right): > > https://codereview.chromium.org/2511143006/diff/120001/third_party/WebKit/pub... > third_party/WebKit/public/platform/WebMediaPlayer.h:233: virtual void > stablyFilledViewport(bool isMostlyFillingViewport) {} > On 2016/11/30 19:06:21, xhwang wrote: > > On 2016/11/30 18:58:16, xjz wrote: > > > On 2016/11/30 18:48:23, Z_DONOTUSE wrote: > > > > On 2016/11/30 at 18:13:09, xhwang wrote: > > > > > nit: Could you please clarify what "stably" and "mostly" exactly mean? > > > > > > > > How about just filledViewport(bool isMostlyFillingViewport)? As we tweak > > > > heuristics, the "stably" bit might change or get dropped entirely. We > might > > > also > > > > add new things. > > > > > > I am fine with either way, but think this one is simpler. > > > xhwang: WDYT? > > > > filledViewport(bool isMostlyFillingViewport) l-g-t-m :) > > I don't like filledViewport, for multiple reasons: > > 1. "filled" can read as an adjective, not a verb. That suggests a naming like > "didFillViewport" but that brings me to reason #2: > > 2. The naming of this function is exposing the internal details of the > heuristics, but we already know these details will be changing somewhat in a > soon-upcoming CL. So, the "filledViewport" naming locks in the current > implementation details, rather than exposing a callback interface for a specific > purpose. > > So, let's step back and think about what this is being used for and what > information it is meant to communicate: that the video has been positioned and > sized in such a way to indicate the user is meant to view or engage with it > immersively. Therefore, I propose: > > onShowingImmersively(bool) <-- my favorite > or becameDominantVisibleContent(bool) > or becamePrimaryEngagingElement(bool) > > Which one do you guys like? Thanks miu. The first two lgtm. :) ojan, xhwang, and esprehn: WDYT?
On 2016/11/30 20:05:02, xjz wrote: > On 2016/11/30 19:56:56, miu wrote: > > > https://codereview.chromium.org/2511143006/diff/120001/third_party/WebKit/pub... > > File third_party/WebKit/public/platform/WebMediaPlayer.h (right): > > > > > https://codereview.chromium.org/2511143006/diff/120001/third_party/WebKit/pub... > > third_party/WebKit/public/platform/WebMediaPlayer.h:233: virtual void > > stablyFilledViewport(bool isMostlyFillingViewport) {} > > On 2016/11/30 19:06:21, xhwang wrote: > > > On 2016/11/30 18:58:16, xjz wrote: > > > > On 2016/11/30 18:48:23, Z_DONOTUSE wrote: > > > > > On 2016/11/30 at 18:13:09, xhwang wrote: > > > > > > nit: Could you please clarify what "stably" and "mostly" exactly mean? > > > > > > > > > > How about just filledViewport(bool isMostlyFillingViewport)? As we tweak > > > > > heuristics, the "stably" bit might change or get dropped entirely. We > > might > > > > also > > > > > add new things. > > > > > > > > I am fine with either way, but think this one is simpler. > > > > xhwang: WDYT? > > > > > > filledViewport(bool isMostlyFillingViewport) l-g-t-m :) > > > > I don't like filledViewport, for multiple reasons: > > > > 1. "filled" can read as an adjective, not a verb. That suggests a naming like > > "didFillViewport" but that brings me to reason #2: > > > > 2. The naming of this function is exposing the internal details of the > > heuristics, but we already know these details will be changing somewhat in a > > soon-upcoming CL. So, the "filledViewport" naming locks in the current > > implementation details, rather than exposing a callback interface for a > specific > > purpose. > > > > So, let's step back and think about what this is being used for and what > > information it is meant to communicate: that the video has been positioned and > > sized in such a way to indicate the user is meant to view or engage with it > > immersively. Therefore, I propose: > > > > onShowingImmersively(bool) <-- my favorite > > or becameDominantVisibleContent(bool) > > or becamePrimaryEngagingElement(bool) > > > > Which one do you guys like? > > Thanks miu. The first two lgtm. :) > ojan, xhwang, and esprehn: WDYT? I like the idea of having an API that summarizes the purpose, not the impl detail. Immersive would be ambiguous given VR is so hot these days: https://www.google.com/search?q=immersive&oq=immersive&aqs=chrome..69i57j0l5.... I don't have any better suggestions though...
Any of those three LGTM. I agree that the old names leaked too much implementation detail. On Wed, Nov 30, 2016, 12:05 PM <xjz@chromium.org> wrote: > On 2016/11/30 19:56:56, miu wrote: > > > > https://codereview.chromium.org/2511143006/diff/120001/third_party/WebKit/pub... > > File third_party/WebKit/public/platform/WebMediaPlayer.h (right): > > > > > > https://codereview.chromium.org/2511143006/diff/120001/third_party/WebKit/pub... > > third_party/WebKit/public/platform/WebMediaPlayer.h:233: virtual void > > stablyFilledViewport(bool isMostlyFillingViewport) {} > > On 2016/11/30 19:06:21, xhwang wrote: > > > On 2016/11/30 18:58:16, xjz wrote: > > > > On 2016/11/30 18:48:23, Z_DONOTUSE wrote: > > > > > On 2016/11/30 at 18:13:09, xhwang wrote: > > > > > > nit: Could you please clarify what "stably" and "mostly" exactly > mean? > > > > > > > > > > How about just filledViewport(bool isMostlyFillingViewport)? As we > tweak > > > > > heuristics, the "stably" bit might change or get dropped entirely. > We > > might > > > > also > > > > > add new things. > > > > > > > > I am fine with either way, but think this one is simpler. > > > > xhwang: WDYT? > > > > > > filledViewport(bool isMostlyFillingViewport) l-g-t-m :) > > > > I don't like filledViewport, for multiple reasons: > > > > 1. "filled" can read as an adjective, not a verb. That suggests a naming > like > > "didFillViewport" but that brings me to reason #2: > > > > 2. The naming of this function is exposing the internal details of the > > heuristics, but we already know these details will be changing somewhat > in a > > soon-upcoming CL. So, the "filledViewport" naming locks in the current > > implementation details, rather than exposing a callback interface for a > specific > > purpose. > > > > So, let's step back and think about what this is being used for and what > > information it is meant to communicate: that the video has been > positioned and > > sized in such a way to indicate the user is meant to view or engage with > it > > immersively. Therefore, I propose: > > > > onShowingImmersively(bool) <-- my favorite > > or becameDominantVisibleContent(bool) > > or becamePrimaryEngagingElement(bool) > > > > Which one do you guys like? > > Thanks miu. The first two lgtm. :) > ojan, xhwang, and esprehn: WDYT? > > https://codereview.chromium.org/2511143006/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Any of those three LGTM. I agree that the old names leaked too much implementation detail. On Wed, Nov 30, 2016, 12:05 PM <xjz@chromium.org> wrote: > On 2016/11/30 19:56:56, miu wrote: > > > > https://codereview.chromium.org/2511143006/diff/120001/third_party/WebKit/pub... > > File third_party/WebKit/public/platform/WebMediaPlayer.h (right): > > > > > > https://codereview.chromium.org/2511143006/diff/120001/third_party/WebKit/pub... > > third_party/WebKit/public/platform/WebMediaPlayer.h:233: virtual void > > stablyFilledViewport(bool isMostlyFillingViewport) {} > > On 2016/11/30 19:06:21, xhwang wrote: > > > On 2016/11/30 18:58:16, xjz wrote: > > > > On 2016/11/30 18:48:23, Z_DONOTUSE wrote: > > > > > On 2016/11/30 at 18:13:09, xhwang wrote: > > > > > > nit: Could you please clarify what "stably" and "mostly" exactly > mean? > > > > > > > > > > How about just filledViewport(bool isMostlyFillingViewport)? As we > tweak > > > > > heuristics, the "stably" bit might change or get dropped entirely. > We > > might > > > > also > > > > > add new things. > > > > > > > > I am fine with either way, but think this one is simpler. > > > > xhwang: WDYT? > > > > > > filledViewport(bool isMostlyFillingViewport) l-g-t-m :) > > > > I don't like filledViewport, for multiple reasons: > > > > 1. "filled" can read as an adjective, not a verb. That suggests a naming > like > > "didFillViewport" but that brings me to reason #2: > > > > 2. The naming of this function is exposing the internal details of the > > heuristics, but we already know these details will be changing somewhat > in a > > soon-upcoming CL. So, the "filledViewport" naming locks in the current > > implementation details, rather than exposing a callback interface for a > specific > > purpose. > > > > So, let's step back and think about what this is being used for and what > > information it is meant to communicate: that the video has been > positioned and > > sized in such a way to indicate the user is meant to view or engage with > it > > immersively. Therefore, I propose: > > > > onShowingImmersively(bool) <-- my favorite > > or becameDominantVisibleContent(bool) > > or becamePrimaryEngagingElement(bool) > > > > Which one do you guys like? > > Thanks miu. The first two lgtm. :) > ojan, xhwang, and esprehn: WDYT? > > https://codereview.chromium.org/2511143006/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/11/30 21:00:50, chromium-reviews wrote: > Any of those three LGTM. I agree that the old names leaked too much > implementation detail. > > On Wed, Nov 30, 2016, 12:05 PM <mailto:xjz@chromium.org> wrote: > > > On 2016/11/30 19:56:56, miu wrote: > > > > > > > > https://codereview.chromium.org/2511143006/diff/120001/third_party/WebKit/pub... > > > File third_party/WebKit/public/platform/WebMediaPlayer.h (right): > > > > > > > > > > > https://codereview.chromium.org/2511143006/diff/120001/third_party/WebKit/pub... > > > third_party/WebKit/public/platform/WebMediaPlayer.h:233: virtual void > > > stablyFilledViewport(bool isMostlyFillingViewport) {} > > > On 2016/11/30 19:06:21, xhwang wrote: > > > > On 2016/11/30 18:58:16, xjz wrote: > > > > > On 2016/11/30 18:48:23, Z_DONOTUSE wrote: > > > > > > On 2016/11/30 at 18:13:09, xhwang wrote: > > > > > > > nit: Could you please clarify what "stably" and "mostly" exactly > > mean? > > > > > > > > > > > > How about just filledViewport(bool isMostlyFillingViewport)? As we > > tweak > > > > > > heuristics, the "stably" bit might change or get dropped entirely. > > We > > > might > > > > > also > > > > > > add new things. > > > > > > > > > > I am fine with either way, but think this one is simpler. > > > > > xhwang: WDYT? > > > > > > > > filledViewport(bool isMostlyFillingViewport) l-g-t-m :) > > > > > > I don't like filledViewport, for multiple reasons: > > > > > > 1. "filled" can read as an adjective, not a verb. That suggests a naming > > like > > > "didFillViewport" but that brings me to reason #2: > > > > > > 2. The naming of this function is exposing the internal details of the > > > heuristics, but we already know these details will be changing somewhat > > in a > > > soon-upcoming CL. So, the "filledViewport" naming locks in the current > > > implementation details, rather than exposing a callback interface for a > > specific > > > purpose. > > > > > > So, let's step back and think about what this is being used for and what > > > information it is meant to communicate: that the video has been > > positioned and > > > sized in such a way to indicate the user is meant to view or engage with > > it > > > immersively. Therefore, I propose: > > > > > > onShowingImmersively(bool) <-- my favorite > > > or becameDominantVisibleContent(bool) > > > or becamePrimaryEngagingElement(bool) > > > > > > Which one do you guys like? > > > > Thanks miu. The first two lgtm. :) > > ojan, xhwang, and esprehn: WDYT? > > > > https://codereview.chromium.org/2511143006/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Seems that the second one is the winner. :) Renamed. PTAL.
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...
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 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...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
ping esprehn@: need your rubber stamp on the following files: third_party/WebKit/Source/web/BUILD.gn third_party/WebKit/public/platform/WebMediaPlayer.h Thanks!
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) 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 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
esprehn@chromium.org changed reviewers: - ojan@google.com
The CQ bit was checked by xjz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org, ojan@chromium.org, xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/2511143006/#ps260001 (title: "Rebase again.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1480744124394110,
"parent_rev": "8524861214db4b7f06a55e623e150a5e61b3b9fb", "commit_rev":
"318c5f187a435df3f05f1c81db5a2b586dc7286f"}
Message was sent while issue was closed.
Description was changed from ========== Detect change on the intersection of video and viewport. This Cl: 1. Abstracts the calculation of intersection geometry from IntersectionObserver. Calculating algorithms keep unchanged. 2. When a media is playing, periodically checks whether the intersection with viewport has changed. If changed, notifies WebMediaPlayerImpl. 3. Adds logic to start remoting when video consistently covers most of the viewport for a certain time. BUG=643964 ========== to ========== Detect change on the intersection of video and viewport. This Cl: 1. Abstracts the calculation of intersection geometry from IntersectionObserver. Calculating algorithms keep unchanged. 2. When a media is playing, periodically checks whether the intersection with viewport has changed. If changed, notifies WebMediaPlayerImpl. 3. Adds logic to start remoting when video consistently covers most of the viewport for a certain time. BUG=643964 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Detect change on the intersection of video and viewport. This Cl: 1. Abstracts the calculation of intersection geometry from IntersectionObserver. Calculating algorithms keep unchanged. 2. When a media is playing, periodically checks whether the intersection with viewport has changed. If changed, notifies WebMediaPlayerImpl. 3. Adds logic to start remoting when video consistently covers most of the viewport for a certain time. BUG=643964 ========== to ========== Detect change on the intersection of video and viewport. This Cl: 1. Abstracts the calculation of intersection geometry from IntersectionObserver. Calculating algorithms keep unchanged. 2. When a media is playing, periodically checks whether the intersection with viewport has changed. If changed, notifies WebMediaPlayerImpl. 3. Adds logic to start remoting when video consistently covers most of the viewport for a certain time. BUG=643964 Committed: https://crrev.com/cdbbe7359047bf173cff5bff708b59efb889862c Cr-Commit-Position: refs/heads/master@{#436166} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/cdbbe7359047bf173cff5bff708b59efb889862c Cr-Commit-Position: refs/heads/master@{#436166} |
