Presentation API: reject screen availability observing on Android low ram devices.

Presentation API: reject screen availability observing on Android low ram devices.

This is changing the MediaRouter interfaces (MediaSinksObserver) in
order to allow the Android implementation to be able to reject
observing.

BUG=507702
Committed: https://crrev.com/0c8ce52069446e76b6c8bb31950b8b03ed716004
Cr-Commit-Position: refs/heads/master@{#352140}

[mlamouri (slow - plz ping), 2015-09-29 11:04:30]

mlamouri@chromium.org changed reviewers:
+ avayvod@chromium.org, bauerb@chromium.org, mfoltz@chromium.org

[mlamouri (slow - plz ping), 2015-09-29 11:04:30]

Could you please review the following changes:

avayvod:
  *

bauerb: (OWNER stamp required)
 
chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterTest.java

mfoltz:
  chrome/browser/media/router/media_router.h
  chrome/browser/media/router/media_router_mojo_impl.cc
  chrome/browser/media/router/media_router_mojo_impl.h
  chrome/browser/media/router/media_sinks_observer.cc
  chrome/browser/media/router/media_sinks_observer.h
  chrome/browser/media/router/mock_media_router.h
  chrome/browser/media/router/presentation_service_delegate_impl.cc

[mlamouri (slow - plz ping), 2015-09-29 11:04:53]

https://codereview.chromium.org/1376703002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastMediaRouteProvider.java
(right):

https://codereview.chromium.org/1376703002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastMediaRouteProvider.java:204:
} catch (NoClassDefFoundError e) {
avayvod@, do you know if I can avoid this?

[Bernhard Bauer, 2015-09-29 11:24:07]

https://codereview.chromium.org/1376703002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouter.java
(right):

https://codereview.chromium.org/1376703002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouter.java:126:
return false;
Nit: Consider inlining this block.

https://codereview.chromium.org/1376703002/diff/20001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterTest.java
(right):

https://codereview.chromium.org/1376703002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterTest.java:40:
public static class FauxActivityManager extends ShadowActivityManager {
FakeActivityManager?

https://codereview.chromium.org/1376703002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterTest.java:59:
sIsLowRamDevice = true;
You could split this off into a different test.

https://codereview.chromium.org/1376703002/diff/20001/content/browser/present...
File content/browser/presentation/presentation_service_impl_unittest.cc (right):

https://codereview.chromium.org/1376703002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:74: bool
AddScreenAvailabilityListener(
Is this overriding a method? If so, please add override.

https://codereview.chromium.org/1376703002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:79:
listener->OnScreenAvailabilityNotSupported();
Nit: C++ usually doesn't use braces for single-line bodies.

(Also, if you remove the braces, please add an empty line afterwards to have
some visual space).

[mlamouri (slow - plz ping), 2015-09-29 11:34:27]

Comments applied. PTAL.

https://codereview.chromium.org/1376703002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouter.java
(right):

https://codereview.chromium.org/1376703002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouter.java:126:
return false;
On 2015/09/29 at 11:24:07, Bernhard Bauer wrote:
> Nit: Consider inlining this block.

I did consider it but I am afraid the early return would not be obvious.

https://codereview.chromium.org/1376703002/diff/20001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterTest.java
(right):

https://codereview.chromium.org/1376703002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterTest.java:40:
public static class FauxActivityManager extends ShadowActivityManager {
On 2015/09/29 at 11:24:07, Bernhard Bauer wrote:
> FakeActivityManager?

Sure.

https://codereview.chromium.org/1376703002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterTest.java:59:
sIsLowRamDevice = true;
On 2015/09/29 at 11:24:07, Bernhard Bauer wrote:
> You could split this off into a different test.

Done.

https://codereview.chromium.org/1376703002/diff/20001/content/browser/present...
File content/browser/presentation/presentation_service_impl_unittest.cc (right):

https://codereview.chromium.org/1376703002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:74: bool
AddScreenAvailabilityListener(
On 2015/09/29 at 11:24:07, Bernhard Bauer wrote:
> Is this overriding a method? If so, please add override.

Oups.

https://codereview.chromium.org/1376703002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:79:
listener->OnScreenAvailabilityNotSupported();
On 2015/09/29 at 11:24:07, Bernhard Bauer wrote:
> Nit: C++ usually doesn't use braces for single-line bodies.
> 
> (Also, if you remove the braces, please add an empty line afterwards to have
some visual space).

Done.

[Bernhard Bauer, 2015-09-29 12:19:23]

lgtm

[whywhat, 2015-09-29 12:29:22]

https://codereview.chromium.org/1376703002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouter.java
(right):

https://codereview.chromium.org/1376703002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouter.java:122:
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT) {
Please, use org.chromium.base.SysUtils.isLowEndDevice

https://codereview.chromium.org/1376703002/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_sinks_observer.cc (right):

https://codereview.chromium.org/1376703002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_sinks_observer.cc:16: is_active_ =
router_->RegisterMediaSinksObserver(this);
Maybe we shouldn't actually do this work in the ctor?
E.g. we could have a static method that returns the initialized instance if
observing is supported and null otherwise?

https://codereview.chromium.org/1376703002/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/presentation_service_delegate_impl.cc (right):

https://codereview.chromium.org/1376703002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:189:
listener->OnScreenAvailabilityNotSupported();
I think you need to make sinks_observer a scoped_ptr, otherwise you're leaking
memory here.

[whywhat, 2015-09-29 13:02:22]

also lgtm % comments

[mlamouri (slow - plz ping), 2015-09-29 14:44:52]

mfoltz@, PTAL.

https://codereview.chromium.org/1376703002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouter.java
(right):

https://codereview.chromium.org/1376703002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouter.java:122:
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT) {
On 2015/09/29 at 12:29:22, whywhat wrote:
> Please, use org.chromium.base.SysUtils.isLowEndDevice

Ok. It does simplify a few things.

https://codereview.chromium.org/1376703002/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_sinks_observer.cc (right):

https://codereview.chromium.org/1376703002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_sinks_observer.cc:16: is_active_ =
router_->RegisterMediaSinksObserver(this);
On 2015/09/29 at 12:29:22, whywhat wrote:
> Maybe we shouldn't actually do this work in the ctor?
> E.g. we could have a static method that returns the initialized instance if
observing is supported and null otherwise?

As discussed, we could have a ::Create() static method but that would not work
well with inheritance.

https://codereview.chromium.org/1376703002/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/presentation_service_delegate_impl.cc (right):

https://codereview.chromium.org/1376703002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:189:
listener->OnScreenAvailabilityNotSupported();
On 2015/09/29 at 12:29:22, whywhat wrote:
> I think you need to make sinks_observer a scoped_ptr, otherwise you're leaking
memory here.

Good catch!

[mark a. foltz, 2015-10-01 18:33:28]

LGTM, comments but no changes requested

https://codereview.chromium.org/1376703002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastMediaRouteProvider.java
(right):

https://codereview.chromium.org/1376703002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastMediaRouteProvider.java:205:
// Happens with Robolectric.
This comment didn't make sense to me, maybe it does to Android developers :)

https://codereview.chromium.org/1376703002/diff/60001/chrome/browser/media/ro...
File chrome/browser/media/router/presentation_service_delegate_impl.cc (right):

https://codereview.chromium.org/1376703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:188: if
(!sinks_observer_->is_active()) {
It's slightly weird that you are creating a sinks observer, only to immediately
destroy it because the router created it as inactive.

I believe this is because we do the registration/unregistration in the
MediaSinksObserver ctor/dtor.  Some refactoring to allow the router to create
one (or refuse to) instead would probably make this clearer.

However, not necessary to land this - out of scope for this fix anyway.

[mlamouri (slow - plz ping), 2015-10-01 21:40:45]

https://codereview.chromium.org/1376703002/diff/60001/chrome/browser/media/ro...
File chrome/browser/media/router/presentation_service_delegate_impl.cc (right):

https://codereview.chromium.org/1376703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:188: if
(!sinks_observer_->is_active()) {
On 2015/10/01 at 18:33:28, mfoltz_ooo_until_10_4 wrote:
> It's slightly weird that you are creating a sinks observer, only to
immediately destroy it because the router created it as inactive.
> 
> I believe this is because we do the registration/unregistration in the
MediaSinksObserver ctor/dtor.  Some refactoring to allow the router to create
one (or refuse to) instead would probably make this clearer.
> 
> However, not necessary to land this - out of scope for this fix anyway.

Anton made the same comment and we considered using a ::Create() static method
but that is a bit complex given that there are MediaSinksObserver and
PresentationMediaSinksObserver. This is something we might look into later.

[mlamouri (slow - plz ping), 2015-10-01 21:40:48]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-10-01 21:40:48]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
avayvod@chromium.org
Link to the patchset: https://codereview.chromium.org/1376703002/#ps60001
(title: "review comments")

[commit-bot: I haz the power, 2015-10-01 21:41:09]

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

[commit-bot: I haz the power, 2015-10-01 21:44:02]

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

[commit-bot: I haz the power, 2015-10-01 21:44:03]

Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mlamouri (slow - plz ping), 2015-10-01 22:03:11]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-10-01 22:03:12]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
mfoltz@chromium.org, avayvod@chromium.org
Link to the patchset: https://codereview.chromium.org/1376703002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2015-10-01 22:03:38]

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

[commit-bot: I haz the power, 2015-10-01 22:08:33]

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

[commit-bot: I haz the power, 2015-10-01 22:08:34]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[mlamouri (slow - plz ping), 2015-10-02 10:26:57]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-10-02 10:26:58]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
mfoltz@chromium.org, avayvod@chromium.org
Link to the patchset: https://codereview.chromium.org/1376703002/#ps100001
(title: "fix gmock usage")

[commit-bot: I haz the power, 2015-10-02 10:27:54]

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

[commit-bot: I haz the power, 2015-10-02 10:42:55]

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

[commit-bot: I haz the power, 2015-10-02 10:42:56]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)

[mlamouri (slow - plz ping), 2015-10-02 11:48:06]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2015-10-02 11:48:17]

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

[commit-bot: I haz the power, 2015-10-02 12:05:24]

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

[commit-bot: I haz the power, 2015-10-02 12:05:25]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[mlamouri (slow - plz ping), 2015-10-02 13:53:37]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-10-02 13:53:38]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
mfoltz@chromium.org, avayvod@chromium.org
Link to the patchset: https://codereview.chromium.org/1376703002/#ps140001
(title: "fix and add tests")

[commit-bot: I haz the power, 2015-10-02 13:54:43]

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

[commit-bot: I haz the power, 2015-10-02 16:49:24]

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

[commit-bot: I haz the power, 2015-10-02 16:49:25]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[mlamouri (slow - plz ping), 2015-10-02 17:19:41]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2015-10-02 17:21:10]

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

[commit-bot: I haz the power, 2015-10-02 20:06:26]

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

[commit-bot: I haz the power, 2015-10-02 20:06:27]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[mlamouri (slow - plz ping), 2015-10-02 20:13:33]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2015-10-02 20:15:29]

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

[commit-bot: I haz the power, 2015-10-02 21:47:48]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-10-02 21:48:33]

Patchset 8 (id:??) landed as
https://crrev.com/0c8ce52069446e76b6c8bb31950b8b03ed716004
Cr-Commit-Position: refs/heads/master@{#352140}