Move Windows-specific first run task out of chrome_browser_main.cc by introducing first_run::DoFirs…

Move Windows-specific first run task out of chrome_browser_main.cc by introducing first_run::DoFirstRunTasks().


BUG=None (fixing TODO).
TEST=Shortcut creation on first run for system-level installs works the same way as before (i.e. Active Setup task is fired 5s after launching Chrome on first run).


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

[gab, 2012-11-21 16:10:09]

@Nico, as per your request on https://codereview.chromium.org/11420045/,
removing OS-specific section, but doing it in a separate CL (i.e. this one).

Thanks,
Gab

[Nico, 2012-11-21 18:38:22]

Thanks!

https://codereview.chromium.org/11413121/diff/3001/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/11413121/diff/3001/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:1200: first_run::DoFirstRunTasks();
Could this conceivably run after AutoImport() below? If so, we (I) could move
the posix bits 8 lines further down into this new function too.


Also, what do you think about making this a method on BrowserMainParts instead?
That's where many of these functions live.

[gab, 2012-11-21 19:39:13]

Reply below.

https://codereview.chromium.org/11413121/diff/3001/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/11413121/diff/3001/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:1200: first_run::DoFirstRunTasks();
On 2012/11/21 18:38:22, Nico wrote:
> Could this conceivably run after AutoImport() below? If so, we (I) could move
> the posix bits 8 lines further down into this new function too.
> 

Maybe, preferably it's not inside the !|first_run_ui_bypass| block though; I
also think it's preferable if the tasks are launched before being blocked on
potential AutoImport UI...

> 
> Also, what do you think about making this a method on BrowserMainParts
instead?
> That's where many of these functions live.

I debated that, I think that to make it look nice in parts we would need to add
something like:
NonInteractiveFirstRunInit() --> (or PreInteractiveFirstRunInit() to keep naming
consistent?)
InteractiveFirstRunInit()
PostInteractiveFirstRunInit()
to ChromeBrowserMainParts.

[Nico, 2012-11-21 20:55:35]

https://codereview.chromium.org/11413121/diff/3001/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/11413121/diff/3001/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:1200: first_run::DoFirstRunTasks();
On 2012/11/21 19:39:14, gab wrote:
> On 2012/11/21 18:38:22, Nico wrote:
> > Could this conceivably run after AutoImport() below? If so, we (I) could
move
> > the posix bits 8 lines further down into this new function too.
> > 
> 
> Maybe, preferably it's not inside the !|first_run_ui_bypass| block though; I
> also think it's preferable if the tasks are launched before being blocked on
> potential AutoImport UI...

first_run_ui_bypass is only set by --no-first-run, which is only used for
testing as far as I know. But ok.

The posix code can't move up, because it reads the file that's potentially
written by AutoImport(). Not wanting to move windows down makes sense though.

> 
> > 
> > Also, what do you think about making this a method on BrowserMainParts
> instead?
> > That's where many of these functions live.
> 
> I debated that, I think that to make it look nice in parts we would need to
add
> something like:
> NonInteractiveFirstRunInit() --> (or PreInteractiveFirstRunInit() to keep
naming
> consistent?)
> InteractiveFirstRunInit()
> PostInteractiveFirstRunInit()
> to ChromeBrowserMainParts.

We could omit the middle one, but this organization would allow moving the posix
code somewhere else too. I think that's slightly nicer?

[gab, 2012-11-22 01:46:42]

Alright, so I looked into ChromeBrowserMainParts, correct me if I'm wrong, but
this is how I think it works (taking the example of PreProfileInit()):

Platform-specific main part gets the init call e.g.
ChromeBrowserMainPartsWin::PreMainMessageLoopRun()

It calls the generic ChromeBrowserMainParts::PreMainMessageLoopRun()

which calls ChromeBrowserMainParts::PreMainMessageLoopRunImpl() -- before
calling PreMainMessageLoopRun() for the extra parts

ChromeBrowserMainParts::PreMainMessageLoopRunImpl() eventually calls
ChromeBrowserMainParts::PreProfileInit() which calls PreProfileInit() for the
extra parts registered by ChromeContentBrowserClient::CreateBrowserMainParts().

----------------------------------

My question is then how is something like
ChromeBrowserMainPartsLinux::PreProfileInit() called (as it is not an "extra
part", but the "main part" itself).

I'm on Windows so I can't easily debug into this to find out and there isn't
currently such a part on Windows. Ideally I feel like the extra parts are meant
for toolkit things (i.e. gtk, views, ash, aura, etc.) where as the normal parts
are used to split platform-specific initialization; I only need to hook in
normal parts imo, but it's unclear to me how this is supposed to work.

From my pov, ChromeBrowserMainParts::PreProfileInit() should have a way of
calling back into it's own part, but at a level higher (i.e. back in the
OS-specific call that invoked ChromeBrowserMainParts::PreMainMessageLoopRun()
originally)...

Is the "main part" somehow also an "extra part", if so I don't see where this is
happening... (especially given ChromeBrowserMainExtraParts is not a subclass of
ChromeBrowserMainParts).

Thanks for helping me make sense of this!

Cheers,
Gab

[gab, 2012-11-22 19:31:45]

My bad, somehow hadn't realized the call to PreProfileInit() in
ChromeBrowserMainParts would get virtual dispatched to it's
ChromeBrowserMainPartsOS counterpart by nature of the subclassing.

CL upcoming.

Thanks,
Gab

On 2012/11/22 01:46:42, gab wrote:
> Alright, so I looked into ChromeBrowserMainParts, correct me if I'm wrong, but
> this is how I think it works (taking the example of PreProfileInit()):
> 
> Platform-specific main part gets the init call e.g.
> ChromeBrowserMainPartsWin::PreMainMessageLoopRun()
> 
> It calls the generic ChromeBrowserMainParts::PreMainMessageLoopRun()
> 
> which calls ChromeBrowserMainParts::PreMainMessageLoopRunImpl() -- before
> calling PreMainMessageLoopRun() for the extra parts
> 
> ChromeBrowserMainParts::PreMainMessageLoopRunImpl() eventually calls
> ChromeBrowserMainParts::PreProfileInit() which calls PreProfileInit() for the
> extra parts registered by
ChromeContentBrowserClient::CreateBrowserMainParts().
> 
> ----------------------------------
> 
> My question is then how is something like
> ChromeBrowserMainPartsLinux::PreProfileInit() called (as it is not an "extra
> part", but the "main part" itself).
> 
> I'm on Windows so I can't easily debug into this to find out and there isn't
> currently such a part on Windows. Ideally I feel like the extra parts are
meant
> for toolkit things (i.e. gtk, views, ash, aura, etc.) where as the normal
parts
> are used to split platform-specific initialization; I only need to hook in
> normal parts imo, but it's unclear to me how this is supposed to work.
> 
> From my pov, ChromeBrowserMainParts::PreProfileInit() should have a way of
> calling back into it's own part, but at a level higher (i.e. back in the
> OS-specific call that invoked ChromeBrowserMainParts::PreMainMessageLoopRun()
> originally)...
> 
> Is the "main part" somehow also an "extra part", if so I don't see where this
is
> happening... (especially given ChromeBrowserMainExtraParts is not a subclass
of
> ChromeBrowserMainParts).
> 
> Thanks for helping me make sense of this!
> 
> Cheers,
> Gab

[gab, 2012-11-28 22:42:02]

Added first run entries to ChromeBrowserMainParts; and also to extra parts to
stay in sync with what is done for other ChromeBrowserMain specific parts.

Not sure where to put the PostInteractiveFirstRunInit() call though (see comment
below).

Cheers,
Gab

https://codereview.chromium.org/11413121/diff/3001/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/11413121/diff/3001/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:1200: first_run::DoFirstRunTasks();
On 2012/11/21 20:55:35, Nico wrote:
> On 2012/11/21 19:39:14, gab wrote:
> > On 2012/11/21 18:38:22, Nico wrote:
> > > Could this conceivably run after AutoImport() below? If so, we (I) could
> move
> > > the posix bits 8 lines further down into this new function too.
> > > 
> > 
> > Maybe, preferably it's not inside the !|first_run_ui_bypass| block though; I
> > also think it's preferable if the tasks are launched before being blocked on
> > potential AutoImport UI...
> 
> first_run_ui_bypass is only set by --no-first-run, which is only used for
> testing as far as I know. But ok.
> 
> The posix code can't move up, because it reads the file that's potentially
> written by AutoImport(). Not wanting to move windows down makes sense though.
> 
> > 
> > > 
> > > Also, what do you think about making this a method on BrowserMainParts
> > instead?
> > > That's where many of these functions live.
> > 
> > I debated that, I think that to make it look nice in parts we would need to
> add
> > something like:
> > NonInteractiveFirstRunInit() --> (or PreInteractiveFirstRunInit() to keep
> naming
> > consistent?)
> > InteractiveFirstRunInit()
> > PostInteractiveFirstRunInit()
> > to ChromeBrowserMainParts.
> 
> We could omit the middle one, but this organization would allow moving the
posix
> code somewhere else too. I think that's slightly nicer?

Done.

https://codereview.chromium.org/11413121/diff/1005/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/11413121/diff/1005/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:1227: if
(GoogleUpdateSettings::GetCollectStatsConsent())
If this can go outside of first_run_ui_bypass (and ideally after line 1440 (just
before exiting "if (is_first_run)); the PostInteractiveFirstRunInit() could go
on line 1241.

[Nico, 2012-11-29 01:38:28]

LGTM, thanks!

I think the posix post-import logic can go outside of skipfirstrun. I'll look at
that once this is in.

https://codereview.chromium.org/11413121/diff/1005/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/11413121/diff/1005/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:950: //   ... additional setup, including
first_run::AutoImport()
The "…including…" part is a bit confusing to me. It sounds like
PreInteractiveFirstRunInit() is responsible for calling it.

[gab, 2012-11-29 18:32:19]

Thanks, fixed comment and added a TODO for you to move the POSIX-specific code
out of there.

Gab

https://codereview.chromium.org/11413121/diff/1005/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/11413121/diff/1005/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:950: //   ... additional setup, including
first_run::AutoImport()
On 2012/11/29 01:38:28, Nico wrote:
> The "…including…" part is a bit confusing to me. It sounds like
> PreInteractiveFirstRunInit() is responsible for calling it.

Done.

[commit-bot: I haz the power, 2012-11-29 18:33:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/11413121/11001

[commit-bot: I haz the power, 2012-11-29 23:35:19]

Retried try job too often on linux_rel for step(s) content_unittests

[commit-bot: I haz the power, 2012-11-29 23:41:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/11413121/11001

[commit-bot: I haz the power, 2012-11-30 02:06:22]

Change committed as 170354