Make the second click on the app shelf icon close the window-list bubble.

Second click on app shelf icon should hide window-list bubble.

BUG=343005
Committed: https://crrev.com/a133d669d91b67f154ee7b07095e9bcad2303cbd
Cr-Commit-Position: refs/heads/master@{#307125}

[xdai1, 2014-12-02 20:22:14]

xdai@chromium.org changed reviewers:
+ skuhne@chromium.org

[xdai1, 2014-12-02 20:23:45]

Please help to review the cl, thanks!

[xdai1, 2014-12-02 21:25:16]

Patchset #1 (id:1) has been deleted

[Mr4D (OOO till 08-26), 2014-12-02 21:40:12]

Please see comment!

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

https://codereview.chromium.org/772963002/diff/20001/ash/shelf/shelf_view.cc#...
ash/shelf/shelf_view.cc:1568: // the current press event.
You do not want to do this because of various reasons:

- A double click is usually always wiggling in its location. 
- A double click only counts as such if various timings are met.

I think what you want is to check (event.type() & EF_IS_DOUBLE_CLICK) to see if
the user double clicked at roughly the same spot.

However - looking at the bug in particular, I guess you do not want to do
anything here, but instead in line 1696:

You want to check in line 1734 if the menu is currently open and if it is, you
want to close it, mark the event as handled and return instead.

[xdai1, 2014-12-05 00:30:01]

Please help to take a look at the code, thanks:)

[Mr4D (OOO till 08-26), 2014-12-05 15:21:48]

Great start!

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

https://codereview.chromium.org/772963002/diff/40001/ash/shelf/shelf_view.cc#...
ash/shelf/shelf_view.cc:1576: if (!IsUsableEvent(event)) {
No {}'s

https://codereview.chromium.org/772963002/diff/40001/ash/shelf/shelf_view.cc#...
ash/shelf/shelf_view.cc:1576: if (!IsUsableEvent(event)) {
Aha! Nice find! Yes, this is much better then the suggested class override! Good
job!

https://codereview.chromium.org/772963002/diff/40001/ash/shelf/shelf_view.cc#...
ash/shelf/shelf_view.cc:1608: bool canceled) {
Reset the variable.

(Note: If you would not do it, you couldn't use the button until you have
clicked another one...)

https://codereview.chromium.org/772963002/diff/40001/ash/shelf/shelf_view.cc#...
ash/shelf/shelf_view.cc:1674: if (!is_usable_event_ && (view_index ==
last_view_index_)) {
The index does not need to be checked since we are in a press/release cycle.
As far as I know it is not possible to press multiple buttons at once via touch.

https://codereview.chromium.org/772963002/diff/40001/ash/shelf/shelf_view.cc#...
ash/shelf/shelf_view.cc:1892: bool ShelfView::IsUsableEvent(const ui::Event&
event) {
Rename to IsRepostEvent() (and negate the logic accordingly). Somehow that
expresses better what this does.

https://codereview.chromium.org/772963002/diff/40001/ash/shelf/shelf_view.cc#...
ash/shelf/shelf_view.cc:1901: return (delta.InMilliseconds() != 0);
Ahhhh! Much better!

https://codereview.chromium.org/772963002/diff/40001/ash/shelf/shelf_view.h
File ash/shelf/shelf_view.h (right):

https://codereview.chromium.org/772963002/diff/40001/ash/shelf/shelf_view.h#n...
ash/shelf/shelf_view.h:440: bool is_usable_event_;
The name is rather misleading. How about "is_repost_event_"?

https://codereview.chromium.org/772963002/diff/40001/ash/shelf/shelf_view.h#n...
ash/shelf/shelf_view.h:443: int last_view_index_;
You might not need this. Please see comment below.

https://codereview.chromium.org/772963002/diff/40001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/772963002/diff/40001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_controller.cc:916: closing_event_time_ =
event.time_stamp();
You have to check all references to this use if we are not breaking other use
cases. I have seen it being used by others. I will run all unit tests (trybots)
to see if something breaks.

[xdai1, 2014-12-05 18:50:23]

Patchset #3 (id:60001) has been deleted

[xdai1, 2014-12-05 19:01:47]

Thanks Stefan! Can you please take another look at the code? I addressed some of
the comments but still don't have any clue about why the test failed...

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

https://codereview.chromium.org/772963002/diff/40001/ash/shelf/shelf_view.cc#...
ash/shelf/shelf_view.cc:1608: bool canceled) {
On 2014/12/05 15:21:47, Mr4D wrote:
> Reset the variable.
> 
> (Note: If you would not do it, you couldn't use the button until you have
> clicked another one...)

I reset the variable at line 1675 and line 1680. I think that should do the
work.

https://codereview.chromium.org/772963002/diff/40001/ash/shelf/shelf_view.cc#...
ash/shelf/shelf_view.cc:1674: if (!is_usable_event_ && (view_index ==
last_view_index_)) {
On 2014/12/05 15:21:47, Mr4D wrote:
> The index does not need to be checked since we are in a press/release cycle.
> As far as I know it is not possible to press multiple buttons at once via
touch.

I think we still need to check the index because only if the second click is on
the same shelf icon (view_index == last_view_index_), it should be ignored. But
if the second click is on a different shelf icon (view_index !=
last_view_index_), we still need to handle it (first close the previous menu,
and then open the current menu).

https://codereview.chromium.org/772963002/diff/40001/ash/shelf/shelf_view.h
File ash/shelf/shelf_view.h (right):

https://codereview.chromium.org/772963002/diff/40001/ash/shelf/shelf_view.h#n...
ash/shelf/shelf_view.h:440: bool is_usable_event_;
On 2014/12/05 15:21:48, Mr4D wrote:
> The name is rather misleading. How about "is_repost_event_"?

Done.

https://codereview.chromium.org/772963002/diff/40001/ash/shelf/shelf_view.h#n...
ash/shelf/shelf_view.h:443: int last_view_index_;
On 2014/12/05 15:21:48, Mr4D wrote:
> You might not need this. Please see comment below.

We may still need this. Please see comment in shelf_view.cc.

[xdai1, 2014-12-05 20:00:59]

Patchset #5 (id:120001) has been deleted

[xdai1, 2014-12-05 20:03:59]

Patchset #5 (id:140001) has been deleted

[xdai1, 2014-12-05 20:07:03]

Please take another look, thanks!

[Mr4D (OOO till 08-26), 2014-12-05 20:57:14]

A few more nits and that should be it!

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

https://codereview.chromium.org/772963002/diff/40001/ash/shelf/shelf_view.cc#...
ash/shelf/shelf_view.cc:1608: bool canceled) {
It's not enough. This could be failing:

Click on the button while the menu is open and hold it pressed. Drag the mouse
out. Then lift the button. go back to the button and click. No menu should show.

Alternatively you could do in line 1576 
is_usable_event_ = IsUsableEvent(event);
and get rid of the if (..).

https://codereview.chromium.org/772963002/diff/160001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/772963002/diff/160001/ash/shelf/shelf_view.cc...
ash/shelf/shelf_view.cc:1577: is_repost_event_ = true;
As stated above: You might want to do

is_repost_event_ = IsRepostEvent(event) &&
                   index == last_pressed_index_;

here.

https://codereview.chromium.org/772963002/diff/160001/ash/shelf/shelf_view.cc...
ash/shelf/shelf_view.cc:1674: if (is_repost_event_ && (view_index ==
last_index_at_repost_)) {
line 1674 - 1683 could then be replaced with this:
 
if (is_repost_event_) 
  return;

last_pressed_index_ = view_index;

https://codereview.chromium.org/772963002/diff/160001/ash/shelf/shelf_view.h
File ash/shelf/shelf_view.h (right):

https://codereview.chromium.org/772963002/diff/160001/ash/shelf/shelf_view.h#...
ash/shelf/shelf_view.h:446: int last_index_at_repost_;
Sorry for this, but with the porposed changes below last_pressed_index_ is
perfect.

[xdai1, 2014-12-05 21:35:11]

Please take another look. Thanks:)

[Mr4D (OOO till 08-26), 2014-12-05 22:03:58]

lgtm!

[xdai1, 2014-12-05 22:14:40]

xdai@chromium.org changed reviewers:
+ sky@chromium.org

[xdai1, 2014-12-05 22:14:40]

Thanks Stefan! sky@, can you help to review the cl please? Thanks!

[sky, 2014-12-05 23:29:32]

LGTM

[xdai1, 2014-12-05 23:34:52]

The CQ bit was checked by xdai@chromium.org

[commit-bot: I haz the power, 2014-12-05 23:35:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/772963002/180001

[commit-bot: I haz the power, 2014-12-06 00:45:13]

Committed patchset #6 (id:180001)

[commit-bot: I haz the power, 2014-12-06 00:46:29]

Patchset 6 (id:??) landed as
https://crrev.com/a133d669d91b67f154ee7b07095e9bcad2303cbd
Cr-Commit-Position: refs/heads/master@{#307125}