Shelf: Hide overflow bubble when app launched from overflow.

Shelf: Hide overflow bubble when app launched from overflow.

When the shelf is in autohide, the shelf does not hide after launching from overflow shelf. This is because the overflow shelf remains visible after lauching an app, which this CL fixes.

BUG=625924
TEST=manual,ash_unittests --gtest_filter="ShelfViewTest.*"

Review-Url: https://codereview.chromium.org/2807473003
Cr-Commit-Position: refs/heads/master@{#462869}
Committed: https://chromium.googlesource.com/chromium/src/+/4242daf2b69dfc2c76b92adeeeb5d7efc756d59f

[sammiequon, 2017-04-06 21:32:09]

The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run

[sammiequon, 2017-04-06 21:32:23]

Description was changed from

==========
Shelf: Hide overflow bubble when app launched from overflow.

When the shelf is in autohide, the shelf does not hide after launching from
overflow shelf. This is because the overflow shelf remains visible after
lauching an app, which this CL fixes.

BUG=625924
TEST=manual,ash_unittests --gtest_filter="ShelfViewTest.*"
==========

to

==========
Shelf: Hide overflow bubble when app launched from overflow.

When the shelf is in autohide, the shelf does not hide after launching from
overflow shelf. This is because the overflow shelf remains visible after
lauching an app, which this CL fixes.

BUG=625924
TEST=manual,ash_unittests --gtest_filter="ShelfViewTest.*"
==========

[sammiequon, 2017-04-06 21:32:23]

sammiequon@chromium.org changed reviewers:
+ msw@chromium.org

[sammiequon, 2017-04-06 21:32:42]

msw@ - Please take a look. Thanks!

[commit-bot: I haz the power, 2017-04-06 21:34:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2807473003/diff/1/ash/common/shelf/shelf_view.cc
File ash/common/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2807473003/diff/1/ash/common/shelf/shelf_view...
ash/common/shelf/shelf_view.cc:441: // If this is the overflow shelf,
|main_shelf_| will not be null. When an app
nit: s/app/item/

https://codereview.chromium.org/2807473003/diff/1/ash/common/shelf/shelf_view...
ash/common/shelf/shelf_view.cc:444: if (main_shelf_) {
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();

https://codereview.chromium.org/2807473003/diff/1/ash/common/shelf/shelf_view...
ash/common/shelf/shelf_view.cc:446: main_shelf_->ToggleOverflowBubble();
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.

https://codereview.chromium.org/2807473003/diff/1/ash/shelf/shelf_view_unitte...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2807473003/diff/1/ash/shelf/shelf_view_unitte...
ash/shelf/shelf_view_unittest.cc:1862: const gfx::Point
kPointNotOnShelfOrOverflow = gfx::Point(0, 0);
Remove this constant and skip set_current_location; (0,0) is the default
EventGenerator |initial_location|. If you want, instead check that
generator.current_location() is not on the shelf or overflow bubble (but that
might also be a bit unnecessary)

https://codereview.chromium.org/2807473003/diff/1/ash/shelf/shelf_view_unitte...
ash/shelf/shelf_view_unittest.cc:1898: ShelfViewTestAPI test_api_for_overflow2(
optional nit: |test_api_for_overflow1|

[commit-bot: I haz the power, 2017-04-06 22:19:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 22:19:53]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2807473003/diff/1/ash/common/shelf/shelf_view.cc
File ash/common/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2807473003/diff/1/ash/common/shelf/shelf_view...
ash/common/shelf/shelf_view.cc:441: // If this is the overflow shelf,
|main_shelf_| will not be null. When an app
On 2017/04/06 21:57:50, msw wrote:
> nit: s/app/item/

Done.

https://codereview.chromium.org/2807473003/diff/1/ash/common/shelf/shelf_view...
ash/common/shelf/shelf_view.cc:444: if (main_shelf_) {
On 2017/04/06 21:57:50, msw wrote:
> 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();

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

https://codereview.chromium.org/2807473003/diff/1/ash/common/shelf/shelf_view...
ash/common/shelf/shelf_view.cc:446: main_shelf_->ToggleOverflowBubble();
On 2017/04/06 21:57:50, msw wrote:
> 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.

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?

https://codereview.chromium.org/2807473003/diff/1/ash/shelf/shelf_view_unitte...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2807473003/diff/1/ash/shelf/shelf_view_unitte...
ash/shelf/shelf_view_unittest.cc:1862: const gfx::Point
kPointNotOnShelfOrOverflow = gfx::Point(0, 0);
On 2017/04/06 21:57:50, msw wrote:
> Remove this constant and skip set_current_location; (0,0) is the default
> EventGenerator |initial_location|. If you want, instead check that
> generator.current_location() is not on the shelf or overflow bubble (but that
> might also be a bit unnecessary)

Done.

https://codereview.chromium.org/2807473003/diff/1/ash/shelf/shelf_view_unitte...
ash/shelf/shelf_view_unittest.cc:1898: ShelfViewTestAPI test_api_for_overflow2(
On 2017/04/06 21:57:50, msw wrote:
> optional nit: |test_api_for_overflow1|

Done.

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

lgtm with minor comments, I'll peek again after they're addressed.

https://codereview.chromium.org/2807473003/diff/1/ash/common/shelf/shelf_view.cc
File ash/common/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2807473003/diff/1/ash/common/shelf/shelf_view...
ash/common/shelf/shelf_view.cc:444: if (main_shelf_) {
On 2017/04/06 23:03:11, sammiequon wrote:
> On 2017/04/06 21:57:50, msw wrote:
> > 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();
> 
> 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).

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

https://codereview.chromium.org/2807473003/diff/1/ash/common/shelf/shelf_view...
ash/common/shelf/shelf_view.cc:446: main_shelf_->ToggleOverflowBubble();
On 2017/04/06 23:03:11, sammiequon wrote:
> On 2017/04/06 21:57:50, msw wrote:
> > 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.
> 
> 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?

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.

https://codereview.chromium.org/2807473003/diff/20001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2807473003/diff/20001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_view.cc:481: // which is associated with the main shelf.
Note: this will invalidate
nit: remove this Note, or rephrase as "Hiding the bubble posts a task to close
the widget, so this object is not synchronously destroyed and member access
below is allowed." (but really there's no need for the comment)

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

Thanks!

https://codereview.chromium.org/2807473003/diff/1/ash/common/shelf/shelf_view.cc
File ash/common/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2807473003/diff/1/ash/common/shelf/shelf_view...
ash/common/shelf/shelf_view.cc:444: if (main_shelf_) {
On 2017/04/06 23:19:03, msw wrote:
> On 2017/04/06 23:03:11, sammiequon wrote:
> > On 2017/04/06 21:57:50, msw wrote:
> > > 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();
> > 
> > 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).
> 
> 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."

Done.

https://codereview.chromium.org/2807473003/diff/1/ash/common/shelf/shelf_view...
ash/common/shelf/shelf_view.cc:446: main_shelf_->ToggleOverflowBubble();
On 2017/04/06 23:19:03, msw wrote:
> On 2017/04/06 23:03:11, sammiequon wrote:
> > On 2017/04/06 21:57:50, msw wrote:
> > > 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.
> > 
> > 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?
> 
> 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.

Acknowledged. Moved back to the regular location.

https://codereview.chromium.org/2807473003/diff/20001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2807473003/diff/20001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_view.cc:481: // which is associated with the main shelf.
Note: this will invalidate
On 2017/04/06 23:19:03, msw wrote:
> nit: remove this Note, or rephrase as "Hiding the bubble posts a task to close
> the widget, so this object is not synchronously destroyed and member access
> below is allowed." (but really there's no need for the comment)

Removed.

[msw, 2017-04-06 23:37:19]

still lgtm with one nit that I forgot earlier

https://codereview.chromium.org/2807473003/diff/40001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2807473003/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_view.cc:446: if (main_shelf_->IsShowingOverflowBubble())
optional nit: combine the nested if statements:
if (main_shelf_ && main_shelf_->IsShowingOverflowBubble())
  main_shelf_->ToggleOverflowBubble();

[sammiequon, 2017-04-06 23:55:23]

https://codereview.chromium.org/2807473003/diff/40001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2807473003/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_view.cc:446: if (main_shelf_->IsShowingOverflowBubble())
On 2017/04/06 23:37:19, msw wrote:
> optional nit: combine the nested if statements:
> if (main_shelf_ && main_shelf_->IsShowingOverflowBubble())
>   main_shelf_->ToggleOverflowBubble();

Done.

[sammiequon, 2017-04-06 23:55:29]

The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 23:56:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msw, 2017-04-07 00:03:03]

lgtm

[commit-bot: I haz the power, 2017-04-07 00:34:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-07 00:34:20]

Dry run: This issue passed the CQ dry run.

[sammiequon, 2017-04-07 15:35:44]

The CQ bit was checked by sammiequon@chromium.org

[sammiequon, 2017-04-07 15:35:53]

On 2017/04/07 00:03:03, msw wrote:
> lgtm

Thanks!

[commit-bot: I haz the power, 2017-04-07 15:36:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 15:42:51]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491579344383450,
"parent_rev": "a341a03cb3c251ef8c43d76fce50c77e6abd69bc", "commit_rev":
"4242daf2b69dfc2c76b92adeeeb5d7efc756d59f"}

[commit-bot: I haz the power, 2017-04-07 15:43:37]

Description was changed from

==========
Shelf: Hide overflow bubble when app launched from overflow.

When the shelf is in autohide, the shelf does not hide after launching from
overflow shelf. This is because the overflow shelf remains visible after
lauching an app, which this CL fixes.

BUG=625924
TEST=manual,ash_unittests --gtest_filter="ShelfViewTest.*"
==========

to

==========
Shelf: Hide overflow bubble when app launched from overflow.

When the shelf is in autohide, the shelf does not hide after launching from
overflow shelf. This is because the overflow shelf remains visible after
lauching an app, which this CL fixes.

BUG=625924
TEST=manual,ash_unittests --gtest_filter="ShelfViewTest.*"

Review-Url: https://codereview.chromium.org/2807473003
Cr-Commit-Position: refs/heads/master@{#462869}
Committed:
https://chromium.googlesource.com/chromium/src/+/4242daf2b69dfc2c76b92adeeeb5...
==========

[commit-bot: I haz the power, 2017-04-07 15:43:38]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/4242daf2b69dfc2c76b92adeeeb5...

[sammiequon, 2017-04-13 21:18:04]

Patchset #5 (id:80001) has been deleted