[Messages] Throttle event handlers for the "input" event handlers

RESOLVED FIXED in FxOS-S7 (18Sep)

Status

Firefox OS
Gaia::SMS
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: julienw, Assigned: Stéphane Roucheray, Mentored)

Tracking

unspecified
FxOS-S7 (18Sep)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sms-papercuts][lang=js][good first bug])

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
There are several places where we use "input" handlers. Problem is that the "input" event is sent synchronously for each change. We should throttle this event so that we don't block on this event each time something changes.

We also need to check the "key*" events; while some should still be synchronous, some could be made asynchronous (eg: the "keypress" event in conversation.js).

We should add a "throttle" function in utils.js and use it in the appropriate places.
(Reporter)

Comment 1

3 years ago
(It's more "debounce" than "throttle" here: we don't want the function to run "right now").
Summary: [Messages] Throttle event handlers for the "input" event handlers → [Messages] Debounce event handlers for the "input" event handlers
(Reporter)

Comment 2

3 years ago
OK, it's really throttling but without the first execution ;)
Summary: [Messages] Debounce event handlers for the "input" event handlers → [Messages] Throttle event handlers for the "input" event handlers
(Assignee)

Comment 3

2 years ago
I am working on this one.
Created attachment 8656544 [details] [review]
[gaia] sroucheray:add-throttle > mozilla-b2g:master
(Assignee)

Comment 5

2 years ago
I implemented Utils.throttle and its unit tests.
I used throttling with the "to" field keyboard handlers in conversation.js. The metrics might require adjustments. For now it's 300ms delay.
I was not able to test it on a real device.
Hi Stéphane,

If you don't have the device, maybe you can try out the mulet or B2G desktop[1] here. I assign the bug to you since you already created the patch. Please remember to ask the review on the patch(enter the detail and set the review flag with one of the peer) I saw your commit message that you might want julien to review your patch, so I simply left some comments without testing. Just a reminder that Julien is in PTO now so you can ask for review from :azasypkin(also in PTO but will be online next Monday) or me :)

BTW I saw some failed unit tests about the recipient behavior in conversation-test.js. Please also adjust the test in conversation-test.js if necessary, thanks!

[1]https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Developing_Gaia/Different_ways_to_run_Gaia
Assignee: nobody → stephane.roucheray
(Assignee)

Comment 7

2 years ago
Hi Steve,

I have answered some of your comments and modified the code. I fixed the conversion_test.js test failures.
This is my first patch so any new comments (about the code as well as about the process) is very welcome :)
(Assignee)

Comment 8

2 years ago
Comment on attachment 8656544 [details] [review]
[gaia] sroucheray:add-throttle > mozilla-b2g:master

>https://github.com/mozilla-b2g/gaia/pull/31677
Attachment #8656544 - Flags: review?(schung)
Hi Stéphane,

I left some comment on the github. Mostly are about unit test and convention, and one question about the preventLastcall. Please ping me when you're ready, thanks!
(Assignee)

Comment 10

2 years ago
Hi Steve,

I answered most of your comments on github and changed the throttle signature and associated tests.
Please have a look and give me your feedbacks ! Thanks.
(In reply to Stéphane Roucheray from comment #10)
> Hi Steve,
> 
> I answered most of your comments on github and changed the throttle
> signature and associated tests.
> Please have a look and give me your feedbacks ! Thanks.

Ok, it really close but just last few nits! The question I asked about https://github.com/mozilla-b2g/gaia/pull/31677/files#diff-547b790d842789bb9f2a73a778bc81cfR760 is because I'm a little bit confused about the why we didn't reset previous to 0 for non-preventFisrtCall case, but now I realize that you still want to check the duration for next call, not just treat it as the first call. Sorry about the noise :/
(Assignee)

Comment 12

2 years ago
Steve, I think I fixed all the last things you commented. Tell me if that's ok for you!
Comment on attachment 8656544 [details] [review]
[gaia] sroucheray:add-throttle > mozilla-b2g:master

Looks great and thanks for your contribution! Please squash your commits into one commit and request "checkin-needed" in the Keywords fields.
Attachment #8656544 - Flags: review?(schung) → review+
(Assignee)

Comment 14

2 years ago
This has been a great first experience and thanks for your comments and feedbacks !
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Reporter)

Updated

2 years ago
Mentor: felash@gmail.com → schung@mozilla.com
https://github.com/mozilla-b2g/gaia/commit/e0bd0ec64986fe7b99699434da502404fb6ae128
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S7 (18Sep)

Updated

2 years ago
Keywords: checkin-needed
(Reporter)

Comment 16

2 years ago
Thanks Stéphane !
You need to log in before you can comment on or make changes to this bug.