Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2043703003:
🍆 Move GCM browser initialization to chrome layer. (Closed)
|
Created:
4 years, 6 months ago by Peter Wen Modified:
4 years, 6 months ago Reviewers:
Peter Beverloo CC:
chromium-reviews, mvanouwerkerk+watch_chromium.org, johnme+watch_chromium.org, harkness+watch_chromium.org, zea+watch_chromium.org, Michael van Ouwerkerk Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove GCM browser initialization to chrome layer.
The only java-side user of GCMDriver is ChromeGcmListenerService.java
so rather than use BrowserStartupController and doing all the
necessary initialization (like PathUtils) separately, use the unified
ChromeBrowserInitializer path instead.
Tested codepath according to: http://crbug.com/585421#c7
BUG=583086
Committed: https://crrev.com/287cd4fcdeff6f62024f67d6959f87264fa820ce
Cr-Commit-Position: refs/heads/master@{#398312}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fix per review. #Patch Set 3 : Move FB suppressant. #
Messages
Total messages: 14 (6 generated)
Description was changed from ========== Move GCM browser initialization to chrome layer. The only java-side user of GCMDriver is ChromeGcmListenerService.java so rather than use BrowserStartupController and doing all the necessary initialization (like PathUtils) separately, use the unified ChromeBrowserInitializer path instead. BUG=583086 ========== to ========== Move GCM browser initialization to chrome layer. The only java-side user of GCMDriver is ChromeGcmListenerService.java so rather than use BrowserStartupController and doing all the necessary initialization (like PathUtils) separately, use the unified ChromeBrowserInitializer path instead. Tested codepath according to: http://crbug.com/585421#c7 BUG=583086 ==========
wnwen@chromium.org changed reviewers: + peter@chromium.org
Hi Peter, could you take a look at this cleanup CL? Thanks.
Code SGTM, one question: Would you perhaps have a very brief overview of the differences in behaviour we get from switching from ChromeBrowserInitializer to BrowserStartupController? It initializes a whole bunch of other things aside of the BrowserStartupController, what is the impact of this on memory usage? GCM messages are received at (effectively) random moments when Chrome may not be running, and another application (e.g. a game or social networking app) may be in the foreground. Higher memory usage might increase the failure rate. https://codereview.chromium.org/2043703003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java (right): https://codereview.chromium.org/2043703003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java:71: System.exit(-1); You need to move the findbugs rule for using System.exit() from GCMDriver.java to this file. https://codereview.chromium.org/2043703003/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java (right): https://codereview.chromium.org/2043703003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java:182: GCMDriver.onMessageReceived(appId, senderId, extras); Please move this call to within the try{} block. (Even if fail() bails out, it doesn't read as if it adheres to the contract onMessageReceived now makes with its callers.) https://codereview.chromium.org/2043703003/diff/1/components/gcm_driver/andro... File components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java (right): https://codereview.chromium.org/2043703003/diff/1/components/gcm_driver/andro... components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java:122: Log.e(TAG, "Failed to instantiate GCMDriver."); This should probably be an exception? If we now, for whatever reason, fail to initialize the native GCMDriverAndroid instance, we'll get this error and then NPE on line 144. Could we throw here with a clear exception instead? (Previously we'd never execute the task at all, see line 181-182.)
ChromeBrowserInitializer should now become the only way to start up the Chrome browser process in Android. The code in preInflationStartup is all very lightweight. It does, however, kick off some pre-fetching which would be very beneficial to startup time should the user click on the notification. The code in startChromeBrowserProcessesSync is mostly necessary checks anyways, they just used to be assumptions in GCMDriver but are now explicitly checked and started. So yes, there may be slight memory implications as more resource prefetching is kicked off, but they should not be significant, especially since we expect that notifications likely trigger full browser startup anyways. Here is the original CL for migrating to ChromeBrowserInitializer: https://codereview.chromium.org/1521013002 You may remember this CL partially migrating callers that need PathUtils to CBI (http://crrev.com/1745623002). This bug is a follow-up to that code to fully ensure that either we're a different application which needs PathUtils, or we're Chrome and use CBI to ensure we have everything necessary. https://codereview.chromium.org/2043703003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java (right): https://codereview.chromium.org/2043703003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java:71: System.exit(-1); On 2016/06/07 10:31:46, Peter Beverloo - OOO June 8-16 wrote: > You need to move the findbugs rule for using System.exit() from GCMDriver.java > to this file. Done. Strangely there was no suppression of findbugs in GCMDriver.java, is there another way to suppress other than the annotation? https://codereview.chromium.org/2043703003/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java (right): https://codereview.chromium.org/2043703003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java:182: GCMDriver.onMessageReceived(appId, senderId, extras); On 2016/06/07 10:31:46, Peter Beverloo - OOO June 8-16 wrote: > Please move this call to within the try{} block. (Even if fail() bails out, it > doesn't read as if it adheres to the contract onMessageReceived now makes with > its callers.) Done. https://codereview.chromium.org/2043703003/diff/1/components/gcm_driver/andro... File components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java (right): https://codereview.chromium.org/2043703003/diff/1/components/gcm_driver/andro... components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java:122: Log.e(TAG, "Failed to instantiate GCMDriver."); On 2016/06/07 10:31:46, Peter Beverloo - OOO June 8-16 wrote: > This should probably be an exception? If we now, for whatever reason, fail to > initialize the native GCMDriverAndroid instance, we'll get this error and then > NPE on line 144. Could we throw here with a clear exception instead? > > (Previously we'd never execute the task at all, see line 181-182.) Done. If this turns out to be a problem, we can always revert to the old behaviour of silently returning.
lgtm On 2016/06/07 12:26:52, Peter Wen wrote: > ChromeBrowserInitializer should now become the only way to start up the Chrome > browser process in Android. The code in preInflationStartup is all very > lightweight. It does, however, kick off some pre-fetching which would be very > beneficial to startup time should the user click on the notification. Yes, but in that scenario the screen is on already. Push messages (the GCMDriver you're touching here) often occur when the screen is off as well. Nonetheless, thank you for the explanation. It sounds like this is the direction we want to go in, but I'll note this as an attention point for the M53 system health plan. > The code in startChromeBrowserProcessesSync is mostly necessary checks anyways, > they just used to be assumptions in GCMDriver but are now explicitly checked and > started. > > So yes, there may be slight memory implications as more resource prefetching is > kicked off, but they should not be significant, especially since we expect that > notifications likely trigger full browser startup anyways. > > Here is the original CL for migrating to ChromeBrowserInitializer: > https://codereview.chromium.org/1521013002 > > You may remember this CL partially migrating callers that need PathUtils to CBI > (http://crrev.com/1745623002). This bug is a follow-up to that code to fully > ensure that either we're a different application which needs PathUtils, or we're > Chrome and use CBI to ensure we have everything necessary. > > https://codereview.chromium.org/2043703003/diff/1/chrome/android/java/src/org... > File > chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java > (right): > > https://codereview.chromium.org/2043703003/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java:71: > System.exit(-1); > On 2016/06/07 10:31:46, Peter Beverloo - OOO June 8-16 wrote: > > You need to move the findbugs rule for using System.exit() from GCMDriver.java > > to this file. > > Done. > > Strangely there was no suppression of findbugs in GCMDriver.java, is there > another way to suppress other than the annotation? That surprises me, I do recall there being a line for this. (findbugs_known_issues.txt?) Do we only generate diffs these days? I don't know how that works :-(. Maybe +mvanouwerkerk has an idea. > https://codereview.chromium.org/2043703003/diff/1/chrome/android/javatests/sr... > File > chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java > (right): > > https://codereview.chromium.org/2043703003/diff/1/chrome/android/javatests/sr... > chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java:182: > GCMDriver.onMessageReceived(appId, senderId, extras); > On 2016/06/07 10:31:46, Peter Beverloo - OOO June 8-16 wrote: > > Please move this call to within the try{} block. (Even if fail() bails out, it > > doesn't read as if it adheres to the contract onMessageReceived now makes with > > its callers.) > > Done. > > https://codereview.chromium.org/2043703003/diff/1/components/gcm_driver/andro... > File > components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java > (right): > > https://codereview.chromium.org/2043703003/diff/1/components/gcm_driver/andro... > components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java:122: > Log.e(TAG, "Failed to instantiate GCMDriver."); > On 2016/06/07 10:31:46, Peter Beverloo - OOO June 8-16 wrote: > > This should probably be an exception? If we now, for whatever reason, fail to > > initialize the native GCMDriverAndroid instance, we'll get this error and then > > NPE on line 144. Could we throw here with a clear exception instead? > > > > (Previously we'd never execute the task at all, see line 181-182.) > > Done. > > If this turns out to be a problem, we can always revert to the old behaviour of > silently returning.
cc+mvanouwerkerk@ for why GCMDriver.java did not need/have @SuppressFBWarnings for System.exit(-1) before.
The CQ bit was checked by wnwen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org Link to the patchset: https://codereview.chromium.org/2043703003/#ps40001 (title: "Move FB suppressant.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2043703003/40001
Message was sent while issue was closed.
Description was changed from ========== Move GCM browser initialization to chrome layer. The only java-side user of GCMDriver is ChromeGcmListenerService.java so rather than use BrowserStartupController and doing all the necessary initialization (like PathUtils) separately, use the unified ChromeBrowserInitializer path instead. Tested codepath according to: http://crbug.com/585421#c7 BUG=583086 ========== to ========== Move GCM browser initialization to chrome layer. The only java-side user of GCMDriver is ChromeGcmListenerService.java so rather than use BrowserStartupController and doing all the necessary initialization (like PathUtils) separately, use the unified ChromeBrowserInitializer path instead. Tested codepath according to: http://crbug.com/585421#c7 BUG=583086 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Move GCM browser initialization to chrome layer. The only java-side user of GCMDriver is ChromeGcmListenerService.java so rather than use BrowserStartupController and doing all the necessary initialization (like PathUtils) separately, use the unified ChromeBrowserInitializer path instead. Tested codepath according to: http://crbug.com/585421#c7 BUG=583086 ========== to ========== Move GCM browser initialization to chrome layer. The only java-side user of GCMDriver is ChromeGcmListenerService.java so rather than use BrowserStartupController and doing all the necessary initialization (like PathUtils) separately, use the unified ChromeBrowserInitializer path instead. Tested codepath according to: http://crbug.com/585421#c7 BUG=583086 Committed: https://crrev.com/287cd4fcdeff6f62024f67d6959f87264fa820ce Cr-Commit-Position: refs/heads/master@{#398312} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/287cd4fcdeff6f62024f67d6959f87264fa820ce Cr-Commit-Position: refs/heads/master@{#398312} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
