Closed Bug 845468 Opened 7 years ago Closed 6 years ago

Change - Use Win8 UI elements for Sync set up and pairing

Categories

(Firefox for Metro Graveyard :: Sync, defect)

x86
Windows 8.1
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ywang, Assigned: TimAbraldes)

References

Details

Attachments

(12 files, 6 obsolete files)

1.51 MB, image/png
Details
1.51 MB, image/png
Details
745.39 KB, image/png
Details
1.49 MB, image/png
Details
1.47 MB, image/png
Details
1.48 MB, image/png
Details
1.49 MB, image/png
Details
1.48 MB, image/png
Details
3.29 KB, image/png
Details
1.37 KB, image/png
Details
591 bytes, patch
ally
: review+
Details | Diff | Splinter Review
117.60 KB, patch
ally
: review+
Details | Diff | Splinter Review
Currently Sync set up and pairing trigger the information dialog/banner. We should change that to side flyout, since Sync is not an urgent task that requires the users' action right away.

Will attach the interaction flows and detail spec soon
Blocks: metrov2planning
No longer blocks: metrov1triage
general->sync 10 bugs
Component: General → Sync
Blocks: metrov2defect&change
No longer blocks: metrov2planning
This is blocking my work in bug 866304 so I'll take a look and see if I can knock this out quickly.
Assignee: nobody → tabraldes
Status: NEW → ASSIGNED
Blocks: 866304
No longer blocks: metrov2defect&change
Note: The "Plus" icon and the texts to the right:"Set up sync"/"Pair a device" should both be click-able/touchable.
Blocks: 831978
No longer blocks: 831978
Blocks: 886563
Blocks: 831978
No longer blocks: 886563
Attached patch Patch v1 (obsolete) — Splinter Review
Attached patch Part 2 Patch v1 (obsolete) — Splinter Review
Splitting the patch up to make the diff cleaner
Attachment #767862 - Attachment is obsolete: true
Attached patch Part 1 Patch v1Splinter Review
This patch exists solely to clean up the diff.  This must be applied before the part 2 patch.
Attachment #767875 - Flags: review?(fyan)
Comment on attachment 767873 [details] [diff] [review]
Part 2 Patch v1

This patch organizes our existing flyouts into a "flyouts" subdirectory of browser/metro/base/content, and implements the sync flyout.

The part 1 patch must be applied before this patch.
Attachment #767873 - Flags: review?(fyan)
Attachment #767873 - Flags: review?(fyan) → review?(ally)
Attachment #767875 - Flags: review?(fyan) → review?(ally)
Comment on attachment 767873 [details] [diff] [review]
Part 2 Patch v1

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

r+ with nits, the localization in particular needs some work.

I was unable to get the first patch to apply, so I trust that you've run & tested your own code. :)

::: browser/metro/base/content/browser-ui.js
@@ +2001,2 @@
>      });
> +    // Sync

thanks for removing that nasty whitespace :)

::: browser/metro/base/content/browser.xul
@@ +144,5 @@
>      <key id="key_quit" key="q" modifiers="accel" command="cmd_quit"/>
>      <key id="key_addBoomkark" key="d" modifiers="accel" command="cmd_addBookmark"/>
>      <key id="key_console" key="j" modifiers="accel,shift" oncommand="PanelUI.show('console-container')"/>
> +    <key id="key_options"
> +         key="o"

we don't really respect an 80 character/line limit in the metro code base, and these now stick out oddly.
I'd stick with the original all on one line layout to match the other <key>s
If you have a compelling reason, I'm open to being persuaded otherwise.

@@ +433,5 @@
>              onclick="if (event.button == 0) { Browser.onAboutPolicyClick(); }"
>              class="text-link" value="&aboutHeader.policy.label;"/>
>      </flyoutpanel>
>  
> +#ifdef MOZ_SERVICES_SYNC

It looks like you did a good job of preserving the MOZ_SERVICES_SYNC wrapping everywhere. Thanks! However, before you push to m-c, you may want to do a try push to ensure we don't break Seamonkey.

If you decide not to do that, be sure to keep an eye on the seamonkey builds when it hits m-c just in case.

@@ +457,5 @@
> +        <separator />
> +        <vbox flex="1" pack="center" align="start">
> +          <description id="sync-setup-code1"
> +                       class="syncJPAKECode">
> +            ....

nit So we have the '....' here & later in the js. I would rather we store a 'default jpake string' somewhere and use that than have 6 '....'.

@@ +462,5 @@
> +          </description>
> +          <description id="sync-setup-code2"
> +                       class="syncJPAKECode">
> +            ....
> +          </description>

might be a good candidate for the below nit/grumble

@@ +486,5 @@
> +        </description>
> +        <separator />
> +        <description>&sync.flyout.setup.description3;</description>
> +        <description>&sync.flyout.setup.description4;</description>
> +        <description class="syncInstructionText">

General nit/grumble: This file is very inconsistent about when all attributes inside of a given tag go on one line versus multiple lines. I don't expect you to fix the world, but you might consider condensing some of the shorter ones onto one line such as  the <description> &sync.flyout.setup.description5.

::: browser/metro/base/content/flyouts/aboutFlyout.js
@@ +6,3 @@
>  
> +Cu.import("resource://gre/modules/Services.jsm");
> +var gAppUpdater;

nit: while we're modifying this line, let's change that var to let.

::: browser/metro/base/content/flyouts/flyoutUI.js
@@ +13,5 @@
> +      return;
> +    }
> +
> +    Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +    Cu.import("resource://gre/modules/Services.jsm");

Any particular reason for doing the import here & not hte top of the file?

@@ +18,5 @@
> +
> +    this._isInitialized = true;
> +    let scriptContexts = {};
> +
> +    [

nit: I kind of want to store this object and do the .forEach in a separate statement. The #ifdef makes it hard to read as is.

::: browser/metro/base/content/flyouts/syncFlyout.js
@@ +13,5 @@
> +      return;
> +    }
> +
> +    this._isInitialized = true;
> +    this._bundle = Services.strings.createBundle("chrome://browser/locale/sync.properties");

This is one of those things that might need to be wrapped in #ifdef. If I read the code right, there shouldn't be a code path to this file if sync is disabled at build time, so I might be overly paranoid.

@@ +303,5 @@
> +    let self = this;
> +    this._pairJpake = new Weave.JPAKEClient({
> +        onPaired: function() {
> +          self._pairJpake.sendAndComplete(
> +            {

nit: move the { up with the ({. We have strict cuddling policy around here. :)

::: browser/metro/base/jar.mn
@@ +32,5 @@
> +* content/flyouts/flyoutUI.js                  (content/flyouts/flyoutUI.js)
> +* content/flyouts/aboutFlyout.js               (content/flyouts/aboutFlyout.js)
> +  content/flyouts/prefsFlyout.js               (content/flyouts/prefsFlyout.js)
> +#ifdef MOZ_SERVICES_SYNC
> +  content/flyouts/syncFlyout.js                (content/flyouts/syncFlyout.js)

You might want to stick this with content/RemoteTabs.js Keeping the files that are subject to the ifdef rule together is a good idea for future readers

::: browser/metro/locales/en-US/chrome/sync.dtd
@@ +2,5 @@
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
> +<!-- Flyout -->
> +<!ENTITY sync.flyout.title                     "Sync">

This should be &syncBrand.fullName.label

@@ +7,4 @@
>  
> +<!-- Flyout when not connected -->
> +<!ENTITY sync.flyout.presetup.description1     "Access your bookmarks, passwords, and open tabs across your devices">
> +<!ENTITY sync.flyout.presetup.setup.label      "Set Up Sync">

Where ever you have 'Sync' as a title/brand, we should be using &syncBrand.shortName.label.

@@ +15,5 @@
> +<!ENTITY sync.flyout.setup.description2        "If you don&apos;t have a Sync account, you can create one on your Firefox desktop browser.">
> +<!ENTITY sync.flyout.setup.manual.label        "I&apos;m not near my computer...">
> +<!ENTITY sync.flyout.setup.description3        "Note:">
> +<!ENTITY sync.flyout.setup.description4        "You can select &quot;Pair a Device&quot; through">
> +<!ENTITY sync.flyout.setup.description5        "[Desktop] Preferences -&gt; Sync">

We should either use the localized short brand names for desktop & android or leave a localization note pointing to the branding files telling localizers to manually make them the same. We should probably really do the former if possible.
Attachment #767873 - Flags: review?(ally) → review+
Attachment #767875 - Flags: review?(ally) → review+
Attached patch Part 2 Patch v2 (obsolete) — Splinter Review
Carrying forward r+.  For some reason, links are now orange instead of light blue.  I'll figure that out, post the updated patch, and submit to try.


(In reply to :Ally Naaktgeboren from comment #17)
[...]
> ::: browser/metro/base/content/browser.xul
> @@ +144,5 @@
> >      <key id="key_quit" key="q" modifiers="accel" command="cmd_quit"/>
> >      <key id="key_addBoomkark" key="d" modifiers="accel" command="cmd_addBookmark"/>
> >      <key id="key_console" key="j" modifiers="accel,shift" oncommand="PanelUI.show('console-container')"/>
> > +    <key id="key_options"
> > +         key="o"
> 
> we don't really respect an 80 character/line limit in the metro code base,
> and these now stick out oddly.
> I'd stick with the original all on one line layout to match the other <key>s
> If you have a compelling reason, I'm open to being persuaded otherwise.

Done.
 
> @@ +433,5 @@
> >              onclick="if (event.button == 0) { Browser.onAboutPolicyClick(); }"
> >              class="text-link" value="&aboutHeader.policy.label;"/>
> >      </flyoutpanel>
> >  
> > +#ifdef MOZ_SERVICES_SYNC
> 
> It looks like you did a good job of preserving the MOZ_SERVICES_SYNC
> wrapping everywhere. Thanks! However, before you push to m-c, you may want
> to do a try push to ensure we don't break Seamonkey.
> 
> If you decide not to do that, be sure to keep an eye on the seamonkey builds
> when it hits m-c just in case.

I'll send to try today.

> @@ +457,5 @@
> > +        <separator />
> > +        <vbox flex="1" pack="center" align="start">
> > +          <description id="sync-setup-code1"
> > +                       class="syncJPAKECode">
> > +            ....
> 
> nit So we have the '....' here & later in the js. I would rather we store a
> 'default jpake string' somewhere and use that than have 6 '....'.

I fixed this by making this element a <textbox> instead of a <description> and setting the placeholder to '....'

Do you think that's a good alternative?

> @@ +462,5 @@
> > +          </description>
> > +          <description id="sync-setup-code2"
> > +                       class="syncJPAKECode">
> > +            ....
> > +          </description>
> 
> might be a good candidate for the below nit/grumble
> 
> @@ +486,5 @@
> > +        </description>
> > +        <separator />
> > +        <description>&sync.flyout.setup.description3;</description>
> > +        <description>&sync.flyout.setup.description4;</description>
> > +        <description class="syncInstructionText">
> 
> General nit/grumble: This file is very inconsistent about when all
> attributes inside of a given tag go on one line versus multiple lines. I
> don't expect you to fix the world, but you might consider condensing some of
> the shorter ones onto one line such as  the <description>
> &sync.flyout.setup.description5.

How about I file a follow-up bug to clean up the spacing weirdness in this file?  As an added bonus, I could probably make the whole thing respect 80-character line limits :)

> ::: browser/metro/base/content/flyouts/aboutFlyout.js
> @@ +6,3 @@
> >  
> > +Cu.import("resource://gre/modules/Services.jsm");
> > +var gAppUpdater;
> 
> nit: while we're modifying this line, let's change that var to let.

Done.

> ::: browser/metro/base/content/flyouts/flyoutUI.js
> @@ +13,5 @@
> > +      return;
> > +    }
> > +
> > +    Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> > +    Cu.import("resource://gre/modules/Services.jsm");
> 
> Any particular reason for doing the import here & not hte top of the file?

My reasoning was something like "if the flyoutUI never gets opened then we save the hit of loading those 'jsm's!"  That might not be particularly rational reasoning though :)

> @@ +18,5 @@
> > +
> > +    this._isInitialized = true;
> > +    let scriptContexts = {};
> > +
> > +    [
> 
> nit: I kind of want to store this object and do the .forEach in a separate
> statement. The #ifdef makes it hard to read as is.

Done.

> ::: browser/metro/base/content/flyouts/syncFlyout.js
> @@ +13,5 @@
> > +      return;
> > +    }
> > +
> > +    this._isInitialized = true;
> > +    this._bundle = Services.strings.createBundle("chrome://browser/locale/sync.properties");
> 
> This is one of those things that might need to be wrapped in #ifdef. If I
> read the code right, there shouldn't be a code path to this file if sync is
> disabled at build time, so I might be overly paranoid.

This should indeed never get reached if sync is disabled at build time.  I imagine that the try run will discover any potential issues?

> @@ +303,5 @@
> > +    let self = this;
> > +    this._pairJpake = new Weave.JPAKEClient({
> > +        onPaired: function() {
> > +          self._pairJpake.sendAndComplete(
> > +            {
> 
> nit: move the { up with the ({. We have strict cuddling policy around here.
> :)

Snuggled!

> ::: browser/metro/base/jar.mn
> @@ +32,5 @@
> > +* content/flyouts/flyoutUI.js                  (content/flyouts/flyoutUI.js)
> > +* content/flyouts/aboutFlyout.js               (content/flyouts/aboutFlyout.js)
> > +  content/flyouts/prefsFlyout.js               (content/flyouts/prefsFlyout.js)
> > +#ifdef MOZ_SERVICES_SYNC
> > +  content/flyouts/syncFlyout.js                (content/flyouts/syncFlyout.js)
> 
> You might want to stick this with content/RemoteTabs.js Keeping the files
> that are subject to the ifdef rule together is a good idea for future readers

Done.

> ::: browser/metro/locales/en-US/chrome/sync.dtd
> @@ +2,5 @@
> >     - License, v. 2.0. If a copy of the MPL was not distributed with this
> >     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> >  
> > +<!-- Flyout -->
> > +<!ENTITY sync.flyout.title                     "Sync">
> 
> This should be &syncBrand.fullName.label

Done.  Though I made it "&syncBrand.shortName.label;"

> @@ +7,4 @@
> >  
> > +<!-- Flyout when not connected -->
> > +<!ENTITY sync.flyout.presetup.description1     "Access your bookmarks, passwords, and open tabs across your devices">
> > +<!ENTITY sync.flyout.presetup.setup.label      "Set Up Sync">
> 
> Where ever you have 'Sync' as a title/brand, we should be using
> &syncBrand.shortName.label.

Done.

> @@ +15,5 @@
> > +<!ENTITY sync.flyout.setup.description2        "If you don&apos;t have a Sync account, you can create one on your Firefox desktop browser.">
> > +<!ENTITY sync.flyout.setup.manual.label        "I&apos;m not near my computer...">
> > +<!ENTITY sync.flyout.setup.description3        "Note:">
> > +<!ENTITY sync.flyout.setup.description4        "You can select &quot;Pair a Device&quot; through">
> > +<!ENTITY sync.flyout.setup.description5        "[Desktop] Preferences -&gt; Sync">
> 
> We should either use the localized short brand names for desktop & android
> or leave a localization note pointing to the branding files telling
> localizers to manually make them the same. We should probably really do the
> former if possible.

A cursory search shows that there aren't really localized strings out there for "Desktop" and "Android".  Are you suggesting that we use "&brandShortName;" and "something similar for Android?"
Attachment #767873 - Attachment is obsolete: true
Attachment #771073 - Flags: review+
Attached patch Part 2 Patch v3 (obsolete) — Splinter Review
This updated patch deals with resetting the "disconnect warning" when the flyout is dismissed and shown again (bug 889707).

Additionally, this patch fixes a problem with flyouts where they couldn't be opened rapidly (STR: Try spamming ctrl+A/O/S in a build without this patch applied)

I'm going to look over the diff to make sure everything looks right, then submit to try and decide whether I think this needs re-review.
Attachment #772171 - Flags: review+
Attached patch Part 2 patch v4 (obsolete) — Splinter Review
Fixing up a test that was broken in the last patch.

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=7a7166c0d742
Attachment #771073 - Attachment is obsolete: true
Attachment #772171 - Attachment is obsolete: true
Attachment #772186 - Flags: review+
Comment on attachment 772186 [details] [diff] [review]
Part 2 patch v4

Ally; the try run seems to be going pretty well.  Do you mind taking another look at this patch before I land? The main difference is the updated flyout code. Also any nits from the previous review should be addressed now.
Attachment #772186 - Flags: review+ → review?(ally)
Comment on attachment 772186 [details] [diff] [review]
Part 2 patch v4

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

Most of it looks pretty good (both running and in the diff), but I have found a few issues. 

When metro is paired via sync, and the sync log is showing a successful sync, the last synced field in the ui doesn't update.

I also note that the Remote Tabs entry isn't appearing on the start screen after setup, although the history grid populates with synced content. Is this a bug?

I also might like to add a proposal to use a pref to hide/expose direct access to the sync logs to make it easier for dogfooders/us to keep an eye on what sync is doing. That to be clear, is totally optional though.

::: browser/metro/base/content/browser-ui.js
@@ +1321,5 @@
>        case "cmd_history":
>          PanelUI.show("history-container");
>          break;
>        case "cmd_remoteTabs":
> +        if (Weave.Status.checkSetup() != Weave.CLIENT_NOT_CONFIGURED) {

since this invokes the weave object, you might want to ifdef this.

@@ +1767,5 @@
>  
>    _isEventInsidePopup: function _isEventInsidePopup(aEvent) {
>      if (!this._popup)
>        return false;
> +

empty new line?
Attachment #772186 - Flags: review?(ally)
Attached patch Part 2 patch v5 (obsolete) — Splinter Review
(In reply to :Ally Naaktgeboren from comment #22)
[...]
> When metro is paired via sync, and the sync log is showing a successful
> sync, the last synced field in the ui doesn't update.

I added an observer to listen for sync events; this also fixes the fact that we weren't showing a throbber when a sync was in progress.

> I also note that the Remote Tabs entry isn't appearing on the start screen
> after setup, although the history grid populates with synced content. Is
> this a bug?

I'm not sure; I'll try to debug this.

> I also might like to add a proposal to use a pref to hide/expose direct
> access to the sync logs to make it easier for dogfooders/us to keep an eye
> on what sync is doing. That to be clear, is totally optional though.

This sounds like a good idea!  I'll file this as a follow-up.

> ::: browser/metro/base/content/browser-ui.js
> @@ +1321,5 @@
> >        case "cmd_history":
> >          PanelUI.show("history-container");
> >          break;
> >        case "cmd_remoteTabs":
> > +        if (Weave.Status.checkSetup() != Weave.CLIENT_NOT_CONFIGURED) {
> 
> since this invokes the weave object, you might want to ifdef this.

Done.

> @@ +1767,5 @@
> >  
> >    _isEventInsidePopup: function _isEventInsidePopup(aEvent) {
> >      if (!this._popup)
> >        return false;
> > +
> 
> empty new line?

Fixed.
Attachment #772186 - Attachment is obsolete: true
Attachment #772933 - Flags: review?(ally)
[...]
> > I also note that the Remote Tabs entry isn't appearing on the start screen
> > after setup, although the history grid populates with synced content. Is
> > this a bug?
> 
> I'm not sure; I'll try to debug this.

I investigated this, and the Remote Tabs entry only appears if there are actually tabs to populate it with.  From looking at the code, I'm guessing that this was an intentional behavior change.  If we want to change it back, we should probably do that as part of a separate bug.
Comment on attachment 767875 [details] [diff] [review]
Part 1 Patch v1

the part 1 patch is obsolete since it got folded into the part 2 patch
Attachment #767875 - Attachment is obsolete: true
Comment on attachment 767875 [details] [diff] [review]
Part 1 Patch v1

Oops I totally lied about that.  Sorry for the spam.
Attachment #767875 - Attachment is obsolete: false
Comment on attachment 772933 [details] [diff] [review]
Part 2 patch v5

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

::: browser/metro/base/content/flyouts/syncFlyout.js
@@ +294,5 @@
> +  },
> +
> +  onDisconnectButton: function() {
> +    Weave.Service.startOver();
> +    this.showInitialScreen();

You might want to check with rnewman on this. I believe the process of starting over involves network activity, and can take a while. This is why we had the ui disabled here until 'weave:service:start-over:finish' fired. If the signup is enabled while this process is taking place you may end up with lots of weird errors getting filed by qa.
Attached patch Part 2 patch v6Splinter Review
Wow that bitrotted something fierce. Hopefully that doesn't happen again.
Attachment #772933 - Attachment is obsolete: true
Attachment #772933 - Flags: review?(ally)
Attachment #772975 - Flags: review?(ally)
(In reply to Jim Mathies [:jimm] from comment #27)
> Comment on attachment 772933 [details] [diff] [review]
> Part 2 patch v5
> 
> Review of attachment 772933 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/metro/base/content/flyouts/syncFlyout.js
> @@ +294,5 @@
> > +  },
> > +
> > +  onDisconnectButton: function() {
> > +    Weave.Service.startOver();
> > +    this.showInitialScreen();
> 
> You might want to check with rnewman on this. I believe the process of
> starting over involves network activity, and can take a while. This is why
> we had the ui disabled here until 'weave:service:start-over:finish' fired.
> If the signup is enabled while this process is taking place you may end up
> with lots of weird errors getting filed by qa.

From what I can tell (both in an MXR search and from testing manually), the desktop browser doesn't wait for the 'weave:service:start-ver:finish' message to fire before allowing a new sync login to happen.  That does seem counter-intuitive though; I would expect that something could go wrong if the user logs in to sync before the previous information has been fully removed.

rnewman: Can you clear up whether we need to do something special between calling Weave.Service.startOver and re-enabling our sync login UI?
Flags: needinfo?(rnewman)
startOver is synchronous (as is the rest of Sync), so once it successfully returns there's nothing further to do.

You *could* wait for weave:service:start-over:finish, but all of our chrome JS runs on one thread anyway, so it's kinda pointless, right?

(This, also, is a damned if you do, damned if you don't situation: startOver will *not* notify anything if an exception is thrown in any non-network portion (e.g., if touching prefs throws). Sucky code. And I apologize on behalf of the long-departed authors of this code for blocking your chrome thread!)
Flags: needinfo?(rnewman)
(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #24)
> [...]
> I investigated this, and the Remote Tabs entry only appears if there are
> actually tabs to populate it with.  From looking at the code, I'm guessing
> that this was an intentional behavior change.  If we want to change it back,
> we should probably do that as part of a separate bug.

Ok, we can stick with that.
Comment on attachment 772975 [details] [diff] [review]
Part 2 patch v6

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

I'm a little concerned about the exceptions that showed up in the sync log on my first pass of testing this patch. However, two subsequent sync pairings went slowly but smoothly, and the exception was during a fetch from the phx server that it's not related to your patch as such.

With respect to the [Android]/[Desktop] directions, I feel like this is looks like we forgot something, but the shortBrands could be equally confusing in the case of [Desktop] (where it would say 'firefox'. I can't come up with a more elegant answer, so let's go with whats in the mocks(and currently in this patch) and if we get feedback from our dogfooders or l10n community, we can solve it then with more information.

Make sure to follow the follow up bugs on the behind-a-pref button to the sync logs & the one on reformatting browser.xul to be consistent that we have discussed over the course of this bug.

Now that I've run this on a slow machine, the lack of feedback about the progress of the first sync is painful. (I set up sync. It told me about my tabs. where are tabs on my start screen?) We'll see how the dogfooders respond and act accordingly.
Attachment #772975 - Flags: review?(ally) → review+
(In reply to :Ally Naaktgeboren from comment #32)
> Comment on attachment 772975 [details] [diff] [review]
> Part 2 patch v6
> 
> Review of attachment 772975 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm a little concerned about the exceptions that showed up in the sync log
> on my first pass of testing this patch. However, two subsequent sync
> pairings went slowly but smoothly, and the exception was during a fetch from
> the phx server that it's not related to your patch as such.

Let's keep an eye on it!

> With respect to the [Android]/[Desktop] directions, I feel like this is
> looks like we forgot something, but the shortBrands could be equally
> confusing in the case of [Desktop] (where it would say 'firefox'. I can't
> come up with a more elegant answer, so let's go with whats in the mocks(and
> currently in this patch) and if we get feedback from our dogfooders or l10n
> community, we can solve it then with more information.

Yeah this seems to make the most sense.

> Make sure to follow the follow up bugs on the behind-a-pref button to the
> sync logs & the one on reformatting browser.xul to be consistent that we
> have discussed over the course of this bug.

Will do!

> Now that I've run this on a slow machine, the lack of feedback about the
> progress of the first sync is painful. (I set up sync. It told me about my
> tabs. where are tabs on my start screen?) We'll see how the dogfooders
> respond and act accordingly.

I'll add this to the list of follow-ups to file :)
https://hg.mozilla.org/integration/mozilla-inbound/rev/96a8f3189caf

My reviewer requested a limerick
which I'll write if it helps make the patch stick
  I will push to inbound and am trying
  not to end this with poor Philor crying
I don't want him to think I'm a... careless commiter
There once was a careless committer
Whose verse could be posted to Twitter
His patches hit Inbound
Let's hope they don't rebound
Lest philor reach for his hitter.
https://hg.mozilla.org/mozilla-central/rev/96a8f3189caf
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Do we have a bug filed on adding high dpi versions of chrome://browser/skin/images/plus-34.png? If not, we should file one and have it block bug 833192.
(In reply to Jim Mathies [:jimm] from comment #37)
> Do we have a bug filed on adding high dpi versions of
> chrome://browser/skin/images/plus-34.png? If not, we should file one and
> have it block bug 833192.

I don't think we do.  I'll file as part of my follow-up-bug-filing from this bug.
(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #18)
[...]
> (In reply to :Ally Naaktgeboren from comment #17)
[...]
> > General nit/grumble: This file is very inconsistent about when all
> > attributes inside of a given tag go on one line versus multiple lines. I
> > don't expect you to fix the world, but you might consider condensing some of
> > the shorter ones onto one line such as  the <description>
> > &sync.flyout.setup.description5.
> 
> How about I file a follow-up bug to clean up the spacing weirdness in this
> file?  As an added bonus, I could probably make the whole thing respect
> 80-character line limits :)

Filed bug 892725.


(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #23)
[...]
> (In reply to :Ally Naaktgeboren from comment #22)
> [...]
> > I also might like to add a proposal to use a pref to hide/expose direct
> > access to the sync logs to make it easier for dogfooders/us to keep an eye
> > on what sync is doing. That to be clear, is totally optional though.
> 
> This sounds like a good idea!  I'll file this as a follow-up.

Filed bug 892733.


(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #38)
> (In reply to Jim Mathies [:jimm] from comment #37)
> > Do we have a bug filed on adding high dpi versions of
> > chrome://browser/skin/images/plus-34.png? If not, we should file one and
> > have it block bug 833192.
> 
> I don't think we do.  I'll file as part of my follow-up-bug-filing from this
> bug.

Filed bug 892742.


(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #33)
> (In reply to :Ally Naaktgeboren from comment #32)
[...]
> > Now that I've run this on a slow machine, the lack of feedback about the
> > progress of the first sync is painful. (I set up sync. It told me about my
> > tabs. where are tabs on my start screen?) We'll see how the dogfooders
> > respond and act accordingly.
> 
> I'll add this to the list of follow-ups to file :)

Filed bug 892739.
Asking here about strings before opening follow-ups (I got lost in the comments)

Is there a specific reason for mixing Sync (hard-coded) and &syncBrand.shortName.label;?

<!ENTITY sync.flyout.manualsetup.description1 "Please enter your &syncBrand.shortName.label; account information and the Recover Key generated by your computer">
Typo: it's RecoverY Key, not Recover Key.

<!ENTITY sync.flyout.setup.manual.label "I&apos;m not near my computer...">
Should use "…" (single unicode character) instead of "..."

<!ENTITY sync.flyout.pairNewDevice.note3 "[Desktop] Tools -&gt; Set Up Sync -&gt; I have an Account">
Why "Tools"? This is valid only for a small subset of Windows.

<!ENTITY sync.flyout.setup.description5 "[Desktop] Preferences -&gt; Sync">
Again. Preferences is good for Linux and Mac, not Windows. Maybe "Settings" (generic, os-agnostic)?
Filed bug 893961 for string change follow-up (Francesco please check out bug 893961)
Mozilla/5.0 (Windows NT 6.2; WOW64; rv:25.0) Gecko/20130716 Firefox/25.0

Verified that the Synk flyouts appear as designed in the mockups, except the "Fail to set up sync" one, which I could not get. How can I trigger that message?
Flags: needinfo?(ywang)
I'm not sure how to make sync easy-setup fail (without changing the code), but Ally might have an idea.

My first thought would be to add the line "this._pairJpake.abort();" directly after the call to "this._pairJpake.pairWithPIN"
Flags: needinfo?(ywang) → needinfo?(ally)
Depends on: 893781
Sorry for the delay! This didn't show up on my dashboard for some reason.

I would think 
http://mxr.mozilla.org/mozilla-central/source/services/sync/tests/unit/test_jpakeclient.js

#235, #353, #415 might be good places to look.

However, rnewman remains the source of truth. It's been almost a year since I've worked on sync.
Flags: needinfo?(ally) → needinfo?(rnewman)
That's exactly how I'd suggest testing failure for this, Ally.
Flags: needinfo?(rnewman)
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.