Make the minimize state work when creating a chrome window in CrOS

Make the minimize state work when creating a chrome window in CrOS

This CL makes SHOW_STATE_MINIMIZE state work when creating a
chrome window in CrOS, so we don't need to call Minimize()
manually in tabs_api.cc

BUG=473228
TEST=NativeWidgetAuraTest.CreateMinimized

Committed: https://crrev.com/d5d6762c5539bc7d6098588323a16a24826e1149
Cr-Commit-Position: refs/heads/master@{#361898}

[joone, 2015-05-15 20:14:59]

joone.hur@intel.com changed reviewers:
+ sadrul@chromium.org

[joone, 2015-05-15 20:15:00]

sadrul@ could you review this patch?

[joone, 2015-05-15 20:17:40]

joone.hur@intel.com changed reviewers:
+ erg@chromium.org

[sadrul, 2015-05-15 20:30:53]

sadrul@chromium.org changed reviewers:
+ oshima@chromium.org

[sadrul, 2015-05-15 20:30:54]

+oshima@

https://codereview.chromium.org/1130033003/diff/1/ui/views/widget/native_widg...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/1130033003/diff/1/ui/views/widget/native_widg...
ui/views/widget/native_widget_aura.cc:503: 
Looks like you can just do window_->SetProperty() unconditionally, and that
would be sufficient?

[oshima, 2015-05-15 20:45:53]

https://codereview.chromium.org/1130033003/diff/1/ui/views/widget/native_widg...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/1130033003/diff/1/ui/views/widget/native_widg...
ui/views/widget/native_widget_aura.cc:503: 
On 2015/05/15 20:30:54, sadrul wrote:
> Looks like you can just do window_->SetProperty() unconditionally, and that
> would be sufficient?

Yes, that should be sufficient. One caveat: IIRC, chrome keeps at least one
window visible at startup, even if all windows were minimized when shutting down
last time. I don't think this will affect it, but you probably want to double
check that behavior.

[joone, 2015-05-18 18:16:34]

On 2015/05/15 20:45:53, oshima wrote:
>
https://codereview.chromium.org/1130033003/diff/1/ui/views/widget/native_widg...
> File ui/views/widget/native_widget_aura.cc (right):
> 
>
https://codereview.chromium.org/1130033003/diff/1/ui/views/widget/native_widg...
> ui/views/widget/native_widget_aura.cc:503: 
> On 2015/05/15 20:30:54, sadrul wrote:
> > Looks like you can just do window_->SetProperty() unconditionally, and that
> > would be sufficient?
> 
> Yes, that should be sufficient. One caveat: IIRC, chrome keeps at least one
> window visible at startup, even if all windows were minimized when shutting
down
> last time. I don't think this will affect it, but you probably want to double
> check that behavior.

I tested all the window state cases with the updated patch, they work fine.

[sadrul, 2015-05-18 19:51:43]

https://codereview.chromium.org/1130033003/diff/20001/ui/views/widget/native_...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/1130033003/diff/20001/ui/views/widget/native_...
ui/views/widget/native_widget_aura.cc:490: 
Why? This looks like a change in behaviour.

[joone, 2015-05-18 20:22:44]

https://codereview.chromium.org/1130033003/diff/20001/ui/views/widget/native_...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/1130033003/diff/20001/ui/views/widget/native_...
ui/views/widget/native_widget_aura.cc:490: 
On 2015/05/18 19:51:42, sadrul wrote:
> Why? This looks like a change in behaviour.

The minimized state doesn't work when we running the blow code.

[sadrul, 2015-05-18 20:29:28]

https://codereview.chromium.org/1130033003/diff/20001/ui/views/widget/native_...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/1130033003/diff/20001/ui/views/widget/native_...
ui/views/widget/native_widget_aura.cc:490: 
On 2015/05/18 20:22:43, joone wrote:
> On 2015/05/18 19:51:42, sadrul wrote:
> > Why? This looks like a change in behaviour.
> 
> The minimized state doesn't work when we running the blow code.

Why? We currently do run the code below for MINIMIZED, right? Why can't we do
that if we set the property?

[joone, 2015-05-18 20:40:51]

On 2015/05/18 20:29:28, sadrul wrote:
>
https://codereview.chromium.org/1130033003/diff/20001/ui/views/widget/native_...
> File ui/views/widget/native_widget_aura.cc (right):
> 
>
https://codereview.chromium.org/1130033003/diff/20001/ui/views/widget/native_...
> ui/views/widget/native_widget_aura.cc:490: 
> On 2015/05/18 20:22:43, joone wrote:
> > On 2015/05/18 19:51:42, sadrul wrote:
> > > Why? This looks like a change in behaviour.
> > 
> > The minimized state doesn't work when we running the blow code.
> 
> Why? We currently do run the code below for MINIMIZED, right? Why can't we do
> that if we set the property?

That's the bug that MINIMIZED doesn't work:
http://code.google.com/p/chromium/issues/detail?id=473228

[oshima, 2015-05-18 20:48:07]

https://codereview.chromium.org/1130033003/diff/20001/ui/views/widget/native_...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/1130033003/diff/20001/ui/views/widget/native_...
ui/views/widget/native_widget_aura.cc:490: 
On 2015/05/18 20:29:27, sadrul wrote:
> On 2015/05/18 20:22:43, joone wrote:
> > On 2015/05/18 19:51:42, sadrul wrote:
> > > Why? This looks like a change in behaviour.
> > 
> > The minimized state doesn't work when we running the blow code.
> 
> Why? We currently do run the code below for MINIMIZED, right? Why can't we do
> that if we set the property?

Showing (or activating) minimized window will unminimize the window, and that's
probably why. That's being said, allowing MINIMINIZED here may sounds a
contradiction because a minimized window isn't shown. Maybe caller should just
call minimized?

[joone, 2015-05-19 00:29:14]

On 2015/05/18 20:48:07, oshima wrote:
>
https://codereview.chromium.org/1130033003/diff/20001/ui/views/widget/native_...
> File ui/views/widget/native_widget_aura.cc (right):
> 
>
https://codereview.chromium.org/1130033003/diff/20001/ui/views/widget/native_...
> ui/views/widget/native_widget_aura.cc:490: 
> On 2015/05/18 20:29:27, sadrul wrote:
> > On 2015/05/18 20:22:43, joone wrote:
> > > On 2015/05/18 19:51:42, sadrul wrote:
> > > > Why? This looks like a change in behaviour.
> > > 
> > > The minimized state doesn't work when we running the blow code.
> > 
> > Why? We currently do run the code below for MINIMIZED, right? Why can't we
do
> > that if we set the property?
> 
> Showing (or activating) minimized window will unminimize the window, and
that's
> probably why. That's being said, allowing MINIMINIZED here may sounds a
> contradiction because a minimized window isn't shown. Maybe caller should just
> call minimized?

Currently, Minimize() is directly called in tabs_api.cc, but according to the
bug report, this is a workaround.

[oshima, 2015-05-19 22:53:00]

can you double check with the owner if the decision was made based on the fact
that minimized window isn't shown?

[joone, 2015-05-19 23:23:28]

On 2015/05/19 22:53:00, oshima wrote:
> can you double check with the owner if the decision was made based on the fact
> that minimized window isn't shown?

kalman@ could you confirm this?

[joone, 2015-05-19 23:24:52]

On 2015/05/19 22:53:00, oshima wrote:
> can you double check with the owner if the decision was made based on the fact
> that minimized window isn't shown?

kalman@ could you confirm this?

[limasdf, 2015-05-20 16:47:24]

limasdf@gmail.com changed reviewers:
+ kalman@chromium.org, limasdf@gmail.com

[limasdf, 2015-05-20 16:47:24]

Sorry to interrupt.

Joone, you can find a test extension on crbug.com/459841

With this CL, below extension code doesn't create window(*actually doesn't
Show()) which is not intended.

chrome.windows.create({
  state: 'minimized'
});

and I believe this CL changes behavior of chrome.app.window api too.

Adding @kalman FYI

https://codereview.chromium.org/1130033003/diff/40001/ui/views/widget/native_...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/1130033003/diff/40001/ui/views/widget/native_...
ui/views/widget/native_widget_aura.cc:488: if (state ==
ui::SHOW_STATE_MINIMIZED)
If ui::SHOW_STATE_MINIMIZED state doesn't meet show(), There's no way to see the
window.

I think we need to handle ui::SHOW_STATE_MINIMIZED property in
ui/aura/window.h/cc to Show() can handle it.

[not at google - send to devlin, 2015-05-20 20:18:48]

kalman@chromium.org changed reviewers:
+ tapted@chromium.org

[not at google - send to devlin, 2015-05-20 20:18:48]

There was a lot of investigation into why some platforms didn't support some of
the initial configurations. IIRC limasdf@ covered these. Could we discuss in the
bug what the proposed solution is? tapted@ could be a good person to include in
the conversation.

[joone, 2015-05-20 22:55:20]

On 2015/05/20 16:47:24, Sung-guk Lim wrote:
> Sorry to interrupt.
> 
> Joone, you can find a test extension on crbug.com/459841
> 
> With this CL, below extension code doesn't create window(*actually doesn't
> Show()) which is not intended.
> 
> chrome.windows.create({
>   state: 'minimized'
> });
> 
> and I believe this CL changes behavior of chrome.app.window api too.
> 
> Adding @kalman FYI
> 
>
https://codereview.chromium.org/1130033003/diff/40001/ui/views/widget/native_...
> File ui/views/widget/native_widget_aura.cc (right):
> 
>
https://codereview.chromium.org/1130033003/diff/40001/ui/views/widget/native_...
> ui/views/widget/native_widget_aura.cc:488: if (state ==
> ui::SHOW_STATE_MINIMIZED)
> If ui::SHOW_STATE_MINIMIZED state doesn't meet show(), There's no way to see
the
> window.
> 
> I think we need to handle ui::SHOW_STATE_MINIMIZED property in
> ui/aura/window.h/cc to Show() can handle it.

Hello Sung-guk,

Thanks for testing my CL.
I fixed the problem so extension windows can be minimized when launching.
Please take a look at my recent CL.

[joone, 2015-05-20 23:09:58]

On 2015/05/20 20:18:48, kalman wrote:
> There was a lot of investigation into why some platforms didn't support some
of
> the initial configurations. IIRC limasdf@ covered these. Could we discuss in
the
> bug what the proposed solution is? tapted@ could be a good person to include
in
> the conversation.

kalman, I'm working on this issue for each platform:
It was fixed in Windows and Linux:
https://crrev.com/051aece17c0e035938cdd99233967f1cba865b4a (Winows)
https://crrev.com/3a3f2f572a3a96957a6180056e7cbd30f551200d (Linux)

I will fix this for Mac.

[joone, 2015-05-26 17:30:44]

On 2015/05/20 23:09:58, joone wrote:
> On 2015/05/20 20:18:48, kalman wrote:
> > There was a lot of investigation into why some platforms didn't support some
> of
> > the initial configurations. IIRC limasdf@ covered these. Could we discuss in
> the
> > bug what the proposed solution is? tapted@ could be a good person to include
> in
> > the conversation.
> 
> kalman, I'm working on this issue for each platform:
> It was fixed in Windows and Linux:
> https://crrev.com/051aece17c0e035938cdd99233967f1cba865b4a (Winows)
> https://crrev.com/3a3f2f572a3a96957a6180056e7cbd30f551200d (Linux)
> 
> I will fix this for Mac.

kalman@, can you take a look at my recent patch?
Sung-guk, can you test my recent patch again?

[not at google - send to devlin, 2015-05-26 20:11:26]

This looks fine, but @limasdf there was a test you added for this - is there
something that should change about the test to make sure we're hitting ChromeOS
now?

[tapted, 2015-05-26 23:25:30]

On 2015/05/26 20:11:26, kalman wrote:
> This looks fine, but @limasdf there was a test you added for this - is there
> something that should change about the test to make sure we're hitting
ChromeOS
> now?

This is ExtensionWindowCreateTest, AcceptState - It's just Desktop Linux that
has special treatment, since checking `IsMinimized` on native X11 is
asynchronous. The test on ChromeOS should be effective (i.e. if it still passes
on ChromeOS, then the test is doing its job).

However, if you do as I suggest and remove the #if for Desktop Linux as well, it
needs to be checked manually on Desktop Linux.

https://codereview.chromium.org/1130033003/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/1130033003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:666: // once mapped, but not
minimize or fullscreen.
I think minimize was added to DWTHX11's ShowWindowWithState in
http://crrev.com/329764 so the Minimize() below should now be redundant on
Desktop Linux as well.

I.e. this entire #if block should be able to be deleted now.

Likely the one below for fullscreen can go too, since handling for that was also
added in r329764.

[joone, 2015-05-27 17:50:22]

https://codereview.chromium.org/1130033003/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/1130033003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:666: // once mapped, but not
minimize or fullscreen.
On 2015/05/26 23:25:30, tapted wrote:
> I think minimize was added to DWTHX11's ShowWindowWithState in
> http://crrev.com/329764 so the Minimize() below should now be redundant on
> Desktop Linux as well.
> 
> I.e. this entire #if block should be able to be deleted now.
> 
> Likely the one below for fullscreen can go too, since handling for that was
also
> added in r329764.

Can I create a separated CL for the Desktop Linux case? Because the below code
that handles ui::SHOW_STATE_FULLSCREEN can be removed together.

[joone, 2015-05-27 21:57:43]

On 2015/05/27 17:50:22, joone wrote:
>
https://codereview.chromium.org/1130033003/diff/60001/chrome/browser/extensio...
> File chrome/browser/extensions/api/tabs/tabs_api.cc (right):
> 
>
https://codereview.chromium.org/1130033003/diff/60001/chrome/browser/extensio...
> chrome/browser/extensions/api/tabs/tabs_api.cc:666: // once mapped, but not
> minimize or fullscreen.
> On 2015/05/26 23:25:30, tapted wrote:
> > I think minimize was added to DWTHX11's ShowWindowWithState in
> > http://crrev.com/329764 so the Minimize() below should now be redundant on
> > Desktop Linux as well.
> > 
> > I.e. this entire #if block should be able to be deleted now.
> > 
> > Likely the one below for fullscreen can go too, since handling for that was
> also
> > added in r329764.
> 
> Can I create a separated CL for the Desktop Linux case? Because the below code
> that handles ui::SHOW_STATE_FULLSCREEN can be removed together.

I created a CL for dealing with Desktop Linux cases:
https://codereview.chromium.org/1159703006/

[joone, 2015-05-28 16:52:18]

tapted@, kalman@, thanks for the review on
https://codereview.chromium.org/1159703006/

Due to the change, I rebased this CL so it would be good to take a look at it.

[tapted, 2015-05-28 23:55:02]

https://codereview.chromium.org/1130033003/diff/80001/ui/views/widget/native_...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/1130033003/diff/80001/ui/views/widget/native_...
ui/views/widget/native_widget_aura.cc:499: Minimize();
Minimize() is literally `window_->SetProperty(aura::client::kShowStateKey,
ui::SHOW_STATE_MINIMIZED);` so this should be redundant with the change above,
unless something in between is "undoing" the minimize, which should be avoided.

Does it still work after removing this line?

[tapted, 2015-05-29 00:02:16]

https://codereview.chromium.org/1130033003/diff/80001/ui/views/widget/native_...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/1130033003/diff/80001/ui/views/widget/native_...
ui/views/widget/native_widget_aura.cc:499: Minimize();
On 2015/05/28 23:55:02, tapted wrote:
> Minimize() is literally `window_->SetProperty(aura::client::kShowStateKey,
> ui::SHOW_STATE_MINIMIZED);` so this should be redundant with the change above,
> unless something in between is "undoing" the minimize, which should be
avoided.
> 
> Does it still work after removing this line?

(maybe you just need to include ui::SHOW_STATE_MINIMIZED in the `if (state !=
ui::SHOW_STATE_INACTIVE)` condition above? - that would make sense to me, but
sadrul might have other ideas :)

[joone, 2015-05-29 00:52:01]

https://codereview.chromium.org/1130033003/diff/80001/ui/views/widget/native_...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/1130033003/diff/80001/ui/views/widget/native_...
ui/views/widget/native_widget_aura.cc:499: Minimize();
On 2015/05/29 00:02:16, tapted wrote:
> On 2015/05/28 23:55:02, tapted wrote:
> > Minimize() is literally `window_->SetProperty(aura::client::kShowStateKey,
> > ui::SHOW_STATE_MINIMIZED);` so this should be redundant with the change
above,
> > unless something in between is "undoing" the minimize, which should be
> avoided.
> > 
> > Does it still work after removing this line?
> 
> (maybe you just need to include ui::SHOW_STATE_MINIMIZED in the `if (state !=
> ui::SHOW_STATE_INACTIVE)` condition above? - that would make sense to me, but
> sadrul might have other ideas :)

It works fine so I've uploaded the patch.

[tapted, 2015-05-29 00:57:42]

https://codereview.chromium.org/1130033003/diff/100001/ui/views/widget/native...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/1130033003/diff/100001/ui/views/widget/native...
ui/views/widget/native_widget_aura.cc:492: if (state ==
ui::SHOW_STATE_MINIMIZED)
Sorry - I meant this should be part of the existing condition statement (rather
than a new condition statement).

i.e. on line 490

if (state != ui::SHOW_STATE_INACTIVE && state != ui::SHOW_STATE_MINIMIZED) {

Rationale: if the initial show state is minimized, the window shouldn't be
activated at all.

It should be unnecessary to call Minimize() after the SetProperty(..) call on
line 487, since it will do exactly the same thing.

[joone, 2015-05-29 01:11:27]

https://codereview.chromium.org/1130033003/diff/100001/ui/views/widget/native...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/1130033003/diff/100001/ui/views/widget/native...
ui/views/widget/native_widget_aura.cc:492: if (state ==
ui::SHOW_STATE_MINIMIZED)
On 2015/05/29 00:57:42, tapted wrote:
> Sorry - I meant this should be part of the existing condition statement
(rather
> than a new condition statement).
> 
> i.e. on line 490
> 
> if (state != ui::SHOW_STATE_INACTIVE && state != ui::SHOW_STATE_MINIMIZED) {
> 
> Rationale: if the initial show state is minimized, the window shouldn't be
> activated at all.
> 
> It should be unnecessary to call Minimize() after the SetProperty(..) call on
> line 487, since it will do exactly the same thing.

Minimized() should be called. Without calling it, 'minimized' state doesn't
work. There seems no way to minimize a window initially.
Instead, we open a window and minimize it forcibly.

[tapted, 2015-05-29 01:25:12]

https://codereview.chromium.org/1130033003/diff/100001/ui/views/widget/native...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/1130033003/diff/100001/ui/views/widget/native...
ui/views/widget/native_widget_aura.cc:492: if (state ==
ui::SHOW_STATE_MINIMIZED)
On 2015/05/29 01:11:27, joone wrote:
> On 2015/05/29 00:57:42, tapted wrote:
> > Sorry - I meant this should be part of the existing condition statement
> (rather
> > than a new condition statement).
> > 
> > i.e. on line 490
> > 
> > if (state != ui::SHOW_STATE_INACTIVE && state != ui::SHOW_STATE_MINIMIZED) {
> > 
> > Rationale: if the initial show state is minimized, the window shouldn't be
> > activated at all.
> > 
> > It should be unnecessary to call Minimize() after the SetProperty(..) call
on
> > line 487, since it will do exactly the same thing.
> 
> Minimized() should be called. Without calling it, 'minimized' state doesn't
> work.

Can you find out why? i.e. What is "undoing" the call to SetProperty in line
487. Maybe you need to set a breakpoint in aura::Window::SetProperty to find
out.

Maybe that will lead to a nicer fix.

Maybe it's not be nicer, in which case it probably doesn't make sense to call
SetProperty() until after the call to Show() (i.e. move line 488 down to line
500). Maybe that breaks other things, but we need a proper understanding of the
problem before trying to fix it.

> There seems no way to minimize a window initially.
> Instead, we open a window and minimize it forcibly.

[oshima, 2015-05-29 09:24:21]

https://codereview.chromium.org/1130033003/diff/100001/ui/views/widget/native...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/1130033003/diff/100001/ui/views/widget/native...
ui/views/widget/native_widget_aura.cc:492: if (state ==
ui::SHOW_STATE_MINIMIZED)
On 2015/05/29 01:25:12, tapted wrote:
> On 2015/05/29 01:11:27, joone wrote:
> > On 2015/05/29 00:57:42, tapted wrote:
> > > Sorry - I meant this should be part of the existing condition statement
> > (rather
> > > than a new condition statement).
> > > 
> > > i.e. on line 490
> > > 
> > > if (state != ui::SHOW_STATE_INACTIVE && state != ui::SHOW_STATE_MINIMIZED)
{
> > > 
> > > Rationale: if the initial show state is minimized, the window shouldn't be
> > > activated at all.
> > > 
> > > It should be unnecessary to call Minimize() after the SetProperty(..) call
> on
> > > line 487, since it will do exactly the same thing.
> > 
> > Minimized() should be called. Without calling it, 'minimized' state doesn't
> > work.
> 
> Can you find out why? i.e. What is "undoing" the call to SetProperty in line
> 487. Maybe you need to set a breakpoint in aura::Window::SetProperty to find
> out.

This is because showing minimized window will unminimize the window.
(In other words, it does not make sense to "Show" minimized window because
it's hidden)
You can simply skip the show if the minimized state is requested.

> 
> Maybe that will lead to a nicer fix.
> 
> Maybe it's not be nicer, in which case it probably doesn't make sense to call
> SetProperty() until after the call to Show() (i.e. move line 488 down to line
> 500). Maybe that breaks other things, but we need a proper understanding of
the
> problem before trying to fix it.
> 
> > There seems no way to minimize a window initially.
> > Instead, we open a window and minimize it forcibly.
>

[joone, 2015-05-29 18:34:13]

oshima@ I tried to skip the show if the minimized state is requested, but a
normal window is opened.

tapted@ All the window states including the minimized state work fine by calling
SetProperty after the Show() call.

[oshima, 2015-05-29 18:35:42]

On 2015/05/29 18:34:13, joone wrote:
> oshima@ I tried to skip the show if the minimized state is requested, but a
> normal window is opened.

Didn't you activate it? Activating the minimized window will also show the
window.

> 
> tapted@ All the window states including the minimized state work fine by
calling
> SetProperty after the Show() call.

[joone, 2015-05-29 19:47:49]

On 2015/05/29 18:35:42, oshima wrote:
> On 2015/05/29 18:34:13, joone wrote:
> > oshima@ I tried to skip the show if the minimized state is requested, but a
> > normal window is opened.
> 
> Didn't you activate it? Activating the minimized window will also show the
> window.
> 
> > 
> > tapted@ All the window states including the minimized state work fine by
> calling
> > SetProperty after the Show() call.

If we don't call Activate(), window is not created.

[joone, 2015-05-29 19:50:40]

On 2015/05/29 19:47:49, joone wrote:
> On 2015/05/29 18:35:42, oshima wrote:
> > On 2015/05/29 18:34:13, joone wrote:
> > > oshima@ I tried to skip the show if the minimized state is requested, but
a
> > > normal window is opened.
> > 
> > Didn't you activate it? Activating the minimized window will also show the
> > window.
> > 
> > > 
> > > tapted@ All the window states including the minimized state work fine by
> > calling
> > > SetProperty after the Show() call.
> 
> If we don't call Activate(), window is not created.

It seems to be hidden because I see the minimized window name in Task Manager,
but
there is no way to restore it.

[oshima, 2015-05-29 19:58:30]

On 2015/05/29 19:50:40, joone wrote:
> On 2015/05/29 19:47:49, joone wrote:
> > On 2015/05/29 18:35:42, oshima wrote:
> > > On 2015/05/29 18:34:13, joone wrote:
> > > > oshima@ I tried to skip the show if the minimized state is requested,
but
> a
> > > > normal window is opened.
> > > 
> > > Didn't you activate it? Activating the minimized window will also show the
> > > window.
> > > 
> > > > 
> > > > tapted@ All the window states including the minimized state work fine by
> > > calling
> > > > SetProperty after the Show() call.
> > 
> > If we don't call Activate(), window is not created.
> 
> It seems to be hidden because I see the minimized window name in Task Manager,
> but
> there is no way to restore it.

Do you mean there is no corresponding item in launcher? If so, it sounds like a
bug.

[joone, 2015-05-29 19:59:45]

On 2015/05/29 19:58:30, oshima wrote:
> On 2015/05/29 19:50:40, joone wrote:
> > On 2015/05/29 19:47:49, joone wrote:
> > > On 2015/05/29 18:35:42, oshima wrote:
> > > > On 2015/05/29 18:34:13, joone wrote:
> > > > > oshima@ I tried to skip the show if the minimized state is requested,
> but
> > a
> > > > > normal window is opened.
> > > > 
> > > > Didn't you activate it? Activating the minimized window will also show
the
> > > > window.
> > > > 
> > > > > 
> > > > > tapted@ All the window states including the minimized state work fine
by
> > > > calling
> > > > > SetProperty after the Show() call.
> > > 
> > > If we don't call Activate(), window is not created.
> > 
> > It seems to be hidden because I see the minimized window name in Task
Manager,
> > but
> > there is no way to restore it.
> 
> Do you mean there is no corresponding item in launcher? If so, it sounds like
a
> bug.

Yes.

[joone, 2015-06-01 22:42:54]

oshima@ what do you think about the patch set 7?

[oshima, 2015-06-01 22:50:22]

On 2015/06/01 22:42:54, joone wrote:
> oshima@ what do you think about the patch set 7?

Let me look into why it doesn't show up in the launcher first. by the way, when
you skip show for minimized,
does the window show up in overview mode?

[joone, 2015-06-01 23:04:31]

On 2015/06/01 22:50:22, oshima wrote:
> On 2015/06/01 22:42:54, joone wrote:
> > oshima@ what do you think about the patch set 7?
> 
> Let me look into why it doesn't show up in the launcher first. by the way,
when
> you skip show for minimized,
> does the window show up in overview mode?

Yes, I see the minimized window in overview mode.

[oshima, 2015-06-01 23:08:46]

On 2015/06/01 23:04:31, joone wrote:
> On 2015/06/01 22:50:22, oshima wrote:
> > On 2015/06/01 22:42:54, joone wrote:
> > > oshima@ what do you think about the patch set 7?
> > 
> > Let me look into why it doesn't show up in the launcher first. by the way,
> when
> > you skip show for minimized,
> > does the window show up in overview mode?
> 
> Yes, I see the minimized window in overview mode.

It should be a bug in the luncher then. Would you mind filing a bug and assign
it to me?

[joone, 2015-06-02 23:59:14]

On 2015/06/01 23:08:46, oshima wrote:
> On 2015/06/01 23:04:31, joone wrote:
> > On 2015/06/01 22:50:22, oshima wrote:
> > > On 2015/06/01 22:42:54, joone wrote:
> > > > oshima@ what do you think about the patch set 7?
> > > 
> > > Let me look into why it doesn't show up in the launcher first. by the way,
> > when
> > > you skip show for minimized,
> > > does the window show up in overview mode?
> > 
> > Yes, I see the minimized window in overview mode.
> 
> It should be a bug in the luncher then. Would you mind filing a bug and assign
> it to me?
Here is the bug: https://code.google.com/p/chromium/issues/detail?id=495878

[oshima, 2015-06-03 01:15:43]

On 2015/06/02 23:59:14, joone wrote:
> On 2015/06/01 23:08:46, oshima wrote:
> > On 2015/06/01 23:04:31, joone wrote:
> > > On 2015/06/01 22:50:22, oshima wrote:
> > > > On 2015/06/01 22:42:54, joone wrote:
> > > > > oshima@ what do you think about the patch set 7?
> > > > 
> > > > Let me look into why it doesn't show up in the launcher first. by the
way,
> > > when
> > > > you skip show for minimized,
> > > > does the window show up in overview mode?
> > > 
> > > Yes, I see the minimized window in overview mode.
> > 
> > It should be a bug in the luncher then. Would you mind filing a bug and
assign
> > it to me?
> Here is the bug: https://code.google.com/p/chromium/issues/detail?id=495878

thanks. I'll look into it tonight.

[joone, 2015-06-03 02:32:37]

On 2015/06/03 01:15:43, oshima wrote:
> On 2015/06/02 23:59:14, joone wrote:
> > On 2015/06/01 23:08:46, oshima wrote:
> > > It should be a bug in the luncher then. Would you mind filing a bug and
> assign
> > > it to me?
> > Here is the bug: https://code.google.com/p/chromium/issues/detail?id=495878
> 
> thanks. I'll look into it tonight.

I pushed my CL I worked on for this bug, but I'm not sure if there are not
regressions with this fix.
https://codereview.chromium.org/1155533006/

[oshima, 2015-06-06 00:47:35]

https://codereview.chromium.org/1130033003/diff/140001/ui/views/widget/native...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/1130033003/diff/140001/ui/views/widget/native...
ui/views/widget/native_widget_aura.cc:489: window_->Show();
Thank you joone@ for investigation.

It turns out that we do activate the window once even when showing in minimized
state on other platforms, and chrome depends on this behavior. (BrowserList's
active_list will never have this browser because it won't be activated
otherwise). Therefore, I'm convinced that we should do the same for consistency.

you can just minimize at the end.

[joone, 2015-06-06 01:02:22]

On 2015/06/06 00:47:35, oshima wrote:
>
https://codereview.chromium.org/1130033003/diff/140001/ui/views/widget/native...
> File ui/views/widget/native_widget_aura.cc (right):
> 
>
https://codereview.chromium.org/1130033003/diff/140001/ui/views/widget/native...
> ui/views/widget/native_widget_aura.cc:489: window_->Show();
> Thank you joone@ for investigation.
> 
> It turns out that we do activate the window once even when showing in
minimized
> state on other platforms, and chrome depends on this behavior. (BrowserList's
> active_list will never have this browser because it won't be activated
> otherwise). Therefore, I'm convinced that we should do the same for
consistency.
> 
> you can just minimize at the end.

How about this CL?
https://codereview.chromium.org/1130033003/#ps60001

[joone, 2015-06-06 01:50:58]

On 2015/06/06 01:02:22, joone wrote:
> On 2015/06/06 00:47:35, oshima wrote:
> >
>
https://codereview.chromium.org/1130033003/diff/140001/ui/views/widget/native...
> > File ui/views/widget/native_widget_aura.cc (right):
> > 
> >
>
https://codereview.chromium.org/1130033003/diff/140001/ui/views/widget/native...
> > ui/views/widget/native_widget_aura.cc:489: window_->Show();
> > Thank you joone@ for investigation.
> > 
> > It turns out that we do activate the window once even when showing in
> minimized
> > state on other platforms, and chrome depends on this behavior.
(BrowserList's
> > active_list will never have this browser because it won't be activated
> > otherwise). Therefore, I'm convinced that we should do the same for
> consistency.
> > 
> > you can just minimize at the end.
> 
> How about this CL?
> https://codereview.chromium.org/1130033003/#ps60001

I've rebased the above CL. Thanks!

[oshima, 2015-06-08 17:20:13]

lgtm with a nit

https://codereview.chromium.org/1130033003/diff/160001/ui/views/widget/native...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/1130033003/diff/160001/ui/views/widget/native...
ui/views/widget/native_widget_aura.cc:499: Minimize();
Can you add comment something like?

// On desktop aura, a window is activated first even when it is shown as
minimized. Do the same for consistency.

[joone, 2015-06-08 21:01:38]

The CQ bit was checked by joone.hur@intel.com

[joone, 2015-06-08 21:01:39]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/1130033003/#ps180001
(title: "Add comment")

[joone, 2015-06-08 21:01:54]

On 2015/06/08 17:20:13, oshima wrote:
> lgtm with a nit
> 
>
https://codereview.chromium.org/1130033003/diff/160001/ui/views/widget/native...
> File ui/views/widget/native_widget_aura.cc (right):
> 
>
https://codereview.chromium.org/1130033003/diff/160001/ui/views/widget/native...
> ui/views/widget/native_widget_aura.cc:499: Minimize();
> Can you add comment something like?
> 
> // On desktop aura, a window is activated first even when it is shown as
> minimized. Do the same for consistency.

Done. Thanks!

[commit-bot: I haz the power, 2015-06-08 21:04:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1130033003/180001

[commit-bot: I haz the power, 2015-06-08 21:16:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-08 21:16:58]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[joone, 2015-06-09 00:09:11]

On 2015/06/08 21:16:58, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

oshima@, I may need LGTM from you for the update CL.


** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/browser/extensions/api/tabs/tabs_api.cc
    ui/views/widget/native_widget_aura.cc

[oshima, 2015-06-09 00:41:36]

sorry I should have said "i'm not the owner".  you need approval from ui/views
and chrome/browser/extensions owners

[not at google - send to devlin, 2015-06-10 17:27:03]

Everybody seems happy, and I have zero to add either way - so lgtm.

[sadrul, 2015-06-10 17:34:47]

Can you add a test?

[joone, 2015-06-10 18:14:06]

The CQ bit was checked by joone.hur@intel.com

[commit-bot: I haz the power, 2015-06-10 18:15:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1130033003/180001

[commit-bot: I haz the power, 2015-06-10 18:23:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-10 18:23:21]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[joone, 2015-06-10 18:24:10]

On 2015/06/10 17:34:47, sadrul wrote:
> Can you add a test?

There is already a test case for the minimized state:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

[joone, 2015-06-10 18:53:59]

On 2015/06/10 17:27:03, kalman wrote:
> Everybody seems happy, and I have zero to add either way - so lgtm.

tapted@ This CL may also need your LGTM.

[limasdf, 2015-06-10 19:03:44]

Joone, You still need lg from sadrul@. (tapted is owner of native_widget_mac*
stuff.)

Plus, I think what sadrul@ said is that adding a test to
native_widget_aura_unittest.cc.

[joone, 2015-06-12 21:17:56]

On 2015/06/10 19:03:44, limasdf wrote:
> Joone, You still need lg from sadrul@. (tapted is owner of native_widget_mac*
> stuff.)
> 
> Plus, I think what sadrul@ said is that adding a test to
> native_widget_aura_unittest.cc.

limasdf, I added a test case to native_widget_aura_unittest.cc. Thanks!
sadrul@ Could you take a look at the test case if this could cover the change?

[sadrul, 2015-06-12 21:53:49]

https://codereview.chromium.org/1130033003/diff/200001/ui/views/widget/native...
File ui/views/widget/native_widget_aura_unittest.cc (right):

https://codereview.chromium.org/1130033003/diff/200001/ui/views/widget/native...
ui/views/widget/native_widget_aura_unittest.cc:109:
window->ShowWithWindowState(ui::SHOW_STATE_MINIMIZED);
Can you instead do:

  Widget::InitParams params(TYPE_WINDOW);
  params.show_state = ui::SHOW_STATE_MINIMIZED;
  ...
  widget->Init(params);
  EXPECT_TRUE(widget->IsMinimized());

?

[joone, 2015-06-12 23:51:41]

https://codereview.chromium.org/1130033003/diff/200001/ui/views/widget/native...
File ui/views/widget/native_widget_aura_unittest.cc (right):

https://codereview.chromium.org/1130033003/diff/200001/ui/views/widget/native...
ui/views/widget/native_widget_aura_unittest.cc:109:
window->ShowWithWindowState(ui::SHOW_STATE_MINIMIZED);
On 2015/06/12 21:53:49, sadrul wrote:
> Can you instead do:
> 
>   Widget::InitParams params(TYPE_WINDOW);
>   params.show_state = ui::SHOW_STATE_MINIMIZED;
>   ...
>   widget->Init(params);
>   EXPECT_TRUE(widget->IsMinimized());
> 
> ?

Done.

[varkha, 2015-06-19 16:14:55]

varkha@chromium.org changed reviewers:
+ varkha@chromium.org

[varkha, 2015-06-19 16:14:57]

https://codereview.chromium.org/1130033003/diff/220001/ui/views/widget/native...
File ui/views/widget/native_widget_aura_unittest.cc (right):

https://codereview.chromium.org/1130033003/diff/220001/ui/views/widget/native...
ui/views/widget/native_widget_aura_unittest.cc:103: TEST_F(NativeWidgetAuraTest,
MinimizedStateTest) {
Maybe s/MinimizedStateTest/CreateMinimized to be more specific?

[joone, 2015-06-19 20:18:39]

https://codereview.chromium.org/1130033003/diff/220001/ui/views/widget/native...
File ui/views/widget/native_widget_aura_unittest.cc (right):

https://codereview.chromium.org/1130033003/diff/220001/ui/views/widget/native...
ui/views/widget/native_widget_aura_unittest.cc:103: TEST_F(NativeWidgetAuraTest,
MinimizedStateTest) {
On 2015/06/19 16:14:56, varkha wrote:
> Maybe s/MinimizedStateTest/CreateMinimized to be more specific?

Done.

[joone, 2015-06-25 16:12:39]

On 2015/06/19 20:18:39, joone wrote:
>
https://codereview.chromium.org/1130033003/diff/220001/ui/views/widget/native...
> File ui/views/widget/native_widget_aura_unittest.cc (right):
> 
>
https://codereview.chromium.org/1130033003/diff/220001/ui/views/widget/native...
> ui/views/widget/native_widget_aura_unittest.cc:103:
TEST_F(NativeWidgetAuraTest,
> MinimizedStateTest) {
> On 2015/06/19 16:14:56, varkha wrote:
> > Maybe s/MinimizedStateTest/CreateMinimized to be more specific?
> 
> Done.

sadrul@ ping?

[sadrul, 2015-06-30 17:18:06]

https://codereview.chromium.org/1130033003/diff/240001/ui/views/widget/native...
File ui/views/widget/native_widget_aura_unittest.cc (right):

https://codereview.chromium.org/1130033003/diff/240001/ui/views/widget/native...
ui/views/widget/native_widget_aura_unittest.cc:113:
EXPECT_TRUE(widget->IsMinimized());
The change is in ShowWithWindowState(). Shouldn't this test exercise that code
by calling widget->Show() too?

[joone, 2015-06-30 23:51:40]

https://codereview.chromium.org/1130033003/diff/240001/ui/views/widget/native...
File ui/views/widget/native_widget_aura_unittest.cc (right):

https://codereview.chromium.org/1130033003/diff/240001/ui/views/widget/native...
ui/views/widget/native_widget_aura_unittest.cc:113:
EXPECT_TRUE(widget->IsMinimized());
On 2015/06/30 17:18:05, sadrul wrote:
> The change is in ShowWithWindowState(). Shouldn't this test exercise that code
> by calling widget->Show() too?

If so, we need to call widget->Minimize() together.

[joone, 2015-07-01 00:05:43]

https://codereview.chromium.org/1130033003/diff/260001/ui/views/widget/native...
File ui/views/widget/native_widget_aura_unittest.cc (right):

https://codereview.chromium.org/1130033003/diff/260001/ui/views/widget/native...
ui/views/widget/native_widget_aura_unittest.cc:113: widget->Minimize();
Now, this test case is almost like the change in ShowWithWindowState().

[sadrul, 2015-07-01 03:50:56]

https://codereview.chromium.org/1130033003/diff/260001/ui/views/widget/native...
File ui/views/widget/native_widget_aura_unittest.cc (right):

https://codereview.chromium.org/1130033003/diff/260001/ui/views/widget/native...
ui/views/widget/native_widget_aura_unittest.cc:113: widget->Minimize();
On 2015/07/01 00:05:42, joone wrote:
> Now, this test case is almost like the change in ShowWithWindowState().

Isn't the purpose of this CL to make sure Show() does the right thing, and an
explicit call to Minimize() isn't necessary? Isn't that the change you are
making in tabs_api.cc too? Why do we need to call Minimize() here, then?

[joone, 2015-07-01 22:50:15]

Please take a look at the updated CL.
Thanks!

https://codereview.chromium.org/1130033003/diff/260001/ui/views/widget/native...
File ui/views/widget/native_widget_aura_unittest.cc (right):

https://codereview.chromium.org/1130033003/diff/260001/ui/views/widget/native...
ui/views/widget/native_widget_aura_unittest.cc:113: widget->Minimize();
On 2015/07/01 03:50:56, sadrul wrote:
> On 2015/07/01 00:05:42, joone wrote:
> > Now, this test case is almost like the change in ShowWithWindowState().
> 
> Isn't the purpose of this CL to make sure Show() does the right thing, and an
> explicit call to Minimize() isn't necessary? Isn't that the change you are
> making in tabs_api.cc too? Why do we need to call Minimize() here, then?

Yes, Minimized() should not be necessary, but the show_state can be changed to
SHOW_STATE_DEFAULT in Widget::Show() by passing  saved_show_state_ to
ShowWithWindowState(). I think Widget::Show() should consider
SHOW_STATE_MINIMIZED state like SHOW_STATE_FULLSCREEN state.

[joone, 2015-09-28 07:23:58]

sadrul@ ping?

[joone, 2015-09-30 02:26:37]

joone.hur@intel.com changed reviewers:
+ sky@chromium.org

[joone, 2015-09-30 02:28:55]

sky@ can you review the test case?

[sadrul, 2015-09-30 04:03:51]

Some of the test failures in the trybots look related. Mind taking a look at
those?

https://codereview.chromium.org/1130033003/diff/300001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/1130033003/diff/300001/ui/views/widget/widget...
ui/views/widget/widget.cc:628: if (IsFullscreen())
braces here too

[joone, 2015-10-07 15:59:47]

sadrul@ All bots are now green.

https://codereview.chromium.org/1130033003/diff/300001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/1130033003/diff/300001/ui/views/widget/widget...
ui/views/widget/widget.cc:628: if (IsFullscreen())
On 2015/09/30 04:03:51, sadrul wrote:
> braces here too

Done.

[sadrul, 2015-10-07 16:32:54]

https://codereview.chromium.org/1130033003/diff/320001/ui/views/widget/native...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/1130033003/diff/320001/ui/views/widget/native...
ui/views/widget/native_widget_aura.cc:487: }
In earlier patchsets, the property was being set unconditionally. Can you
explain why it is necessary to remove that change? Please get another review
from oshima@ for this as well.

[varkha, 2015-10-07 18:16:57]

https://codereview.chromium.org/1130033003/diff/320001/ui/views/widget/native...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/1130033003/diff/320001/ui/views/widget/native...
ui/views/widget/native_widget_aura.cc:487: }
On 2015/10/07 16:32:54, sadrul wrote:
> In earlier patchsets, the property was being set unconditionally. Can you
> explain why it is necessary to remove that change? Please get another review
> from oshima@ for this as well.

BrowserRemembersDockedState tests were failing because the tests expect a
browser window to have NORMAL state upon creation (rather than allowing DEFAULT
state to be committed). Probably the other test failures had a similar reason. 
It could be that NORMAL and DEFAULT should be the only states excluded since
MINIMIZED is handled below.
I have tried replacing the condition with this:
  if (state != ui::SHOW_STATE_NORMAL && state != ui::SHOW_STATE_DEFAULT)
and the 4 interactive_ui_tests that were failing are passing but I didn't run
the full try jobs.

[joone, 2015-10-16 07:29:49]

https://codereview.chromium.org/1130033003/diff/320001/ui/views/widget/native...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/1130033003/diff/320001/ui/views/widget/native...
ui/views/widget/native_widget_aura.cc:487: }
On 2015/10/07 16:32:54, sadrul wrote:
> In earlier patchsets, the property was being set unconditionally. Can you
> explain why it is necessary to remove that change? Please get another review
> from oshima@ for this as well.

I thought that we could remove this condition, but I saw that some test cases
failed due to my change. So I just removed my change to recover the orignial
code.

[joone, 2015-11-17 02:00:49]

On 2015/10/16 07:29:49, joone wrote:
>
https://codereview.chromium.org/1130033003/diff/320001/ui/views/widget/native...
> File ui/views/widget/native_widget_aura.cc (right):
> 
>
https://codereview.chromium.org/1130033003/diff/320001/ui/views/widget/native...
> ui/views/widget/native_widget_aura.cc:487: }
> On 2015/10/07 16:32:54, sadrul wrote:
> > In earlier patchsets, the property was being set unconditionally. Can you
> > explain why it is necessary to remove that change? Please get another review
> > from oshima@ for this as well.
> 
> I thought that we could remove this condition, but I saw that some test cases
> failed due to my change. So I just removed my change to recover the orignial
> code.

oshima@ could you review my CL?

[oshima, 2015-11-17 23:18:09]

On 2015/11/17 02:00:49, joone wrote:
> On 2015/10/16 07:29:49, joone wrote:
> >
>
https://codereview.chromium.org/1130033003/diff/320001/ui/views/widget/native...
> > File ui/views/widget/native_widget_aura.cc (right):
> > 
> >
>
https://codereview.chromium.org/1130033003/diff/320001/ui/views/widget/native...
> > ui/views/widget/native_widget_aura.cc:487: }
> > On 2015/10/07 16:32:54, sadrul wrote:
> > > In earlier patchsets, the property was being set unconditionally. Can you
> > > explain why it is necessary to remove that change? Please get another
review
> > > from oshima@ for this as well.
> > 
> > I thought that we could remove this condition, but I saw that some test
cases
> > failed due to my change. So I just removed my change to recover the orignial
> > code.
> 
> oshima@ could you review my CL?

I need to refresh my memory. I'll review by EOD.

[oshima, 2015-11-18 19:07:17]

https://codereview.chromium.org/1130033003/diff/320001/ui/views/widget/native...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/1130033003/diff/320001/ui/views/widget/native...
ui/views/widget/native_widget_aura.cc:487: }
On 2015/10/16 07:29:49, joone wrote:
> On 2015/10/07 16:32:54, sadrul wrote:
> > In earlier patchsets, the property was being set unconditionally. Can you
> > explain why it is necessary to remove that change? Please get another review
> > from oshima@ for this as well.
> 
> I thought that we could remove this condition, but I saw that some test cases
> failed due to my change. So I just removed my change to recover the orignial
> code.

I'm looking into these failures now to see if we can eliminate the condition.

https://codereview.chromium.org/1130033003/diff/340001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/1130033003/diff/340001/ui/views/widget/widget...
ui/views/widget/widget.cc:635: if (IsFullscreen()) {
how about

ui::ShowState show_state =
   IsFullscreen() ? ui::SHOW_STATE_FULLSCREEN :
   (IsMinimized() ? ui::SHOW_STATE_MINIMIZED : saved_show_state_);
native_widget_->ShowWithWindowState(show_state);

[joone, 2015-11-19 06:32:01]

https://codereview.chromium.org/1130033003/diff/340001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/1130033003/diff/340001/ui/views/widget/widget...
ui/views/widget/widget.cc:635: if (IsFullscreen()) {
On 2015/11/18 19:07:16, oshima wrote:
> how about
> 
> ui::ShowState show_state =
>    IsFullscreen() ? ui::SHOW_STATE_FULLSCREEN :
>    (IsMinimized() ? ui::SHOW_STATE_MINIMIZED : saved_show_state_);
> native_widget_->ShowWithWindowState(show_state);
> 

This code looks better so I applied this change to the updated CL.
Thanks!

[joone, 2015-11-20 22:55:56]

oshima@ could you review the updated CL?

[oshima, 2015-11-20 23:16:39]

On 2015/11/20 22:55:56, joone wrote:
> oshima@ could you review the updated CL?

I found the issue and am trying to find a right fix now. Sorry for taking time.
(see https://codereview.chromium.org/1454143002)

[oshima, 2015-11-21 02:03:26]

On 2015/11/20 23:16:39, oshima wrote:
> On 2015/11/20 22:55:56, joone wrote:
> > oshima@ could you review the updated CL?
> 
> I found the issue and am trying to find a right fix now. Sorry for taking
time.
> (see https://codereview.chromium.org/1454143002)

Actually, my fix will be independent, so this CL lgtm.

[joone, 2015-11-21 04:43:21]

The CQ bit was checked by joone.hur@intel.com

[joone, 2015-11-21 04:43:21]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1130033003/#ps360001
(title: "apply the new condition code")

[commit-bot: I haz the power, 2015-11-21 04:43:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1130033003/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1130033003/360001

[commit-bot: I haz the power, 2015-11-21 04:53:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-21 04:54:00]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[joone, 2015-11-21 05:05:10]

joone.hur@intel.com changed reviewers:
+ rdevlin.cronin@chromium.org
- kalman@chromium.org

[joone, 2015-11-21 05:05:11]

sadrul@ devlin@ could you review this CL?

[Devlin, 2015-11-23 20:58:19]

extensions lgtm

[sadrul, 2015-11-25 20:44:32]

lgtm

https://codereview.chromium.org/1130033003/diff/360001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/1130033003/diff/360001/ui/views/widget/widget...
ui/views/widget/widget.cc:641: (IsMinimized() ? ui::SHOW_STATE_MINIMIZED :
saved_show_state_);
Don't need ()

[joone, 2015-11-26 15:49:36]

The CQ bit was checked by joone.hur@intel.com

[joone, 2015-11-26 15:49:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, sadrul@chromium.org, oshima@chromium.org,
kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1130033003/#ps380001
(title: "remove the parentheses in the inline if statement")

[joone, 2015-11-26 15:50:38]

Thanks!

https://codereview.chromium.org/1130033003/diff/360001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/1130033003/diff/360001/ui/views/widget/widget...
ui/views/widget/widget.cc:641: (IsMinimized() ? ui::SHOW_STATE_MINIMIZED :
saved_show_state_);
On 2015/11/25 20:44:32, sadrul wrote:
> Don't need ()

Done.

[commit-bot: I haz the power, 2015-11-26 15:56:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1130033003/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1130033003/380001

[commit-bot: I haz the power, 2015-11-26 16:06:11]

Description was changed from

==========
Make the minimize state work when creating a chrome window in CrOS

This CL makes SHOW_STATE_MINIMIZE state work when creating a
chrome window in CrOS, so we don't need to call Minimize()
manually in tabs_api.cc

BUG=473228
TEST=NativeWidgetAuraTest.CreateMinimized
==========

to

==========
Make the minimize state work when creating a chrome window in CrOS

This CL makes SHOW_STATE_MINIMIZE state work when creating a
chrome window in CrOS, so we don't need to call Minimize()
manually in tabs_api.cc

BUG=473228
TEST=NativeWidgetAuraTest.CreateMinimized
==========

[commit-bot: I haz the power, 2015-11-26 16:06:12]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2015-11-26 16:07:55]

Description was changed from

==========
Make the minimize state work when creating a chrome window in CrOS

This CL makes SHOW_STATE_MINIMIZE state work when creating a
chrome window in CrOS, so we don't need to call Minimize()
manually in tabs_api.cc

BUG=473228
TEST=NativeWidgetAuraTest.CreateMinimized
==========

to

==========
Make the minimize state work when creating a chrome window in CrOS

This CL makes SHOW_STATE_MINIMIZE state work when creating a
chrome window in CrOS, so we don't need to call Minimize()
manually in tabs_api.cc

BUG=473228
TEST=NativeWidgetAuraTest.CreateMinimized

Committed: https://crrev.com/d5d6762c5539bc7d6098588323a16a24826e1149
Cr-Commit-Position: refs/heads/master@{#361898}
==========

[commit-bot: I haz the power, 2015-11-26 16:07:57]

Patchset 20 (id:??) landed as
https://crrev.com/d5d6762c5539bc7d6098588323a16a24826e1149
Cr-Commit-Position: refs/heads/master@{#361898}