Prevents double-clicks on a tab close button from aslo maximizing the browser

ash/wm/workspace/workspace_event_handler.cc

Index: ash/wm/workspace/workspace_event_handler.cc
diff --git a/ash/wm/workspace/workspace_event_handler.cc b/ash/wm/workspace/workspace_event_handler.cc
index 65a8841b46b68701908da3ddcb3dda49446dbb58..baf36edad8b1258677752f471801e4a364b8857e 100644
--- a/ash/wm/workspace/workspace_event_handler.cc
+++ b/ash/wm/workspace/workspace_event_handler.cc
@@ -16,7 +16,8 @@
 namespace ash {
 namespace internal {
 
-WorkspaceEventHandler::WorkspaceEventHandler() {
+WorkspaceEventHandler::WorkspaceEventHandler()
+    : event_target_component_(HTNOWHERE) {
 }
 
 WorkspaceEventHandler::~WorkspaceEventHandler() {
@@ -41,10 +42,13 @@ void WorkspaceEventHandler::OnMouseEvent(ui::MouseEvent* event) {
       break;
     case ui::ET_MOUSE_PRESSED: {
       wm::WindowState* target_state = wm::GetWindowState(target);
+      int previous_target_component = event_target_component_;
+      event_target_component_ = target->delegate()->
+          GetNonClientComponent(event->location());

[varkha, 2014/04/03 04:13:21]

Does this logic allow a right-click to "pollute" the previous_target_component
state if it comes first? I am not sure if our logic of detecting double click
will make this a non-issue, but I think the xxx_target_component_ variables
should only be set for left mouse clicks.

[flackr, 2014/04/04 18:51:45]

As long as we're consistent with the use of the variable and comment it
appropriately I don't think it matters. Since we're only using it for double
clicks I'd be in favor of renaming it to double_click_component_ and only
setting it on left clicks.

[jonross, 2014/04/07 15:49:30]

Using a mouse for input I wasn't able to get a state where right-click
interrupts a double-click action. However if that ever did occur it would break
this code. I wrote a unittest that demonstrates this.

I'll change this to handle left-mouse only.

[jonross, 2014/04/07 15:49:30]

Done.

[jonross, 2014/04/07 15:49:30]

Done.

       if (event->flags() & ui::EF_IS_DOUBLE_CLICK &&
           event->IsOnlyLeftMouseButton() &&
-          target->delegate()->GetNonClientComponent(event->location()) ==
-          HTCAPTION) {
+          event_target_component_ == HTCAPTION &&
+          event_target_component_ == previous_target_component) {

[varkha, 2014/04/03 04:13:21]

Once here you probably want to reset the event_target_component_ variable (?)

[jonross, 2014/04/07 15:49:30]

Since WindowEventHandler can receive each event up to two times, this is a good
idea. Otherwise two consecutive double-click events would trigger this
behaviour. 

         ash::Shell::GetInstance()->metrics()->RecordUserMetricsAction(
             ash::UMA_TOGGLE_MAXIMIZE_CAPTION_CLICK);
         const wm::WMEvent wm_event(wm::WM_EVENT_TOGGLE_MAXIMIZE_CAPTION);
@@ -64,24 +68,28 @@ void WorkspaceEventHandler::OnGestureEvent(ui::GestureEvent* event) {
   if (event->handled())
     return;
   aura::Window* target = static_cast<aura::Window*>(event->target());
-  if (event->type() == ui::ET_GESTURE_TAP &&
-      target->delegate()->GetNonClientComponent(event->location()) ==
-      HTCAPTION) {
-    if (event->details().tap_count() == 2) {
-      ash::Shell::GetInstance()->metrics()->RecordUserMetricsAction(
-          ash::UMA_TOGGLE_MAXIMIZE_CAPTION_GESTURE);
-      // Note: TouchUMA::GESTURE_FRAMEVIEW_TAP is counted twice each time
-      // TouchUMA::GESTURE_MAXIMIZE_DOUBLETAP is counted once.
-      TouchUMA::GetInstance()->RecordGestureAction(
-          TouchUMA::GESTURE_MAXIMIZE_DOUBLETAP);
-      const wm::WMEvent wm_event(wm::WM_EVENT_TOGGLE_MAXIMIZE_CAPTION);
-      wm::GetWindowState(target)->OnWMEvent(&wm_event);
-      event->StopPropagation();
-      return;
-    } else {
-      // Note: TouchUMA::GESTURE_FRAMEVIEW_TAP is counted twice for each tap.
-      TouchUMA::GetInstance()->RecordGestureAction(
-          TouchUMA::GESTURE_FRAMEVIEW_TAP);
+  if (event->type() == ui::ET_GESTURE_TAP) {

[varkha, 2014/04/03 04:13:21]

I think this method would benefit from early exits.

[jonross, 2014/04/07 15:49:30]

Done.

+    int previous_target_component = event_target_component_;
+    event_target_component_ = target->delegate()->
+        GetNonClientComponent(event->location());
+    if (event_target_component_ == HTCAPTION) {
+      if (event->details().tap_count() == 2 &&
+          event_target_component_ == previous_target_component) {
+        ash::Shell::GetInstance()->metrics()->RecordUserMetricsAction(
+            ash::UMA_TOGGLE_MAXIMIZE_CAPTION_GESTURE);
+        // Note: TouchUMA::GESTURE_FRAMEVIEW_TAP is counted twice each time
+        // TouchUMA::GESTURE_MAXIMIZE_DOUBLETAP is counted once.
+        TouchUMA::GetInstance()->RecordGestureAction(
+            TouchUMA::GESTURE_MAXIMIZE_DOUBLETAP);
+        const wm::WMEvent wm_event(wm::WM_EVENT_TOGGLE_MAXIMIZE_CAPTION);
+        wm::GetWindowState(target)->OnWMEvent(&wm_event);
+        event->StopPropagation();
+        return;
+      } else {

[varkha, 2014/04/03 04:13:21]

Should be no need for else after return.

[flackr, 2014/04/04 18:51:45]

In fact, explicitly forbidden by style guide:
http://www.chromium.org/developers/coding-style#Code_Formatting

[jonross, 2014/04/07 15:49:30]

Done.

+        // Note: TouchUMA::GESTURE_FRAMEVIEW_TAP is counted twice for each tap.
+        TouchUMA::GetInstance()->RecordGestureAction(
+            TouchUMA::GESTURE_FRAMEVIEW_TAP);
+      }
     }
   }
 }