Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
19957002:
Run the later parts of startup as UI thread tasks (Closed)
|
Created:
7 years, 5 months ago by aberent Modified:
7 years, 4 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, android-webview-reviews_chromium.org, jochen+watch_chromium.org, Andrew Hayden (chromium.org), Maria, bulach, stuartmorgan, leng Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionRun the later parts of startup as UI thread tasks
This CL splits the later parts of startup, from thread creation onwards,
into multiple UI thread tasks. Depending on the StartupTaskRunner passed
to CreateThreads the tasks are all run immediately, or are queued one at
a time on the UI thread. This, on platforms where the UI is
already running, allows the UI to remain interactive while Chrome is
initialized.
BUG=231856
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215042
Patch Set 1 #
Total comments: 2
Patch Set 2 : Run the later parts of startup as UI thread tasks - minor comment fixes #
Total comments: 32
Patch Set 3 : Run the later parts of startup as UI thread tasks - patch for Joth's comments #
Total comments: 6
Patch Set 4 : Run the later parts of startup as UI thread tasks - patch for Yaron's comments #
Total comments: 16
Patch Set 5 : Run the later parts of startup as UI thread tasks - rethought configuration #Patch Set 6 : Run the later parts of startup as UI thread tasks -test fixes #Patch Set 7 : Run the later parts of startup as UI thread tasks - fix component build #
Total comments: 8
Patch Set 8 : Run the later parts of startup as UI thread tasks - Yaron's comments + Mac build fix #Patch Set 9 : Run the later parts of startup as UI thread tasks - one more Mac fix #
Total comments: 12
Patch Set 10 : Run the later parts of startup as UI thread tasks - respond to Yaron's and jam's comments #
Total comments: 6
Patch Set 11 : Run the later parts of startup as UI thread tasks - fix jam@'s comments #
Total comments: 10
Patch Set 12 : Run the later parts of startup as UI thread tasks - Jam's nits #Messages
Total messages: 33 (0 generated)
https://codereview.chromium.org/19957002/diff/1/content/browser/android/brows... File content/browser/android/browser_jni_registrar.cc (right): https://codereview.chromium.org/19957002/diff/1/content/browser/android/brows... content/browser/android/browser_jni_registrar.cc:66: content::AndroidStartupObserver::RegisterStartupObserver}, The only real change in this file. https://codereview.chromium.org/19957002/diff/1/content/browser/android/brows... content/browser/android/browser_jni_registrar.cc:71: {"WebViewStatics", content::RegisterWebViewStatics}, }; Reformatted by git cl format. Not sure why it is using an indent of 4 here. https://codereview.chromium.org/19957002/diff/2001/chrome/browser/chrome_brow... File chrome/browser/chrome_browser_main.h (right): https://codereview.chromium.org/19957002/diff/2001/chrome/browser/chrome_brow... chrome/browser/chrome_browser_main.h:136: const content::MainFunctionParams parameters_; This change is needed because the parameters are now used after the caller of the constructor (which owns the parameters passed to the constructor) has exited.
https://codereview.chromium.org/19957002/diff/2001/content/browser/android/br... File content/browser/android/browser_jni_registrar.cc (right): https://codereview.chromium.org/19957002/diff/2001/content/browser/android/br... content/browser/android/browser_jni_registrar.cc:66: content::AndroidStartupObserver::RegisterStartupObserver}, This is the only real change in this block. The rest are formatting changes caused by running git cl format.
PS2 - I just read aw/ and content/public/ Just realized though you didn't actually request review yet :) hopefully this is still useful https://codereview.chromium.org/19957002/diff/2001/content/public/android/jav... File content/public/android/java/src/org/chromium/content/app/ChildProcessService.java (right): https://codereview.chromium.org/19957002/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:147: contentMain.start(true, new StartupObserver()); initApplicationContext & start are still static (I think?) so it will give a warning (checkbugs?) to call these methods on object rather than class. https://codereview.chromium.org/19957002/diff/2001/content/public/android/jav... File content/public/android/java/src/org/chromium/content/app/ContentMain.java (right): https://codereview.chromium.org/19957002/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/app/ContentMain.java:23: public class ContentMain { this class only has static methods. Could you make it abstract (and private ctor if needed) https://codereview.chromium.org/19957002/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/app/ContentMain.java:27: static public void initApplicationContext(Context context) { please revert these 4 keyword transpose edits - it's against the grain $ git grep "public static" | wc -l 948 $ git grep "static public" | wc -l 9 https://codereview.chromium.org/19957002/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/app/ContentMain.java:32: * Start the ContentMainRunner in native side. document the new params https://codereview.chromium.org/19957002/diff/2001/content/public/android/jav... File content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java (right): https://codereview.chromium.org/19957002/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java:49: * UI tasks nit: i think convention is to indent this (to align with 'true' in this case_ https://codereview.chromium.org/19957002/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java:82: if(observer == null) { nit: you could make the Aw caller just pass null in this case then. https://codereview.chromium.org/19957002/diff/2001/content/public/android/jav... File content/public/android/java/src/org/chromium/content/browser/StartupObserver.java (right): https://codereview.chromium.org/19957002/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/StartupObserver.java:5: nit: spurious \n https://codereview.chromium.org/19957002/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/StartupObserver.java:10: public class StartupObserver { class comment - mostly mention which class you use this with https://codereview.chromium.org/19957002/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/StartupObserver.java:13: public void allTasksRun() {}; nit: run sounds like a command here rather than past participle. maybe:- didRunAllStartupTasks() allSartupTasksRan() https://codereview.chromium.org/19957002/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/StartupObserver.java:14: } (vaguely wonder if we need a class for this, or could just use Runnable ) https://codereview.chromium.org/19957002/diff/2001/content/public/browser/bro... File content/public/browser/browser_main_runner.h (right): https://codereview.chromium.org/19957002/diff/2001/content/public/browser/bro... content/public/browser/browser_main_runner.h:11: #include "content/public/common/startup_task_runner.h" you could just fwd declare StartupTaskRunner here https://codereview.chromium.org/19957002/diff/2001/content/public/common/star... File content/public/common/startup_task_runner.h (right): https://codereview.chromium.org/19957002/diff/2001/content/public/common/star... content/public/common/startup_task_runner.h:16: // This class runs startup tasks.The tasks are either run immediately inline, nit: space after stop. https://codereview.chromium.org/19957002/diff/2001/content/public/common/star... content/public/common/startup_task_runner.h:19: // keep the UI responsive during startup. nit: I think comment block goes inside namespace immediately before the class decl. https://codereview.chromium.org/19957002/diff/2001/content/public/common/star... content/public/common/startup_task_runner.h:23: class StartupTaskRunner : public base::RefCounted<StartupTaskRunner> { remind me: why not using SequencedTaskRunner ? could add a comment here why. https://codereview.chromium.org/19957002/diff/2001/content/public/common/star... content/public/common/startup_task_runner.h:29: virtual void AllTasksRun() = 0; same comment as the java observer
Joth, many thanks for the early review, I was about to start working out who should review when your comments arrived. https://codereview.chromium.org/19957002/diff/2001/content/public/android/jav... File content/public/android/java/src/org/chromium/content/app/ChildProcessService.java (right): https://codereview.chromium.org/19957002/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:147: contentMain.start(true, new StartupObserver()); On 2013/07/22 16:28:15, joth wrote: > initApplicationContext & start are still static (I think?) so it will give a > warning (checkbugs?) to call these methods on object rather than class. Done. I should have reverted these changes; at one point in developing this CL I thought I needed a real ContentMain object, but I don't. Will fix on next update. https://codereview.chromium.org/19957002/diff/2001/content/public/android/jav... File content/public/android/java/src/org/chromium/content/app/ContentMain.java (right): https://codereview.chromium.org/19957002/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/app/ContentMain.java:23: public class ContentMain { On 2013/07/22 16:28:15, joth wrote: > this class only has static methods. Could you make it abstract (and private ctor > if needed) We could make it abstract, but other classes with only static methods in Chrome aren't abstract and don't have private constructors. See, for example, LibraryLoader or AndroidBrowserProcess. https://codereview.chromium.org/19957002/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/app/ContentMain.java:27: static public void initApplicationContext(Context context) { On 2013/07/22 16:28:15, joth wrote: > please revert these 4 keyword transpose edits - it's against the grain > > $ git grep "public static" | wc -l > 948 > > $ git grep "static public" | wc -l > 9 Done. Accidental transposition. https://codereview.chromium.org/19957002/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/app/ContentMain.java:32: * Start the ContentMainRunner in native side. On 2013/07/22 16:28:15, joth wrote: > document the new params Done. https://codereview.chromium.org/19957002/diff/2001/content/public/android/jav... File content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java (right): https://codereview.chromium.org/19957002/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java:49: * UI tasks On 2013/07/22 16:28:15, joth wrote: > nit: i think convention is to indent this (to align with 'true' in this case_ Done. https://codereview.chromium.org/19957002/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java:82: if(observer == null) { On 2013/07/22 16:28:15, joth wrote: > nit: you could make the Aw caller just pass null in this case then. Done. I was worried about whether null checks would work correctly across the JNI interface, but I am now clear that they will, so have moved handling of null into the C++ side. https://codereview.chromium.org/19957002/diff/2001/content/public/android/jav... File content/public/android/java/src/org/chromium/content/browser/StartupObserver.java (right): https://codereview.chromium.org/19957002/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/StartupObserver.java:5: On 2013/07/22 16:28:15, joth wrote: > nit: spurious \n Done. https://codereview.chromium.org/19957002/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/StartupObserver.java:10: public class StartupObserver { On 2013/07/22 16:28:15, joth wrote: > class comment - mostly mention which class you use this with Done. https://codereview.chromium.org/19957002/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/StartupObserver.java:13: public void allTasksRun() {}; On 2013/07/22 16:28:15, joth wrote: > nit: run sounds like a command here rather than past participle. maybe:- > > didRunAllStartupTasks() > > allSartupTasksRan() Done. https://codereview.chromium.org/19957002/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/StartupObserver.java:14: } On 2013/07/22 16:28:15, joth wrote: > (vaguely wonder if we need a class for this, or could just use Runnable ) I don't think one can call a Runnable's run method across the JNI. In addition since StartupObserver objects are passed through a number of levels having an explicit class seems clearer for the function arguments. https://codereview.chromium.org/19957002/diff/2001/content/public/common/star... File content/public/common/startup_task_runner.h (right): https://codereview.chromium.org/19957002/diff/2001/content/public/common/star... content/public/common/startup_task_runner.h:16: // This class runs startup tasks.The tasks are either run immediately inline, On 2013/07/22 16:28:15, joth wrote: > nit: space after stop. Done. https://codereview.chromium.org/19957002/diff/2001/content/public/common/star... content/public/common/startup_task_runner.h:19: // keep the UI responsive during startup. On 2013/07/22 16:28:15, joth wrote: > nit: I think comment block goes inside namespace immediately before the class > decl. Done. https://codereview.chromium.org/19957002/diff/2001/content/public/common/star... content/public/common/startup_task_runner.h:23: class StartupTaskRunner : public base::RefCounted<StartupTaskRunner> { On 2013/07/22 16:28:15, joth wrote: > remind me: why not using SequencedTaskRunner ? > could add a comment here why. I have extended the class comment with a note explaining the difference. This could (with some changes) possibly have been derived from SequencedTaskRunner, or more precisely SingleThreadedTaskRunner, but has extra requirements that are not guaranteed by those, and are not provided by the existing implementations. It also used in a unique way, so I don't think there would have been any advantage in doing so from the caller's side. https://codereview.chromium.org/19957002/diff/2001/content/public/common/star... content/public/common/startup_task_runner.h:29: virtual void AllTasksRun() = 0; On 2013/07/22 16:28:15, joth wrote: > same comment as the java observer Done.
The last message was very incomplete. It should have been something more like Reviewers: joth@chromium.org, jam@chromium.org, yfriedman@chromium.org. https://codereview.chromium.org/19957002/diff/1/content/browser/android/brows... File content/browser/android/browser_jni_registrar.cc (right): https://codereview.chromium.org/19957002/diff/1/content/browser/android/brows... content/browser/android/browser_jni_registrar.cc:66: content::AndroidStartupObserver::RegisterStartupObserver}, The only real change in this file. https://codereview.chromium.org/19957002/diff/1/content/browser/android/brows... content/browser/android/browser_jni_registrar.cc:71: {"WebViewStatics", content::RegisterWebViewStatics}, }; Reformatted by git cl format. Not sure why it is using an indent of 4 here. https://codereview.chromium.org/19957002/diff/2001/chrome/browser/chrome_brow... File chrome/browser/chrome_browser_main.h (right): https://codereview.chromium.org/19957002/diff/2001/chrome/browser/chrome_brow... chrome/browser/chrome_browser_main.h:136: const content::MainFunctionParams parameters_; This change is needed because the parameters are now used after the caller of the constructor (which owns the parameters passed to the constructor) has exited. Description: Run the later parts of startup as UI thread tasks This CL splits the later parts of startup, from thread creation onwards, into multiple UI thread tasks. Depending on the StartupTaskRunner passed to CreateThreads the tasks are all run immediately, or are queued one at a time on the UI thread. This, on platforms where the UI is already running, allows the UI to remain interactive while Chrome is initialized. BUG=231856 Please review this at https://codereview.chromium.org/19957002/ SVN Base: https://chromium.googlesource.com/chromium/src.git@master Affected files: M android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java M android_webview/lib/main/aw_main_delegate.cc M chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java M chrome/app/android/chrome_main_delegate_android.h M chrome/app/android/chrome_main_delegate_android.cc M chrome/browser/chrome_browser_main.h M content/app/android/content_main.cc A content/browser/android/android_startup_observer.h A content/browser/android/android_startup_observer.cc M content/browser/android/browser_jni_registrar.cc M content/browser/browser_main.cc M content/browser/browser_main_loop.h M content/browser/browser_main_loop.cc M content/browser/browser_main_runner.cc A content/common/startup_task_runner.cc A content/common/startup_task_runner_unittest.cc M content/content_browser.gypi M content/content_common.gypi M content/content_jni.gypi M content/content_tests.gypi M content/public/android/java/src/org/chromium/content/app/ChildProcessService.java M content/public/android/java/src/org/chromium/content/app/ContentMain.java M content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java A content/public/android/java/src/org/chromium/content/browser/StartupObserver.java M content/public/app/content_main_delegate.h M content/public/app/content_main_delegate.cc M content/public/browser/browser_main_runner.h A content/public/common/startup_task_runner.h M content/public/test/browser_test_base.cc M content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellActivity.java M content/shell/shell_browser_main.cc On Mon, Jul 22, 2013 at 8:48 PM, <aberent@chromium.org> wrote: > https://codereview.chromium.**org/19957002/<https://codereview.chromium.org/1... >
this is going to take a bit of time to review. in the meantime, can you share updated data about how long these stages are taking, now that the webkit thread is gone? This is adding a bit of complexity to an already complicated part of chrome (startup), so I'd like to see compelling data to know that we really want this. Thanks
https://codereview.chromium.org/19957002/diff/17001/chrome/app/android/chrome... File chrome/app/android/chrome_main_delegate_android.cc (right): https://codereview.chromium.org/19957002/diff/17001/chrome/app/android/chrome... chrome/app/android/chrome_main_delegate_android.cc:38: // For Chrome on android we want to run the startup tasks incrementally, so Any reason not to do this on testshell as well and avoid the unnecessary conditional? Or put another way: why configure this from internal repo? If you want to make this conditional, it could just be a flag. https://codereview.chromium.org/19957002/diff/17001/content/browser/android/a... File content/browser/android/android_startup_observer.h (right): https://codereview.chromium.org/19957002/diff/17001/content/browser/android/a... content/browser/android/android_startup_observer.h:24: virtual void OVERRIDE AllStartupTasksRan(); OVERRIDE is typically placed at the end https://codereview.chromium.org/19957002/diff/17001/content/browser/browser_m... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/19957002/diff/17001/content/browser/browser_m... content/browser/browser_main_loop.cc:605: task_runner->AddTask(create_thread); To jam's point, the two slow thread inits that I knew of were: IO thread and webkit deprecated. IO is now async and webkit deprecated is gone, so is this part still worth it? I know there are other parts of startup where this might apply but it maybe unnecessary at this granularity.
On 2013/07/22 23:34:15, jam wrote: > this is going to take a bit of time to review. in the meantime, can you share > updated data about how long these stages are taking, now that the webkit thread > is gone? This is adding a bit of complexity to an already complicated part of > chrome (startup), so I'd like to see compelling data to know that we really want > this. Thanks On my latest traces (with caches empty) on master the call to Looper.DispatchMessage which calls all this takes 932ms on Galaxy Nexus. Of this 702ms is in create threads. With my change the slowest call to Looper.DispatchMessage takes 340ms. This considerably improves responsiveness to input (although we are still some way from the ideal of < 100ms). With my changes the startup tasks are: * Initialization up to start of CreateThreads (including creating the startup tasks) - 245ms * Task to run ChromeBrowserMainParts::PreCreateThreads - 340ms * Tasks to to create threads - 6ms * Task to run BrowserMainLoop::BrowserThreadsStarted - 77ms * Task to run ChromeBrowserMainParts::PreMainMessageLoopRun - 246ms To go further I think there may be ways of breaking up PreCreateThreads and possibly PreMainMessageLoopRun, but have not yet investigated in any detail. I can't share the full traces to the whole of chromium.org, since they contain confidential information about Chrome for Android, but will share them within Google.
On 2013/07/23 11:57:46, aberent wrote: > On 2013/07/22 23:34:15, jam wrote: > > this is going to take a bit of time to review. in the meantime, can you share > > updated data about how long these stages are taking, now that the webkit > thread > > is gone? This is adding a bit of complexity to an already complicated part of > > chrome (startup), so I'd like to see compelling data to know that we really > want > > this. Thanks > > On my latest traces (with caches empty) on master the call to > Looper.DispatchMessage which calls all this takes 932ms on Galaxy Nexus. Of this > 702ms is in create threads. With my change the slowest call to > Looper.DispatchMessage takes 340ms. This considerably improves responsiveness to > input (although we are still some way from the ideal of < 100ms). With my > changes the startup tasks are: > > * Initialization up to start of CreateThreads (including creating the startup > tasks) - 245ms > > * Task to run ChromeBrowserMainParts::PreCreateThreads - 340ms > > * Tasks to to create threads - 6ms > > * Task to run BrowserMainLoop::BrowserThreadsStarted - 77ms > > * Task to run ChromeBrowserMainParts::PreMainMessageLoopRun - 246ms > > To go further I think there may be ways of breaking up PreCreateThreads and > possibly PreMainMessageLoopRun, but have not yet investigated in any detail. > > I can't share the full traces to the whole of http://chromium.org, since they contain > confidential information about Chrome for Android, but will share them within > Google. Those times are actually on Nexus S, not Galaxy Nexus.
https://codereview.chromium.org/19957002/diff/17001/chrome/app/android/chrome... File chrome/app/android/chrome_main_delegate_android.cc (right): https://codereview.chromium.org/19957002/diff/17001/chrome/app/android/chrome... chrome/app/android/chrome_main_delegate_android.cc:38: // For Chrome on android we want to run the startup tasks incrementally, so On 2013/07/23 02:14:03, Yaron wrote: > Any reason not to do this on testshell as well and avoid the unnecessary > conditional? Or put another way: why configure this from internal repo? If you > want to make this conditional, it could just be a flag. This has to be conditional, since there are a number of secondary broadcast receivers and activities provided by Chrome on Android (and implemented in the internal repo) that assume that the browser starts synchronously. These include the AccountsChangeReceiver and the various bookmark receivers and activities. As such the decision on whether startup is allowed to be asynchronous has to be in the internal repo. I have not investigated whether we could change these to support asynchronous startup, but, at the very least, this would involve considerable extra work. I considered making this a command line flag, but the observer had to be passed down anyway, so it seemed to make sense to pass down the control of this by the same route. https://codereview.chromium.org/19957002/diff/17001/content/browser/android/a... File content/browser/android/android_startup_observer.h (right): https://codereview.chromium.org/19957002/diff/17001/content/browser/android/a... content/browser/android/android_startup_observer.h:24: virtual void OVERRIDE AllStartupTasksRan(); On 2013/07/23 02:14:03, Yaron wrote: > OVERRIDE is typically placed at the end Done. https://codereview.chromium.org/19957002/diff/17001/content/browser/browser_m... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/19957002/diff/17001/content/browser/browser_m... content/browser/browser_main_loop.cc:605: task_runner->AddTask(create_thread); On 2013/07/23 02:14:03, Yaron wrote: > To jam's point, the two slow thread inits that I knew of were: IO thread and > webkit deprecated. IO is now async and webkit deprecated is gone, so is this > part still worth it? I know there are other parts of startup where this might > apply but it maybe unnecessary at this granularity. The time for actual thread creation is now trivial (~6ms), but has to happen after PreCreateThreads, which is the largest single block of work. As such, PreCreateThreads has to run in a task, so anything following it, including thread creation, has to run in tasks. I will change this to create all the threads in a single task, rather than creating a separate task for each thread creation.
On 2013/07/23 13:44:53, aberent wrote: > https://codereview.chromium.org/19957002/diff/17001/chrome/app/android/chrome... > File chrome/app/android/chrome_main_delegate_android.cc (right): > > https://codereview.chromium.org/19957002/diff/17001/chrome/app/android/chrome... > chrome/app/android/chrome_main_delegate_android.cc:38: // For Chrome on android > we want to run the startup tasks incrementally, so > On 2013/07/23 02:14:03, Yaron wrote: > > Any reason not to do this on testshell as well and avoid the unnecessary > > conditional? Or put another way: why configure this from internal repo? If you > > want to make this conditional, it could just be a flag. > This has to be conditional, since there are a number of secondary broadcast > receivers and activities provided by Chrome on Android (and implemented in the > internal repo) that assume that the browser starts synchronously. These include > the AccountsChangeReceiver and the various bookmark receivers and activities. As > such the decision on whether startup is allowed to be asynchronous has to be in > the internal repo. > > I have not investigated whether we could change these to support asynchronous > startup, but, at the very least, this would involve considerable extra work. I don't understand what you mean by the above re secondary broadcast receivers and activities. Do you mean that sometimes chrome on android will use this mode, but others times not, depending on how it's stated? From what I remember from our discussion, I was under the impression that this would be an internal detail of content and not be exposed through the content api. it would be unfortunate to have to expose this, and to have different behavior for chrome vs content shell. https://codereview.chromium.org/19957002/diff/29001/content/public/app/conten... File content/public/app/content_main_delegate.h (right): https://codereview.chromium.org/19957002/diff/29001/content/public/app/conten... content/public/app/content_main_delegate.h:60: scoped_ptr<StartupTaskRunner::Observer> observer); here and above: ContentMainDelegate is an interface to go from content->embedder. you're adding methods that are the other way around. also this is an interface with no members see http://www.chromium.org/developers/content-module/content-api for more background on the design of the content api https://codereview.chromium.org/19957002/diff/29001/content/public/common/sta... File content/public/common/startup_task_runner.h (right): https://codereview.chromium.org/19957002/diff/29001/content/public/common/sta... content/public/common/startup_task_runner.h:34: virtual void AllStartupTasksRan() = 0; see content api link about not having single-method interfaces and using callbacks instead https://codereview.chromium.org/19957002/diff/29001/content/public/common/sta... content/public/common/startup_task_runner.h:68: DISALLOW_COPY_AND_ASSIGN(StartupTaskRunner); see content api docs about interfaces vs concrete classes in the public api
On 2013/07/24 06:45:23, jam wrote: > On 2013/07/23 13:44:53, aberent wrote: > > > https://codereview.chromium.org/19957002/diff/17001/chrome/app/android/chrome... > > File chrome/app/android/chrome_main_delegate_android.cc (right): > > > > > https://codereview.chromium.org/19957002/diff/17001/chrome/app/android/chrome... > > chrome/app/android/chrome_main_delegate_android.cc:38: // For Chrome on > android > > we want to run the startup tasks incrementally, so > > On 2013/07/23 02:14:03, Yaron wrote: > > > Any reason not to do this on testshell as well and avoid the unnecessary > > > conditional? Or put another way: why configure this from internal repo? If > you > > > want to make this conditional, it could just be a flag. > > This has to be conditional, since there are a number of secondary broadcast > > receivers and activities provided by Chrome on Android (and implemented in the > > internal repo) that assume that the browser starts synchronously. These > include > > the AccountsChangeReceiver and the various bookmark receivers and activities. > As > > such the decision on whether startup is allowed to be asynchronous has to be > in > > the internal repo. > > > > I have not investigated whether we could change these to support asynchronous > > startup, but, at the very least, this would involve considerable extra work. > > > I don't understand what you mean by the above re secondary broadcast receivers > and activities. Do you mean that sometimes chrome on android will use this mode, > but others times not, depending on how it's stated? > Yes, sort of. In Android there are a number of mechanisms through which installed applications can communicate. One of these is that an application can register for certain broadcasts, and can define entry points for those broadcasts. One of the broadcasts that the system generates is a broadcast when the set of accounts on the phone changes (e.g. when somebody adds their business Google account as well their personal Google account to the phone). Chrome registers for this broadcast, and when it receives it changes Chrome's account information. Because Chrome is registered for this broadcast the O.S. will start Chrome when this broadcast is sent. This doesn't start the Chrome UI, and Chrome is closed down as soon as it has handled the change in the account information, but, yes, this is a different way of starting Chrome. A second case is bookmark handling. In Android one can create shortcuts for bookmarks on the Android home screen. Clicking on these will go to whichever browser is currently registered to handle bookmarks (starting it if necessary). In addition the entry point (activity in Android terminology) for handling opening a bookmark there are various broadcast receivers and activities for handling changes to bookmarks. All these secondary entry points to Chrome currently consist of a single Java function that does everything needed to handle the broadcast or activity before exiting, hence handling it without needing a UI thread message handler. I am not sure what assumptions the OS or other applications make about the timing of these handlers, but they may assume that they complete synchronously. There is also one other case that I didn't mention before, because it doesn't actually use this code (it has its own delegate class). The Android Webview API includes a function AwBrowserProcess.start() which is defined to completely start the browser components. Unfortunately, since this is part of a public API, and has to remain compatible with applications that used the old Android Webview implementation, we can't make Webview browser startup asynchronous. If you look at the Webview delegate class you will see that I always force synchronous startup in this case. > From what I remember from our discussion, I was under the impression that this > would be an internal detail of content and not be exposed through the content > api. it would be unfortunate to have to expose this, and to have different > behavior for chrome vs content shell. Sorry I mislead you. I always knew that the Java side of startup had to be aware that the C++ was starting asynchronously, to ensure that they didn't run the remaining Java startup code (creating tabs etc.) too early, so the callback, or some similar mechanism, was always going to be needed. I hadn't, at that point, however, realized that there were cases in Android where startup had to be synchronous. If you look at the startup code for content_shell and chromium_shell (ContentShellActivity.java and ChromiumTestShellActivity.java) you will see that in both cases I am using asynchronous startup, as I will be for normal Chrome startup once the downstream patch lands.
On 2013/07/24 09:52:31, aberent wrote: > On 2013/07/24 06:45:23, jam wrote: > > On 2013/07/23 13:44:53, aberent wrote: > > > > > > https://codereview.chromium.org/19957002/diff/17001/chrome/app/android/chrome... > > > File chrome/app/android/chrome_main_delegate_android.cc (right): > > > > > > > > > https://codereview.chromium.org/19957002/diff/17001/chrome/app/android/chrome... > > > chrome/app/android/chrome_main_delegate_android.cc:38: // For Chrome on > > android > > > we want to run the startup tasks incrementally, so > > > On 2013/07/23 02:14:03, Yaron wrote: > > > > Any reason not to do this on testshell as well and avoid the unnecessary > > > > conditional? Or put another way: why configure this from internal repo? If > > you > > > > want to make this conditional, it could just be a flag. > > > This has to be conditional, since there are a number of secondary broadcast > > > receivers and activities provided by Chrome on Android (and implemented in > the > > > internal repo) that assume that the browser starts synchronously. These > > include > > > the AccountsChangeReceiver and the various bookmark receivers and > activities. > > As > > > such the decision on whether startup is allowed to be asynchronous has to be > > in > > > the internal repo. > > > > > > I have not investigated whether we could change these to support > asynchronous > > > startup, but, at the very least, this would involve considerable extra work. > > > > > > I don't understand what you mean by the above re secondary broadcast receivers > > and activities. Do you mean that sometimes chrome on android will use this > mode, > > but others times not, depending on how it's stated? > > > Yes, sort of. In Android there are a number of mechanisms through which > installed applications can communicate. One of these is that an application can > register for certain broadcasts, and can define entry points for those > broadcasts. One of the broadcasts that the system generates is a broadcast when > the set of accounts on the phone changes (e.g. when somebody adds their business > Google account as well their personal Google account to the phone). Chrome > registers for this broadcast, and when it receives it changes Chrome's account > information. Because Chrome is registered for this broadcast the O.S. will start > Chrome when this broadcast is sent. This doesn't start the Chrome UI, and Chrome > is closed down as soon as it has handled the change in the account information, > but, yes, this is a different way of starting Chrome. > > A second case is bookmark handling. In Android one can create shortcuts for > bookmarks on the Android home screen. Clicking on these will go to whichever > browser is currently registered to handle bookmarks (starting it if necessary). > In addition the entry point (activity in Android terminology) for handling > opening a bookmark there are various broadcast receivers and activities for > handling changes to bookmarks. > > All these secondary entry points to Chrome currently consist of a single Java > function that does everything needed to handle the broadcast or activity before > exiting, hence handling it without needing a UI thread message handler. I am not > sure what assumptions the OS or other applications make about the timing of > these handlers, but they may assume that they complete synchronously. > > There is also one other case that I didn't mention before, because it doesn't > actually use this code (it has its own delegate class). The Android Webview API > includes a function AwBrowserProcess.start() which is defined to completely > start the browser components. Unfortunately, since this is part of a public API, > and has to remain compatible with applications that used the old Android Webview > implementation, we can't make Webview browser startup asynchronous. If you look > at the Webview delegate class you will see that I always force synchronous > startup in this case. Thanks for the background. My next question would be to ask why we can't have content start asynchronously all the time on android. For the cases that you give, it sounds like there's no UI. Would it be possible to create a chrome message loop which only pumps chrome tasks, post a task to it after calling content main's initialization, and blocking till that task runs? That would allow us to hide all this implementation detail from content/public, and would make all the users of content on android support this without doing any work. > > > From what I remember from our discussion, I was under the impression that this > > would be an internal detail of content and not be exposed through the content > > api. it would be unfortunate to have to expose this, and to have different > > behavior for chrome vs content shell. > > Sorry I mislead you. I always knew that the Java side of startup had to be aware > that the C++ was starting asynchronously, to ensure that they didn't run the > remaining Java startup code (creating tabs etc.) too early, so the callback, or > some similar mechanism, was always going to be needed. I hadn't, at that point, > however, realized that there were cases in Android where startup had to be > synchronous. > > If you look at the startup code for content_shell and chromium_shell > (ContentShellActivity.java and ChromiumTestShellActivity.java) you will see that > in both cases I am using asynchronous startup, as I will be for normal Chrome > startup once the downstream patch lands.
On 2013/07/24 19:15:00, jam wrote: > On 2013/07/24 09:52:31, aberent wrote: > > On 2013/07/24 06:45:23, jam wrote: > > > On 2013/07/23 13:44:53, aberent wrote: > > > > > > > > > > https://codereview.chromium.org/19957002/diff/17001/chrome/app/android/chrome... > > > > File chrome/app/android/chrome_main_delegate_android.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/19957002/diff/17001/chrome/app/android/chrome... > > > > chrome/app/android/chrome_main_delegate_android.cc:38: // For Chrome on > > > android > > > > we want to run the startup tasks incrementally, so > > > > On 2013/07/23 02:14:03, Yaron wrote: > > > > > Any reason not to do this on testshell as well and avoid the unnecessary > > > > > conditional? Or put another way: why configure this from internal repo? > If > > > you > > > > > want to make this conditional, it could just be a flag. > > > > This has to be conditional, since there are a number of secondary > broadcast > > > > receivers and activities provided by Chrome on Android (and implemented in > > the > > > > internal repo) that assume that the browser starts synchronously. These > > > include > > > > the AccountsChangeReceiver and the various bookmark receivers and > > activities. > > > As > > > > such the decision on whether startup is allowed to be asynchronous has to > be > > > in > > > > the internal repo. > > > > > > > > I have not investigated whether we could change these to support > > asynchronous > > > > startup, but, at the very least, this would involve considerable extra > work. > > > > > > > > > I don't understand what you mean by the above re secondary broadcast > receivers > > > and activities. Do you mean that sometimes chrome on android will use this > > mode, > > > but others times not, depending on how it's stated? > > > > > Yes, sort of. In Android there are a number of mechanisms through which > > installed applications can communicate. One of these is that an application > can > > register for certain broadcasts, and can define entry points for those > > broadcasts. One of the broadcasts that the system generates is a broadcast > when > > the set of accounts on the phone changes (e.g. when somebody adds their > business > > Google account as well their personal Google account to the phone). Chrome > > registers for this broadcast, and when it receives it changes Chrome's account > > information. Because Chrome is registered for this broadcast the O.S. will > start > > Chrome when this broadcast is sent. This doesn't start the Chrome UI, and > Chrome > > is closed down as soon as it has handled the change in the account > information, > > but, yes, this is a different way of starting Chrome. > > > > A second case is bookmark handling. In Android one can create shortcuts for > > bookmarks on the Android home screen. Clicking on these will go to whichever > > browser is currently registered to handle bookmarks (starting it if > necessary). > > In addition the entry point (activity in Android terminology) for handling > > opening a bookmark there are various broadcast receivers and activities for > > handling changes to bookmarks. > > > > All these secondary entry points to Chrome currently consist of a single Java > > function that does everything needed to handle the broadcast or activity > before > > exiting, hence handling it without needing a UI thread message handler. I am > not > > sure what assumptions the OS or other applications make about the timing of > > these handlers, but they may assume that they complete synchronously. > > > > There is also one other case that I didn't mention before, because it doesn't > > actually use this code (it has its own delegate class). The Android Webview > API > > includes a function AwBrowserProcess.start() which is defined to completely > > start the browser components. Unfortunately, since this is part of a public > API, > > and has to remain compatible with applications that used the old Android > Webview > > implementation, we can't make Webview browser startup asynchronous. If you > look > > at the Webview delegate class you will see that I always force synchronous > > startup in this case. > > Thanks for the background. > > My next question would be to ask why we can't have content start asynchronously > all the time on android. For the cases that you give, it sounds like there's no > UI. Would it be possible to create a chrome message loop which only pumps chrome > tasks, post a task to it after calling content main's initialization, and > blocking till that task runs? That would allow us to hide all this > implementation detail from content/public, and would make all the users of > content on android support this without doing any work. > I am not sure that I am still thinking clearly today (it is quite late here), so I may find the answers myself in the morning, however I think this would be difficult to do for a number of reasons: 1. Some of the lower level startup code checks that it is being run on the correct type of message loop (e.g. MessageLoopForUI::current(), which is used during startup, does this). As such we would have to create a message loop of type base::MessageLoop::TYPE_UI. The Android version of the UI message loop is implemented using callbacks to Java, and hence to the UI thread's system message loop. Changing this would require major work, and we would still somehow have to pass down to base what sort of Android UI message loop we wanted in each case. As such doing this would I think simply move the complexity from content to base. 2. For at least the Android Webview case we do have a UI message loop (the application's UI thread) to which Chrome will later post its events. The Android Webview API requires that the browser is started within a single UI thread function call and hence within the processing of a single UI thread message. To handle this case we would have to create a second temporary UI thread event loop, which is both difficult on Android and, I believe, banned by Chrome architecture standards. 3. I am unclear what you mean by "post a task to it after calling content main's initialization". If the aim is to avoid the callback then this would have to be posted to StartupTaskRunner, not to the message loop, since otherwise it would overtake the tasks in the StartupTaskRunner queue (just as input events intentionally do).
https://codereview.chromium.org/19957002/diff/29001/content/public/common/sta... File content/public/common/startup_task_runner.h (right): https://codereview.chromium.org/19957002/diff/29001/content/public/common/sta... content/public/common/startup_task_runner.h:34: virtual void AllStartupTasksRan() = 0; On 2013/07/24 06:45:24, jam wrote: > see content api link about not having single-method interfaces and using > callbacks instead Done. https://codereview.chromium.org/19957002/diff/29001/content/public/common/sta... content/public/common/startup_task_runner.h:68: DISALLOW_COPY_AND_ASSIGN(StartupTaskRunner); On 2013/07/24 06:45:24, jam wrote: > see content api docs about interfaces vs concrete classes in the public api After looking at this I realized that I don't actually need StartupTaskRunner to be an api class. The only thing that was being called from above the api was the constructor. I am now, instead, passing down the constructor's arguments so that StartupTaskRunner is only ever referenced within content.
https://codereview.chromium.org/19957002/diff/29001/chrome/android/testshell/... File chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java (right): https://codereview.chromium.org/19957002/diff/29001/chrome/android/testshell/... chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java:56: protected void onCreate(Bundle savedInstanceState) { If you make this final, you can pass it through to finishInitialization and drop the member variable https://codereview.chromium.org/19957002/diff/29001/chrome/app/android/chrome... File chrome/app/android/chrome_main_delegate_android.h (right): https://codereview.chromium.org/19957002/diff/29001/chrome/app/android/chrome... chrome/app/android/chrome_main_delegate_android.h:32: virtual void OVERRIDE EnableIncrementalStartup(bool enable); Again, please move OVERRIDE to the end. Please verify throughout your patch https://codereview.chromium.org/19957002/diff/29001/content/browser/android/a... File content/browser/android/android_startup_observer.h (right): https://codereview.chromium.org/19957002/diff/29001/content/browser/android/a... content/browser/android/android_startup_observer.h:22: jobject java_observer_; Make this a ScopedJavaGlobalRef and clear out the destructor. In general, we try to avoid hanging jobjects as ownership/lifetime isn't clear. https://codereview.chromium.org/19957002/diff/29001/content/browser/browser_m... File content/browser/browser_main_runner.cc (right): https://codereview.chromium.org/19957002/diff/29001/content/browser/browser_m... content/browser/browser_main_runner.cc:45: const scoped_refptr<StartupTaskRunner>& task_runner) OVERRIDE { Does this need to be a refptr? Couldn't ownership pass to BrowserMainRunnerImpl? https://codereview.chromium.org/19957002/diff/29001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/StartupObserver.java (right): https://codereview.chromium.org/19957002/diff/29001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/StartupObserver.java:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2013
On 2013/07/24 22:17:18, aberent wrote: > On 2013/07/24 19:15:00, jam wrote: > > On 2013/07/24 09:52:31, aberent wrote: > > > On 2013/07/24 06:45:23, jam wrote: > > > > On 2013/07/23 13:44:53, aberent wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/19957002/diff/17001/chrome/app/android/chrome... > > > > > File chrome/app/android/chrome_main_delegate_android.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/19957002/diff/17001/chrome/app/android/chrome... > > > > > chrome/app/android/chrome_main_delegate_android.cc:38: // For Chrome on > > > > android > > > > > we want to run the startup tasks incrementally, so > > > > > On 2013/07/23 02:14:03, Yaron wrote: > > > > > > Any reason not to do this on testshell as well and avoid the > unnecessary > > > > > > conditional? Or put another way: why configure this from internal > repo? > > If > > > > you > > > > > > want to make this conditional, it could just be a flag. > > > > > This has to be conditional, since there are a number of secondary > > broadcast > > > > > receivers and activities provided by Chrome on Android (and implemented > in > > > the > > > > > internal repo) that assume that the browser starts synchronously. These > > > > include > > > > > the AccountsChangeReceiver and the various bookmark receivers and > > > activities. > > > > As > > > > > such the decision on whether startup is allowed to be asynchronous has > to > > be > > > > in > > > > > the internal repo. > > > > > > > > > > I have not investigated whether we could change these to support > > > asynchronous > > > > > startup, but, at the very least, this would involve considerable extra > > work. > > > > > > > > > > > > I don't understand what you mean by the above re secondary broadcast > > receivers > > > > and activities. Do you mean that sometimes chrome on android will use this > > > mode, > > > > but others times not, depending on how it's stated? > > > > > > > Yes, sort of. In Android there are a number of mechanisms through which > > > installed applications can communicate. One of these is that an application > > can > > > register for certain broadcasts, and can define entry points for those > > > broadcasts. One of the broadcasts that the system generates is a broadcast > > when > > > the set of accounts on the phone changes (e.g. when somebody adds their > > business > > > Google account as well their personal Google account to the phone). Chrome > > > registers for this broadcast, and when it receives it changes Chrome's > account > > > information. Because Chrome is registered for this broadcast the O.S. will > > start > > > Chrome when this broadcast is sent. This doesn't start the Chrome UI, and > > Chrome > > > is closed down as soon as it has handled the change in the account > > information, > > > but, yes, this is a different way of starting Chrome. > > > > > > A second case is bookmark handling. In Android one can create shortcuts for > > > bookmarks on the Android home screen. Clicking on these will go to whichever > > > browser is currently registered to handle bookmarks (starting it if > > necessary). > > > In addition the entry point (activity in Android terminology) for handling > > > opening a bookmark there are various broadcast receivers and activities for > > > handling changes to bookmarks. > > > > > > All these secondary entry points to Chrome currently consist of a single > Java > > > function that does everything needed to handle the broadcast or activity > > before > > > exiting, hence handling it without needing a UI thread message handler. I am > > not > > > sure what assumptions the OS or other applications make about the timing of > > > these handlers, but they may assume that they complete synchronously. > > > > > > There is also one other case that I didn't mention before, because it > doesn't > > > actually use this code (it has its own delegate class). The Android Webview > > API > > > includes a function AwBrowserProcess.start() which is defined to completely > > > start the browser components. Unfortunately, since this is part of a public > > API, > > > and has to remain compatible with applications that used the old Android > > Webview > > > implementation, we can't make Webview browser startup asynchronous. If you > > look > > > at the Webview delegate class you will see that I always force synchronous > > > startup in this case. > > > > Thanks for the background. > > > > My next question would be to ask why we can't have content start > asynchronously > > all the time on android. For the cases that you give, it sounds like there's > no > > UI. Would it be possible to create a chrome message loop which only pumps > chrome > > tasks, post a task to it after calling content main's initialization, and > > blocking till that task runs? That would allow us to hide all this > > implementation detail from content/public, and would make all the users of > > content on android support this without doing any work. > > > > I am not sure that I am still thinking clearly today (it is quite late here), so > I may find the answers myself in the morning, however I think this would be > difficult to do for a number of reasons: > > 1. Some of the lower level startup code checks that it is being run on the > correct type of message loop (e.g. MessageLoopForUI::current(), which is used > during startup, does this). As such we would have to create a message loop of > type base::MessageLoop::TYPE_UI. The Android version of the UI message loop is > implemented using callbacks to Java, and hence to the UI thread's system message > loop. Changing this would require major work, and we would still somehow have to > pass down to base what sort of Android UI message loop we wanted in each case. > As such doing this would I think simply move the complexity from content to > base. > > 2. For at least the Android Webview case we do have a UI message loop (the > application's UI thread) to which Chrome will later post its events. The Android > Webview API requires that the browser is started within a single UI thread > function call and hence within the processing of a single UI thread message. To > handle this case we would have to create a second temporary UI thread event > loop, which is both difficult on Android and, I believe, banned by Chrome > architecture standards. > > 3. I am unclear what you mean by "post a task to it after calling content main's > initialization". If the aim is to avoid the callback then this would have to be > posted to StartupTaskRunner, not to the message loop, since otherwise it would > overtake the tasks in the StartupTaskRunner queue (just as input events > intentionally do). The next version, which I am about to upload, moves the configuration interface for startup to content/public/android/java, so it is much less visible to other platforms. This version is significantly different (and, I think, simpler) than previous versions.
https://codereview.chromium.org/19957002/diff/2001/content/public/browser/bro... File content/public/browser/browser_main_runner.h (right): https://codereview.chromium.org/19957002/diff/2001/content/public/browser/bro... content/public/browser/browser_main_runner.h:11: #include "content/public/common/startup_task_runner.h" On 2013/07/22 16:28:15, joth wrote: > you could just fwd declare StartupTaskRunner here This file is no longer part of the CL. https://codereview.chromium.org/19957002/diff/29001/chrome/android/testshell/... File chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java (right): https://codereview.chromium.org/19957002/diff/29001/chrome/android/testshell/... chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java:56: protected void onCreate(Bundle savedInstanceState) { On 2013/07/25 00:05:17, Yaron wrote: > If you make this final, you can pass it through to finishInitialization and drop > the member variable Done. https://codereview.chromium.org/19957002/diff/29001/chrome/app/android/chrome... File chrome/app/android/chrome_main_delegate_android.h (right): https://codereview.chromium.org/19957002/diff/29001/chrome/app/android/chrome... chrome/app/android/chrome_main_delegate_android.h:32: virtual void OVERRIDE EnableIncrementalStartup(bool enable); On 2013/07/25 00:05:17, Yaron wrote: > Again, please move OVERRIDE to the end. Please verify throughout your patch Done. And verified throughout patch. https://codereview.chromium.org/19957002/diff/29001/content/browser/android/a... File content/browser/android/android_startup_observer.h (right): https://codereview.chromium.org/19957002/diff/29001/content/browser/android/a... content/browser/android/android_startup_observer.h:22: jobject java_observer_; On 2013/07/25 00:05:17, Yaron wrote: > Make this a ScopedJavaGlobalRef and clear out the destructor. In general, we try > to avoid hanging jobjects as ownership/lifetime isn't clear. Following a rethink of configuration this class no longer exists, and I am no longer storing any Java objects on the C++ side. https://codereview.chromium.org/19957002/diff/29001/content/browser/browser_m... File content/browser/browser_main_runner.cc (right): https://codereview.chromium.org/19957002/diff/29001/content/browser/browser_m... content/browser/browser_main_runner.cc:45: const scoped_refptr<StartupTaskRunner>& task_runner) OVERRIDE { On 2013/07/25 00:05:17, Yaron wrote: > Does this need to be a refptr? Couldn't ownership pass to BrowserMainRunnerImpl? This has gone away due a restructuring following Jam's comments about the content API. https://codereview.chromium.org/19957002/diff/29001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/StartupObserver.java (right): https://codereview.chromium.org/19957002/diff/29001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/StartupObserver.java:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/07/25 00:05:17, Yaron wrote: > 2013 Class no longer exists, but I have checked the copyrights of my other new source files. https://codereview.chromium.org/19957002/diff/29001/content/public/app/conten... File content/public/app/content_main_delegate.h (right): https://codereview.chromium.org/19957002/diff/29001/content/public/app/conten... content/public/app/content_main_delegate.h:60: scoped_ptr<StartupTaskRunner::Observer> observer); On 2013/07/24 06:45:24, jam wrote: > here and above: ContentMainDelegate is an interface to go from > content->embedder. you're adding methods that are the other way around. also > this is an interface with no members > > see http://www.chromium.org/developers/content-module/content-api for more > background on the design of the content api Done. I have rethought how I configure the startup queue, so that the configuration is now stored in the Android Java parts on content (or defaults to synchronous with a trivial callback). There is a new interface in the Android Java component for configuring this.
https://codereview.chromium.org/19957002/diff/88001/content/browser/browser_m... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/19957002/diff/88001/content/browser/browser_m... content/browser/browser_main_loop.cc:520: scoped_refptr<StartupTaskRunner> task_runner = Again, can't this be owned by BrowserMainRunnerImpl? https://codereview.chromium.org/19957002/diff/88001/content/browser/browser_m... File content/browser/browser_main_runner.cc (right): https://codereview.chromium.org/19957002/diff/88001/content/browser/browser_m... content/browser/browser_main_runner.cc:109: created_threads_ = true; It seems like this is no longer correct. In the async case, we'll set this immediately but the threads haven't been created yet as we haven't yielded. https://codereview.chromium.org/19957002/diff/88001/content/shell/android/she... File content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellActivity.java (right): https://codereview.chromium.org/19957002/diff/88001/content/shell/android/she... content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellActivity.java:113: String shellUrl = ShellManager.DEFAULT_SHELL_URL; Shouldn't this also be part of finishInitialization? Why is this video stuff different?
New patch follows once I have rerun a few more tests https://codereview.chromium.org/19957002/diff/88001/content/browser/browser_m... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/19957002/diff/88001/content/browser/browser_m... content/browser/browser_main_loop.cc:520: scoped_refptr<StartupTaskRunner> task_runner = On 2013/07/30 00:06:24, Yaron wrote: > Again, can't this be owned by BrowserMainRunnerImpl? I assume you are suggesting making this a class member and removing the scoped_refptr. If not, then please explain further. I am anyway unclear why you are suggesting BrowserMainRunnerImpl rather than BrowserMainLoop, since BrowserMainRunnerImpl now needs no knowledge of this, and moving to there would mean passing it through extra levels. This would be possible to make this a class variable but it makes lifetime management more difficult. The StartupTaskRunner object has to stay alive as long as there are tasks in the queue, but is then redundant. This is handled at the moment by passing it (within its implementation) to base::Bind(), which then manages its lifetime using a scoped_refptr, and deletes it once the last task has run. We could tell Bind not to manage it, but in this case the owning class would have to delete it. At the moment BrowserMainRunnerImpl doesn't know when the last startup task has finished, so doesn't know when to delete it (this could be fixed, but would complicate the code). As such we would probably end up leaving this object around for the whole life of Chrome, rather than deleting it at the end of startup. Please explain further why you think using a class member variable would be better in this case, if this is what you meant. https://codereview.chromium.org/19957002/diff/88001/content/browser/browser_m... File content/browser/browser_main_runner.cc (right): https://codereview.chromium.org/19957002/diff/88001/content/browser/browser_m... content/browser/browser_main_runner.cc:109: created_threads_ = true; On 2013/07/30 00:06:24, Yaron wrote: > It seems like this is no longer correct. In the async case, we'll set this > immediately but the threads haven't been created yet as we haven't yielded. This is only used to decide whether to call BrowserMainLoop::ShutdownThreadsAndCleanUp() during shutdown, so I have now moved the logic to decide what shutdown processing to do into that function, and recorded in BrowserMainLoop (which creates the threads) whether the threads have been created. https://codereview.chromium.org/19957002/diff/88001/content/shell/android/she... File content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellActivity.java (right): https://codereview.chromium.org/19957002/diff/88001/content/shell/android/she... content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellActivity.java:113: String shellUrl = ShellManager.DEFAULT_SHELL_URL; On 2013/07/30 00:06:24, Yaron wrote: > Shouldn't this also be part of finishInitialization? Why is this video stuff > different? I am not sure why it is different, but it is reproducing what it did before. In the original code this is only executed if init returns false, which means that the process was already running. If the process is already running the callback won't be called, so we need this code. Your comment has, however, made me realize that I was not handling a false return from init correctly in other asynchronous cases. Those have now been fixed.
lgtm I'm happy from an Android perspective. Some minor nits as suggestions for the test. https://codereview.chromium.org/19957002/diff/88001/content/browser/browser_m... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/19957002/diff/88001/content/browser/browser_m... content/browser/browser_main_loop.cc:520: scoped_refptr<StartupTaskRunner> task_runner = On 2013/07/30 15:01:54, aberent wrote: > On 2013/07/30 00:06:24, Yaron wrote: > > Again, can't this be owned by BrowserMainRunnerImpl? > > I assume you are suggesting making this a class member and removing the > scoped_refptr. If not, then please explain further. I am anyway unclear why you > are suggesting BrowserMainRunnerImpl rather than BrowserMainLoop, since > BrowserMainRunnerImpl now needs no knowledge of this, and moving to there would > mean passing it through extra levels. > > This would be possible to make this a class variable but it makes lifetime > management more difficult. The StartupTaskRunner object has to stay alive as > long as there are tasks in the queue, but is then redundant. This is handled at > the moment by passing it (within its implementation) to base::Bind(), which then > manages its lifetime using a scoped_refptr, and deletes it once the last task > has run. We could tell Bind not to manage it, but in this case the owning class > would have to delete it. At the moment BrowserMainRunnerImpl doesn't know when > the last startup task has finished, so doesn't know when to delete it (this > could be fixed, but would complicate the code). As such we would probably end up > leaving this object around for the whole life of Chrome, rather than deleting it > at the end of startup. > > Please explain further why you think using a class member variable would be > better in this case, if this is what you meant. > > Err, you're right that I should've said BrowserMainLoop. I guess I have an unhealthy aversion to refptr. I don't see it as a major problem that the task_runner is owned until BrowserMainLoop is deleted but it is unnecessary. Ok, you've convinced me. Thanks for bearing with me https://codereview.chromium.org/19957002/diff/88001/content/shell/android/she... File content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellActivity.java (right): https://codereview.chromium.org/19957002/diff/88001/content/shell/android/she... content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellActivity.java:113: String shellUrl = ShellManager.DEFAULT_SHELL_URL; On 2013/07/30 15:01:54, aberent wrote: > On 2013/07/30 00:06:24, Yaron wrote: > > Shouldn't this also be part of finishInitialization? Why is this video stuff > > different? > > I am not sure why it is different, but it is reproducing what it did before. In > the original code this is only executed if init returns false, which means that > the process was already running. If the process is already running the callback > won't be called, so we need this code. > > Your comment has, however, made me realize that I was not handling a false > return from init correctly in other asynchronous cases. Those have now been > fixed. I think that in practice, this always starts the browser process since it's basically only used for instrumentation tests and they always re-launch. That said, I guess it's fine to leave as-is for this CL. https://codereview.chromium.org/19957002/diff/106001/chrome/android/testshell... File chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java (right): https://codereview.chromium.org/19957002/diff/106001/chrome/android/testshell... chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java:63: @Override Indentation appears off throughout this function. https://codereview.chromium.org/19957002/diff/106001/content/browser/startup_... File content/browser/startup_task_runner_unittest.cc (right): https://codereview.chromium.org/19957002/diff/106001/content/browser/startup_... content/browser/startup_task_runner_unittest.cc:26: bool observer_called = false; Nit: I think it's common to place these members as part of the test fixture. https://codereview.chromium.org/19957002/diff/106001/content/browser/startup_... content/browser/startup_task_runner_unittest.cc:30: private: Nit: A little unorthodox. Please move private to the bototm. https://codereview.chromium.org/19957002/diff/106001/content/browser/startup_... content/browser/startup_task_runner_unittest.cc:51: int Task3() { Nit: renaming FailingTask (and the local variable name) makes it easier to see what your'e doing. https://codereview.chromium.org/19957002/diff/106001/content/browser/startup_... content/browser/startup_task_runner_unittest.cc:161: base::Closure task; Nit: member variabe as well.
thanks, I really appreciate hiding this from the content api. just one small nit https://codereview.chromium.org/19957002/diff/106001/content/browser/browser_... File content/browser/browser_startup_configuration.cc (right): https://codereview.chromium.org/19957002/diff/106001/content/browser/browser_... content/browser/browser_startup_configuration.cc:11: void BrowserStartupComplete(int result) {} this way of switching behavior for android/non-android isn't a pattern that we use. why not just have one method that's in the calling code, and have ifdef's in it for android?
New patch to follow soon. https://codereview.chromium.org/19957002/diff/106001/chrome/android/testshell... File chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java (right): https://codereview.chromium.org/19957002/diff/106001/chrome/android/testshell... chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java:63: @Override On 2013/07/30 18:53:22, Yaron wrote: > Indentation appears off throughout this function. Done. https://codereview.chromium.org/19957002/diff/106001/content/browser/browser_... File content/browser/browser_startup_configuration.cc (right): https://codereview.chromium.org/19957002/diff/106001/content/browser/browser_... content/browser/browser_startup_configuration.cc:11: void BrowserStartupComplete(int result) {} On 2013/07/31 00:29:27, jam wrote: > this way of switching behavior for android/non-android isn't a pattern that we > use. why not just have one method that's in the calling code, and have ifdef's > in it for android? Done. https://codereview.chromium.org/19957002/diff/106001/content/browser/startup_... File content/browser/startup_task_runner_unittest.cc (right): https://codereview.chromium.org/19957002/diff/106001/content/browser/startup_... content/browser/startup_task_runner_unittest.cc:26: bool observer_called = false; On 2013/07/30 18:53:22, Yaron wrote: > Nit: I think it's common to place these members as part of the test fixture. Normally I would, but these would have to be static members, since they are used by Observer and SaveTaskArg, which themselves have to be either outside the class or static members. As such they would need to be defined outside the class, so I don't in this case see a gain in readability by declaring them inside the class. Their scope is already restricted to the test through the anonymous namespace. I have, however, changed the order of things in the file so that it is more readable. https://codereview.chromium.org/19957002/diff/106001/content/browser/startup_... content/browser/startup_task_runner_unittest.cc:30: private: On 2013/07/30 18:53:22, Yaron wrote: > Nit: A little unorthodox. Please move private to the bototm. Done. https://codereview.chromium.org/19957002/diff/106001/content/browser/startup_... content/browser/startup_task_runner_unittest.cc:51: int Task3() { On 2013/07/30 18:53:22, Yaron wrote: > Nit: renaming FailingTask (and the local variable name) makes it easier to see > what your'e doing. Done. https://codereview.chromium.org/19957002/diff/106001/content/browser/startup_... content/browser/startup_task_runner_unittest.cc:161: base::Closure task; On 2013/07/30 18:53:22, Yaron wrote: > Nit: member variabe as well. Could only be static member variable since used by SaveTaskArg. As discussed above I see little advantage in this.
lgtm with nits and removing browser_startup_configuration.* https://codereview.chromium.org/19957002/diff/123001/content/browser/browser_... File content/browser/browser_startup_configuration.h (right): https://codereview.chromium.org/19957002/diff/123001/content/browser/browser_... content/browser/browser_startup_configuration.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. i should have been clearer: i had meant that you don't need this header and cc file as well. just move these methods to the caller. https://codereview.chromium.org/19957002/diff/123001/content/browser/startup_... File content/browser/startup_task_runner.h (right): https://codereview.chromium.org/19957002/diff/123001/content/browser/startup_... content/browser/startup_task_runner.h:20: typedef base::Callback<int(void)> StartupTask; nit: please document what the int return value is used for https://codereview.chromium.org/19957002/diff/123001/content/browser/startup_... content/browser/startup_task_runner.h:36: nit: here and below, no blank like after "public" and "private" per convention
https://codereview.chromium.org/19957002/diff/123001/content/browser/browser_... File content/browser/browser_startup_configuration.h (right): https://codereview.chromium.org/19957002/diff/123001/content/browser/browser_... content/browser/browser_startup_configuration.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2013/07/31 18:07:52, jam wrote: > i should have been clearer: i had meant that you don't need this header and cc > file as well. just move these methods to the caller. Done as discussed on IM. https://codereview.chromium.org/19957002/diff/123001/content/browser/startup_... File content/browser/startup_task_runner.h (right): https://codereview.chromium.org/19957002/diff/123001/content/browser/startup_... content/browser/startup_task_runner.h:20: typedef base::Callback<int(void)> StartupTask; On 2013/07/31 18:07:52, jam wrote: > nit: please document what the int return value is used for Done. https://codereview.chromium.org/19957002/diff/123001/content/browser/startup_... content/browser/startup_task_runner.h:36: On 2013/07/31 18:07:52, jam wrote: > nit: here and below, no blank like after "public" and "private" per convention Done.
https://codereview.chromium.org/19957002/diff/117002/content/browser/startup_... File content/browser/startup_task_runner.cc (right): https://codereview.chromium.org/19957002/diff/117002/content/browser/startup_... content/browser/startup_task_runner.cc:15: void (*const startup_complete_callback)(int result), nit: use callbacks instead of c function pointers https://codereview.chromium.org/19957002/diff/117002/content/browser/startup_... content/browser/startup_task_runner.cc:41: if(startup_complete_callback_ != NULL) { nit: if (startup_complete_callback) https://codereview.chromium.org/19957002/diff/117002/content/browser/startup_... content/browser/startup_task_runner.cc:42: startup_complete_callback_(result); nit: spacing https://codereview.chromium.org/19957002/diff/117002/content/browser/startup_... content/browser/startup_task_runner.cc:48: // Run the current task nit: this comment and below are just describing the code https://codereview.chromium.org/19957002/diff/117002/content/browser/startup_... content/browser/startup_task_runner.cc:52: if(startup_complete_callback_ != NULL) { nit if (startup_complete_callback_)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aberent@chromium.org/19957002/134001
https://codereview.chromium.org/19957002/diff/117002/content/browser/startup_... File content/browser/startup_task_runner.cc (right): https://codereview.chromium.org/19957002/diff/117002/content/browser/startup_... content/browser/startup_task_runner.cc:15: void (*const startup_complete_callback)(int result), On 2013/07/31 21:26:53, jam wrote: > nit: use callbacks instead of c function pointers Done. https://codereview.chromium.org/19957002/diff/117002/content/browser/startup_... content/browser/startup_task_runner.cc:41: if(startup_complete_callback_ != NULL) { On 2013/07/31 21:26:53, jam wrote: > nit: if (startup_complete_callback) Done. https://codereview.chromium.org/19957002/diff/117002/content/browser/startup_... content/browser/startup_task_runner.cc:42: startup_complete_callback_(result); On 2013/07/31 21:26:53, jam wrote: > nit: spacing Done. https://codereview.chromium.org/19957002/diff/117002/content/browser/startup_... content/browser/startup_task_runner.cc:48: // Run the current task On 2013/07/31 21:26:53, jam wrote: > nit: this comment and below are just describing the code Done. https://codereview.chromium.org/19957002/diff/117002/content/browser/startup_... content/browser/startup_task_runner.cc:52: if(startup_complete_callback_ != NULL) { On 2013/07/31 21:26:53, jam wrote: > nit if (startup_complete_callback_) Done.
On 2013/08/01 13:56:37, aberent wrote: > https://codereview.chromium.org/19957002/diff/117002/content/browser/startup_... > File content/browser/startup_task_runner.cc (right): > > https://codereview.chromium.org/19957002/diff/117002/content/browser/startup_... > content/browser/startup_task_runner.cc:15: void (*const > startup_complete_callback)(int result), > On 2013/07/31 21:26:53, jam wrote: > > nit: use callbacks instead of c function pointers > > Done. > > https://codereview.chromium.org/19957002/diff/117002/content/browser/startup_... > content/browser/startup_task_runner.cc:41: if(startup_complete_callback_ != > NULL) { > On 2013/07/31 21:26:53, jam wrote: > > nit: if (startup_complete_callback) > > Done. > > https://codereview.chromium.org/19957002/diff/117002/content/browser/startup_... > content/browser/startup_task_runner.cc:42: startup_complete_callback_(result); > On 2013/07/31 21:26:53, jam wrote: > > nit: spacing > > Done. > > https://codereview.chromium.org/19957002/diff/117002/content/browser/startup_... > content/browser/startup_task_runner.cc:48: // Run the current task > On 2013/07/31 21:26:53, jam wrote: > > nit: this comment and below are just describing the code > > Done. > > https://codereview.chromium.org/19957002/diff/117002/content/browser/startup_... > content/browser/startup_task_runner.cc:52: if(startup_complete_callback_ != > NULL) { > On 2013/07/31 21:26:53, jam wrote: > > nit if (startup_complete_callback_) > > Done. Commit stopped because I have just hit a crash on Android. Investigating.
On 2013/08/01 14:57:20, aberent wrote: > On 2013/08/01 13:56:37, aberent wrote: > > > https://codereview.chromium.org/19957002/diff/117002/content/browser/startup_... > > File content/browser/startup_task_runner.cc (right): > > > > > https://codereview.chromium.org/19957002/diff/117002/content/browser/startup_... > > content/browser/startup_task_runner.cc:15: void (*const > > startup_complete_callback)(int result), > > On 2013/07/31 21:26:53, jam wrote: > > > nit: use callbacks instead of c function pointers > > > > Done. > > > > > https://codereview.chromium.org/19957002/diff/117002/content/browser/startup_... > > content/browser/startup_task_runner.cc:41: if(startup_complete_callback_ != > > NULL) { > > On 2013/07/31 21:26:53, jam wrote: > > > nit: if (startup_complete_callback) > > > > Done. > > > > > https://codereview.chromium.org/19957002/diff/117002/content/browser/startup_... > > content/browser/startup_task_runner.cc:42: startup_complete_callback_(result); > > On 2013/07/31 21:26:53, jam wrote: > > > nit: spacing > > > > Done. > > > > > https://codereview.chromium.org/19957002/diff/117002/content/browser/startup_... > > content/browser/startup_task_runner.cc:48: // Run the current task > > On 2013/07/31 21:26:53, jam wrote: > > > nit: this comment and below are just describing the code > > > > Done. > > > > > https://codereview.chromium.org/19957002/diff/117002/content/browser/startup_... > > content/browser/startup_task_runner.cc:52: if(startup_complete_callback_ != > > NULL) { > > On 2013/07/31 21:26:53, jam wrote: > > > nit if (startup_complete_callback_) > > > > Done. > > > Commit stopped because I have just hit a crash on Android. Investigating. Crash also occurs on existing master, so continuing commit. Bug https://code.google.com/p/chromium/issues/detail?id=266905 (Googlers only).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aberent@chromium.org/19957002/134001
Message was sent while issue was closed.
Change committed as 215042 |
