Simplify Context usage in MediaNotification

Simplify Context usage in MediaNotification

Currently the MediaNotification usages Context objects from many
sources, either on service creation, or getting the global
ApplicationContext. Actually, using the global ApplicationContext
should be sufficient.

BUG=705920
Review-Url: https://codereview.chromium.org/2783633002
Cr-Commit-Position: refs/heads/master@{#460350}
Committed: https://chromium.googlesource.com/chromium/src/+/0d99ea0a684a3e463a19d9d99f1c0d0a49099f48

[Zhiqiang Zhang (Slow), 2017-03-28 12:39:53]

zqzhang@chromium.org changed reviewers:
+ avayvod@chromium.org

[whywhat, 2017-03-28 19:24:51]

Any particular reason to do this change?

As learned from the internetz, it seems like one should use the most specific
context available with the same or greater lifecycle... Since our notification
and service has a lifecycle greater than the particular Chrome activity (meaning
the activity can die, but the notification will be alive), we should probably
use the application context everywhere.

lgtm

https://codereview.chromium.org/2783633002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java
(right):

https://codereview.chromium.org/2783633002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:860:
MediaSessionCompat mediaSession = new MediaSessionCompat(getContext(),
nit: perhaps cache the context locally here so it doesn't look like excessive
calculation?
Also, ContextUtils.getApplicationContext() can technically return null, is it
ever going to happen when our code is running? :)

[Zhiqiang Zhang (Slow), 2017-03-29 10:35:48]

https://codereview.chromium.org/2783633002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java
(right):

https://codereview.chromium.org/2783633002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:860:
MediaSessionCompat mediaSession = new MediaSessionCompat(getContext(),
On 2017/03/28 at 19:24:51, whywhat wrote:
> nit: perhaps cache the context locally here so it doesn't look like excessive
calculation?

Done.

> Also, ContextUtils.getApplicationContext() can technically return null, is it
ever going to happen when our code is running? :)

It should not for production, but possible for tests.
We can use ContextUtils.initApplicationContextForTests() to ensure this.

[Zhiqiang Zhang (Slow), 2017-03-29 10:35:56]

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

[commit-bot: I haz the power, 2017-03-29 10:36:13]

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

[Zhiqiang Zhang (Slow), 2017-03-29 10:39:08]

On 2017/03/28 at 19:24:51, avayvod wrote:
> Any particular reason to do this change?

It's for easing the effort for writing tests. If we have different Contexts
everywhere, then we need to set them individually in the tests.

> As learned from the internetz, it seems like one should use the most specific
context available with the same or greater lifecycle... Since our notification
and service has a lifecycle greater than the particular Chrome activity (meaning
the activity can die, but the notification will be alive), we should probably
use the application context everywhere.

Yes, that's also part of the reason. Actually we don't care if it's an
ApplicationContext or other kind of contexts. We only use the context to
construct Intents for starting services. The context is only used to get the
package name.

[commit-bot: I haz the power, 2017-03-29 11:12:37]

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

[commit-bot: I haz the power, 2017-03-29 11:12:38]

Dry run: This issue passed the CQ dry run.

[Zhiqiang Zhang (Slow), 2017-03-29 11:13:16]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2017-03-29 11:13:16]

The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org
Link to the patchset: https://codereview.chromium.org/2783633002/#ps20001
(title: "fixed nits")

[commit-bot: I haz the power, 2017-03-29 11:13:33]

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-03-29 11:34:09]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490785996024990,
"parent_rev": "87301efc7eee4e37e3af750bc43f2977b386652b", "commit_rev":
"0d99ea0a684a3e463a19d9d99f1c0d0a49099f48"}

[commit-bot: I haz the power, 2017-03-29 11:34:58]

Description was changed from

==========
Simplify Context usage in MediaNotification

Currently the MediaNotification usages Context objects from many
sources, either on service creation, or getting the global
ApplicationContext. Actually, using the global ApplicationContext
should be sufficient.

BUG=705920
==========

to

==========
Simplify Context usage in MediaNotification

Currently the MediaNotification usages Context objects from many
sources, either on service creation, or getting the global
ApplicationContext. Actually, using the global ApplicationContext
should be sufficient.

BUG=705920

Review-Url: https://codereview.chromium.org/2783633002
Cr-Commit-Position: refs/heads/master@{#460350}
Committed:
https://chromium.googlesource.com/chromium/src/+/0d99ea0a684a3e463a19d9d99f1c...
==========

[commit-bot: I haz the power, 2017-03-29 11:34:59]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/0d99ea0a684a3e463a19d9d99f1c...