Keep the SystemModalContainerEventHandler added

components/exo/pointer.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/exo/pointer.h"
 
 #include "ash/common/display/display_info.h"
 #include "ash/common/shell_window_ids.h"
 #include "ash/display/display_manager.h"
 #include "ash/shell.h"
@@ skipping 105 matching lines @@
       aura::client::GetCursorClient(focus_->window()->GetRootWindow());
   if (cursor_client)
     cursor_client->SetCursor(
         focus_->window()->GetCursor(gfx::ToFlooredPoint(location_)));
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // ui::EventHandler overrides:
 
 void Pointer::OnMouseEvent(ui::MouseEvent* event) {
   Surface* target = GetEffectiveTargetForEvent(event);

[oshima, 2016/07/13 21:34:00]

can you move this after the following check?

same for the touch.cc

[hariank, 2016/07/13 23:27:22]

Done.

[hariank, 2016/07/13 23:27:22]

Done.

 
+  if (!ash::Shell::GetInstance()->CanWindowReceiveEvents(

[reveman, 2016/07/13 18:55:09]

Can you add a short comment above this to explain why this might return false
and why we should then ignore the event?

[hariank, 2016/07/13 23:27:22]

Done.

+          static_cast<aura::Window*>(event->target()))) {
+    event->StopPropagation();

[reveman, 2016/07/13 18:55:09]

Is it really correct to call StopPropagation here? Isn't that supposed to mean
that the event was consumed? and we're not really consuming the event, just
ignoring it.

[oshima, 2016/07/13 21:34:00]

Shell::CanWindowReceiveEvents is used to block event in system modal. Note that
the event->target() isn't the exo widget, so this is ok.
(and this is same as what SystemModalContainerEventFilter does).

That's being said, we should test the case where it does not block events that
are targeted to arc++ modal windows. hariank@, can you add a test case for it?

[hariank, 2016/07/13 23:27:22]

Done.

[reveman, 2016/07/14 11:15:40]

I'm still failing to see why StopPropagation should be called here. I understand
that it doesn't really do any real harm now but it still seems incorrect to me.
It makes sense that SystemModalContainerEventFilter consumes the event as it's
supposed to prevent the event from being delivered to other event handlers.
However, that's not the purpose of Pointer::OnMouseEvent, Pointer::OnMouseEvent
just does pre processing of events potentially targeted at exo windows. I would
not expect the presence of an exo::Pointer instance to have an impact on further
processing of the event. Keep in mind that we might have exo windows but zero
exo pointer instances or N pointer instances. I'd expect the state of the event
to be the same when it's processed by pointer 1 and pointer 2 and not be marked
as consumed when it hits pointer 2 when it was not marked this way when hitting
pointer 1.

[oshima, 2016/07/14 12:21:27]

I think this is misunderstanding. The event received in pre target handler may
or may not be
exo windows. In other words, it's called for every targets, before the event is
being
sent to that target, which may or may not be exo window.
And CanWindowReceiveEvents checks if that target window can receive an event,
not an exo pointer.

GetEffectiveTargetForEvent(event);

filters out non exo windows, and that's why the reset of the code works for
pointer.

I still think this should not use pre target handler, but I think that this is
safer to merge.
We can revisit this for m54 though.

[reveman, 2016/07/14 13:59:49]

Right, pre target handler is for all windows. I have no problem with calling
CanWindowReceiveEvents here, that makes perfect sense. What I don't understand
is why calling StopPropagation is correct, especially before determining if the
target is an exo window. To me it seems more correct to not call StopPropagation
at all. Please explain why it makes sense to have this call here.

[oshima, 2016/07/14 14:38:27]

We can skip it if you feel strongly about it, it will be called in
SystemModalContainerEventHandler again anyway, which is a bit waste.
This peace of code is generic and unrelated to exo, and we copied it from
SystemModalContainerEventHandler because
this handler handles events before SystemModalContainerEventHandler process the
event. (which is added on the fly)

Actually I have an alternative idea how to fix this. Please hold on.

+    return;
+  }
+
   // If target is different than the current pointer focus then we need to
   // generate enter and leave events.
   if (target != focus_) {
     // First generate a leave event if we currently have a target in focus.
     if (focus_) {
       delegate_->OnPointerLeave(focus_);
       focus_->RemoveSurfaceObserver(this);
       // Require SetCursor() to be called and cursor to be re-defined in
       // response to each OnPointerEnter() call.
       focus_->UnregisterCursorProvider(this);
@@ skipping 176 matching lines @@
 
   if (ui_scale != cursor_scale_) {
     gfx::Transform transform;
     transform.Scale(ui_scale, ui_scale);
     widget_->GetNativeWindow()->SetTransform(transform);
     cursor_scale_ = ui_scale;
   }
 }
 
 }  // namespace exo