New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 707265 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

android_library("media_java") should not depend on "content"

Project Member Reported by xhw...@chromium.org, Mar 31 2017

Issue description

In Chromium "content" depends on "media" and "media" should not depend on "content". This is enforced by our DEPS and BUILD.gn files.

There's one exception now that android_library("media_java") depends on "//content/public/android:content_java_resources" [1]. This is introduced in the word to add MediaServiceThrottler [2].

I am not familiar with how that code works so I don't have any concrete suggestions. But we should look into this.

[1] https://cs.chromium.org/chromium/src/media/base/android/BUILD.gn?rcl=b3fc618111c6c50e9e05a8afb72f6fb40fd3b585&l=122

[2] https://chromium.googlesource.com/chromium/src/+/f524383bbd03acfba4ffe18b7e77a66c46228dcd
 
I think the only reason why we depend on content is because of our use of content.R.raw.empty when creating the watchdog MediaPlayer https://cs.chromium.org/chromium/src/content/public/android/java/res/raw/empty.wav

I can move the resource to media when I delete MediaThrottler, or I can create a new one now in the meantime.
Status: Assigned (was: Available)
Just spoke offline, we should just surgically delete MediaThrottler and move the resource now.

Comment 3 by xhw...@chromium.org, Mar 31 2017

Great! Thanks for looking into this so quickly!
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9542e14c809a1e79e307502b4ed3f00771578b61

commit 9542e14c809a1e79e307502b4ed3f00771578b61
Author: tguilbert <tguilbert@chromium.org>
Date: Wed Apr 12 23:09:16 2017

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}

[modify] https://crrev.com/9542e14c809a1e79e307502b4ed3f00771578b61/chrome/app/generated_resources.grd
[modify] https://crrev.com/9542e14c809a1e79e307502b4ed3f00771578b61/chrome/browser/BUILD.gn
[delete] https://crrev.com/04278218c9ef29904b209f077fd3ee5869898ba2/chrome/browser/android/media/media_throttle_infobar_delegate.cc
[delete] https://crrev.com/04278218c9ef29904b209f077fd3ee5869898ba2/chrome/browser/android/media/media_throttle_infobar_delegate.h
[modify] https://crrev.com/9542e14c809a1e79e307502b4ed3f00771578b61/chrome/browser/android/tab_web_contents_delegate_android.cc
[modify] https://crrev.com/9542e14c809a1e79e307502b4ed3f00771578b61/chrome/browser/android/tab_web_contents_delegate_android.h
[modify] https://crrev.com/9542e14c809a1e79e307502b4ed3f00771578b61/components/infobars/core/infobar_delegate.cc
[modify] https://crrev.com/9542e14c809a1e79e307502b4ed3f00771578b61/components/infobars/core/infobar_delegate.h
[modify] https://crrev.com/9542e14c809a1e79e307502b4ed3f00771578b61/content/browser/BUILD.gn
[modify] https://crrev.com/9542e14c809a1e79e307502b4ed3f00771578b61/content/browser/media/android/browser_media_player_manager.cc
[modify] https://crrev.com/9542e14c809a1e79e307502b4ed3f00771578b61/content/browser/media/android/browser_media_player_manager.h
[delete] https://crrev.com/04278218c9ef29904b209f077fd3ee5869898ba2/content/browser/media/android/media_throttler.cc
[delete] https://crrev.com/04278218c9ef29904b209f077fd3ee5869898ba2/content/browser/media/android/media_throttler.h
[modify] https://crrev.com/9542e14c809a1e79e307502b4ed3f00771578b61/content/browser/media/android/media_web_contents_observer_android.cc
[modify] https://crrev.com/9542e14c809a1e79e307502b4ed3f00771578b61/content/browser/renderer_host/render_message_filter.cc
[modify] https://crrev.com/9542e14c809a1e79e307502b4ed3f00771578b61/content/public/android/BUILD.gn
[delete] https://crrev.com/04278218c9ef29904b209f077fd3ee5869898ba2/content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java
[modify] https://crrev.com/9542e14c809a1e79e307502b4ed3f00771578b61/content/public/browser/web_contents_delegate.cc
[modify] https://crrev.com/9542e14c809a1e79e307502b4ed3f00771578b61/content/public/browser/web_contents_delegate.h
[modify] https://crrev.com/9542e14c809a1e79e307502b4ed3f00771578b61/tools/metrics/histograms/histograms.xml

Project Member

Comment 5 by bugdroid1@chromium.org, Apr 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/86670b9ee927163fffe528e93d8179100fae0b08

commit 86670b9ee927163fffe528e93d8179100fae0b08
Author: tguilbert <tguilbert@chromium.org>
Date: Thu Apr 13 18:48:08 2017

Move empty.wav to media/base

Currently, the MediaServerCrashListener uses an empty wav file to create
the MediaPlayer it uses a watchdog to listen for server crashed.
Empty.wav is in content/public/android.

With the recent removal of the MediaThrottler, it is now possible to
move empty.wav to media/base, in order to break the dependency on
//content.

BUG= 707265 
TEST=

Review-Url: https://codereview.chromium.org/2815173002
Cr-Commit-Position: refs/heads/master@{#464479}

[modify] https://crrev.com/86670b9ee927163fffe528e93d8179100fae0b08/media/base/android/BUILD.gn
[rename] https://crrev.com/86670b9ee927163fffe528e93d8179100fae0b08/media/base/android/java/res/raw/empty.wav
[modify] https://crrev.com/86670b9ee927163fffe528e93d8179100fae0b08/media/base/android/java/src/org/chromium/media/MediaServerCrashListener.java

Status: Fixed (was: Assigned)

Sign in to add a comment