[Android] Log notification shown/blocked for all other notifications

[Android] Log notification shown/blocked for all other notifications

- Added new SystemNotificationTypes and logging for Media Capture,
Physical Web, Media, Sites and Sync notifications.

- This now captures all notifications that are posted right now.

BUG=706813, 688435
Review-Url: https://codereview.chromium.org/2833583004
Cr-Commit-Position: refs/heads/master@{#466670}
Committed: https://chromium.googlesource.com/chromium/src/+/8191a0ccfd4ffb453432bf35875022899f22a3a2

[awdf, 2017-04-20 15:53:17]

awdf@chromium.org changed reviewers:
+ dtrainor@chromium.org, peter@chromium.org

[awdf, 2017-04-20 15:53:17]

Part 2 of 2.
peter@ & dtrainor@ - PTAL.

[Peter Beverloo, 2017-04-20 17:34:23]

peter@chromium.org changed reviewers:
+ isherman@chromium.org

[Peter Beverloo, 2017-04-20 17:34:24]

lgtm

+isherman for uma

https://codereview.chromium.org/2833583004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java
(right):

https://codereview.chromium.org/2833583004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java:205:
NotificationUmaTracker.MEDIA_CAPTURE, ChannelsInitializer.CHANNEL_ID_MEDIA);
nit: put this immediately after notify()

https://codereview.chromium.org/2833583004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java
(right):

https://codereview.chromium.org/2833583004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:589:
webApkPackage, notificationBuilder, platformTag, PLATFORM_ID);
What should we log for WebAPKs? I agree that it'd bias Chrome's metrics since
the changes wouldn't apply to Chrome, but still good to think about.

https://codereview.chromium.org/2833583004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUmaTracker.java
(right):

https://codereview.chromium.org/2833583004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUmaTracker.java:47:

why the whitespace?

[Ilya Sherman, 2017-04-20 20:15:04]

histograms.xml lgtm

[awdf, 2017-04-21 13:44:12]

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

[commit-bot: I haz the power, 2017-04-21 13:44:30]

Dry run: 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-21 15:17:32]

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

[commit-bot: I haz the power, 2017-04-21 15:17:35]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[nyquist, 2017-04-21 15:34:08]

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

[nyquist, 2017-04-21 15:34:08]

lgtm % other comments

[mlamouri (slow - plz ping), 2017-04-22 10:34:57]

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

[mlamouri (slow - plz ping), 2017-04-22 10:34:58]

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

https://codereview.chromium.org/2833583004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:810:
NotificationUmaTracker.MEDIA, ChannelDefinitions.CHANNEL_ID_MEDIA);
Wouldn't this be called every time the notification state changes to 'pause' and
only for playback notifications? (see condition above)

[awdf, 2017-04-24 10:51:26]

On 2017/04/22 10:34:58, mlamouri wrote:
>
https://codereview.chromium.org/2833583004/diff/40001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java
> (right):
> 
>
https://codereview.chromium.org/2833583004/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:810:
> NotificationUmaTracker.MEDIA, ChannelDefinitions.CHANNEL_ID_MEDIA);
> Wouldn't this be called every time the notification state changes to 'pause'
and
> only for playback notifications? (see condition above)

Thanks for calling this out. I was wrongly assuming notifications are only ever
posted via NotificationManager.notify, and simply logging whenever that is
called, but on closer inspection (looking 2 lines further down!) I see Media
notifications (& download summary notifications on O) are normally posted via
Service.startForeground.

I have moved the logging out of the 'if' statement so it's logged regardless
whether the media notification is posted via notify() or startForeground().

(Ideally one might assume this UMA should be recorded only when a *new*
notification is posted, rather than an existing one being updated, but keeping
track of that would require more logic, so I propose we log whenever a
notification is posted *or* updated).

[mlamouri (slow - plz ping), 2017-04-24 13:17:06]

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

[mlamouri (slow - plz ping), 2017-04-24 13:17:08]

+avayvod@ for review as he knows this code a bit more than I do (dropping
myself).

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

https://codereview.chromium.org/2833583004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:171:
private NotificationUmaTracker mNotificationUmaTracker;
nit: probably should be moved above

https://codereview.chromium.org/2833583004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:622:
MediaNotificationManager(NotificationUmaTracker umaTracker, int notificationId)
{
not a huge fan of adding this but will let avayvod@ decide :)

https://codereview.chromium.org/2833583004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:796:
NotificationUmaTracker.MEDIA, ChannelDefinitions.CHANNEL_ID_MEDIA);
I'm afraid this is going to be called way too many times. One thing that could
be done is to move the call to `showNotification()`. I wouldn't mind some basic
tests to make sure we do things right.

[mlamouri (slow - plz ping), 2017-04-24 13:17:21]

mlamouri@chromium.org changed reviewers:
- mlamouri@chromium.org

[awdf, 2017-04-24 13:54:24]

avayvod@ please take a look at chrome/browser/media parts, thanks!

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

https://codereview.chromium.org/2833583004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:622:
MediaNotificationManager(NotificationUmaTracker umaTracker, int notificationId)
{
On 2017/04/24 13:17:07, mlamouri wrote:
> not a huge fan of adding this but will let avayvod@ decide :)

Done.

https://codereview.chromium.org/2833583004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:796:
NotificationUmaTracker.MEDIA, ChannelDefinitions.CHANNEL_ID_MEDIA);
On 2017/04/24 13:17:07, mlamouri wrote:
> I'm afraid this is going to be called way too many times. One thing that could
> be done is to move the call to `showNotification()`. I wouldn't mind some
basic
> tests to make sure we do things right.

Good point - as it stands this would be logged every time the track changes (and
potentially even more frequently in future eg if track progress was added).

Looking at the code it seems updateNotification is always called for the first
time for a given notification from onServiceStarted (Anton can you verify
this?). So I have moved the logging there.

Since we are mocking out the uma tracker anyway I have added a 'verify' that it
gets called to one of the lifecycle tests.

[whywhat, 2017-04-24 14:42:07]

chrome/browser/media/ui lgtm % nits (note that I don't own chrome/browser/media)

I believe onServiceStarted() is the right place to call the uma tracker from.

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

https://codereview.chromium.org/2833583004/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:112:
private final NotificationUmaTracker mNotificationUmaTracker;
nit: this seems to be inconsistent with every other use of the uma tracker
(albeit with a good goal of adding a test in mind). maybe use GetInstance() like
elsewhere and allow it to be overridden with something like SetInstaceForTest()
in the uma tracker class itself?

https://codereview.chromium.org/2833583004/diff/120001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerServiceLifecycleTest.java
(right):

https://codereview.chromium.org/2833583004/diff/120001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerServiceLifecycleTest.java:194:
verify(mMockUmaTracker)
nit: Given our concerns of this happening too often, add verify(mMockUmaTracker,
never()) where onServiceStarted() is not called in the test cases above?

[awdf, 2017-04-24 16:29:09]

Thanks for the review Anton!

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

https://codereview.chromium.org/2833583004/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:112:
private final NotificationUmaTracker mNotificationUmaTracker;
On 2017/04/24 14:42:07, whywhat wrote:
> nit: this seems to be inconsistent with every other use of the uma tracker
> (albeit with a good goal of adding a test in mind). maybe use GetInstance()
like
> elsewhere and allow it to be overridden with something like
SetInstaceForTest()
> in the uma tracker class itself?

I see the consistency argument and also appreciate it's kinda weird to sometimes
inject a singleton, but I am also reluctant to add the ugly
'setForTest'/sInstanceForTesting boilerplate required (like eg at
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...).
At least with the way I have it the dependency is explicit, so eg if this
dependency disappears in future the test will stop compiling.

(Aside: Ultimately I'm not a fan of singletons, I would prefer a nice
dependency-injection tree -where everything is instantiated once and passed via
constructor parameters- but I understand this is practically impossible with the
way we call into java from native code via static methods everywhere.)

https://codereview.chromium.org/2833583004/diff/120001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerServiceLifecycleTest.java
(right):

https://codereview.chromium.org/2833583004/diff/120001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerServiceLifecycleTest.java:194:
verify(mMockUmaTracker)
On 2017/04/24 14:42:07, whywhat wrote:
> nit: Given our concerns of this happening too often, add
verify(mMockUmaTracker,
> never()) where onServiceStarted() is not called in the test cases above?

Done. (Required adding another 'clearInvocations' method for my mock - which has
the encouraging javadoc "Try to avoid this method at all costs." ...! Don't
think I'm making things worse though.)

[awdf, 2017-04-24 16:29:55]

The CQ bit was checked by awdf@chromium.org

[awdf, 2017-04-24 16:29:56]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
isherman@chromium.org, nyquist@chromium.org, avayvod@chromium.org
Link to the patchset: https://codereview.chromium.org/2833583004/#ps140001
(title: "Verify media notifications not logged in other test cases")

[commit-bot: I haz the power, 2017-04-24 16:30:45]

CQ is trying da patch.

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

[whywhat, 2017-04-24 17:20:03]

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

https://codereview.chromium.org/2833583004/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:112:
private final NotificationUmaTracker mNotificationUmaTracker;
On 2017/04/24 at 16:29:08, awdf wrote:
> On 2017/04/24 14:42:07, whywhat wrote:
> > nit: this seems to be inconsistent with every other use of the uma tracker
> > (albeit with a good goal of adding a test in mind). maybe use GetInstance()
like
> > elsewhere and allow it to be overridden with something like
SetInstaceForTest()
> > in the uma tracker class itself?
> 
> I see the consistency argument and also appreciate it's kinda weird to
sometimes inject a singleton, but I am also reluctant to add the ugly
'setForTest'/sInstanceForTesting boilerplate required (like eg at
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...).
At least with the way I have it the dependency is explicit, so eg if this
dependency disappears in future the test will stop compiling.
> 
> (Aside: Ultimately I'm not a fan of singletons, I would prefer a nice
dependency-injection tree -where everything is instantiated once and passed via
constructor parameters- but I understand this is practically impossible with the
way we call into java from native code via static methods everywhere.)

I'm mostly worried about caching the instance which is unlikely a part of the
(unspecified) contract of GetInstance() method because everything else is not
caching. If something invalidates the old value returned by GetInstance() due to
some Android lifecycle change or future Chrome changes, it would be very hard to
notice.

Using GetInstance() at the call site as well as Set*ForTesting would also be
consistent with how singletons are used in Clank in general, IMO.

I understand your preferences as well and I don't feel too strong about it,
hence the nit: prefix :)

[commit-bot: I haz the power, 2017-04-24 17:33:58]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1493051395647490,
"parent_rev": "ecab6cbf101ed644f2eba3dfad4fc816fb7252e2", "commit_rev":
"8191a0ccfd4ffb453432bf35875022899f22a3a2"}

[commit-bot: I haz the power, 2017-04-24 17:34:08]

Description was changed from

==========
[Android] Log notification shown/blocked for all other notifications

- Added new SystemNotificationTypes and logging for Media Capture,
Physical Web, Media, Sites and Sync notifications.

- This now captures all notifications that are posted right now.

BUG=706813,688435
==========

to

==========
[Android] Log notification shown/blocked for all other notifications

- Added new SystemNotificationTypes and logging for Media Capture,
Physical Web, Media, Sites and Sync notifications.

- This now captures all notifications that are posted right now.

BUG=706813,688435

Review-Url: https://codereview.chromium.org/2833583004
Cr-Commit-Position: refs/heads/master@{#466670}
Committed:
https://chromium.googlesource.com/chromium/src/+/8191a0ccfd4ffb453432bf358750...
==========

[commit-bot: I haz the power, 2017-04-24 17:34:10]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/8191a0ccfd4ffb453432bf358750...