Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2716493002:
Add a variations parameter to control WebAPK renderer launching. (Closed)
DescriptionAdd a variations parameter to control WebAPK renderer launching.
This CL adds a variations parameter for launching renderer in WebAPK
process. When enabled, WebAPK would launch Chrome's renderer process as
its own process; otherwise, WebAPK would launch Chrome's renderer
process as Chrome's process.
BUG=692627
Review-Url: https://codereview.chromium.org/2716493002
Cr-Commit-Position: refs/heads/master@{#453240}
Committed: https://chromium.googlesource.com/chromium/src/+/3dc27d31ac4b1454c0c75cec4d01f6d0b9ff2be6
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressing comments #
Total comments: 4
Patch Set 3 : Lazy init and removing functions #
Total comments: 8
Patch Set 4 : Removing redundant function and lazy init #
Total comments: 4
Patch Set 5 : Nits #Patch Set 6 : Rebase to master #
Messages
Total messages: 44 (28 generated)
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add finch flag for launching WebAPK renderer process. BUG= ========== to ========== Add finch flag for launching WebAPK renderer process. This CL adds a finch flag for launching WebAPK renderer process. With the flag enabled, WebAPK would launch Chrome's renderer process as its own process; with the flag disabled, WebAPK would launch Chrome's renderer process as Chrome's process. BUG=692627 ==========
zpeng@chromium.org changed reviewers: + dominickn@chromium.org, pkotwicz@chromium.org
Hi Dominick & Peter, PTAL. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please update the description to be something like, "Add a variations parameter to control WebAPK renderer launching". https://codereview.chromium.org/2716493002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2716493002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:33: /** Indicates whether to launch WebAPK renderer or not. */ Nit: newline above this. https://codereview.chromium.org/2716493002/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapk/chrome_webapk_host.cc (right): https://codereview.chromium.org/2716493002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/chrome_webapk_host.cc:17: const char* kWebApkRenderer = "webapk_renderer"; It's not clear what true and false mean here. Can this variable and constant be something like, "launch_renderer_in_webapk_process" / kLaunchRendererInWebApkProcess? https://codereview.chromium.org/2716493002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/chrome_webapk_host.cc:41: jboolean CanLaunchWebApkRenderer( CanLaunchRendererInWebApkProcess ?
Description was changed from ========== Add finch flag for launching WebAPK renderer process. This CL adds a finch flag for launching WebAPK renderer process. With the flag enabled, WebAPK would launch Chrome's renderer process as its own process; with the flag disabled, WebAPK would launch Chrome's renderer process as Chrome's process. BUG=692627 ========== to ========== Add a variations parameter to control WebAPK renderer launching. This CL adds a variations parameter for launching WebAPK renderer process. When enabled, WebAPK would launch Chrome's renderer process as its own process; otherwise, WebAPK would launch Chrome's renderer process as Chrome's process. BUG=692627 ==========
Description was changed from ========== Add a variations parameter to control WebAPK renderer launching. This CL adds a variations parameter for launching WebAPK renderer process. When enabled, WebAPK would launch Chrome's renderer process as its own process; otherwise, WebAPK would launch Chrome's renderer process as Chrome's process. BUG=692627 ========== to ========== Add a variations parameter to control WebAPK renderer launching. This CL adds a variations parameter for launching renderer in WebAPK process. When enabled, WebAPK would launch Chrome's renderer process as its own process; otherwise, WebAPK would launch Chrome's renderer process as Chrome's process. BUG=692627 ==========
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Dominick, PTAL! https://codereview.chromium.org/2716493002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2716493002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:33: /** Indicates whether to launch WebAPK renderer or not. */ On 2017/02/23 00:17:28, dominickn wrote: > Nit: newline above this. Done. https://codereview.chromium.org/2716493002/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapk/chrome_webapk_host.cc (right): https://codereview.chromium.org/2716493002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/chrome_webapk_host.cc:17: const char* kWebApkRenderer = "webapk_renderer"; On 2017/02/23 00:17:28, dominickn wrote: > It's not clear what true and false mean here. Can this variable and constant be > something like, "launch_renderer_in_webapk_process" / > kLaunchRendererInWebApkProcess? Done. https://codereview.chromium.org/2716493002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/chrome_webapk_host.cc:41: jboolean CanLaunchWebApkRenderer( On 2017/02/23 00:17:28, dominickn wrote: > CanLaunchRendererInWebApkProcess ? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, but it would be good to confirm whether or not native will always be loaded when the WebApkActivity constructor is called (otherwise the boolean might be set to false accidentally) https://codereview.chromium.org/2716493002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2716493002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:37: public WebApkActivity() { Is it possible for native to not be loaded when this constructor is called?
https://codereview.chromium.org/2716493002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2716493002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:171: protected void initializeChildProcessCreationParams() { You can remove this function. From git blame it looks like this function was added so that ChildProcessCreationParams#get() returns the right think in ChildProcessLauncher#warmUp() This function was added in https://codereview.chromium.org/2017963003 Specifically, I am suggesting: - Checking the variations flag in onResumeWithNative() and onPauseWithNative() - Removing the initializeChildProcessCreationParams() override because boliu@ will ignore the value of ChildProcessCreationParams#get() in ChildProcessLauncher#warmUp(). boliu@'s CL is a regression and will make it take longer to launch the renderer in the WebAPK process. However, I think that the regression from boliu@'s CL is OK
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Dominick & Peter, PTAL! https://codereview.chromium.org/2716493002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2716493002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:37: public WebApkActivity() { On 2017/02/23 01:56:02, dominickn wrote: > Is it possible for native to not be loaded when this constructor is called? Yes. Now uses lazy init. Thanks! https://codereview.chromium.org/2716493002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:171: protected void initializeChildProcessCreationParams() { On 2017/02/23 15:27:32, pkotwicz wrote: > You can remove this function. From git blame it looks like this function was > added so that ChildProcessCreationParams#get() returns the right think in > ChildProcessLauncher#warmUp() This function was added in > https://codereview.chromium.org/2017963003 > > Specifically, I am suggesting: > - Checking the variations flag in onResumeWithNative() and onPauseWithNative() > - Removing the initializeChildProcessCreationParams() override because boliu@ > will ignore the value of ChildProcessCreationParams#get() in > ChildProcessLauncher#warmUp(). boliu@'s CL is a regression and will make it take > longer to launch the renderer in the WebAPK process. However, I think that the > regression from boliu@'s CL is OK Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2716493002/diff/30004/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java (right): https://codereview.chromium.org/2716493002/diff/30004/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:114: Nit: Can you move this function to after canInstallWebApk() That way all of the install related functions are together https://codereview.chromium.org/2716493002/diff/30004/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (left): https://codereview.chromium.org/2716493002/diff/30004/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:162: protected void initializeChildProcessCreationParams() { Can you please remove the empty method call from AsyncInitializationActivity#initializeChildProcessCreationParams() and delete the invocation in AsyncInitializationActivity#setContentViewAndLoadLibrary() https://codereview.chromium.org/2716493002/diff/30004/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2716493002/diff/30004/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:161: // {@link ChildProcessLauncher} knows which application's renderer service to connect. Nits: - "and set" -> "and sets" - "to connect." -> "to connect to." https://codereview.chromium.org/2716493002/diff/30004/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:162: if (mCanLaunchRendererInWebApkProcess == null) { Can you do the initialization in WebApkActivity#finishNativeInitialization() finishNativeInitialization() is called before onResumeWithNative() WebApkActivity#finishNativeInitialization() would override AsyncInitializationActivity#finishNativeInitialization() NativeInitializationController#onNativeInitializationComplete() is responsible for calling onResumeWithNative()
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
zpeng@chromium.org changed reviewers: + dfalcantara@chromium.org
Thanks Dominick & Peter, PTAL! Hi Dan, PTAL. Thanks! https://codereview.chromium.org/2716493002/diff/30004/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java (right): https://codereview.chromium.org/2716493002/diff/30004/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:114: On 2017/02/23 21:11:57, pkotwicz wrote: > Nit: Can you move this function to after canInstallWebApk() > > That way all of the install related functions are together Done. https://codereview.chromium.org/2716493002/diff/30004/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (left): https://codereview.chromium.org/2716493002/diff/30004/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:162: protected void initializeChildProcessCreationParams() { On 2017/02/23 21:11:57, pkotwicz wrote: > Can you please remove the empty method call from > AsyncInitializationActivity#initializeChildProcessCreationParams() and delete > the invocation in AsyncInitializationActivity#setContentViewAndLoadLibrary() Done. Thanks! https://codereview.chromium.org/2716493002/diff/30004/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2716493002/diff/30004/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:161: // {@link ChildProcessLauncher} knows which application's renderer service to connect. On 2017/02/23 21:11:57, pkotwicz wrote: > Nits: > - "and set" -> "and sets" > - "to connect." -> "to connect to." Done. https://codereview.chromium.org/2716493002/diff/30004/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:162: if (mCanLaunchRendererInWebApkProcess == null) { On 2017/02/23 21:11:57, pkotwicz wrote: > Can you do the initialization in WebApkActivity#finishNativeInitialization() > > finishNativeInitialization() is called before onResumeWithNative() > > WebApkActivity#finishNativeInitialization() would override > AsyncInitializationActivity#finishNativeInitialization() > NativeInitializationController#onNativeInitializationComplete() is responsible > for calling onResumeWithNative() Done. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
still lgtm. Thanks for following up on the native initialisation
lgtm
LGTM with nits https://codereview.chromium.org/2716493002/diff/50001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java (right): https://codereview.chromium.org/2716493002/diff/50001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:127: */ Nit: You can put this comment on one line like this /* Comment */ https://codereview.chromium.org/2716493002/diff/50001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2716493002/diff/50001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:35: private Boolean mCanLaunchRendererInWebApkProcess; This can now be a lowercase b boolean
Thanks Dan, Dominick, and Peter! Going to attempt CQ https://codereview.chromium.org/2716493002/diff/50001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java (right): https://codereview.chromium.org/2716493002/diff/50001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:127: */ On 2017/02/24 16:45:07, pkotwicz wrote: > Nit: You can put this comment on one line like this > > /* Comment */ Done. https://codereview.chromium.org/2716493002/diff/50001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2716493002/diff/50001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:35: private Boolean mCanLaunchRendererInWebApkProcess; On 2017/02/24 16:45:07, pkotwicz wrote: > This can now be a lowercase b boolean Done. Thanks!
The CQ bit was checked by zpeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, pkotwicz@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2716493002/#ps70001 (title: "Nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by zpeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, pkotwicz@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2716493002/#ps90001 (title: "Rebase to master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 90001, "attempt_start_ts": 1488213504256380,
"parent_rev": "4b014c7ebbe5ed0d2abf313d04699caf6c6ed18d", "commit_rev":
"3dc27d31ac4b1454c0c75cec4d01f6d0b9ff2be6"}
Message was sent while issue was closed.
Description was changed from ========== Add a variations parameter to control WebAPK renderer launching. This CL adds a variations parameter for launching renderer in WebAPK process. When enabled, WebAPK would launch Chrome's renderer process as its own process; otherwise, WebAPK would launch Chrome's renderer process as Chrome's process. BUG=692627 ========== to ========== Add a variations parameter to control WebAPK renderer launching. This CL adds a variations parameter for launching renderer in WebAPK process. When enabled, WebAPK would launch Chrome's renderer process as its own process; otherwise, WebAPK would launch Chrome's renderer process as Chrome's process. BUG=692627 Review-Url: https://codereview.chromium.org/2716493002 Cr-Commit-Position: refs/heads/master@{#453240} Committed: https://chromium.googlesource.com/chromium/src/+/3dc27d31ac4b1454c0c75cec4d01... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:90001) as https://chromium.googlesource.com/chromium/src/+/3dc27d31ac4b1454c0c75cec4d01... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
