839460 - timeout in browser/metro/base/tests/browser_plugin_input_mouse.js with "Switch primary and secondary buttons" enabled

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

Attachment: patch

browser_plugin_input_mouse always times out on my laptop, and finally I figured out it was because I swapped my mouse buttons in the Mouse control panel (since I mouse with my left hand), but the test was always synthesizing events from the *physical* left button.

Switching to EventUtils.synthesizeMouse fixes the test for me; is it still testing the right thing or do we really require synthesizeNativeMouse?

Comment 1

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

The point of browser_plugin_input_mouse.js is to test whether the metro widget code is correctly forwarding mouse input to plugins.  I'm surprised that EventUtils.synthesizeMouse is causing the test to pass since I don't see any code in that path that forwards input to the plugin.  Regardless, using EventUtils.synthesizeMouse doesn't test the code path that we are trying to test with browser_plugin_input_mouse.js

I would consider modifying MetroWidget::SynthesizeNativeMouseEvent [1] to check whether "switch primary and secondary buttons" is enabled and switch the input if so.  The only issue there is that we may reasonably want to test whether our code handles that setting correctly, and we won't be able to write such tests if our function for synthesizing mouse events takes the setting into account.

[1] https://mxr.mozilla.org/projects-central/source/elm/widget/windows/winrt/MetroWidget.cpp#541

Comment 2

[Matt Brubeck (:mbrubeck)]

(In reply to Tim Abraldes (:tabraldes) from comment #1)
> I would consider modifying MetroWidget::SynthesizeNativeMouseEvent [1] to
> check whether "switch primary and secondary buttons" is enabled and switch
> the input if so.  The only issue there is that we may reasonably want to
> test whether our code handles that setting correctly, and we won't be able
> to write such tests if our function for synthesizing mouse events takes the
> setting into account.

Perhaps at the start of this test we should do "BOOL origValue = SwapMouseButtons(false)", and then restore the original value at the end of the test.

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

Implements the idea from comment 2.

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: patch v3

Sorry, just adding a comment and shuffling some lines around.

Comment 5

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 711971 [details] [diff] [review]
patch v2

Review of attachment 711971 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great!

Comment 6

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/projects/elm/rev/c0c66d93cb7d