Suspend idle WebMediaPlayer instances after some time.

Suspend idle WebMediaPlayer instances after some time.

Releases resources which may slowly accumulate for carelessly created
players that a user will never see. For now, only enabled on Android,
but long term we'll want this on all platforms since it can avoid
major OOM crashes on sites which leak elements.

Since RendererWebMediaPlayerDelegate is now getting a bit complicated,
adds a browser test (necessary to construct a RenderViewTest) to cover
old and new functionality alike.

Currently players will be suspended after 15 seconds of being paused,
and resumed upon playback, playback rate changes from 0 to above 0,
and seek events.

Modifies WebMediaPlayerImpl to handle suspensions when a page might
be in the foreground. Covers the following:
- Seek will now resume the pipeline if initiated in the foreground.
- Play will now resume the pipeline if initiated in the foreground.

Due to the above it's no longer necessary for OnShown() to resume the
player when it's in the paused or ended state.

BUG=590099
TEST=new unittests, manual testing.

Committed: https://crrev.com/5bbc487ef9a375236c2b8ca9196d34c60d767323
Cr-Commit-Position: refs/heads/master@{#378113}

[DaleCurtis, 2016-02-26 02:39:32]

Patchset #1 (id:1) has been deleted

[DaleCurtis, 2016-02-26 02:39:43]

Description was changed from

==========
Suspend idle WebMediaPlayer instances after some time.

Releases resources which may slowly accumulate for carelessly created
players that a user will never see. For now, only enabled on Android,
but long term we'll want this on all platforms since it can avoid
major OOM crashes on sites which leak elements.

Since RendererWebMediaPlayerDelegate is now getting a bit complicated,
adds a browser test (necessary to construct a RenderViewTest) to cover
old and new functionality alike.

Currently players will be suspended after 15 seconds of being paused,
and resumed upon playback, playback rate changes from 0 to above 0,
and seek events.

BUG=tbd
TEST=new unittests, manual testing.
==========

to

==========
Suspend idle WebMediaPlayer instances after some time.

Releases resources which may slowly accumulate for carelessly created
players that a user will never see. For now, only enabled on Android,
but long term we'll want this on all platforms since it can avoid
major OOM crashes on sites which leak elements.

Since RendererWebMediaPlayerDelegate is now getting a bit complicated,
adds a browser test (necessary to construct a RenderViewTest) to cover
old and new functionality alike.

Currently players will be suspended after 15 seconds of being paused,
and resumed upon playback, playback rate changes from 0 to above 0,
and seek events.

Modifies WebMediaPlayerImpl to handle suspensions when a page might
be in the foreground. Covers the following:
- Seek will now resume the pipeline if initiated in the foreground.
- Play will now resume the pipeline if initiated in the foreground.

Also fixes the following:
- Power save blockers being held when play() is called from the
  background. Not a common issue since it would require users to
  run with --disable-gesture-requirement-for-media.

BUG=590099
TEST=new unittests, manual testing.
==========

[DaleCurtis, 2016-02-26 02:39:43]

dalecurtis@chromium.org changed reviewers:
+ sandersd@chromium.org

[DaleCurtis, 2016-02-26 02:40:34]

Do you see this integrating with your new changes?

[sandersd (OOO until July 31), 2016-02-27 00:22:20]

Please also handle |ended_| where necessary. In particular:
  - Does changing the rate matter when ended?
  - Don't resume in OnShown() when ended.

https://codereview.chromium.org/1739473003/diff/40001/content/renderer/media/...
File content/renderer/media/renderer_webmediaplayer_delegate.cc (right):

https://codereview.chromium.org/1739473003/diff/40001/content/renderer/media/...
content/renderer/media/renderer_webmediaplayer_delegate.cc:151: // using a
sentinel value of an empty TimeTicks.
Please comment on which thread methods run on such that this can be a problem.

https://codereview.chromium.org/1739473003/diff/40001/content/renderer/media/...
content/renderer/media/renderer_webmediaplayer_delegate.cc:168:
id_map_.Lookup(kv.first)->OnHidden(true);
I'm going to keep commenting that this is the wrong method and/or method name on
every CL.

https://codereview.chromium.org/1739473003/diff/40001/content/renderer/media/...
content/renderer/media/renderer_webmediaplayer_delegate.cc:174: it =
idle_delegate_map_.erase(it);
It looks like this can race with line 153.

https://codereview.chromium.org/1739473003/diff/40001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1739473003/diff/40001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:362: NotifyPlaybackStarted();
If you swap the order, then no test is needed because this will be a no-op while
suspended.

[sandersd (OOO until July 31), 2016-02-27 00:26:23]

https://codereview.chromium.org/1739473003/diff/40001/content/renderer/media/...
File content/renderer/media/renderer_webmediaplayer_delegate_browsertest.cc
(right):

https://codereview.chromium.org/1739473003/diff/40001/content/renderer/media/...
content/renderer/media/renderer_webmediaplayer_delegate_browsertest.cc:189:
base::TimeDelta());
What is the purpose of the third delegate? It seems to be the same as the first
test for delegate_id_1.

[DaleCurtis, 2016-02-27 00:54:10]

Description was changed from

==========
Suspend idle WebMediaPlayer instances after some time.

Releases resources which may slowly accumulate for carelessly created
players that a user will never see. For now, only enabled on Android,
but long term we'll want this on all platforms since it can avoid
major OOM crashes on sites which leak elements.

Since RendererWebMediaPlayerDelegate is now getting a bit complicated,
adds a browser test (necessary to construct a RenderViewTest) to cover
old and new functionality alike.

Currently players will be suspended after 15 seconds of being paused,
and resumed upon playback, playback rate changes from 0 to above 0,
and seek events.

Modifies WebMediaPlayerImpl to handle suspensions when a page might
be in the foreground. Covers the following:
- Seek will now resume the pipeline if initiated in the foreground.
- Play will now resume the pipeline if initiated in the foreground.

Also fixes the following:
- Power save blockers being held when play() is called from the
  background. Not a common issue since it would require users to
  run with --disable-gesture-requirement-for-media.

BUG=590099
TEST=new unittests, manual testing.
==========

to

==========
Suspend idle WebMediaPlayer instances after some time.

Releases resources which may slowly accumulate for carelessly created
players that a user will never see. For now, only enabled on Android,
but long term we'll want this on all platforms since it can avoid
major OOM crashes on sites which leak elements.

Since RendererWebMediaPlayerDelegate is now getting a bit complicated,
adds a browser test (necessary to construct a RenderViewTest) to cover
old and new functionality alike.

Currently players will be suspended after 15 seconds of being paused,
and resumed upon playback, playback rate changes from 0 to above 0,
and seek events.

Modifies WebMediaPlayerImpl to handle suspensions when a page might
be in the foreground. Covers the following:
- Seek will now resume the pipeline if initiated in the foreground.
- Play will now resume the pipeline if initiated in the foreground.

Due to the above it's no longer necessary for OnShown() to resume the
player when it's in the paused or ended state.

BUG=590099
TEST=new unittests, manual testing.
==========

[DaleCurtis, 2016-02-27 00:55:23]

Decided to leave rate 0 unchanged since it should rare and the extra state
management is annoying.

https://codereview.chromium.org/1739473003/diff/40001/content/renderer/media/...
File content/renderer/media/renderer_webmediaplayer_delegate.cc (right):

https://codereview.chromium.org/1739473003/diff/40001/content/renderer/media/...
content/renderer/media/renderer_webmediaplayer_delegate.cc:151: // using a
sentinel value of an empty TimeTicks.
On 2016/02/27 at 00:22:20, sandersd wrote:
> Please comment on which thread methods run on such that this can be a problem.


Added a comment below about how calls might trigger during idle cleanup.

https://codereview.chromium.org/1739473003/diff/40001/content/renderer/media/...
content/renderer/media/renderer_webmediaplayer_delegate.cc:168:
id_map_.Lookup(kv.first)->OnHidden(true);
On 2016/02/27 at 00:22:20, sandersd wrote:
> I'm going to keep commenting that this is the wrong method and/or method name
on every CL.


Ignoring per offline discussion.

https://codereview.chromium.org/1739473003/diff/40001/content/renderer/media/...
content/renderer/media/renderer_webmediaplayer_delegate.cc:174: it =
idle_delegate_map_.erase(it);
On 2016/02/27 at 00:22:20, sandersd wrote:
> It looks like this can race with line 153.

Ignoring per offline discussion.

https://codereview.chromium.org/1739473003/diff/40001/content/renderer/media/...
File content/renderer/media/renderer_webmediaplayer_delegate_browsertest.cc
(right):

https://codereview.chromium.org/1739473003/diff/40001/content/renderer/media/...
content/renderer/media/renderer_webmediaplayer_delegate_browsertest.cc:189:
base::TimeDelta());
On 2016/02/27 at 00:26:23, sandersd wrote:
> What is the purpose of the third delegate? It seems to be the same as the
first test for delegate_id_1.

The purpose of the third delegate is just to have multiple delegates outstanding
and to have tests with a delegate which has survived multiple gauntlets of idle
tests. Your question did make me realize this should also test if the timer is
actually stopped -- so added a test for that.

https://codereview.chromium.org/1739473003/diff/40001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1739473003/diff/40001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:362: NotifyPlaybackStarted();
On 2016/02/27 at 00:22:20, sandersd wrote:
> If you swap the order, then no test is needed because this will be a no-op
while suspended.

Done.

[sandersd (OOO until July 31), 2016-02-27 00:59:31]

lgtm

[DaleCurtis, 2016-02-27 02:11:30]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-27 02:12:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1739473003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1739473003/80001

[sandersd (OOO until July 31), 2016-02-27 02:52:06]

lgtm

[DaleCurtis, 2016-02-27 02:59:12]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2016-02-27 02:59:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/1739473003/#ps100001
(title: "NON_EXPORTED_BASE.")

[commit-bot: I haz the power, 2016-02-27 02:59:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1739473003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1739473003/100001

[commit-bot: I haz the power, 2016-02-27 04:15:14]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-02-27 04:17:54]

Description was changed from

==========
Suspend idle WebMediaPlayer instances after some time.

Releases resources which may slowly accumulate for carelessly created
players that a user will never see. For now, only enabled on Android,
but long term we'll want this on all platforms since it can avoid
major OOM crashes on sites which leak elements.

Since RendererWebMediaPlayerDelegate is now getting a bit complicated,
adds a browser test (necessary to construct a RenderViewTest) to cover
old and new functionality alike.

Currently players will be suspended after 15 seconds of being paused,
and resumed upon playback, playback rate changes from 0 to above 0,
and seek events.

Modifies WebMediaPlayerImpl to handle suspensions when a page might
be in the foreground. Covers the following:
- Seek will now resume the pipeline if initiated in the foreground.
- Play will now resume the pipeline if initiated in the foreground.

Due to the above it's no longer necessary for OnShown() to resume the
player when it's in the paused or ended state.

BUG=590099
TEST=new unittests, manual testing.
==========

to

==========
Suspend idle WebMediaPlayer instances after some time.

Releases resources which may slowly accumulate for carelessly created
players that a user will never see. For now, only enabled on Android,
but long term we'll want this on all platforms since it can avoid
major OOM crashes on sites which leak elements.

Since RendererWebMediaPlayerDelegate is now getting a bit complicated,
adds a browser test (necessary to construct a RenderViewTest) to cover
old and new functionality alike.

Currently players will be suspended after 15 seconds of being paused,
and resumed upon playback, playback rate changes from 0 to above 0,
and seek events.

Modifies WebMediaPlayerImpl to handle suspensions when a page might
be in the foreground. Covers the following:
- Seek will now resume the pipeline if initiated in the foreground.
- Play will now resume the pipeline if initiated in the foreground.

Due to the above it's no longer necessary for OnShown() to resume the
player when it's in the paused or ended state.

BUG=590099
TEST=new unittests, manual testing.

Committed: https://crrev.com/5bbc487ef9a375236c2b8ca9196d34c60d767323
Cr-Commit-Position: refs/heads/master@{#378113}
==========

[commit-bot: I haz the power, 2016-02-27 04:17:55]

Patchset 5 (id:??) landed as
https://crrev.com/5bbc487ef9a375236c2b8ca9196d34c60d767323
Cr-Commit-Position: refs/heads/master@{#378113}