Converting (Alt+LeftClick -> RightClick) to (Search+LeftClick -> RightClick)

Converting (Alt+LeftClick -> RightClick) to (Search+LeftClick -> RightClick)

Some web apps have behaviors for Alt+LeftClick so we had to do this
conversion.

However this change will result in popping up the AppList whenever
Search+Click is pressed and then search is released while in a web
app that consumes the Search+Click by showing a context menu and
then prevents default. This context menu differs from the native
context menu in that it doesn't have its own MenuEventDispatcher.
So the Search release event is passed to the accelerator controller
and then the controller thinks the both current and previous
accelerators are the "Search" key.

We had to fix this by listening to mouse events in the AcceleratorFilter
and then clear the current accelerator in the accelerator history.

BUG=248762
TEST=unit_tests --gtest_filter=EventRewriterTest.*

Committed: https://crrev.com/404d0118fa4c40676cbd2f6da3e5ba5d62f39b61
Cr-Commit-Position: refs/heads/master@{#328213}

[afakhry, 2015-04-30 23:54:41]

Patchset #1 (id:1) has been deleted

[afakhry, 2015-04-30 23:58:41]

afakhry@chromium.org changed reviewers:
+ derat@chromium.org, sadrul@chromium.org

[afakhry, 2015-04-30 23:58:42]

sadrul@chromium.org: Please review changes in:
- accelerator_filter.*

derat@chromium.org: Please review changes in:
- event_rewriter*

Thanks!

[Daniel Erat, 2015-05-01 03:06:00]

derat@chromium.org changed reviewers:
+ kpschoedel@chromium.org

[Daniel Erat, 2015-05-01 03:06:00]

kpschoedel@ is a better reviewer for c/b/chromeos/events than me. i'm happy to
approve for c/b/chromeos, though.

[Daniel Erat, 2015-05-01 03:12:21]

https://codereview.chromium.org/1117173003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/events/event_rewriter_unittest.cc (right):

https://codereview.chromium.org/1117173003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/events/event_rewriter_unittest.cc:2094:
EXPECT_FALSE((ui::EF_LEFT_MOUSE_BUTTON | ui::EF_COMMAND_DOWN) ^
what's the reason for the change to EXPECT_FALSE and xor? mind adding a comment
explaining this?

[sadrul, 2015-05-01 03:21:18]

https://codereview.chromium.org/1117173003/diff/20001/ui/wm/core/accelerator_...
File ui/wm/core/accelerator_filter.cc (right):

https://codereview.chromium.org/1117173003/diff/20001/ui/wm/core/accelerator_...
ui/wm/core/accelerator_filter.cc:77: // a default empty key accelerator.
Interesting! Would it be possible to write a test for this?

It is possible that some synthesized mouse event is generated in response to
some accelerator (e.g. if a new window is created under the mouse-cursor, or the
window under the cursor goes away). Would that cause any issues?

[kpschoedel, 2015-05-01 14:17:25]

EventRewriter LGTM

https://codereview.chromium.org/1117173003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/events/event_rewriter_unittest.cc (right):

https://codereview.chromium.org/1117173003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/events/event_rewriter_unittest.cc:2094:
EXPECT_FALSE((ui::EF_LEFT_MOUSE_BUTTON | ui::EF_COMMAND_DOWN) ^
I see the original succeeds if only *one* of the flags is set, which was
probably not the intent, but maybe an EXPECT_EQ would be clearer.

[afakhry, 2015-05-02 00:02:43]

https://codereview.chromium.org/1117173003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/events/event_rewriter_unittest.cc (right):

https://codereview.chromium.org/1117173003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/events/event_rewriter_unittest.cc:2094:
EXPECT_FALSE((ui::EF_LEFT_MOUSE_BUTTON | ui::EF_COMMAND_DOWN) ^
On 2015/05/01 14:17:25, kpschoedel wrote:
> I see the original succeeds if only *one* of the flags is set, which was
> probably not the intent, but maybe an EXPECT_EQ would be clearer.

Done.

https://codereview.chromium.org/1117173003/diff/20001/ui/wm/core/accelerator_...
File ui/wm/core/accelerator_filter.cc (right):

https://codereview.chromium.org/1117173003/diff/20001/ui/wm/core/accelerator_...
ui/wm/core/accelerator_filter.cc:77: // a default empty key accelerator.
On 2015/05/01 03:21:18, sadrul wrote:
> Interesting! Would it be possible to write a test for this?
> 
> It is possible that some synthesized mouse event is generated in response to
> some accelerator (e.g. if a new window is created under the mouse-cursor, or
the
> window under the cursor goes away). Would that cause any issues?

I don't this will create any issues. There are only two accelerators in the
AcceleratorController::CanPerformAction() for which we need to check the
previous accelerator, namely toggle applist and next IME. I tested both with
this change and they're working fine.

But as an extra care, in the second patch I'm ignoring synthesized mouse events.

I also added a test that fails without this code in
|accelerator_filter_unittest.cc|

[Daniel Erat, 2015-05-04 15:40:02]

lgtm

[sadrul, 2015-05-04 17:16:17]

It would be nice to move accelerator_filter_unittest.cc to live closer to
accelerator_filter.cc (not in this CL, of course).

I have left a couple of comments about the synthesized mouse events case. LGTM
with those addressed.

https://codereview.chromium.org/1117173003/diff/40001/ash/accelerators/accele...
File ash/accelerators/accelerator_filter_unittest.cc (right):

https://codereview.chromium.org/1117173003/diff/40001/ash/accelerators/accele...
ash/accelerators/accelerator_filter_unittest.cc:147:
generator.ReleaseLeftButton();
ClickLeftButton() instead of Press/Release?

https://codereview.chromium.org/1117173003/diff/40001/ash/accelerators/accele...
ash/accelerators/accelerator_filter_unittest.cc:149:
EXPECT_FALSE(ash::Shell::GetInstance()->GetAppListTargetVisibility());
If you leave the synth. bit in AcceleratorFilter, can you update this test to
add a case where generating a synth mouse-event doesn't reset the state?

https://codereview.chromium.org/1117173003/diff/40001/ui/wm/core/accelerator_...
File ui/wm/core/accelerator_filter.cc (right):

https://codereview.chromium.org/1117173003/diff/40001/ui/wm/core/accelerator_...
ui/wm/core/accelerator_filter.cc:81: return;
Remove the check for synthesized event if it isn't necessary. If it is
necessary, please update the comment to explain *why* the synthesized mouse
events are ignored here.

[afakhry, 2015-05-04 20:17:08]

https://codereview.chromium.org/1117173003/diff/40001/ash/accelerators/accele...
File ash/accelerators/accelerator_filter_unittest.cc (right):

https://codereview.chromium.org/1117173003/diff/40001/ash/accelerators/accele...
ash/accelerators/accelerator_filter_unittest.cc:147:
generator.ReleaseLeftButton();
On 2015/05/04 17:16:17, sadrul wrote:
> ClickLeftButton() instead of Press/Release?

Done.

https://codereview.chromium.org/1117173003/diff/40001/ash/accelerators/accele...
ash/accelerators/accelerator_filter_unittest.cc:149:
EXPECT_FALSE(ash::Shell::GetInstance()->GetAppListTargetVisibility());
On 2015/05/04 17:16:17, sadrul wrote:
> If you leave the synth. bit in AcceleratorFilter, can you update this test to
> add a case where generating a synth mouse-event doesn't reset the state?

Done.

https://codereview.chromium.org/1117173003/diff/40001/ui/wm/core/accelerator_...
File ui/wm/core/accelerator_filter.cc (right):

https://codereview.chromium.org/1117173003/diff/40001/ui/wm/core/accelerator_...
ui/wm/core/accelerator_filter.cc:81: return;
On 2015/05/04 17:16:17, sadrul wrote:
> Remove the check for synthesized event if it isn't necessary. If it is
> necessary, please update the comment to explain *why* the synthesized mouse
> events are ignored here.

Done.

[afakhry, 2015-05-04 23:21:29]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2015-05-04 23:21:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
kpschoedel@chromium.org, sadrul@chromium.org, derat@chromium.org
Link to the patchset: https://codereview.chromium.org/1117173003/#ps60001
(title: "Added a test case for ignoring synthesized mouse events.")

[commit-bot: I haz the power, 2015-05-04 23:22:12]

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

[commit-bot: I haz the power, 2015-05-04 23:27:38]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-05-04 23:28:46]

Patchset 3 (id:??) landed as
https://crrev.com/404d0118fa4c40676cbd2f6da3e5ba5d62f39b61
Cr-Commit-Position: refs/heads/master@{#328213}