Fix the issue that When hiding a window will change its transient child windows' property.

When switching between users, the window that belongs to
the old user gets hidden, which also hides all its
transient child windows. That will clear the transient
window's modal property and set it to a normal window -
which is incorrect.

Based on the comments above the code that clears the
window's modality state, we move this line out into
OnWindowDestroying().

BUG=447743
Committed: https://crrev.com/14e7d0bd7fe57cf225882d271c9875191089e8ce
Cr-Commit-Position: refs/heads/master@{#316646}

[xdai1, 2015-01-16 18:11:47]

Patchset #1 (id:1) has been deleted

[xdai1, 2015-01-16 18:21:42]

xdai@chromium.org changed reviewers:
+ ben@chromium.org, skuhne@chromium.org

[xdai1, 2015-01-16 18:21:42]

Please help to review the CL, Thanks!

[Mr4D (OOO till 08-26), 2015-01-16 21:38:10]

Basically this looks good - however - testing this in a unit test should be
easy. Could you therefore add a test for this?

Many thanks!

[xdai1, 2015-01-20 05:26:21]

Patchset #4 (id:80001) has been deleted

[xdai1, 2015-01-20 07:22:31]

Patchset #5 (id:120001) has been deleted

[xdai1, 2015-01-20 07:22:40]

Patchset #4 (id:100001) has been deleted

[xdai1, 2015-01-20 17:25:44]

Patchset #3 (id:60001) has been deleted

[xdai1, 2015-01-20 18:58:47]

Please take another look, thanks!

[Mr4D (OOO till 08-26), 2015-01-20 19:17:22]

lgtm

https://codereview.chromium.org/838033006/diff/160001/chrome/browser/ui/ash/m...
File
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/838033006/diff/160001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:1126:
// and the property of transient windows.
Thanks for writing the test!

Since we have some special logic for transient windows ourselves it is good to
have a test here.

That said - ideally we should have a FocusController test direct in
focus_controller_unittest.cc, because the main logic for this is there.

[Create 2 windows, make one the transient child of the other, hide the (parent)
window and show it again. The transient child should remain the transient child
all the time.]

But I am fine with the way this is.

[xdai1, 2015-01-22 23:27:30]

@ben, can you help to review the code? Thanks!

[xdai1, 2015-01-31 00:27:01]

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

[xdai1, 2015-01-31 00:27:02]

sky@, can you help to review the code please? Thanks!

[sky, 2015-02-02 15:40:22]

Ben is a better person to review this one.

[Ben Goodger (Google), 2015-02-17 19:27:33]

lgtm

[xdai1, 2015-02-17 19:33:13]

The CQ bit was checked by xdai@chromium.org

[commit-bot: I haz the power, 2015-02-17 19:34:09]

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

[commit-bot: I haz the power, 2015-02-17 20:34:28]

Committed patchset #4 (id:160001)

[commit-bot: I haz the power, 2015-02-17 20:35:06]

Patchset 4 (id:??) landed as
https://crrev.com/14e7d0bd7fe57cf225882d271c9875191089e8ce
Cr-Commit-Position: refs/heads/master@{#316646}

[sky, 2015-02-18 23:14:20]

https://codereview.chromium.org/838033006/diff/160001/chrome/browser/ui/ash/m...
File
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/838033006/diff/160001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:1126:
// and the property of transient windows.
On 2015/01/20 19:17:22, Mr4D wrote:
> Thanks for writing the test!
> 
> Since we have some special logic for transient windows ourselves it is good to
> have a test here.
> 
> That said - ideally we should have a FocusController test direct in
> focus_controller_unittest.cc, because the main logic for this is there.
> 
> [Create 2 windows, make one the transient child of the other, hide the
(parent)
> window and show it again. The transient child should remain the transient
child
> all the time.]
> 
> But I am fine with the way this is.

I would have preferred a test in focus_controller_unittest too. Was that not
possible?

[xdai1, 2015-02-18 23:18:33]

On 2015/02/18 23:14:20, sky wrote:
>
https://codereview.chromium.org/838033006/diff/160001/chrome/browser/ui/ash/m...
> File
>
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/838033006/diff/160001/chrome/browser/ui/ash/m...
>
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:1126:
> // and the property of transient windows.
> On 2015/01/20 19:17:22, Mr4D wrote:
> > Thanks for writing the test!
> > 
> > Since we have some special logic for transient windows ourselves it is good
to
> > have a test here.
> > 
> > That said - ideally we should have a FocusController test direct in
> > focus_controller_unittest.cc, because the main logic for this is there.
> > 
> > [Create 2 windows, make one the transient child of the other, hide the
> (parent)
> > window and show it again. The transient child should remain the transient
> child
> > all the time.]
> > 
> > But I am fine with the way this is.
> 
> I would have preferred a test in focus_controller_unittest too. Was that not
> possible?

NP, I will add the test in focus_controller_unittest.cc in another CL.

[sky, 2015-02-19 16:19:49]

Thanks!

On Wed, Feb 18, 2015 at 3:18 PM,  <xdai@chromium.org> wrote:
> On 2015/02/18 23:14:20, sky wrote:
>
>
https://codereview.chromium.org/838033006/diff/160001/chrome/browser/ui/ash/m...
>>
>> File
>
>
>
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc
>>
>> (right):
>
>
>
>
https://codereview.chromium.org/838033006/diff/160001/chrome/browser/ui/ash/m...
>
>
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:1126:
>>
>> // and the property of transient windows.
>> On 2015/01/20 19:17:22, Mr4D wrote:
>> > Thanks for writing the test!
>> >
>> > Since we have some special logic for transient windows ourselves it is
>> > good
>
> to
>>
>> > have a test here.
>> >
>> > That said - ideally we should have a FocusController test direct in
>> > focus_controller_unittest.cc, because the main logic for this is there.
>> >
>> > [Create 2 windows, make one the transient child of the other, hide the
>> (parent)
>> > window and show it again. The transient child should remain the
>> > transient
>> child
>> > all the time.]
>> >
>> > But I am fine with the way this is.
>
>
>> I would have preferred a test in focus_controller_unittest too. Was that
>> not
>> possible?
>
>
> NP, I will add the test in focus_controller_unittest.cc in another CL.
>
> https://codereview.chromium.org/838033006/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.