Don't open Chrome browser window on first login of a new user. Because it will hide Getting Started…

Don't open Chrome browser window on first login of a new user. Because it will hind Getting Started Guide window.

BUG=chromium:239755

[Dmitry Polukhin, 2013-05-15 06:57:22]

LGTM

[cylee1, 2013-05-15 07:21:39]

Hi Scott,
  We intended to show the Getting Started Guide App on first launch of a new
user. So suspend the default Chrome tab (it will hind the app window). The
change is CrOS only.
  plz refer to https://code.google.com/p/chromium/issues/detail?id=239755 for
details, or mlchan@ as our chief PM.
  OWNER approve needed.

[Finnur, 2013-05-15 10:24:32]

Just a few comments while looking at this.

But before we get to that I thought I'd answer a the overall question you posed
in email: I'm not sure what side-effects this has. I'd suggest having an OWNER
take a closer look (sky/beng/pkasting). It looks like you've added Scott
already, so he's probably a good candidate.

https://codereview.chromium.org/15163003/diff/1/chrome/browser/ui/startup/sta...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/15163003/diff/1/chrome/browser/ui/startup/sta...
chrome/browser/ui/startup/startup_browser_creator.cc:257: // Not to open default
Chrome window for the first login of a new
nit: Not to open -> Don't open

https://codereview.chromium.org/15163003/diff/1/chrome/browser/ui/startup/sta...
chrome/browser/ui/startup/startup_browser_creator.cc:261: // Note(cylee):
Ideally this check should be put in ProcessCmdLineImpl()
style: Note(cylee) is not something we use. Use either:

Note: Ideally...
or
TODO(cylee): Find a way to put this check in ProcessCmdLineImpl()...

It sounds like you want the former.

https://codereview.chromium.org/15163003/diff/1/chrome/browser/ui/startup/sta...
chrome/browser/ui/startup/startup_browser_creator.cc:262: // before calling this
function. However in chromeos/login/login_utils.cc
nit: suggest rewording: "However, chromeos/login/login_utils.cc calls this
function directly..."

https://codereview.chromium.org/15163003/diff/1/chrome/browser/ui/startup/sta...
chrome/browser/ui/startup/startup_browser_creator.cc:267: }
style: No braces around single line if clauses.

https://codereview.chromium.org/15163003/diff/1/chrome/browser/ui/startup/sta...
chrome/browser/ui/startup/startup_browser_creator.cc:269: 
nit: Also, suggest changing 'hind'->'hide' in the CL description

[cylee1, 2013-05-15 13:39:49]

https://codereview.chromium.org/15163003/diff/1/chrome/browser/ui/startup/sta...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/15163003/diff/1/chrome/browser/ui/startup/sta...
chrome/browser/ui/startup/startup_browser_creator.cc:257: // Not to open default
Chrome window for the first login of a new
On 2013/05/15 10:24:33, Finnur wrote:
> nit: Not to open -> Don't open

Done.

https://codereview.chromium.org/15163003/diff/1/chrome/browser/ui/startup/sta...
chrome/browser/ui/startup/startup_browser_creator.cc:261: // Note(cylee):
Ideally this check should be put in ProcessCmdLineImpl()
On 2013/05/15 10:24:33, Finnur wrote:
> style: Note(cylee) is not something we use. Use either:
> 
> Note: Ideally...
> or
> TODO(cylee): Find a way to put this check in ProcessCmdLineImpl()...
> 
> It sounds like you want the former.

Done.

https://codereview.chromium.org/15163003/diff/1/chrome/browser/ui/startup/sta...
chrome/browser/ui/startup/startup_browser_creator.cc:262: // before calling this
function. However in chromeos/login/login_utils.cc
On 2013/05/15 10:24:33, Finnur wrote:
> nit: suggest rewording: "However, chromeos/login/login_utils.cc calls this
> function directly..."

Done.

https://codereview.chromium.org/15163003/diff/1/chrome/browser/ui/startup/sta...
chrome/browser/ui/startup/startup_browser_creator.cc:267: }
On 2013/05/15 10:24:33, Finnur wrote:
> style: No braces around single line if clauses.

Done.

https://codereview.chromium.org/15163003/diff/1/chrome/browser/ui/startup/sta...
chrome/browser/ui/startup/startup_browser_creator.cc:269: 
On 2013/05/15 10:24:33, Finnur wrote:
> nit: Also, suggest changing 'hind'->'hide' in the CL description

Done.

[sky, 2013-05-15 15:21:55]

Also, have you trun browser_tests to make sure they aren't effected?

https://codereview.chromium.org/15163003/diff/7001/chrome/browser/ui/startup/...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/15163003/diff/7001/chrome/browser/ui/startup/...
chrome/browser/ui/startup/startup_browser_creator.cc:600: }
Can you do your check here instead and set silent_launch to true?

[cylee1, 2013-05-15 18:10:44]

I haven't run try bot yet. Will do.

https://codereview.chromium.org/15163003/diff/7001/chrome/browser/ui/startup/...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/15163003/diff/7001/chrome/browser/ui/startup/...
chrome/browser/ui/startup/startup_browser_creator.cc:600: }
On 2013/05/15 15:21:55, sky wrote:
> Can you do your check here instead and set silent_launch to true?

That was my first try. But somewhere in  chromeos/login/login_utils.cc calls
StartupBrowserCreator::LaunchBrowser() directly instead of this function.

[sky, 2013-05-15 21:14:47]

On Wed, May 15, 2013 at 11:10 AM,  <cylee@chromium.org> wrote:
> I haven't run try bot yet. Will do.
>
>
>
>
https://codereview.chromium.org/15163003/diff/7001/chrome/browser/ui/startup/...
> File chrome/browser/ui/startup/startup_browser_creator.cc (right):
>
>
https://codereview.chromium.org/15163003/diff/7001/chrome/browser/ui/startup/...
> chrome/browser/ui/startup/startup_browser_creator.cc:600: }
> On 2013/05/15 15:21:55, sky wrote:
>>
>> Can you do your check here instead and set silent_launch to true?
>
>
> That was my first try. But somewhere in  chromeos/login/login_utils.cc
> calls StartupBrowserCreator::LaunchBrowser() directly instead of this
> function.
>
> https://codereview.chromium.org/15163003/

Why?

[cylee1, 2013-05-16 11:47:37]

> > That was my first try. But somewhere in  chromeos/login/login_utils.cc
> > calls StartupBrowserCreator::LaunchBrowser() directly instead of this
> > function.
> >
> > https://codereview.chromium.org/15163003/
> 
> Why?

Sorry are you asking about the code or about the browser test?
About the code: somewhere in chromeos/login/login_utils.cc

  // TODO(pastarmovj): Restart the browser and apply any flags set by the user.
  // See: http://crosbug.com/39249
  
  browser_creator.LaunchBrowser(*CommandLine::ForCurrentProcess(),
                                profile,
                                base::FilePath(),
                                chrome::startup::IS_PROCESS_STARTUP,
                                first_run,
                                &return_code);

About the test:
  I've tried to run try bot but all returns -1 without detail links.
  My change applies is supposed to apply to ChromeOS only, but all jobs fails. I
haven't figure out what's going on here.

[cylee1, 2013-05-16 12:49:25]

The implementation is not ideal since it'll break many test.
Please ignore the CL from now.

I'll Submit another CL and close this one, because this one is submitted by
@chromium.org and doesn't have permission to run try bot.
I'll start a new one with @google.com account.

[sky, 2013-05-16 15:47:45]

On Thu, May 16, 2013 at 4:47 AM,  <cylee@chromium.org> wrote:
>> > That was my first try. But somewhere in  chromeos/login/login_utils.cc
>> > calls StartupBrowserCreator::LaunchBrowser() directly instead of this
>> > function.
>> >
>> > https://codereview.chromium.org/15163003/
>
>
>> Why?
>
>
> Sorry are you asking about the code or about the browser test?
> About the code: somewhere in chromeos/login/login_utils.cc
>
>   // TODO(pastarmovj): Restart the browser and apply any flags set by the
> user.
>   // See: http://crosbug.com/39249

Sorry for not being clear. Yes, that is what I was asking about. Can
you really hit this code path for a new user?

  -Scott

>   browser_creator.LaunchBrowser(*CommandLine::ForCurrentProcess(),
>                                 profile,
>                                 base::FilePath(),
>                                 chrome::startup::IS_PROCESS_STARTUP,
>                                 first_run,
>                                 &return_code);
>
> About the test:
>   I've tried to run try bot but all returns -1 without detail links.
>   My change applies is supposed to apply to ChromeOS only, but all jobs
> fails. I
> haven't figure out what's going on here.
>
> https://codereview.chromium.org/15163003/

[cylee, 2013-05-16 16:14:33]

> >   // TODO(pastarmovj): Restart the browser and apply any flags set by the
> > user.
> >   // See: http://crosbug.com/39249
> 
> Sorry for not being clear. Yes, that is what I was asking about. Can
> you really hit this code path for a new user?
> 
>   -Scott
Yes I've deployed a chrome to my pixel and it works.
The normal path calls StartupBrowserCreator::Start() (from
chrome_browser_main.cc), then it checks all flags and silent_launch, then in
turns call  StartupBrowserCreator::LaunchBrowser(). 
However as the comments in chromeos/login/login_utils.cc says, it forcibly
restarts the browser by calling LaunchBrowser(). So some logic in
StartupBrowserCreator::Start()  is skipped.