Here's a link to some of the desktop design work: http://people.mozilla.com/~faaborg/files/firefox4Mockups/jpakeFlow-i1/jpakeFlow-i1.htm more details in this comment: https://bugzilla.mozilla.org/show_bug.cgi?id=602682#c25

Note that we're string frozen for fennec, too.

Beta 3 string freeze for Fennec is Nov 12th

All the UI discussion is going on over in bug 602682

Two amendments to the image in http://www.flickr.com/photos/madhava_work/5164874423/ 1. replace "auto-sync data [yes|no]" with "Synchronize [ On | Off ]" (default is On, obviously) 2. Instead of a confirmation dialog on "Remove", let's - disconnect - change the UI so it looks like you're not connected (i.e. pre-jpake) - put up a notification bar: ----------------------------------------------------- Your Firefox Sync account has been removed. (x) ( undo ) ----------------------------------------------------- and dismiss this when the user leaves the manager, or hits the (x)

This is the connection flow: http://www.flickr.com/photos/madhava_work/5164827140/sizes/l/

This patch switches fennec over to host all it's own sync related strings for UI. We also add the new setup screens. The JPAKE screen is useless for now, but the fallback screen works with the existing Sync impl. Other parts of the UI are not operational too: Synchronize toggle and the Remove button for example. Remove button just does a simple disconnect for now. The patch supports all the different states found in the design mockups. We should be able to show all the screens in their correct context.

Comment on attachment 490683 [details] [diff] [review] patch (mostly for stings) >"From a Firefox Sync connected computer, go to Sync options and select 'Add a device'" I think we need some sort of "Help" link or button after these instructions. It should open a web page with detailed instructions for the following cases: * Getting Firefox Sync and creating an account. * Finding the "Add a device" command in desktop Firefox. * What to do if desktop Firefox has an old version without "Add a device." These situations need more than just the "I'm not near my computer" fallback link. I don't want to land this without at least a plan for addressing this, because this is going to be the common case until all desktop Firefox users are updated to Fx4 beta 8 or later, or have a newer Firefox Sync add-on installed. So we need to solve this for b3, and the change may involve string changes or additions.

I kinda detest using "links" in mobile UIs, but we could add something or a button to get help. Help could be a live link or an in-product page

Are all casings in those strings as they should be? I don't know the rules why it would be "Sync Now" vs "Sync now" or "Account Name" vs "Account name". Just want to make sure we have those nasty details in check, too.

Comment on attachment 490683 [details] [diff] [review] patch (mostly for stings) >+ <button label="&sync.remove;" oncommand="WeaveGlue.disconnect();" /> I agree with tchung's comment on IRC that "Remove" is a weird name for this button. "Log out" or "Disconnect" seems better to me. >+ <textbox readonly="true">code</textbox> >+ <textbox readonly="true">goes</textbox> >+ <textbox readonly="true">here</textbox> The vertical alignment of the text in these boxes is off, in my Linux build. I suggest styling these so they look more disabled. Right now it looks like I'm supposed to type in them. >+ <description style="font-size: 18px; text-decoration: underline" onclick="document.getElementById('syncsetup-jpake').hidden = true; document.getElementById('syncsetup-manual').hidden = false;">&sync.fallback;</description> After you click on this, there's no way to get back to the JPAKE UI - even if you cancel setup and then click on "Connect" again. There should be a "Back" button, or it should be reset when you press "Cancel." Pressing escape should cancel setup (or go back to the previous setup screen). >+ <button id="syncsetup-customserver-checkbox" type="checkbox" class="button-checkbox" pack="start"> >+ <image class="button-image-icon"/> >+ <description class="prompt-checkbox-label" flex="1">&sync.customServer;</description> >+ </button> >+ <textbox id="syncsetup-customserver" placeholder="&sync.serverURL;"/> We should check the "custom server" checkbox automatically if you start typing into the textbox. >+ changeSync: function changeSync() { >+ // XXX enable/disable sync without actually disconnecting >+ }, Needs a fix, or a blocking followup bug. >--- a/locales/en-US/chrome/preferences.dtd >-<!ENTITY sync.account2 "Account Name"> >+++ b/locales/en-US/chrome/sync.dtd >+<!ENTITY sync.account "Account Name"> Why did this change from sync.account2 back to sync.account? >+lastSync.label=Last Update: %S I think this should be "Last update: %S" to match the capitalization of the other preference labels. >+<!ENTITY sync.account "Account Name"> >+<!ENTITY sync.syncKey "Sync Key"> ...and should these textbox hints be sentence-case too? >+remove.label=Your Firefox Sync account has been removed >+remove.undo=Undo These are unused. Will they be used in a followup?

(In reply to comment #11) > I agree with tchung's comment on IRC that "Remove" is a weird name for this > button. "Log out" or "Disconnect" seems better to me. I was looking for something stronger than disconnect, given that disconnect, in the current version, merely logs you out (leaving your credentials in place) -- in practice, it's more like a "suspend syncing". What this actually does is dissociate your account from the device. Thinking about it more now, and independently of what "disconnect" means in the current UI, I could see using it, given the symmetry with what "Connect" does. Also, we have the "undo" design now, which make misinterpretation here less severe. > The vertical alignment of the text in these boxes is off, in my Linux build. > > I suggest styling these so they look more disabled. Right now it looks like I'm > supposed to type in them. Yes - styling is still to come, presumably. > After you click on this, there's no way to get back to the JPAKE UI - even if > you cancel setup and then click on "Connect" again. There should be a "Back" > button, or it should be reset when you press "Cancel." I thought about having a button or link to go back (to toggle between the two, essentially), but ended up dropping it in the name of removing anything unnecessary -- I thought that a user could simply cancel, and, on "pressing" connect again they'd see the jpake ui again.

(In reply to comment #12) > (In reply to comment #11) > > > I agree with tchung's comment on IRC that "Remove" is a weird name for this > > button. "Log out" or "Disconnect" seems better to me. > > I was looking for something stronger than disconnect, given that disconnect, in > the current version, merely logs you out (leaving your credentials in place) Note that we want to get rid of that. Once you've paired your device with your Sync account, it will always sync. Also, I agree with your symmetry argument regarding "Connect".

(In reply to comment #13) > Note that we want to get rid of that. Once you've paired your device with your > Sync account, it will always sync. Also, I agree with your symmetry argument > regarding "Connect". OK - sold on "Disconnect" next to the Account name. Regarding the "it will always sync" -- this is the reason for the "Syncronize [yes|no]", given that many of us in mobile-land have run into times when we don't want sync running.

(In reply to comment #11) > We should check the "custom server" checkbox automatically if you start typing > into the textbox. Although leaving the field disabled until the checkbox is checked would reinforce the idea that you don't need to specify a server unless you actually want to use a custom one.

(In reply to comment #12) > I was looking for something stronger than disconnect, given that disconnect, in > the current version, merely logs you out (leaving your credentials in place) With the patch here, "Remove" still leaves your credentials in place if you go through the fallback UI. Should this be fixed?

I think it should. If you're actually going to the trouble of disconnecting (rather than just turning off synchronization), I don't think we should leave your credentials "hidden" in there.

On the desktop we went with the longer string "stop using this account." The main use case is that you are about to give the computer up and don't want it associated anymore (so you don't start to capture someone else's passwords). It's assumed that the user will also be clearing out all of the local data after they disassociate.

(In reply to comment #11) > Comment on attachment 490683 [details] [diff] [review] > patch (mostly for stings) I will make an updated patch with any changes to the strings, but all of the behavior changes will be a second patch on this bug. This patch is primarily to get the strings landed for l10n. The iterative tweaking of behavior will happen afterward. I just needed enough functionality to 1) see all the screens with the new strings 2) keep sync working

Oh, and yes, I know that means this feature will be partially working / broken while the second patch is being created and reviewed - we'll live. The second patch is still slated for b3.

(In reply to comment #12) > I thought about having a button or link to go back (to toggle between the two, > essentially), but ended up dropping it in the name of removing anything > unnecessary -- I thought that a user could simply cancel, and, on "pressing" > connect again they'd see the jpake ui again. Yes, this is the way it will work

(In reply to comment #17) > I think it should. If you're actually going to the trouble of disconnecting > (rather than just turning off synchronization), I don't think we should leave > your credentials "hidden" in there. Yep, also planned

(In reply to comment #10) > Are all casings in those strings as they should be? I don't know the rules why > it would be "Sync Now" vs "Sync now" or "Account Name" vs "Account name". > > Just want to make sure we have those nasty details in check, too. Good point. I'll double check those. As pointed out by Matt, some are wrong.

Comment on attachment 490683 [details] [diff] [review] patch (mostly for stings) r+ with the following string changes, just to get the strings and basic UI in: sync.account -> sync.account2 Remove -> Disconnect Last Update -> Last update Account Name -> Account name Sync Key -> Sync key All of the behavior changes we can do in a followup bug.

(In reply to comment #24) > Comment on attachment 490683 [details] [diff] [review] > patch (mostly for stings) > > r+ with the following string changes, just to get the strings and basic UI in: > > sync.account -> sync.account2 I reverted the entity bump since the string has been moved to a brand new file > Remove -> Disconnect > Last Update -> Last update Done > Account Name -> Account name > Sync Key -> Sync key We don't have good guidelines for placeholder case. Desktop Firefox uses sentence case for one placeholder but title case for 2 others. Fennec uses title case in the urlbar placeholder. Since the placeholders are taking the place of labels, and labels have title case, I stuck with title case. > All of the behavior changes we can do in a followup bug. I plan to do followup patch on this bug

Just the strings and basic UI. Part 2 will fix more behavior.

pushed part 1: http://hg.mozilla.org/mobile-browser/rev/fabd294b1266

The sync ui is not hooked into the back button; it should go back to the preferences.

This layout of prefs controls (after discussion with stuart) gets rid of most of the ambiguity about what "synchronize [on|off]" does: http://www.flickr.com/photos/madhava_work/5183207328/in/photostream/

(In reply to comment #29) > This layout of prefs controls (after discussion with stuart) gets rid of most > of the ambiguity about what "synchronize [on|off]" does: > > http://www.flickr.com/photos/madhava_work/5183207328/in/photostream/ Really? Could you write down what that meaning is? It looks like "Enable Sync" controls whether or not I can even connect to Sync.

Here's a mockup with the disabled states. There are two options shown - the first of the two (disabled, not invisible) is my preferred. http://www.flickr.com/photos/madhava_work/5184268555/

(In reply to comment #31) > Here's a mockup with the disabled states. There are two options shown - the > first of the two (disabled, not invisible) is my preferred. > > http://www.flickr.com/photos/madhava_work/5184268555/ In these mockups, there is no way to manually sync ("Sync Now") while auto-sync is disabled...

(In reply to comment #32) > (In reply to comment #31) > > Here's a mockup with the disabled states. There are two options shown - the > > first of the two (disabled, not invisible) is my preferred. > > > > http://www.flickr.com/photos/madhava_work/5184268555/ > > In these mockups, there is no way to manually sync ("Sync Now") while auto-sync > is disabled... I think that is on purpose.

Icon for the connect screen is here: http://mozilla.seanmartell.com/fennec/sync-64x64.png

(In reply to comment #32) > (In reply to comment #31) > > Here's a mockup with the disabled states. There are two options shown - the > > first of the two (disabled, not invisible) is my preferred. > > > > http://www.flickr.com/photos/madhava_work/5184268555/ > > In these mockups, there is no way to manually sync ("Sync Now") while auto-sync > is disabled... yeah - a working manual sync button makes sense if you've just turned off _auto_ sync, but less so when the switch enables/disables sync overall.

Madhava: were you going to add the two "Show me how" support links we have when adding computers on desktop? -How to find "Add a device" (filed bug 613415) -How to find your Sync Key (filed bug 613417) On a related note, do SUMO articles format correctly when being displayed on a mobile device running fennec?

For the second device, if the code expires, or the user needs a new code because they entered the first one incorrectly, I think we should just refresh the code on the device (when they look back there is a new one).

(In reply to comment #27) > pushed part 1: > http://hg.mozilla.org/mobile-browser/rev/fabd294b1266 Hi guys, Apart from the obvious "code" "goes" "here" strings which I assume are just temporary placeholders, there's two hard-coded "Cancel"s and one "Connect" in http://hg.mozilla.org/mobile-browser/diff/fabd294b1266/chrome/content/browser.xul#l1.35. Would you mind changing them to entities?

lots of changes coming. will need tests

This patch adds some fucntionality and changes the theme a bit: * Use the new black background fullscreen dialog style * Add support for hardware back button * Adds "subgroup" concept to preferences by removing separators and indenting * Makes "Details" button expand and collapse the subgroup of options * Add a CSS rule to support checked="true" for buttons to look "pressed" * Adds support for the new "Enable Sync" option which controls all Sync functionality ** We disconnect / connect when "Enable Sync" toggles ** UI uses the 2nd approach for disabled state (http://www.flickr.com/photos/madhava_work/5184268555/) since setting widgets don't yet have a disabled state themselves. * Setup data is stored in a JS object, not the textbox UI * Undo disconnect uses a copy of the setup data to do the "undo"

the patch also fixes the hardcoded strings. unfortunately, I had to re-use an add-on manager string for "Cancel". I'll fix this for beta 4 since we are string frozen for b3.

As we're using l10n-merge anyway, I'd suggest to use the real string right away.

Comment on attachment 492402 [details] [diff] [review] part 2 (basic functionality) >+++ b/chrome/content/browser.xul >- <toolbarbutton id="tool-panel-close" command="cmd_panel"/> >+ <toolbarbutton class="tool-panel-close" command="cmd_panel"/> This requires some trivial fixes to chrome/tests/browser_preferences_* >+ <button id="syncsetup-usecustomserver" type="checkbox" class="button-checkbox" pack="start"> > <image class="button-image-icon"/> >+ <description class="syncsetup-label" flex="1">&sync.customServer;</description> > </button> > <textbox id="syncsetup-customserver" placeholder="&sync.serverURL;"/> We should either disable this textbox when the button is unchecked, or check the button automatically when you type in the textbox. Need a followup bug to either use the custom server setting, or remove it from the UI for b3. >+++ b/chrome/content/sync.js >+ openSetup: function openSetup() { > // Show the connect UI > document.getElementById("syncsetup-container").hidden = false; > document.getElementById("syncsetup-jpake").hidden = false; > document.getElementById("syncsetup-manual").hidden = true; >+ >+ BrowserUI.pushDialog(this); > }, A dialog needs a close() method, (for handleEscape) but you renamed close -> closeSetup below. I still think there should be a "back" action from the manual setup back to the easy setup, but I'll leave that decision to Madhava. >+ enableSync: function enableSync() { Could we rename this to e.g. "toggleSyncEnabled" (or keep the name but add a boolean argument)? >+ if (enabled) { > // ... >+ } else if (!enabled) { Can just be "else". >+ Weave.Service.login(Weave.Service.username, this.setupData.password, this.normalizePassphrase(this.setupData.syncKey)); I think we can use normalizePassphrase and hyphenatePassphrase from Weave.Utils now, and get rid of the ones in mobile-browser. See Pike's comment about the string, above - we should use the real string ID, as long as we're sure l10n-merge will prevent it from breaking us.

(In reply to comment #45) > Comment on attachment 492402 [details] [diff] [review] > part 2 (basic functionality) > > >+++ b/chrome/content/browser.xul > > >- <toolbarbutton id="tool-panel-close" command="cmd_panel"/> > >+ <toolbarbutton class="tool-panel-close" command="cmd_panel"/> > > This requires some trivial fixes to chrome/tests/browser_preferences_* I added IDs to the XUL and changed the class name a little > >+ <button id="syncsetup-usecustomserver" type="checkbox" class="button-checkbox" pack="start"> > > <image class="button-image-icon"/> > >+ <description class="syncsetup-label" flex="1">&sync.customServer;</description> > > </button> > > <textbox id="syncsetup-customserver" placeholder="&sync.serverURL;"/> > > We should either disable this textbox when the button is unchecked, or check > the button automatically when you type in the textbox. I added disabling the textbox > Need a followup bug to either use the custom server setting, or remove it from > the UI for b3. I put basic handling for a custom server based on Philipp's comments in bug 591661. > >+++ b/chrome/content/sync.js > > >+ openSetup: function openSetup() { > > // Show the connect UI > > document.getElementById("syncsetup-container").hidden = false; > > document.getElementById("syncsetup-jpake").hidden = false; > > document.getElementById("syncsetup-manual").hidden = true; > >+ > >+ BrowserUI.pushDialog(this); > > }, > > A dialog needs a close() method, (for handleEscape) but you renamed close -> > closeSetup below. changed back to close() > >+ enableSync: function enableSync() { > > Could we rename this to e.g. "toggleSyncEnabled" (or keep the name but add a > boolean argument)? Renamed > >+ if (enabled) { > > // ... > >+ } else if (!enabled) { > > Can just be "else". Done > >+ Weave.Service.login(Weave.Service.username, this.setupData.password, this.normalizePassphrase(this.setupData.syncKey)); > > I think we can use normalizePassphrase and hyphenatePassphrase from Weave.Utils > now, and get rid of the ones in mobile-browser. Done > See Pike's comment about the string, above - we should use the real string ID, > as long as we're sure l10n-merge will prevent it from breaking us. Done

pushed part 2: http://hg.mozilla.org/mobile-browser/rev/76f01dd37fa9

(In reply to comment #37) > Madhava: were you going to add the two "Show me how" support links we have when > adding computers on desktop? > > -How to find "Add a device" (filed bug 613415) > -How to find your Sync Key (filed bug 613417) > > On a related note, do SUMO articles format correctly when being displayed on a > mobile device running fennec? I didn't realize we were building this support content - I'll look for a way to get them in. But no, SUMO articles do not currently format well on a mobile device - you see the desktop layout.

(In reply to comment #48) > (In reply to comment #37) > > Madhava: were you going to add the two "Show me how" support links we have when > > adding computers on desktop? > > > > -How to find "Add a device" (filed bug 613415) > > -How to find your Sync Key (filed bug 613417) Actually - for the latter one - is that useful to include on mobile? If they're not near their computer, will the article about finding your sync key help? And if they _are_ near their computer, wouldn't they use jpake instead? That said, there's value to the first one, definitely.

Two variants: http://www.flickr.com/photos/madhava_work/5201976716/ The latter is more consistent with the desktop, and also makes it more clear that it's a thing you can tap. It takes you out of the connection flow, given that it has to open a webpage; BUT if you're lost, that's a pretty good tradeoff.

Tested custom server url: https://stage-auth.services.mozilla.com/. Need to enter the trailing slash at the end on the fennec sync-fallback UI in order to successfully connect to the server. it fails without it.

(In reply to comment #51) > Need to enter the trailing slash at the end on the fennec sync-fallback UI in > order to successfully connect to the server. it fails without it. The Firefox UI adds https:// and trailing slash when needed. Would be good if the Fennec UI could do the same. Here's the relevant portion of the Firefox UI code: http://mxr.mozilla.org/services-central/source/fx-sync/ui/firefox/content/setup.js#663 Also note the isValid function that checks whether a server that was entered is actually a valid Sync server or not.

(In reply to comment #52) > Also note the isValid function that checks whether a server that was entered is > actually a valid Sync server or not. Actually, please ignore that. isValid() is only used for account creation, not for connecting to an existing account. So please don't add that check to Fennec because it only supports existing accounts anyway.

Some nits with the styling of the current implementation: jpake screen: https://bugzilla.mozilla.org/show_bug.cgi?id=606590 manual connect screen: http://www.flickr.com/photos/madhava_work/5202078787/in/photostream/

that jpake screen should actually be this link: http://www.flickr.com/photos/madhava_work/5202680838/in/photostream/

This patch adds implementation of JPAKE connection, stealing heavily from the Firefox patches. It also adds code to validate the server URL and tweaks the CSS style a little, based on Madhava's input. We still need to test the JPAKE feature when the back-end impl lands. I don't know if I like the way the sync logo looks in the dialogs.

Oops, forgot to add the logo png to the patch

Comment on attachment 492914 [details] [diff] [review] part 3 (jpake + tweaks) v2 >diff --git a/chrome/content/browser.xul b/chrome/content/browser.xul > <vbox align="center" flex="1"> >- <description class="syncsetup-code">code</description> >- <description class="syncsetup-code">goes</description> >- <description class="syncsetup-code">here</description> >+ <description id="syncsetup-code1" class="syncsetup-code">code</description> >+ <description id="syncsetup-code2" class="syncsetup-code">goes</description> >+ <description id="syncsetup-code3" class="syncsetup-code">here</description> "code goes here"? Are those strings temporary or do we want to localize them (or remove them)? >diff --git a/chrome/content/sync.js b/chrome/content/sync.js >+ }); >+ this._jpakeclient.receiveNoPIN(); > }, > > openManual: function openManual() { > // Push the current setup data into the UI > if (this.setupData && "account" in this.setupData) { > this._elements.account.value = this.setupData.account; > this._elements.password.value = this.setupData.password; >- this._elements.synckey.value = this.setupData.syncKey; >- if (this.setupData.customServer && this.setupData.customServer.length) { >+ this._elements.synckey.value = this.setupData.synckey; >+ if (this.setupData.serverURL && this.setupData.serverURL.length) { > this._elements.usecustomserver.checked = true; > this._elements.customserver.disabled = false; >- this._elements.customserver.value = this.setupData.customServer; >+ this._elements.customserver.value = this.setupData.serverURL; > } else { > this._elements.usecustomserver.checked = false; > this._elements.customserver.disabled = true; > this._elements.customserver.value = ""; > } Is there any reason for the properties of this._elements to not be camelCase? Same question for this.setupData.synckey? > close: function close() { ... > this._elements.usecustomserver.checked = false; > this._elements.customserver.disable = true; disabled? >+ >+ _validateServer: function _validateServer(aURL) { >+ let uri = Weave.Utils.makeURI(aURL); >+ >+ if (!uri) >+ uri = Weave.Utils.makeURI("https://" + aURL); >+ >+ if (!uri) >+ return ""; >+ return uri.spec; > } I'm a bit unsure of the name, the code does not really seem to validate a server. _getServerSpec? r+ with nits

Comment on attachment 492914 [details] [diff] [review] part 3 (jpake + tweaks) v2 >+ onComplete: function onComplete(aCredentials) { >+ self.close(); >+ >+ self.setupData = {}; >+ self.setupData.account = aCredentials.account; >+ self.setupData.password = aCredentials.password; >+ self.setupData.synckey = aCredentials.synckey; >+ self.setupData.serverURL = aCredentials.serverURL; You own aCredentials so you can just do self.setupData = aCredentials;

Comment on attachment 492914 [details] [diff] [review] part 3 (jpake + tweaks) v2 The simplified crypto (bug 603489) is going to land shortly before the JPAKE stuff. It has a few implications on how the UI presents the Sync Key which I think should be reflected in this patch as well. Unfortunately up until now UI code made assumptions about the Sync Key (length == 20 for example). Fortunately this will be fixed. Going forward UI should use * Weave.Utils.hyphenatePassphrase() to convert a Sync Key value (e.g. Weave.Service.passphrase) before showing it to the user, * Weave.Utils.normalizePassphrase() to convert a user entered value to the raw Sync Key (e.g. something that we can assign to Weave.Service.passphrase or pass to Weave.Service.login()), * Weave.Utils.isPassphrase() to check whether the entered value is a valid Sync Key. * We also have Weave.Utils.hyphenatePartialPassphrase() which can be used to hyphenate a Sync Key as the user types it in. Bug 612699 implements these changes for the integrated Firefox UI (it does a bit more than that since also no longer allow custom passphrases). >+ <textbox id="syncsetup-account" class="syncsetup-edit" placeholder="&sync.account;"/> >+ <textbox id="syncsetup-password" class="syncsetup-edit" placeholder="&sync.password;" type="password"/> >+ <textbox id="syncsetup-synckey" class="syncsetup-edit" placeholder="&sync.syncKey;"/> In the Firefox UI we'll have an onkeyup event listener here to hyphenate the Sync Key as you type: onPassphraseKeyUp: function (event) { if (event.keyCode != event.DOM_VK_BACK_SPACE) { let el = event.target; el.value = Weave.Utils.hyphenatePartialPassphrase(el.value); } this.checkFields(); }, > // Push the current setup data into the UI > if (this.setupData && "account" in this.setupData) { > this._elements.account.value = this.setupData.account; > this._elements.password.value = this.setupData.password; >- this._elements.synckey.value = this.setupData.syncKey; >- if (this.setupData.customServer && this.setupData.customServer.length) { >+ this._elements.synckey.value = this.setupData.synckey; You want to use Weave.Utils.hyphenatePassphrase() on this.setupData.synckey here before showing it in the textbox, unless this.setupData.synckey should already contain the presentable form. It's not clear to me. If the latter is the case you want to hyphenate the value you get from JPAKE. > if (this.setupData) { >- if (this.setupData.customServer && this.setupData.customServer.length) >- Weave.Service.serverURL = this.setupData.customServer; >- Weave.Service.login(Weave.Service.username, this.setupData.password, Weave.Utils.normalizePassphrase(this.setupData.syncKey)); >+ if (this.setupData.serverURL && this.setupData.serverURL.length) >+ Weave.Service.serverURL = this.setupData.serverURL; >+ Weave.Service.login(Weave.Service.username, this.setupData.password, Weave.Utils.normalizePassphrase(this.setupData.synckey)); Here for instance it seems like this.setupData.synckey contains the presentable form of the Sync Key. > let pp = Weave.Service.passphrase || ""; > if (pp.length == 20) > pp = Weave.Utils.hyphenatePassphrase(pp); This hardcoded length assumption needs to be removed from UI code. You can use Weave.Utils.isPassphrase() instead.

OK, I am updating the patch to store setupData.synckey in raw form and only hyphenate and normalize when dealing with the UI.

Updated with Phillip's feedback

A probably better revision to the fennec sync screen layouts: http://grab.by/7EBh (from mart3ll)

Comment on attachment 493800 [details] [diff] [review] part 3 (jpake + tweaks) v3 >+ //Components.utils.import("resource://services-sync/jpakeclient.js"); ... >+ this._jpakeclient = new JPAKEClient({ nit: Use Weave.JPAKEClient rather than doing the import yourself. We're trying to have UI code only go through the "Weave" namespace (it also means you're automatically being lazy about the import which is good). I know my patch sets a bad example, I will fix that. Sorry for not spotting this earlier.

* Changed the way we import the JPAKEClient based on Philipp's comment. * Removed key generation property * Tweaked style a little based on Madhava's mockup (adding strings needs to wait for after b3)

Comment on attachment 494930 [details] [diff] [review] part 3 (jpake + tweaks) v4 Just some nits: >+++ b/chrome/content/browser.xul >+ <description id="syncsetup-code1" class="syncsetup-code">code</description> >+ <description id="syncsetup-code2" class="syncsetup-code">goes</description> >+ <description id="syncsetup-code3" class="syncsetup-code">here</description> Remove the placeholder text ("code goes here") - we shouldn't need it anymore. >+++ b/chrome/content/sync.js >+ self.setupData = {}; >+ self.setupData.account = aCredentials.account; >+ self.setupData.password = aCredentials.password; >+ self.setupData.synckey = aCredentials.synckey; >+ self.setupData.serverURL = aCredentials.serverURL; I think this would be more readable as: self.setupData = { account: aCrendentials.account, password: aCredentials.password, /* ... */ }; (or even "self.setupData = aCredentials;") > this.setupData = {}; > this.setupData.account = this._elements.account.value; > this.setupData.password = this._elements.password.value; >+ this.setupData.synckey = Weave.Utils.normalizePassphrase(this._elements.synckey.value); >+ this.setupData.serverURL = this._validateServer(this._elements.customserver.value); Same here. >+ if (!this._jpakeclient) To avoid ES5 Strict warnings, use 'if (!"_jpakeclient" in this)' or initialize _jpakeclient to null (and set it to null again instead of deleting it). >+ onAbort: function onAbort(error) { aError

> (or even "self.setupData = aCredentials;") Philipp already suggested that and I forgot. Thanks. All Matt's suggestions have been made locally.

pushed: http://hg.mozilla.org/mobile-browser/rev/85d4b6d54c79 I was able to receive JPAKE codes fine. I was not able to test connecting via JPAKE after adding codes to Firefox desktop via "Add a device". Will test ASAP.

Verified Fixed on: Mozilla/5.0(Android; Linux armv7l; rv:2.0b8pre) Gecko/20101214 Firefox/4.0b8pre Fennec/4.0b3pre