Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2813303005:
[Blink>Media] Moving autoplay logic to AutoplayPolicy (Closed)
|
Created:
3 years, 8 months ago by Zhiqiang Zhang (Slow) Modified:
3 years, 8 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, mlamouri+watch-blink_chromium.org, sandersd (OOO until July 31), nessy, Srirama Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Blink>Media] Moving autoplay logic to AutoplayPolicy
This CL moves autoplay code from HTMLMediaElement to the
AutoplayPolicy class, which helps decoupling the autoplay logic
from HTMLMediaElement.
BUG=712606
Review-Url: https://codereview.chromium.org/2813303005
Cr-Commit-Position: refs/heads/master@{#466137}
Committed: https://chromium.googlesource.com/chromium/src/+/12d76adf4bb340d8e430a18c3a664940157800f6
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : fixed event order for setMuted and added comments #
Total comments: 20
Patch Set 4 : addressed nits #
Total comments: 40
Patch Set 5 : addressed more comments #Patch Set 6 : fixed muted #Patch Set 7 : docs #
Total comments: 5
Patch Set 8 : fixed broken test #
Total comments: 1
Patch Set 9 : addressed nits #Messages
Total messages: 52 (36 generated)
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
zqzhang@chromium.org changed reviewers: + mlamouri@chromium.org
zqzhang@chromium.org changed reviewers: + mlamouri@chromium.org
PTAL
PTAL
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_...)
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== moving autoplay logic to AutoplayPolicy temporary BUG= ========== to ========== [Blink>Media] Moving autoplay logic to AutoplayPolicy This CL moves autoplay code from HTMLMediaElement to the AutoplayPolicy class, which helps decoupling the autoplay logic from HTMLMediaElement. BUG=712606 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I might need more than one pass to review this (it's not a trivial change). I've left a few comments below. Note that the ideal state which should _not_ happen in this CL is for HTMLMediaElement to not have code specific to the autoplay policy. I would like us to set the autoplay policy when HTMLMediaElement is created and let AutoplayPolicy decide what can and can't be done. https://codereview.chromium.org/2813303005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2813303005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2268: } The logic is slightly changing with this change. I'm all for a simpler logic but what makes you think it will have no side effects? https://codereview.chromium.org/2813303005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2276: // anywhere else! Comment is out of date, right? https://codereview.chromium.org/2813303005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2483: autoplay_policy_->CheckUnmuteShouldPauseAutoplay(); Not sure this is at the right place. Should this be called before UnlockUserGesture? https://codereview.chromium.org/2813303005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/2813303005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.h:334: AutoplayUmaHelper& autoplay_uma_helper() { return *autoplay_uma_helper_; } For some reasons, Blink coding style doesn't allow foo_bar(). Instead, it recommends FooBar() which IMO is terrible because of the name clash with constructors. Can you name these GetFooBar()? https://codereview.chromium.org/2813303005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.h:731: Member<AutoplayUmaHelper> autoplay_uma_helper_; How hard would it be to move `autoplay_uma_helper_` in AutoplayPolicy? https://codereview.chromium.org/2813303005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/media/AutoplayPolicy.cpp (right): https://codereview.chromium.org/2813303005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.cpp:96: if (!autoplay_visibility_observer_) { Early return? https://codereview.chromium.org/2813303005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.cpp:105: if (autoplay_visibility_observer_) { ditto https://codereview.chromium.org/2813303005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/media/AutoplayPolicy.h (right): https://codereview.chromium.org/2813303005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:18: class AutoplayPolicy final : public GarbageCollectedFinalized<AutoplayPolicy> { Why do you need this to be GCFinalized? Sounds like Finalized isn't needed, is it? https://codereview.chromium.org/2813303005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:34: // gesture and record UMAs, so can only be called per call of play(). "called _once_ per call" ? https://codereview.chromium.org/2813303005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:77: DECLARE_VIRTUAL_TRACE(); This is usually public, move this just above `private`.
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2813303005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2813303005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2268: } On 2017/04/18 at 13:28:28, mlamouri wrote: > The logic is slightly changing with this change. I'm all for a simpler logic but what makes you think it will have no side effects? OK, I moved the code a bit so it should have the same behavior now. https://codereview.chromium.org/2813303005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2276: // anywhere else! On 2017/04/18 at 13:28:28, mlamouri wrote: > Comment is out of date, right? Done. https://codereview.chromium.org/2813303005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2483: autoplay_policy_->CheckUnmuteShouldPauseAutoplay(); On 2017/04/18 at 13:28:28, mlamouri wrote: > Not sure this is at the right place. Should this be called before UnlockUserGesture? The logic won't change actually. The original logic was to check whether the video was autoplaying muted. If yes, and and there's no user gesture, we should pause the video. The gesture is unlocked before the gesture check. The CheckUnmuteShouldPauseAutoplay (now called RequestAutoplayUnmute) does just that :) https://codereview.chromium.org/2813303005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/2813303005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.h:334: AutoplayUmaHelper& autoplay_uma_helper() { return *autoplay_uma_helper_; } On 2017/04/18 at 13:28:28, mlamouri wrote: > For some reasons, Blink coding style doesn't allow foo_bar(). Instead, it recommends FooBar() which IMO is terrible because of the name clash with constructors. Can you name these GetFooBar()? Done. https://codereview.chromium.org/2813303005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.h:731: Member<AutoplayUmaHelper> autoplay_uma_helper_; On 2017/04/18 at 13:28:28, mlamouri wrote: > How hard would it be to move `autoplay_uma_helper_` in AutoplayPolicy? Makes sense. Done :) https://codereview.chromium.org/2813303005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/media/AutoplayPolicy.cpp (right): https://codereview.chromium.org/2813303005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.cpp:96: if (!autoplay_visibility_observer_) { On 2017/04/18 at 13:28:28, mlamouri wrote: > Early return? Done. https://codereview.chromium.org/2813303005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.cpp:105: if (autoplay_visibility_observer_) { On 2017/04/18 at 13:28:28, mlamouri wrote: > ditto Done. https://codereview.chromium.org/2813303005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/media/AutoplayPolicy.h (right): https://codereview.chromium.org/2813303005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:18: class AutoplayPolicy final : public GarbageCollectedFinalized<AutoplayPolicy> { On 2017/04/18 at 13:28:29, mlamouri wrote: > Why do you need this to be GCFinalized? Sounds like Finalized isn't needed, is it? Done. https://codereview.chromium.org/2813303005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:34: // gesture and record UMAs, so can only be called per call of play(). On 2017/04/18 at 13:28:28, mlamouri wrote: > "called _once_ per call" ? Done. https://codereview.chromium.org/2813303005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:77: DECLARE_VIRTUAL_TRACE(); On 2017/04/18 at 13:28:28, mlamouri wrote: > This is usually public, move this just above `private`. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
It looks good in general. I really appreciate the effort to reduce the number of methods available from AutoplayPolicy. See comments below. https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1788: if (autoplay_policy_->RequestAutoplayByAttribute()) { So much clean up \o/ https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2243: // The only place to call RequestPlay(), don't call from anywhere else! I would move this to the header of AutoplayPolicy. Probably isn't very useful here. https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2244: Nullable<ExceptionCode> exception_code = autoplay_policy_->RequestPlay(); neat! :) https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2459: bool should_pause_after_unmute = autoplay_policy_->RequestAutoplayUnmute(); This is checking if there is a lock but the lock will have changed from the line above. I think we might need to split this in two methods sadly :( https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3866: } Aren't you missing the autoplay_policy_ call here? https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/media/AutoplayPolicy.cpp (right): https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.cpp:27: bool IsDocumentWhitelisted(Document& document) { nit: add comment explaining what this is doing? https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.cpp:136: if (ShouldAutoplay()) { maybe an early return would be better? :) https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.cpp:181: return nullptr; empty line above? https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.cpp:195: return locked_pending_user_gesture_; stupid question: I know it's a copy-paste from the previous code but should this consider `locked_pending_user_gesture_if_cross_origin_experiment_enabled_`? maybe not the right CL to do this change but wdyt? https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/media/AutoplayPolicy.h (right): https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:21: AutoplayPolicy(HTMLMediaElement*); explicit? https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:23: AutoplayUmaHelper& GetAutoplayUmaHelper() { return *autoplay_uma_helper_; } Doesn't seem to be called at all https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:31: bool IsEligibleForAutoplayMuted() const; It doesn't seem to be called outside of `AutoplayPolicy`. https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:35: void StopAutoplayMutedWhenVisible(); StartAutoplayMutedWhenVisible() isn't called outside of `AutoplayPolicy`. Can you add a TODO in my name to hide StopAutoplayMutedWhenVisible from HTMLMediaElement? I have an idea in how to do it but let's not increase the complexity of this CL :) https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:52: bool IsAutoplayingMuted(); Could this be `const`? https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:58: bool IsLockedPendingUserGesture() const; I can't see this being called outside of this class. https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:60: bool IsLockedPendingUserGestureIfCrossOriginExperimentEnabled() const; ditto https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:76: bool IsGestureNeededForPlaybackIfCrossOriginExperimentEnabled() const; This doesn't seem to be called by anything outside of `AutoplayPolicy`. Does it have to be public? https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:78: bool IsGestureNeededForPlaybackIfPendingUserGestureIsLocked() const; Same here https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:82: bool IsAutoplayAllowedPerSettings() const; and here https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:86: void OnVisibilityChangedForAutoplay(bool is_visible); and here :) https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:96: bool locked_pending_user_gesture_if_cross_origin_experiment_enabled_; Maybe these bool should be initialised here?
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was 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_...)
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL +CC: sandersd FYI https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2243: // The only place to call RequestPlay(), don't call from anywhere else! On 2017/04/19 at 14:09:55, mlamouri wrote: > I would move this to the header of AutoplayPolicy. Probably isn't very useful here. Done. https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2459: bool should_pause_after_unmute = autoplay_policy_->RequestAutoplayUnmute(); On 2017/04/19 at 14:09:55, mlamouri wrote: > This is checking if there is a lock but the lock will have changed from the line above. I think we might need to split this in two methods sadly :( Moved all these logic to AutoplayPolicy. The gesture utilization is also moved there. https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3866: } On 2017/04/19 at 14:09:55, mlamouri wrote: > Aren't you missing the autoplay_policy_ call here? Yes indeed. Done. https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/media/AutoplayPolicy.cpp (right): https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.cpp:27: bool IsDocumentWhitelisted(Document& document) { On 2017/04/19 at 14:09:55, mlamouri wrote: > nit: add comment explaining what this is doing? Done. https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.cpp:136: if (ShouldAutoplay()) { On 2017/04/19 at 14:09:55, mlamouri wrote: > maybe an early return would be better? :) Done. Also moved the code a bit to make the logic simpler. https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.cpp:181: return nullptr; On 2017/04/19 at 14:09:55, mlamouri wrote: > empty line above? Done. https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.cpp:195: return locked_pending_user_gesture_; On 2017/04/19 at 14:09:55, mlamouri wrote: > stupid question: I know it's a copy-paste from the previous code but should this consider `locked_pending_user_gesture_if_cross_origin_experiment_enabled_`? maybe not the right CL to do this change but wdyt? We shouldn't consider locked_pending_user_gesture_if_cross_origin_experiment_enabled_ here. The history is that we need to pretend the cross-origin experiment is ON to record the UMA. Therefore we are having a separate set of flags/methods for the experiment. The logic should be the same with the original user gesture lock, but it will only take effect when the experiment is really ON. You can check ComputeLockedPendingUserGesture() https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/media/AutoplayPolicy.h (right): https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:21: AutoplayPolicy(HTMLMediaElement*); On 2017/04/19 at 14:09:55, mlamouri wrote: > explicit? Done. https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:23: AutoplayUmaHelper& GetAutoplayUmaHelper() { return *autoplay_uma_helper_; } On 2017/04/19 at 14:09:55, mlamouri wrote: > Doesn't seem to be called at all Done. https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:31: bool IsEligibleForAutoplayMuted() const; On 2017/04/19 at 14:09:56, mlamouri wrote: > It doesn't seem to be called outside of `AutoplayPolicy`. Moved to private. https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:35: void StopAutoplayMutedWhenVisible(); On 2017/04/19 at 14:09:55, mlamouri wrote: > StartAutoplayMutedWhenVisible() isn't called outside of `AutoplayPolicy`. > Can you add a TODO in my name to hide StopAutoplayMutedWhenVisible from HTMLMediaElement? I have an idea in how to do it but let's not increase the complexity of this CL :) Ah, yes. Added comments above, and moved Start...() to private. https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:52: bool IsAutoplayingMuted(); On 2017/04/19 at 14:09:56, mlamouri wrote: > Could this be `const`? Done. https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:58: bool IsLockedPendingUserGesture() const; On 2017/04/19 at 14:09:56, mlamouri wrote: > I can't see this being called outside of this class. Moved to private https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:60: bool IsLockedPendingUserGestureIfCrossOriginExperimentEnabled() const; On 2017/04/19 at 14:09:55, mlamouri wrote: > ditto Moved to private https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:76: bool IsGestureNeededForPlaybackIfCrossOriginExperimentEnabled() const; On 2017/04/19 at 14:09:55, mlamouri wrote: > This doesn't seem to be called by anything outside of `AutoplayPolicy`. Does it have to be public? Moved to private https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:78: bool IsGestureNeededForPlaybackIfPendingUserGestureIsLocked() const; On 2017/04/19 at 14:09:56, mlamouri wrote: > Same here Moved to private https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:82: bool IsAutoplayAllowedPerSettings() const; On 2017/04/19 at 14:09:55, mlamouri wrote: > and here This one is used by AutoplayUmaHelper :) https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:86: void OnVisibilityChangedForAutoplay(bool is_visible); On 2017/04/19 at 14:09:56, mlamouri wrote: > and here :) Moved to private https://codereview.chromium.org/2813303005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:96: bool locked_pending_user_gesture_if_cross_origin_experiment_enabled_; On 2017/04/19 at 14:09:56, mlamouri wrote: > Maybe these bool should be initialised here? Done. Not sure if it matters as we initialize then in the ctor anyway.
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_...)
Looks great! lgtm with the tests passing :) https://codereview.chromium.org/2813303005/diff/110001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/media/AutoplayPolicy.cpp (right): https://codereview.chromium.org/2813303005/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/media/AutoplayPolicy.cpp:158: if (!IsEligibleForAutoplayMuted()) Maybe add a comment reminding the user that this only makes sense because of the `!IsGestureNeddedForPlayback` above? https://codereview.chromium.org/2813303005/diff/110001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/media/AutoplayPolicy.h (right): https://codereview.chromium.org/2813303005/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:61: bool IsAutoplayAllowedPerSettings() const; I think you can move this to |private|.
PTAL #7..#8 https://codereview.chromium.org/2813303005/diff/110001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/media/AutoplayPolicy.cpp (right): https://codereview.chromium.org/2813303005/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/media/AutoplayPolicy.cpp:122: bool was_autoplaying_muted = IsAutoplayingMuted(); I found why the tests are broken... Actually the muted attribute throughout this method is always true, as it is called before the muted attribute is set to new value. Then IsGestureNeededForPlayback() will return false because the muted attribute is true. PTAL the new code which fixed the issue. BTW, I'm a bit surprised I didn't see this in my local debug build, because the pause event will be sent out anyway when the media finishes. I assume the test timeout is longer than the media duration. Maybe we should increase the test file duration. https://codereview.chromium.org/2813303005/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/media/AutoplayPolicy.cpp:158: if (!IsEligibleForAutoplayMuted()) On 2017/04/20 at 14:37:27, mlamouri wrote: > Maybe add a comment reminding the user that this only makes sense because of the `!IsGestureNeddedForPlayback` above? Done. https://codereview.chromium.org/2813303005/diff/110001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/media/AutoplayPolicy.h (right): https://codereview.chromium.org/2813303005/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:61: bool IsAutoplayAllowedPerSettings() const; On 2017/04/20 at 14:37:27, mlamouri wrote: > I think you can move this to |private|. Done.
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2813303005/diff/130001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/media/AutoplayPolicy.h (right): https://codereview.chromium.org/2813303005/diff/130001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:48: bool IsAutoplayingMuted(bool muted) const; could you have |IsAutoplayingMutedInternal| with that `bool` so you don't clutter the HTMLMediaElement call with this?
On 2017/04/20 at 15:51:06, mlamouri wrote: > lgtm > > https://codereview.chromium.org/2813303005/diff/130001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/html/media/AutoplayPolicy.h (right): > > https://codereview.chromium.org/2813303005/diff/130001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/media/AutoplayPolicy.h:48: bool IsAutoplayingMuted(bool muted) const; > could you have |IsAutoplayingMutedInternal| with that `bool` so you don't clutter the HTMLMediaElement call with this? Done.
The CQ bit was checked by zqzhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2813303005/#ps150001 (title: "addressed nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
zqzhang@chromium.org changed reviewers: + esprehn@chromium.org
+esprehn: BUILD.gn
rs lgtm
The CQ bit was checked by zqzhang@chromium.org
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": 150001, "attempt_start_ts": 1492720698205030,
"parent_rev": "ac9a37e939ced07b8d5f4f74f4829013895f0cf4", "commit_rev":
"12d76adf4bb340d8e430a18c3a664940157800f6"}
Message was sent while issue was closed.
Description was changed from ========== [Blink>Media] Moving autoplay logic to AutoplayPolicy This CL moves autoplay code from HTMLMediaElement to the AutoplayPolicy class, which helps decoupling the autoplay logic from HTMLMediaElement. BUG=712606 ========== to ========== [Blink>Media] Moving autoplay logic to AutoplayPolicy This CL moves autoplay code from HTMLMediaElement to the AutoplayPolicy class, which helps decoupling the autoplay logic from HTMLMediaElement. BUG=712606 Review-Url: https://codereview.chromium.org/2813303005 Cr-Commit-Position: refs/heads/master@{#466137} Committed: https://chromium.googlesource.com/chromium/src/+/12d76adf4bb340d8e430a18c3a66... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:150001) as https://chromium.googlesource.com/chromium/src/+/12d76adf4bb340d8e430a18c3a66... |
