Shelf: Hide overflow bubble when app launched from overflow.

ash/common/shelf/shelf_view.cc

Index: ash/common/shelf/shelf_view.cc
diff --git a/ash/common/shelf/shelf_view.cc b/ash/common/shelf/shelf_view.cc
index d99b9e76fdbea9a0df593b57f7a6eec0da50f799..844239690f3c803b996037fe547becec129e6b3a 100644
--- a/ash/common/shelf/shelf_view.cc
+++ b/ash/common/shelf/shelf_view.cc
@@ -438,6 +438,14 @@ void ShelfView::ButtonPressed(views::Button* sender,
   if (!ShouldEventActivateButton(sender, event))
     return;
 
+  // If this is the overflow shelf, |main_shelf_| will not be null. When an app

[msw, 2017/04/06 21:57:50]

nit: s/app/item/

[sammiequon, 2017/04/06 23:03:11]

Done.

+  // is pressed on the overflow shelf, we want to hide the overflow bubble,
+  // which is associated with the main shelf.
+  if (main_shelf_) {

[msw, 2017/04/06 21:57:50]

For my own curiosity, how does the overflow bubble [already] close if a button
on the main shelf is pressed?
 Is the app actually launched or does that *just* close the overflow bubble?
Perhaps consider this solution to handle both cases explicitly:
  // Hide the overflow shelf when an item is selected.
  ShelfView* shelf_view = main_shelf_ ? main_shelf_ : this;
  if (shelf_view->IsShowingOverflowBubble())
    shelf_view->ToggleOverflowBubble();

[sammiequon, 2017/04/06 23:03:11]

The overview bubble listens for pointer events and hides itself if any down
events happen anywhere outside its bounds. (See [1] for source)

[1]
https://cs.chromium.org/chromium/src/ash/common/shelf/overflow_bubble.cc?rcl=...

I think your proposed solution would end up with double handling or the check
will always return false if it is the main shelf. I was not sure how changing
some of the code in that file will affect other things so I chose the way which
would get the job done with the least possible side effects (or so I believe).

[msw, 2017/04/06 23:19:03]

Okay, what you have is fine, just add something like this to your comment above.
"Button presses on the main shelf cause the overflow bubble to close itself via
PointerWatcher functionality."

[sammiequon, 2017/04/06 23:34:46]

Done.

+    if (main_shelf_->IsShowingOverflowBubble())
+      main_shelf_->ToggleOverflowBubble();

[msw, 2017/04/06 21:57:50]

I wonder, can closing the overflow bubble in this manner destroy this [overflow]
ShelfView synchronously? If so, the class member access below this statement
would cause invalid memory access. Perhaps this works okay because it actually
calls OverflowBubble::Hide(), which invokes bubble_->GetWidget()->Close(), and
Widget::Close posts a task to actually destroy the object and its views
asynchronously.

[sammiequon, 2017/04/06 23:03:11]

Hmm, I did not notice sender being used below and no crashes came up with the
cases i tested :-). Would moving it lower and adding a note suffice?

[msw, 2017/04/06 23:19:03]

This is probably fine as-is, given the aync Widget::Close. My concern wasn't use
of |sender|, but use of members of this ShelfView instance (like |view_model_|,
|model_|, |weak_ptr_factory_|, etc.), if this view were actually destroyed
synchronously within the ToggleOverflowBubble call (but that's probably not
happening). Moving the code lower doesn't actually help anything, unless it's
moved to the very end of the function (or at least if there's no member access
afterwards). I suggest moving the block to the very bottom or back where it was.

[sammiequon, 2017/04/06 23:34:46]

Acknowledged. Moved back to the regular location.

+  }
+
   // Record the index for the last pressed shelf item.
   last_pressed_index_ = view_model_->GetIndexOfView(sender);
   DCHECK_LT(-1, last_pressed_index_);