Keep a static reference to Java GCMDriver instance

Keep a static reference to Java GCMDriver instance

This is so that intent handlers etc will be able to access the GCMDriver
instance (if any), or if it's null they'll know that they need to launch
the native browser process and tell it to create a C++ GCMDriverAndroid.

BUG=350384
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276187

[fgorski, 2014-06-05 05:33:56]

What are the benefits of that approach? (I want to understand the change
better.)

I currently don't know of/see any major issues with the change, however it made
me wonder, if we can count on the environment to always work with that class on
the same thread. Or to flip it around, when accessed from another thread, can we
always count on that static instance to be there? I am considering that because
we can expect that class to be invoked either from native, or by the Android
through Broadcast Receiver (or whatever other mechanism for listening to GCM
messages)

[johnme, 2014-06-06 19:20:31]

On 2014/06/05 05:33:56, fgorski wrote:
> What are the benefits of that approach? (I want to understand the change
> better.)
> 
> I currently don't know of/see any major issues with the change, however it
made
> me wonder, if we can count on the environment to always work with that class
on
> the same thread. Or to flip it around, when accessed from another thread, can
we
> always count on that static instance to be there? I am considering that
because
> we can expect that class to be invoked either from native, or by the Android
> through Broadcast Receiver (or whatever other mechanism for listening to GCM
> messages)

Hi Filip, this probably makes more sense in the context of the next patch, where
you see GCMListener objects created by the framework calling these static
methods (since they don't have a reference to GCMDriver):
https://codereview.chromium.org/314293006/

And I use ThreadUtils.runOnUiThread in that patch for the intent receivers,
since I've confirmed that they do (at least sometimes) get executed on threads
other than the UI thread.

[fgorski, 2014-06-09 17:31:03]

lgtm.

Thanks for explanation. I checked out the other patch and I think I understand
it a little better. When are you going to be working on launchNative part?

https://codereview.chromium.org/316963003/diff/20001/components/gcm_driver/an...
File
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java
(right):

https://codereview.chromium.org/316963003/diff/20001/components/gcm_driver/an...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java:73:
if (sInstance != null)
I assume that you will also add the launchNativeThen... to the registration
calls. Is that the case?

https://codereview.chromium.org/316963003/diff/20001/components/gcm_driver/an...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java:128:
if (sInstance != null) {
When you implement the native startup, is there still chance that some of the
messages get dropped?

[johnme, 2014-06-09 20:19:58]

Thanks for the review!

> When are you going to be working on launchNative part?

I started a patch locally, which uses
BrowserStartupController.get(mApplication).startBrowserProcessesSync to start
the native browser process. But before I spend more time on this I want to land
crrev.com/316963003, crrev.com/314293006 and crrev.com/317823007, then hook up a
GCMAppHandler for the Push API. Hopefully that'll be in ~two days :-)

https://codereview.chromium.org/316963003/diff/20001/components/gcm_driver/an...
File
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java
(right):

https://codereview.chromium.org/316963003/diff/20001/components/gcm_driver/an...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java:73:
if (sInstance != null)
On 2014/06/09 17:31:03, fgorski wrote:
> I assume that you will also add the launchNativeThen... to the registration
> calls. Is that the case?

I wasn't planning to, as if registration or unregistration take longer than the
lifetime of the page, it's impossible to resolve the Promises used in
https://w3c.github.io/push-api/.

However now you draw attention to it, this probably needs to update a cached
map<app_id, registration_id> somewhere (possibly Java-side, to avoid waking up
native). I'd added a TODO.

https://codereview.chromium.org/316963003/diff/20001/components/gcm_driver/an...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java:128:
if (sInstance != null) {
On 2014/06/09 17:31:03, fgorski wrote:
> When you implement the native startup, is there still chance that some of the
> messages get dropped?

Yes, especially as it's running in the background, Chrome could get killed. I've
added a TODO to store the messages first and redeliver later if delivery fails.

[johnme, 2014-06-09 20:20:59]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2014-06-09 20:29:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/johnme@chromium.org/316963003/40001

[commit-bot: I haz the power, 2014-06-10 08:37:40]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)

[johnme, 2014-06-10 10:15:01]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2014-06-10 10:17:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/johnme@chromium.org/316963003/60001

[commit-bot: I haz the power, 2014-06-10 14:04:44]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)

[commit-bot: I haz the power, 2014-06-10 14:35:11]

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

[commit-bot: I haz the power, 2014-06-10 14:35:12]

Try jobs failed on following builders:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)

[johnme, 2014-06-10 15:36:48]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2014-06-10 15:38:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/johnme@chromium.org/316963003/80001

[commit-bot: I haz the power, 2014-06-10 22:51:03]

Change committed as 276187