Rework the way the FindBrowserWithProfile/Type methods work....

Rework the way the FindBrowserWithProfile/Type methods work.

We now always walk the last active list backwards rather than consulting the last active then walking the registered browser list forwards. I now also maintain a fallback to walk the entire registered list of browsers forward if the active scan fails. This is likely only in a testing environment where a Browser may never have been activated.

This ensures that when the last active browser is a popup or app frame the last active TYPE_NORMAL browser is located when opening a new tab.

http://crbug.com/17498
TEST=Open an app frame. Open a browser window (Ctrl+N) and load a page. Minimize it. Open another browser window and minimize it. Activate the app frame. Press Ctrl+T. The second browser window should be restored and have a new tab added to it rather than the first.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30531

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30659

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30662

[sky, 2009-10-26 22:02:56]

LGTM.

http://codereview.chromium.org/330013/diff/2001/3002
File chrome/browser/browser_list.cc (right):

http://codereview.chromium.org/330013/diff/2001/3002#newcode88
Line 88: if (type == Browser::TYPE_ANY || browser->type() == type) {
nit: combine this into a single if and just return it's value.

[Ben Goodger (Google), 2009-10-27 16:34:48]

Is there a reason we couldn't hoist the enumeration out into a
browser_constants.h file or something like that so we don't have to carry
duplicates?

-Ben

On Tue, Oct 27, 2009 at 8:32 AM, Scott Violet <sky@chromium.org> wrote:

> I just landed a change that copies Browser::Types into BrowserProxy.
> Before you land this change be sure and update BrowserProxy to match
> the new values otherwise some tests are going to fail.
>
>  -Scott
>
> On Mon, Oct 26, 2009 at 3:02 PM,  <sky@chromium.org> wrote:
> > LGTM.
> >
> >
> >
> > http://codereview.chromium.org/330013/diff/2001/3002
> > File chrome/browser/browser_list.cc (right):
> >
> > http://codereview.chromium.org/330013/diff/2001/3002#newcode88
> > Line 88: if (type == Browser::TYPE_ANY || browser->type() == type) {
> > nit: combine this into a single if and just return it's value.
> >
> > http://codereview.chromium.org/330013
> >
>

[sky, 2009-10-27 16:57:43]

I had to revert my change.

Good question. Let me figure out why my change broke the world, land
that, then figure out how to share a file between the two builds.

  -Scott

On Tue, Oct 27, 2009 at 9:34 AM, Ben Goodger (Google) <ben@chromium.org> wr=
ote:
> Is there a reason we couldn't hoist the enumeration out into a
> browser_constants.h file or something like that so we don't have to carry
> duplicates?
> -Ben
>
> On Tue, Oct 27, 2009 at 8:32 AM, Scott Violet <sky@chromium.org> wrote:
>>
>> I just landed a change that copies Browser::Types into BrowserProxy.
>> Before you land this change be sure and update BrowserProxy to match
>> the new values otherwise some tests are going to fail.
>>
>> =A0-Scott
>>
>> On Mon, Oct 26, 2009 at 3:02 PM, =A0<sky@chromium.org> wrote:
>> > LGTM.
>> >
>> >
>> >
>> > http://codereview.chromium.org/330013/diff/2001/3002
>> > File chrome/browser/browser_list.cc (right):
>> >
>> > http://codereview.chromium.org/330013/diff/2001/3002#newcode88
>> > Line 88: if (type =3D=3D Browser::TYPE_ANY || browser->type() =3D=3D t=
ype) {
>> > nit: combine this into a single if and just return it's value.
>> >
>> > http://codereview.chromium.org/330013
>> >
>
>

[Ben Goodger (Google), 2009-10-31 05:15:35]

Hey Scott,

I made some changes here to get this to pass the browser_tests. It turned out
that the extensions browser_tests were relying on the window to have been
activated, however when run in the test environment it seems like no windows
ever get activated, so I added a fallback to the code: if no matching browser is
found in the last active list, a forward walk of all registered browsers is
performed.

[sky, 2009-11-02 15:49:03]

OK

Seems like it would be worth adding a comment as to why you fallback to a
forward search.