Ensure we have a valid profile when using BrowserInit::LaunchWithProfile::Ope...

Ensure we have a valid profile when using BrowserInit::LaunchWithProfile::OpenURLsInBrowser() directly and the browser passed in is a popup window.
BUG=17520
TEST=see bug.

[brettw, 2009-07-29 17:17:01]

http://codereview.chromium.org/160331/diff/1/2
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/160331/diff/1/2#newcode563
Line 563: profile_ = browser->profile();
I'm thinking of putting this below the current
    browser = Browser::Create(profile_);
as
  else if (!profile_)
    profile_ = browser->profile();
since then you won't have the case where you don't actually use browser and you
can remove that part of your comment.

[brettw, 2009-07-29 17:48:21]

http://codereview.chromium.org/160331/diff/1/2
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/160331/diff/1/2#newcode563
Line 563: profile_ = browser->profile();
Okay, I was wrong, can you put a blank line after this?

[Avi (use Gerrit), 2009-08-05 19:20:01]

brettw—

Did you LGTM this? Right now browser tests are broken in general, so I want to
get the crash fix in and deal with the tests later.

[brettw, 2009-08-07 16:38:24]

I was waiting for the test, which it looks like pink added and forgot to send
email maybe? I'll look soon.

[brettw, 2009-08-07 16:39:41]

LGTM

http://codereview.chromium.org/160331/diff/1004/1006
File chrome/browser/browser_init_browsertest.cc (right):

http://codereview.chromium.org/160331/diff/1004/1006#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
s/2006-2008/2009

http://codereview.chromium.org/160331/diff/1004/1006#newcode15
Line 15: protected:
I would just delete this "protected" line.

[Avi (use Gerrit), 2009-08-07 17:33:42]

On 2009/08/07 16:38:24, brettw wrote:
> I was waiting for the test, which it looks like pink added and forgot to send
> email maybe? I'll look soon.

He added the test. He had issues with it running, but right now all of
browser_tests doesn't work on the Mac (http://crbug.com/16322). For now I want
to get the crash fix in, and we can debug the browser_test problems later (the
bug has already been pushed to M4).

[Avi (use Gerrit), 2009-08-10 19:15:29]

BTW, this seems to break the UI test SecondLaunch. Working on it.

[Avi (use Gerrit), 2009-08-10 21:16:01]

Brett—

This is a problem. In SecondLaunch, when the second launch ends up calling
OpenURLsInBrowser, we can't tell what's going on. The only solution to this
seems to be just always open a new browser rather than trying to find an
existing "normal" browser. This would keep the initialization of the profile and
stop the crash, though.

The unit test is confusing me. Pink calls window->Activate but
BrowserList::GetLastActive keeps returning null. Any thoughts?

[Avi (use Gerrit), 2009-08-10 21:28:25]

On 2009/08/10 21:16:01, Avi wrote:
> BrowserList::GetLastActive keeps returning null.

... and that's because that's updated by ActivationChanged, which only happens
with an NCActivate message.

Apparently that doesn't happen during our browser tests. And without that
updating, this entire unit test is kinda useless.

Should I put in the crash fix without a working test?

[brettw, 2009-08-10 23:21:47]

On 2009/08/10 21:28:25, Avi wrote:
> On 2009/08/10 21:16:01, Avi wrote:
> > BrowserList::GetLastActive keeps returning null.
> 
> ... and that's because that's updated by ActivationChanged, which only happens
> with an NCActivate message.
> 
> Apparently that doesn't happen during our browser tests. And without that
> updating, this entire unit test is kinda useless.
> 
> Should I put in the crash fix without a working test?

We should make sure that function gets called so the test works. You can do
whatever necessary manually in the test to make this get called (like calling
BrowserView::ActivationChanged).

Brett

[Avi (use Gerrit), 2009-08-11 16:19:39]

On 2009/08/10 23:21:47, brettw wrote:
> You can do
> whatever necessary manually in the test to make this get called (like calling
> BrowserView::ActivationChanged).

That's difficult, as it involves manually walking the view systems, making lots
of nasty casts, and hoping nothing changes. Working through the details; will
let you know how it goes.

[Avi (use Gerrit), 2009-08-11 16:21:22]

I'm not sure what the point is. Why go to a lot of trouble to explicitly call a
function that you know will turn around and set the state that you're going to
check on the next line? If I can't say "please activate this window" in some
sane way, is this a useful test?

[brettw, 2009-08-11 16:23:39]

This is not a test of activation. This is a test of launching a new browser
window when the wrong type has activation.

[Avi (use Gerrit), 2009-08-11 16:36:08]

On 2009/08/11 16:23:39, brettw wrote:
> This is not a test of activation. This is a test of launching a new browser
> window when the wrong type has activation.

More precisely, this is test of launching a new browser when the wrong type was
specified as the target. Activation actually has nothing to do with this. I'm
taking a different approach to the test; I'll send you the CL.

[brettw, 2009-08-11 16:38:48]

I thought it got the target from the currently active one. Another approach
would be fine, though.

[Avi (use Gerrit), 2009-08-11 16:43:17]

On 2009/08/11 16:38:48, brettw wrote:
> I thought it got the target from the currently active one. Another approach
> would be fine, though.

The crash scenario was:
- real code gets active browser (which happens to be a popup)
- real code passes that browser to OpenURLsInBrowser
- boom

The unit test is:
- unit test creates popup browser
- unit test passes that browser to OpenURLsInBrowser
- see if boom happens or not

It isn't that we're using the active browser, it's ensuring that whatever
browser is passed into OpenURLsInBrowser, no crashes happen.

[Avi (use Gerrit), 2009-08-11 20:33:47]

Adopting; see http://codereview.chromium.org/164341