Make system modal check more strict

Make system modal check more strict

* Take visibility into account
* Make it system  modal only when MODAL_TYPE_SYSTEM is set.
  - DCHECK if the modal is set to other modal type.

BUG=620406
TEST=covered by unittest

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

[oshima, 2016-06-25 21:27:27]

Description was changed from

==========
Update modality when visibility is changed

BUG=620406
TEST=covered by unittest
==========

to

==========
Make system modal check more strict

* Take visibility into account
* Make it system  modal only when MODAL_TYPE_SYSTEM is set.
  - DCHECK if the modal is set to other modal type.

BUG=620406
TEST=covered by unittest
==========

[oshima, 2016-06-25 21:27:27]

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

[oshima, 2016-06-25 21:27:40]

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

[commit-bot: I haz the power, 2016-06-25 21:27:53]

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, 2016-06-25 22:10:49]

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

[commit-bot: I haz the power, 2016-06-25 22:10:49]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-06-27 00:33:43]

LGTM with some optional suggestions

https://codereview.chromium.org/2098183002/diff/1/ash/wm/system_modal_contain...
File ash/wm/system_modal_container_layout_manager.cc (right):

https://codereview.chromium.org/2098183002/diff/1/ash/wm/system_modal_contain...
ash/wm/system_modal_container_layout_manager.cc:61: // DCHECK to avoid simple
mistake.
optional: Maybe add a line to the header file class comment explaining what type
of windows this layout manager/container manages?

https://codereview.chromium.org/2098183002/diff/1/ash/wm/system_modal_contain...
ash/wm/system_modal_container_layout_manager.cc:198:
DCHECK(window->IsVisible());
Thanks for adding all the DCHECKs. It makes the code easier to understand.

https://codereview.chromium.org/2098183002/diff/1/ash/wm/system_modal_contain...
File ash/wm/system_modal_container_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2098183002/diff/1/ash/wm/system_modal_contain...
ash/wm/system_modal_container_layout_manager_unittest.cc:685:
modal_window->Show();
optional: Maybe test it prior to modal_window->Show() to exercise
OnWindowAddedToLayout()?

https://codereview.chromium.org/2098183002/diff/1/ash/wm/system_modal_contain...
ash/wm/system_modal_container_layout_manager_unittest.cc:687:
EXPECT_TRUE(Shell::GetPrimaryRootWindowController()
optional nit: Cache pointer to SystemModalLayoutManager?

[oshima, 2016-06-27 02:25:51]

Patchset #2 (id:20001) has been deleted

[oshima, 2016-06-27 02:26:07]

https://codereview.chromium.org/2098183002/diff/1/ash/wm/system_modal_contain...
File ash/wm/system_modal_container_layout_manager.cc (right):

https://codereview.chromium.org/2098183002/diff/1/ash/wm/system_modal_contain...
ash/wm/system_modal_container_layout_manager.cc:61: // DCHECK to avoid simple
mistake.
On 2016/06/27 00:33:43, James Cook wrote:
> optional: Maybe add a line to the header file class comment explaining what
type
> of windows this layout manager/container manages?

Done.

https://codereview.chromium.org/2098183002/diff/1/ash/wm/system_modal_contain...
File ash/wm/system_modal_container_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2098183002/diff/1/ash/wm/system_modal_contain...
ash/wm/system_modal_container_layout_manager_unittest.cc:685:
modal_window->Show();
On 2016/06/27 00:33:43, James Cook wrote:
> optional: Maybe test it prior to modal_window->Show() to exercise
> OnWindowAddedToLayout()?

Done. (turns out the function was calling show, so I create using view::Widget
API instead)

https://codereview.chromium.org/2098183002/diff/1/ash/wm/system_modal_contain...
ash/wm/system_modal_container_layout_manager_unittest.cc:687:
EXPECT_TRUE(Shell::GetPrimaryRootWindowController()
On 2016/06/27 00:33:43, James Cook wrote:
> optional nit: Cache pointer to SystemModalLayoutManager?

Done.

[oshima, 2016-06-27 02:26:11]

The CQ bit was checked by oshima@chromium.org

[oshima, 2016-06-27 02:26:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2098183002/#ps40001
(title: "addressed comments")

[commit-bot: I haz the power, 2016-06-27 02:26:24]

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, 2016-06-27 02:53:25]

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

[commit-bot: I haz the power, 2016-06-27 02:53:26]

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

[oshima, 2016-06-27 03:35:07]

The CQ bit was checked by oshima@chromium.org

[oshima, 2016-06-27 03:35:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2098183002/#ps60001
(title: "fixed cut")

[commit-bot: I haz the power, 2016-06-27 03:35:19]

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, 2016-06-27 04:18:55]

Description was changed from

==========
Make system modal check more strict

* Take visibility into account
* Make it system  modal only when MODAL_TYPE_SYSTEM is set.
  - DCHECK if the modal is set to other modal type.

BUG=620406
TEST=covered by unittest
==========

to

==========
Make system modal check more strict

* Take visibility into account
* Make it system  modal only when MODAL_TYPE_SYSTEM is set.
  - DCHECK if the modal is set to other modal type.

BUG=620406
TEST=covered by unittest
==========

[commit-bot: I haz the power, 2016-06-27 04:18:56]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-06-27 04:20:53]

Description was changed from

==========
Make system modal check more strict

* Take visibility into account
* Make it system  modal only when MODAL_TYPE_SYSTEM is set.
  - DCHECK if the modal is set to other modal type.

BUG=620406
TEST=covered by unittest
==========

to

==========
Make system modal check more strict

* Take visibility into account
* Make it system  modal only when MODAL_TYPE_SYSTEM is set.
  - DCHECK if the modal is set to other modal type.

BUG=620406
TEST=covered by unittest

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

[commit-bot: I haz the power, 2016-06-27 04:20:54]

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