Closed Bug 1091161 Opened 5 years ago Closed 5 years ago

Separate gInitializeTimerFunc from the actual initialize callback so we can retry initialization on demand

Categories

(Hello (Loop) :: Client, defect, P2)

defect
Points:
2

Tracking

(firefox35 fixed, firefox36 fixed)

RESOLVED FIXED
mozilla36
Iteration:
36.2
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
Blocking Flags:
backlog Fx35+

People

(Reporter: MattN, Assigned: MattN)

References

Details

Attachments

(1 file)

Attached patch v.1 patchSplinter Review
This moves the initialize callback function called after the timer to its own function so if we want to retry a failed initialization, we can just call that new function instead of waiting for the timer again if we called gInitializeTimerFunc. This is needed for the retry button in bug 1074670.

More work may be needed in a follow-up to make sure delayedInitialize can be retried successfully to resolve failures at any point in initialization.

Other cleanup:
* flattened MozLoopService.register to just call MozLoopServiceInternal.promiseRegisteredWithServers since the pref checks didn't seemed to just be duplicated without a good reason I could see. I also renamed it to promiseRegisteredWithServers to reduce the number of function names which added confusion.
* switched to Timer.jsm to get rid of the timer global

All tests pass for me.
Attachment #8513721 - Flags: review?(pkerr)
Flags: qe-verify-
Flags: firefox-backlog+
Needed for bug 1074670
backlog: --- → Fx35+
Priority: -- → P2
Comment on attachment 8513721 [details] [diff] [review]
v.1 patch

Review of attachment 8513721 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/MozLoopService.jsm
@@ +947,5 @@
> +   * Can be called more than once (e.g. if the initial setup fails at some phase).
> +   * @param {Deferred} deferredInitialization
> +   */
> +  delayedInitialize: Task.async(function*(deferredInitialization) {
> +    yield this.promiseRegisteredWithServers().then(Task.async(function*() {

The logic flow, I think, is correct. Is there a reason you cannot directly use the yield statements to step through the asynchronous promise returning calls without the nested then() calls?
Attachment #8513721 - Flags: review?(pkerr) → review+
(In reply to Paul Kerr [:pkerr] from comment #2)
> The logic flow, I think, is correct. Is there a reason you cannot directly
> use the yield statements to step through the asynchronous promise returning
> calls without the nested then() calls?

The logic flow was the same as before for the most part. I had tried to get rid of the the nesting the other day but ran into an issue. I tried again after your review comment and now it looks much nicer. Good call!

Pushed: https://hg.mozilla.org/integration/fx-team/rev/0b4f5a605288
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/0b4f5a605288
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla36
Comment on attachment 8513721 [details] [diff] [review]
v.1 patch

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