This bug will do a few more Sync changes since Firefox 4. This is the less invasive changes. I purposely kept Firefox format of the expressions when I copied this over, if I need any changed, I can do that.

Accidentally had part of another patch here.

Comment on attachment 558101 [details] [diff] [review] v1.1 Only from glancing at the patch I found these, actual review pending: >+++ b/suite/common/sync/syncGenericChange.js >+ if (this._duringSetup) { >+ this._dialog.getButton("finish").disabled = false; >+ } No block for single-line conditionals. >+++ b/suite/common/sync/syncSetup.js > * Edward Lee <edilee@mozilla.com> >+ * Edward Lee <edilee@mozilla.com> Hrm... >+++ b/suite/common/sync/syncUtils.js >- Services.ww.activeWindow.openDialog(changeXUL, "", changeOpt, type); >+ Weave.Svc.WinWatcher.activeWindow.openDialog(changeXUL, "", changeOpt, >+ type, duringSetup); This changed between the bug you ported and now (Services use). Please check with the current FF files (both this case and any others there may be).

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #2) > >+++ b/suite/common/sync/syncGenericChange.js > >+ if (this._duringSetup) { > >+ this._dialog.getButton("finish").disabled = false; > >+ } > > No block for single-line conditionals. Done (only case of it in this patch) > >+++ b/suite/common/sync/syncSetup.js > > * Edward Lee <edilee@mozilla.com> > >+ * Edward Lee <edilee@mozilla.com> > > Hrm... No idea how that happened (I don't remember touching that line.) > >+++ b/suite/common/sync/syncUtils.js > >- Services.ww.activeWindow.openDialog(changeXUL, "", changeOpt, type); > >+ Weave.Svc.WinWatcher.activeWindow.openDialog(changeXUL, "", changeOpt, > >+ type, duringSetup); > > This changed between the bug you ported and now (Services use). Please check > with the current FF files (both this case and any others there may be). I tried to look at the current state in all my patches, I missed that Services change here though, fixed.

Sorry, I was mostly busy doing other stuff this weekend (but at least I was able to prepare 2.4b2!). Just so you cannot say I didn't do anything: The bug 620593 port part is OK. :-) The bug 626099 port part is also OK regarding the spacer and CSS, but I wonder what the benefit regarding the labels (value attributes vs. text) or if it might even regress things (I think I remember there was some difference; maybe Neil knows). I found that with the patch, the second link in the paragraph on the Sign In wizard page ("I have lost my other device.") doesn't work anymore. It's the same with FF, but I'd rather not want to regress this. Any chance that you can track this down? Must be the bug 630885 port since bug 626099 didn't change what was called there. [With current, unpatched trunk this opens the Change your Sync Key dialog, filling in "undefined" for Your Sync Key, but that's for another bug; possibly already fixed for FF and in our to-port queue.] Other options include: Ask the FF devs (philiKON, rnewman) or move to an extra bug.

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #4) > Sorry, I was mostly busy doing other stuff this weekend (but at least I was > able to prepare 2.4b2!). > > Just so you cannot say I didn't do anything: The bug 620593 port part is OK. > :-) > > The bug 626099 port part is also OK regarding the spacer and CSS, but I > wonder what the benefit regarding the labels (value attributes vs. text) or > if it might even regress things (I think I remember there was some > difference; maybe Neil knows). IIRC it allows these attribs to wrap if needed by l10n. > I found that with the patch, the second link in the paragraph on the Sign In > wizard page ("I have lost my other device.") doesn't work anymore. It's the > same with FF, but I'd rather not want to regress this. Any chance that you > can track this down? Must be the bug 630885 port since bug 626099 didn't > change what was called there. [With current, unpatched trunk this opens the > Change your Sync Key dialog, filling in "undefined" for Your Sync Key, but > that's for another bug; possibly already fixed for FF and in our to-port > queue.] Other options include: Ask the FF devs (philiKON, rnewman) or move > to an extra bug. These two bugs I am working on with r? to you, fix all that is identified on our to-port-queue. This would definitely be 630885 as far as what changed here, but thats been fixed and wfm for a while on Firefox (as of last I checked). This did have an issue you identified above that I fixed locally, but *might* cause your issue here. |Weave.Svc.WinWatcher.activeWindow.openDialog| rather than |Services.ww.activeWindow.openDialog| Was there anything in your error console when you tested? (Its hard for me to test, since I already have Sync setup) I would rather stay bug-for-bug with Firefox here, unless we can find a solution to the one they introduced...

(In reply to Justin Wood (:Callek) from comment #5) > Was there anything in your error console when you tested? (Its hard for me > to test, since I already have Sync setup) I would rather stay bug-for-bug > with Firefox here, unless we can find a solution to the one they > introduced... To be clear, were you testing with the wizard format of the dialog as adjusted in our Bug 684537

(In reply to Justin Wood (:Callek) from comment #5) > IIRC it allows these attribs to wrap if needed by l10n. I think we should do a quick check (e.g. change the value of such a label to a longer string and see if it wraps the old/new way) and/or consult Neil. > This did have an issue you identified above that I fixed locally, > but *might* cause your issue here. > > |Weave.Svc.WinWatcher.activeWindow.openDialog| rather than > |Services.ww.activeWindow.openDialog| Nah pal, I fixed that locally, too. ;-) > Was there anything in your error console when you tested? Not reliably, otherwise I would have posted it. But I think I saw that FF showed an in-UI error message in this case, something like "please provide a user name". Maybe that's expected behavior and our current one (without the patch: dialog with "undefined", see comment 4) is wrong. In that case we'd have to check and see what needs to be ported additionally to get that. In any case, clicking the label/link should/must do *something*. BTW my STR were: 1. Tools/Set Up Sync 2. Connect 3. I don't have the device with me 4. I have lost my other device. I.e. I intentionally didn't fill in any account info. [The "undefined" is a regression by itself, maybe even caused by one of my own porting patches. If this dialog is not to be opened under this circumstances anymore, we can ignore it, though.] > (Its hard for me to test, since I already have Sync setup) Lame excuse, that's what profile management is for (-P, create fresh profile)! :-)

(In reply to Justin Wood (:Callek) from comment #6) > To be clear, were you testing with the wizard format of the dialog as > adjusted in our Bug 684537 No, I only had this bug's patch applied (with the fixes as per comment 2). I'll see how it looks with that other bug's patch applied additionally, but only several hours from now.

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #8) > (In reply to Justin Wood (:Callek) from comment #6) > > To be clear, were you testing with the wizard format of the dialog as > > adjusted in our Bug 684537 > > No, I only had this bug's patch applied (with the fixes as per comment 2). > > I'll see how it looks with that other bug's patch applied additionally, but > only several hours from now. Hrm, I just tested, got same behavior; then tested in Firefox and got the error message that I think is satisfactory here. C.F. Bug 642969 which was exactly for this reason. Are you ok with me spinning off a new bug for 642969 right after these two bugs land. or are you planning on requiring me to update this patch?

(In reply to Justin Wood (:Callek) from comment #9) > Are you ok with me spinning off a new bug for 642969 right after these two > bugs land. or are you planning on requiring me to update this patch? Please wrap it in here (BTW AFAICS the obs change is obsolete), I think it's small enough, prevents the regression, and the l10n freeze (Aurora uplift) is approaching fast so I either want to have this in together or none of them. In any case I'll try to apply it locally and see where it gets me.

> The bug 626099 port part is also OK regarding the spacer and CSS, but I wonder what > the benefit regarding the labels (value attributes vs. text) or if it might even > regress things (I think I remember there was some difference; maybe Neil knows). The latter construct creates a textNode as a child of <label>. This allows the text to wrap.

(In reply to Philip Chee from comment #11) > The latter construct creates a textNode as a child of <label>. This allows > the text to wrap. Heh, sounds logical, thanks for the explanation! :-) I guess I'll go and update <https://developer.mozilla.org/en/XUL/label> then (since no-one else seems to either know or care over there). Now the only thing I'm still concerned about there is the whitespace before and after the entity (with the patch). I hope it makes no practical difference here, or that XUL handles this differently than HTML. In any case I found that even without this patch we already have two occurrences of labels with a text node instead of a value: &setup.tosLink.label; and &setup.ppLink.label; (both from bug 622408 for FF, ported to SM as part of the initial dialogs patch) so I finally accept (and appreciate!) the wrapping aspect.

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #10) > (BTW AFAICS the obs change is obsolete) Correction: It's not (I was thinking in terms of syncUI.js, but this is a different file). > the l10n freeze (Aurora uplift) is approaching fast Actually I misread the patch on bug 642969, the l10n changes there were against /services so no l10n impact for us regarding that bug. However that reminded me that I'll have to check each and every entity that is referred to in the patches, be it in XUL or JS files. Oh well...

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #12) > Now the only thing I'm still concerned about there is the whitespace before > and after the entity (with the patch). I hope it makes no practical > difference here, or that XUL handles this differently than HTML. I checked now using the DOM Inspector and found that the whitespace is in fact maintained, so in general one should not do this (even if it looks nicer). In this case, though, the two labels are to be displayed as two separate sentences so it doesn't matter much how much whitespace there is. The spacer is obsolete, too, it seems (maybe due to the surrounding description tag), but I'm not opposed to keep it.

This additionally folds in: Bug 642969 - "I have lost my other device" link doesn't do anything at all, unless you've typed username and password. r=philikon Bug 636353 - Default file name for sync key isn't localized. r=rnewman Which were both in the single push that held up you approving this one, and neither was currently tracked by our to-be-tracked bug (I just added them). Both easy enough to apply here.

Comment on attachment 559933 [details] [diff] [review] v2 Indentation glitches (tabs vs spaces): > case Weave.LOGIN_FAILED_LOGIN_REJECTED: >+ case Weave.LOGIN_FAILED_NO_USERNAME: >+ case Weave.LOGIN_FAILED_NO_PASSWORD: > passphraseSave: function(elid) { > let dialogTitle = this._stringBundle.GetStringFromName("save.synckey.title"); >+ let defaultSaveName = this.bundle.GetStringFromName("save.default.label"); > this._preparePPiframe(elid, function(iframe) { Will check the rest later.

Comment on attachment 559933 [details] [diff] [review] v2 Review of attachment 559933 [details] [diff] [review]: ----------------------------------------------------------------- r=me with all nits addressed (from here and comment 16). Please include the ported bug numbers in the check-in comment (to help searching the log). This bug's summary is a little... generic. Consider changing it to include "(port bug 620593, bug 626099, bug 630885, bug 636353 and bug 642969)". TUVM! :-) BTW: There are some error messages triggered by canceling the wizard. I filed FF bug 686366 for them (so that I can port back to SM an r+ patch). ::: suite/common/sync/syncGenericChange.js @@ +106,5 @@ > introText2.textContent = this._str("change.synckey.introText2"); > warningText.textContent = this._str("change.synckey.warningText"); > this._dialog.getButton("accept").label = this._str("change.synckey.acceptButton"); > + if (this._duringSetup) > + this._dialog.getButton("finish").disabled = false; s/finish/accept/ (this will only be changed in bug 684537!) ::: suite/common/sync/syncSetup.js @@ +173,5 @@ > + if ([Weave.LOGIN_FAILED_INVALID_PASSPHRASE, > + Weave.LOGIN_FAILED_NO_PASSPHRASE, > + Weave.LOGIN_SUCCEEDED].indexOf(Weave.Status.login) == -1) { > + return; > + } No block, no curly braces.

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #17) > Comment on attachment 559933 [details] [diff] [review] > v2 > ::: suite/common/sync/syncSetup.js > @@ +173,5 @@ > > + if ([Weave.LOGIN_FAILED_INVALID_PASSPHRASE, > > + Weave.LOGIN_FAILED_NO_PASSPHRASE, > > + Weave.LOGIN_SUCCEEDED].indexOf(Weave.Status.login) == -1) { > > + return; > > + } > > No block, no curly braces. Besides just copying Firefox, I've always been of the mind that if the if spans multiple lines to block-style it. That said, I went ahead with your requested modification. > s/finish/accept/ (this will only be changed in bug 684537!) Adjusted, (and 684537 locally). http://hg.mozilla.org/comm-central/rev/807e670a5b48

(In reply to Justin Wood (:Callek) from comment #18) > > No block, no curly braces. > > Besides just copying Firefox, I've always been of the mind that if the if > spans multiple lines to block-style it. AFAIUI the relevant question is whether the statement "body" consists of a single command (e.g. assignment) or not. So it doesn't even matter how many lines any part spans. But hey, personally I'm a 1TBS fan where everything is block styled, with no freakin' exception. ;-)

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #16) > > passphraseSave: function(elid) { > > let dialogTitle = this._stringBundle.GetStringFromName("save.synckey.title"); ^^^^^^^^^^^^^ > >+ let defaultSaveName = this.bundle.GetStringFromName("save.default.label"); ^^^^^^ OMG, stupid us for not catching this. Too much copy & paste (from bug 636353). Now it's broken. :-( Filed bug 689039.