Verdi [:verdi] Best to slack me Reporter
	Description • 12 years ago

When I try to set up sync in Firefox Metro I can successfully add the code to Firefox on my Mac and get the success screen. But in Firefox Metro the "Connect to Sync" screen with the message "Waiting for other device..." never goes away. If I wait a 10 min and hit Cancel, the button doesn't respond. The only way to get out of this is to open the task manager and quit Firefox Metro.

	Asa Dotzler [:asa]
	Comment 2 • 12 years ago

We've got something wrong with all of our Message Dialogs (that piece of WIndows 8 UI) where they cannot be dismissed. This might be related to that. If so, it's a dupe of bug 840855.

	Matt Brubeck (:mbrubeck) Assignee
	Comment 5 • 12 years ago

The root cause was an exception thrown by this line, which prevents WeaveGlue._init from running and leaves our sync UI code in a bad state:
http://hg.mozilla.org/mozilla-central/file/f2ec16a9feea/browser/metro/base/content/sync.js#l31

This line was broken because of the ".bind(this)" two lines further down.  This means that "onReady" is not actually added as the observer; the observer is actually a bound function that calls onReady, and there's no way to remove it.

This patch fixes the exception by adding WeaveGlue as the observer instead of a bound function.

	Allison Naaktgeboren :ally
	Comment 6 • 12 years ago

Comment on attachment 717356 [details] [diff] [review]
patch

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

r+*
This reasoning sounds sensible, the patch looks good, and patch works for me as expected. 

However, I'm going to double check with the sync owners to make sure this doesn't break a sync assumption. *(I doubt it, but please don't land until we hear back)

	Allison Naaktgeboren :ally
	Comment 7 • 12 years ago

talked to the sync group, you're good to go

	Matt Brubeck (:mbrubeck) Assignee
	Comment 8 • 12 years ago

Comment on attachment 717356 [details] [diff] [review]
patch

Setting an unowned r? flag as a reminder to myself to wait for a second review.

	Richard Newman [:rnewman]
	Comment 9 • 12 years ago

Comment on attachment 717356 [details] [diff] [review]
patch

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

Hoping I suffice as the second reviewer.

::: browser/metro/base/content/sync.js
@@ +25,5 @@
>      if (service.ready) {
>        this._init();
> +    } else {
> +      Services.obs.addObserver(this, "weave:service:ready", false);
> +      service.ensureLoaded();

I have a gentle preference for early return, rather than else, but nbd.

	Matt Brubeck (:mbrubeck) Assignee
	Comment 10 • 12 years ago

(Oops, obviously I missed comment 7 when adding comment 8.  But thanks for the review, Richard!)

	Matt Brubeck (:mbrubeck) Assignee
	Comment 11 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/f14dc834f18e

	Verdi [:verdi] Best to slack me Reporter
	Comment 12 • 12 years ago

Beautiful! I just found this bug in my metro awesome bar after successfully syncing.