ui::PlatformWindowDelegate::OnClosed() isn't called when a ui::X11Window is closed

ui::PlatformWindowDelegate::OnClosed() isn't called when a ui::X11Window is closed

This problem manifests itself in simple Mojo apps that use NativeViewportService on X11. When the NativeViewport window is closed, the NativeViewportClient OnDestroyed() method does not run.

BUG=421536
Committed: https://crrev.com/b37606e1c2a6703e58a61a978e8e32be72acae0c
Cr-Commit-Position: refs/heads/master@{#298926}

[hansmuller, 2014-10-08 19:17:23]

hansmuller@chromium.org changed reviewers:
+ ben@chromium.org

[hansmuller, 2014-10-08 19:42:50]

There's a complete description of the problem in the bug report -
crbug.com/421536

This is a one-line change.

[Ben Goodger (Google), 2014-10-08 20:45:40]

https://codereview.chromium.org/644443002/diff/1/ui/platform_window/x11/x11_w...
File ui/platform_window/x11/x11_window.cc (right):

https://codereview.chromium.org/644443002/diff/1/ui/platform_window/x11/x11_w...
ui/platform_window/x11/x11_window.cc:59: delegate_->OnClosed();
I'm no Xpert, is there no callback you receive in response to XDestroyWindow()
that can call this?

[hansmuller, 2014-10-08 21:57:45]

On 2014/10/08 20:45:40, Ben Goodger (Google) wrote:
>
https://codereview.chromium.org/644443002/diff/1/ui/platform_window/x11/x11_w...
> File ui/platform_window/x11/x11_window.cc (right):
> 
>
https://codereview.chromium.org/644443002/diff/1/ui/platform_window/x11/x11_w...
> ui/platform_window/x11/x11_window.cc:59: delegate_->OnClosed();
> I'm no Xpert, is there no callback you receive in response to XDestroyWindow()
> that can call this?

There is the DestroyNotify event however X11Window::Destroy stops processing
events before the post-facto notification arrives. I don't think it would be
worth the trouble to carry on consuming events until the window's DestroyNotify
event showed up, since we're explicitly destroying it.

[Ben Goodger (Google), 2014-10-09 16:57:23]

lgtm

[hansmuller, 2014-10-09 17:03:40]

The CQ bit was checked by hansmuller@chromium.org

[commit-bot: I haz the power, 2014-10-09 17:06:53]

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

[commit-bot: I haz the power, 2014-10-09 17:29:23]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2014-10-09 17:30:09]

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

[jwd, 2014-10-09 21:37:54]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/647573002/ by jwd@chromium.org.

The reason for reverting is: Only cl that seems related to
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...
falures..