563329 - Add a preference to enable/disable click hold context menus

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

Actually the maemo version of Fennec use click hold context menus to be able to show a context menus during a long touch (equivalent of a long left click). 

It would be nice to be able to simulate that on desktop.

Also the current duration before showing the context menus is hard coded, but 500ms is fine on desktop but it is too short on device.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2 - use a pref instead of a configure option

This patch use a pref instead of a configure option

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 443090 [details] [diff] [review]
Patch v0.2 - use a pref instead of a configure option

If the nits mentioned on IRC are fixed.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

...so, use AddObserver. And please add some tests if there aren't any
tests for this code.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: patch v0.2 - Fix nits

This patch fix nits from IRC.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #3)
> ...so, use AddObserver. And please add some tests if there aren't any
> tests for this code.

I'm looking for tests. Thanks for your quick reply.

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: patch v0.2 - Fix nits

I've left an error in the previous patch.
Tests will come.

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch + tests

Updated patch + tests.
I've send it to tryserver.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 443320 [details] [diff] [review]
Patch + tests

I've updated the patch on trunk and also i've changed the prefs in all.js compare to the previous patch just to be sure it is disabled by default on all platforms.

Everything went fine on try server.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 443320 [details] [diff] [review]
Patch + tests


> void
> nsEventStateManager::BeginTrackingDragGesture(nsPresContext* aPresContext,
>                                               nsMouseEvent* inDownEvent,
>                                               nsIFrame* inDownFrame)
> {
>+  NS_ASSERTION(inDownEvent->widget, "aEvent-widget is null");
>+
This assertion is useless. We crash if ->widget is null.


Do you really need to use so many timeouts in the test. That looks
pretty error-prone. Couldn't you just add event listeners and if 
contextmenu etc is dispatched, then the test passes?

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

(In reply to comment #9)
> (From update of attachment 443320 [details] [diff] [review])
> 
> > void
> > nsEventStateManager::BeginTrackingDragGesture(nsPresContext* aPresContext,
> >                                               nsMouseEvent* inDownEvent,
> >                                               nsIFrame* inDownFrame)
> > {
> >+  NS_ASSERTION(inDownEvent->widget, "aEvent-widget is null");
> >+

> This assertion is useless. We crash if ->widget is null.
> 

I've changed it with a check on widget, otherwise we crash if the event is generated using nsIDOMWindowUtils.dispatchDOMEventViaPresShell because inDownEvent->widget is null and we are using it to get the coordinate right after.

> 
> Do you really need to use so many timeouts in the test. That looks
> pretty error-prone. Couldn't you just add event listeners and if 
> contextmenu etc is dispatched, then the test passes?

I've rewrite the tests but I'm still keeping some setTimeout/executeSoon otherwise i'm running into some weird race conditions issue.

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Do I need to ask a sr for the patch?

Comment 12

[Justin Wood (:Callek)]

removing checkin-needed until c#11 is answered in some way.

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #11)
> Do I need to ask a sr for the patch?

no

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #13)
> (In reply to comment #11)
> > Do I need to ask a sr for the patch?
> 
> no

Thanks. I'll land it tomorrow if possible.

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

http://hg.mozilla.org/mozilla-central/rev/5e6cec74c4b4

Comment 16

[Marco Bonardo [:mak]]

disabled the test and filed Bug 572327 due to timeouts.

Comment 17

[Aakash Desai [:aakashd]]

verified FIXED on build:

Mozilla/5.0 (X11; U; Linux armv71; Nokia N900; en-US; rv:1.9.3a6pre) Gecko/2010621 Namoroka/3.7a6pre Fennec/2.0a1pre