Android - UMA for notification click latency via NotificationJobService

Android - UMA for notification click latency via NotificationJobService

BUG=685210

[awdf, 2017-03-22 17:15:29]

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

[commit-bot: I haz the power, 2017-03-22 17:16:00]

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

[awdf, 2017-03-22 17:17:52]

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

[awdf, 2017-03-22 17:17:52]

peter@ - please review

[Peter Beverloo, 2017-03-22 17:25:24]

lgtm % nits

https://codereview.chromium.org/2769713002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java
(right):

https://codereview.chromium.org/2769713002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java:44:
static final String NOTIFICATION_JOB_STARTED_TIME_MS =
"notification_job_started_time_ms";
nit: these are extras, why not prefix them as such? I'd drop NOTIFICATION_ if
it's getting too long, that's already clear from the class' name.

https://codereview.chromium.org/2769713002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java
(right):

https://codereview.chromium.org/2769713002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:62:
putJobStartedTimeInExtras(extras);
nit: why not inline this? It's just one extra call, and would be equally clear
with a comment.

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

https://codereview.chromium.org/2769713002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:223:
if (begin > 0 && end > 0) {
nit: I'd have a mild preference for checking w/ hasExtra() as opposed to the
default value. Don't feel too strongly though. Should probably also check for
end>begin to deal with reboots?

https://codereview.chromium.org/2769713002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java
(right):

https://codereview.chromium.org/2769713002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java:53:
putJobScheduledTimeInExtras(extras);
nit: dito re: inlining

https://codereview.chromium.org/2769713002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2769713002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:42183: +    the onStartJob callback.
nit: detail that it's only measured on Android N+

[awdf, 2017-03-22 18:13:52]

Patchset #4 (id:60001) has been deleted

[awdf, 2017-03-22 18:14:52]

https://codereview.chromium.org/2769713002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java
(right):

https://codereview.chromium.org/2769713002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java:44:
static final String NOTIFICATION_JOB_STARTED_TIME_MS =
"notification_job_started_time_ms";
On 2017/03/22 17:25:24, Peter Beverloo wrote:
> nit: these are extras, why not prefix them as such? I'd drop NOTIFICATION_ if
> it's getting too long, that's already clear from the class' name.

Done.

https://codereview.chromium.org/2769713002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java
(right):

https://codereview.chromium.org/2769713002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:62:
putJobStartedTimeInExtras(extras);
On 2017/03/22 17:25:24, Peter Beverloo wrote:
> nit: why not inline this? It's just one extra call, and would be equally clear
> with a comment.

There's a school of thought that comments that explain *what* the code does (as
opposed to the 'why') would be better off as method names. Keeps methods
readable and shorter.

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

https://codereview.chromium.org/2769713002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:223:
if (begin > 0 && end > 0) {
On 2017/03/22 17:25:24, Peter Beverloo wrote:
> nit: I'd have a mild preference for checking w/ hasExtra() as opposed to the
> default value. Don't feel too strongly though. Should probably also check for
> end>begin to deal with reboots?

Done.

https://codereview.chromium.org/2769713002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2769713002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:42183: +    the onStartJob callback.
On 2017/03/22 17:25:24, Peter Beverloo wrote:
> nit: detail that it's only measured on Android N+

Done.

[awdf, 2017-03-22 18:25:36]

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

[awdf, 2017-03-22 18:25:37]

isherman@ - please review change to histograms.xml

[Ilya Sherman, 2017-03-22 21:27:54]

Metrics LGTM.  Incidentally, the JobScheduler API is considered to be stable as
of Android M, to the best of my knowledge.  Is there any particular reason
you're targeting N+ rather than M+?

[awdf, 2017-03-23 11:30:56]

On 2017/03/22 21:27:54, Ilya Sherman wrote:
> Metrics LGTM.  Incidentally, the JobScheduler API is considered to be stable
as
> of Android M, to the best of my knowledge.  Is there any particular reason
> you're targeting N+ rather than M+?

We only *have* to use it from O+, due to background check restrictions. But we
wanted to test it out and nobody has O yet, hence targeting N. We didn't target
M yet to minimise risks in case using JobScheduler does increase latency
significantly - this is a time sensitive job in response to a user gesture. This
UMA will hopefully reassure us, so in due course we could start using the
JobScheduler on M+ via BackgroundTaskScheduler.java. (Although the extent to
which measurements on N can be generalised to M is a bit moot).

[awdf, 2017-03-23 11:34:57]

The CQ bit was checked by awdf@chromium.org

[awdf, 2017-03-23 11:34:58]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/2769713002/#ps70001
(title: "Nits")

[commit-bot: I haz the power, 2017-03-23 11:35:11]

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-23 12:15:22]

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

[commit-bot: I haz the power, 2017-03-23 12:15:23]

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