Fix WindowTreeClient::RequestClose(window_id).

Fix WindowTreeClient::RequestClose(window_id).

In the new client-lib, non-root windows can also be closed using
WindowTreeClient::RequestClose.
If the window is the root window, we send close request to the entire
WindowTreeHost. Otherwise we ask the delegate of the window to close it.
We are not adding a delegate to the root window because functions
like Window::GetToplevelWindow() would fail.

BUG=676091
TEST=aura_unittests

Committed: https://crrev.com/655b0a08086da3328bee25e73070fcd30f7e5e9e
Cr-Commit-Position: refs/heads/master@{#441557}

[riajiang, 2017-01-03 20:08:14]

Patchset #2 (id:20001) has been deleted

[riajiang, 2017-01-03 20:11:15]

Description was changed from

==========
WIP Fix WindowTreeClient::RequestClose(window_id).

BUG=676091
==========

to

==========
Fix WindowTreeClient::RequestClose(window_id).

In the new client-lib, non-root windows can also be closed using 
WindowTreeClient::RequestClose.
If the window is the root window, we send close request to the entire
WindowTreeHost. Otherwise we ask the delegate of the window to close it.
We are not adding a delegate to the root window because functions
like Window::GetToplevelWindow() would fail.

BUG=676091
TEST=aura_unittests
==========

[riajiang, 2017-01-03 20:11:15]

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

[riajiang, 2017-01-03 20:12:10]

Hi sadrul@, PTAL, thanks!

[sadrul, 2017-01-04 20:08:06]

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

[sadrul, 2017-01-04 20:08:07]

+sky@ for his thoughts.

lgtm

https://codereview.chromium.org/2604453002/diff/60001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2604453002/diff/60001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:1323: }
Don't need {}

As discussed offline, the alternate is to use a delegate when creating the
root-window for the WTHostMus, and close the WTH from its OnRequestClose()
override. However, doing that would cause aura::Window::GetToplevelWindow() to
always return the root-window, which may not be what we want. So special-casing
the root window here seems like the better solution.

+sky@ for thoughts.

[sky, 2017-01-04 21:00:57]

On 2017/01/04 20:08:07, sadrul wrote:
> +sky@ for his thoughts.
> 
> lgtm
> 
>
https://codereview.chromium.org/2604453002/diff/60001/ui/aura/mus/window_tree...
> File ui/aura/mus/window_tree_client.cc (right):
> 
>
https://codereview.chromium.org/2604453002/diff/60001/ui/aura/mus/window_tree...
> ui/aura/mus/window_tree_client.cc:1323: }
> Don't need {}
> 
> As discussed offline, the alternate is to use a delegate when creating the
> root-window for the WTHostMus, and close the WTH from its OnRequestClose()
> override. However, doing that would cause aura::Window::GetToplevelWindow() to
> always return the root-window, which may not be what we want. So
special-casing
> the root window here seems like the better solution.
> 
> +sky@ for thoughts.

Is this bug happening because a client is asking to close a window that the
client created directly (not the embed root)? If so, the client owns these
windows and can just delete them immediately, it shouldn't use RequestClose.

[riajiang, 2017-01-04 22:00:23]

On 2017/01/04 21:00:57, sky wrote:
> On 2017/01/04 20:08:07, sadrul wrote:
> > +sky@ for his thoughts.
> > 
> > lgtm
> > 
> >
>
https://codereview.chromium.org/2604453002/diff/60001/ui/aura/mus/window_tree...
> > File ui/aura/mus/window_tree_client.cc (right):
> > 
> >
>
https://codereview.chromium.org/2604453002/diff/60001/ui/aura/mus/window_tree...
> > ui/aura/mus/window_tree_client.cc:1323: }
> > Don't need {}
> > 
> > As discussed offline, the alternate is to use a delegate when creating the
> > root-window for the WTHostMus, and close the WTH from its OnRequestClose()
> > override. However, doing that would cause aura::Window::GetToplevelWindow()
to
> > always return the root-window, which may not be what we want. So
> special-casing
> > the root window here seems like the better solution.
> > 
> > +sky@ for thoughts.
> 
> Is this bug happening because a client is asking to close a window that the
> client created directly (not the embed root)? If so, the client owns these
> windows and can just delete them immediately, it shouldn't use RequestClose.

This bug happens when the embedder wants to close the embeddee, e.g., when I
click close
button for the browser (ash closes chrome), it was crashing because root window
doesn't
have a delegate.

[sky, 2017-01-04 22:13:36]

Ok, thanks for the explanation. One question. How come you are allowing IsRoot
to be false?

[riajiang, 2017-01-04 22:37:15]

On 2017/01/04 22:13:36, sky wrote:
> Ok, thanks for the explanation. One question. How come you are allowing IsRoot
> to be false?

I thought with the new aura-mus client-lib, non-root windows can also 
be closed using WindowTreeClient::RequestClose, so we ask the window
delegate to close it if it's not a root window (what this function was
doing) and only ask the WTHost to close if it's root window? Did I 
misunderstand something? Will it always be root if the request is 
coming from embedder?

[sky, 2017-01-04 22:50:50]

WmRequestClose is only applicable to roots, so you shouldn't need to
remove the check early in the function.

On Wed, Jan 4, 2017 at 2:37 PM,  <riajiang@chromium.org> wrote:
> On 2017/01/04 22:13:36, sky wrote:
>> Ok, thanks for the explanation. One question. How come you are allowing
>> IsRoot
>> to be false?
>
> I thought with the new aura-mus client-lib, non-root windows can also
> be closed using WindowTreeClient::RequestClose, so we ask the window
> delegate to close it if it's not a root window (what this function was
> doing) and only ask the WTHost to close if it's root window? Did I
> misunderstand something? Will it always be root if the request is
> coming from embedder?
>
> https://codereview.chromium.org/2604453002/

-- 
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.

[riajiang, 2017-01-05 00:49:07]

On 2017/01/04 22:50:50, sky wrote:
> WmRequestClose is only applicable to roots, so you shouldn't need to
> remove the check early in the function.
> 
> On Wed, Jan 4, 2017 at 2:37 PM,  <mailto:riajiang@chromium.org> wrote:
> > On 2017/01/04 22:13:36, sky wrote:
> >> Ok, thanks for the explanation. One question. How come you are allowing
> >> IsRoot
> >> to be false?
> >
> > I thought with the new aura-mus client-lib, non-root windows can also
> > be closed using WindowTreeClient::RequestClose, so we ask the window
> > delegate to close it if it's not a root window (what this function was
> > doing) and only ask the WTHost to close if it's root window? Did I
> > misunderstand something? Will it always be root if the request is
> > coming from embedder?
> >
> > https://codereview.chromium.org/2604453002/
> 
> -- 
> 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.
> 

Oh okay I see. I removed the non-root case - so it's always sending the 
close request to WTHostMus now, tests are also simplified.

[riajiang, 2017-01-05 00:50:00]

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

[commit-bot: I haz the power, 2017-01-05 00:50:34]

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-05 00:51:16]

LGTM

[commit-bot: I haz the power, 2017-01-05 02:00:01]

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

[commit-bot: I haz the power, 2017-01-05 02:00:02]

Dry run: This issue passed the CQ dry run.

[riajiang, 2017-01-05 02:04:34]

The CQ bit was checked by riajiang@chromium.org

[riajiang, 2017-01-05 02:04:34]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2604453002/#ps80001
(title: "remove non-root case")

[commit-bot: I haz the power, 2017-01-05 02:04:54]

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-05 02:08:20]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483581874408910,
"parent_rev": "cebb69ed04aff08f7bcc3c8535c2039f03c74c2b", "commit_rev":
"9f9f901b3c8b3e51339b6b6e57789c550eaa555f"}

[commit-bot: I haz the power, 2017-01-05 02:08:47]

Description was changed from

==========
Fix WindowTreeClient::RequestClose(window_id).

In the new client-lib, non-root windows can also be closed using 
WindowTreeClient::RequestClose.
If the window is the root window, we send close request to the entire
WindowTreeHost. Otherwise we ask the delegate of the window to close it.
We are not adding a delegate to the root window because functions
like Window::GetToplevelWindow() would fail.

BUG=676091
TEST=aura_unittests
==========

to

==========
Fix WindowTreeClient::RequestClose(window_id).

In the new client-lib, non-root windows can also be closed using 
WindowTreeClient::RequestClose.
If the window is the root window, we send close request to the entire
WindowTreeHost. Otherwise we ask the delegate of the window to close it.
We are not adding a delegate to the root window because functions
like Window::GetToplevelWindow() would fail.

BUG=676091
TEST=aura_unittests

Review-Url: https://codereview.chromium.org/2604453002
==========

[commit-bot: I haz the power, 2017-01-05 02:08:51]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2017-01-05 02:10:50]

Description was changed from

==========
Fix WindowTreeClient::RequestClose(window_id).

In the new client-lib, non-root windows can also be closed using 
WindowTreeClient::RequestClose.
If the window is the root window, we send close request to the entire
WindowTreeHost. Otherwise we ask the delegate of the window to close it.
We are not adding a delegate to the root window because functions
like Window::GetToplevelWindow() would fail.

BUG=676091
TEST=aura_unittests

Review-Url: https://codereview.chromium.org/2604453002
==========

to

==========
Fix WindowTreeClient::RequestClose(window_id).

In the new client-lib, non-root windows can also be closed using
WindowTreeClient::RequestClose.
If the window is the root window, we send close request to the entire
WindowTreeHost. Otherwise we ask the delegate of the window to close it.
We are not adding a delegate to the root window because functions
like Window::GetToplevelWindow() would fail.

BUG=676091
TEST=aura_unittests

Committed: https://crrev.com/655b0a08086da3328bee25e73070fcd30f7e5e9e
Cr-Commit-Position: refs/heads/master@{#441557}
==========

[commit-bot: I haz the power, 2017-01-05 02:10:51]

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

[sadrul, 2017-01-05 02:49:00]

https://codereview.chromium.org/2604453002/diff/80001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_client.cc (left):

https://codereview.chromium.org/2604453002/diff/80001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:1317:
window->GetWindow()->delegate()->OnRequestClose();
I think WindowDelegate::OnRequestClose() can now be removed, since this was the
only place that called it?