Do not mutate ui::Event properties during nested event processing

ui/events/event_processor.cc

 // Copyright 2013 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 "ui/events/event_processor.h"
 
 #include "ui/events/event_target.h"
 #include "ui/events/event_targeter.h"
 
 namespace ui {
 
 EventDispatchDetails EventProcessor::OnEventFromSource(Event* event) {
   EventTarget* root = GetRootTarget();
   CHECK(root);
   EventTargeter* targeter = root->GetEventTargeter();
   CHECK(targeter);
 
   PrepareEventForDispatch(event);
   EventTarget* target = targeter->FindTargetForEvent(root, event);
+  scoped_ptr<Event> event_copy(Event::Clone(*event));

[sadrul, 2014/09/03 16:23:53]

I was thinking of something more like this:
  scoped_ptr<Event> event_copy;
  if (event->phase() != EP_PREDISPATCH)
    event_copy = Event::Clone(*event);
  while (target) {
    if (event_copy) {
      ...
    } else {
      ...
    }
  }

i.e. do the clone only if needed.

[tdanderson, 2014/09/03 20:20:02]

Done.

 
   while (target) {
-    EventDispatchDetails details = DispatchEvent(target, event);
+    EventDispatchDetails details;
+    if (event->phase() != EP_PREDISPATCH) {
+      // |event| is currently being processed by another EventProcessor, which
+      // expects the event's state (e.g., target, phase, etc.) to remain
+      // unchanged once |this| has finished its nested processing (with the
+      // exception of whether or not the event was handled). Thus dispatch
+      // a copy of the event instead.
+      details = DispatchEvent(target, event_copy.get());
+
+      if (event_copy->stopped_propagation())
+        event->StopPropagation();
+      else if (event_copy->handled())
+        event->SetHandled();
+    } else {
+      // |this| is the only EventProcessor currently processing |event|, so
+      // copying the event prior to dispatch is unnecessary.
+      details = DispatchEvent(target, event);

[sadrul, 2014/09/03 16:23:53]

DispatchEvent() would set the event-phase to POSTDISPATCH. So you would end up
dispatching event_copy in the next iteration, and send event_copy with phase ==
POSTDISPATCH in the iteration after that.

With this change, let's add a CHECK in EventDispatcherDelegate::DispatchEvent
that event->phase() == PREDISPATCH

[tdanderson, 2014/09/03 20:20:02]

Thanks for catching this! In the next patch set I believe we will always do the
right thing: for any invocation of OnEventFromSource(), we'll either always
enter the if at each loop iteration (if this invocation is doing nested
processing) or always enter the else at each loop iteration (if this invocation
is not doing nested processing).
This check will fail on ToT, and it will also fail if I were to add it in the
next patch set. DispatchEvent() is called with an event having a phase of
POSTDISPATCH at each iteration of the while loop except for the first. But since
the first thing EDD::DispatchEvent() does is reset the phase of the event to
PREDISPATCH, I don't think this will present a problem.

Are you concerned that attributes of the event other than the phase will be
incorrect before re-dispatching when we allow bubbling?

+    }
+
     if (details.dispatcher_destroyed ||
         details.target_destroyed ||
         event->handled()) {
       return details;
     }
 
     target = targeter->FindNextBestTarget(target, event);
   }
 
   return EventDispatchDetails();
 }
 
 void EventProcessor::PrepareEventForDispatch(Event* event) {
 }
 
 }  // namespace ui