858454 - mozKeyboard.sendKey takes more than 10ms

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

Description

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

Calling mozKeyboard.sendKey cost a bit more than 10ms in the click method of the latin.js. This is likely before a call to this API fire synchronously 3 calls to nsIDOMWindowUtils.sendKeyEvent that triggers a widget->dispatchEvent.
I don't know if those calls are synced or not?

A simple "fix" is to wrap the API call into a setTimeout(..., 0) so it does not block user actions and will be postpone when there is some times on the event loop.

A smarter fix could be to do that inside the mozKeyboard.sendKey code.

The right fix involves understanding is widget->dispatchEvent is sync or not and in this case I still believe that setTimeout(..., 0) is not that bad.The main issue here being that the keyboard lives on the main thread and the mozKeyboard api is not e10s ready.

Comment 1

[Andreas Gal :gal]

Yep, those calls are sync because they return whether you took the default action.

Comment 2

[Andreas Gal :gal]

Attachment: send keyboard events from the event loop, not sync

Comment 3

[Andreas Gal :gal]

Attachment: send keyboard events from the event loop, one by one, to avoid hogging the event loop for too long

Comment 4

[Andreas Gal :gal]

Attachment: send keyboard events from the event loop, one by one, to avoid hogging the event loop for too long

Comment 5

[Andreas Gal :gal]

Please steal and land when you r+. Thanks!

Comment 6

[[:fabrice] Fabrice Desré]

Comment on attachment 734290 [details] [diff] [review]
send keyboard events from the event loop, one by one, to avoid hogging the event loop for too long

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

lgtm, but I did not run any timing to check if we get the improvements we expect.

Comment 7

[Andreas Gal :gal]

Attachment: tested version with a few fixes, carrying forward r=fabrice

Comment 8

[Andreas Gal :gal]

https://hg.mozilla.org/integration/mozilla-inbound/rev/db5fe4e235ee

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/db5fe4e235ee

Comment 10

[Andreas Gal :gal]

Comment on attachment 734341 [details] [diff] [review]
tested version with a few fixes, carrying forward r=fabrice

I tested this on device and this definitely feels more performant so I think we should take this for v1.1

Comment 11

[Ryan VanderMeulen [:RyanVM]]

I assume that was meant to be an approval-b2g18?

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/0e919898ca8a