Unify window fullscreen and minimizing implementation

Unify window fullscreen and minimizing implementation.

Currently, we have several places which implement the
toggle fullscreen logic. To avoid duplicate code in
WmWindow::SetFullscreen(), DesktopWindowTreeHostMus::SetFullscreen()
and NativeWidgetAura::SetFullscreen(), we abstract
the common logic wm::SetWindowFullscreen() in //ui/wm,
and use it in those three functions.


BUG=670496
Review-Url: https://codereview.chromium.org/2625113004
Cr-Commit-Position: refs/heads/master@{#445876}
Committed: https://chromium.googlesource.com/chromium/src/+/95e5e9f467769c8f0f22577eefa7267b14fbbdbd

[Peng, 2017-01-13 19:42:25]

Description was changed from

==========
Unify window fullscreen and minimizing implementation

BUG=
==========

to

==========
Unify window fullscreen and minimizing implementation

Add a temporary window show state SHOW_STATE_PREVIOUS
which will be used to restore previous show state from
fullscreen and minimized states.


BUG=None
==========

[Peng, 2017-01-13 19:45:51]

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

[Peng, 2017-01-13 19:45:52]

Hi Oshima, PTAL. Thanks.

[oshima, 2017-01-18 01:43:36]

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

[oshima, 2017-01-18 01:43:37]

The approach looks reasonable, but sky@ should be the right reviewer for this.

Alternative approach is to use NORMAL state, and interpret it as "previous" if
it's switching from minimize/fullscreen to normal, but this is probably more
readable and less error prone.

[sky, 2017-01-18 16:48:16]

Can you elaborate on the bug you're trying to solve? You don't have a BUG= and
the description isn't particularly detailed.

https://codereview.chromium.org/2625113004/diff/20001/ui/base/ui_base_types.h
File ui/base/ui_base_types.h (right):

https://codereview.chromium.org/2625113004/diff/20001/ui/base/ui_base_types.h...
ui/base/ui_base_types.h:24: SHOW_STATE_PREVIOUS   = 7,  // a temporary state for
restoring the previous
I would prefer not to expose this as a real state. It means everyone has to deal
with it, e.g.:
DCHECK_NE(show_state, ui::SHOW_STATE_PREVIOUS);

[Peng, 2017-01-18 16:56:53]

Description was changed from

==========
Unify window fullscreen and minimizing implementation

Add a temporary window show state SHOW_STATE_PREVIOUS
which will be used to restore previous show state from
fullscreen and minimized states.


BUG=None
==========

to

==========
Unify window fullscreen and minimizing implementation

Add a temporary window show state SHOW_STATE_PREVIOUS
which will be used to restore previous show state from
fullscreen and minimized states.


BUG=670496
==========

[Peng, 2017-01-18 18:57:59]

Description was changed from

==========
Unify window fullscreen and minimizing implementation

Add a temporary window show state SHOW_STATE_PREVIOUS
which will be used to restore previous show state from
fullscreen and minimized states.


BUG=670496
==========

to

==========
Unify window fullscreen and minimizing implementation.

Currently, we have several places which implement the
toggle fullscreen logic. To avoid duplicate code in
WmWindow::SetFullscreen(), DesktopWindowTreeHostMus::SetFullscreen()
and NativeWidgetAura::SetFullscreen(), A temporary window show state
SHOW_STATE_PREVIOUS is added for restoring show state
from fullscreen and minimized show states.


BUG=670496
==========

[Peng, 2017-01-18 18:58:20]

Description was changed from

==========
Unify window fullscreen and minimizing implementation.

Currently, we have several places which implement the
toggle fullscreen logic. To avoid duplicate code in
WmWindow::SetFullscreen(), DesktopWindowTreeHostMus::SetFullscreen()
and NativeWidgetAura::SetFullscreen(), A temporary window show state
SHOW_STATE_PREVIOUS is added for restoring show state
from fullscreen and minimized show states.


BUG=670496
==========

to

==========
Unify window fullscreen and minimizing implementation.

Currently, we have several places which implement the
toggle fullscreen logic. To avoid duplicate code in
WmWindow::SetFullscreen(), DesktopWindowTreeHostMus::SetFullscreen()
and NativeWidgetAura::SetFullscreen(), A temporary 
window show state SHOW_STATE_PREVIOUS is added for
restoring show state from fullscreen and minimized
show states.


BUG=670496
==========

[Peng, 2017-01-18 19:02:34]

On 2017/01/18 16:48:16, sky wrote:
> Can you elaborate on the bug you're trying to solve? You don't have a BUG= and
> the description isn't particularly detailed.

I updated the description. This CL is a follow up CL for CL
https://codereview.chromium.org/2622053004/ which
modifies the default behavior when exiting the fullscreen. They are necessary
for getting
rid of ash dependence from exo.

> 
> https://codereview.chromium.org/2625113004/diff/20001/ui/base/ui_base_types.h
> File ui/base/ui_base_types.h (right):
> 
>
https://codereview.chromium.org/2625113004/diff/20001/ui/base/ui_base_types.h...
> ui/base/ui_base_types.h:24: SHOW_STATE_PREVIOUS   = 7,  // a temporary state
for
> restoring the previous
> I would prefer not to expose this as a real state. It means everyone has to
deal
> with it, e.g.:
> DCHECK_NE(show_state, ui::SHOW_STATE_PREVIOUS);

If we don't add this temporary show state, probably we have to have duplicate
exiting fullscreen
logic in several places. WDYT and suggestion? Thanks.

[sky, 2017-01-18 21:55:39]

On 2017/01/18 19:02:34, Peng wrote:
> On 2017/01/18 16:48:16, sky wrote:
> > Can you elaborate on the bug you're trying to solve? You don't have a BUG=
and
> > the description isn't particularly detailed.
> 
> I updated the description. This CL is a follow up CL for CL
> https://codereview.chromium.org/2622053004/ which
> modifies the default behavior when exiting the fullscreen. They are necessary
> for getting
> rid of ash dependence from exo.
> 
> > 
> >
https://codereview.chromium.org/2625113004/diff/20001/ui/base/ui_base_types.h
> > File ui/base/ui_base_types.h (right):
> > 
> >
>
https://codereview.chromium.org/2625113004/diff/20001/ui/base/ui_base_types.h...
> > ui/base/ui_base_types.h:24: SHOW_STATE_PREVIOUS   = 7,  // a temporary state
> for
> > restoring the previous
> > I would prefer not to expose this as a real state. It means everyone has to
> deal
> > with it, e.g.:
> > DCHECK_NE(show_state, ui::SHOW_STATE_PREVIOUS);
> 
> If we don't add this temporary show state, probably we have to have duplicate
> exiting fullscreen
> logic in several places. WDYT and suggestion? Thanks.

Thanks for updating the description. Can't you refactor the logic into a common
place? Introducing a temporary state to the possible states is confusing and
error prone. We should definitely avoid that.

[Peng, 2017-01-18 22:13:23]

On 2017/01/18 21:55:39, sky wrote:
> On 2017/01/18 19:02:34, Peng wrote:
> > On 2017/01/18 16:48:16, sky wrote:
> > > Can you elaborate on the bug you're trying to solve? You don't have a BUG=
> and
> > > the description isn't particularly detailed.
> > 
> > I updated the description. This CL is a follow up CL for CL
> > https://codereview.chromium.org/2622053004/ which
> > modifies the default behavior when exiting the fullscreen. They are
necessary
> > for getting
> > rid of ash dependence from exo.
> > 
> > > 
> > >
> https://codereview.chromium.org/2625113004/diff/20001/ui/base/ui_base_types.h
> > > File ui/base/ui_base_types.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2625113004/diff/20001/ui/base/ui_base_types.h...
> > > ui/base/ui_base_types.h:24: SHOW_STATE_PREVIOUS   = 7,  // a temporary
state
> > for
> > > restoring the previous
> > > I would prefer not to expose this as a real state. It means everyone has
to
> > deal
> > > with it, e.g.:
> > > DCHECK_NE(show_state, ui::SHOW_STATE_PREVIOUS);
> > 
> > If we don't add this temporary show state, probably we have to have
duplicate
> > exiting fullscreen
> > logic in several places. WDYT and suggestion? Thanks.
> 
> Thanks for updating the description. Can't you refactor the logic into a
common
> place? Introducing a temporary state to the possible states is confusing and
> error prone. We should definitely avoid that.

The logic is closer to ash::WmWindow. Seems the logic should be in
ash/common/wm.
But I don't see any code in /ui/views uses ash. Probably ash/common/wm is not
the place.
And seems /ui/views is not a good place too. Can you give me a suggestion?

BTW, I have another idea to fix this issue by removing
DesktopWindowTreeHostMus::SetFullscreen()
and NativeWidgetAura::SetFullscreen(), and let call sites of those two method to
use
ash::wm::WmWindow::SetFullscreen() directlly. WDYT?

[sky, 2017-01-18 23:27:28]

On Wed, Jan 18, 2017 at 2:13 PM,  <penghuang@chromium.org> wrote:
> On 2017/01/18 21:55:39, sky wrote:
>> On 2017/01/18 19:02:34, Peng wrote:
>> > On 2017/01/18 16:48:16, sky wrote:
>> > > Can you elaborate on the bug you're trying to solve? You don't have a
>> > > BUG=
>> and
>> > > the description isn't particularly detailed.
>> >
>> > I updated the description. This CL is a follow up CL for CL
>> > https://codereview.chromium.org/2622053004/ which
>> > modifies the default behavior when exiting the fullscreen. They are
> necessary
>> > for getting
>> > rid of ash dependence from exo.
>> >
>> > >
>> > >
>>
>>
https://codereview.chromium.org/2625113004/diff/20001/ui/base/ui_base_types.h
>> > > File ui/base/ui_base_types.h (right):
>> > >
>> > >
>> >
>>
>
https://codereview.chromium.org/2625113004/diff/20001/ui/base/ui_base_types.h...
>> > > ui/base/ui_base_types.h:24: SHOW_STATE_PREVIOUS = 7, // a temporary
> state
>> > for
>> > > restoring the previous
>> > > I would prefer not to expose this as a real state. It means everyone
>> > > has
> to
>> > deal
>> > > with it, e.g.:
>> > > DCHECK_NE(show_state, ui::SHOW_STATE_PREVIOUS);
>> >
>> > If we don't add this temporary show state, probably we have to have
> duplicate
>> > exiting fullscreen
>> > logic in several places. WDYT and suggestion? Thanks.
>>
>> Thanks for updating the description. Can't you refactor the logic into a
> common
>> place? Introducing a temporary state to the possible states is confusing
>> and
>> error prone. We should definitely avoid that.
>
> The logic is closer to ash::WmWindow. Seems the logic should be in
> ash/common/wm.
> But I don't see any code in /ui/views uses ash. Probably ash/common/wm is
> not
> the place.
> And seems /ui/views is not a good place too. Can you give me a suggestion?

Can you elaborate on what code you want to share? Is it just the new
code in WmWindow::OnWindowPropertyChanged? It seems like that would be
hard to share given it calls into functions in WmWindow.

ash depends upon aura and views.
views depends upon aura, but not ash
aura doesn't depend upon either.

One place we've moved some code used by both ash and views is ui/wm.
There are utility functions there and is likely the best place for
sharing code between ash and views.

  -Scott
>
> BTW, I have another idea to fix this issue by removing
> DesktopWindowTreeHostMus::SetFullscreen()
> and NativeWidgetAura::SetFullscreen(), and let call sites of those two
> method to
> use
> ash::wm::WmWindow::SetFullscreen() directlly. WDYT?
>
>
> https://codereview.chromium.org/2625113004/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peng, 2017-01-23 19:47:03]

Thanks for your suggestion. I updated CL. PTAL. Thanks.

On 2017/01/18 23:27:28, sky wrote:
> On Wed, Jan 18, 2017 at 2:13 PM,  <mailto:penghuang@chromium.org> wrote:
> > On 2017/01/18 21:55:39, sky wrote:
> >> On 2017/01/18 19:02:34, Peng wrote:
> >> > On 2017/01/18 16:48:16, sky wrote:
> >> > > Can you elaborate on the bug you're trying to solve? You don't have a
> >> > > BUG=
> >> and
> >> > > the description isn't particularly detailed.
> >> >
> >> > I updated the description. This CL is a follow up CL for CL
> >> > https://codereview.chromium.org/2622053004/ which
> >> > modifies the default behavior when exiting the fullscreen. They are
> > necessary
> >> > for getting
> >> > rid of ash dependence from exo.
> >> >
> >> > >
> >> > >
> >>
> >>
> https://codereview.chromium.org/2625113004/diff/20001/ui/base/ui_base_types.h
> >> > > File ui/base/ui_base_types.h (right):
> >> > >
> >> > >
> >> >
> >>
> >
>
https://codereview.chromium.org/2625113004/diff/20001/ui/base/ui_base_types.h...
> >> > > ui/base/ui_base_types.h:24: SHOW_STATE_PREVIOUS = 7, // a temporary
> > state
> >> > for
> >> > > restoring the previous
> >> > > I would prefer not to expose this as a real state. It means everyone
> >> > > has
> > to
> >> > deal
> >> > > with it, e.g.:
> >> > > DCHECK_NE(show_state, ui::SHOW_STATE_PREVIOUS);
> >> >
> >> > If we don't add this temporary show state, probably we have to have
> > duplicate
> >> > exiting fullscreen
> >> > logic in several places. WDYT and suggestion? Thanks.
> >>
> >> Thanks for updating the description. Can't you refactor the logic into a
> > common
> >> place? Introducing a temporary state to the possible states is confusing
> >> and
> >> error prone. We should definitely avoid that.
> >
> > The logic is closer to ash::WmWindow. Seems the logic should be in
> > ash/common/wm.
> > But I don't see any code in /ui/views uses ash. Probably ash/common/wm is
> > not
> > the place.
> > And seems /ui/views is not a good place too. Can you give me a suggestion?
> 
> Can you elaborate on what code you want to share? Is it just the new
> code in WmWindow::OnWindowPropertyChanged? It seems like that would be
> hard to share given it calls into functions in WmWindow.
> 
> ash depends upon aura and views.
> views depends upon aura, but not ash
> aura doesn't depend upon either.
> 
> One place we've moved some code used by both ash and views is ui/wm.
> There are utility functions there and is likely the best place for
> sharing code between ash and views.
> 
>   -Scott

> >
> > BTW, I have another idea to fix this issue by removing
> > DesktopWindowTreeHostMus::SetFullscreen()
> > and NativeWidgetAura::SetFullscreen(), and let call sites of those two
> > method to
> > use
> > ash::wm::WmWindow::SetFullscreen() directlly. WDYT?
> >
> >
> > https://codereview.chromium.org/2625113004/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
>

[Peng, 2017-01-23 19:58:33]

The CQ bit was checked by penghuang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 19:59:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peng, 2017-01-23 20:03:36]

Description was changed from

==========
Unify window fullscreen and minimizing implementation.

Currently, we have several places which implement the
toggle fullscreen logic. To avoid duplicate code in
WmWindow::SetFullscreen(), DesktopWindowTreeHostMus::SetFullscreen()
and NativeWidgetAura::SetFullscreen(), A temporary 
window show state SHOW_STATE_PREVIOUS is added for
restoring show state from fullscreen and minimized
show states.


BUG=670496
==========

to

==========
Unify window fullscreen and minimizing implementation.

Currently, we have several places which implement the
toggle fullscreen logic. To avoid duplicate code in
WmWindow::SetFullscreen(), DesktopWindowTreeHostMus::SetFullscreen()
and NativeWidgetAura::SetFullscreen(), we abstract
the common logic wm::SetWindowFullscreen() in //ui/wm,
and use it in those three functions.


BUG=670496
==========

[commit-bot: I haz the power, 2017-01-23 20:10:55]

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

[commit-bot: I haz the power, 2017-01-23 20:10:56]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sky, 2017-01-23 20:13:34]

https://codereview.chromium.org/2625113004/diff/60001/ash/wm/window_util.cc
File ash/wm/window_util.cc (right):

https://codereview.chromium.org/2625113004/diff/60001/ash/wm/window_util.cc#n...
ash/wm/window_util.cc:58: void SetWindowFullscreen(aura::Window* window, bool
fullscreen) {
Can't callers call through to ::wm::SetWindowFullscreen() directly?

[Peng, 2017-01-23 20:23:46]

https://codereview.chromium.org/2625113004/diff/60001/ash/wm/window_util.cc
File ash/wm/window_util.cc (right):

https://codereview.chromium.org/2625113004/diff/60001/ash/wm/window_util.cc#n...
ash/wm/window_util.cc:58: void SetWindowFullscreen(aura::Window* window, bool
fullscreen) {
On 2017/01/23 20:13:34, sky wrote:
> Can't callers call through to ::wm::SetWindowFullscreen() directly?

Done.

[Peng, 2017-01-23 20:24:19]

The CQ bit was checked by penghuang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 20:24:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 21:09:59]

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

[commit-bot: I haz the power, 2017-01-23 21:10:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2017-01-23 21:12:19]

This looks good. Could you add test coverage of kPreFullscreenShowStateKey
though? I don't see it in this patch.

[Peng, 2017-01-23 21:24:36]

On 2017/01/23 21:12:19, sky wrote:
> This looks good. Could you add test coverage of kPreFullscreenShowStateKey
> though? I don't see it in this patch.


Unittests for kPreFullscreenShowStateKey are included in the previous patch.

https://codereview.chromium.org/2622053004/diff/140001/ash/wm/window_state_un...

[sky, 2017-01-24 00:28:10]

LGTM

[Peng, 2017-01-24 20:25:42]

The CQ bit was checked by penghuang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 20:26:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2017-01-24 21:56:34]

LGTM with the following fixed.

https://codereview.chromium.org/2625113004/diff/100001/ui/wm/core/window_util.cc
File ui/wm/core/window_util.cc (right):

https://codereview.chromium.org/2625113004/diff/100001/ui/wm/core/window_util...
ui/wm/core/window_util.cc:90: auto current_state =
window->GetProperty(aura::client::kShowStateKey);
It isn't obvious what the type is here, please use a type.
And name it current_show_state.

https://codereview.chromium.org/2625113004/diff/100001/ui/wm/core/window_util...
ui/wm/core/window_util.cc:97: auto pre_state = current_state;
pre_show_state.

https://codereview.chromium.org/2625113004/diff/100001/ui/wm/core/window_util...
ui/wm/core/window_util.cc:108: auto pre_fullscreen_state =
pre_fullscreen_show_state. And same comment about auto.

[commit-bot: I haz the power, 2017-01-24 22:09:03]

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

[commit-bot: I haz the power, 2017-01-24 22:09:04]

Dry run: This issue passed the CQ dry run.

[Peng, 2017-01-24 22:13:03]

https://codereview.chromium.org/2625113004/diff/100001/ui/wm/core/window_util.cc
File ui/wm/core/window_util.cc (right):

https://codereview.chromium.org/2625113004/diff/100001/ui/wm/core/window_util...
ui/wm/core/window_util.cc:90: auto current_state =
window->GetProperty(aura::client::kShowStateKey);
On 2017/01/24 21:56:33, sky wrote:
> It isn't obvious what the type is here, please use a type.
> And name it current_show_state.

Done.

https://codereview.chromium.org/2625113004/diff/100001/ui/wm/core/window_util...
ui/wm/core/window_util.cc:97: auto pre_state = current_state;
On 2017/01/24 21:56:33, sky wrote:
> pre_show_state.

Done.

https://codereview.chromium.org/2625113004/diff/100001/ui/wm/core/window_util...
ui/wm/core/window_util.cc:108: auto pre_fullscreen_state =
On 2017/01/24 21:56:33, sky wrote:
> pre_fullscreen_show_state. And same comment about auto.

Done.

[Peng, 2017-01-24 22:13:24]

The CQ bit was checked by penghuang@chromium.org

[Peng, 2017-01-24 22:13:24]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2625113004/#ps120001
(title: "Fix review issue")

[commit-bot: I haz the power, 2017-01-24 22:13:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 00:34:36]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1485296004264720,
"parent_rev": "1cefc6fcc6a93d34bced3d8f85504cc149977a6b", "commit_rev":
"95e5e9f467769c8f0f22577eefa7267b14fbbdbd"}

[commit-bot: I haz the power, 2017-01-25 00:35:08]

Description was changed from

==========
Unify window fullscreen and minimizing implementation.

Currently, we have several places which implement the
toggle fullscreen logic. To avoid duplicate code in
WmWindow::SetFullscreen(), DesktopWindowTreeHostMus::SetFullscreen()
and NativeWidgetAura::SetFullscreen(), we abstract
the common logic wm::SetWindowFullscreen() in //ui/wm,
and use it in those three functions.


BUG=670496
==========

to

==========
Unify window fullscreen and minimizing implementation.

Currently, we have several places which implement the
toggle fullscreen logic. To avoid duplicate code in
WmWindow::SetFullscreen(), DesktopWindowTreeHostMus::SetFullscreen()
and NativeWidgetAura::SetFullscreen(), we abstract
the common logic wm::SetWindowFullscreen() in //ui/wm,
and use it in those three functions.


BUG=670496

Review-Url: https://codereview.chromium.org/2625113004
Cr-Commit-Position: refs/heads/master@{#445876}
Committed:
https://chromium.googlesource.com/chromium/src/+/95e5e9f467769c8f0f22577eefa7...
==========

[commit-bot: I haz the power, 2017-01-25 00:35:09]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/95e5e9f467769c8f0f22577eefa7...