Make the requirements for activing VideoWakeLock stricter. [chromium/src : main]
Dale Curtis (Gerrit)
Attention is currently required from: Frank Liberato, Yoav Weiss.
Dale Curtis would like Frank Liberato and Yoav Weiss to review this change.
Make the requirements for activing VideoWakeLock stricter.
We've received several complaints that the video wake lock is being
held too liberally. E.g., for looping small emoji/videos and minor
video ads.
As such start enforcing stricter requirements:
- At least 75% of the video must be on screen.
- The video must take up at least 20% of the viewport.
Since visible wake lock restrictions launched in M84, this also
removes the stale RuntimeFeature entry for it. The new behavior is
behind a default on base::Feature flag per new feature policy (so
it can be turned off in the field if needed).
Fixed: 1340424
Change-Id: I70896baffb0b7b791f6dd29e0b99e8df4126bceb
---
M content/child/runtime_features.cc
M media/base/media_switches.cc
M media/base/media_switches.h
M third_party/blink/renderer/core/html/media/video_wake_lock.cc
M third_party/blink/renderer/core/html/media/video_wake_lock.h
M third_party/blink/renderer/core/html/media/video_wake_lock_test.cc
M third_party/blink/renderer/platform/runtime_enabled_features.json5
7 files changed, 236 insertions(+), 101 deletions(-)
To view, visit change 3964046. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Frank Liberato, Yoav Weiss.
Patch set 6:Commit-Queue +1
Frank Liberato (Gerrit)
Attention is currently required from: Dale Curtis, Yoav Weiss.
4 comments:
File third_party/blink/renderer/core/html/media/video_wake_lock.h:
Patch Set #7, Line 111:
float visibility_threshold_ = 0.75f;you might be able to make this const since it's only modified in the ctor.
Patch Set #7, Line 115:
float size_threshold_ = 0.2f;i didn't see anything that modified this -- const / static constexpr?
File third_party/blink/renderer/core/html/media/video_wake_lock.cc:
Patch Set #7, Line 166:
: has_volume;i don't understand why this depends on the feature rather than always being the "then" clause. if there's no audio, why care if it has volume? or is this a spec thing?
Patch Set #7, Line 175:
(page_visible && ((is_visible_ && is_big_enough_) || has_audio)));just to make sure that i understand the intended behavior -- background tabs get no wake lock even if it has audio?
To view, visit change 3964046. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Frank Liberato, Yoav Weiss.
2 comments:
File third_party/blink/renderer/core/html/media/video_wake_lock.cc:
Patch Set #7, Line 166:
: has_volume;
i don't understand why this depends on the feature rather than always being the "then" clause. […]
I'm just trying to be safe and preserve the existing behavior behind a flag that we can turn off in case of stable regression.
Patch Set #7, Line 175:
(page_visible && ((is_visible_ && is_big_enough_) || has_audio)));
just to make sure that i understand the intended behavior -- background tabs get no wake lock even i […]
There are two wake locks a video one and an audio one, the video one keeps the screen on and prevents sleep while the audio one prevents sleep. So yes background videos shouldn't keep the screen on.
To view, visit change 3964046. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Dale Curtis, Frank Liberato.
1 comment:
Patchset:
Can you split the stable flag removal from the behavior change?
To view, visit change 3964046. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Frank Liberato, Yoav Weiss.
Dale Curtis has uploaded this change for review.
Make the requirements for activing VideoWakeLock stricter.
We've received several complaints that the video wake lock is being
held too liberally. E.g., for looping small emoji/videos and minor
video ads.
As such start enforcing stricter requirements:
- At least 75% of the video must be on screen.
- The video must take up at least 20% of the viewport.
Since visible wake lock restrictions launched in M84, this also
removes the stale RuntimeFeature entry for it. The new behavior is
behind a default on base::Feature flag per new feature policy (so
it can be turned off in the field if needed).
Fixed: 1340424
Change-Id: I70896baffb0b7b791f6dd29e0b99e8df4126bceb
---
M third_party/blink/renderer/core/html/media/video_wake_lock.cc
M third_party/blink/renderer/core/html/media/video_wake_lock.h
M third_party/blink/renderer/core/html/media/video_wake_lock_test.cc
3 files changed, 278 insertions(+), 79 deletions(-)
Dale Curtis (Gerrit)
Attention is currently required from: Frank Liberato, Yoav Weiss.
2 comments:
Patchset:
Can you split the stable flag removal from the behavior change?
Patchset:
Moving yoav to cc since changes are all in media now.
To view, visit change 3964046. To unsubscribe, or for help writing mail filters, visit settings.