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

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:
1 month, 1 week ago by Peter Beverloo
Modified:
1 day 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
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 ...
1 month, 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 ...
1 month 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 ...
1 month 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 ...
1 month 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 ...
1 month 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 ...
1 month 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: > ...
1 month 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 ...
1 month 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 ...
1 month 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 > ...
1 month 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 > ...
1 month 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; ...
4 weeks, 1 day 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 ...
4 weeks, 1 day ago (2017-02-23 17:28:09 UTC) #29
nyquist
4 weeks, 1 day 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 d1a128a62