Remove MediaThrottler and MediaThrottleInfoBar

Remove MediaThrottler and MediaThrottleInfoBar

Since the removal of WebMediaPlayerAndroid, we use the
MediaServiceThrottler instead of the MediaThrottler.

This CL cleans up the unused code. It also prevents the
instantiation of BrowserMediaPlayerManager.

A follow up CL to merge BrowserMediaPlayerManager and
RemoteMediaPlayerManager will follow in Q2 2017.

This change frees up ~4.0Kib

BUG=707265, 570711
TEST= built/deployed/ran chrome, made sure nothing crashed.

Review-Url: https://codereview.chromium.org/2794283003
Cr-Commit-Position: refs/heads/master@{#464195}
Committed: https://chromium.googlesource.com/chromium/src/+/9542e14c809a1e79e307502b4ed3f00771578b61

[tguilbert, 2017-04-04 23:22:25]

Patchset #2 (id:20001) has been deleted

[tguilbert, 2017-04-04 23:27:47]

tguilbert@chromium.org changed reviewers:
+ avayvod@chromium.org, dalecurtis@chromium.org

[tguilbert, 2017-04-04 23:27:47]

PTAL!

Thanks,
Thomas

https://codereview.chromium.org/2794283003/diff/40001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/2794283003/diff/40001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:73: // |g_factory|
should always be set to create a RemoteMediaPlayerManager,
DaleCurtis/Whywhat: can you think of any counter examples?

https://codereview.chromium.org/2794283003/diff/40001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:76: NOTREACHED();
Is this unnecessary? I am just trying to catch any places where we might
accidentally still be creating a BrowserMediaPlayerManager directly.

Will webview still end up with a RemoteMediaPlayerManager?

[DaleCurtis, 2017-04-04 23:38:30]

https://codereview.chromium.org/2794283003/diff/40001/chrome/browser/android/...
File chrome/browser/android/media/media_throttle_infobar_delegate.cc (left):

https://codereview.chromium.org/2794283003/diff/40001/chrome/browser/android/...
chrome/browser/android/media/media_throttle_infobar_delegate.cc:78:
IDS_MEDIA_THROTTLE_INFOBAR_BLOCK_BUTTON :
Can delete these from
https://cs.chromium.org/chromium/src/chrome/app/generated_resources.grd?l=10429

https://codereview.chromium.org/2794283003/diff/40001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/2794283003/diff/40001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:73: // |g_factory|
should always be set to create a RemoteMediaPlayerManager,
On 2017/04/04 at 23:27:47, tguilbert wrote:
> DaleCurtis/Whywhat: can you think of any counter examples?

The only way one could be created is through this path when no factory is set:

https://cs.chromium.org/chromium/src/content/browser/media/android/media_web_...

There's only one instance of this call though, should we just replace it with
new RemoteMediaPlayerManager() to be explicit?

https://cs.chromium.org/chromium/src/content/browser/media/android/media_web_...

Then you can delete this whole method and all the factory fluff.

https://codereview.chromium.org/2794283003/diff/40001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:381: if
(fullscreen_player_id_ == player_id && fullscreen_player_is_released_) {
I think all the fullscreen player stuff can be deleted too. We don't support
SurfaceView for MediaPlayerRenderer right?

[whywhat, 2017-04-04 23:41:55]

https://codereview.chromium.org/2794283003/diff/40001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/2794283003/diff/40001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:73: // |g_factory|
should always be set to create a RemoteMediaPlayerManager,
On 2017/04/04 at 23:27:47, tguilbert wrote:
> DaleCurtis/Whywhat: can you think of any counter examples?

Not, apart from webview.

https://codereview.chromium.org/2794283003/diff/40001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:76: NOTREACHED();
On 2017/04/04 at 23:27:47, tguilbert wrote:
> Is this unnecessary? I am just trying to catch any places where we might
accidentally still be creating a BrowserMediaPlayerManager directly.
> 
> Will webview still end up with a RemoteMediaPlayerManager?

I hope webview doesn't use BMPM at all now after Spitzer.
Otherwise, it shouldn't use RMPM as they don't support casting anything.

Can you just call RMPM::Create directly from here somehow?

[tguilbert, 2017-04-06 20:50:59]

https://codereview.chromium.org/2794283003/diff/40001/chrome/browser/android/...
File chrome/browser/android/media/media_throttle_infobar_delegate.cc (left):

https://codereview.chromium.org/2794283003/diff/40001/chrome/browser/android/...
chrome/browser/android/media/media_throttle_infobar_delegate.cc:78:
IDS_MEDIA_THROTTLE_INFOBAR_BLOCK_BUTTON :
On 2017/04/04 23:38:29, DaleCurtis wrote:
> Can delete these from
>
https://cs.chromium.org/chromium/src/chrome/app/generated_resources.grd?l=10429

Done.

https://codereview.chromium.org/2794283003/diff/40001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/2794283003/diff/40001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:73: // |g_factory|
should always be set to create a RemoteMediaPlayerManager,
On 2017/04/04 23:38:30, DaleCurtis wrote:
> On 2017/04/04 at 23:27:47, tguilbert wrote:
> > DaleCurtis/Whywhat: can you think of any counter examples?
> 
> The only way one could be created is through this path when no factory is set:
> 
>
https://cs.chromium.org/chromium/src/content/browser/media/android/media_web_...
> 
> There's only one instance of this call though, should we just replace it with
> new RemoteMediaPlayerManager() to be explicit?
> 
>
https://cs.chromium.org/chromium/src/content/browser/media/android/media_web_...
> 
> Then you can delete this whole method and all the factory fluff.

Unfortunately, after some looking into this, I don't think we can remove the
factory fluff right away. RemoteMediaPlayerBridge is in src/chrome, and
including it in src/content code violates layering.

https://codereview.chromium.org/2794283003/diff/40001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:76: NOTREACHED();
On 2017/04/04 23:41:55, whywhat wrote:
> On 2017/04/04 at 23:27:47, tguilbert wrote:
> > Is this unnecessary? I am just trying to catch any places where we might
> accidentally still be creating a BrowserMediaPlayerManager directly.
> > 
> > Will webview still end up with a RemoteMediaPlayerManager?
> 
> I hope webview doesn't use BMPM at all now after Spitzer.
> Otherwise, it shouldn't use RMPM as they don't support casting anything.
> 
> Can you just call RMPM::Create directly from here somehow?

I get linking issues, due to it being in src/chrome.

My concern is that, while there is no real use for the BMPM in webview, the
following path might still trigger it's creation:
https://cs.chromium.org/chromium/src/media/blink/webmediaplayer_impl.cc?l=426
https://cs.chromium.org/chromium/src/media/blink/webmediaplayer_cast_android....
https://cs.chromium.org/chromium/src/content/renderer/media/android/renderer_...
https://cs.chromium.org/chromium/src/content/browser/media/android/media_web_...

The easy way to check would be to just start a webview instance and see if we
run into the NOTREACHED, but I will not be able to check until tomorrow.

https://codereview.chromium.org/2794283003/diff/40001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:381: if
(fullscreen_player_id_ == player_id && fullscreen_player_is_released_) {
On 2017/04/04 23:38:30, DaleCurtis wrote:
> I think all the fullscreen player stuff can be deleted too. We don't support
> SurfaceView for MediaPlayerRenderer right?

Agreed, and yes, we no longer use SurfaceView. However, I haven't looked much
into the fullscreen code, and I think this is beyond the scope of this CL. I
opened crbug/709200 to formally track some of this work.

[tguilbert, 2017-04-10 23:45:24]

Description was changed from

==========
Remove MediaThrottler and MediaThrottleInfoBar

Since the removal of WebMediaPlayerAndroid, we use the
MediaServiceThrottler instead of the MediaThrottler.

This CL cleans up the unused code. It also prevents the accidental
instantiation of BrowserMediaPlayerManager (even if it should no longer
be instantiated), since the code is no longer throttled.

A follow up CL to merge BrowserMediaPlayerManager and
RemoteMediaPlayerManager will follow in Q2 2017.

BUG=707265, 570711
TEST= built/deployed/ran chrome, made sure nothing crashed.
==========

to

==========
Remove MediaThrottler and MediaThrottleInfoBar

Since the removal of WebMediaPlayerAndroid, we use the
MediaServiceThrottler instead of the MediaThrottler.

This CL cleans up the unused code. It also prevents the
instantiation of BrowserMediaPlayerManager.

A follow up CL to merge BrowserMediaPlayerManager and
RemoteMediaPlayerManager will follow in Q2 2017.

BUG=707265, 570711
TEST= built/deployed/ran chrome, made sure nothing crashed.
==========

[tguilbert, 2017-04-10 23:47:35]

Fixed the webview issue and made sure those webview tests don't crash anymore.

PTAL, thanks!

https://codereview.chromium.org/2794283003/diff/40001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/2794283003/diff/40001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:76: NOTREACHED();
On 2017/04/06 20:50:59, tguilbert wrote:
> On 2017/04/04 23:41:55, whywhat wrote:
> > On 2017/04/04 at 23:27:47, tguilbert wrote:
> > > Is this unnecessary? I am just trying to catch any places where we might
> > accidentally still be creating a BrowserMediaPlayerManager directly.
> > > 
> > > Will webview still end up with a RemoteMediaPlayerManager?
> > 
> > I hope webview doesn't use BMPM at all now after Spitzer.
> > Otherwise, it shouldn't use RMPM as they don't support casting anything.
> > 
> > Can you just call RMPM::Create directly from here somehow?
> 
> I get linking issues, due to it being in src/chrome.
> 
> My concern is that, while there is no real use for the BMPM in webview, the
> following path might still trigger it's creation:
> https://cs.chromium.org/chromium/src/media/blink/webmediaplayer_impl.cc?l=426
>
https://cs.chromium.org/chromium/src/media/blink/webmediaplayer_cast_android....
>
https://cs.chromium.org/chromium/src/content/renderer/media/android/renderer_...
>
https://cs.chromium.org/chromium/src/content/browser/media/android/media_web_...
> 
> The easy way to check would be to just start a webview instance and see if we
> run into the NOTREACHED, but I will not be able to check until tomorrow.

I did end up double checking this on Friday.
Webview tests that involved video ended up crashing when this nullptr/NOTREACHED
was here.

I am following Dale's suggestion and dropping IPC messages whenever webview
tries to instanciate a BMPM

[DaleCurtis, 2017-04-10 23:49:47]

https://codereview.chromium.org/2794283003/diff/100001/content/browser/media/...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/2794283003/diff/100001/content/browser/media/...
content/browser/media/android/browser_media_player_manager.cc:77: if (g_factory)
Ternary if you want.

https://codereview.chromium.org/2794283003/diff/100001/content/browser/media/...
File content/browser/media/android/media_throttler.cc (left):

https://codereview.chromium.org/2794283003/diff/100001/content/browser/media/...
content/browser/media/android/media_throttler.cc:9: #include
"jni/MediaThrottler_jni.h"
Delete Java code?

[tguilbert, 2017-04-10 23:50:02]

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

[commit-bot: I haz the power, 2017-04-10 23:50:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tguilbert, 2017-04-11 00:15:25]

Description was changed from

==========
Remove MediaThrottler and MediaThrottleInfoBar

Since the removal of WebMediaPlayerAndroid, we use the
MediaServiceThrottler instead of the MediaThrottler.

This CL cleans up the unused code. It also prevents the
instantiation of BrowserMediaPlayerManager.

A follow up CL to merge BrowserMediaPlayerManager and
RemoteMediaPlayerManager will follow in Q2 2017.

BUG=707265, 570711
TEST= built/deployed/ran chrome, made sure nothing crashed.
==========

to

==========
Remove MediaThrottler and MediaThrottleInfoBar

Since the removal of WebMediaPlayerAndroid, we use the
MediaServiceThrottler instead of the MediaThrottler.

This CL cleans up the unused code. It also prevents the
instantiation of BrowserMediaPlayerManager.

A follow up CL to merge BrowserMediaPlayerManager and
RemoteMediaPlayerManager will follow in Q2 2017.

This change frees up ~4.0Kib

BUG=707265, 570711
TEST= built/deployed/ran chrome, made sure nothing crashed.
==========

[tguilbert, 2017-04-11 00:15:55]

https://codereview.chromium.org/2794283003/diff/100001/content/browser/media/...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/2794283003/diff/100001/content/browser/media/...
content/browser/media/android/browser_media_player_manager.cc:77: if (g_factory)
On 2017/04/10 23:49:47, DaleCurtis wrote:
> Ternary if you want.

Done.

https://codereview.chromium.org/2794283003/diff/100001/content/browser/media/...
File content/browser/media/android/media_throttler.cc (left):

https://codereview.chromium.org/2794283003/diff/100001/content/browser/media/...
content/browser/media/android/media_throttler.cc:9: #include
"jni/MediaThrottler_jni.h"
On 2017/04/10 23:49:47, DaleCurtis wrote:
> Delete Java code?

Oops! Done :P

[tguilbert, 2017-04-11 00:27:18]

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

[tguilbert, 2017-04-11 00:27:57]

tguilbert@chromium.org changed reviewers:
+ dtrainor@chromium.org, kinuko@chromium.org, mpearson@chromium.org,
pkasting@chromium.org

[tguilbert, 2017-04-11 00:27:59]

For OWNERS review, can you PTAL at the following files?

pkasting -- components/infobar/*
dtrainor -- chrome/browser/android/*
kinuko   -- content/browser/renderer_host/* and content/public/*
mpearson -- tools/metrics/*

Thank you!
Thomas

[commit-bot: I haz the power, 2017-04-11 00:28:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Kasting, 2017-04-11 00:41:24]

LGTM

https://codereview.chromium.org/2794283003/diff/120001/components/infobars/co...
File components/infobars/core/infobar_delegate.h (right):

https://codereview.chromium.org/2794283003/diff/120001/components/infobars/co...
components/infobars/core/infobar_delegate.h:83: MEDIA_THROTTLE_INFOBAR_DELEGATE
= 10,
Nit: Comment out with "Removed" as elsewhere

[DaleCurtis, 2017-04-11 00:42:08]

dalecurtis@chromium.org changed reviewers:
- dtrainor@chromium.org, kinuko@chromium.org, mpearson@chromium.org,
pkasting@chromium.org

[DaleCurtis, 2017-04-11 00:42:09]

Remove Java from content/public/android/BUILD.gn file?

[DaleCurtis, 2017-04-11 00:42:34]

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

[DaleCurtis, 2017-04-11 00:42:34]

Whoops, must have been looking at an old version. See the deletion now, lgtm!

[commit-bot: I haz the power, 2017-04-11 01:42:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 01:42:58]

Dry run: This issue passed the CQ dry run.

[Mark P, 2017-04-11 04:31:43]

mpearson@chromium.org changed reviewers:
+ mpearson@chromium.org

[Mark P, 2017-04-11 04:31:44]

histograms.xml lgtm

[whywhat, 2017-04-11 14:56:06]

lgtm

[tguilbert, 2017-04-11 21:55:01]

tguilbert@chromium.org changed reviewers:
+ dtrainor@chromium.org, kinuko@chromium.org

[tguilbert, 2017-04-11 21:55:02]

Somehow some of the reviewers were removed from the CL. I am re-adding them.

Can you OWNERS review the following directories?
dtrainor -- chrome/browser/android/*
kinuko   -- content/browser/renderer_host/* and content/public/*

Thank you!
Thomas

[kinuko, 2017-04-12 14:53:17]

> kinuko   -- content/browser/renderer_host/* and content/public/*

lgtm

[David Trainor- moved to gerrit, 2017-04-12 16:22:09]

chrome/browser/android lgtm

[tguilbert, 2017-04-12 19:35:08]

The CQ bit was checked by tguilbert@chromium.org

[commit-bot: I haz the power, 2017-04-12 19:36:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 23:09:22]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1492025708731250,
"parent_rev": "04278218c9ef29904b209f077fd3ee5869898ba2", "commit_rev":
"9542e14c809a1e79e307502b4ed3f00771578b61"}

[commit-bot: I haz the power, 2017-04-12 23:10:13]

Description was changed from

==========
Remove MediaThrottler and MediaThrottleInfoBar

Since the removal of WebMediaPlayerAndroid, we use the
MediaServiceThrottler instead of the MediaThrottler.

This CL cleans up the unused code. It also prevents the
instantiation of BrowserMediaPlayerManager.

A follow up CL to merge BrowserMediaPlayerManager and
RemoteMediaPlayerManager will follow in Q2 2017.

This change frees up ~4.0Kib

BUG=707265, 570711
TEST= built/deployed/ran chrome, made sure nothing crashed.
==========

to

==========
Remove MediaThrottler and MediaThrottleInfoBar

Since the removal of WebMediaPlayerAndroid, we use the
MediaServiceThrottler instead of the MediaThrottler.

This CL cleans up the unused code. It also prevents the
instantiation of BrowserMediaPlayerManager.

A follow up CL to merge BrowserMediaPlayerManager and
RemoteMediaPlayerManager will follow in Q2 2017.

This change frees up ~4.0Kib

BUG=707265, 570711
TEST= built/deployed/ran chrome, made sure nothing crashed.

Review-Url: https://codereview.chromium.org/2794283003
Cr-Commit-Position: refs/heads/master@{#464195}
Committed:
https://chromium.googlesource.com/chromium/src/+/9542e14c809a1e79e307502b4ed3...
==========

[commit-bot: I haz the power, 2017-04-12 23:10:15]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/9542e14c809a1e79e307502b4ed3...