Closed Bug 1036054 Opened 5 years ago Closed 5 years ago

Convert Sync pane dialog to be in-content

Categories

(Firefox :: Preferences, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 33

People

(Reporter: Paenglab, Assigned: Paenglab)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Convert sync.js to use a alert() instead a dialog.
Attached patch syncPaneIncontent.patch (obsolete) — Splinter Review
(In reply to Matthew N. [:MattN] (behind on bugmail) from comment #63)
> Comment on attachment 8448050 [details] [diff] [review]
> inContentDialogs.patch
> 
> Review of attachment 8448050 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: browser/components/preferences/in-content/sync.js
> @@ +287,5 @@
> >          let heading = sb.formatStringFromName("firefoxAccountsVerificationSentHeading",
> >                                                [data.email], 1);
> >          let description = sb.GetStringFromName("firefoxAccountVerificationSentDescription");
> >  
> > +        alert(heading + "\n\n" + description);
> 
> Why is this change necessary?

This is to make also the info and confirmation dialogs in-content through alert() and confirm().

I know, not super needed, but more consistent with the other dialogs.
Attachment #8453944 - Flags: review?(MattN+bmo)
Comment on attachment 8453944 [details] [diff] [review]
syncPaneIncontent.patch

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

::: browser/components/preferences/in-content/sync.js
@@ +287,5 @@
>          let heading = sb.formatStringFromName("firefoxAccountsVerificationSentHeading",
>                                                [data.email], 1);
>          let description = sb.GetStringFromName("firefoxAccountVerificationSentDescription");
>  
> +        alert(heading + "\n\n" + description);

MDN says to avoid this for chrome code and this doesn't give the title styling. It seems like the proper way is to use allowTabModal on the prompter: https://mxr.mozilla.org/mozilla-central/source/toolkit/components/prompts/src/nsPrompter.js?rev=8a7368027aad&mark=502-507#502

Example: https://mxr.mozilla.org/mozilla-central/source/browser/metro/components/PromptService.js?rev=69d61e42d5df&mark=42#41

@@ +298,5 @@
>      this.openContentInBrowser(url);
>    },
>  
> +  unlinkFirefoxAccount: function(confirmed) {
> +    if (confirmed) {

Please revert the argument name change as I think it was fine as it was AFAICT.

@@ +307,5 @@
>        let body = sb.GetStringFromName("disconnect.verify.heading") +
>                   "\n\n" +
>                   sb.formatStringFromName("disconnect.verify.description",
>                                           [brandShortName], 1);
> +      let pressed = confirm(body);

I would consider this a UX regression to go from nicely labelled buttons to the default confirm ones.
Attachment #8453944 - Flags: review?(MattN+bmo) → review-
Attached patch syncPaneIncontent.patch (obsolete) — Splinter Review
Is this what you thought with the allowTabModal?
Attachment #8453944 - Attachment is obsolete: true
Attachment #8456448 - Flags: review?(MattN+bmo)
Comment on attachment 8456448 [details] [diff] [review]
syncPaneIncontent.patch

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

Looks good. Well done other than the style issues below.

::: browser/components/preferences/in-content/sync.js
@@ +288,5 @@
>          let heading = sb.formatStringFromName("firefoxAccountsVerificationSentHeading",
>                                                [data.email], 1);
>          let description = sb.GetStringFromName("firefoxAccountVerificationSentDescription");
>  
> +        let factory = Components.classes['@mozilla.org/prompter;1']

We usually use double quotes for JS in browser/toolkit. You can use Cc as a shorthand for Components.classes

@@ +289,5 @@
>                                                [data.email], 1);
>          let description = sb.GetStringFromName("firefoxAccountVerificationSentDescription");
>  
> +        let factory = Components.classes['@mozilla.org/prompter;1']
> +                                .getService(Components.interfaces.nsIPromptFactory);

You can use Ci as a shorthand for Components.interfaces

@@ +292,5 @@
> +        let factory = Components.classes['@mozilla.org/prompter;1']
> +                                .getService(Components.interfaces.nsIPromptFactory);
> +        let prompt = factory.getPrompt(window, Components.interfaces.nsIPrompt);
> +        let bag = prompt.QueryInterface(Components.interfaces.nsIWritablePropertyBag2);
> +        bag.setPropertyAsBool('allowTabModal', true);

double quotes please

@@ +294,5 @@
> +        let prompt = factory.getPrompt(window, Components.interfaces.nsIPrompt);
> +        let bag = prompt.QueryInterface(Components.interfaces.nsIWritablePropertyBag2);
> +        bag.setPropertyAsBool('allowTabModal', true);
> +
> +        prompt.alert.apply(null, [title, heading + "\n\n" + description]);

Why didn't you call the function directly instead of using .apply?
e.g. prompt.alert(title, heading + "\n\n" + description);
Attachment #8456448 - Flags: review?(MattN+bmo) → feedback+
Attached patch syncPaneIncontent.patch (obsolete) — Splinter Review
Fixed the comments.
Attachment #8456448 - Attachment is obsolete: true
Attachment #8456679 - Flags: review?(MattN+bmo)
Comment on attachment 8456679 [details] [diff] [review]
syncPaneIncontent.patch

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

You should probably run this on try server in case there are tests for the dialogs.

I wondered why you didn't convert the prompt in the startOver method too but you told me on IRC that it was because it was for old sync only and it seems reasonable to leave those as they'll be removed soon.

Thanks for all your contributions!

::: browser/components/preferences/in-content/sync.js
@@ +278,5 @@
>                                                [data.email], 1);
>          let description = sb.GetStringFromName("firefoxAccountVerificationSentDescription");
>  
> +        let factory = Cc["@mozilla.org/prompter;1"]
> +                         .getService(Ci.nsIPromptFactory);

Nit: this is not indented properly. The leading period should be two spaces in from the left of "Cc".

@@ +281,5 @@
> +        let factory = Cc["@mozilla.org/prompter;1"]
> +                         .getService(Ci.nsIPromptFactory);
> +        let prompt = factory.getPrompt(window, Ci.nsIPrompt);
> +        let bag = prompt.QueryInterface(Ci.nsIWritablePropertyBag2);
> +        bag.setPropertyAsBool("allowTabModal", true);

This stuff could probably be in a function to avoid duplication for the two callers but I suppose it's fine for just these two since they are using different prompt methods. A helper to get the prompt would probably still be good but doesn't need to block.

@@ +283,5 @@
> +        let prompt = factory.getPrompt(window, Ci.nsIPrompt);
> +        let bag = prompt.QueryInterface(Ci.nsIWritablePropertyBag2);
> +        bag.setPropertyAsBool("allowTabModal", true);
> +
> +        prompt.alert(title, heading + "\n\n" + description);

Please file a follow-up to get the title to show in this case. Right now we don't show the title in the dialog but I think we should for cases like this (as opposed to when it's showing the remote origin for untrusted dialogs). See http://hg.mozilla.org/mozilla-central/annotate/869971ad9fd6/toolkit/components/prompts/content/tabprompts.xml#l168

@@ +289,5 @@
>      });
>    },
>  
>    openOldSyncSupportPage: function() {
> +    let url = Services.urlFormatter.formatURLPref("app.support.baseURL") + "old-sync"

While you're touching this line, can you add the missing semicolon on the end?

@@ +311,5 @@
>                          (ps.BUTTON_POS_1 * ps.BUTTON_TITLE_CANCEL) +
>                          ps.BUTTON_POS_1_DEFAULT;
> +
> +      let factory = Cc["@mozilla.org/prompter;1"]
> +                       .getService(Ci.nsIPromptFactory);

Ditto.

@@ +323,5 @@
>        if (pressed != 0) { // 0 is the "continue" button
>          return;
>        }
>      }
> +    Components.utils.import("resource://gre/modules/FxAccounts.jsm");

While you're touching this line, can you switch "Components.utils" to "Cu"?
Attachment #8456679 - Flags: review?(MattN+bmo) → review+
Comments fixed.

Try: https://tbpl.mozilla.org/?tree=Try&rev=74b6bf3afed6

I think the xpcshell failures aren't from my patch.
Attachment #8456679 - Attachment is obsolete: true
Attachment #8456814 - Flags: review+
Keywords: checkin-needed
(In reply to Matthew N. [:MattN] (behind on bugmail) from comment #6)
> Comment on attachment 8456679 [details] [diff] [review]
> syncPaneIncontent.patch
> 
> Review of attachment 8456679 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +283,5 @@
> > +        let prompt = factory.getPrompt(window, Ci.nsIPrompt);
> > +        let bag = prompt.QueryInterface(Ci.nsIWritablePropertyBag2);
> > +        bag.setPropertyAsBool("allowTabModal", true);
> > +
> > +        prompt.alert(title, heading + "\n\n" + description);
> 
> Please file a follow-up to get the title to show in this case. Right now we
> don't show the title in the dialog but I think we should for cases like this
> (as opposed to when it's showing the remote origin for untrusted dialogs).
> See
> http://hg.mozilla.org/mozilla-central/annotate/869971ad9fd6/toolkit/
> components/prompts/content/tabprompts.xml#l168

Filed bug 1039462 for this.
https://hg.mozilla.org/mozilla-central/rev/c12579b0b856
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
You need to log in before you can comment on or make changes to this bug.