Closed Bug 901852 Opened 9 years ago Closed 9 years ago

[sms][mms] don't run the time header scheduler if the page is hidden

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: julienw, Assigned: eragonj)

References

Details

(Whiteboard: [mentor=:julienw][good-first-bug])

Attachments

(1 file)

Currently the time header scheduler is run as long as the sms app is running. We should be good citizen and not wake up the device if we're hidden, contributing to save the user's battery.

There are 2 ways do to this:
* since the scheduler is started by thread_ui.js, this object should control this: listen to page visibility event, and start/stop the time header scheduler accordingly
* have everything handled inside utils.js. "startTimeHeaderScheduler" would set a boolean and listen to the page visibility event to react accordingly.

My preference is the first way. 

This is a small addition in terms of code but can contribute to save the battery, that's why I'm marking it to block.
not block koi but nice to have. please land it directly when you have the patch then it will be taken for v1.2 before 9/16 and v1.3 after 9/16
blocking-b2g: koi? → ---
Ccing a bunch of people that might be interested in taking this.

Also, currently the time header scheduler is actually started by ThreadUI but it's also used by ThreadListUI. Might be a good time to move all this in a separate object too (ping Rick ;) ).
Whiteboard: [mentor=:julienw][good-first-bug]
Assignee: nobody → ejchen
Hi Julien, I am interested in this bug and I just took it.
Julien, can you give me some feedback about my implementation when you are free ? Thanks a lot !!
Comment on attachment 814692 [details]
Pointer to Github pull request 12736.html

Hi EJ,
if you request someone's feedback, please use feedback flag within attachemnt.

Since we already listened the visibilitychanged event in message_manager and applied updateTimeHeaders to update header, maybe the proper way here is removing the header scheduler from thread_ui and replace updateTimeHeaders with timeheader scheduler in message_manager?

By the way, please don't forget the unit test for every patch ;)
Attachment #814692 - Flags: feedback?(felash)
(In reply to Steve Chung [:steveck](PTO 10/1~10/8) from comment #5)
> Comment on attachment 814692 [details]
> Pointer to Github pull request 12736.html
> 
> Hi EJ,
> if you request someone's feedback, please use feedback flag within
> attachemnt.
> 
> Since we already listened the visibilitychanged event in message_manager and
> applied updateTimeHeaders to update header, maybe the proper way here is
> removing the header scheduler from thread_ui and replace updateTimeHeaders
> with timeheader scheduler in message_manager?
> 
> By the way, please don't forget the unit test for every patch ;)

Thanks Steve, I forgot to add feedback? field. And I will add them later on next updated patch. Thanks.
Hello eragon! I left some comments on your patch, one of them for you and the other for julienw :)
(In reply to Rick Waldron from comment #7)
> Hello eragon! I left some comments on your patch, one of them for you and
> the other for julienw :)

Thanks Rick. I will keep that in mind :)
Comment on attachment 814692 [details]
Pointer to Github pull request 12736.html

Added comments on the pull request

might be a good time to extract all the time headers stuff in its own object :)
Attachment #814692 - Flags: feedback?(felash)
(and thanks EJ for taking it !)
Comment on attachment 814692 [details]
Pointer to Github pull request 12736.html

(In reply to Julien Wajsberg [:julienw] (in Paris-Web conference thursday and friday) from comment #10)
> (and thanks EJ for taking it !)

Thanks for giving me the chance to know more about sms, Julien. 

BTW, I just pushed the new one with unit test. please take a look when you have time ! Big thanks :P
Attachment #814692 - Flags: review?(felash)
Julien, I left some thinkings about implementations on Github. Please take a look (in outdated diff).

THanks !
Just answered on github, thanks !
Comment on attachment 814692 [details]
Pointer to Github pull request 12736.html

EJ, please request review again once you're ready.

Thanks !
Attachment #814692 - Flags: review?(felash)
OK! I will ping you back when I finished about the patch update.

Thanks Julien !!
Comment on attachment 814692 [details]
Pointer to Github pull request 12736.html

Julien I just made few changes about this patch, I left a long comments on Github, so please check them there. 

Thanks for your great help, I really appreciate it !
Attachment #814692 - Flags: review?(felash)
Comment on attachment 814692 [details]
Pointer to Github pull request 12736.html

Thanks a lot, this really looks better now :)

Just added more comments on github, I'm confident we can finish this soon ;)
Attachment #814692 - Flags: review?(felash)
Comment on attachment 814692 [details]
Pointer to Github pull request 12736.html

Julien, I followed your comments and updated the codebase a little bit. Please take a look when you have time :) thanks !!
Attachment #814692 - Flags: review?(felash)
Comment on attachment 814692 [details]
Pointer to Github pull request 12736.html

some more comments on github

please request review when you're ready, thanks !
Attachment #814692 - Flags: review?(felash)
Comment on attachment 814692 [details]
Pointer to Github pull request 12736.html

Hi Julien, in this change, I fixed jshint warnings at the same time. Please take a look when you have time !! thanks ;)
Attachment #814692 - Flags: review?(felash)
Comment on attachment 814692 [details]
Pointer to Github pull request 12736.html

So, some more comments on github about tests.
Next time will be ok I'm sure ;)
Attachment #814692 - Flags: review?(felash)
Blocks: 917639
Comment on attachment 814692 [details]
Pointer to Github pull request 12736.html

r=me after squash + rebase with a green travis

thanks !
Attachment #814692 - Flags: review+
(In reply to Julien Wajsberg [:julienw] from comment #22)
> Comment on attachment 814692 [details]
> Pointer to Github pull request 12736.html
> 
> r=me after squash + rebase with a green travis
> 
> thanks !

Thanks Julien, just rebased + squashed the commits. waiting for green light :)
Thanks all. 

Landed on master : b918db0e5dcf05c5807867a3f6b7c685388950f5.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 932318
You need to log in before you can comment on or make changes to this bug.