mash: Preliminary support for widget hit test masks

mash: Preliminary support for widget hit test masks

This plumbs widget hit test masks through to the window server and uses them
for hit testing. It has the following limitations:
* Like "additional client areas" the mask is a rectangle, not a path.
* The mask is only updated on window bounds and client area change.

This fixes the system tray bubble not closing when you click in its bottom
border region, in particular the part that overlaps the tray button.

BUG=612566
TEST=added to mus_ws_unittests and views_mus_unittests

Committed: https://crrev.com/265527f8745b9d36e277d7c93ed7818204baf323
Cr-Commit-Position: refs/heads/master@{#395133}

[James Cook, 2016-05-19 17:02:48]

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

[James Cook, 2016-05-19 17:02:48]

sky, please take a look.

[sky, 2016-05-19 18:13:18]

https://codereview.chromium.org/1991973003/diff/40001/components/mus/public/i...
File components/mus/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/1991973003/diff/40001/components/mus/public/i...
components/mus/public/interfaces/window_tree.mojom:135: SetHitTestMask(uint32
window_id, mojo.Rect mask);
Add support for a null mask that clears the hit test mask.

https://codereview.chromium.org/1991973003/diff/40001/components/mus/ws/serve...
File components/mus/ws/server_window.h (right):

https://codereview.chromium.org/1991973003/diff/40001/components/mus/ws/serve...
components/mus/ws/server_window.h:239: gfx::Rect hit_test_mask_;
I think this should be a pointer, so that clients can specify no hit test mask
(null), empty, meaning there is no valid region of the window, or a mask.

https://codereview.chromium.org/1991973003/diff/40001/ui/views/mus/native_wid...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/1991973003/diff/40001/ui/views/mus/native_wid...
ui/views/mus/native_widget_mus.cc:1309: if
(!native_widget_delegate_->HasHitTestMask())
This case should set the hit test mask to null.

[James Cook, 2016-05-19 21:47:46]

sky, please take another look

https://codereview.chromium.org/1991973003/diff/40001/components/mus/public/i...
File components/mus/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/1991973003/diff/40001/components/mus/public/i...
components/mus/public/interfaces/window_tree.mojom:135: SetHitTestMask(uint32
window_id, mojo.Rect mask);
On 2016/05/19 18:13:17, sky wrote:
> Add support for a null mask that clears the hit test mask.

Done.

https://codereview.chromium.org/1991973003/diff/40001/components/mus/ws/serve...
File components/mus/ws/server_window.h (right):

https://codereview.chromium.org/1991973003/diff/40001/components/mus/ws/serve...
components/mus/ws/server_window.h:239: gfx::Rect hit_test_mask_;
On 2016/05/19 18:13:17, sky wrote:
> I think this should be a pointer, so that clients can specify no hit test mask
> (null), empty, meaning there is no valid region of the window, or a mask.

Done.

https://codereview.chromium.org/1991973003/diff/40001/ui/views/mus/native_wid...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/1991973003/diff/40001/ui/views/mus/native_wid...
ui/views/mus/native_widget_mus.cc:1309: if
(!native_widget_delegate_->HasHitTestMask())
On 2016/05/19 18:13:17, sky wrote:
> This case should set the hit test mask to null.

Done.

[sky, 2016-05-19 22:03:27]

https://codereview.chromium.org/1991973003/diff/60001/ui/views/mus/native_wid...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/1991973003/diff/60001/ui/views/mus/native_wid...
ui/views/mus/native_widget_mus.cc:1308: void
NativeWidgetMus::UpdateHitTestMask() {
Do you need to make this a no-op for the windowmanager side? If you do, use the
surfacetype as a way to check for it.

[sky, 2016-05-19 22:03:44]

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

[sky, 2016-05-19 22:03:45]

Also, +dcheng for mojom change

[James Cook, 2016-05-19 22:35:41]

https://codereview.chromium.org/1991973003/diff/60001/ui/views/mus/native_wid...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/1991973003/diff/60001/ui/views/mus/native_wid...
ui/views/mus/native_widget_mus.cc:1308: void
NativeWidgetMus::UpdateHitTestMask() {
On 2016/05/19 22:03:27, sky wrote:
> Do you need to make this a no-op for the windowmanager side? If you do, use
the
> surfacetype as a way to check for it.

Do you mean for the Widget/NativeWidget that represents the non-client frame? If
so, I figured it was OK for the window manager to set a mask (e.g. to create
"tabs" for window captions ala BeOS).

I'm happy to add an early-out (for SurfaceType::UNDERLAY I presume) if you'd
like.

[sky, 2016-05-19 22:53:59]

On Thu, May 19, 2016 at 3:35 PM,  <jamescook@chromium.org> wrote:
>
>
https://codereview.chromium.org/1991973003/diff/60001/ui/views/mus/native_wid...
> File ui/views/mus/native_widget_mus.cc (right):
>
>
https://codereview.chromium.org/1991973003/diff/60001/ui/views/mus/native_wid...
> ui/views/mus/native_widget_mus.cc:1308: void
> NativeWidgetMus::UpdateHitTestMask() {
> On 2016/05/19 22:03:27, sky wrote:
>> Do you need to make this a no-op for the windowmanager side? If you
> do, use the
>> surfacetype as a way to check for it.
>
> Do you mean for the Widget/NativeWidget that represents the non-client
> frame?

Yes.

> If so, I figured it was OK for the window manager to set a mask
> (e.g. to create "tabs" for window captions ala BeOS).

The problem is the way things are now both the client and wm will set
the client mask (they'll both call to UpdateHitTestMask). It'll be a
race as to which one takes. I think it better to have
UpdateHitTestMask do nothing for underlays. If the wm does want to
clobber the hit test mask, then it can be explicit, and perhaps we
would want a hit test mask explicitly for the wm side.

You can see how I did things for UpdateClientArea (virtual, with the
wm overriding), but I think it's error prone and very subtle to keep
going that route. I would rather we be explicit and say the wm side
doesn't do certain things in NativeWidgetMus.

Does that make sense?

  -Scott

>
> I'm happy to add an early-out (for SurfaceType::UNDERLAY I presume) if
> you'd like.
>
> https://codereview.chromium.org/1991973003/

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

[James Cook, 2016-05-19 23:15:23]

sky, please take another look

Yes, that makes sense. I agree explicitly checking for the surface makes sense
-- I've been confused in the past by the window manager's native widget
overriding virtual methods.

[sky, 2016-05-20 15:49:53]

LGTM

[James Cook, 2016-05-20 17:27:33]

dcheng, can I get OWNERS/security review for
components/mus/public/interfaces/window_tree.mojom ?

[dcheng, 2016-05-20 17:44:19]

https://codereview.chromium.org/1991973003/diff/80001/components/mus/ws/windo...
File components/mus/ws/window_tree.cc (right):

https://codereview.chromium.org/1991973003/diff/80001/components/mus/ws/windo...
components/mus/ws/window_tree.cc:1304: void WindowTree::SetHitTestMask(Id
transport_window_id, mojo::RectPtr mask) {
Once we have a StructTraits for mojo::RectPtr -> gfx::Rect and remove the
TypeConverter, how will we support the 'nullable rect' case? Will the null
mojo::Rect be translated to the empty gfx::Rect?

[James Cook, 2016-05-20 17:59:48]

https://codereview.chromium.org/1991973003/diff/80001/components/mus/ws/windo...
File components/mus/ws/window_tree.cc (right):

https://codereview.chromium.org/1991973003/diff/80001/components/mus/ws/windo...
components/mus/ws/window_tree.cc:1304: void WindowTree::SetHitTestMask(Id
transport_window_id, mojo::RectPtr mask) {
On 2016/05/20 17:44:19, dcheng wrote:
> Once we have a StructTraits for mojo::RectPtr -> gfx::Rect and remove the
> TypeConverter, how will we support the 'nullable rect' case? Will the null
> mojo::Rect be translated to the empty gfx::Rect?

I just talked to rockot@. Apparently there's a nullability concept for
StructTraits in the pipeline, so optional mojo.Rects will become either a
base::Optional<> or a unique_ptr<>. We'll need to use something like that here,
since being able to pass a non-null empty rect is desirable in this case.

[dcheng, 2016-05-20 18:02:10]

this particularly change is pretty trivial and lgtm

is there any documentation about how mus is structured overall? It will help
security reviewers to have a picture of the different components in mus, how
they talk to each other (e.g. who talks to WindowTree, what process does it live
in).

[James Cook, 2016-05-20 18:12:01]

On 2016/05/20 18:02:10, dcheng wrote:
> this particularly change is pretty trivial and lgtm
> 
> is there any documentation about how mus is structured overall? It will help
> security reviewers to have a picture of the different components in mus, how
> they talk to each other (e.g. who talks to WindowTree, what process does it
live
> in).

Replied off thread.

[James Cook, 2016-05-20 18:23:13]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2016-05-20 18:23:13]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1991973003/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2016-05-20 18:23:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1991973003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1991973003/100001

[commit-bot: I haz the power, 2016-05-20 19:18:08]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-20 19:20:27]

Description was changed from

==========
mash: Preliminary support for widget hit test masks

This plumbs widget hit test masks through to the window server and uses them
for hit testing. It has the following limitations:
* Like "additional client areas" the mask is a rectangle, not a path.
* The mask is only updated on window bounds and client area change.

This fixes the system tray bubble not closing when you click in its bottom
border region, in particular the part that overlaps the tray button.

BUG=612566
TEST=added to mus_ws_unittests and views_mus_unittests
==========

to

==========
mash: Preliminary support for widget hit test masks

This plumbs widget hit test masks through to the window server and uses them
for hit testing. It has the following limitations:
* Like "additional client areas" the mask is a rectangle, not a path.
* The mask is only updated on window bounds and client area change.

This fixes the system tray bubble not closing when you click in its bottom
border region, in particular the part that overlaps the tray button.

BUG=612566
TEST=added to mus_ws_unittests and views_mus_unittests

Committed: https://crrev.com/265527f8745b9d36e277d7c93ed7818204baf323
Cr-Commit-Position: refs/heads/master@{#395133}
==========

[commit-bot: I haz the power, 2016-05-20 19:20:29]

Patchset 6 (id:??) landed as
https://crrev.com/265527f8745b9d36e277d7c93ed7818204baf323
Cr-Commit-Position: refs/heads/master@{#395133}

[dcheng, 2016-05-20 20:13:51]

On 2016/05/20 at 18:12:01, jamescook wrote:
> On 2016/05/20 18:02:10, dcheng wrote:
> > this particularly change is pretty trivial and lgtm
> > 
> > is there any documentation about how mus is structured overall? It will help
> > security reviewers to have a picture of the different components in mus, how
> > they talk to each other (e.g. who talks to WindowTree, what process does it
live
> > in).
> 
> Replied off thread.

Hmm, I don't see the email?

[James Cook, 2016-05-20 20:23:33]

On 2016/05/20 20:13:51, dcheng wrote:
> On 2016/05/20 at 18:12:01, jamescook wrote:
> > On 2016/05/20 18:02:10, dcheng wrote:
> > > this particularly change is pretty trivial and lgtm
> > > 
> > > is there any documentation about how mus is structured overall? It will
help
> > > security reviewers to have a picture of the different components in mus,
how
> > > they talk to each other (e.g. who talks to WindowTree, what process does
it
> live
> > > in).
> > 
> > Replied off thread.
> 
> Hmm, I don't see the email?

Re-sent to your corp email address.