Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2611333002:
Android web notifications: Schedule job instead of starting service (N+) (Closed)
DescriptionAndroid web notifications: Schedule job instead of starting service (N+)
- Previously we always started a service when a web notification was
clicked or dismissed.
- Now we instead schedule a job to handle the notification intent on
N+, since this is encouraged by Android.
- We use the Android JobScheduler, rather than GcmNetworkManager or
FirebaseJobDispatcher, since the JobScheduler allows us to execute
immediately (hopefully).
BUG=663427, 680816
Review-Url: https://codereview.chromium.org/2611333002
Cr-Commit-Position: refs/heads/master@{#446072}
Committed: https://chromium.googlesource.com/chromium/src/+/d54134355647e110b2d6793d2b257dceb4b706ed
Patch Set 1 #
Total comments: 15
Patch Set 2 : rebase #Patch Set 3 : Review comments + split out the new JobService #Patch Set 4 : fix up javadoc link #
Total comments: 12
Patch Set 5 : Add comments and todos, remove @RequiresApi annotations #Patch Set 6 : fixup another javadoc link #Patch Set 7 : rebase #
Total comments: 12
Patch Set 8 : Stop calling runOnUiThread unnecessarily; improve comments #
Total comments: 2
Patch Set 9 : Return false from startJob & remove jobFinished call #
Total comments: 14
Patch Set 10 : Improving comments and other nits #
Total comments: 8
Messages
Total messages: 42 (19 generated)
awdf@chromium.org changed reviewers: + peter@chromium.org
I would prefer to tie the Extras initialization together than rely on people reading comments and setting a new extras in two places but Jobs need a PersistableBundle which is api 21+, whereas Intents need a regular Bundle, and their common ancestor BaseBundle is also 21+ so I couldn't find a way to write one method for both cases :(
Thanks! Why can't we use the ChromeBackgroundService? https://codereview.chromium.org/2611333002/diff/1/chrome/android/java/Android... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2611333002/diff/1/chrome/android/java/Android... chrome/android/java/AndroidManifest.xml:790: android:permission="android.permission.BIND_JOB_SERVICE"/> This is API level 21, will it be ignored by earlier versions? I'd like an OWNER of this file to confirm it won't complicate our update path. https://codereview.chromium.org/2611333002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java (right): https://codereview.chromium.org/2611333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:226: static String getNotificationReply(Intent intent) { nit: why remove `private`? That's more restricted than package-level protected. https://codereview.chromium.org/2611333002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java (right): https://codereview.chromium.org/2611333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java:47: if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.LOLLIPOP_MR1) { What's your reasoning for using the Job Scheduler for LMR1+ as opposed to N+? https://codereview.chromium.org/2611333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java:50: .Builder(0 /* jobId */, I think we'll need a magic value for our job. The documentation says: > Subsequent calls to cancel, or jobs created with the same jobId, will > update the pre-existing job with the same id. https://codereview.chromium.org/2611333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java:53: .setOverrideDeadline(1) I think we can set this to zero? https://codereview.chromium.org/2611333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java:68: @NonNull micro nit: up to you, but I'd prefer to not have @NonNull as well. Right now the contract in our code is "non-null unless marked @Nullable".
> Why can't we use the ChromeBackgroundService? Because I had to increase the timeout 10x to make the NotificationPlatformBridge tests pass when I tried using a GcmTaskService. And even then they sometimes failed. They actually say "... to prevent abuse the scheduler will only set an alarm at a minimum of 30 seconds in the future." (from https://developers.google.com/android/reference/com/google/android/gms/gcm/On... ). JobScheduler doesn't seem to have this restriction though.
https://codereview.chromium.org/2611333002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java (right): https://codereview.chromium.org/2611333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java:117: dispatchIntentOnUIThread(intent); Oops - I should really be calling onJobFinished here - https://developer.android.com/reference/android/app/job/JobService.html#jobFi..., boolean) - will fix
https://codereview.chromium.org/2611333002/diff/1/chrome/android/java/Android... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2611333002/diff/1/chrome/android/java/Android... chrome/android/java/AndroidManifest.xml:790: android:permission="android.permission.BIND_JOB_SERVICE"/> On 2017/01/09 14:28:05, Peter Beverloo wrote: > This is API level 21, will it be ignored by earlier versions? I'd like an OWNER > of this file to confirm it won't complicate our update path. I think this would just be ignored. Still installs fine on K - I checked. However it *is* a problem to use the same service on all platforms, because JobService, which the JobScheduler requires us to inherit from, is API 21+, so we need a different service for at least pre-L. (Also I forgot to actually leave in the code to handle the normal start service call when I adapted it for startJob :D ) I'll fix this and check it works on all platforms. https://codereview.chromium.org/2611333002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java (right): https://codereview.chromium.org/2611333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:226: static String getNotificationReply(Intent intent) { On 2017/01/09 14:28:05, Peter Beverloo wrote: > nit: why remove `private`? That's more restricted than package-level protected. Because it's now also called from NotificationService.getJobExtrasFromIntent. https://codereview.chromium.org/2611333002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java (right): https://codereview.chromium.org/2611333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java:47: if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.LOLLIPOP_MR1) { On 2017/01/09 14:28:05, Peter Beverloo wrote: > What's your reasoning for using the Job Scheduler for LMR1+ as opposed to N+? I guess because while GcmTaskService is unusable for our purposes due to its min 30s delay, so far I haven't seen evidence of JobScheduler causing us lag for notification clicks. (Using an adb command to watch posted tasks, it appears to execute immediately when I test). And Android already encourages use of JobScheduler for apps targeting L+ rather than starting services. But I see your point - the risk of lag exists, so there's no point taking it without good reason. That good reason could be to gather metrics on one platform so we know what the impact will be once we are forced to use this on future platforms. Could we run an experiment on N where only some notification clicks use JobScheduler, and time both code paths? For now I will limit this to >=N.
https://codereview.chromium.org/2611333002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java (right): https://codereview.chromium.org/2611333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java:47: if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.LOLLIPOP_MR1) { On 2017/01/11 14:56:18, awdf wrote: > On 2017/01/09 14:28:05, Peter Beverloo wrote: > > What's your reasoning for using the Job Scheduler for LMR1+ as opposed to N+? > > I guess because while GcmTaskService is unusable for our purposes due to its min > 30s delay, so far I haven't seen evidence of JobScheduler causing us lag for > notification clicks. (Using an adb command to watch posted tasks, it appears to > execute immediately when I test). And Android already encourages use of > JobScheduler for apps targeting L+ rather than starting services. > > But I see your point - the risk of lag exists, so there's no point taking it > without good reason. That good reason could be to gather metrics on one platform > so we know what the impact will be once we are forced to use this on future > platforms. Could we run an experiment on N where only some notification clicks > use JobScheduler, and time both code paths? > > For now I will limit this to >=N. Done. https://codereview.chromium.org/2611333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java:50: .Builder(0 /* jobId */, On 2017/01/09 14:28:05, Peter Beverloo wrote: > I think we'll need a magic value for our job. The documentation says: > > > Subsequent calls to cancel, or jobs created with the same jobId, will > > update the pre-existing job with the same id. Done. The only other place we create job scheduler jobs is in CrashReceiverService, and they've nabbed 42 of course .. wondering if it makes sense to put the job ids in one central place to decrease risk of future magic number clash, not sure where though? https://codereview.chromium.org/2611333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java:53: .setOverrideDeadline(1) On 2017/01/09 14:28:05, Peter Beverloo wrote: > I think we can set this to zero? Ah yes, I think I did this while trying to use the gcm OneOffTaskBuilder since it requires windowEnd > windowStart, but JobInfo.Builder doesn't seem to have this restriction. Done. https://codereview.chromium.org/2611333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java:68: @NonNull On 2017/01/09 14:28:05, Peter Beverloo wrote: > micro nit: up to you, but I'd prefer to not have @NonNull as well. Right now the > contract in our code is "non-null unless marked @Nullable". Done. https://codereview.chromium.org/2611333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java:117: dispatchIntentOnUIThread(intent); On 2017/01/10 18:10:24, awdf wrote: > Oops - I should really be calling onJobFinished here - > https://developer.android.com/reference/android/app/job/JobService.html#jobFi..., > boolean) - will fix Done.
peter@chromium.org changed reviewers: + nyquist@chromium.org
lg, but this really needs a review by someone much more familiar with our Java code. +Tommy, would you mind giving this a look over? https://codereview.chromium.org/2611333002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java (right): https://codereview.chromium.org/2611333002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:1: // Copyright 2014 The Chromium Authors. All rights reserved. nit: 2017 https://codereview.chromium.org/2611333002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:23: @RequiresApi(api = Build.VERSION_CODES.LOLLIPOP) We don't seem to use @RequiresApi anywhere yet. I'd really like one of the Java owners to judge whether we should. In either case, if it's set on the class then it's redundant on the methods. (Even though getJobExtrasFromIntent is MR1, but since we'd only use it on N+ we can pick that as a baseline.) https://codereview.chromium.org/2611333002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:27: static final int JOB_ID = 21; nit: this could use a line of explanation at least. I think it'd be great to generalise a "registry" somewhere. https://codereview.chromium.org/2611333002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:75: jobFinished(params, false); This probably need a TODO for marking the job as finished at the right time? https://codereview.chromium.org/2611333002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java (right): https://codereview.chromium.org/2611333002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java:42: // Android encourages us to use the JobScheduler instead of starting services on N+ nit: I would add a few things in the comment here. 1) Why do we use the JobScheduler as opposed to the GCMNetworkManager? 2) Why is the deadline set to zero? It'd also be great if you could cover these in the CL description, since they'll likely be raised by the next reviewer as well. https://codereview.chromium.org/2611333002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java:42: // Android encourages us to use the JobScheduler instead of starting services on N+ nit: TODO for adding UMA?
https://codereview.chromium.org/2611333002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java (right): https://codereview.chromium.org/2611333002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2017/01/13 01:23:22, Peter Beverloo (OOO- Jan 19th) wrote: > nit: 2017 Done. https://codereview.chromium.org/2611333002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:23: @RequiresApi(api = Build.VERSION_CODES.LOLLIPOP) On 2017/01/13 01:23:21, Peter Beverloo (OOO- Jan 19th) wrote: > We don't seem to use @RequiresApi anywhere yet. I'd really like one of the Java > owners to judge whether we should. In either case, if it's set on the class then > it's redundant on the methods. (Even though getJobExtrasFromIntent is MR1, but > since we'd only use it on N+ we can pick that as a baseline.) Changed to @TargetApi(N) (since we do have other examples of TargetApi in the code, and it doesn't technically 'require' N) and removed the annotations on the methods. https://codereview.chromium.org/2611333002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:27: static final int JOB_ID = 21; On 2017/01/13 01:23:22, Peter Beverloo (OOO- Jan 19th) wrote: > nit: this could use a line of explanation at least. I think it'd be great to > generalise a "registry" somewhere. Done. Agree ideally this would be defined alongside other job ids, just unsure where to put it. (chrome/android/java/utils? in a new chrome/android/java/jobs package? defined as values in xml?? none of these seem great options) https://codereview.chromium.org/2611333002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:75: jobFinished(params, false); On 2017/01/13 01:23:22, Peter Beverloo (OOO- Jan 19th) wrote: > This probably need a TODO for marking the job as finished at the right time? Done. https://codereview.chromium.org/2611333002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java (right): https://codereview.chromium.org/2611333002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java:42: // Android encourages us to use the JobScheduler instead of starting services on N+ On 2017/01/13 01:23:22, Peter Beverloo (OOO- Jan 19th) wrote: > nit: I would add a few things in the comment here. > > 1) Why do we use the JobScheduler as opposed to the GCMNetworkManager? > 2) Why is the deadline set to zero? > > It'd also be great if you could cover these in the CL description, since they'll > likely be raised by the next reviewer as well. Done. https://codereview.chromium.org/2611333002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java:42: // Android encourages us to use the JobScheduler instead of starting services on N+ On 2017/01/13 01:23:22, Peter Beverloo (OOO- Jan 19th) wrote: > nit: TODO for adding UMA? Done.
Description was changed from ========== Android notifications: Use JobScheduler to start NotificationService on 22+ BUG=663427 ========== to ========== Android web notifications:Schedule job instead of starting service on N+ - Previously we always started a service when a web notification was clicked or dismissed. - Now we instead schedule a job to handle the notification intent on N+, since this is encouraged by Android. - We use the Android JobScheduler, rather than GcmNetworkManager or FirebaseDispatcher, since the JobScheduler allows us to execute immediately (hopefully). BUG=663427,680816 ==========
Description was changed from ========== Android web notifications:Schedule job instead of starting service on N+ - Previously we always started a service when a web notification was clicked or dismissed. - Now we instead schedule a job to handle the notification intent on N+, since this is encouraged by Android. - We use the Android JobScheduler, rather than GcmNetworkManager or FirebaseDispatcher, since the JobScheduler allows us to execute immediately (hopefully). BUG=663427,680816 ========== to ========== Android web notifications: Schedule job instead of starting service (N+) - Previously we always started a service when a web notification was clicked or dismissed. - Now we instead schedule a job to handle the notification intent on N+, since this is encouraged by Android. - We use the Android JobScheduler, rather than GcmNetworkManager or FirebaseDispatcher, since the JobScheduler allows us to execute immediately (hopefully). BUG=663427,680816 ==========
Description was changed from ========== Android web notifications: Schedule job instead of starting service (N+) - Previously we always started a service when a web notification was clicked or dismissed. - Now we instead schedule a job to handle the notification intent on N+, since this is encouraged by Android. - We use the Android JobScheduler, rather than GcmNetworkManager or FirebaseDispatcher, since the JobScheduler allows us to execute immediately (hopefully). BUG=663427,680816 ========== to ========== Android web notifications: Schedule job instead of starting service (N+) - Previously we always started a service when a web notification was clicked or dismissed. - Now we instead schedule a job to handle the notification intent on N+, since this is encouraged by Android. - We use the Android JobScheduler, rather than GcmNetworkManager or FirebaseDispatcher, since the JobScheduler allows us to execute immediately (hopefully). BUG=663427,680816 ==========
> On 2017/01/13 01:23:22, Peter Beverloo (OOO- Jan 19th) wrote: > > nit: I would add a few things in the comment here. > > > > 1) Why do we use the JobScheduler as opposed to the GCMNetworkManager? > > 2) Why is the deadline set to zero? > > > > It'd also be great if you could cover these in the CL description, since > they'll > > likely be raised by the next reviewer as well. > I've now updated the CL description too :)
The CQ bit was checked by awdf@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2611333002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java (right): https://codereview.chromium.org/2611333002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:69: ThreadUtils.runOnUiThread(new Runnable() { Isn't this already called on the main thread? From the documentation of onStartJob(...): "Override this method with the callback logic for your job. Any such logic needs to be performed on a separate thread, as this function is executed on your application's main thread."
https://codereview.chromium.org/2611333002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java (right): https://codereview.chromium.org/2611333002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:27: static final int JOB_ID = 21; Since this is a shared resource, I wonder if we should split this out to something that can be re-used across all of //chrome? https://codereview.chromium.org/2611333002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:72: NotificationService.dispatchIntentOnUIThread(NotificationJobService.this, intent); Is this a long-running thing to do, or is this quick? If it's long-running, could you pass along a callback and call jobFinished() in that callback instead? https://codereview.chromium.org/2611333002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:74: // TODO(awdf) This may be marking the job as finished too early. Nit: Missing : after ) https://codereview.chromium.org/2611333002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java (right): https://codereview.chromium.org/2611333002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java:46: // TODO(awdf) Add UMA to check this does not actually introduce noticeable latency. Nit: Missing : after )
https://codereview.chromium.org/2611333002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java (right): https://codereview.chromium.org/2611333002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:27: static final int JOB_ID = 21; On 2017/01/18 at 02:32:25, nyquist wrote: > Since this is a shared resource, I wonder if we should split this out to something that can be re-used across all of //chrome? Are you just talking about this job id or the whole JobService? Wrt the job id, we talked about putting all job ids in one place to reduce risk of collisions, just not sure where that place should be. Any ideas? (chrome/android/java/utils? in a new chrome/android/java/jobs package? defined as values in xml??) Currently there's just this one and CrashReceiverService.MINIDUMP_UPLOADING_JOB_ID. (If we do that I'll rename this one to something more specific to notifcation jobs). https://codereview.chromium.org/2611333002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:69: ThreadUtils.runOnUiThread(new Runnable() { On 2017/01/17 at 22:36:45, nyquist wrote: > Isn't this already called on the main thread? > From the documentation of onStartJob(...): > "Override this method with the callback logic for your job. Any such logic needs to be performed on a separate thread, as this function is executed on your application's main thread." You're right, we can just call it directly. Done. https://codereview.chromium.org/2611333002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:72: NotificationService.dispatchIntentOnUIThread(NotificationJobService.this, intent); On 2017/01/18 at 02:32:25, nyquist wrote: > Is this a long-running thing to do, or is this quick? If it's long-running, could you pass along a callback and call jobFinished() in that callback instead? So dispatchIntentOnUIThread starts up Chrome synchronously (which can take a while if it's not already running), and then calls through to nativeOnNotificationClicked/Closed (which kicks off some asynchronous stuff to handle the event) before returning. I could pass a callback to call at the end of dispatchIntentOnUIThread, but it would have the same effect as calling jobFinished here. Or we could attempt to call jobFinished after native has handled the event, (this would ensure we keep the wakelock while native is processing the event), but we can't pass a java callback to native (can we?!) so I guess we would have to pass the job parameters through when we call the native methods and have native call a java method with them when done.. This is a bit out of scope right now though, hence the TODO below (now updated to explain this). https://codereview.chromium.org/2611333002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:74: // TODO(awdf) This may be marking the job as finished too early. On 2017/01/18 at 02:32:25, nyquist wrote: > Nit: Missing : after ) Done. https://codereview.chromium.org/2611333002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java (right): https://codereview.chromium.org/2611333002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java:46: // TODO(awdf) Add UMA to check this does not actually introduce noticeable latency. On 2017/01/18 at 02:32:25, nyquist wrote: > Nit: Missing : after ) Done.
ctate@google.com changed reviewers: + ctate@google.com
https://codereview.chromium.org/2611333002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java (right): https://codereview.chromium.org/2611333002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:70: return true; This is incoherent. Calling jobFinished() means you're telling the OS that you've done all the needful work of this job and it is to be stopped right away; but then returning 'true' from onStartJob() tells the OS that you are *not* done with your job's work, and it should keep you running until you call jobFinished() ... which you've already done. Just return 'false' from onStartJob(); you needn't call jobFinished() explicitly at all. Now, if you do eventually choose to wait for Chrome native code to acknowledge completion of (asynchronous) work [per awdf's TODO], then you'll need to refactor this anyway: in that case, return 'true' without calling jobFinished() here, and then call jobFinished() when the Chrome native code is done handling its end of things.
https://codereview.chromium.org/2611333002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java (right): https://codereview.chromium.org/2611333002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:70: return true; On 2017/01/18 at 20:20:50, ctate wrote: > This is incoherent. Calling jobFinished() means you're telling the OS that you've done all the needful work of this job and it is to be stopped right away; but then returning 'true' from onStartJob() tells the OS that you are *not* done with your job's work, and it should keep you running until you call jobFinished() ... which you've already done. > > Just return 'false' from onStartJob(); you needn't call jobFinished() explicitly at all. > > Now, if you do eventually choose to wait for Chrome native code to acknowledge completion of (asynchronous) work [per awdf's TODO], then you'll need to refactor this anyway: in that case, return 'true' without calling jobFinished() here, and then call jobFinished() when the Chrome native code is done handling its end of things. Thanks! I've updated the patch to return false and not call jobFinished.
The CQ bit was checked by awdf@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2611333002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java (right): https://codereview.chromium.org/2611333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:53: */ Since this API might be new to many people, could you add an @return documentation, maybe even including what that means for the API? https://codereview.chromium.org/2611333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:61: } Nit: empty line after this conditional? https://codereview.chromium.org/2611333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:62: final Intent intent = Nit: unnecessary final now. https://codereview.chromium.org/2611333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:65: NotificationService.dispatchIntentOnUIThread(NotificationJobService.this, intent); Do we want to assert we're on the UI thread with ThreadUtils.assertOnUiThread()? https://codereview.chromium.org/2611333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:67: // TODO(awdf): Maybe we should return true here and call jobFinished to release the wake Is the plan to do this as part of the work in the current bug? If not, could you file a bug about this follow up? TODOs with "maybe do foo" often does not get followed up. https://codereview.chromium.org/2611333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:74: return false; Could you add a short comment about us not being able to stop any actual work here? https://codereview.chromium.org/2611333002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java (right): https://codereview.chromium.org/2611333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java:44: // rather than GcmNetworkManager or FirebaseDispatcher since the JobScheduler allows Nit: FirebaseJobDispatcher (same with CL description)
https://codereview.chromium.org/2611333002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java (right): https://codereview.chromium.org/2611333002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:27: static final int JOB_ID = 21; On 2017/01/18 15:37:58, awdf wrote: > On 2017/01/18 at 02:32:25, nyquist wrote: > > Since this is a shared resource, I wonder if we should split this out to > something that can be re-used across all of //chrome? > > Are you just talking about this job id or the whole JobService? > > Wrt the job id, we talked about putting all job ids in one place to reduce risk > of collisions, just not sure where that place should be. Any ideas? > (chrome/android/java/utils? in a new chrome/android/java/jobs package? defined > as values in xml??) Currently there's just this one and > CrashReceiverService.MINIDUMP_UPLOADING_JOB_ID. (If we do that I'll rename this > one to something more specific to notifcation jobs). Yeah, I think we might have to deal with that if we do a generic job service. Let's leave this one here for now. https://codereview.chromium.org/2611333002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:72: NotificationService.dispatchIntentOnUIThread(NotificationJobService.this, intent); On 2017/01/18 15:37:58, awdf wrote: > On 2017/01/18 at 02:32:25, nyquist wrote: > > Is this a long-running thing to do, or is this quick? If it's long-running, > could you pass along a callback and call jobFinished() in that callback instead? > > So dispatchIntentOnUIThread starts up Chrome synchronously (which can take a > while if it's not already running), and then calls through to > nativeOnNotificationClicked/Closed (which kicks off some asynchronous stuff to > handle the event) before returning. > > I could pass a callback to call at the end of dispatchIntentOnUIThread, but it > would have the same effect as calling jobFinished here. > > Or we could attempt to call jobFinished after native has handled the event, > (this would ensure we keep the wakelock while native is processing the event), > but we can't pass a java callback to native (can we?!) so I guess we would have > to pass the job parameters through when we call the native methods and have > native call a java method with them when done.. This is a bit out of scope right > now though, hence the TODO below (now updated to explain this). Well, there are ways to store a callback and later invoke it from C++, but I agree, this would better be done in a follow-up.
Description was changed from ========== Android web notifications: Schedule job instead of starting service (N+) - Previously we always started a service when a web notification was clicked or dismissed. - Now we instead schedule a job to handle the notification intent on N+, since this is encouraged by Android. - We use the Android JobScheduler, rather than GcmNetworkManager or FirebaseDispatcher, since the JobScheduler allows us to execute immediately (hopefully). BUG=663427,680816 ========== to ========== Android web notifications: Schedule job instead of starting service (N+) - Previously we always started a service when a web notification was clicked or dismissed. - Now we instead schedule a job to handle the notification intent on N+, since this is encouraged by Android. - We use the Android JobScheduler, rather than GcmNetworkManager or FirebaseJobDispatcher, since the JobScheduler allows us to execute immediately (hopefully). BUG=663427,680816 ==========
awdf@chromium.org changed required reviewers: + nyquist@chromium.org
Addressing Tommy's comments. https://codereview.chromium.org/2611333002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java (right): https://codereview.chromium.org/2611333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:53: */ On 2017/01/23 at 22:49:22, nyquist wrote: > Since this API might be new to many people, could you add an @return documentation, maybe even including what that means for the API? Done. This basically now duplicates JobScheduler.onStartJob documentation, but hopefully is still helpful. https://codereview.chromium.org/2611333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:61: } On 2017/01/23 at 22:49:22, nyquist wrote: > Nit: empty line after this conditional? Done. https://codereview.chromium.org/2611333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:62: final Intent intent = On 2017/01/23 at 22:49:22, nyquist wrote: > Nit: unnecessary final now. Done. https://codereview.chromium.org/2611333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:65: NotificationService.dispatchIntentOnUIThread(NotificationJobService.this, intent); On 2017/01/23 at 22:49:22, nyquist wrote: > Do we want to assert we're on the UI thread with ThreadUtils.assertOnUiThread()? Probably - we only expect to be called by Android on the UI thread but it's a good idea to make this assumption explicit. Done. https://codereview.chromium.org/2611333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:67: // TODO(awdf): Maybe we should return true here and call jobFinished to release the wake On 2017/01/23 at 22:49:22, nyquist wrote: > Is the plan to do this as part of the work in the current bug? If not, could you file a bug about this follow up? TODOs with "maybe do foo" often does not get followed up. Done - crbug.com/685197 & updated the TODO. https://codereview.chromium.org/2611333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:74: return false; On 2017/01/23 at 22:49:22, nyquist wrote: > Could you add a short comment about us not being able to stop any actual work here? Done. https://codereview.chromium.org/2611333002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java (right): https://codereview.chromium.org/2611333002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java:44: // rather than GcmNetworkManager or FirebaseDispatcher since the JobScheduler allows On 2017/01/23 at 22:49:22, nyquist wrote: > Nit: FirebaseJobDispatcher (same with CL description) Done. https://codereview.chromium.org/2611333002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java (right): https://codereview.chromium.org/2611333002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:87: // (crbug.com/685197) it may by infeasible to cancel this halfway through. perhaps this is too pessimistic - peter@ ?
lgtm https://codereview.chromium.org/2611333002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java (right): https://codereview.chromium.org/2611333002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:68: return false; Nit: If this ever happens, should we log it? Or is this expected to happen every so often and isn't really interesting? https://codereview.chromium.org/2611333002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:87: // (crbug.com/685197) it may by infeasible to cancel this halfway through. On 2017/01/25 16:16:01, awdf wrote: > perhaps this is too pessimistic - peter@ ? Looks OK to me.
The CQ bit was checked by awdf@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
pressed 'commit' before I saw this last nit but it's a fair comment https://codereview.chromium.org/2611333002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java (right): https://codereview.chromium.org/2611333002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:68: return false; On 2017/01/25 at 17:14:56, nyquist wrote: > Nit: If this ever happens, should we log it? Or is this expected to happen every so often and isn't really interesting? Afaict, this shouldn't happen as our broadcast receiver which starts jobs when it receives intents is not exported, and the only place we send it intents is in NotificationPlatformBridge where these fields are always set. So yes we should probably either remove this check or log this failure case, here and in NotificationService.onHandleIntent. Perhaps this check should even be pushed earlier to NotificationService.BroadcastReceiver.onReceive before the paths diverge. I'll file a bug.
lgtm, thanks for filing all the follow-up bugs! https://codereview.chromium.org/2611333002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java (right): https://codereview.chromium.org/2611333002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java:7: import android.content.Intent; Is this necessary for the documentation link? It's unused otherwise... https://codereview.chromium.org/2611333002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java (right): https://codereview.chromium.org/2611333002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:87: // (crbug.com/685197) it may by infeasible to cancel this halfway through. On 2017/01/25 17:14:56, nyquist wrote: > On 2017/01/25 16:16:01, awdf wrote: > > perhaps this is too pessimistic - peter@ ? > > Looks OK to me. +1 It is important to note here that the synchronous job processing in onStartJob solely cares about the Android side of things. Chrome will have plenty of work to do after that method finishes. I have no idea what we'd do when onStopJob() is called. Theoretically we could kill the SW/renderer and abort things immediately, but we'll have to carefully think about the impact of that.
thanks for the lgtm peter! https://codereview.chromium.org/2611333002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java (right): https://codereview.chromium.org/2611333002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java:7: import android.content.Intent; On 2017/01/25 at 18:12:19, Peter Beverloo wrote: > Is this necessary for the documentation link? It's unused otherwise... It's used by the doc link yes (although actually I could have removed the param and brackets and it would still link to the method.. I am not going to file a bug for this though :P) https://codereview.chromium.org/2611333002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java (right): https://codereview.chromium.org/2611333002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java:87: // (crbug.com/685197) it may by infeasible to cancel this halfway through. On 2017/01/25 at 18:12:19, Peter Beverloo wrote: > On 2017/01/25 17:14:56, nyquist wrote: > > On 2017/01/25 16:16:01, awdf wrote: > > > perhaps this is too pessimistic - peter@ ? > > > > Looks OK to me. > > +1 > > It is important to note here that the synchronous job processing in onStartJob solely cares about the Android side of things. Chrome will have plenty of work to do after that method finishes. > > I have no idea what we'd do when onStopJob() is called. Theoretically we could kill the SW/renderer and abort things immediately, but we'll have to carefully think about the impact of that. yeah that's what I was thinking.
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1485365985109390,
"parent_rev": "27b5996c01a50eacdd3f3e62bf6877fd6aabaec0", "commit_rev":
"d54134355647e110b2d6793d2b257dceb4b706ed"}
Message was sent while issue was closed.
Description was changed from ========== Android web notifications: Schedule job instead of starting service (N+) - Previously we always started a service when a web notification was clicked or dismissed. - Now we instead schedule a job to handle the notification intent on N+, since this is encouraged by Android. - We use the Android JobScheduler, rather than GcmNetworkManager or FirebaseJobDispatcher, since the JobScheduler allows us to execute immediately (hopefully). BUG=663427,680816 ========== to ========== Android web notifications: Schedule job instead of starting service (N+) - Previously we always started a service when a web notification was clicked or dismissed. - Now we instead schedule a job to handle the notification intent on N+, since this is encouraged by Android. - We use the Android JobScheduler, rather than GcmNetworkManager or FirebaseJobDispatcher, since the JobScheduler allows us to execute immediately (hopefully). BUG=663427,680816 Review-Url: https://codereview.chromium.org/2611333002 Cr-Commit-Position: refs/heads/master@{#446072} Committed: https://chromium.googlesource.com/chromium/src/+/d54134355647e110b2d6793d2b25... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/d54134355647e110b2d6793d2b25... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
