Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2690163008: Route through a JobService when receiving a message for the GCM Driver (Closed)

Created:
10 months ago by Peter Beverloo
Modified:
8 months, 3 weeks ago
Reviewers:
nyquist, awdf
CC:
agrieve+watch_chromium.org, android-webview-reviews_chromium.org, awdf+watch_chromium.org, chromium-reviews, johnme+watch_chromium.org, kalyank, mlamouri+watch-notifications_chromium.org, Peter Beverloo, sadrul, zea+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Route through a JobService when receiving a message for the GCM Driver There are cases when the message handling routine for a received GCM message may be limited to ten seconds of execution time. Route messages through the Job Scheduler since we may need more than that, which is the case for some of the GCM Driver's customers (e.g. Web Push Notifications.) BUG=688898

Patch Set 1 #

Patch Set 2 : Route through a JobService when receiving a message for the GCM Driver #

Total comments: 17

Patch Set 3 : Comments and build fix #

Total comments: 8

Patch Set 4 : findbugs #

Total comments: 14

Patch Set 5 : Route through a JobService when receiving a message for the GCM Driver #

Unified diffs Side-by-side diffs Delta from patch set Stats (+544 lines, -62 lines) Patch
M android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/android/java/AndroidManifest.xml View 1 2 3 4 1 chunk +5 lines, -0 lines 0 comments Download
A chrome/android/java/src/org/chromium/chrome/browser/JobSchedulerConstants.java View 1 2 3 4 1 chunk +25 lines, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java View 1 chunk +0 lines, -4 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java View 2 chunks +2 lines, -1 line 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java View 1 2 4 chunks +63 lines, -23 lines 0 comments Download
A chrome/android/java/src/org/chromium/chrome/browser/services/gcm/GCMJobService.java View 1 chunk +55 lines, -0 lines 0 comments Download
M chrome/android/java_sources.gni View 1 2 3 4 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java View 1 2 2 chunks +6 lines, -1 line 0 comments Download
M components/gcm_driver/android/BUILD.gn View 1 1 chunk +12 lines, -0 lines 0 comments Download
M components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java View 3 chunks +5 lines, -33 lines 0 comments Download
A components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMMessage.java View 1 2 3 4 1 chunk +177 lines, -0 lines 0 comments Download
A components/gcm_driver/android/junit/src/org/chromium/components/gcm_driver/GCMMessageTest.java View 1 1 chunk +178 lines, -0 lines 0 comments Download
M testing/buildbot/chromium.linux.json View 1 2 chunks +6 lines, -0 lines 0 comments Download
M testing/buildbot/gn_isolate_map.pyl View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M testing/buildbot/manage.py View 1 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 32 (18 generated)
Peter Beverloo
+awdf for a first round of review The GCMMessage class will be getting some unit ...
10 months ago (2017-02-15 20:25:42 UTC) #2
Peter Beverloo
This is ready for review now. +awdf for a full review +nyquist since I'm doing ...
10 months ago (2017-02-16 15:47:40 UTC) #6
awdf
https://codereview.chromium.org/2690163008/diff/20001/chrome/android/java/AndroidManifest.xml File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2690163008/diff/20001/chrome/android/java/AndroidManifest.xml#newcode768 chrome/android/java/AndroidManifest.xml:768: <service android:name="org.chromium.chrome.browser.services.gcm.GCMJobService" Something I hadn't fully appreciated when creating ...
10 months ago (2017-02-16 21:26:48 UTC) #9
Peter Beverloo
https://codereview.chromium.org/2690163008/diff/20001/chrome/android/java/AndroidManifest.xml File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2690163008/diff/20001/chrome/android/java/AndroidManifest.xml#newcode768 chrome/android/java/AndroidManifest.xml:768: <service android:name="org.chromium.chrome.browser.services.gcm.GCMJobService" On 2017/02/16 21:26:48, awdf wrote: > Something ...
9 months, 4 weeks ago (2017-02-17 15:43:59 UTC) #11
Peter Beverloo
https://codereview.chromium.org/2690163008/diff/40001/components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMMessage.java File components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMMessage.java (right): https://codereview.chromium.org/2690163008/diff/40001/components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMMessage.java#newcode134 components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMMessage.java:134: return mRawData; > EI_EXPOSE_REP: May expose internal representation by ...
9 months, 4 weeks ago (2017-02-17 17:33:22 UTC) #15
awdf
https://codereview.chromium.org/2690163008/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java File chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java (right): https://codereview.chromium.org/2690163008/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java#newcode100 chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java:100: public static void scheduleOrDispatchMessageToDriver( On 2017/02/17 at 15:43:59, Peter ...
9 months, 4 weeks ago (2017-02-18 07:51:41 UTC) #16
Peter Beverloo
PTAL https://codereview.chromium.org/2690163008/diff/40001/components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMMessage.java File components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMMessage.java (right): https://codereview.chromium.org/2690163008/diff/40001/components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMMessage.java#newcode134 components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMMessage.java:134: return mRawData; On 2017/02/18 07:51:41, awdf wrote: > ...
9 months, 3 weeks ago (2017-02-21 14:56:10 UTC) #18
nyquist
This CL looks very reasonable to me. https://codereview.chromium.org/2690163008/diff/60001/android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java File android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java (right): https://codereview.chromium.org/2690163008/diff/60001/android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java#newcode38 android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java:38: private static ...
9 months, 3 weeks ago (2017-02-22 11:38:02 UTC) #22
awdf
lgtm https://codereview.chromium.org/2690163008/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java File chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java (right): https://codereview.chromium.org/2690163008/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java#newcode55 chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java:55: Log.e(TAG, "Received an invalid GCM Message", e); probably ...
9 months, 3 weeks ago (2017-02-23 12:40:18 UTC) #23
johnmarienettal
On 2017/02/23 12:40:18, awdf wrote: > lgtm > > https://codereview.chromium.org/2690163008/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java > File > chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java > ...
9 months, 3 weeks ago (2017-02-23 12:48:39 UTC) #24
johnmarienettal
On 2017/02/23 12:40:18, awdf wrote: > lgtm > > https://codereview.chromium.org/2690163008/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java > File > chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java > ...
9 months, 3 weeks ago (2017-02-23 12:48:41 UTC) #25
Peter Beverloo
Thanks both! https://codereview.chromium.org/2690163008/diff/60001/android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java File android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java (right): https://codereview.chromium.org/2690163008/diff/60001/android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java#newcode38 android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java:38: private static final int MINIDUMP_UPLOADING_JOB_ID = 2; ...
9 months, 3 weeks ago (2017-02-23 17:26:56 UTC) #28
Peter Beverloo
https://codereview.chromium.org/2690163008/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java File chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java (right): https://codereview.chromium.org/2690163008/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java#newcode55 chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java:55: Log.e(TAG, "Received an invalid GCM Message", e); On 2017/02/23 ...
9 months, 3 weeks ago (2017-02-23 17:28:09 UTC) #29
nyquist
9 months, 3 weeks ago (2017-02-24 08:22:40 UTC) #32
lgtm

Also, up to you whether you would want this to use this framework:
https://codereview.chromium.org/2714463002/

https://codereview.chromium.org/2690163008/diff/60001/components/gcm_driver/a...
File
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMMessage.java
(right):

https://codereview.chromium.org/2690163008/diff/60001/components/gcm_driver/a...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMMessage.java:75:
mCollapseKey = extras.getString(bundleCollapseKey); // May be null.
On 2017/02/23 17:26:56, Peter Beverloo wrote:
> On 2017/02/22 11:38:02, nyquist wrote:
> > The nittiest of nits: I think your formatted removed one of the spaces
before
> > the //. I think we should have two.
> 
> `git cl format` warns if I add two spaces :/

I see. That's messed up. :-/

Powered by Google App Engine
This is Rietveld 0eb02b776