Make media notification classes configurable

Make media notification classes configurable

This CL makes the media notification classes configurable in the
following ways:

* Using Class<?> instead of class names for simplicity.
* Having a static map in MediaNotificationManager to look up the service
  class, receiver class, notification group name for a given
  notification ID.

BUG=705925
Review-Url: https://codereview.chromium.org/2776243003
Cr-Commit-Position: refs/heads/master@{#460394}
Committed: https://chromium.googlesource.com/chromium/src/+/52ec59e6f64784544c395fba8c448fea01033de4

[Zhiqiang Zhang (Slow), 2017-03-28 12:40:11]

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

[whywhat, 2017-03-28 19:35:02]

lgtm

I'm not so sure it's worth it to keep the map in the static memory and do
lookups instead of simple if/switch statements.

Does this change help testing in any way?

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

https://codereview.chromium.org/2776243003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:604:
mNotificationId = notificationId;
hm, can't we just initialize something like mNotificationOptions or
mServiceClass, mReceiverClass, mGroupName here with the notificationId? perhaps
pre-initialize MediaNotificationManager instances with notification id's in the
map in the first place?

[Zhiqiang Zhang (Slow), 2017-03-29 14:16:51]

On 2017/03/28 at 19:35:02, avayvod wrote:
> lgtm
> 
> I'm not so sure it's worth it to keep the map in the static memory and do
lookups instead of simple if/switch statements.
> 
> Does this change help testing in any way?

Yes, it is used for testing. The reason is that the service and receiver objects
are created by Android, we need to pass the mocked class to Android so that a
mocked object is created.

After this change, if we want to check if a listener service method is called,
we can just add a new NotificationOptions in the tests, with a mocked service
class. Before this change, it is hard to do so.

BTW, the CL description seems outdated. Will update it accordingly.

[Zhiqiang Zhang (Slow), 2017-03-29 14:17:00]

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

https://codereview.chromium.org/2776243003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:604:
mNotificationId = notificationId;
On 2017/03/28 at 19:35:02, whywhat wrote:
> hm, can't we just initialize something like mNotificationOptions or
mServiceClass, mReceiverClass, mGroupName here with the notificationId?

A problem I had is that the services and receivers are not associated with the
manager so they cannot access the mNotificationOptions from MNM. That's why they
keep a copy of their notification id's, which will be used to look up for the
corresponding service class/receiver class.

> perhaps pre-initialize MediaNotificationManager instances with notification
id's in the map in the first place?

We still have clear(), which removes the MNM instances from sManagers. I think
unless we assume there is always a MNM for each notification id, we should not
initialize before use.

[Zhiqiang Zhang (Slow), 2017-03-29 14:43:55]

Description was changed from

==========
Use class instead of class name for creating notification intent

BUG=705925
==========

to

==========
Make media notification classes configurable

This CL makes the media notification classes configurable in the
following ways:

* Using <Class> instead of class names for simplicity.
* Having a static map in MediaNotificationManager to look up the service
  class, receiver class, notification group name for a given
  notification ID.

BUG=705925
==========

[Zhiqiang Zhang (Slow), 2017-03-29 14:45:17]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2017-03-29 14:45:17]

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/2776243003/#ps40001
(title: "rebased")

[commit-bot: I haz the power, 2017-03-29 14:45:35]

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 15:15:40]

Description was changed from

==========
Make media notification classes configurable

This CL makes the media notification classes configurable in the
following ways:

* Using <Class> instead of class names for simplicity.
* Having a static map in MediaNotificationManager to look up the service
  class, receiver class, notification group name for a given
  notification ID.

BUG=705925
==========

to

==========
Make media notification classes configurable

This CL makes the media notification classes configurable in the
following ways:

* Using Class<?> instead of class names for simplicity.
* Having a static map in MediaNotificationManager to look up the service
  class, receiver class, notification group name for a given
  notification ID.

BUG=705925
==========

[commit-bot: I haz the power, 2017-03-29 15:26:33]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490798717395910,
"parent_rev": "86d0ef1b1d5e95c274e3d106de79b7b6575cd18e", "commit_rev":
"52ec59e6f64784544c395fba8c448fea01033de4"}

[commit-bot: I haz the power, 2017-03-29 15:27:20]

Description was changed from

==========
Make media notification classes configurable

This CL makes the media notification classes configurable in the
following ways:

* Using Class<?> instead of class names for simplicity.
* Having a static map in MediaNotificationManager to look up the service
  class, receiver class, notification group name for a given
  notification ID.

BUG=705925
==========

to

==========
Make media notification classes configurable

This CL makes the media notification classes configurable in the
following ways:

* Using Class<?> instead of class names for simplicity.
* Having a static map in MediaNotificationManager to look up the service
  class, receiver class, notification group name for a given
  notification ID.

BUG=705925

Review-Url: https://codereview.chromium.org/2776243003
Cr-Commit-Position: refs/heads/master@{#460394}
Committed:
https://chromium.googlesource.com/chromium/src/+/52ec59e6f64784544c395fba8c44...
==========

[commit-bot: I haz the power, 2017-03-29 15:27:20]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/52ec59e6f64784544c395fba8c44...