Closed Bug 1093500 Opened 5 years ago Closed 5 years ago

Cleanup registration by pulling push URLs from the push handler

Categories

(Hello (Loop) :: Client, defect)

defect
Not set
Points:
5

Tracking

(firefox35 fixed, firefox36 fixed)

RESOLVED FIXED
mozilla36
Iteration:
36.2
Tracking Status
firefox35 --- fixed
firefox36 --- fixed

People

(Reporter: MattN, Assigned: MattN)

References

Details

User Story

* Factored push registration out to promiseRegisteredWithPushServer
* registerForNotification will use an existing endpoint and resolve the promise with it, if it exists
* pushURLs aren't passed around as registration function arguments so that we don't need to duplicate logic to pull existing endpoints from the pushHandler in loginToFxA and to implement retry (bug 1074670). registerWithLoopServer pulls them from the push handler.
* Removes the FxA login dependency on gRegisterredDeferred (and therefore the assumption that a guest registration is required before an FxA one) - Bug 1076428
* Replace gRegisteredDeferred with a map called deferredRegistrations which also handles FxA registrations
* Handling of the registration deferreds is more centralized making logic easier to follow
* client code now ensures the proper registration instead of always the guest one (which isn't useful and somewhat wasteful if the user is logged in)

Attachments

(1 file, 1 obsolete file)

This is part of bug 1074670 which needs registration methods to be called more than once.
Flags: qe-verify-
Flags: firefox-backlog+
/r/163 - Bug 1093500 - Cleanup Loop registration by pulling push URLs from the push handler.

Pull down this commit:

hg pull review -r 8e192f9fc25303121de23f6226e961fd4e412498
/r/163 - Bug 1093500 - Cleanup Loop registration by pulling push URLs from the push handler. r=pkerr

Pull down this commit:

hg pull review -r 4b34ae4bb4ce46b00293f16619e6bdf78762997f
https://reviewboard.mozilla.org/r/161/#review73

::: browser/components/loop/test/xpcshell/test_loopservice_restart.js
(Diff revision 2)
> -    Assert.equal(MozLoopServiceInternal.pushHandler.pushUrl, kEndPointUrl, "Push URL should match");
> +    Assert.equal(MozLoopServiceInternal.pushHandler.registrationPushURL, kEndPointUrl, "Push URL should match");

This was broken but went unnoticed until my patch exposed it.

::: browser/components/loop/test/xpcshell/test_loopservice_restart.js
(Diff revision 2)
> -  mockPushHandler.pushUrl = kEndPointUrl;
> +  mockPushHandler.registrationPushURL = kEndPointUrl;

ditto

::: browser/components/loop/test/desktop-local/conversation_test.js
(Diff revision 2)
> +      LOOP_SESSION_TYPE: {
> +        GUEST: 1,
> +        FXA: 2
> +      },

Needed for the move to use LOOP_SESSION_TYPE in requestCallUrl

::: browser/components/loop/MozLoopService.jsm
(Diff revision 2)
> +    log.debug("hawkRequest: " + path, sessionType);

This is useful since regular hawk debug logging doesn't clearly indicate the sessionType of the request.
https://reviewboard.mozilla.org/r/163/#review81

::: browser/components/loop/MozLoopService.jsm
(Diff revision 2)
> +    let deferred = Promise.defer();

How long will Promise.defer() be available for use in a JSM? There is already an issue in some of the test environments dropping support for this method.
Comment on attachment 8516542 [details]
MozReview Request: bz://1093500/MattN

Review comments posted at:

https://reviewboard.mozilla.org/r/163/
Attachment #8516542 - Flags: review?(pkerr) → review+
https://reviewboard.mozilla.org/r/163/#review85

> How long will Promise.defer() be available for use in a JSM? There is already an issue in some of the test environments dropping support for this method.

I'm not sure but I don't think it's a problem for now since there are other uses in this file.
https://hg.mozilla.org/mozilla-central/rev/6d819f81dcbd
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla36
Comment on attachment 8516542 [details]
MozReview Request: bz://1093500/MattN

Approval Request Comment
Landed on aurora per IRC with lsblakk with a=loop-only
Attachment #8516542 - Flags: approval-mozilla-aurora?
Attachment #8516542 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8516542 - Attachment is obsolete: true
Attachment #8618546 - Flags: review+
You need to log in before you can comment on or make changes to this bug.