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

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

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.

Comment 1

[Joe Cheng [:jcheng] (please needinfo)]

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

Comment 2

[Julien Wajsberg [:julienw]]

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 ;) ).

Comment 3

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Hi Julien, I am interested in this bug and I just took it.

Comment 4

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Attachment: Pointer to Github pull request 12736.html

Julien, can you give me some feedback about my implementation when you are free ? Thanks a lot !!

Comment 5

[Steve Chung [:steveck]]

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 ;)

Comment 6

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(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.

Comment 7

[Rick Waldron [:rwaldron]]

Hello eragon! I left some comments on your patch, one of them for you and the other for julienw :)

Comment 8

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(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 9

[Julien Wajsberg [:julienw]]

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 :)

Comment 10

[Julien Wajsberg [:julienw]]

(and thanks EJ for taking it !)

Comment 11

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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

Comment 12

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Julien, I left some thinkings about implementations on Github. Please take a look (in outdated diff).

THanks !

Comment 13

[Julien Wajsberg [:julienw]]

Just answered on github, thanks !

Comment 14

[Julien Wajsberg [:julienw]]

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

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

Thanks !

Comment 15

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

OK! I will ping you back when I finished about the patch update.

Thanks Julien !!

Comment 16

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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 !

Comment 17

[Julien Wajsberg [:julienw]]

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 ;)

Comment 18

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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 !!

Comment 19

[Julien Wajsberg [:julienw]]

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 !

Comment 20

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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 ;)

Comment 21

[Julien Wajsberg [:julienw]]

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 ;)

Comment 22

[Julien Wajsberg [:julienw]]

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

r=me after squash + rebase with a green travis

thanks !

Comment 23

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(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 :)

Comment 24

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Thanks all. 

Landed on master : b918db0e5dcf05c5807867a3f6b7c685388950f5.