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

RESOLVED FIXED in 2.2 S6 (20feb)
(NeedInfo from)

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: arogers, Assigned: jrburke, NeedInfo)

Tracking

unspecified
2.2 S6 (20feb)
Dependency tree / graph

Firefox Tracking Flags

(tracking-b2g:+, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(2 attachments)

Reporter

Description

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

Updated

5 years ago
feature-b2g: 2.2? → 2.2+
QA Whiteboard: [2.2-feature-qa+]

Comment 1

5 years ago
[Tracking Requested - why for this release]:
feature-b2g: 2.2+ → ---
tracking-b2g: --- → ?
QA Whiteboard: [2.2-feature-qa+]
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.
Assignee

Updated

4 years ago
Duplicate of this bug: 907028
Assignee

Updated

4 years ago
QA Contact: jrburke
Summary: Migrate Email to use requestSyncAPI (Phase1) → Migrate Email to use navigator.sync (RequestSync) API (Phase1)
Assignee: nobody → jrburke
QA Contact: jrburke
Assignee

Updated

4 years ago
Depends on: 1128065
Assignee

Updated

4 years ago
Depends on: 1128220
Assignee

Comment 6

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

Gelam pull request.
Attachment #8560671 - Flags: review?(bugmail)
Assignee

Comment 7

4 years ago
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.
Attachment #8560675 - Flags: review?(gaye)
Attachment #8560675 - Flags: review?(bugmail)
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.
Attachment #8560671 - Flags: review?(bugmail) → review+
Attachment #8560675 - Flags: review?(bugmail) → review+
Assignee

Updated

4 years ago
Target Milestone: --- → 2.2 S6 (20feb)
Assignee

Comment 9

4 years ago
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.
Attachment #8560675 - Flags: review?(gaye) → review?(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!
Attachment #8560675 - Flags: review?(kgrandon) → review+
Assignee

Updated

4 years ago
Keywords: checkin-needed
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.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(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.
Flags: needinfo?(jlal)
Assignee

Comment 15

4 years ago
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
Attachment #8560675 - Flags: approval-gaia-v2.2?
Depends on: 1132479
(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
Attachment #8560675 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
Assignee

Comment 18

4 years ago
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.
Depends on: 1133446
Assignee

Updated

4 years ago
Blocks: 1219853
You need to log in before you can comment on or make changes to this bug.