Closed
Bug 1184012
Opened 9 years ago
Closed 9 years ago
[Messages] Throttle event handlers for the "input" event handlers
Categories
(Firefox OS Graveyard :: Gaia::SMS, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
FxOS-S7 (18Sep)
People
(Reporter: julienw, Assigned: stephane.roucheray, Mentored)
Details
(Whiteboard: [sms-papercuts][lang=js][good first bug])
Attachments
(1 file)
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•9 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•9 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•9 years ago
|
||
I am working on this one.
Comment 4•9 years ago
|
||
Assignee | ||
Comment 5•9 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.
Comment 6•9 years ago
|
||
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•9 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•9 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)
Comment 9•9 years ago
|
||
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•9 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.
Comment 11•9 years ago
|
||
(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•9 years ago
|
||
Steve, I think I fixed all the last things you commented. Tell me if that's ok for you!
Comment 13•9 years ago
|
||
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•9 years ago
|
||
This has been a great first experience and thanks for your comments and feedbacks !
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Reporter | ||
Updated•9 years ago
|
Mentor: felash → schung
Comment 15•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S7 (18Sep)
Updated•9 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 16•9 years ago
|
||
Thanks Stéphane !
You need to log in
before you can comment on or make changes to this bug.
Description
•