1098289 - Migrate Email to use navigator.sync (RequestSync) API (Phase1)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Description

[Adam Rogers (:arog)]

Migrate the email application to use requestSync API instead of the alarm API for background sync operations.

Comment 1

[Jacob Shen]

[Tracking Requested - why for this release]:

Comment 2

[Dylan Oliver [:doliver]]

triage: tracking+ for this bug -- if time permits after the landing of Bug 1018320 we will try to make progress on this for 2.2.

Comment 4

[GitHub Autolander Bot]

Attachment: [PullReq] jrburke:bug1098289-email-request-sync to mozilla-b2g:master

Comment 5

[GitHub Autolander Bot]

Attachment: [PullReq] jrburke:bug1098289-email-request-sync to mozilla-b2g:master

Comment 6

[James Burke [:jrburke]]

Comment on attachment 8560671 [details] [review]
[PullReq] jrburke:bug1098289-email-request-sync to mozilla-b2g:master

Gelam pull request.

Comment 7

[James Burke [:jrburke]]

Comment on attachment 8560675 [details] [review]
[PullReq] jrburke:bug1098289-email-request-sync to mozilla-b2g:master

Gaia pull request. Includes the GELAM changes for easier "download and install" of one patch. Asking :asuth for the gaia email parts.

Asking :gaye to give a review of just the mock_navigator_moz_set_message_handler.js change, since he last touched that file. It is a small change, should hopefully be a straightforward inspection, tried to keep it backwards compatible.

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8560671 [details] [review]
[PullReq] jrburke:bug1098289-email-request-sync to mozilla-b2g:master

r=asuth with the test_account_updates test race fix addressed/worked-around.

Comment 9

[James Burke [:jrburke]]

Comment on attachment 8560675 [details] [review]
[PullReq] jrburke:bug1098289-email-request-sync to mozilla-b2g:master

Looking closer at the Firefox Modules page and recent activity in the shared test folder, and :gaye's unavailability at the moment, asking :kgrandon for review for just the mock_navigator_moz_set_message_handler.js change, as it is in shared/test.

It is a small change, should hopefully be a straightforward inspection, kept the file backwards compatible. Treeherder build passes for the tests it is involved in. There was an Li failure (apparently missed locally due to a rebase maybe), but I have since fixed it and Li will pass once the automation kicks in again.

Comment 10

[Kevin Grandon :kgrandon]

Comment on attachment 8560675 [details] [review]
[PullReq] jrburke:bug1098289-email-request-sync to mozilla-b2g:master

The changes in shared/ look fine to me. I just glanced over them so please make sure we have a green on gaia-try before landing. Thanks!

Comment 11

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#bOPdd-A-SaKynmhcH_Plng

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 12

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/c23c5bce1322a758a6fbc2e63c6029466041e7b0

Comment 13

[Kevin Grandon :kgrandon]

(In reply to Autolander from comment #11)
> http://docs.taskcluster.net/tools/task-graph-inspector/#bOPdd-A-
> SaKynmhcH_Plng
> 
> The pull request failed to pass integration tests. It could not be landed,
> please try again.

Interesting, so travis seems to be passing on GELAM, but taskcluster-based tests are not? Seems like we need to get that fixed if we want to use autolander. Any idea jlal?

jrburke - it appears that the GELAM patch didn't land yet, might want to press the merge button.

Comment 14

[James Burke [:jrburke]]

GELAM merged to master:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/dc1bdadefa7b970c589d6a206975f42460fd9d39

from pull request:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/368

Left some notes on the test runs I did before merging here:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/368#issuecomment-73975111

Comment 15

[James Burke [:jrburke]]

Comment on attachment 8560675 [details] [review]
[PullReq] jrburke:bug1098289-email-request-sync to mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
New feature, uses the request-sync API, bug 1018320, also a new feature for 2.2.

[User impact] if declined:
Possibility of worse battery usage -- using navigator.sync is supposed to help manage sync tasks better with network and grouping awareness. Old approach used mozAlarms will be used.

[Testing completed]:
Tested on flame device in limited syncing runs.

[Risk to taking this patch] (and alternatives if risky):
The navigator.sync plumbing is new to 2.2, so there is risk that it could have bugs, which could include not syncing close enough to user expectations, dropping syncs, or syncing too much. However, it is likely best to discover those by using the API in a gaia app.

As for the email changes specifically, it was more of a mechanical translation from the mozAlarms API, and the manual testing locally indicates the wiring was done correctly.

[String changes made]:
none

Comment 16

[bhavana bajaj [:bajaj]]

(In reply to James Burke [:jrburke] from comment #15)
> Comment on attachment 8560675 [details] [review]
> [PullReq] jrburke:bug1098289-email-request-sync to mozilla-b2g:master
> 
> [Approval Request Comment]
> [Bug caused by] (feature/regressing bug #):
> New feature, uses the request-sync API, bug 1018320, also a new feature for
> 2.2.
> 
> [User impact] if declined:
> Possibility of worse battery usage -- using navigator.sync is supposed to
> help manage sync tasks better with network and grouping awareness. Old
> approach used mozAlarms will be used.
> 
> [Testing completed]:
> Tested on flame device in limited syncing runs.
> 
> [Risk to taking this patch] (and alternatives if risky):
> The navigator.sync plumbing is new to 2.2, so there is risk that it could
> have bugs, which could include not syncing close enough to user
> expectations, dropping syncs, or syncing too much. However, it is likely
> best to discover those by using the API in a gaia app.
given the timeline hoping we can manage the fallouts. Please do get in touch with your functional area QA to see if any additional testing will be useful here.
> 
> As for the email changes specifically, it was more of a mechanical
> translation from the mozAlarms API, and the manual testing locally indicates
> the wiring was done correctly.
> 
> [String changes made]:
> none

Comment 17

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/201dfab02cfa1281651a73c69ec04e27cc64328f

Comment 18

[James Burke [:jrburke]]

The change for v2.2 was reverted because navigator.sync is not in the gecko 37 used in the branch. Revert commit:

https://github.com/mozilla-b2g/gaia/commit/da509caa7395d3d090ce973e8de082b4680a590d

I am following up on the status of navigator.sync feature for 2.2 in bug 1018320 comment 88.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

This time with feeling.

v2.2: https://github.com/mozilla-b2g/gaia/commit/0784b5ea817eea6f575cfd1d55eeb975067b4268