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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 1 week ago by Peter Beverloo
Modified:
1 month, 1 week ago
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
Commit queue not available (can’t edit this change).

Messages

Total messages: 32 (18 generated)
Peter Beverloo
+awdf for a first round of review The GCMMessage class will be getting some unit ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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: > ...
2 months, 1 week ago (2017-02-21 14:56:10 UTC) #18
nyquist (nychthemeron ping)
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 ...
2 months, 1 week 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 ...
2 months 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 > ...
2 months 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 > ...
2 months 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; ...
2 months 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 ...
2 months ago (2017-02-23 17:28:09 UTC) #29
nyquist (nychthemeron ping)
2 months 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. :-/
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46