Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2490783002:
Refactor WebMediaPlayerDelegate interface. (Closed)
|
Created:
4 years, 1 month ago by sandersd (OOO until July 31) Modified:
3 years, 11 months ago CC:
apacible+watch_chromium.org, chromium-reviews, feature-media-reviews_chromium.org, miu+watch_chromium.org, xjz+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor WebMediaPlayerDelegate interface.
This is part one of two, it changes the interfaces but tries to
maintain most of the old behavior (and client implementations) as
possible. Part two will rewrite WMPI's logic in a simpler way.
This CL includes two fixes to WMPI's logic:
- We now become non-idle during seek, and so will not suspend
immediately after completing a seek.
- We now track non-suspended time after loading progress and will
resume upon foregrounding (pre-HaveFutureData).
BUG=622313
Review-Url: https://codereview.chromium.org/2490783002
Cr-Commit-Position: refs/heads/master@{#443767}
Committed: https://chromium.googlesource.com/chromium/src/+/35d2c3fe00d136c5cdc59f9703c47fba912e5c7b
Patch Set 1 #
Total comments: 3
Patch Set 2 : Tweak method names. #Patch Set 3 : Implement new interface. #Patch Set 4 : Behavior should now be identical to before changes. #Patch Set 5 : Rebase. #Patch Set 6 : Unit tests. #
Total comments: 42
Patch Set 7 : Update WMPA properly. #
Total comments: 37
Patch Set 8 : Address comments. #Patch Set 9 : Improve timer handling. #Patch Set 10 : Fix did_play_ to only be set once per play. #
Total comments: 9
Patch Set 11 : Clarify comments and names. #
Total comments: 8
Patch Set 12 : Requested changes. #Patch Set 13 : Rebase. #Patch Set 14 : Switch to EXPECT_TRUE/EXPECT_FALSE #Patch Set 15 : Update Android-only WebMediaPlayerMS test #Patch Set 16 : Rebase. #Patch Set 17 : Restore Histograms test functionality. #Patch Set 18 : Unit tests found a real bug! #Messages
Total messages: 84 (45 generated)
watk@chromium.org changed reviewers: + watk@chromium.org
https://codereview.chromium.org/2490783002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_delegate.h (right): https://codereview.chromium.org/2490783002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_delegate.h:53: virtual bool IsClosed() = 0; I wonder if all the {on,is}{hidden,shown} should have frame in the name: "IsFrameHidden". So it's clear it's not related to visibility on the page. https://codereview.chromium.org/2490783002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_delegate.h:69: virtual void DidPlay(int player_id, I like player_id a lot
https://codereview.chromium.org/2490783002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_delegate.h (right): https://codereview.chromium.org/2490783002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_delegate.h:53: virtual bool IsClosed() = 0; On 2016/11/08 23:34:47, watk wrote: > I wonder if all the {on,is}{hidden,shown} should have frame in the name: > "IsFrameHidden". So it's clear it's not related to visibility on the page. Done.
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
Looks good.
On 2016/11/08 23:48:17, DaleCurtis wrote: > Looks good. FYI: PS#4 should be identical behavior to before my changes. Following patchsets sill be easiest to understand relative to it.
Patchset #5 (id:80001) has been deleted
Description was changed from ========== Proposed delegate interface. BUG=622313 ========== to ========== Refactor WebMediaPlayerDelegate interface. This is part one of two, it changes the interfaces but tries to maintain most of the old behavior (an client implementations) as possible. Part two will rewrite WMPI's logic in a simpler way. This CL includes two fixes to WMPI's logic: - We now become non-idle during seek, and so will not suspend immediately after completing a seek. - We now track non-suspended time after loading progress and will resume upon foregrounding (pre-HaveFutureData). BUG=622313 ==========
I have decided to split this into two parts, this first part should be ready. PTAL.
The CQ bit was checked by sandersd@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
==========
Refactor WebMediaPlayerDelegate interface.
This is part one of two, it changes the interfaces but tries to maintain most of
the old behavior (an client implementations) as possible. Part two will rewrite
WMPI's logic in a simpler way.
This CL includes two fixes to WMPI's logic:
- We now become non-idle during seek, and so will not suspend immediately
after completing a seek.
- We now track non-suspended time after loading progress and will resume upon
foregrounding (pre-HaveFutureData).
BUG=622313
==========
to
==========
Refactor WebMediaPlayerDelegate interface.
This is part one of two, it changes the interfaces but tries to
maintain most of the old behavior (an client implementations) as
possible. Part two will rewrite WMPI's logic in a simpler way.
This CL includes two fixes to WMPI's logic:
- We now become non-idle during seek, and so will not suspend
immediately after completing a seek.
- We now track non-suspended time after loading progress and will
resume upon foregrounding (pre-HaveFutureData).
BUG=622313
==========
sandersd@chromium.org changed reviewers: + avayvod@chromium.org
avayvod@: Please review changes to background playback logic and UMA reporting. This is intended to be mostly a no-op, but some parts of the implementation are rather different.
Nice, I like it. My eyes glazed over half way through the tests though https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/android/webmediaplayer_android.cc:1254: // If we're idle or playing video, pause and release resources; audio only Not clear what "If we're idle" means any more. https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... File content/renderer/media/android/webmediaplayer_android.h (right): https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/android/webmediaplayer_android.h:219: void OnFrameShown() override; This is great https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:126: if (idle_player_map_.count(player_id) || stale_players_.count(player_id)) I just found out about base::ContainsValue yesterday. I haven't decided if it's better than just using count() though. https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:145: stale_players_.erase(it); erase(key_type) returns the number of elements removed so you can skip the find(). https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:215: if (observer) { At some point we should call these observers "players" probably. https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:271: // Cleanup idle players. Clean up https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:345: // with this same caveat). Why did you decide to not DCHECK that SetIdle isn't called? Because you'd have to do some weird accounting? https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... File content/renderer/media/renderer_webmediaplayer_delegate.h (right): https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.h:31: // An interface to collect WebMediaPlayer state changes and to fan out commands Not an interface https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.h:39: RendererWebMediaPlayerDelegate(content::RenderFrame* render_frame); Keep explicit? https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.h:52: bool has_video, You swapped audio and video in the implementation https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.h:98: void CleanupIdlePlayers(base::TimeDelta timeout); While you're changing this (:]) s/Cleanup/CleanUp/ because cleanup is not a verb. https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.h:112: // inactivity these players will be notified and become stale. I'm fine with stale, but I'm wondering if suspendable is better? https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.h:118: // the player is suspended. Should probably reword this now. "paused" and "suspended" seem like they should be "idle" and "stale". https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.h:122: // exceeded |idle_timeout_| since their last pause state. since they became idle? https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.h:125: // Clock used for calculating when players have expired. May be overridden expired? https://codereview.chromium.org/2490783002/diff/120001/media/blink/webmediapl... File media/blink/webmediaplayer_delegate.h (right): https://codereview.chromium.org/2490783002/diff/120001/media/blink/webmediapl... media/blink/webmediaplayer_delegate.h:99: // it has gone stale. Sounds a bit like SetIdle can move you from stale to idle. I think you should mention that IsStale() implies IsIdle(). https://codereview.chromium.org/2490783002/diff/120001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2490783002/diff/120001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:809: return preroll_attempt_duration < kLoadingToIdleTimeout; maybe this should be kPrerollAttemptTimeout now? https://codereview.chromium.org/2490783002/diff/120001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:829: // TODO(sandersd): Should this be on the same stack? It might be surprsing surprising https://codereview.chromium.org/2490783002/diff/120001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1999: if (delegate_ && delegate_->IsFrameHidden()) Seems like this should still call IsHidden? https://codereview.chromium.org/2490783002/diff/120001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (left): https://codereview.chromium.org/2490783002/diff/120001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:224: // TODO(sandersd): This should move into WebMediaPlayerDelegate. Can it not move in? https://codereview.chromium.org/2490783002/diff/120001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2490783002/diff/120001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:357: // since then. I don't think this is clear if you don't understand the issue. Maybe copy some of the comment at line 1370 LHS of wmpi.cc
https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... File content/renderer/media/renderer_webmediaplayer_delegate.h (right): https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.h:112: // inactivity these players will be notified and become stale. On 2016/12/10 00:01:30, watk wrote: > I'm fine with stale, but I'm wondering if suspendable is better? After reading through the whole thing stale has grown on me.
https://codereview.chromium.org/2490783002/diff/140001/content/renderer/media... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/2490783002/diff/140001/content/renderer/media... content/renderer/media/android/webmediaplayer_android.cc:300: (IsBackgroundVideoCandidate() && delegate_ && nit: isBackgroundVideoCandidate will already check everything but delegate_->IsBackgroundVideoPlaybackAllowed(), should you remove the redundant conditions then? https://codereview.chromium.org/2490783002/diff/140001/content/renderer/media... File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2490783002/diff/140001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:55: is_frame_closed_; nit: I wonder if that's useful, esp. since you seem to always check for IsFrameHidden() && !IsFrameClosed() except for one case. Seems like this could check && !is_frame_closed_ (with a rename to IsFrameHiddenAndAlive(), maybe?) and maybe we don't need to report the metric if the frame is closed? https://codereview.chromium.org/2490783002/diff/140001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:204: if (is_hidden != is_frame_hidden_for_testing_) { nit: early return seems to be the preferred way in the media codebase IIUC https://codereview.chromium.org/2490783002/diff/140001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:206: background_video_allowed_ &= is_hidden; nit: why is this needed here? maybe the test can update the bool in a different way? https://codereview.chromium.org/2490783002/diff/140001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:265: bool did_play = did_play_; nit: the name is too similar to did_play_. did_play_before, old_did_play_value? https://codereview.chromium.org/2490783002/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2490783002/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1910: background_suspended && can_play_backgrounded; hm, I don't think background_suspended will ever be true at the same time as can_play_backgrounded now, will it? before it would be if can_play_backgrounded was true while is_background_playing was false. https://codereview.chromium.org/2490783002/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_impl_unittest.cc (right): https://codereview.chromium.org/2490783002/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl_unittest.cc:443: // TODO(sandersd): Make sure both variants are tested. nit: the enabled version should be tested on Android at least, I don't recall why it didn't work on desktop at the time I added this but IIRC it didn't, hence the if instead of two cases where one sets the flag to true and another to false. maybe worth trying again?
dalecurtis@chromium.org changed reviewers: - dalecurtis@chromium.org
Not going to have time to review this; so deferring to watk@ and avayvod@.
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org, tguilbert@chromium.org
actually, +tguilbert since some of this will be important for MediaPlayer integration later.
dalecurtis@chromium.org changed reviewers: - dalecurtis@chromium.org
LGTM with minor comments. https://codereview.chromium.org/2490783002/diff/140001/content/renderer/media... File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2490783002/diff/140001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:146: idle_player_map_[player_id] = tick_clock_->NowTicks() - idle_timeout_; Can you add a simple comment that explains why it isn't NowTicks() directly? Is it expected/ok to fall back to staleness almost immediately after clearing the stale flag? https://codereview.chromium.org/2490783002/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2490783002/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:829: // TODO(sandersd): Should this be on the same stack? It might be surprsing Typo: surprising https://codereview.chromium.org/2490783002/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_impl_unittest.cc (right): https://codereview.chromium.org/2490783002/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl_unittest.cc:338: IdleSuspendIsDisabledIfLoadingProgressedRecently) { didLoadingProgress() is the only method that calls ClearStaleFlag(). Is there worth in exercising that path?
Description was changed from
==========
Refactor WebMediaPlayerDelegate interface.
This is part one of two, it changes the interfaces but tries to
maintain most of the old behavior (an client implementations) as
possible. Part two will rewrite WMPI's logic in a simpler way.
This CL includes two fixes to WMPI's logic:
- We now become non-idle during seek, and so will not suspend
immediately after completing a seek.
- We now track non-suspended time after loading progress and will
resume upon foregrounding (pre-HaveFutureData).
BUG=622313
==========
to
==========
Refactor WebMediaPlayerDelegate interface.
This is part one of two, it changes the interfaces but tries to
maintain most of the old behavior (and client implementations) as
possible. Part two will rewrite WMPI's logic in a simpler way.
This CL includes two fixes to WMPI's logic:
- We now become non-idle during seek, and so will not suspend
immediately after completing a seek.
- We now track non-suspended time after loading progress and will
resume upon foregrounding (pre-HaveFutureData).
BUG=622313
==========
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
Had some time to go over this today. It's a bit worrying this deletes so many tests. It's not obvious that they've been carried over in a new form. https://codereview.chromium.org/2490783002/diff/140001/content/renderer/media... File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2490783002/diff/140001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:73: id_map_.Remove(player_id); Looks fairly gnarly now. I wonder if we should just have a player_id->struct map instead. https://codereview.chromium.org/2490783002/diff/140001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:126: if (idle_player_map_.count(player_id) || stale_players_.count(player_id)) IsIdle()? https://codereview.chromium.org/2490783002/diff/140001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:183: IPC_MESSAGE_UNHANDLED(return false) Neat, didn't know this could be done. https://codereview.chromium.org/2490783002/diff/140001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:250: if (!pending_update_task_) { || idle_timer_.IsRunning() ? Though I guess that might harm aggressive cleanup. It's a bit weird that we manually post the task in some cases and let the timer take care of it in others. https://codereview.chromium.org/2490783002/diff/140001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:338: // If we fail to erase, it means that the player called SetIdle(false) Hmm, if this is an API violation should it be a CHECK()? https://codereview.chromium.org/2490783002/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_delegate.h (right): https://codereview.chromium.org/2490783002/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_delegate.h:94: // Set the player's idle state. While idle, a player may recieve an Seems this is always called after one of the above methods? Should idleness instead be a bit on all of those calls? https://codereview.chromium.org/2490783002/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_delegate.h:98: // Get the player's idle state. A player is idle after SetIdle(true), even if Unused? https://codereview.chromium.org/2490783002/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_delegate.h:102: // Returns a stale player to an idle state, and resumes OnIdleTimeout() calls Hmm, don't really love exposing the stale concept outside of the delegate. If you roll idleness into player methods and let IsIdle == IsStale (as it exists today), does that solve the use cases?
https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/android/webmediaplayer_android.cc:1254: // If we're idle or playing video, pause and release resources; audio only On 2016/12/10 00:01:30, watk wrote: > Not clear what "If we're idle" means any more. Reworded. https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:126: if (idle_player_map_.count(player_id) || stale_players_.count(player_id)) On 2016/12/10 00:01:30, watk wrote: > I just found out about base::ContainsValue yesterday. I haven't decided if it's > better than just using count() though. Acknowledged. https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:145: stale_players_.erase(it); On 2016/12/10 00:01:30, watk wrote: > erase(key_type) returns the number of elements removed so you can skip the > find(). Done. https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:215: if (observer) { On 2016/12/10 00:01:30, watk wrote: > At some point we should call these observers "players" probably. Indeed we should. I'm going to skip that right now to avoid making this CL any bigger. https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:271: // Cleanup idle players. On 2016/12/10 00:01:30, watk wrote: > Clean up Done. https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:345: // with this same caveat). On 2016/12/10 00:01:30, watk wrote: > Why did you decide to not DCHECK that SetIdle isn't called? Because you'd have > to do some weird accounting? Basically yes. I came up with something I think is better, PTAL. https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... File content/renderer/media/renderer_webmediaplayer_delegate.h (right): https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.h:31: // An interface to collect WebMediaPlayer state changes and to fan out commands On 2016/12/10 00:01:30, watk wrote: > Not an interface Done. https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.h:39: RendererWebMediaPlayerDelegate(content::RenderFrame* render_frame); On 2016/12/10 00:01:31, watk wrote: > Keep explicit? Done. https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.h:52: bool has_video, On 2016/12/10 00:01:30, watk wrote: > You swapped audio and video in the implementation Done. https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.h:98: void CleanupIdlePlayers(base::TimeDelta timeout); On 2016/12/10 00:01:31, watk wrote: > While you're changing this (:]) s/Cleanup/CleanUp/ because cleanup is not a > verb. Done. https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.h:112: // inactivity these players will be notified and become stale. On 2016/12/10 00:12:30, watk wrote: > On 2016/12/10 00:01:30, watk wrote: > > I'm fine with stale, but I'm wondering if suspendable is better? > > After reading through the whole thing stale has grown on me. Acknowledged. https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.h:118: // the player is suspended. On 2016/12/10 00:01:31, watk wrote: > Should probably reword this now. "paused" and "suspended" seem like they should > be "idle" and "stale". Done. https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.h:122: // exceeded |idle_timeout_| since their last pause state. On 2016/12/10 00:01:31, watk wrote: > since they became idle? Done. https://codereview.chromium.org/2490783002/diff/120001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.h:125: // Clock used for calculating when players have expired. May be overridden On 2016/12/10 00:01:30, watk wrote: > expired? Done. https://codereview.chromium.org/2490783002/diff/120001/media/blink/webmediapl... File media/blink/webmediaplayer_delegate.h (right): https://codereview.chromium.org/2490783002/diff/120001/media/blink/webmediapl... media/blink/webmediaplayer_delegate.h:99: // it has gone stale. On 2016/12/10 00:01:31, watk wrote: > Sounds a bit like SetIdle can move you from stale to idle. I think you should > mention that IsStale() implies IsIdle(). Done. https://codereview.chromium.org/2490783002/diff/120001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2490783002/diff/120001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:809: return preroll_attempt_duration < kLoadingToIdleTimeout; On 2016/12/10 00:01:31, watk wrote: > maybe this should be kPrerollAttemptTimeout now? Done. https://codereview.chromium.org/2490783002/diff/120001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:829: // TODO(sandersd): Should this be on the same stack? It might be surprsing On 2016/12/10 00:01:31, watk wrote: > surprising Done. https://codereview.chromium.org/2490783002/diff/120001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1999: if (delegate_ && delegate_->IsFrameHidden()) On 2016/12/10 00:01:31, watk wrote: > Seems like this should still call IsHidden? I'm not sure. I think that means that we could report 'Shown' on tab closing, which sounds undesirable. I'd rather report 'Hidden', as the new code does. Of course the best case is that we report 'ended', but I didn't check that we do that correctly in every path. https://codereview.chromium.org/2490783002/diff/120001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (left): https://codereview.chromium.org/2490783002/diff/120001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:224: // TODO(sandersd): This should move into WebMediaPlayerDelegate. On 2016/12/10 00:01:31, watk wrote: > Can it not move in? It's not really necessary any more, separating the state into multiple types fixed the main issues. https://codereview.chromium.org/2490783002/diff/120001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2490783002/diff/120001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:357: // since then. On 2016/12/10 00:01:31, watk wrote: > I don't think this is clear if you don't understand the issue. Maybe copy some > of the comment at line 1370 LHS of wmpi.cc Done. https://codereview.chromium.org/2490783002/diff/140001/content/renderer/media... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/2490783002/diff/140001/content/renderer/media... content/renderer/media/android/webmediaplayer_android.cc:300: (IsBackgroundVideoCandidate() && delegate_ && On 2016/12/12 18:11:23, whywhat wrote: > nit: isBackgroundVideoCandidate will already check everything but > delegate_->IsBackgroundVideoPlaybackAllowed(), should you remove the redundant > conditions then? Done. https://codereview.chromium.org/2490783002/diff/140001/content/renderer/media... File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2490783002/diff/140001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:55: is_frame_closed_; On 2016/12/12 18:11:23, whywhat wrote: > nit: I wonder if that's useful, esp. since you seem to always check for > IsFrameHidden() && !IsFrameClosed() except for one case. > Seems like this could check && !is_frame_closed_ (with a rename to > IsFrameHiddenAndAlive(), maybe?) and maybe we don't need to report the metric if > the frame is closed? I thought this too, but I found the calling code to be more clear when the closed state is explicitly tested for. I can be convinced otherwise if you feel strongly. https://codereview.chromium.org/2490783002/diff/140001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:73: id_map_.Remove(player_id); On 2016/12/20 22:34:04, DaleCurtis_OOO_Dec14_Jan6 wrote: > Looks fairly gnarly now. I wonder if we should just have a player_id->struct map > instead. Probably a good idea, each player can just have a state or some flags, and we can walk the whole list each time. The code is easier to write with all these indexes, but it's certainly not simpler overall. I'd prefer to put this off until next time I make nontrivial changes here though, given how long this CL has been in progress. https://codereview.chromium.org/2490783002/diff/140001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:126: if (idle_player_map_.count(player_id) || stale_players_.count(player_id)) On 2016/12/20 22:34:04, DaleCurtis_OOO_Dec14_Jan6 wrote: > IsIdle()? Done. https://codereview.chromium.org/2490783002/diff/140001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:146: idle_player_map_[player_id] = tick_clock_->NowTicks() - idle_timeout_; On 2016/12/15 22:01:06, tguilbert wrote: > Can you add a simple comment that explains why it isn't NowTicks() directly? Is > it expected/ok to fall back to staleness almost immediately after clearing the > stale flag? Done. https://codereview.chromium.org/2490783002/diff/140001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:183: IPC_MESSAGE_UNHANDLED(return false) On 2016/12/20 22:34:04, DaleCurtis_OOO_Dec14_Jan6 wrote: > Neat, didn't know this could be done. Yeah, I saw this elsewhere and felt foolish for the bools I've used in the past. https://codereview.chromium.org/2490783002/diff/140001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:204: if (is_hidden != is_frame_hidden_for_testing_) { On 2016/12/12 18:11:23, whywhat wrote: > nit: early return seems to be the preferred way in the media codebase IIUC Done. https://codereview.chromium.org/2490783002/diff/140001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:206: background_video_allowed_ &= is_hidden; On 2016/12/12 18:11:23, whywhat wrote: > nit: why is this needed here? maybe the test can update the bool in a different > way? Done. (Assuming that just writing it more explicitly is clear enough.) https://codereview.chromium.org/2490783002/diff/140001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:250: if (!pending_update_task_) { On 2016/12/20 22:34:04, DaleCurtis_OOO_Dec14_Jan6 wrote: > || idle_timer_.IsRunning() ? Though I guess that might harm aggressive cleanup. > It's a bit weird that we manually post the task in some cases and let the timer > take care of it in others. I would say that the timer just so happens to run exactly the same logic as the update task. I'm not sure how to write a comment that expresses that intent without making it sound more complicated than it is, though. https://codereview.chromium.org/2490783002/diff/140001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:265: bool did_play = did_play_; On 2016/12/12 18:11:23, whywhat wrote: > nit: the name is too similar to did_play_. did_play_before, old_did_play_value? Done. https://codereview.chromium.org/2490783002/diff/140001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:338: // If we fail to erase, it means that the player called SetIdle(false) On 2016/12/20 22:34:04, DaleCurtis_OOO_Dec14_Jan6 wrote: > Hmm, if this is an API violation should it be a CHECK()? I've replaced this logic entirely. The addition of ClearStaleFlag() provided a much cleaner option for WMPI, which was the only user. Now IsStale() returns true inside of OnIdleTimeout(), which is a much better model. https://codereview.chromium.org/2490783002/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_delegate.h (right): https://codereview.chromium.org/2490783002/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_delegate.h:94: // Set the player's idle state. While idle, a player may recieve an On 2016/12/20 22:34:04, DaleCurtis wrote: > Seems this is always called after one of the above methods? Should idleness > instead be a bit on all of those calls? There is one counterexample, webmediaplayer_ms calls PlayerGone() without calling SetIdle(). We could combine them as you say and then have webmediaplayer_ms use PlayerGone(IsIdle()). WDYT? https://codereview.chromium.org/2490783002/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_delegate.h:98: // Get the player's idle state. A player is idle after SetIdle(true), even if On 2016/12/20 22:34:05, DaleCurtis wrote: > Unused? WMPI uses this in one case to basically result in SetIdle(IsIdle()). After the planned followup to improve WMPI's computation, it should indeed be entirely unused and will be removed. (Depending on the outcome of SetIdle() above.) https://codereview.chromium.org/2490783002/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_delegate.h:102: // Returns a stale player to an idle state, and resumes OnIdleTimeout() calls On 2016/12/20 22:34:04, DaleCurtis wrote: > Hmm, don't really love exposing the stale concept outside of the delegate. If > you roll idleness into player methods and let IsIdle == IsStale (as it exists > today), does that solve the use cases? (See above.) https://codereview.chromium.org/2490783002/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2490783002/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:829: // TODO(sandersd): Should this be on the same stack? It might be surprsing On 2016/12/15 22:01:06, tguilbert wrote: > Typo: surprising Done. https://codereview.chromium.org/2490783002/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1910: background_suspended && can_play_backgrounded; On 2016/12/12 18:11:23, whywhat wrote: > hm, I don't think background_suspended will ever be true at the same time as > can_play_backgrounded now, will it? before it would be if can_play_backgrounded > was true while is_background_playing was false. Original computation restored. https://codereview.chromium.org/2490783002/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_impl_unittest.cc (right): https://codereview.chromium.org/2490783002/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl_unittest.cc:338: IdleSuspendIsDisabledIfLoadingProgressedRecently) { On 2016/12/15 22:01:06, tguilbert wrote: > didLoadingProgress() is the only method that calls ClearStaleFlag(). Is there > worth in exercising that path? That path is exercised by DidLoadingProgressTriggersResume. https://codereview.chromium.org/2490783002/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl_unittest.cc:443: // TODO(sandersd): Make sure both variants are tested. On 2016/12/12 18:11:23, whywhat wrote: > nit: the enabled version should be tested on Android at least, I don't recall > why it didn't work on desktop at the time I added this but IIRC it didn't, hence > the if instead of two cases where one sets the flag to true and another to > false. maybe worth trying again? Done.
https://codereview.chromium.org/2490783002/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_delegate.h (right): https://codereview.chromium.org/2490783002/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_delegate.h:94: // Set the player's idle state. While idle, a player may recieve an On 2017/01/05 at 23:12:21, sandersd wrote: > On 2016/12/20 22:34:04, DaleCurtis wrote: > > Seems this is always called after one of the above methods? Should idleness > > instead be a bit on all of those calls? > > There is one counterexample, webmediaplayer_ms calls PlayerGone() without calling SetIdle(). We could combine them as you say and then have webmediaplayer_ms use PlayerGone(IsIdle()). WDYT? Does that make sense? Isn't IsIdle() always true when PlayerGone() is called?
On 2017/01/05 23:20:36, DaleCurtis wrote: > https://codereview.chromium.org/2490783002/diff/140001/media/blink/webmediapl... > File media/blink/webmediaplayer_delegate.h (right): > > https://codereview.chromium.org/2490783002/diff/140001/media/blink/webmediapl... > media/blink/webmediaplayer_delegate.h:94: // Set the player's idle state. While > idle, a player may recieve an > On 2017/01/05 at 23:12:21, sandersd wrote: > > On 2016/12/20 22:34:04, DaleCurtis wrote: > > > Seems this is always called after one of the above methods? Should idleness > > > instead be a bit on all of those calls? > > > > There is one counterexample, webmediaplayer_ms calls PlayerGone() without > calling SetIdle(). We could combine them as you say and then have > webmediaplayer_ms use PlayerGone(IsIdle()). WDYT? > > Does that make sense? Isn't IsIdle() always true when PlayerGone() is called? I'm not sure if it makes sense, but it does exactly preserve the previous behavior. (PlayerGone() didn't do much before.) It looks like WebMediaPlayerMS will always pause media in this specific case, so we can hard code the idle flag to true here if that's the direction we decide to go.
lgtm
looking again with a fresh head - added some nits https://codereview.chromium.org/2490783002/diff/200001/content/renderer/media... File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2490783002/diff/200001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:54: return render_frame()->IsHidden() || is_frame_hidden_for_testing_ || nit: I'd check is_frame_hidden_for_testing_ separately and early to allow having a test RWMPD with a null render frame and also to separate the test hook from the actual logic. https://codereview.chromium.org/2490783002/diff/200001/media/blink/webmediapl... File media/blink/webmediaplayer_delegate.h (right): https://codereview.chromium.org/2490783002/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_delegate.h:56: virtual bool IsFrameHidden() = 0; nit: say that IsFrameHidden also considers closed frame to be hidden? it's now a bit inconsistent with OnFrameHidden() which will not get called when IsFrameHidden becomes true (OnFrameClosed will be called though). Perhaps, my earlier suggestion of making IsFrameClosed part of IsFrameHidden was bad and the original version was cleaner. https://codereview.chromium.org/2490783002/diff/200001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2490783002/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1363: void WebMediaPlayerImpl::OnFrameHidden() { nit: Following discussion with Dale around naming for visibility in https://codereview.chromium.org/2552493002 -- maybe OnForeground(ed)/OnBackground(ed)? https://codereview.chromium.org/2490783002/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1878: delegate_ && delegate_->IsBackgroundVideoPlaybackAllowed(); nit: I'm a bit concerned that IsBgVideoPlaybackAllowed can be confused with IsResumeBgVideosEnabled() (used above). The latter is a feature flag (which we should just remove and replace with true in a cleanup CL), the former is a per player bit set everytime the video is resumed in bg by the user and reset every time the video comes to the foreground (it's similar to a user gesture lock but bg specific). So maybe IsBgVideoPlaybackUnlocked() ?
https://codereview.chromium.org/2490783002/diff/200001/content/renderer/media... File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2490783002/diff/200001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:54: return render_frame()->IsHidden() || is_frame_hidden_for_testing_ || On 2017/01/06 17:18:51, whywhat wrote: > nit: I'd check is_frame_hidden_for_testing_ separately and early to allow having > a test RWMPD with a null render frame and also to separate the test hook from > the actual logic. Done. https://codereview.chromium.org/2490783002/diff/200001/media/blink/webmediapl... File media/blink/webmediaplayer_delegate.h (right): https://codereview.chromium.org/2490783002/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_delegate.h:56: virtual bool IsFrameHidden() = 0; On 2017/01/06 17:18:51, whywhat wrote: > nit: say that IsFrameHidden also considers closed frame to be hidden? it's now a > bit inconsistent with OnFrameHidden() which will not get called when > IsFrameHidden becomes true (OnFrameClosed will be called though). Perhaps, my > earlier suggestion of making IsFrameClosed part of IsFrameHidden was bad and the > original version was cleaner. Done. https://codereview.chromium.org/2490783002/diff/200001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2490783002/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1363: void WebMediaPlayerImpl::OnFrameHidden() { On 2017/01/06 17:18:52, whywhat wrote: > nit: Following discussion with Dale around naming for visibility in > https://codereview.chromium.org/2552493002 -- maybe > OnForeground(ed)/OnBackground(ed)? I think I prefer to keep these names, since they are render frame concepts with the same meaning. I'll wait for Dale to comment, though ;-) The point is well taken though, we've got a bunch of related concepts with confusingly similar names. (And some outright collisions: suspend/resume is also used for media element network state.) https://codereview.chromium.org/2490783002/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1878: delegate_ && delegate_->IsBackgroundVideoPlaybackAllowed(); On 2017/01/06 17:18:51, whywhat wrote: > nit: I'm a bit concerned that IsBgVideoPlaybackAllowed can be confused with > IsResumeBgVideosEnabled() (used above). > The latter is a feature flag (which we should just remove and replace with true > in a cleanup CL), the former is a per player bit set everytime the video is > resumed in bg by the user and reset every time the video comes to the foreground > (it's similar to a user gesture lock but bg specific). > > So maybe IsBgVideoPlaybackUnlocked() ? Done.
The CQ bit was checked by sandersd@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...)
https://codereview.chromium.org/2490783002/diff/200001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2490783002/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1363: void WebMediaPlayerImpl::OnFrameHidden() { On 2017/01/06 at 23:08:35, sandersd wrote: > On 2017/01/06 17:18:52, whywhat wrote: > > nit: Following discussion with Dale around naming for visibility in > > https://codereview.chromium.org/2552493002 -- maybe > > OnForeground(ed)/OnBackground(ed)? > > I think I prefer to keep these names, since they are render frame concepts with the same meaning. I'll wait for Dale to comment, though ;-) > > The point is well taken though, we've got a bunch of related concepts with confusingly similar names. (And some outright collisions: suspend/resume is also used for media element network state.) Saving the rename for later sgtm.
There's still quite a few tests wholesale removed in this CL. Can you bring those back or explain further why they were removed? https://codereview.chromium.org/2490783002/diff/220001/content/renderer/media... File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2490783002/diff/220001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:54: if (is_frame_hidden_for_testing_) Hmm, this should be tripping the presubmit. I didn't think you could use for_testing_ variables like this. https://codereview.chromium.org/2490783002/diff/220001/content/renderer/media... File content/renderer/media/renderer_webmediaplayer_delegate.h (right): https://codereview.chromium.org/2490783002/diff/220001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.h:52: bool has_audio, Why did you reorder these? The order matches that in the delegate message and elsewhere. If anything we should just replace this with a enum instead of reordering. https://codereview.chromium.org/2490783002/diff/220001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.h:79: friend class RendererWebMediaPlayerDelegateTest; Do we need all these ForTesting() methods if the test class is a friend? https://codereview.chromium.org/2490783002/diff/220001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.h:106: bool did_play_video_ = false; has_played_video_ for consistency?
> There's still quite a few tests wholesale removed in this CL. Can you > bring those back or explain further why they were removed? As discussed, most of those tests were in fact preserved, just shuffled around a bit. I did discover that the background playback browsertest cases were not restored when I undid a refactor there, that test has been restored now. (And the playing video set test was removed, the definition of playing video has changed and doesn't affect anything useful to test anymore.) https://codereview.chromium.org/2490783002/diff/220001/content/renderer/media... File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2490783002/diff/220001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:54: if (is_frame_hidden_for_testing_) On 2017/01/10 21:28:23, DaleCurtis wrote: > Hmm, this should be tripping the presubmit. I didn't think you could use > for_testing_ variables like this. It appears this is allowed. Do you have a preferred pattern for this? https://codereview.chromium.org/2490783002/diff/220001/content/renderer/media... File content/renderer/media/renderer_webmediaplayer_delegate.h (right): https://codereview.chromium.org/2490783002/diff/220001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.h:52: bool has_audio, On 2017/01/10 21:28:23, DaleCurtis wrote: > Why did you reorder these? The order matches that in the delegate message and > elsewhere. If anything we should just replace this with a enum instead of > reordering. Done. https://codereview.chromium.org/2490783002/diff/220001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.h:79: friend class RendererWebMediaPlayerDelegateTest; On 2017/01/10 21:28:23, DaleCurtis wrote: > Do we need all these ForTesting() methods if the test class is a friend? I suppose not; at the very least they can be made private. I do prefer this style though, rather than making boilerplate wrapper functions in the test class. Do you have a strong preference? https://codereview.chromium.org/2490783002/diff/220001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.h:106: bool did_play_video_ = false; On 2017/01/10 21:28:23, DaleCurtis wrote: > has_played_video_ for consistency? Done.
Thanks, lgtm!
The CQ bit was checked by sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tguilbert@chromium.org, avayvod@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2490783002/#ps260001 (title: "Rebase.")
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org, tguilbert@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2490783002/#ps280001 (title: "Switch to EXPECT_TRUE/EXPECT_FALSE")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Patchset #15 (id:300001) has been deleted
The CQ bit was checked by sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org, tguilbert@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2490783002/#ps320001 (title: "Update Android-only WebMediaPlayerMS test")
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org, tguilbert@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2490783002/#ps340001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #16 (id:340001) has been deleted
The CQ bit was checked by sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org, tguilbert@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2490783002/#ps360001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org, tguilbert@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2490783002/#ps380001 (title: "Restore Histograms test functionality.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sandersd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_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 sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org, tguilbert@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2490783002/#ps400001 (title: "Unit tests found a real bug!")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #18 (id:400001) has been deleted
The CQ bit was checked by sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org, tguilbert@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2490783002/#ps420001 (title: "Unit tests found a real bug!")
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": 420001, "attempt_start_ts": 1484354806760050,
"parent_rev": "aa96a2c8835f1681fa482a9b31316d2f3cd2ccf6", "commit_rev":
"35d2c3fe00d136c5cdc59f9703c47fba912e5c7b"}
Message was sent while issue was closed.
Description was changed from
==========
Refactor WebMediaPlayerDelegate interface.
This is part one of two, it changes the interfaces but tries to
maintain most of the old behavior (and client implementations) as
possible. Part two will rewrite WMPI's logic in a simpler way.
This CL includes two fixes to WMPI's logic:
- We now become non-idle during seek, and so will not suspend
immediately after completing a seek.
- We now track non-suspended time after loading progress and will
resume upon foregrounding (pre-HaveFutureData).
BUG=622313
==========
to
==========
Refactor WebMediaPlayerDelegate interface.
This is part one of two, it changes the interfaces but tries to
maintain most of the old behavior (and client implementations) as
possible. Part two will rewrite WMPI's logic in a simpler way.
This CL includes two fixes to WMPI's logic:
- We now become non-idle during seek, and so will not suspend
immediately after completing a seek.
- We now track non-suspended time after loading progress and will
resume upon foregrounding (pre-HaveFutureData).
BUG=622313
Review-Url: https://codereview.chromium.org/2490783002
Cr-Commit-Position: refs/heads/master@{#443767}
Committed:
https://chromium.googlesource.com/chromium/src/+/35d2c3fe00d136c5cdc59f9703c4...
==========
Message was sent while issue was closed.
Committed patchset #18 (id:420001) as https://chromium.googlesource.com/chromium/src/+/35d2c3fe00d136c5cdc59f9703c4... |
