Route through a JobService when receiving a message for the GCM Driver

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

[Peter Beverloo, 2017-02-15 20:25:42]

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

[Peter Beverloo, 2017-02-15 20:25:42]

+awdf for a first round of review

The GCMMessage class will be getting some unit tests, and I'll figure out how to
add the raw data to the PersistableBundle.

[Peter Beverloo, 2017-02-16 15:47:03]

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

[commit-bot: I haz the power, 2017-02-16 15:47:24]

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

[Peter Beverloo, 2017-02-16 15:47:40]

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

[Peter Beverloo, 2017-02-16 15:47:40]

This is ready for review now.

+awdf for a full review
+nyquist since I'm doing JobScheduler and Manifest stuff, and word has it that
you're working on generalizing that. Worth waiting? (This is for M58.)

[commit-bot: I haz the power, 2017-02-16 16:24:10]

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

[commit-bot: I haz the power, 2017-02-16 16:24:11]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[awdf, 2017-02-16 21:26:48]

https://codereview.chromium.org/2690163008/diff/20001/chrome/android/java/And...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/2690163008/diff/20001/chrome/android/java/And...
chrome/android/java/AndroidManifest.xml:768: <service
android:name="org.chromium.chrome.browser.services.gcm.GCMJobService"
Something I hadn't fully appreciated when creating the NotificationJobService is
that this is going to be set in stone forever now it's in the manifest. We could
get weird crashes if we delete or change the name of the service and someone
upgrades Chrome with a job pending for the old job service. So Android's advice
is to treat JobServices as a public api and not change them.

Hence the thought here is to create just one JobService for Chrome , it's
nonideal but more maintainable. Not sure what the advice is right now in terms
of waiting for that, Tommy?

https://codereview.chromium.org/2690163008/diff/20001/chrome/android/java/src...
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...
chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java:77:
* of the browser process, and forward the message to the GCM Driver.
nit: could add "Must be called on the UI thread."

https://codereview.chromium.org/2690163008/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java:80:
public static void dispatchMessageToDriver(Context applicationContext,
GCMMessage message) {
nit: can you make this method default (package) protected instead of public?
it's only called from this package

https://codereview.chromium.org/2690163008/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java:100:
public static void scheduleOrDispatchMessageToDriver(
nit: place this method above dispatchMessageToDriver?

https://codereview.chromium.org/2690163008/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java:104:
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) {
nit: I'm wondering if we should have a common shouldUseJobScheduler method for
all job services in case we decide to change this to M or O in the future

if we end up with only one job service this is moot though

https://codereview.chromium.org/2690163008/diff/20001/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/20001/components/gcm_driver/a...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMMessage.java:23:
public class GCMMessage {
nit: worth implementing hashcode and equals()? might make tests simpler..
Android Studio / IntelliJ can generate these in 3 clicks, I don't know about
Sublime..

https://codereview.chromium.org/2690163008/diff/20001/components/gcm_driver/a...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMMessage.java:72:
return;
shouldn't we throw an IllegalArgumentException or something here? previously it
early-returned from ChromeListenerService.pushMessageReceived if there was no
subtype but now it will carry on blithely, until validatePersistableBundle in
the schedule-job case anyway, and completely in the non-job scheduler case,
afaict.

https://codereview.chromium.org/2690163008/diff/20001/components/gcm_driver/a...
File
components/gcm_driver/android/junit/src/org/chromium/components/gcm_driver/GCMMessageTest.java
(right):

https://codereview.chromium.org/2690163008/diff/20001/components/gcm_driver/a...
components/gcm_driver/android/junit/src/org/chromium/components/gcm_driver/GCMMessageTest.java:57:
assertEquals("MyCollapseKey", message.getCollapseKey());
check message.getRawData too?

[Peter Beverloo, 2017-02-17 15:43:49]

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

[Peter Beverloo, 2017-02-17 15:43:59]

https://codereview.chromium.org/2690163008/diff/20001/chrome/android/java/And...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/2690163008/diff/20001/chrome/android/java/And...
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 I hadn't fully appreciated when creating the NotificationJobService
is
> that this is going to be set in stone forever now it's in the manifest. We
could
> get weird crashes if we delete or change the name of the service and someone
> upgrades Chrome with a job pending for the old job service. So Android's
advice
> is to treat JobServices as a public api and not change them.
> 
> Hence the thought here is to create just one JobService for Chrome , it's
> nonideal but more maintainable. Not sure what the advice is right now in terms
> of waiting for that, Tommy?

I'm all for that, and am definitely up for some waiting, but since I'm OOO the
week of branch point I don't want to delay this until the very last minute
either. We do need this for M58.

https://codereview.chromium.org/2690163008/diff/20001/chrome/android/java/src...
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...
chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java:77:
* of the browser process, and forward the message to the GCM Driver.
On 2017/02/16 21:26:48, awdf wrote:
> nit: could add "Must be called on the UI thread."

Done.

https://codereview.chromium.org/2690163008/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java:80:
public static void dispatchMessageToDriver(Context applicationContext,
GCMMessage message) {
On 2017/02/16 21:26:48, awdf wrote:
> nit: can you make this method default (package) protected instead of public?
> it's only called from this package

Done.

https://codereview.chromium.org/2690163008/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java:100:
public static void scheduleOrDispatchMessageToDriver(
On 2017/02/16 21:26:48, awdf wrote:
> nit: place this method above dispatchMessageToDriver?

Done. Why?

https://codereview.chromium.org/2690163008/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java:104:
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) {
On 2017/02/16 21:26:48, awdf wrote:
> nit: I'm wondering if we should have a common shouldUseJobScheduler method for
> all job services in case we decide to change this to M or O in the future
> 
> if we end up with only one job service this is moot though

I think that'll depend on the feature, doesn't it? Don't feel strongly either
way, it could be a good way to centralize UMA too.

https://codereview.chromium.org/2690163008/diff/20001/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/20001/components/gcm_driver/a...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMMessage.java:23:
public class GCMMessage {
On 2017/02/16 21:26:48, awdf wrote:
> nit: worth implementing hashcode and equals()? might make tests simpler..
> Android Studio / IntelliJ can generate these in 3 clicks, I don't know about
> Sublime..

I really don't feel strongly, but mostly due to my ignorance about Java best
practices. Sublime doesn't create them for me at all, so I've left them out
unless you feel strongly.

https://codereview.chromium.org/2690163008/diff/20001/components/gcm_driver/a...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMMessage.java:72:
return;
On 2017/02/16 21:26:48, awdf wrote:
> shouldn't we throw an IllegalArgumentException or something here? previously
it
> early-returned from ChromeListenerService.pushMessageReceived if there was no
> subtype but now it will carry on blithely, until validatePersistableBundle in
> the schedule-job case anyway, and completely in the non-job scheduler case,
> afaict.

Done. I've added a try/catch in ChromeGcmListenerService because we do get this
from an external source, so shouldn't cause crashes over it.

https://codereview.chromium.org/2690163008/diff/20001/components/gcm_driver/a...
File
components/gcm_driver/android/junit/src/org/chromium/components/gcm_driver/GCMMessageTest.java
(right):

https://codereview.chromium.org/2690163008/diff/20001/components/gcm_driver/a...
components/gcm_driver/android/junit/src/org/chromium/components/gcm_driver/GCMMessageTest.java:57:
assertEquals("MyCollapseKey", message.getCollapseKey());
On 2017/02/16 21:26:48, awdf wrote:
> check message.getRawData too?

I'm testing getRawData elsewhere because its semantics are interesting (null vs.
empty buffer vs. filled buffer). See testRawDataSerializationBehaviour. Want me
to copy it here anyway?

[commit-bot: I haz the power, 2017-02-17 15:44:12]

Dry run: 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-02-17 17:25:07]

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

[commit-bot: I haz the power, 2017-02-17 17:25:08]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Peter Beverloo, 2017-02-17 17:33:22]

https://codereview.chromium.org/2690163008/diff/40001/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/40001/components/gcm_driver/a...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMMessage.java:134:
return mRawData;
> EI_EXPOSE_REP: May expose internal representation by returning reference to
mutable object
>   In class org.chromium.components.gcm_driver.GCMMessage
>   In method org.chromium.components.gcm_driver.GCMMessage.getRawData()
>   Field org.chromium.components.gcm_driver.GCMMessage.mRawData
>  At GCMMessage.java:[line 134]

I guess I either make a copy or suppress the warning (@SuppressFBWarnings). The
scope of this object is narrow enough to avoid making a copy - concerns with
suppressing the warning?

https://codereview.chromium.org/2690163008/diff/40001/components/gcm_driver/a...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMMessage.java:138:
return mDataKeysAndValuesArray;
> EI_EXPOSE_REP: May expose internal representation by returning reference to
mutable object
>   In class org.chromium.components.gcm_driver.GCMMessage
>   In method
org.chromium.components.gcm_driver.GCMMessage.getDataKeysAndValuesArray()
>   Field org.chromium.components.gcm_driver.GCMMessage.mDataKeysAndValuesArray
>   At GCMMessage.java:[line 138]

I guess I either make a copy or suppress the warning (@SuppressFBWarnings). The
scope of this object is narrow enough to avoid making a copy - concerns with
suppressing the warning?

https://codereview.chromium.org/2690163008/diff/40001/components/gcm_driver/a...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMMessage.java:157:
persistableBundle.putString(BUNDLE_KEY_RAW_DATA, new String());
> DM_STRING_VOID_CTOR: Method invokes inefficient new String() constructor
>   In class org.chromium.components.gcm_driver.GCMMessage
>   In method
org.chromium.components.gcm_driver.GCMMessage.toPersistableBundle()
>   At GCMMessage.java:[line 157]

I mean, I want a non-null empty string. What are the options here? Suppression
as well?

[awdf, 2017-02-18 07:51:41]

https://codereview.chromium.org/2690163008/diff/20001/chrome/android/java/src...
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...
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 Beverloo wrote:
> On 2017/02/16 21:26:48, awdf wrote:
> > nit: place this method above dispatchMessageToDriver?
> 
> Done. Why?

because this one calls dispatchMessageToDrive so logically comes between the
previous method & dispatchMessageToDrive (at least, this bugged me while
reviewing & scrolling, if I edited the code I probably wouldn't notice as I'd
just ctrl-click the method name in my IDE to jump about.. so this is a really
micro nit!)

https://codereview.chromium.org/2690163008/diff/40001/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/40001/components/gcm_driver/a...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMMessage.java:134:
return mRawData;
On 2017/02/17 at 17:33:22, Peter Beverloo wrote:
> > EI_EXPOSE_REP: May expose internal representation by returning reference to
mutable object
> >   In class org.chromium.components.gcm_driver.GCMMessage
> >   In method org.chromium.components.gcm_driver.GCMMessage.getRawData()
> >   Field org.chromium.components.gcm_driver.GCMMessage.mRawData
> >  At GCMMessage.java:[line 134]
> 
> I guess I either make a copy or suppress the warning (@SuppressFBWarnings).
The scope of this object is narrow enough to avoid making a copy - concerns with
suppressing the warning?

I guess it's okay since the two places it's called can be trusted not to mutate
the data. Probably worth a comment though.

(huh, never seen this kind of warning before, looks useful, where do I see it?
it is just part of the compile output?)

https://codereview.chromium.org/2690163008/diff/40001/components/gcm_driver/a...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMMessage.java:157:
persistableBundle.putString(BUNDLE_KEY_RAW_DATA, new String());
On 2017/02/17 at 17:33:22, Peter Beverloo wrote:
> > DM_STRING_VOID_CTOR: Method invokes inefficient new String() constructor
> >   In class org.chromium.components.gcm_driver.GCMMessage
> >   In method
org.chromium.components.gcm_driver.GCMMessage.toPersistableBundle()
> >   At GCMMessage.java:[line 157]
> 
> I mean, I want a non-null empty string. What are the options here? Suppression
as well?

can't you just pass "" ?

[Peter Beverloo, 2017-02-21 14:56:01]

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

[Peter Beverloo, 2017-02-21 14:56:10]

PTAL

https://codereview.chromium.org/2690163008/diff/40001/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/40001/components/gcm_driver/a...
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:
> On 2017/02/17 at 17:33:22, Peter Beverloo wrote:
> > > EI_EXPOSE_REP: May expose internal representation by returning reference
to
> mutable object
> > >   In class org.chromium.components.gcm_driver.GCMMessage
> > >   In method org.chromium.components.gcm_driver.GCMMessage.getRawData()
> > >   Field org.chromium.components.gcm_driver.GCMMessage.mRawData
> > >  At GCMMessage.java:[line 134]
> > 
> > I guess I either make a copy or suppress the warning (@SuppressFBWarnings).
> The scope of this object is narrow enough to avoid making a copy - concerns
with
> suppressing the warning?
> 
> I guess it's okay since the two places it's called can be trusted not to
mutate
> the data. Probably worth a comment though.
> 
> (huh, never seen this kind of warning before, looks useful, where do I see it?
> it is just part of the compile output?)
> 

Done.

https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...

https://codereview.chromium.org/2690163008/diff/40001/components/gcm_driver/a...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMMessage.java:138:
return mDataKeysAndValuesArray;
On 2017/02/17 17:33:22, Peter Beverloo wrote:
> > EI_EXPOSE_REP: May expose internal representation by returning reference to
> mutable object
> >   In class org.chromium.components.gcm_driver.GCMMessage
> >   In method
> org.chromium.components.gcm_driver.GCMMessage.getDataKeysAndValuesArray()
> >   Field
org.chromium.components.gcm_driver.GCMMessage.mDataKeysAndValuesArray
> >   At GCMMessage.java:[line 138]
> 
> I guess I either make a copy or suppress the warning (@SuppressFBWarnings).
The
> scope of this object is narrow enough to avoid making a copy - concerns with
> suppressing the warning?

Done.

https://codereview.chromium.org/2690163008/diff/40001/components/gcm_driver/a...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMMessage.java:157:
persistableBundle.putString(BUNDLE_KEY_RAW_DATA, new String());
On 2017/02/18 07:51:41, awdf wrote:
> On 2017/02/17 at 17:33:22, Peter Beverloo wrote:
> > > DM_STRING_VOID_CTOR: Method invokes inefficient new String() constructor
> > >   In class org.chromium.components.gcm_driver.GCMMessage
> > >   In method
> org.chromium.components.gcm_driver.GCMMessage.toPersistableBundle()
> > >   At GCMMessage.java:[line 157]
> > 
> > I mean, I want a non-null empty string. What are the options here?
Suppression
> as well?
> 
> can't you just pass "" ?

Let's try.

[commit-bot: I haz the power, 2017-02-21 14:56:20]

Dry run: 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-02-21 16:05:22]

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

[commit-bot: I haz the power, 2017-02-21 16:05:24]

Dry run: This issue passed the CQ dry run.

[nyquist, 2017-02-22 11:38:02]

This CL looks very reasonable to me.

https://codereview.chromium.org/2690163008/diff/60001/android_webview/java/sr...
File
android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java
(right):

https://codereview.chromium.org/2690163008/diff/60001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java:38:
private static final int MINIDUMP_UPLOADING_JOB_ID = 2;
Do we need to change this ID? Wouldn't the comment be enough, and refer to this
ID from the constants file?

https://codereview.chromium.org/2690163008/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/JobSchedulerConstants.java
(right):

https://codereview.chromium.org/2690163008/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/JobSchedulerConstants.java:16:
public static final int NOTIFICATION_SERVICE_JOB_ID = 0;
Are we sure we want to risk using 0 for a unique ID? In case someone assumes
it's a non-good number, etc.?

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:33:
private String mSenderId;
I think all these members can be final?

https://codereview.chromium.org/2690163008/diff/60001/components/gcm_driver/a...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMMessage.java:62:
final String bundleCollapseKey = "collapse_key";
Nit: Why are these final? I don't think we usually do that in chromium, even
though it never changes, unless it's required for callbacks, anonymous inner
classes, etc.

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.
The nittiest of nits: I think your formatted removed one of the spaces before
the //. I think we should have two.

[awdf, 2017-02-23 12:40:18]

lgtm

https://codereview.chromium.org/2690163008/diff/60001/chrome/android/java/src...
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...
chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java:55:
Log.e(TAG, "Received an invalid GCM Message", e);
probably not important, but previously this was just a Log.w not .e

[johnmarienettal, 2017-02-23 12:48:39]

On 2017/02/23 12:40:18, awdf wrote:
> lgtm
> 
>
https://codereview.chromium.org/2690163008/diff/60001/chrome/android/java/src...
> 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...
>
chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java:55:
> Log.e(TAG, "Received an invalid GCM Message", e);
> probably not important, but previously this was just a Log.w not .e



W

[johnmarienettal, 2017-02-23 12:48:41]

On 2017/02/23 12:40:18, awdf wrote:
> lgtm
> 
>
https://codereview.chromium.org/2690163008/diff/60001/chrome/android/java/src...
> 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...
>
chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java:55:
> Log.e(TAG, "Received an invalid GCM Message", e);
> probably not important, but previously this was just a Log.w not .e



W

[Peter Beverloo, 2017-02-23 17:25:13]

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

[commit-bot: I haz the power, 2017-02-23 17:25:45]

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

[Peter Beverloo, 2017-02-23 17:26:56]

Thanks both!

https://codereview.chromium.org/2690163008/diff/60001/android_webview/java/sr...
File
android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java
(right):

https://codereview.chromium.org/2690163008/diff/60001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/crash/CrashReceiverService.java:38:
private static final int MINIDUMP_UPLOADING_JOB_ID = 2;
On 2017/02/22 11:38:02, nyquist wrote:
> Do we need to change this ID? Wouldn't the comment be enough, and refer to
this
> ID from the constants file?

Done.

https://codereview.chromium.org/2690163008/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/JobSchedulerConstants.java
(right):

https://codereview.chromium.org/2690163008/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/JobSchedulerConstants.java:16:
public static final int NOTIFICATION_SERVICE_JOB_ID = 0;
On 2017/02/22 11:38:02, nyquist wrote:
> Are we sure we want to risk using 0 for a unique ID? In case someone assumes
> it's a non-good number, etc.?

Fair enough - updated.

https://codereview.chromium.org/2690163008/diff/60001/chrome/android/java/src...
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...
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 12:40:18, awdf wrote:
> probably not important, but previously this was just a Log.w not .e

Then I'd argue it should've been Log.e :) The framework gave us unexpected
information which means we're dropping something meant for the user on the floor
- that's an error.

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:33:
private String mSenderId;
On 2017/02/22 11:38:02, nyquist wrote:
> I think all these members can be final?

Yup. Done.

https://codereview.chromium.org/2690163008/diff/60001/components/gcm_driver/a...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMMessage.java:62:
final String bundleCollapseKey = "collapse_key";
On 2017/02/22 11:38:02, nyquist wrote:
> Nit: Why are these final? I don't think we usually do that in chromium, even
> though it never changes, unless it's required for callbacks, anonymous inner
> classes, etc.

Largely just copied, it doesn't really matter. Removed it.

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/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 :/

[Peter Beverloo, 2017-02-23 17:28:09]

https://codereview.chromium.org/2690163008/diff/60001/chrome/android/java/src...
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...
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 17:26:55, Peter Beverloo wrote:
> On 2017/02/23 12:40:18, awdf wrote:
> > probably not important, but previously this was just a Log.w not .e
> 
> Then I'd argue it should've been Log.e :) The framework gave us unexpected
> information which means we're dropping something meant for the user on the
floor
> - that's an error.

To be clear - good catch :) I hadn't consciously made the change.

[commit-bot: I haz the power, 2017-02-23 18:17:08]

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

[commit-bot: I haz the power, 2017-02-23 18:17:10]

Dry run: This issue passed the CQ dry run.

[nyquist, 2017-02-24 08:22:40]

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. :-/