Show overview mode button in TouchView with open modal window

Show overview mode button in TouchView with open modal window

Currently, the overview mode button is only shown within
maximize mode when all of the following conditions hold:

  (1) The user is logged in, and
  (2) The screen is not locked, and
  (3) The user is not in kiosk mode, and
  (4) There are no system modal windows open.

These are the same four conditions for when overview mode may
be engaged by the user. This CL drops condition (4) for
showing the overview mode button in maximize mode, but does
not change the conditions for being able to engage overview
mode. In other words, the overview mode button will now show
for the user when there are modal windows open, but tapping
on this button will remain a no-op.

BUG=482591
TEST=OverviewButtonTrayTest.VisibilityChangesForSystemModalWindow

[tdanderson, 2015-05-01 16:13:39]

tdanderson@chromium.org changed reviewers:
+ bruthig@chromium.org, jonross@chromium.org

[tdanderson, 2015-05-01 16:13:40]

Hey Ben and Jon, mind taking a look?

[jonross, 2015-05-01 17:30:15]

https://codereview.chromium.org/1114383002/diff/1/ash/system/overview/overvie...
File ash/system/overview/overview_button_tray_unittest.cc (right):

https://codereview.chromium.org/1114383002/diff/1/ash/system/overview/overvie...
ash/system/overview/overview_button_tray_unittest.cc:22: #include
"ui/aura/client/aura_constants.h"
Necessary?

https://codereview.chromium.org/1114383002/diff/1/ash/system/overview/overvie...
ash/system/overview/overview_button_tray_unittest.cc:163:
->EnableMaximizeModeWindowManager(false);
Check afterwards to make sure visibility toggling works in both directions?

https://codereview.chromium.org/1114383002/diff/1/ash/test/ash_test_base.cc
File ash/test/ash_test_base.cc (right):

https://codereview.chromium.org/1114383002/diff/1/ash/test/ash_test_base.cc#n...
ash/test/ash_test_base.cc:293: aura::Window*
AshTestBase::CreateTestModalWindowInShellWithDelegateAndType(
Can you call CreateTestWindowInShellWithDelegateAndType and then apply the modal
property afterwards?

If not, can you also add the refactoring TODO here to help explain the duplicate
code?

https://codereview.chromium.org/1114383002/diff/1/ash/wm/dock/docked_window_r...
File ash/wm/dock/docked_window_resizer_unittest.cc (right):

https://codereview.chromium.org/1114383002/diff/1/ash/wm/dock/docked_window_r...
ash/wm/dock/docked_window_resizer_unittest.cc:92: &delegate_,
ui::wm::WINDOW_TYPE_NORMAL, 0, bounds);
Awesome!

[bruthig, 2015-05-01 17:43:35]

lgtm with nit

https://codereview.chromium.org/1114383002/diff/1/ash/system/overview/overvie...
File ash/system/overview/overview_button_tray.h (right):

https://codereview.chromium.org/1114383002/diff/1/ash/system/overview/overvie...
ash/system/overview/overview_button_tray.h:55: // Shows or hides the overview
button. The button be shown when the user is
nit: missing "will"?  "The button be shown..."

[tdanderson, 2015-05-01 18:07:31]

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

[tdanderson, 2015-05-01 18:07:32]

Thanks for the comments. @oshima, mind taking a look?

https://codereview.chromium.org/1114383002/diff/1/ash/system/overview/overvie...
File ash/system/overview/overview_button_tray.h (right):

https://codereview.chromium.org/1114383002/diff/1/ash/system/overview/overvie...
ash/system/overview/overview_button_tray.h:55: // Shows or hides the overview
button. The button be shown when the user is
On 2015/05/01 17:43:34, bruthig wrote:
> nit: missing "will"?  "The button be shown..."

Done.

https://codereview.chromium.org/1114383002/diff/1/ash/system/overview/overvie...
File ash/system/overview/overview_button_tray_unittest.cc (right):

https://codereview.chromium.org/1114383002/diff/1/ash/system/overview/overvie...
ash/system/overview/overview_button_tray_unittest.cc:22: #include
"ui/aura/client/aura_constants.h"
On 2015/05/01 17:30:15, jonross wrote:
> Necessary?

This was part of a quick hack and I meant to remove it. It's gone now.

https://codereview.chromium.org/1114383002/diff/1/ash/system/overview/overvie...
ash/system/overview/overview_button_tray_unittest.cc:163:
->EnableMaximizeModeWindowManager(false);
On 2015/05/01 17:30:15, jonross wrote:
> Check afterwards to make sure visibility toggling works in both directions?

Done.

https://codereview.chromium.org/1114383002/diff/1/ash/test/ash_test_base.cc
File ash/test/ash_test_base.cc (right):

https://codereview.chromium.org/1114383002/diff/1/ash/test/ash_test_base.cc#n...
ash/test/ash_test_base.cc:293: aura::Window*
AshTestBase::CreateTestModalWindowInShellWithDelegateAndType(
On 2015/05/01 17:30:15, jonross wrote:
> Can you call CreateTestWindowInShellWithDelegateAndType and then apply the
modal
> property afterwards?
> 
> If not, can you also add the refactoring TODO here to help explain the
duplicate
> code?

I tried that but it doesn't work. The system modal property must be set on the
window prior to giving the window a parent, otherwise it will not end up as a
descendant of the modal container. I will add a comment.

[jonross, 2015-05-01 18:20:39]

On 2015/05/01 18:07:32, tdanderson wrote:
> Thanks for the comments. @oshima, mind taking a look?
> 
>
https://codereview.chromium.org/1114383002/diff/1/ash/system/overview/overvie...
> File ash/system/overview/overview_button_tray.h (right):
> 
>
https://codereview.chromium.org/1114383002/diff/1/ash/system/overview/overvie...
> ash/system/overview/overview_button_tray.h:55: // Shows or hides the overview
> button. The button be shown when the user is
> On 2015/05/01 17:43:34, bruthig wrote:
> > nit: missing "will"?  "The button be shown..."
> 
> Done.
> 
>
https://codereview.chromium.org/1114383002/diff/1/ash/system/overview/overvie...
> File ash/system/overview/overview_button_tray_unittest.cc (right):
> 
>
https://codereview.chromium.org/1114383002/diff/1/ash/system/overview/overvie...
> ash/system/overview/overview_button_tray_unittest.cc:22: #include
> "ui/aura/client/aura_constants.h"
> On 2015/05/01 17:30:15, jonross wrote:
> > Necessary?
> 
> This was part of a quick hack and I meant to remove it. It's gone now.
> 
>
https://codereview.chromium.org/1114383002/diff/1/ash/system/overview/overvie...
> ash/system/overview/overview_button_tray_unittest.cc:163:
> ->EnableMaximizeModeWindowManager(false);
> On 2015/05/01 17:30:15, jonross wrote:
> > Check afterwards to make sure visibility toggling works in both directions?
> 
> Done.
> 
> https://codereview.chromium.org/1114383002/diff/1/ash/test/ash_test_base.cc
> File ash/test/ash_test_base.cc (right):
> 
>
https://codereview.chromium.org/1114383002/diff/1/ash/test/ash_test_base.cc#n...
> ash/test/ash_test_base.cc:293: aura::Window*
> AshTestBase::CreateTestModalWindowInShellWithDelegateAndType(
> On 2015/05/01 17:30:15, jonross wrote:
> > Can you call CreateTestWindowInShellWithDelegateAndType and then apply the
> modal
> > property afterwards?
> > 
> > If not, can you also add the refactoring TODO here to help explain the
> duplicate
> > code?
> 
> I tried that but it doesn't work. The system modal property must be set on the
> window prior to giving the window a parent, otherwise it will not end up as a
> descendant of the modal container. I will add a comment.

LGTM

[jonross, 2015-05-01 18:29:52]

https://codereview.chromium.org/1114383002/diff/1/ash/test/ash_test_base.cc
File ash/test/ash_test_base.cc (right):

https://codereview.chromium.org/1114383002/diff/1/ash/test/ash_test_base.cc#n...
ash/test/ash_test_base.cc:293: aura::Window*
AshTestBase::CreateTestModalWindowInShellWithDelegateAndType(
On 2015/05/01 18:07:32, tdanderson wrote:
> On 2015/05/01 17:30:15, jonross wrote:
> > Can you call CreateTestWindowInShellWithDelegateAndType and then apply the
> modal
> > property afterwards?
> > 
> > If not, can you also add the refactoring TODO here to help explain the
> duplicate
> > code?
> 
> I tried that but it doesn't work. The system modal property must be set on the
> window prior to giving the window a parent, otherwise it will not end up as a
> descendant of the modal container. I will add a comment.

Acknowledged.

[jonross, 2015-05-01 18:29:53]

https://codereview.chromium.org/1114383002/diff/1/ash/test/ash_test_base.cc
File ash/test/ash_test_base.cc (right):

https://codereview.chromium.org/1114383002/diff/1/ash/test/ash_test_base.cc#n...
ash/test/ash_test_base.cc:293: aura::Window*
AshTestBase::CreateTestModalWindowInShellWithDelegateAndType(
On 2015/05/01 18:07:32, tdanderson wrote:
> On 2015/05/01 17:30:15, jonross wrote:
> > Can you call CreateTestWindowInShellWithDelegateAndType and then apply the
> modal
> > property afterwards?
> > 
> > If not, can you also add the refactoring TODO here to help explain the
> duplicate
> > code?
> 
> I tried that but it doesn't work. The system modal property must be set on the
> window prior to giving the window a parent, otherwise it will not end up as a
> descendant of the modal container. I will add a comment.

Acknowledged.

[oshima, 2015-05-04 19:35:36]

https://codereview.chromium.org/1114383002/diff/20001/ash/test/ash_test_base.cc
File ash/test/ash_test_base.cc (right):

https://codereview.chromium.org/1114383002/diff/20001/ash/test/ash_test_base....
ash/test/ash_test_base.cc:318: aura::client::ParentWindowWithContext(window,
root, bounds);
Window::SetBoundsInScreen ?

[tdanderson, 2015-05-06 16:36:05]

@oshima, can you please take another look?

https://chromiumcodereview.appspot.com/1114383002/diff/20001/ash/test/ash_tes...
File ash/test/ash_test_base.cc (right):

https://chromiumcodereview.appspot.com/1114383002/diff/20001/ash/test/ash_tes...
ash/test/ash_test_base.cc:318: aura::client::ParentWindowWithContext(window,
root, bounds);
On 2015/05/04 19:35:36, oshima wrote:
> Window::SetBoundsInScreen ?

I think SetBounds() here is correct - it's consistent with
AshTestBase::CreateTestWindowInShellWithDelegateAndType().

[oshima, 2015-05-06 16:44:32]

On 2015/05/06 16:36:05, tdanderson wrote:
> @oshima, can you please take another look?
> 
>
https://chromiumcodereview.appspot.com/1114383002/diff/20001/ash/test/ash_tes...
> File ash/test/ash_test_base.cc (right):
> 
>
https://chromiumcodereview.appspot.com/1114383002/diff/20001/ash/test/ash_tes...
> ash/test/ash_test_base.cc:318: aura::client::ParentWindowWithContext(window,
> root, bounds);
> On 2015/05/04 19:35:36, oshima wrote:
> > Window::SetBoundsInScreen ?
> 
> I think SetBounds() here is correct - it's consistent with
> AshTestBase::CreateTestWindowInShellWithDelegateAndType().

That code predates the SetBoundsInScreeen, and it should use SetBoundsInScreen
too.
SetBoundsInScreen should do what these code is doing.

[jonross, 2015-06-04 17:56:41]

On 2015/05/06 16:44:32, oshima wrote:
> On 2015/05/06 16:36:05, tdanderson wrote:
> > @oshima, can you please take another look?
> > 
> >
>
https://chromiumcodereview.appspot.com/1114383002/diff/20001/ash/test/ash_tes...
> > File ash/test/ash_test_base.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/1114383002/diff/20001/ash/test/ash_tes...
> > ash/test/ash_test_base.cc:318: aura::client::ParentWindowWithContext(window,
> > root, bounds);
> > On 2015/05/04 19:35:36, oshima wrote:
> > > Window::SetBoundsInScreen ?
> > 
> > I think SetBounds() here is correct - it's consistent with
> > AshTestBase::CreateTestWindowInShellWithDelegateAndType().
> 
> That code predates the SetBoundsInScreeen, and it should use SetBoundsInScreen
> too.
> SetBoundsInScreen should do what these code is doing.

https://codereview.chromium.org/1149833006/ is landing a fix which also
addressed this issue.

tdanderson@ feel free to close this review.

[tdanderson, 2015-06-04 17:58:34]

On 2015/06/04 17:56:41, jonross wrote:
> On 2015/05/06 16:44:32, oshima wrote:
> > On 2015/05/06 16:36:05, tdanderson wrote:
> > > @oshima, can you please take another look?
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/1114383002/diff/20001/ash/test/ash_tes...
> > > File ash/test/ash_test_base.cc (right):
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/1114383002/diff/20001/ash/test/ash_tes...
> > > ash/test/ash_test_base.cc:318:
aura::client::ParentWindowWithContext(window,
> > > root, bounds);
> > > On 2015/05/04 19:35:36, oshima wrote:
> > > > Window::SetBoundsInScreen ?
> > > 
> > > I think SetBounds() here is correct - it's consistent with
> > > AshTestBase::CreateTestWindowInShellWithDelegateAndType().
> > 
> > That code predates the SetBoundsInScreeen, and it should use
SetBoundsInScreen
> > too.
> > SetBoundsInScreen should do what these code is doing.
> 
> https://codereview.chromium.org/1149833006/ is landing a fix which also
> addressed this issue.
> 
> tdanderson@ feel free to close this review.

Great, thanks for your fix, Jon! Closing this out.