Fixing opening a new tab if none exists to show the enrollment URI for certificates.

Fixing opening a new tab if none exists to show the enrollment URI for certificates.

Replacing FindTabbedBrowser by FindOrCreateTabbedBrowser and Navigate to open the tab.

Using Restore() to actually make the browser window visible. Otherwise it will remain minimized.

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

[pneubeck (no reviews), 2013-01-30 19:15:08]

Hi Scott,

could you please take a look. I wonder why the Restore() is required.
The code is identical to the one in the tray menu which opens the settings page
except for the Restore():

https://cs.corp.google.com/#chrome/src/chrome/browser/chromeos/status/network...

https://cs.corp.google.com/#chrome/src/chrome/browser/ui/chrome_pages.cc&l=13...

The only difference I see is that the dialog is still open while the browser
window is created.
Btw. immediately before the Restore(), IsMinimized() is false!

[sky, 2013-01-30 22:46:27]

https://codereview.chromium.org/12089026/diff/2002/chrome/browser/chromeos/en...
File chrome/browser/chromeos/enrollment_dialog_view.cc (left):

https://codereview.chromium.org/12089026/diff/2002/chrome/browser/chromeos/en...
chrome/browser/chromeos/enrollment_dialog_view.cc:114: // Couldn't find a tabbed
browser: create one.
This does not show the browser.

https://codereview.chromium.org/12089026/diff/2002/chrome/browser/chromeos/en...
chrome/browser/chromeos/enrollment_dialog_view.cc:121:
chrome::AddSelectedTabWithURL(browser, target_uri_,
This does not show browser. So if you created a new browser (or you found a
browser in another workspace) it won't be shown. The bug is here, you need to
show and activate the browser.

[pneubeck (no reviews), 2013-02-19 20:30:11]

Hi Scott,

as far as I could find out, the new browser window remains minimized if a modal
dialog is still open.
That's why in the new patch I postponed the Navigate call until the dialog is
closed.

[sky, 2013-02-19 22:43:51]

We don't create windows minimized, so how does it end up minimized in the first
place?

You should be able to replicate creating a window while a modal dialog is
showing in a unit test (not using the enrollment dialog).

[pneubeck (no reviews), 2013-02-25 16:29:19]

I had hoped to fix this for 26. Since I'm not familiar with the window code, I'm
unsure that I will find the root cause to this bug or that I can set up a
reproducing unit test in time.

Can we commit this for now and I continue searching for the problem
independently?

On 2013/02/19 22:43:51, sky wrote:
> We don't create windows minimized, so how does it end up minimized in the
first
> place?
> 
> You should be able to replicate creating a window while a modal dialog is
> showing in a unit test (not using the enrollment dialog).

[sky, 2013-02-25 16:48:51]

Unfortunately I'm swamped and won't have time to help out. Ask James
Cook if he can find someone that can help you with the right fix.

  -Scott

On Mon, Feb 25, 2013 at 8:29 AM,  <pneubeck@chromium.org> wrote:
> I had hoped to fix this for 26. Since I'm not familiar with the window code,
> I'm
> unsure that I will find the root cause to this bug or that I can set up a
> reproducing unit test in time.
>
> Can we commit this for now and I continue searching for the problem
> independently?
>
>
> On 2013/02/19 22:43:51, sky wrote:
>>
>> We don't create windows minimized, so how does it end up minimized in the
>
> first
>>
>> place?
>
>
>> You should be able to replicate creating a window while a modal dialog is
>> showing in a unit test (not using the enrollment dialog).
>
>
>
>
> https://codereview.chromium.org/12089026/

[Greg Spencer (Chromium), 2013-03-15 21:05:00]

On 2013/02/25 16:48:51, sky wrote:
> Unfortunately I'm swamped and won't have time to help out. Ask James
> Cook if he can find someone that can help you with the right fix.

I'm going to switch reviewers to Steven, since he has some more time.

I'd also suggest that we actually commit this workaround, since it's a
reasonable workaround, and at this point there's no way that a fix that impacts
the window code would be allowed to be merged to R26.  This is a local enough
fix that it may make it in, and we can make the  "real" fix in another CL once
we find that flaw and fix it.  I'll file a separate issue for that bug.

[Greg Spencer (Chromium), 2013-03-15 21:05:30]

Steven, can you review this for Phillipp?

[Greg Spencer (Chromium), 2013-03-15 21:14:12]

Filed bug https://crbug.com/196697 to track the larger issue.

[stevenjb, 2013-03-15 21:15:01]

lgtm

[commit-bot: I haz the power, 2013-03-15 21:41:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/12089026/14001

[sky, 2013-03-15 23:09:36]

Why are we working around the bug here and not trying to find the real fix?

[Greg Spencer (Chromium), 2013-03-15 23:16:30]

On 2013/03/15 23:09:36, sky wrote:
> Why are we working around the bug here and not trying to find the real fix?

We are trying to find it: that's why I submitted the new issue to track the real
bug.  We're also submitting this workaround to it because even if we had a real
fix ready, we wouldn't be able to merge that to R26 because it would be far too
risky.

[stevenjb, 2013-03-15 23:19:57]

On 2013/03/15 23:09:36, sky wrote:
> Why are we working around the bug here and not trying to find the real fix?

I didn't follow the entire history of this review, but patch set 2 seemed like
an OK fix to me. Waiting until the modal dialog is closed before navigating to a
new Browser tab seems reasonable (i.e. not a terrible hack). What was the
primary concern with this patch? I agree that fixing whatever edge case was
getting triggered would be better, but my understanding was that we were trying
to move away from system modal dialogs in the future?

[sky, 2013-03-15 23:27:11]

Patchset 2 is a workaround. Working around bugs leads to a messy code base
and unmaintainable code.

As to merging to 26, how can we say the real fix is going to be any more
risky until we understand the fix? This patch started ~6 weeks ago, why
have we not tried to fix the core issue?


On Fri, Mar 15, 2013 at 4:19 PM, <stevenjb@chromium.org> wrote:

> On 2013/03/15 23:09:36, sky wrote:
>
>> Why are we working around the bug here and not trying to find the real
>> fix?
>>
>
> I didn't follow the entire history of this review, but patch set 2 seemed
> like
> an OK fix to me. Waiting until the modal dialog is closed before
> navigating to a
> new Browser tab seems reasonable (i.e. not a terrible hack). What was the
> primary concern with this patch? I agree that fixing whatever edge case was
> getting triggered would be better, but my understanding was that we were
> trying
> to move away from system modal dialogs in the future?
>
>
>
>
https://chromiumcodereview.**appspot.com/12089026/<https://chromiumcodereview...
>

[Greg Spencer (Chromium), 2013-03-15 23:38:18]

On 2013/03/15 23:27:11, sky wrote:
> Patchset 2 is a workaround. Working around bugs leads to a messy code base
> and unmaintainable code.

Agreed, which is why we will also fix the core issue.  As workarounds go, this
isn't a hack: I can see writing the code in this fashion in the first place.  If
it were a hack, I'd say we just weren't going to be able to fix this in 26 and
wait for a better fix.

> As to merging to 26, how can we say the real fix is going to be any more
> risky until we understand the fix? This patch started ~6 weeks ago, why
> have we not tried to fix the core issue?

Because if you were accepting fixes to be merged to 26, would you take the one
that affects only one small feature, or the one that could potentially affect
all the windows?  It doesn't matter how "safe" the real fix is, it's a matter of
potential exposure that would make it too risky.

As to why it hasn't been fixed yet, it's purely a matter of the right people not
having the time to look at it in depth.  That's why I submitted the bug, so we
won't drop it.  Would you be more comfortable if I marked the bug as
ReleaseBlock-Dev for R27?

[sky, 2013-03-15 23:44:15]

On Fri, Mar 15, 2013 at 4:38 PM, <gspencer@chromium.org> wrote:

> On 2013/03/15 23:27:11, sky wrote:
>
>> Patchset 2 is a workaround. Working around bugs leads to a messy code base
>> and unmaintainable code.
>>
>
> Agreed, which is why we will also fix the core issue.  As workarounds go,
> this
> isn't a hack: I can see writing the code in this fashion in the first
> place.  If
> it were a hack, I'd say we just weren't going to be able to fix this in 26
> and
> wait for a better fix.
>
>
>  As to merging to 26, how can we say the real fix is going to be any more
>> risky until we understand the fix? This patch started ~6 weeks ago, why
>> have we not tried to fix the core issue?
>>
>
> Because if you were accepting fixes to be merged to 26, would you take the
> one
> that affects only one small feature, or the one that could potentially
> affect
> all the windows?  It doesn't matter how "safe" the real fix is, it's a
> matter of
> potential exposure that would make it too risky.
>

I could only make that call once the real fix is understood. Who is to say
whether the real fix isn't a one liner.

  -Scott


>
> As to why it hasn't been fixed yet, it's purely a matter of the right
> people not
> having the time to look at it in depth.  That's why I submitted the bug,
> so we
> won't drop it.  Would you be more comfortable if I marked the bug as
> ReleaseBlock-Dev for R27?
>
>
https://chromiumcodereview.**appspot.com/12089026/<https://chromiumcodereview...
>

[sky, 2013-03-16 00:02:08]

Sorry if I seem cranky here and I don't mean to direct that at you Greg or
Steven. When I ask for an investigation into the real problem 6 weeks ago and
nothing happens, then all of sudden its super urgent I get upset. Especially
when I suspect the real fix is likely to be less code than is in this patch.

  -Scott

On 2013/03/15 23:44:15, sky wrote:
> On Fri, Mar 15, 2013 at 4:38 PM, <mailto:gspencer@chromium.org> wrote:
> 
> > On 2013/03/15 23:27:11, sky wrote:
> >
> >> Patchset 2 is a workaround. Working around bugs leads to a messy code base
> >> and unmaintainable code.
> >>
> >
> > Agreed, which is why we will also fix the core issue.  As workarounds go,
> > this
> > isn't a hack: I can see writing the code in this fashion in the first
> > place.  If
> > it were a hack, I'd say we just weren't going to be able to fix this in 26
> > and
> > wait for a better fix.
> >
> >
> >  As to merging to 26, how can we say the real fix is going to be any more
> >> risky until we understand the fix? This patch started ~6 weeks ago, why
> >> have we not tried to fix the core issue?
> >>
> >
> > Because if you were accepting fixes to be merged to 26, would you take the
> > one
> > that affects only one small feature, or the one that could potentially
> > affect
> > all the windows?  It doesn't matter how "safe" the real fix is, it's a
> > matter of
> > potential exposure that would make it too risky.
> >
> 
> I could only make that call once the real fix is understood. Who is to say
> whether the real fix isn't a one liner.
> 
>   -Scott
> 
> 
> >
> > As to why it hasn't been fixed yet, it's purely a matter of the right
> > people not
> > having the time to look at it in depth.  That's why I submitted the bug,
> > so we
> > won't drop it.  Would you be more comfortable if I marked the bug as
> > ReleaseBlock-Dev for R27?
> >
> >
>
https://chromiumcodereview.**appspot.com/12089026/%3Chttps://chromiumcoderevi...>
> >

[stevenjb, 2013-03-16 00:08:46]

FWIW, I am entirely sympathetic to the sentiment, and certainly didn't take
anything personally.

However, since I am unwilling to prioritize diving into this problem myself, and
am all too aware of the complexity of the code involved, I am sympathetic to the
situation and thus defending my approval of the patchset.

If I felt that the patch made the code less readable or maintainable then I
would make the time to help investigate, but honestly there are, imho, worse
quirks in the code, especially surrounding system modal dialogs which I hope we
can do away with entirely at some point.

[Greg Spencer (Chromium), 2013-03-16 00:25:15]

Yeah, I didn't take it personally either.  I understand why you're cranky, I
just think we can take this workaround without making the code worse, and
perhaps get it in a release earlier.  I'll make sure to push on that bug so we
get to the bottom of the real issue.

[commit-bot: I haz the power, 2013-03-16 09:25:30]

List of reviewers changed. sky@chromium.org did a drive-by without LGTM'ing!

[sky, 2013-03-18 16:10:48]

https://codereview.chromium.org/12089026/diff/14001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/enrollment_dialog_view.cc (right):

https://codereview.chromium.org/12089026/diff/14001/chrome/browser/chromeos/e...
chrome/browser/chromeos/enrollment_dialog_view.cc:128: }
This should set params.window_action to SHOW_WINDOW, otherwise if
FindOrCreateTabbedBrowser creates a new browser I'm not sure Navigate ends up
showing it.

[Greg Spencer (Chromium), 2013-03-19 17:33:07]

On 2013/03/18 16:10:48, sky wrote:
> This should set params.window_action to SHOW_WINDOW, otherwise if
> FindOrCreateTabbedBrowser creates a new browser I'm not sure Navigate ends up
> showing it.

Scott, are you saying that this is perhaps an alternate fix for this bug?

Phillipp, could you try this and see if it solves the problem?

[sky, 2013-03-19 21:18:22]

On Tue, Mar 19, 2013 at 10:33 AM, <gspencer@chromium.org> wrote:

> On 2013/03/18 16:10:48, sky wrote:
>
>> This should set params.window_action to SHOW_WINDOW, otherwise if
>> FindOrCreateTabbedBrowser creates a new browser I'm not sure Navigate
>> ends up
>> showing it.
>>
>
> Scott, are you saying that this is perhaps an alternate fix for this bug?
>

Looking at the bug report I see this is likely the cause of the bug.

  -Scott


> Phillipp, could you try this and see if it solves the problem?
>
>
https://codereview.chromium.**org/12089026/<https://codereview.chromium.org/1...
>

[sky, 2013-03-19 21:21:16]

Also, I think this means you can leave the code as it was (meaning don't
need to move to OnClose) and just just do a show on the browser after
adding the tab.


On Tue, Mar 19, 2013 at 2:18 PM, Scott Violet <sky@chromium.org> wrote:

>
>
>
> On Tue, Mar 19, 2013 at 10:33 AM, <gspencer@chromium.org> wrote:
>
>> On 2013/03/18 16:10:48, sky wrote:
>>
>>> This should set params.window_action to SHOW_WINDOW, otherwise if
>>> FindOrCreateTabbedBrowser creates a new browser I'm not sure Navigate
>>> ends up
>>> showing it.
>>>
>>
>> Scott, are you saying that this is perhaps an alternate fix for this bug?
>>
>
> Looking at the bug report I see this is likely the cause of the bug.
>
>   -Scott
>
>
>> Phillipp, could you try this and see if it solves the problem?
>>
>>
https://codereview.chromium.**org/12089026/<https://codereview.chromium.org/1...
>>
>
>

[pneubeck (no reviews), 2013-03-20 12:00:39]

No it doesn't. That is what we initially tested:

  chrome::NavigateParams params(browser,
                                GURL(target_uri_),
                                content::PAGE_TRANSITION_LINK);
  params.disposition = NEW_FOREGROUND_TAB;
  params.window_action = chrome::NavigateParams::SHOW_WINDOW;
  chrome::Navigate(&params);

  browser->window()->Show();

Doesn't work.
Only a Restore() call works.

I also found out, that the problem only occurs if the previously closed browser
was maximized. So my guess is that it's related to the browsers CreateParams
initial_bounds=SHOW_STATE_DEFAULT.

I can enable/disable the problem exactly by this parameter and the following
browser creation:

  Browser::CreateParams bparams = Browser::CreateParams(profile_,
                                                       
chrome::HOST_DESKTOP_TYPE_ASH);
  bparams.initial_show_state = ui::SHOW_STATE_DEFAULT;  // or STATE_NORMAL
  Browser* browser = new Browser(bparams);
  browser->window()->Show();

On 2013/03/19 21:21:16, sky wrote:
> Also, I think this means you can leave the code as it was (meaning don't
> need to move to OnClose) and just just do a show on the browser after
> adding the tab.
> 
> 
> On Tue, Mar 19, 2013 at 2:18 PM, Scott Violet <mailto:sky@chromium.org> wrote:
> 
> >
> >
> >
> > On Tue, Mar 19, 2013 at 10:33 AM, <mailto:gspencer@chromium.org> wrote:
> >
> >> On 2013/03/18 16:10:48, sky wrote:
> >>
> >>> This should set params.window_action to SHOW_WINDOW, otherwise if
> >>> FindOrCreateTabbedBrowser creates a new browser I'm not sure Navigate
> >>> ends up
> >>> showing it.
> >>>
> >>
> >> Scott, are you saying that this is perhaps an alternate fix for this bug?
> >>
> >
> > Looking at the bug report I see this is likely the cause of the bug.
> >
> >   -Scott
> >
> >
> >> Phillipp, could you try this and see if it solves the problem?
> >>
> >>
>
https://codereview.chromium.**org/12089026/%3Chttps://codereview.chromium.org...>
> >>
> >
> >

[sky, 2013-03-20 14:37:58]

What are the steps to reproduce this? Can you create a test for this?


On Wed, Mar 20, 2013 at 5:00 AM, <pneubeck@chromium.org> wrote:

> No it doesn't. That is what we initially tested:
>
>   chrome::NavigateParams params(browser,
>                                 GURL(target_uri_),
>                                 content::PAGE_TRANSITION_LINK)**;
>   params.disposition = NEW_FOREGROUND_TAB;
>   params.window_action = chrome::NavigateParams::SHOW_**WINDOW;
>   chrome::Navigate(&params);
>
>   browser->window()->Show();
>
> Doesn't work.
> Only a Restore() call works.
>
> I also found out, that the problem only occurs if the previously closed
> browser
> was maximized. So my guess is that it's related to the browsers
> CreateParams
> initial_bounds=SHOW_STATE_**DEFAULT.
>
> I can enable/disable the problem exactly by this parameter and the
> following
> browser creation:
>
>   Browser::CreateParams bparams = Browser::CreateParams(profile_**,
>
> chrome::HOST_DESKTOP_TYPE_ASH)**;
>   bparams.initial_show_state = ui::SHOW_STATE_DEFAULT;  // or STATE_NORMAL
>   Browser* browser = new Browser(bparams);
>   browser->window()->Show();
>
>
> On 2013/03/19 21:21:16, sky wrote:
>
>> Also, I think this means you can leave the code as it was (meaning don't
>> need to move to OnClose) and just just do a show on the browser after
>> adding the tab.
>>
>
>
>  On Tue, Mar 19, 2013 at 2:18 PM, Scott Violet <mailto:sky@chromium.org>
>> wrote:
>>
>
>  >
>> >
>> >
>> > On Tue, Mar 19, 2013 at 10:33 AM, <mailto:gspencer@chromium.org> wrote:
>> >
>> >> On 2013/03/18 16:10:48, sky wrote:
>> >>
>> >>> This should set params.window_action to SHOW_WINDOW, otherwise if
>> >>> FindOrCreateTabbedBrowser creates a new browser I'm not sure Navigate
>> >>> ends up
>> >>> showing it.
>> >>>
>> >>
>> >> Scott, are you saying that this is perhaps an alternate fix for this
>> bug?
>> >>
>> >
>> > Looking at the bug report I see this is likely the cause of the bug.
>> >
>> >   -Scott
>> >
>> >
>> >> Phillipp, could you try this and see if it solves the problem?
>> >>
>> >>
>>
>
> https://codereview.chromium.****org/12089026/%3Chttps://codere**
> view.chromium.org/12089026/ <http://codereview.chromium.org/12089026/>>
>
>> >>
>> >
>> >
>>
>
>
>
https://codereview.chromium.**org/12089026/<https://codereview.chromium.org/1...
>

[pneubeck (no reviews), 2013-03-20 16:08:36]

On 2013/03/20 14:37:58, sky wrote:
> What are the steps to reproduce this? Can you create a test for this?

I modified the patch, such that anybody who likes to reproduce it can do the
following:

- Compile with GYP_DEFINES including chromeos=1
- run chrome binary directly in linux
- open a browser window
- maximize/normalize the window
- close the window
- click on tray -> "Connected to eth0" -> "stub_wifi2" -> "Get new certificate"
--> A browser window should be visible

Starting with
  bparams.initial_show_state = ui::SHOW_STATE_MAXIMIZED;
the browser gets never visible, independent of the state of the previously
closed browser.

No idea how to write a minimal browser or even unit test to reproduce this.
However, I will further search for the cause of this and continue wasting
time...

[sky, 2013-03-20 23:36:13]

On 2013/03/20 16:08:36, pneubeck wrote:
> On 2013/03/20 14:37:58, sky wrote:
> > What are the steps to reproduce this? Can you create a test for this?
> 
> I modified the patch, such that anybody who likes to reproduce it can do the
> following:
> 
> - Compile with GYP_DEFINES including chromeos=1
> - run chrome binary directly in linux
> - open a browser window
> - maximize/normalize the window
> - close the window
> - click on tray -> "Connected to eth0" -> "stub_wifi2" -> "Get new
certificate"
> --> A browser window should be visible
> 
> Starting with
>   bparams.initial_show_state = ui::SHOW_STATE_MAXIMIZED;
> the browser gets never visible, independent of the state of the previously
> closed browser.

I see the bug now and agree it's likely not a trivial fix. I'm ok with the
workaround for now. In particular patchset 2 with addressing my comment there
should be fine.

> No idea how to write a minimal browser or even unit test to reproduce this.

You can do anything from a browser tests. The steps you've outlined are a bit
painful to do from a browser test but still possible. The key thing you have
going on here is trying to show a maximized window while a system modal dialog
is up. That's easy to write a unit test against:

TEST_F(WorkspaceManagerTest, SwitchFromModal) {
  scoped_ptr<Window> modal_window(CreateTestWindowUnparented());
  modal_window->SetBounds(gfx::Rect(10, 11, 21, 22));
  modal_window->SetProperty(aura::client::kModalKey, ui::MODAL_TYPE_SYSTEM);
  SetDefaultParentByPrimaryRootWindow(modal_window.get());
  modal_window->Show();

  scoped_ptr<Window> maximized_window(CreateTestWindow());
  maximized_window->SetProperty(
      aura::client::kShowStateKey, ui::SHOW_STATE_MAXIMIZED);
  maximized_window->Show();
  wm::ActivateWindow(maximized_window.get());
  EXPECT_TRUE(maximized_window->IsVisible());
}

I'll add this to the bug Greg created.

> However, I will further search for the cause of this and continue wasting
> time...

That's surprising, I hadn't realized fixing bugs was a waste of time.

  -Scott

[stevenjb (google-dont-use), 2013-03-21 01:43:45]

On Wed, Mar 20, 2013 at 4:36 PM, <sky@chromium.org> wrote:

>
>  No idea how to write a minimal browser or even unit test to reproduce
>> this.
>>
>
> You can do anything from a browser tests. The steps you've outlined are a
> bit
> painful to do from a browser test but still possible. The key thing you
> have
> going on here is trying to show a maximized window while a system modal
> dialog
> is up. That's easy to write a unit test against:
>
> TEST_F(WorkspaceManagerTest, SwitchFromModal) {
>   scoped_ptr<Window> modal_window(**CreateTestWindowUnparented());
>   modal_window->SetBounds(gfx::**Rect(10, 11, 21, 22));
>   modal_window->SetProperty(**aura::client::kModalKey,
> ui::MODAL_TYPE_SYSTEM);
>   SetDefaultParentByPrimaryRootW**indow(modal_window.get());
>   modal_window->Show();
>
>   scoped_ptr<Window> maximized_window(**CreateTestWindow());
>   maximized_window->SetProperty(
>       aura::client::kShowStateKey, ui::SHOW_STATE_MAXIMIZED);
>   maximized_window->Show();
>   wm::ActivateWindow(maximized_**window.get());
>   EXPECT_TRUE(maximized_window->**IsVisible());
> }
>
> I'll add this to the bug Greg created.
>
> Thanks Scott for this example, that's very helpful I think. There is a
definite learning curve to writing browser tests for windowing UI.


>
>  However, I will further search for the cause of this and continue wasting
>> time...
>>
>
> That's surprising, I hadn't realized fixing bugs was a waste of time.
>
> I don't want to speak for Phillipp, but having been there myself I think
this can safely be interpreted as an expression of frustration when
debugging UI code, especially when there is pressure to get something fixed
for a particular milestone. This was my original concern and reason for
supporting patchset #2 (with the params.window_action addition). I am sorry
for not communicating that better and sooner.

[pneubeck (no reviews), 2013-03-21 08:27:21]

On 2013/03/20 23:36:13, sky wrote:
> On 2013/03/20 16:08:36, pneubeck wrote:
> > On 2013/03/20 14:37:58, sky wrote:
> > > What are the steps to reproduce this? Can you create a test for this?
> > 
> > I modified the patch, such that anybody who likes to reproduce it can do the
> > following:
> > 
> > - Compile with GYP_DEFINES including chromeos=1
> > - run chrome binary directly in linux
> > - open a browser window
> > - maximize/normalize the window
> > - close the window
> > - click on tray -> "Connected to eth0" -> "stub_wifi2" -> "Get new
> certificate"
> > --> A browser window should be visible
> > 
> > Starting with
> >   bparams.initial_show_state = ui::SHOW_STATE_MAXIMIZED;
> > the browser gets never visible, independent of the state of the previously
> > closed browser.
> 
> I see the bug now and agree it's likely not a trivial fix. I'm ok with the
> workaround for now. In particular patchset 2 with addressing my comment there
> should be fine.
> 
> > No idea how to write a minimal browser or even unit test to reproduce this.
> 
> You can do anything from a browser tests. The steps you've outlined are a bit
> painful to do from a browser test but still possible. The key thing you have
> going on here is trying to show a maximized window while a system modal dialog
> is up. That's easy to write a unit test against:
> 
> TEST_F(WorkspaceManagerTest, SwitchFromModal) {
>   scoped_ptr<Window> modal_window(CreateTestWindowUnparented());
>   modal_window->SetBounds(gfx::Rect(10, 11, 21, 22));
>   modal_window->SetProperty(aura::client::kModalKey, ui::MODAL_TYPE_SYSTEM);
>   SetDefaultParentByPrimaryRootWindow(modal_window.get());
>   modal_window->Show();
> 
>   scoped_ptr<Window> maximized_window(CreateTestWindow());
>   maximized_window->SetProperty(
>       aura::client::kShowStateKey, ui::SHOW_STATE_MAXIMIZED);
>   maximized_window->Show();
>   wm::ActivateWindow(maximized_window.get());
>   EXPECT_TRUE(maximized_window->IsVisible());
> }
> 
> I'll add this to the bug Greg created.
> 
> > However, I will further search for the cause of this and continue wasting
> > time...
> 
> That's surprising, I hadn't realized fixing bugs was a waste of time.
> 
>   -Scott

That wasn't what I said.

It is a waste of time, if there are developers who know the UI code much better
and if I don't gain much from learning about that code (it's changing rapidly I
would assume).

But let's stop complaining. I think we all have the same goal but just not
enough time ;-)

[pneubeck (no reviews), 2013-03-21 12:24:26]

I cleaned up the file a bit more:
  - fixed includes
  - let Navigate() open a new browser if necessary

[pneubeck (no reviews), 2013-03-21 12:26:14]

On 2013/03/21 12:24:26, pneubeck wrote:
> I cleaned up the file a bit more:
>   - fixed includes
>   - let Navigate() open a new browser if necessary

PTAL and check Commit if you want to.

[sky, 2013-03-21 15:06:59]

LGTM

[commit-bot: I haz the power, 2013-03-21 15:08:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/12089026/74001

[commit-bot: I haz the power, 2013-03-22 18:08:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/12089026/74001

[commit-bot: I haz the power, 2013-03-23 14:39:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/12089026/74001

[commit-bot: I haz the power, 2013-03-23 16:16:57]

Change committed as 190018