Get an account implementation tracker

RESOLVED FIXED in Thunderbird 10.0

Status

defect
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: protz, Assigned: mconley)

Tracking

(Depends on 1 bug)

unspecified
Thunderbird 10.0
x86_64
Linux
Dependency tree / graph
Bug Flags:
sec-review +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [secr:dveditz][secr:dchan])

Attachments

(3 attachments, 11 obsolete attachments)

2.26 KB, image/png
Details
1.35 KB, image/png
Details
475.60 KB, patch
standard8
: review+
Details | Diff | Splinter Review
Reporter

Description

8 years ago
This is bug is for tracking my progress on landing get an account in the main product.

Here's a preliminary patch that doesn't do much, except for getting the right files into the tree, writing the right manifests, Makefiles, and that sort of stuff. Also, getting rid of the overlays, and doing the right tweaks instead of monkey-patching all sorts of stuff.

Per the phone discussion with JB, here's a list of things that we should watch for.
1. Interaction with opensearch (what happens if the user has made a search engine his favorite choice through opensearch, and he then gets an account through one of our add-supported partners?).
2. Make sure the list of providers is stored remotely so that we can add new partners without having to deploy a new version of Thunderbird (Blake, I believe this is in place already, can you confirm this?).
3. Make sure the API for creating an account (i.e. the remote API) is well-defined. Apparently, this is not the case for Yahoo.
4. Another item related to configuring specific email accounts which I wrote down, but I'm not sure what this is supposed to refer to. Jb, thoughts?

I'll be mainly posting updates for the patch, and pinging Blake and maybe Andreas for design decisions, appearance stuff, etc.

Blake, here's some questions for you.
- why did you rewrite the new ComposeMessage function? there doesn't seem to be anything specific in it,
- I'm not sure I understand what you wanted to overlay with mailWizardOverlay.xul,
- hopefully I didn't miss any place where one can create a new account.

Let me know if you have any thoughts / comments, here would be a great place to keep them.

Going to sleep.
Reporter

Comment 1

8 years ago
Posted patch Giga-patch (obsolete) — Splinter Review
Here's an updated patch with:
- accountProvisioner as xhtml so that one can use localized entities,
- templates tweaked so that they use the string from the properties file,
- all strings localized.

I'm in the process of converting comm-central to git, and I'll work on a branch as soon as it's done, so that bwinton can track my progress on GitHub.
Attachment #559891 - Attachment is obsolete: true
Reporter

Comment 2

8 years ago
Posted patch Roughly works (see below) (obsolete) — Splinter Review
Here's the latest version of the patch. It pretty much does everything we want, including the interaction with opensearch. There's a few remaining problems, though:

① I changed the test to use a local version of all the needed files, and now it's broken, and I can't figure out why. It looks like registering the progressListener on the browser prevents it from ever completing the page load. I suspect some tricky issue in Gecko. Blake, if you could help me out with this, that would be awesome. Just run the test to witness the issue ☺.

② I bumped jquery and jquery-ui in-trunk, because get-an-account used a newer version of the files, and I thought that would be a nice thing to have for addon developers. AFAICT, it doesn't break anything in the facet view.

③ I also did the language stuff, that is, only showing (if there are any) providers in the user's language. I used the intl.accept_languages pref for that, could someone confirm that this is the right way to get the user's language?

④ Blake, I don't know if the test I wrote satisfies you. Please tell me if you want more testing. If you can help out with writing more tests and/or fixing the other broken tests (account creation), that would be great. I suspect fixing account creation-related tests is not much work.

⑤ I did *not* do the password thing: the password is not pre-filled once the provider returns the XML config. I suspect this is a fair bit of work, at least for me: I have no idea how to interact with NSS to add from JS a password into the database, and I don't even know if this is possible. I suggest we fix this in a followup bug.

Blake, I need you to tell me precisely what kind of guarantees you need before landing this. There's still some issues, as you can see above, but I guess once we solve them, we should be able to land this. If, say, a try-build gives no regression, would you agree to r+ this patch?

Thanks!

jonathan
Attachment #560809 - Attachment is obsolete: true
Attachment #568012 - Flags: feedback?(bwinton)
Reporter

Comment 4

8 years ago
I'm using the createInBackend function already, so if the provider emits the right fields in its config.xml file, it should be able to leverage that function by setting rememberPassword... The main problem is remembering passwords for imap accounts, and I don't think that function allows you to do that.

The right way would probably be to enrich createInBackend so that it can take that scenario into account...
Reporter

Comment 5

8 years ago
⑤ Has been answered by bwinton. Writing it down so that I don't lose the information.

<bwinton> So, you call readFromXML (http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/readFromXML.js#52), passing in the XML, and it'll give you back a config.
<bwinton> Then you set config.incoming.password and config.rememberPassword on the config, and pass it to createAccountInBackend

There's also another point we need to address.

⑥ We need Andy to create a search icon for us. Right now, the one I'm using in the opensearch customization dialog (see <http://jonathan.protzenko.free.fr/shutter/Welcome%20to%20Thunderbird_020.png>) is not optimal.
Comment on attachment 568012 [details] [diff] [review]
Roughly works (see below)

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

There are a bunch of things to fix, but I think we're on the right track.

Thanks,
Blake.

::: mail/app/profile/all-thunderbird.js
@@ -724,3 +724,5 @@
> >  pref("mail.taskbar.lists.enabled", true);
> >  pref("mail.taskbar.lists.tasks.enabled", true);
> >  #endif
> > +
> > +// Account provisioner. Commented out prefs are for "the real world".

I think we should use "mail.provider.<blah>" instead of "getanaccount.<blah>".  We should also use the official prefs, and not have my private server in the official code.  ;)

(Perhaps it's time to bug Sancus to get this set up?)

::: mail/base/content/mailWindowOverlay.xul
@@ +84,4 @@
>  <script type="application/javascript" src="chrome://messenger/content/msgViewPickerOverlay.js"/>
>  <script type="application/javascript" src="chrome://messenger/content/plugins.js"/>
>  <script type="application/javascript" src="chrome://global/content/viewZoomOverlay.js"/>
> +<script type="text/javascript;version=1.8" src="chrome://messenger/content/getanaccount/uriListener.js" />

"getanaccount" is a terrible name here, too.  "accountprovisioner" is better, but could it just go in "account" or "newaccount"?

@@ -984,4 +985,4 @@
> >                <menuitem id="newMailAccountMenuItem"
> >                          label="&newEmailAccountCmd.label;"
> >                          accesskey="&newEmailAccountCmd.accesskey;"
> > -                        oncommand="NewMailAccount(msgWindow);"/>
> > +                        oncommand="NewMailAccountProvisioner(msgWindow);"/>

Why not just change the NewMailAccount function?  (I couldn't cause I was in an addon, but that shouldn't be a limitation you run into.)

::: mail/components/Makefile.in
@@ +44,4 @@
>  
>  # Only Mac and Windows have search integration components, but we include at
>  # least one module from search/ on all platforms
> +DIRS    = compose preferences addrbook migration activity search about-support wintaskbar get-an-account

As above, "get-an-account" isn't a great name for the feature.

::: mail/components/get-an-account/content/accountProvisioner.js
@@ +14,5 @@
> + * The Original Code is Account Provisioner Code.
> + *
> + * The Initial Developer of the Original Code is
> + * The Mozilla Foundation.
> + * Portions created by the Initial Developer are Copyright (C) 2010

I think this might be "2011", but maybe not…

@@ +56,5 @@
> + *
> + * @param {String} page The page to get the localstorage for.
> + * @return {nsIDOMStorage} The localstorage for this page.
> + */
> +function getLocalStorage(page) {

I wonder if there's a better place to store these things than localstorage, like, say, prefs?

Actually, since the main reason I needed to do this was because this was a whole different page, instead of being part of the normal account wizard, I wonder if we need to do it at all?  (You did just merge this feature into the regular account wizard, right?)

@@ +85,5 @@
> +
> +/**
> + * Get the default opensearch engine. Stolen from bug 677421.
> + */
> +function getDefaultSearchEngine() {

Shouldn't this be the responsibility of the OpenSearch feature?

@@ +116,5 @@
> + *
> + * @param provider The provider we created the account for.
> + * @param config The created config.
> + */
> +function logSuccess(provider, config) {

I think we can do this through Telemetry, or Test Pilot instead, and then the Privacy and Security people won't yell at us.  ;)

@@ +164,5 @@
> +}
> +
> +function splitName(str) {
> +  let i = str.lastIndexOf(" ");
> +  if (i >= 0)

I think this test should be ">= 1", so that we don't try to send an empty first name if the user enters a space at the start…

And, actually, we might want to trim the string before calling this function.

@@ +172,5 @@
> +}
> +
> +$(function() {
> +  // Snarf the things I need out of the window arguments.
> +  let NewMailAccount = window.arguments[0].NewMailAccount;

I was really hoping these lines wouldn't be necessary once we moved this into core.  Can't we get the core code to put these variables in the newly-opened window?

@@ +203,5 @@
> +      providers[provider.id] = provider;
> +      // Update the terms of service and privacy policy links.
> +      let sep = "";
> +      if (i == data.length - 1)
> +        ;

Is this really an empty statement?  And if so, why not reverse the test in the else clause instead?

@@ +208,5 @@
> +      else if (i == data.length - 2)
> +        sep = stringBundle.get("sepAnd");
> +      else
> +        sep = stringBundle.get("sepComma");
> +      placeholder

How well does all of this appending work with RTL languages?

@@ +227,5 @@
> +  });
> +  let name = storage.getItem("name") || $("#Name").text();
> +  let username = storage.getItem("username");
> +  let domain = storage.getItem("domain");
> +  $("#Name").val(name);

I think we should be consistent with our use of the case of ids, and so this should be "#name".

@@ +240,5 @@
> +    expandSection(false);
> +  });
> +
> +  $("button.existing").click(function() {
> +    actionList.push("Using Existing");

I think we should remove all this actionList stuff, although we may want to leave it in for TestPilot purposes later…

@@ +263,5 @@
> +    $(this).css({"opacity": "0.0", "display": "inline"});
> +  });
> +
> +  $(window).unload(function() {
> +    actionList.push("Closing");

We _really_ want to remove the following two function calls.  :)

@@ +282,5 @@
> +  $(window).keydown(function(e) {
> +    // Handle Cmd-W.
> +    if (e.keyCode == "224") {
> +      metaKey = true;
> +    } else if (e.keyCode == "87" && ((e.ctrlKey && !e.altKey) || metaKey)) {

This looks like it shouldn't be an "else if", but should instead be an "if".

Also, I think we want "var metaKey", or "let metaKey", so that we don't create a global variable.  (Oh, but I may be wrong about that.  Perhaps we want it to be global, cause it needs to be persisted between calls.)

@@ +296,5 @@
> +    actionList.push("Searching");
> +    $("#notifications").children().hide();
> +    saveState();
> +    let name = $("#Name").val();
> +    if (name.length <= 0) {

I think this might actually be the right place to call trim… (or was that strip?)

And while we're doing this, we _really_ need to sanitize that input, cause a name of "<b>Blake</b> Winton", or "<!--" does not do the right thing at all…  (Granted, it gives errors instead of running javascript, but still, not good.  :)

@@ +301,5 @@
> +      $("#Name").select().focus();
> +      $(".search").removeAttr("disabled");
> +      return;
> +    }
> +    $("#notifications .spinner").show();

Is this spinner the same spinner that we use elsewhere in Thunderbird, or are we going to have more than one style of spinner?

Also, how do I cancel this, if I typoed my name, for instance?

@@ +305,5 @@
> +    $("#notifications .spinner").show();
> +    let [firstname, lastname] = splitName(name);
> +    $.getJSON(suggestFromName,
> +              {"first_name": firstname, "last_name": lastname},
> +              function(data) {

Don't we need to tell the Mozilla server which providers the user chose to ask for suggestions?

@@ +310,5 @@
> +      let results = $("#results").empty();
> +      $(".search").removeAttr("disabled");
> +      let searchingFailed = true;
> +      let userLanguages = // "fr-FR, fr"; // for testing
> +        Services.prefs.getCharPref("intl.accept_languages").split(",");

Should we strip spaces after we split on the ","?

@@ +324,5 @@
> +          let group = $("<div class='resultsGroup'></div>");
> +          let header = $("#resultsHeader").clone().removeClass("displayNone");
> +          header.children(".provider").text(providers[provider.provider].label);
> +          if (provider.price)
> +            header.children(".price").text("$"+provider.price);

I think we'll need to localize the prices somehow…  Although, for this version, everything is coming back in USD, so perhaps not yet.

@@ +331,5 @@
> +          group.append(header);
> +          for each (let [j, address] in Iterator(provider.addresses)) {
> +            let tmplData = {
> +              address: address,
> +              priceStr: stringBundle.get("price", [provider.price]),

And it looks like we're doing some localization here.

@@ +359,5 @@
> +          }
> +
> +          results.append(group);
> +        }
> +        if (foundUserLang == 0) {

Okay, so it looks like you're getting suggestions for all the providers, and then filtering out the ones you show.  I think we actually want to show the list of providers before we get suggestions, so that people can opt out if they don't trust, say, Google, and so that we don't ask for Russian email addresses for people living in the US who won't want them.

@@ +403,5 @@
> +    // And add the extra data.
> +    let data = storedData[provider.id];
> +    delete data.provider;
> +    for (let name in data) {
> +      url += (url.indexOf("?") == -1 ? "?" : "&") +

I think it would be better to store the separator in a variable, instead of scanning through the url all the time.

@@ +440,5 @@
> +  $("#results").delegate("div.more, div.address", "click", function() {
> +    let self = $(this);
> +
> +    // Return if we're already expanded
> +    if (self.parent().parent().hasClass("expanded"))

This navigation seems a little fragile.  Could we use self.closest(".something") instead?

@@ +454,5 @@
> +
> +    // And show this box.
> +    self.parent().parent().find(".more").hide();
> +    self.parent().siblings(".extra").slideDown();
> +    self.parent().parent().children().find(".pricing").fadeIn("fast");

Isn't this the same as self.parent().siblings().find(".pricing")?

@@ +490,5 @@
> +      $("#successful_account").show();
> +    });
> +
> +    let isChecked = (getCurrentSearchEngine() == getDefaultSearchEngine())
> +      || (getCurrentSearchEngine() == engine);

So, if the user previously chose the provider's search engine as their default, and then decided to un-check this box, what would you switch it to?

(The code looks like you wouldn't switch it, but that seems confusing to me.  On the other hand, not checking the box is also confusing.  I think we should perhaps not bother showing this page if the user has already selected the provider's search engine as their search engine.)

::: mail/components/get-an-account/content/accountProvisioner.xhtml
@@ +75,5 @@
> +  <script type="text/javascript;version=1.8"
> +          src="chrome://communicator/content/utilityOverlay.js">
> +  </script>
> +  <script type="text/javascript;version=1.8"
> +          src="chrome://messenger/content/getanaccount/jquery.dump.js">

Do we still need this?

@@ +112,5 @@
> +             src="chrome://messenger/content/getanaccount/spinner.gif"/>
> +
> +        <div class="error">
> +          <p>&error.line1;</p>
> +          <p>&error.line2;</p>

Why not just have a <br> in the error?

@@ +207,5 @@
> +  </div>
> +
> +</body>
> +
> +  <!-- Templates!  Woo! -->

Since these are so simple, I wonder if it would make sense to just create the elements inline instead, and remove the dependance on jquery-tmpl.

::: mail/components/get-an-account/content/jquery.dump.js
@@ +1,1 @@
> +/**

I bet we don't need this file anymore.  Hopefully.  :)

::: mail/components/get-an-account/content/uriListener.js
@@ +57,5 @@
> +Services.scriptloader.loadSubScript("chrome://messenger/content/accountcreation/emailWizard.js", accountCreationFuncs);
> +Services.scriptloader.loadSubScript("chrome://messenger/content/accountcreation/sanitizeDatatypes.js", accountCreationFuncs);
> +Services.scriptloader.loadSubScript("chrome://messenger/content/accountcreation/fetchhttp.js", accountCreationFuncs);
> +Services.scriptloader.loadSubScript("chrome://messenger/content/accountcreation/readFromXML.js", accountCreationFuncs);
> +Services.scriptloader.loadSubScript("chrome://messenger/content/accountcreation/guessConfig.js", accountCreationFuncs);

You really need guessConfig?!?

@@ +117,5 @@
> +            success: true,
> +            search_engine: this.params.searchEngine,
> +          });
> +        } catch (e) {
> +          dump(e+"\n");

console.log is the new dump.  ;)

::: mail/components/get-an-account/jar.mn
@@ +1,2 @@
> +messenger.jar:
> +    content/messenger/getanaccount/accountProvisioner.xhtml   (content/accountProvisioner.xhtml)

Blah blah blah "getanaccount should be better named…"  ;)

@@ +8,5 @@
> +    content/messenger/getanaccount/accountCreation.css        (skin/accountCreation.css)
> +    content/messenger/getanaccount/accountProvisioner.css     (skin/accountProvisioner.css)
> +    content/messenger/getanaccount/cvv2.png                   (skin/cvv2.png)
> +    content/messenger/getanaccount/icon64.png                 (skin/icon64.png)
> +    content/messenger/getanaccount/icon.png                   (skin/icon.png)

Do we really need icon64.png and icon.png?

Also, since we've removed the cvv2 validation stuff, I'm fairly sure we don't need that png.

@@ +11,5 @@
> +    content/messenger/getanaccount/icon64.png                 (skin/icon64.png)
> +    content/messenger/getanaccount/icon.png                   (skin/icon.png)
> +    content/messenger/getanaccount/overlay.css                (skin/overlay.css)
> +    content/messenger/getanaccount/search.gif                 (skin/search.gif)
> +    content/messenger/getanaccount/spinner.gif                (skin/spinner.gif)

Isn't there another spinner we can use, instead of including this one?

::: mail/components/get-an-account/skin/accountCreation.css
@@ +1,1 @@
> +#autoconfigWizard * {

I don't think we use this file, because it was designed to re-style the base account wizard, but we can just make our changes in it.

@@ +11,5 @@
> +}
> +
> +#autoconfigWizard,
> +#mastervbox {
> +  background-image: url('chrome://messenger/content/getanaccount/bg.png');

We don't seem to have this image in the patch…

::: mail/components/get-an-account/skin/accountProvisioner.css
@@ +54,5 @@
> +
> +input[type="submit"],
> +button {
> +  -moz-box-shadow: 0 0 0 1px rgba(255, 255, 255, 0.75) inset;
> +  background-image: -moz-linear-gradient(center top , #EEEEEE 0%, #EEEEEE 50%, #E1E1E1 50%, #E1E1E1 100%);

I suspect we want to split this file up into Mac, Linux, and Windows (and possible Windows Aero) versions, because while the colours work decently enough on Mac and Windows, they really don't mesh well with the Linux themes I've seen.

I also think they'll go poorly with the Windows High-Contrast stuff, although I haven't checked that yet.  Perhaps what we really want to do is use system colours in more places?

::: mail/components/get-an-account/skin/overlay.css
@@ +1,1 @@
> +#getanaccount-toolbar-button

I don't think we use this file either.

::: mail/jquery/jar.mn
@@ -1,1 @@
>  messenger.jar:

Does anyone else depend on the previous version of jquery or jquery-ui?  Like mail/test/resources/mozmill/mozmill/extension/content/mozmill.html?  Or mail/base/content/featureConfigurator.xhtml?  Or Extensions?  I'm not sure whether we can just upgrade it without doing a little more research first.

::: mail/locales/en-US/chrome/messenger/getanaccount/accountProvisioner.dtd
@@ +1,5 @@
> +<!ENTITY window.title "Welcome to Thunderbird">
> +<!ENTITY header.label "Want a new email address for Thunderbird?">
> +<!ENTITY error.line1 "Sorry we had no luck finding suggestions.">
> +<!ENTITY error.line2 "You try can search for nicknames or any other term to find more emails">
> +<!ENTITY error.suggest 'Also you might try one of the <a href="http://www.mozillamessaging.com/en-US/thunderbird/features/email_providers.html" class="external">free email account alternatives</a>.'>

I'm slightly surprised we can have < and > in these entities…

::: mail/locales/en-US/chrome/messenger/getanaccount/accountProvisioner.properties
@@ +1,1 @@
> +price=$%S a year

I wonder if we want to put in a note saying that the prices is always in USD, so please don't change that "$" to "€", "£", or "¥".

::: mail/test/mozmill/get-an-account/html/providerList
@@ +1,5 @@
> +[{"id": "hover",
> +  "label": "Hover.com",
> +  "paid": true,
> +  "languages" : ["en-US"],
> +  "api": "https://www.hover.com/tbReg?first={firstname}&last={lastname}&email={email}",

Um, maybe we want to use fake addreses for these, so that we don't potentially hit the network during our tests…

Fake names would also be a good idea, lest someone thinks that we're actually integrating with Hover and AOL…

::: mail/test/mozmill/get-an-account/html/registration.html
@@ +2,5 @@
> +<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
> +<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en">
> +  <head>
> +    <meta http-equiv="content-type" content="text/html; charset=UTF-8">
> +    <title>Blog-o!</title>

Let's change this title, while we're here…

@@ +8,5 @@
> +  <body>
> +  
> +    <div class="title">Local version</div>
> +    <div class="content">
> +      <form action="config.xml" method="GET">

Cute!

::: mail/test/mozmill/get-an-account/test-get-an-account.js
@@ +59,5 @@
> +
> +// RELATIVE_ROOT messes with the collector, so we have to bring the path back
> +// so we get the right path for the resources.
> +// Note: this one adds to '' as we need to make sure that favicon.ico is in the
> +// root directory.

Should we add a fake favicon.ico, too?

@@ +78,5 @@
> +  wh.installInto(module);
> +};
> +
> +// We can't use plan_for_new_window because it expects a window type and I have
> +// no idea what on earth is the windowtype of an HTML window.

I thought it was "AccountSetup", from:
+  window.openDialog(
+    "chrome://messenger/content/getanaccount/accountProvisioner.xhtml",
+    "AccountSetup",
+    // disabling modal for the time being, see 688273 REMOVEME
+    "chrome,titlebar,centerscreen,width=640,height=480",
+    args);

If not, can we pass in a window type in that call, and use it here?

@@ +109,5 @@
> +  let w = wait_for_provisioner_window();
> +
> +  // Fill in some data
> +  let $ = w.$;
> +  $("#Name").val("Green Llama");

I've suggested above a few other tests we should run, like "<!--", or "  Blake".

We're also not testing the failure cases, which is where a lot of the bugs with the current account wizard have cropped up.

@@ +115,5 @@
> +  mc.waitFor(function () $("#results").children().length > 0);
> +
> +  // Click on the first address. The reveals the button with the price.
> +  $(".address:first").click();
> +  mc.waitFor(function () $("button.create:visible").length > 0);

Should we check that the price is what we expect it to be?

@@ +139,5 @@
> +  $ = w.$;
> +  assert_equals(nAccounts(), i + 1);
> +
> +  // W00t account created
> +  dump("\033[01;36m#winning\033[00m\n");

Please no dump statements…  :)

@@ +140,5 @@
> +  assert_equals(nAccounts(), i + 1);
> +
> +  // W00t account created
> +  dump("\033[01;36m#winning\033[00m\n");
> +}

Should we check that the appropriate things get returned for various locales?

::: mailnews/base/prefs/content/AccountManager.js
@@ -385,4 @@
>    let msgWindow = Components.classes["@mozilla.org/messenger/services/session;1"]
>                              .getService(Components.interfaces.nsIMsgMailSession)
>                              .topmostMsgWindow;
> -  NewMailAccount(msgWindow);

So, I used to have to override this function because I was an extension, but I don't think that still applies, so we shouldn't need to do this, right?

::: mailnews/base/prefs/content/AccountManager.xul
@@ +20,4 @@
>  <script type="application/javascript" src="chrome://messenger/content/am-prefs.js"/>
>  <script type="application/javascript" src="chrome://messenger/content/AccountManager.js"/>
>  <script type="application/javascript" src="chrome://messenger/content/am-help.js"/>
> +<script type="application/javascript;version=1.8"

Do we still need this import?  Can't we just include the code here?

::: mailnews/base/prefs/content/accountUtils.js
@@ +330,4 @@
>    setTimeout(msgNewMailAccount, 0, msgWindow, okCallback, extraData);
>  }
>  
> +function NewMailAccountProvisioner(aMsgWindow, args) {

(As mentioned before, ignore me if I'm repeating myself) Do we need this function?
Attachment #568012 - Flags: feedback?(bwinton) → feedback+

Comment 7

8 years ago
Quick comment on getanaccount & accountprovisionner. 
As we're going to need more account provisionners in the future to support more messaging sources, I'm wondering if we should not use emailaccountprovisionner instead. My $0.02...
Reporter

Comment 8

8 years ago
Blake, the "general code cleanup" you sold me earlier in August turned out to be a real daunting amount of work...

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #6)
> 
> ::: mail/app/profile/all-thunderbird.js
> @@ -724,3 +724,5 @@
> > >  pref("mail.taskbar.lists.enabled", true);
> > >  pref("mail.taskbar.lists.tasks.enabled", true);
> > >  #endif
> > > +
> > > +// Account provisioner. Commented out prefs are for "the real world".
I've changed everything into newmailaccount.
> 
> @@ -984,4 +985,4 @@
> > >                <menuitem id="newMailAccountMenuItem"
> > >                          label="&newEmailAccountCmd.label;"
> > >                          accesskey="&newEmailAccountCmd.accesskey;"
> > > -                        oncommand="NewMailAccount(msgWindow);"/>
> > > +                        oncommand="NewMailAccountProvisioner(msgWindow);"/>
> 
> Why not just change the NewMailAccount function?  (I couldn't cause I was in
> an addon, but that shouldn't be a limitation you run into.)
You mentioned this several times in the review, replying here.

So I did not change this because I did *not* integrate this into the main account setup wizard. It's still a separate window, with separate code. We mentioned this during a phone meeting, and I stated that doing it the way you mention (i.e. rewriting the account provisioner to be part of the account wizard) would be too much work for me, and that we ought to go the easiest route. That is, keeping the account provisioner as a separate piece of work that operates separately from the mail account wizard.
> 
> 
> ::: mail/components/get-an-account/content/accountProvisioner.js
> @@ +14,5 @@
> > + * The Original Code is Account Provisioner Code.
> > + *
> > + * The Initial Developer of the Original Code is
> > + * The Mozilla Foundation.
> > + * Portions created by the Initial Developer are Copyright (C) 2010
> 
> I think this might be "2011", but maybe not…
The code you wrote was first written in 2010 I guess, which I why I left the original copyright notice.
> 
> @@ +56,5 @@
> > + *
> > + * @param {String} page The page to get the localstorage for.
> > + * @return {nsIDOMStorage} The localstorage for this page.
> > + */
> > +function getLocalStorage(page) {
> 
> I wonder if there's a better place to store these things than localstorage,
> like, say, prefs?
> 
> Actually, since the main reason I needed to do this was because this was a
> whole different page, instead of being part of the normal account wizard, I
> wonder if we need to do it at all?  (You did just merge this feature into
> the regular account wizard, right?)
As I said above, no. I'm not sure the prefs are fit for storing big chunks of data like this, but we could indeed turn this into a JSON blob that's stored in the prefs.
> 
> @@ +85,5 @@
> > +
> > +/**
> > + * Get the default opensearch engine. Stolen from bug 677421.
> > + */
> > +function getDefaultSearchEngine() {
> 
> Shouldn't this be the responsibility of the OpenSearch feature?
I couldn't find any suitable function in the patch that would allow me to get to that information. There's a function in the patch that returns the current engine, but not the default one...
> 
> @@ +116,5 @@
> > + *
> > + * @param provider The provider we created the account for.
> > + * @param config The created config.
> > + */
> > +function logSuccess(provider, config) {
> 
> I think we can do this through Telemetry, or Test Pilot instead, and then
> the Privacy and Security people won't yell at us.  ;)
I've removed all sorts of logging, we can add more in a followup patch if we want.
> 
> @@ +164,5 @@
> > +}
> > +
> > +function splitName(str) {
> > +  let i = str.lastIndexOf(" ");
> > +  if (i >= 0)
> 
> I think this test should be ">= 1", so that we don't try to send an empty
> first name if the user enters a space at the start…
> 
> And, actually, we might want to trim the string before calling this function.
Fixed + fixed at all calling-sites.
> 
> @@ +172,5 @@
> > +}
> > +
> > +$(function() {
> > +  // Snarf the things I need out of the window arguments.
> > +  let NewMailAccount = window.arguments[0].NewMailAccount;
> 
> I was really hoping these lines wouldn't be necessary once we moved this
> into core.  Can't we get the core code to put these variables in the
> newly-opened window?
I'm not sure what you have in mind. There's different ways to achieve this; either grab any mail:3pane and assume that, because the account provisioner will be modal, we can safely hold references to functions defined in that mail:3pane (using getMostRecentWindow).

The other way is more convoluted; because the call to openDialog returns before the dialog actually has done loading, we would need to pass to the dialog a function that the dialog calls when loading. Not sure it's cleaner...
> 
> @@ +203,5 @@
> > +      providers[provider.id] = provider;
> > +      // Update the terms of service and privacy policy links.
> > +      let sep = "";
> > +      if (i == data.length - 1)
> > +        ;
> 
> Is this really an empty statement?  And if so, why not reverse the test in
> the else clause instead?
That seemed clearer to me: last element of the list, no separator; the one before that, use "and"; all the other ones before, use ",". I can smash everything into a less readable version if you prefer, just let me know what suits you best.
> 
> @@ +208,5 @@
> > +      else if (i == data.length - 2)
> > +        sep = stringBundle.get("sepAnd");
> > +      else
> > +        sep = stringBundle.get("sepComma");
> > +      placeholder
> 
> How well does all of this appending work with RTL languages?
I was thinking, since rtl reverses the order of everything, this should be transparent; maybe we could ask ehsan to give this a try, since he's in your office?
> 
> @@ +227,5 @@
> > +  });
> > +  let name = storage.getItem("name") || $("#Name").text();
> > +  let username = storage.getItem("username");
> > +  let domain = storage.getItem("domain");
> > +  $("#Name").val(name);
> 
> I think we should be consistent with our use of the case of ids, and so this
> should be "#name".
Ok, done.
> 
> @@ +240,5 @@
> > +    expandSection(false);
> > +  });
> > +
> > +  $("button.existing").click(function() {
> > +    actionList.push("Using Existing");
> 
> I think we should remove all this actionList stuff, although we may want to
> leave it in for TestPilot purposes later…
Done, too.
> 
> @@ +263,5 @@
> > +    $(this).css({"opacity": "0.0", "display": "inline"});
> > +  });
> > +
> > +  $(window).unload(function() {
> > +    actionList.push("Closing");
> 
> We _really_ want to remove the following two function calls.  :)
Removed too.
> 
> @@ +282,5 @@
> > +  $(window).keydown(function(e) {
> > +    // Handle Cmd-W.
> > +    if (e.keyCode == "224") {
> > +      metaKey = true;
> > +    } else if (e.keyCode == "87" && ((e.ctrlKey && !e.altKey) || metaKey)) {
> 
> This looks like it shouldn't be an "else if", but should instead be an "if".
> 
> Also, I think we want "var metaKey", or "let metaKey", so that we don't
> create a global variable.  (Oh, but I may be wrong about that.  Perhaps we
> want it to be global, cause it needs to be persisted between calls.)
This was code you wrote. It wasn't working anyway, so I rewrote it instead :).
> 
> @@ +296,5 @@
> > +    actionList.push("Searching");
> > +    $("#notifications").children().hide();
> > +    saveState();
> > +    let name = $("#Name").val();
> > +    if (name.length <= 0) {
> 
> I think this might actually be the right place to call trim… (or was that
> strip?)
Done.
> 
> And while we're doing this, we _really_ need to sanitize that input, cause a
> name of "<b>Blake</b> Winton", or "<!--" does not do the right thing at all…
> (Granted, it gives errors instead of running javascript, but still, not
> good.  :)
XXX NOT DONE YET
> 
> @@ +301,5 @@
> > +      $("#Name").select().focus();
> > +      $(".search").removeAttr("disabled");
> > +      return;
> > +    }
> > +    $("#notifications .spinner").show();
> 
> Is this spinner the same spinner that we use elsewhere in Thunderbird, or
> are we going to have more than one style of spinner?
Yeah it's a different spinner, a bigger one. I don't think we can reuse any of the standard Thunderbird spinners here.
> 
> Also, how do I cancel this, if I typoed my name, for instance?
You just wait for the request to complete, input something else, hit search again.
> 
> @@ +305,5 @@
> > +    $("#notifications .spinner").show();
> > +    let [firstname, lastname] = splitName(name);
> > +    $.getJSON(suggestFromName,
> > +              {"first_name": firstname, "last_name": lastname},
> > +              function(data) {
> 
> Don't we need to tell the Mozilla server which providers the user chose to
> ask for suggestions?
That's the way the whole control-flow was designed. That is, you first type in your name, then you obtain suggestions, then you chose which providers you want to get an email from. At least that's how it worked when I took over the code. Are you suggesting I should change the whole thing around?
> 
> @@ +310,5 @@
> > +      let results = $("#results").empty();
> > +      $(".search").removeAttr("disabled");
> > +      let searchingFailed = true;
> > +      let userLanguages = // "fr-FR, fr"; // for testing
> > +        Services.prefs.getCharPref("intl.accept_languages").split(",");
> 
> Should we strip spaces after we split on the ","?
AFAICT, this pref has no spaces in it. Someone more experienced should confirm this, though.
> 
> 
> @@ +359,5 @@
> > +          }
> > +
> > +          results.append(group);
> > +        }
> > +        if (foundUserLang == 0) {
> 
> Okay, so it looks like you're getting suggestions for all the providers, and
> then filtering out the ones you show.  I think we actually want to show the
> list of providers before we get suggestions, so that people can opt out if
> they don't trust, say, Google, and so that we don't ask for Russian email
> addresses for people living in the US who won't want them.
So that matches what you said earlier, and explains why you wrote that. While it makes sense for the account provisioner to behave the way you describe, this implies:
- several changes in terms of UI,
- changes in the way the API works, e.g. make sure suggestFromName takes a list of providers.

This looks feasible, but I'm surprised that such fundamental changes are discussed two weeks before the expected deadline. I'm not sure I have the time to rewrite all that...
> 
> @@ +403,5 @@
> > +    // And add the extra data.
> > +    let data = storedData[provider.id];
> > +    delete data.provider;
> > +    for (let name in data) {
> > +      url += (url.indexOf("?") == -1 ? "?" : "&") +
> 
> I think it would be better to store the separator in a variable, instead of
> scanning through the url all the time.
I'm not sure I understand what you mean. I can turn this into:

for each (let [i, name] in Iterator(data)) {
  url += (i == 0 ? "?" : "&") +
...

is that what you want?
> 
> @@ +440,5 @@
> > +  $("#results").delegate("div.more, div.address", "click", function() {
> > +    let self = $(this);
> > +
> > +    // Return if we're already expanded
> > +    if (self.parent().parent().hasClass("expanded"))
> 
> This navigation seems a little fragile.  Could we use
> self.closest(".something") instead?
Sure. I've changed the navigation to be a little more robust.
> 
> @@ +454,5 @@
> > +
> > +    // And show this box.
> > +    self.parent().parent().find(".more").hide();
> > +    self.parent().siblings(".extra").slideDown();
> > +    self.parent().parent().children().find(".pricing").fadeIn("fast");
> 
> Isn't this the same as self.parent().siblings().find(".pricing")?
No, because siblings doesn't include the element itself.
> 
> @@ +490,5 @@
> > +      $("#successful_account").show();
> > +    });
> > +
> > +    let isChecked = (getCurrentSearchEngine() == getDefaultSearchEngine())
> > +      || (getCurrentSearchEngine() == engine);
> 
> So, if the user previously chose the provider's search engine as their
> default, and then decided to un-check this box, what would you switch it to?
> 
> (The code looks like you wouldn't switch it, but that seems confusing to me.
> On the other hand, not checking the box is also confusing.  I think we
> should perhaps not bother showing this page if the user has already selected
> the provider's search engine as their search engine.)
Yeah, I've decided to not show the page if the search engine is already the one we want.
> 
> ::: mail/components/get-an-account/content/accountProvisioner.xhtml
> @@ +75,5 @@
> > +  <script type="text/javascript;version=1.8"
> > +          src="chrome://communicator/content/utilityOverlay.js">
> > +  </script>
> > +  <script type="text/javascript;version=1.8"
> > +          src="chrome://messenger/content/getanaccount/jquery.dump.js">
> 
> Do we still need this?
Removed.
> 
> @@ +112,5 @@
> > +             src="chrome://messenger/content/getanaccount/spinner.gif"/>
> > +
> > +        <div class="error">
> > +          <p>&error.line1;</p>
> > +          <p>&error.line2;</p>
> 
> Why not just have a <br> in the error?
The <p>s have some padding around which make the message more readable. Also, I think that's how it was designed in the first place.
> 
> @@ +207,5 @@
> > +  </div>
> > +
> > +</body>
> > +
> > +  <!-- Templates!  Woo! -->
> 
> Since these are so simple, I wonder if it would make sense to just create
> the elements inline instead, and remove the dependance on jquery-tmpl.
More code, more work, more bugs, two weeks away from the merge. Your call.
> 
> ::: mail/components/get-an-account/content/jquery.dump.js
> @@ +1,1 @@
> > +/**
> 
> I bet we don't need this file anymore.  Hopefully.  :)
Yeah, removed.
> 
> ::: mail/components/get-an-account/content/uriListener.js
> @@ +57,5 @@
> > +Services.scriptloader.loadSubScript("chrome://messenger/content/accountcreation/emailWizard.js", accountCreationFuncs);
> > +Services.scriptloader.loadSubScript("chrome://messenger/content/accountcreation/sanitizeDatatypes.js", accountCreationFuncs);
> > +Services.scriptloader.loadSubScript("chrome://messenger/content/accountcreation/fetchhttp.js", accountCreationFuncs);
> > +Services.scriptloader.loadSubScript("chrome://messenger/content/accountcreation/readFromXML.js", accountCreationFuncs);
> > +Services.scriptloader.loadSubScript("chrome://messenger/content/accountcreation/guessConfig.js", accountCreationFuncs);
> 
> You really need guessConfig?!?
It really wasn't clear which files I needed, so I decided to import them all just to be on the safe side. Thsi file indeed isn't needed. Any other files you think I could remove? createAccountInBackend really did have a lot of dependencies...
> 
> @@ +117,5 @@
> > +            success: true,
> > +            search_engine: this.params.searchEngine,
> > +          });
> > +        } catch (e) {
> > +          dump(e+"\n");
> 
> console.log is the new dump.  ;)
I used Components.utils.reportError. I'm not sure how you want me to use console.log; on my system, calling console.log("foo") doesn't seem to perform anything visible.
> 
> ::: mail/components/get-an-account/jar.mn
> @@ +1,2 @@
> > +messenger.jar:
> > +    content/messenger/getanaccount/accountProvisioner.xhtml   (content/accountProvisioner.xhtml)
> 
> Blah blah blah "getanaccount should be better named…"  ;)
Done.
> 
> @@ +8,5 @@
> > +    content/messenger/getanaccount/accountCreation.css        (skin/accountCreation.css)
> > +    content/messenger/getanaccount/accountProvisioner.css     (skin/accountProvisioner.css)
> > +    content/messenger/getanaccount/cvv2.png                   (skin/cvv2.png)
> > +    content/messenger/getanaccount/icon64.png                 (skin/icon64.png)
> > +    content/messenger/getanaccount/icon.png                   (skin/icon.png)
> 
> Do we really need icon64.png and icon.png?
> 
> Also, since we've removed the cvv2 validation stuff, I'm fairly sure we
> don't need that png.
The three files have been removed.
> 
> @@ +11,5 @@
> > +    content/messenger/getanaccount/icon64.png                 (skin/icon64.png)
> > +    content/messenger/getanaccount/icon.png                   (skin/icon.png)
> > +    content/messenger/getanaccount/overlay.css                (skin/overlay.css)
> > +    content/messenger/getanaccount/search.gif                 (skin/search.gif)
> > +    content/messenger/getanaccount/spinner.gif                (skin/spinner.gif)
> 
> Isn't there another spinner we can use, instead of including this one?
It has a very specific size, and I'm not really sure we can use another one, without breaking Andy's intent. However, squib might just be able to reuse that one when he lands mail summary.
> 
> ::: mail/components/get-an-account/skin/accountCreation.css
> @@ +1,1 @@
> > +#autoconfigWizard * {
> 
> I don't think we use this file, because it was designed to re-style the base
> account wizard, but we can just make our changes in it.
Removed.
> 
> @@ +11,5 @@
> > +}
> > +
> > +#autoconfigWizard,
> > +#mastervbox {
> > +  background-image: url('chrome://messenger/content/getanaccount/bg.png');
> 
> We don't seem to have this image in the patch…
File removed.
> 
> ::: mail/components/get-an-account/skin/accountProvisioner.css
> @@ +54,5 @@
> > +
> > +input[type="submit"],
> > +button {
> > +  -moz-box-shadow: 0 0 0 1px rgba(255, 255, 255, 0.75) inset;
> > +  background-image: -moz-linear-gradient(center top , #EEEEEE 0%, #EEEEEE 50%, #E1E1E1 50%, #E1E1E1 100%);
> 
> I suspect we want to split this file up into Mac, Linux, and Windows (and
> possible Windows Aero) versions, because while the colours work decently
> enough on Mac and Windows, they really don't mesh well with the Linux themes
> I've seen.
> 
> I also think they'll go poorly with the Windows High-Contrast stuff,
> although I haven't checked that yet.  Perhaps what we really want to do is
> use system colours in more places?
XXX NOT DONE YET
> 
> ::: mail/components/get-an-account/skin/overlay.css
> @@ +1,1 @@
> > +#getanaccount-toolbar-button
> 
> I don't think we use this file either.
Removed.
> 
> ::: mail/jquery/jar.mn
> @@ -1,1 @@
> >  messenger.jar:
> 
> Does anyone else depend on the previous version of jquery or jquery-ui? 
> Like mail/test/resources/mozmill/mozmill/extension/content/mozmill.html?  Or
> mail/base/content/featureConfigurator.xhtml?  Or Extensions?  I'm not sure
> whether we can just upgrade it without doing a little more research first.
I discussed that with Andrew and the bottom line was: "I think it's allright to upgrade it". Jquery is backwards-compatible, so we shouldn't have too much trouble anyway. Is there anyone specific you want me to bug about this?
> 
> ::: mail/locales/en-US/chrome/messenger/getanaccount/accountProvisioner.dtd
> @@ +1,5 @@
> > +<!ENTITY window.title "Welcome to Thunderbird">
> > +<!ENTITY header.label "Want a new email address for Thunderbird?">
> > +<!ENTITY error.line1 "Sorry we had no luck finding suggestions.">
> > +<!ENTITY error.line2 "You try can search for nicknames or any other term to find more emails">
> > +<!ENTITY error.suggest 'Also you might try one of the <a href="http://www.mozillamessaging.com/en-US/thunderbird/features/email_providers.html" class="external">free email account alternatives</a>.'>
> 
> I'm slightly surprised we can have < and > in these entities…
(it works)
> 
> :::
> mail/locales/en-US/chrome/messenger/getanaccount/accountProvisioner.
> properties
> @@ +1,1 @@
> > +price=$%S a year
> 
> I wonder if we want to put in a note saying that the prices is always in
> USD, so please don't change that "$" to "€", "£", or "¥".
comment added.
> 
> ::: mail/test/mozmill/get-an-account/html/providerList
> @@ +1,5 @@
> > +[{"id": "hover",
> > +  "label": "Hover.com",
> > +  "paid": true,
> > +  "languages" : ["en-US"],
> > +  "api": "https://www.hover.com/tbReg?first={firstname}&last={lastname}&email={email}",
> 
> Um, maybe we want to use fake addreses for these, so that we don't
> potentially hit the network during our tests…
> 
> Fake names would also be a good idea, lest someone thinks that we're
> actually integrating with Hover and AOL…
Done, removed all references to hover and aol in the tests.
> 
> ::: mail/test/mozmill/get-an-account/html/registration.html
> @@ +2,5 @@
> > +<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
> > +<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en">
> > +  <head>
> > +    <meta http-equiv="content-type" content="text/html; charset=UTF-8">
> > +    <title>Blog-o!</title>
> 
> Let's change this title, while we're here…
Done.
> 
> @@ +8,5 @@
> > +  <body>
> > +  
> > +    <div class="title">Local version</div>
> > +    <div class="content">
> > +      <form action="config.xml" method="GET">
> 
> Cute!
> 
> ::: mail/test/mozmill/get-an-account/test-get-an-account.js
> @@ +59,5 @@
> > +
> > +// RELATIVE_ROOT messes with the collector, so we have to bring the path back
> > +// so we get the right path for the resources.
> > +// Note: this one adds to '' as we need to make sure that favicon.ico is in the
> > +// root directory.
> 
> Should we add a fake favicon.ico, too?
I just copy/pasted the code with the comment, I removed the irrelevant part.
> 
> @@ +78,5 @@
> > +  wh.installInto(module);
> > +};
> > +
> > +// We can't use plan_for_new_window because it expects a window type and I have
> > +// no idea what on earth is the windowtype of an HTML window.
> 
> I thought it was "AccountSetup", from:
> +  window.openDialog(
> +    "chrome://messenger/content/getanaccount/accountProvisioner.xhtml",
> +    "AccountSetup",
> +    // disabling modal for the time being, see 688273 REMOVEME
> +    "chrome,titlebar,centerscreen,width=640,height=480",
> +    args);
> 
> If not, can we pass in a window type in that call, and use it here?
No, getMostRecentWindow("AccountSetup") doesn't work. I don't know how to get to the account provisioner window other than using the method I did there. There ought to be a way, but I did spend some looking into it, and I couldn't find anything relevant :(.
> 
> @@ +109,5 @@
> > +  let w = wait_for_provisioner_window();
> > +
> > +  // Fill in some data
> > +  let $ = w.$;
> > +  $("#Name").val("Green Llama");
> 
> I've suggested above a few other tests we should run, like "<!--", or " 
> Blake".
> 
> We're also not testing the failure cases, which is where a lot of the bugs
> with the current account wizard have cropped up.
XXX NOT DONE YET
> 
> @@ +115,5 @@
> > +  mc.waitFor(function () $("#results").children().length > 0);
> > +
> > +  // Click on the first address. The reveals the button with the price.
> > +  $(".address:first").click();
> > +  mc.waitFor(function () $("button.create:visible").length > 0);
> 
> Should we check that the price is what we expect it to be?
XXX NOT DONE YET
> 
> @@ +139,5 @@
> > +  $ = w.$;
> > +  assert_equals(nAccounts(), i + 1);
> > +
> > +  // W00t account created
> > +  dump("\033[01;36m#winning\033[00m\n");
> 
> Please no dump statements…  :)
Removed.
> 
> @@ +140,5 @@
> > +  assert_equals(nAccounts(), i + 1);
> > +
> > +  // W00t account created
> > +  dump("\033[01;36m#winning\033[00m\n");
> > +}
> 
> Should we check that the appropriate things get returned for various locales?
XXX NOT DONE YET
> 
> ::: mailnews/base/prefs/content/AccountManager.js
> @@ -385,4 @@
> >    let msgWindow = Components.classes["@mozilla.org/messenger/services/session;1"]
> >                              .getService(Components.interfaces.nsIMsgMailSession)
> >                              .topmostMsgWindow;
> > -  NewMailAccount(msgWindow);
> 
> So, I used to have to override this function because I was an extension, but
> I don't think that still applies, so we shouldn't need to do this, right?
Explained earlier.
> 
> ::: mailnews/base/prefs/content/AccountManager.xul
> @@ +20,4 @@
> >  <script type="application/javascript" src="chrome://messenger/content/am-prefs.js"/>
> >  <script type="application/javascript" src="chrome://messenger/content/AccountManager.js"/>
> >  <script type="application/javascript" src="chrome://messenger/content/am-help.js"/>
> > +<script type="application/javascript;version=1.8"
> 
> Do we still need this import?  Can't we just include the code here?
Seems to work fine without it.
> 
> ::: mailnews/base/prefs/content/accountUtils.js
> @@ +330,4 @@
> >    setTimeout(msgNewMailAccount, 0, msgWindow, okCallback, extraData);
> >  }
> >  
> > +function NewMailAccountProvisioner(aMsgWindow, args) {
> 
> (As mentioned before, ignore me if I'm repeating myself) Do we need this
> function?
Explained above.
Component: Mail Window Front End → User Interface
Product: Thunderbird → Testopia
Target Milestone: --- → Future
Version: Trunk → unspecified
Reporter

Updated

8 years ago
Component: User Interface → Mail Window Front End
Product: Testopia → Thunderbird
Target Milestone: Future → ---
Reporter

Comment 9

8 years ago
Andy, Andreas, we've added a new page to the account provisioner dialog (see <http://jonathan.protzenko.free.fr/shutter/Welcome%20to%20Thunderbird_020.png>). Is there any chance one of you can create an icon for the dialog in the screenshot? The current icon does not look very professional (I did that).

Thanks,

jonathan
Reporter

Comment 10

8 years ago
Sancus, we're seeing some light at the end of the account provisioner tunnel. Can you tell me whether the official servers are up and running, and if so, what are the URLs that I can use for the providerList API and the suggestFromName API? Right now I'm using Blake's test server, but I think we want to move off that eventually.

Thanks,

jonathan
Reporter

Comment 11

8 years ago
① and ⑤ have been fixed in the github repository. Main items left:
- decide what we want to do with the workflow / language interaction (that's roughly ③),
- add new graphics (that's ⑥),
- improve the test (that's ④)
- determine if we can safely upgrade jquery (that's ②),
- address other review comments.

I'll post updates here.
Comment on attachment 568012 [details] [diff] [review]
Roughly works (see below)

Just a few fly-by comments. I had a look at the file structure and I'm generally happy with that.

I also took a detailed look through the L10n strings, so there's a few comments on those below.

>+    content/messenger/getanaccount/accountCreation.css        (skin/accountCreation.css)
>+    content/messenger/getanaccount/accountProvisioner.css     (skin/accountProvisioner.css)
(and the rest of the skin/ ones)

It surprises me that we're not seemingly allowing any theming here, is that right?
>diff --git a/mail/locales/en-US/chrome/messenger/getanaccount/accountProvisioner.dtd b/mail/locales/en-US/chrome/messenger/getanaccount/accountProvisioner.dtd

>+<!ENTITY window.title "Welcome to Thunderbird">
>+<!ENTITY header.label "Want a new email address for Thunderbird?">

These should both use &brandShortName; instead of Thunderbird. You may need to include brand.dtd in the xul file to make this happen.

>+<!ENTITY error.line1 "Sorry we had no luck finding suggestions.">

The wording here surprises me a bit. I'd have thought it'd be more like "...we were unable to...". I could see L10n having an interesting time interpreting this.

>+<!ENTITY error.suggest 'Also you might try one of the <a href="http://www.mozillamessaging.com/en-US/thunderbird/features/email_providers.html" class="external">free email account alternatives</a>.'>

The normal way of doing this is to provide before, middle and after sections - then you're not relying on L10n to get the html right, and if we change the url we don't have to change it in 50+ places.

See the about dialog for some examples on how we do this:

http://hg.mozilla.org/comm-central/annotate/ba59002a7b9d/mail/locales/en-US/chrome/messenger/aboutDialog.dtd#l34
http://mxr.mozilla.org/comm-central/source/mail/base/content/aboutDialog.xul#101

Also, either that url is out of date or needs to be provided (I do also wonder if we're still intending on hosting that page at the time).

>+<!ENTITY success.title 'Hello <span id="FirstAndLastName"></span>, the following email addresses are available to you:'>

See my comments above, but we can just do this with .start and .end strings (why not <span id="FirstAndLastName"/> ?)


>+<!ENTITY partnership.description "In partnership with several providers, Thunderbird can offer you a new email account. Just fill in your first and last name, or any other words you’d like, in the fields above to get started.">
>+<!ENTITY content.close "I think I’ll configure my account later.">
>+<!ENTITY successful.write.desc "Let your friends and family know about your new address.<br/> That’s why you got this new account, isn’t it?">

Can you use a standard apostrophe in these please? I don't see the need for a special character.

>+++ b/mail/locales/en-US/chrome/messenger/getanaccount/accountProvisioner.properties
>@@ -0,0 +1,9 @@
>+price=$%S a year
>+more=+%S more…

Both of these need a localization note saying what %S is replaced with.

>+sepAnd=\u0020and\u0020

You need to include a localization note here that \u0020 is just a space and should be included before & after.
>+sepComma=,\u0020

Ditto

>+# This better be valid!
>+disclaimer=The search terms used are sent to Mozilla (<a href="http://www.mozilla.org/legal/privacy/thunderbird.html" class="external">Privacy Policy</a>) and to 3rd party email providers <span class="placeholder"></span> to find available email addresses.

You probably don't need that comment now. This also needs redoing in the link style of aboutDialog as mentioned earlier.

>+searchDesc=Use <b>%S</b> as my default search engine

Needs a localisation note about what %S is replaced with (yes, I know its pretty obvious).
Just an additional thought - there appears to be a lack of access keys. Is there a build I can test out somewhere for accessibility?
Posted image Search icon
(In reply to Jonathan Protzenko [:protz] from comment #9)
> Andy, Andreas, we've added a new page to the account provisioner dialog (see
> <http://jonathan.protzenko.free.fr/shutter/Welcome%20to%20Thunderbird_020.
> png>). Is there any chance one of you can create an icon for the dialog in
> the screenshot? The current icon does not look very professional (I did
> that).

Here you go!
Posted image sans sunken-in-look
Just in case the attachment above gives any issues with the sunken-in-effect somehow, here is a plain and flat version.
Thanks for keeping this in a separate module.

A few comments:
1. dashes
mail/locales/en-US/chrome/messenger/getanaccount/ vs. mail/components/get-an-account/

2. window ID:
Patch in emailWizard.js says: "Oddly enough, the same window is re-used"
This is because you re-use the Window ID:

+  window.openDialog(
+    "chrome://messenger/content/getanaccount/accountProvisioner.xhtml",
+    "AccountSetup",

and

     window.openDialog("chrome://messenger/content/accountcreation/emailWizard.xul",
-                      "AccountSetup", "chrome,titlebar,modal,centerscreen",
+                      "AccountSetup", "chrome,titlebar,centerscreen",

Change the first to AccountProvisioning, and remove the

+    window.resizeTo(640, 480);

and this:

+    // disabling modal for the time being, see 688273 REMOVEME
     window.openDialog("chrome://messenger/content/accountcreation/emailWizard.xul",
-                      "AccountSetup", "chrome,titlebar,modal,centerscreen",
+                      "AccountSetup", "chrome,titlebar,centerscreen",

3. Please do not put this before the account setup dialog

Instead, please make a new menu item and separate button in the account management dialog.

 function AutoConfigWizard(okCallback)
 {
-  NewMailAccount(msgWindow, okCallback);
+  NewMailAccountProvisioner(msgWindow, okCallback);
 }
Chatted with bwinton about point 3.

I agree that the wizard that comes up when TB starts and there are no account (first start) should change from the account setup wizard to this new account provisioner.

But please not the menu item and the button in the account manager. Because
1) this looks a bit pushy. "I already have an email address, don't try to sell me a new one all the time, thank you very much".
2) Also, we wanted to make account setup streamlined, as few questions as possible.

bwinton responded: that wouldn't be terrible… Yeah, I could see that…
Concretely:
Configure existing mail account… -> Account creation dialog
Register new mail account… -> Account provisioner
Both in
1. menu File | New | …
2. account manager | [Other actions] button
Reporter

Comment 19

8 years ago
Posted patch Newer version (obsolete) — Splinter Review
Better patch, taking into account part of bwinton's remarks. Thanks to everyone for the very useful review comments.
Attachment #568012 - Attachment is obsolete: true
4. Password in XML

+              accountConfig.incoming.password = (xml..incomingServer..password)[0];

Add this directly to readFromXML()
Okay, I think I've done most of the l10n-affecting cleanup.  There are still some bits that don't work, but I'll be continuing to fix those during this week.  In the meantime, check this out, and let me know what you think…

Thanks,
Blake.
Assignee: jonathan.protzenko → bwinton
Attachment #569437 - Attachment is obsolete: true
Attachment #571543 - Flags: feedback?(mbanner)
Things done:
============
  * Make a new menu item and separate button in the account management dialog.
  * Use "&brandShortName;" instead of "Thunderbird".
  * Change the wording of "Sorry we had no luck finding suggestions."
  * Change error.suggest to use before, middle, and after.
  * Change success.title to use .start and .after.
  * Add notes to newmailaccount/accountProvisioner.properties.
  * Remove the "This better be valid!" comment.
  * Don't hard-code the currency.
  * Add accountConfig.incoming.password directly to readFromXML().
  * Fix the window ID.
  * Fix the URIListener.
  * Save the outgoing password, if there is one.
  * Choose providers before submitting!!!


Things left to do:
==================

possibly-L10n-impacting-things:
  * There appears to be a lack of access keys.
  * RTL fails pretty badly, I think.  :(

potentially-delayable-to-fix-in-aurora-things:
  * We _really_ need to sanitize the input to handle "<b>Blake</b> Winton",
    and "<!--".
  * Clicking on the line between two suggested addresses doesn't expand the list.
  * I suspect we want to split accountProvisioner.css up into Mac, Linux, and
    Windows (and possible Windows Aero) version.
  * Test with the Windows High-Contrast theme.
  * I think what we really want to do is use system colours instead.
  * We're also not testing the failure cases, which is where a lot of the
    bugs with the current account wizard have cropped up.
  * Check that the price is what we expect it to be?
  * Check that the appropriate things get returned for various locales?
Attachment #571543 - Attachment is obsolete: true
Attachment #571543 - Flags: feedback?(mbanner)
Attachment #571965 - Flags: review?(mbanner)
This patch addresses a few things:

1)  The menu item added to mailWindowOverlay.xul had the same ID as the one that sends us to the exiting account dialog.  I updated the ID, and updated the ID in the Mozmill test.  Mozmill tests now pass.

2)  I've made the account provisioner sensitive to the user being offline, or having connection difficulties.  When this happens, we hide the sign-up form and display an error message (the one I created...maybe we can come up with a better one).  The code then polls the JSON file every 5 seconds until it can connect (OR, if we hear the "online" event, if the user has switched from working offline to online).

While doing that, I moved a bit of the code around into an AccountProvisioner object.  This is optional, but I found it helped me keep things organized a little bit better in my head, and helped me from injecting some script globals.  If that's not a cool approach, I can drop it.
Attachment #572075 - Flags: review?(bwinton)
Comment on attachment 572075 [details] [diff] [review]
Subpatch, to be applied to 571965: The next version of the patch.

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

Other than the two nits below (and the rebasing you'll have to do on top of my next patch), I like it.  r=me.

::: mail/base/content/mailWindowOverlay.xul
@@ +982,4 @@
>                           oncommand="gFolderTreeController.newVirtualFolder();"
>                           accesskey="&newVirtualFolderCmd.accesskey;"/>
>                <menuseparator id="newAccountPopupMenuSeparator"/>
> +              <menuitem id="newCreateMailAccountMenuItem"

To follow the label, I think this should be "newCreateEmailAccountMenuItem"…

::: mail/components/newmailaccount/content/accountProvisioner.js
@@ -482,10 +480,10 @@
> > -  $("#success-compose").click(function() {
> > +    AccountProvisioner.tryingToPopulate = true;
> > -    NewComposeMessage(Components.interfaces.nsIMsgCompType.New);
> > +
NaN more ...
> > -  $("#success-addons").click(function() {
> > +    let otherLanguages = $("#otherLanguages");
> > -    openAddonsMgr();
> > +    let inputs = $("#otherLangDesc");
NaN more ...

You'll want to remove this line…  ;)

(git stash ftw!)
Attachment #572075 - Flags: review?(bwinton) → review+
We're getting closer…
Attachment #571965 - Attachment is obsolete: true
Attachment #571965 - Flags: review?(mbanner)
Attachment #572105 - Flags: review?(mbanner)
Comment on attachment 572105 [details] [diff] [review]
The previous patch, but with a working listener.

Of my review comments (comment 16 - 20), point 1 has been adressed, but 2 - 4 are still open.
Esp. point 3 (separate menu items / buttons for creation and setup of existing) is critical for me, and will be important for many users, those same ones who where yelling about the dialog in TB 3.0.
Point 2 and 4 are trivial.
Attachment #572105 - Flags: review-
Comment on attachment 572105 [details] [diff] [review]
The previous patch, but with a working listener.

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

Hey Ben, I thought I did most of what you asked for, so I'm going to point out the relevant bits below.

Bug me Monday morning (my time) on IRC, and we can talk about the things that I missed…

Thanks,
Blake.

::: mail/base/content/mailWindowOverlay.xul
@@ -985,2 +986,6 @@
> > -                        label="&newEmailAccountCmd.label;"
> > +                        label="&newCreateEmailAccountCmd.label;"
> > -                        accesskey="&newEmailAccountCmd.accesskey;"
> > +                        accesskey="&newCreateEmailAccountCmd.accesskey;"
> > +                        oncommand="NewMailAccountProvisioner(msgWindow);"/>
> > +              <menuitem id="newMailAccountMenuItem"
NaN more ...

It really looks like I did point #3…

::: mailnews/base/prefs/content/AccountManager.js
@@ +384,5 @@
>  {
>    let msgWindow = Components.classes["@mozilla.org/messenger/services/session;1"]
>                              .getService(Components.interfaces.nsIMsgMailSession)
>                              .topmostMsgWindow;
> +  NewMailAccountProvisioner(msgWindow);

On further thought, I've reverted this change (to be attached in a following patch), since if someone already has Thunderbird set up, they're unlikely to want to sign up for yet another account.

::: mailnews/base/prefs/content/accountUtils.js
@@ -331,0 +359,34 @@
> > +function NewMailAccountProvisioner(aMsgWindow, args) {
> > +  if (!args)
> > +    args = {};
> > +  if (!aMsgWindow)
NaN more ...

This makes it look like point 2 has been fixed.

::: mailnews/base/prefs/content/accountcreation/readFromXML.js
@@ +92,5 @@
>        iO.username = sanitize.string(iX.username[0]); // may be a %VARIABLE%
> +      if (iX.password) {
> +        d.rememberPassword = true;
> +        iO.password = iX.password[0];
> +      }

I believe this covers point 4.

@@ -213,1 +222,1 @@
> >        try {

(And this is the other part of point 4. ;)
Attachment #572105 - Flags: review- → review?(ben.bucksch)
blake, sorry! I didn't see you mention that you fixed it, so I assumed you didn't.

I just saw that

+    // disabling modal for the time being, see 688273 REMOVEME
     window.openDialog("chrome://messenger/content/accountcreation/emailWizard.xul",
-                      "AccountSetup", "chrome,titlebar,modal,centerscreen",
+                      "AccountSetup", "chrome,titlebar,centerscreen",

is still is there. This should no longer be necessary, now that the windows are separate. I think you simply overlooked it. (And I wrongly interpreted this patch hunk as a hint that my comment 16 ff was ignored, sorry.)

Longer rationale:
This change should be removed in any case, because users may accidentally click outside the dialog in the main window or on the window title bar of the main window, and then the dialog disappears in the background, and then people are left highly confused at best, or at worst with a main window without accounts. I know it did happen to me, several times.
If the non-modal is needed for dev, the devs can remove it. It shouldn't affect users.

       iO.username = sanitize.string(iX.username[0]); // may be a %VARIABLE%
+      if (iX.password) {
+        d.rememberPassword = true;
+        iO.password = iX.password[0];

Please use sanitize.string(iX.password[0]) here, too. (same for outgoing).
Otherwise: Thanks for fixing this.

Most importantly, thanks for separating the new dialog and the account setup dialog in the menu !

However,

+++ b/mailnews/base/prefs/content/AccountManager.js

 function AddMailAccount()
 {
   let msgWindow = Components.classes["@mozilla.org/messenger/services/session;1"]
                             .getService(Components.interfaces.nsIMsgMailSession)
                             .topmostMsgWindow;
-  NewMailAccount(msgWindow);
+  NewMailAccountProvisioner(msgWindow);
 }
 
I think the account manager button still starts the provisioner instead of the account setup, it's not separated into 2 buttons yet. Can you add the separation there, too, please?
 
 function AutoConfigWizard(okCallback)
 {
-  NewMailAccount(msgWindow, okCallback);
+  NewMailAccountProvisioner(msgWindow, { okCallback: okCallback });
 }

Is this the function that's called when we start up without any accounts? (If so, that's OK.) Can you confirm and add a javadoc comment to that effect?

I have no clue why the Composer and other stuff is passed as "extraData", but I'll ignore it, as it affects only the provisioner.

Updated

8 years ago
Attachment #572105 - Flags: review?(ben.bucksch) → review-
(In reply to Ben Bucksch (:BenB) from comment #28)
> blake, sorry! I didn't see you mention that you fixed it, so I assumed you
> didn't.

Heh.  :)

> I just saw that
> 
> +    // disabling modal for the time being, see 688273 REMOVEME
>     
> window.openDialog("chrome://messenger/content/accountcreation/emailWizard.
> xul",
> -                      "AccountSetup", "chrome,titlebar,modal,centerscreen",
> +                      "AccountSetup", "chrome,titlebar,centerscreen",
> 
> is still is there. This should no longer be necessary, now that the windows
> are separate. I think you simply overlooked it. (And I wrongly interpreted
> this patch hunk as a hint that my comment 16 ff was ignored, sorry.)

Strangely enough, it's still necessary, since without it, if you switch between the two account setup screens a few times, then close the Provisioner, the Wizard will pop up modally a couple of times, and require you to manually close it.

In theory I could remove it from both, but I can't do that because bug 688273 is still in effect.  :(

>        iO.username = sanitize.string(iX.username[0]); // may be a %VARIABLE%
> +      if (iX.password) {
> +        d.rememberPassword = true;
> +        iO.password = iX.password[0];
> 
> Please use sanitize.string(iX.password[0]) here, too. (same for outgoing).

I was wondering about that.  I _don't_ want %VARIABLE%s to be replaced in the passwords.  Does sanitize.string do that?

> However,
> 
> +++ b/mailnews/base/prefs/content/AccountManager.js
>  
> I think the account manager button still starts the provisioner instead of
> the account setup, it's not separated into 2 buttons yet. Can you add the
> separation there, too, please?

To quote me:
>::: mailnews/base/prefs/content/AccountManager.js
>@@ +384,5 @@
>>  {
>>    let msgWindow = Components.classes["@mozilla.org/messenger/services/session;1"]
>>                              .getService(Components.interfaces.nsIMsgMailSession)
>>                              .topmostMsgWindow;
>> +  NewMailAccountProvisioner(msgWindow);
>
> On further thought, I've reverted this change (to be attached in a following
> patch), since if someone already has Thunderbird set up, they're unlikely to want
> to sign up for yet another account.

Which is even more than you asked for, I believe.  :)

>  function AutoConfigWizard(okCallback)
>  {
> -  NewMailAccount(msgWindow, okCallback);
> +  NewMailAccountProvisioner(msgWindow, { okCallback: okCallback });
>  }
> 
> Is this the function that's called when we start up without any accounts?
> (If so, that's OK.) Can you confirm and add a javadoc comment to that effect?

Yes, and that sounds reasonable.

> I have no clue why the Composer and other stuff is passed as "extraData",
> but I'll ignore it, as it affects only the provisioner.

For the last screen of the Provisioner, where we offer to let you send email, and other stuff.  Once we move the non-3-pane functions out of the 3-pane, I think we can stop passing this, and get it from a more global object.

Thanks,
Blake.
> if you switch between the two account setup screens a few times, then close
> the Provisioner, the Wizard will pop up modally a couple of times, and require
> you to manually close it.

That sounds like something is wrong in the switch between the screens.

> I can't do that because bug 688273 is still in effect.  

As I said, that only affects development, so that shouldn't change things for users. devs can still change it locally.

(Please note that it wasn't an unsolved problem during the dev of the account creation wizard, maybe because we catch exceptions and log them using log4moz and consoles.)

> I was wondering about that.  I _don't_ want %VARIABLE%s to be replaced in the
> passwords.  Does sanitize.string do that?

Urgs, forbid, no! Sanitize is a security feature, and only that. It merely ensures that you get the variable type and content that you expect.

%VARIABLE% replacement happens much later, by an explicit call to replaceVariables() in emailWizard.js.

> > I've reverted this change
> Which is even more than you asked for, I believe.

Ah, OK, I didn't understand what was "reverted". Fine with me, as you wish.

> For the last screen of the Provisioner, where we offer to let you send email,
> and other stuff.

That sounds strange. Oh, well, not my business.
(In reply to Ben Bucksch (:BenB) from comment #30)
> > if you switch between the two account setup screens a few times, then close
> > the Provisioner, the Wizard will pop up modally a couple of times, and require
> > you to manually close it.
> That sounds like something is wrong in the switch between the screens.

Patches welcome!

> > I was wondering about that.  I _don't_ want %VARIABLE%s to be replaced in the
> > passwords.  Does sanitize.string do that?
> Urgs, forbid, no! Sanitize is a security feature, and only that. It merely
> ensures that you get the variable type and content that you expect.
> 
> %VARIABLE% replacement happens much later, by an explicit call to
> replaceVariables() in emailWizard.js.

Sounds like the comment above is wrong, then.
> iO.username = sanitize.string(iX.username[0]); // may be a %VARIABLE%

Thanks,
Blake.
> Sounds like the comment above is wrong, then.

Ah, I see. No, I think you just misunderstood. It says:

> iO.username = sanitize.string(iX.username[0]); // may be a %VARIABLE%

That means the username may contain "%". It doesn't mean sanitize.string() replaces the variable.

sanitize.* is really just to implement the "check all input" paradigm
(see docs in sanitzeDatatypes.js).
Posted patch Combined patch (obsolete) — Splinter Review
I've combined Blake's latest work with my patch.  I think I'm taking over this one now.

I've added a new string, to be displayed if TB is disconnected, or if the provider server is down:

"Sorry - we're unable to communicate with our sign-up server. Please check your connection."

Can we do any better?

I know we want to get this in tomorrow, and get the strings done, so it got me thinking - should we have a countdown for retry, and a link to auto-retry, a la Google?  If so, should we have some strings for that?
Assignee: bwinton → mconley
Attachment #572075 - Attachment is obsolete: true
Attachment #572105 - Attachment is obsolete: true
Attachment #572105 - Flags: review?(mbanner)
Attachment #572599 - Flags: review?(mbanner)
Posted patch Patch v2 (obsolete) — Splinter Review
Fix-ups from out-of-band review by Standard8 (http://pastebin.mozilla.org/1376774)
Attachment #572599 - Attachment is obsolete: true
Attachment #572599 - Flags: review?(mbanner)
Attachment #572613 - Flags: review?(mbanner)
Comment on attachment 572613 [details] [diff] [review]
Patch v2

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

::: mail/components/newmailaccount/content/accountProvisioner.js
@@ +106,5 @@
> + * Get the current opensearch engine. Stolen from bug 677421.
> + */
> +function getCurrentSearchEngine() {
> +  try {
> +    return Services.prefs.getCharPref("mail.websearch.engine");

This should be using Services.search.currentEngine, however if that doesn't work straight off, use browser.search.selectedEngine for now and file a follow-up for aurora.

@@ +517,5 @@
> +    let engine = window.arguments[0].search_engine;
> +    $("#window").hide();
> +    $("#search_engine_next").click(function () {
> +      if ($("#search_engine_check").prop("checked")) {
> +        Services.prefs.setCharPref("mail.websearch.engine", engine);

This should be using Services.search.currentEngine, however if that doesn't work straight off, use browser.search.selectedEngine for now and file a follow-up for aurora.

::: mail/components/newmailaccount/content/accountProvisioner.xhtml
@@ +116,5 @@
> +        <div class="error">
> +          <p>&error.line1;</p>
> +          <p>&error.line2;</p>
> +          &error.suggest.before;<a
> +            href="http://www.mozillamessaging.com/en-US/thunderbird/features/email_providers.html"

This should be mozilla.org

::: mail/components/newmailaccount/content/uriListener.js
@@ +171,5 @@
> +                              /* in unsigned long */ aState) {
> +  },
> +
> +  QueryInterface: XPCOMUtils.generateQI(
> +    [Ci.nsISupports, Ci.nsISupportsWeakReference, Ci.nsIWebProgressListener]),

nit: generateQI already adds nsISupports.
Posted patch Patch v3 (obsolete) — Splinter Review
(In reply to Mark Banner (:standard8) from comment #35)
> ::: mail/components/newmailaccount/content/accountProvisioner.js
> @@ +106,5 @@
> > +    return Services.prefs.getCharPref("mail.websearch.engine");
> 
> This should be using Services.search.currentEngine, however if that doesn't
> work straight off, use browser.search.selectedEngine for now and file a
> follow-up for aurora.

Changed.

> @@ +517,5 @@
> > +        Services.prefs.setCharPref("mail.websearch.engine", engine);
> This should be using Services.search.currentEngine, however if that doesn't
> work straight off, use browser.search.selectedEngine for now and file a
> follow-up for aurora.

Changed.

> ::: mail/components/newmailaccount/content/accountProvisioner.xhtml
> @@ +116,5 @@
> > +        <div class="error">
> > +          <p>&error.line1;</p>
> > +          <p>&error.line2;</p>
> > +          &error.suggest.before;<a
> > +            href="http://www.mozillamessaging.com/en-US/thunderbird/features/email_providers.html"
> This should be mozilla.org

Fixed.

> ::: mail/components/newmailaccount/content/uriListener.js
> > +  QueryInterface: XPCOMUtils.generateQI(
> > +    [Ci.nsISupports, Ci.nsISupportsWeakReference, Ci.nsIWebProgressListener]),
> nit: generateQI already adds nsISupports.

Fixed, in two places.

I've also added the images that were missing.  (But the patch doesn't seem to work for me, so I'm mostly posting it as a step forward for mconley.)

Later,
Blake.
Attachment #572613 - Attachment is obsolete: true
Attachment #572613 - Flags: review?(mbanner)
Attachment #572684 - Flags: review?(mbanner)
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #36)
> (In reply to Mark Banner (:standard8) from comment #35)
> > ::: mail/components/newmailaccount/content/accountProvisioner.js
> > @@ +106,5 @@
> > > +    return Services.prefs.getCharPref("mail.websearch.engine");
> > 
> > This should be using Services.search.currentEngine, however if that doesn't
> > work straight off, use browser.search.selectedEngine for now and file a
> > follow-up for aurora.
> 
> Changed.
> 
> > @@ +517,5 @@
> > > +        Services.prefs.setCharPref("mail.websearch.engine", engine);
> > This should be using Services.search.currentEngine, however if that doesn't
> > work straight off, use browser.search.selectedEngine for now and file a
> > follow-up for aurora.
> 
> Changed.

D'oh, I forgot to re-generate the patch, so these two aren't actually done.  :(
Posted patch Patch v4Splinter Review
Okay, I think this is everything mentioned fixed.
Attachment #572684 - Attachment is obsolete: true
Attachment #572684 - Flags: review?(mbanner)
Attachment #572708 - Flags: review?(mbanner)
Attachment #572708 - Flags: review?(mbanner) → review+
Assignee

Updated

8 years ago
Depends on: 700535
Assignee

Updated

8 years ago
Depends on: 700536
Ack, backed out (http://hg.mozilla.org/comm-central/rev/453c7e5b617d).  Tree went totally red on me, and I didn't know what to do.

Here's one of the error logs:  http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTrunk/1320721447.1320721905.26783.gz

What did we forget to do?
Hrm - I was able to do a local clobber build with this patch without a problem.  Could that be it?  Maybe the build machines just need a clobber?
(In reply to Mike Conley (:mconley) from comment #41)
> Hrm - I was able to do a local clobber build with this patch without a
> problem.  Could that be it?  Maybe the build machines just need a clobber?

Yep, they do. I'll fix and re-land in a bit.
Was relanded as http://hg.mozilla.org/comm-central/rev/0eb34d6ba3f3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Assignee

Updated

8 years ago
Depends on: 700657
Assignee

Updated

8 years ago
Depends on: 700650
Target Milestone: --- → Thunderbird 10.0

Comment 44

8 years ago
# LOCALIZATION NOTE (more):
#   %S will be the number of email addresses minus two.  (So, we'll show
#   two, and say that there are "(n-2) more…"
more=+%S more…

Would it be possible to make this line support plural forms?

Also, the word "Free" in the table header for e.g. AOL, is not localizable. Please make it such.
Target Milestone: Thunderbird 10.0 → ---
(In reply to Rimas Kudelis from comment #44)
> # LOCALIZATION NOTE (more):
> #   %S will be the number of email addresses minus two.  (So, we'll show
> #   two, and say that there are "(n-2) more…"
> more=+%S more…
> 
> Would it be possible to make this line support plural forms?

Damn, sorry totally missed that in review.

> Also, the word "Free" in the table header for e.g. AOL, is not localizable.
> Please make it such.

Yes we missed that, and that's quite bad.

I realise we don't normally take string changes on aurora, but given the minimal amount of time since we merged, would you think it reasonable that we land a string for "Free" on central *and* aurora, and then do just the plural form on central?

In any case we will fix both of these on central - I'll get Mike to file a couple of follow-up bugs.
Target Milestone: --- → Thunderbird 10.0
Assignee

Updated

8 years ago
Depends on: 701016

Comment 46

8 years ago
(In reply to Mark Banner (:standard8) from comment #45)
> (In reply to Rimas Kudelis from comment #44)
> > # LOCALIZATION NOTE (more):
> > #   %S will be the number of email addresses minus two.  (So, we'll show
> > #   two, and say that there are "(n-2) more…"
> > more=+%S more…
> > 
> > Would it be possible to make this line support plural forms?
> 
> Damn, sorry totally missed that in review.

Well, thanks in advance for fixing in Central! ;)

> > Also, the word "Free" in the table header for e.g. AOL, is not localizable.
> > Please make it such.
> 
> Yes we missed that, and that's quite bad.
> 
> I realise we don't normally take string changes on aurora, but given the
> minimal amount of time since we merged, would you think it reasonable that
> we land a string for "Free" on central *and* aurora, and then do just the
> plural form on central?

I'm always in for that, not really sure about others. But you'll at least have to warn the newsgroup.
Whiteboard: [secr:dveditz][secr:dchan]
Assignee

Updated

8 years ago
Depends on: 704868

Updated

8 years ago
Depends on: 713407
Comment on attachment 572708 [details] [diff] [review]
Patch v4

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

::: mail/components/newmailaccount/content/accountProvisioner.js
@@ +75,5 @@
> +    .getService(Ci.nsIDOMStorageManager);
> +
> +  var uri = Services.io.newURI(url, "", null);
> +  var principal = ssm.getCodebasePrincipal(uri);
> +  return dsm.getLocalStorageForPrincipal(principal, url);

This doesn't seem right to me. The principal is always http://example.com . The site http://example.com would be able to read data stored in localstorage due to same-origin-policy.
Reporter

Comment 48

8 years ago
Hi,

So I'm a little bit surprised by that comment. example.com is not a real domain name; it's owned by IANA and points to their website; I'm pretty sure the RFCs say that no real website will ever live there. I don't think there's a real security threat here, but I'm not expert. Could you elaborate what you had in mind? Maybe if someone spoofs the DNS entries, there's a risk?

If the threat is mild, I'd be in favor of implementing a better solution in comm-central, and live with the current implementation rather than land a big change at such a late stage. I have plenty of ideas for workarounds which I would be happy to bring forward, should it turn out that there's a real security threat here :).

Thanks,

jonathan
If this is just about storing, and needing a key to the storage, I would suggest "http://accountprovisioner.thunderbird.invalid" as hostname.
The risk of this threat is really low in my opinion. It is not a blocker for landing. In order for an attacker to exploit this, the following would need to be true

- user is using Thunderbird in a browsing context
- malicious attacker is on the same network
- attacker is running an active man-in-the-middle attack
- attacker adds content to page to reference example.com
- attacker replaces IANA's 302 for example.com with content that reads localstorage and sends the data


I would suggest changing the scheme to one that can't be influence by the web, file:, chrome:
(In reply to David Chan [:dchan] from comment #50)
> The risk of this threat is really low in my opinion. It is not a blocker for
> landing. In order for an attacker to exploit this, the following would need
> to be true
> 
> - user is using Thunderbird in a browsing context
> - malicious attacker is on the same network
> - attacker is running an active man-in-the-middle attack
> - attacker adds content to page to reference example.com
> - attacker replaces IANA's 302 for example.com with content that reads
> localstorage and sends the data
> 
> 
> I would suggest changing the scheme to one that can't be influence by the
> web, file:, chrome:

Thanks David - done:  http://mxr.mozilla.org/comm-central/source/mail/components/newmailaccount/content/accountProvisioner.js#75
Security review was completed and concerns were addressed.
Depends on: 748830
Flags: sec-review+
You need to log in before you can comment on or make changes to this bug.