549045 - Fix Account Wizard UI

Status: VERIFIED FIXED

(Thunderbird :: Account Manager, defect, P2)

Description

[Ben Bucksch (:BenB)]

The Account Wizard, the part with the hostnames, is not working terribly well. 

Esp:
1. when you need to manually alter the hostnames or SSL config.
1.1. While the config search is going, there is little hint that you can provide the servers yourself. There's a "Manual Config" button, but if you click on it, it randomly creates either a POP or IMAP config, with random hostname, depending on what it was checking at this moment. The results are unchangable (can't change IMAP/POP, and the initially filled out hostname will stay as directory name even if you change it). Bad. (In general, that "Manual Config" button doesn't work very well, in various situations. It simply had not received much coding and testing attention.)
1.2. You click "Stop", but it doesn't stop right away, just fails much faster.
1.3. When you select POP/SSL, the (default) port does not change. It should work roughly like in the Account Manager: Change it if it was the default port, leave unaltered if the user had changed it.
1.4. When you enter hostname, port, protocol and SSL, the wizard ignores and overwrites (!) port, SSL and (I think) protocol and goes on searching. User should have ability to manually specify it and have that used! So, the dropdowns should have an "Auto-detect" entry, which is selected by default when the user changes the hostname. If that's selected, we start probing (on the provided hostname only). If the user specifically selects SSL or a non-standard port, we use that and don't probe. (We'll still verify it, when the user clicks "Create a account".)
1.5 "Retest config" button is above the fields, should be below (time flow should follow read direction: left to right, top to bottom).

Esp. 1.3 and 1.4 are a big annoyance.

2. To simplify the UI, and for bug 549040 (heuristic running in full parallel), I'll probably remove the hostname updates in the fields. Bug 549040 would mean that it's 1) no longer possible to show which hostname we currently try, because we try them all at once and 2) it'll be much faster.
I lump this into the same bug, because it's the same code and fixing 1. without 2. will be much more (and useless) work.

Comment 1

[Ben Bucksch (:BenB)]

Bryan, can you pre-approve the ideas here, before I provide a patch? You'll get another pass when I have a patch and screenshots, I just don't want to waste time :).

Comment 2

[Mark Banner (:standard8)]

> 1.4. When you enter hostname, port, protocol and SSL, the wizard ignores and
> overwrites (!) port, SSL and (I think) protocol and goes on searching. User
> should have ability to manually specify it and have that used!

The outgoing protocol is probably bug 548491 - the code is broken which would get it to not ignore the selection from the user.

Comment 3

[Ben Bucksch (:BenB)]

I'm seeing that for incoming, too, IIRC.

Comment 4

[Jim Porter (:squib)]

I think 1.2 is bug 533680 (see comment 4 on that page).

Comment 5

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #0)
> The Account Wizard, the part with the hostnames, is not working terribly well. 
> 
> Esp:
> 1. when you need to manually alter the hostnames or SSL config.
> 1.1. While the config search is going, there is little hint that you can
> provide the servers yourself. There's a "Manual Config" button, but if you
> click on it, it randomly creates either a POP or IMAP config, with random
> hostname, depending on what it was checking at this moment. The results are
> unchangable (can't change IMAP/POP, and the initially filled out hostname will
> stay as directory name even if you change it). Bad. (In general, that "Manual
> Config" button doesn't work very well, in various situations. It simply had not
> received much coding and testing attention.)

Part of this feels like what we were addressing in bug 532590 comment 7 in terms of the POP or IMAP choice.

> 1.2. You click "Stop", but it doesn't stop right away, just fails much faster.
> 1.3. When you select POP/SSL, the (default) port does not change. It should
> work roughly like in the Account Manager: Change it if it was the default port,
> leave unaltered if the user had changed it.
> 1.4. When you enter hostname, port, protocol and SSL, the wizard ignores and
> overwrites (!) port, SSL and (I think) protocol and goes on searching. User
> should have ability to manually specify it and have that used! So, the
> dropdowns should have an "Auto-detect" entry, which is selected by default when
> the user changes the hostname. If that's selected, we start probing (on the
> provided hostname only). If the user specifically selects SSL or a non-standard
> port, we use that and don't probe. (We'll still verify it, when the user clicks
> "Create a account".)
> 1.5 "Retest config" button is above the fields, should be below (time flow
> should follow read direction: left to right, top to bottom).
> 
> Esp. 1.3 and 1.4 are a big annoyance.
> 
> 2. To simplify the UI, and for bug 549040 (heuristic running in full parallel),
> I'll probably remove the hostname updates in the fields. Bug 549040 would mean
> that it's 1) no longer possible to show which hostname we currently try,
> because we try them all at once and 2) it'll be much faster.
> I lump this into the same bug, because it's the same code and fixing 1. without
> 2. will be much more (and useless) work.

We might need to re-layout the information with this change.  A bunch of the other bugs we have like outgoing username were going to force a new layout change anyway.  It might make sense to split things up into larger incoming and outgoing groups.

Comment 6

[Ben Bucksch (:BenB)]

Actually, I had in mind to remove the incoming/outgoing group entirely - as far as progress display is concerned - and only show "checking common server names" or similar.
(Bug 549040 already removes all the server name progress feedback, because it would fire 50 different server names all at once, right in the beginning, and then nothing, so I only show the very first name and then when we succeed with a server.)

Comment 7

[Ben Bucksch (:BenB)]

Attachment: v1, Screenshot, Start

Comment 8

[Ben Bucksch (:BenB)]

Attachment: v1, Screenshot, Detection

Ignore that the buttons are cut off.

The same for guess (just with "Trying common server names"), see initial description for reasons.

Comment 9

[Ben Bucksch (:BenB)]

Attachment: v1, Screenshot, Result

This is how it looks when we found a config - no matter whether from ISP, our DB, or from guessing.

Comment 10

[Ben Bucksch (:BenB)]

Attachment: v1, Screenshot, Result, with IMAP and POP selection

Same as above, just with the ability to select between IMAP and POP, for bug 532590.

I don't like it, because it makes this nice, simple, easy dialog busy and complicated. We wanted to ask for realname, email address and password only :(.

Comment 11

[Ben Bucksch (:BenB)]

Attachment: v1, Screenshot, Manual edit, 2 rows per server

Comment 12

[Ben Bucksch (:BenB)]

Attachment: v1, Screenshot, Manual edit, 1 rows per server

Manual edit

I don't know how to arrange these widgets. This is easier to process for the brain than the version with 2 rows per server, but makes the dialog very wide even in English, and many other languages are longer.

Comment 13

[Ben Bucksch (:BenB)]

Attachment: v1, Screenshot, Manual edit, default state, 1 row per server

This is how it looks when you go in there before we found any settings. Note that SSL, Auth and Port are on "Auto" by default. If you leave it like that and click on "Re-Test Configuration", they will be auto-detected with our probing code (port, SSL and auth can be fairly well autodetected, unlike hostnames and username form). Once we found the settings, the fields will switch to the detected values, and you can click "Create Account". You may also set them all manually by selecting the value from the dropdown.

This part is entirely broken in the current UI: There's no "auto" value in the dropdowns, and even if you explicitly select "SSL" or "No encryption" and (have to) press "Re-Test", we'll probe it and change the value you selected. This is extremely annoying and got me angry, and I read from other users which were highly upset, too. I think that "Auto" as default is a nice solution for that.

Comment 14

[Ben Bucksch (:BenB)]

I talked with Bryan today about this, including showing him my screenshots, and he agreed with it. (Thanks!)

The UI is mostly done, but needs polishing:
* Buttons enable/disable/hidden not correct yet
* Spaces & sizes
* I want to make the status line and maybe even the domain of the
  server hostnames (e.g. the "web.de" part of "imap.web.de") bold.

Comment 15

[Ben Bucksch (:BenB)]

Attachment: Patch, v1, matching state of screenshots, not yet finished

Comment 16

[Ben Bucksch (:BenB)]

https://hg.mozilla.org/users/mozilla.BenB_bucksch.org/tb-autoconfig-ui-1/

Comment 17

[Alex Ihrig [:Thunderbird_Mail_DE]]

Not sure, but maybe we could fix the following misalignment in this bug. I've attached a screenshot.

Comment 18

[Alex Ihrig [:Thunderbird_Mail_DE]]

Attachment: Screenshot showing the misalignment in German builds

Comment 19

[Ben Bucksch (:BenB)]

Comment on attachment 434832 [details]
Screenshot showing the misalignment in German builds

That's surely interesting, sounds like wrong code or skin.
However, I don't plan changing that upper part in this bug, so could you please file a new bug? Thanks.

Comment 20

[Vlado Valastiak [:wladow]]

That's bug 515390

Comment 21

[Ben Bucksch (:BenB)]

Attachment: Patch, v2 - current state

Comment 22

[Ben Bucksch (:BenB)]

Attachment: Patch, v3 - current state

Comment 23

[Dan Mosedale (:dmosedale, :dmose)]

There's no way we can block 3.1 on another patch of this size and scale, I'm sorry.  :-(

Comment 24

[Ben Bucksch (:BenB)]

Attachment: Fix, v5

Blake, take a look at this when you're done with the migration stuff. It's not finished yet and still has bugs, but I think already working better than the old dialog.

Comment 25

[Bryan Clark (DevTools PM) [:clarkbw]]

Ben, if you have a future version of this patch do you think you could add in a fix for bug 520964 (focus name entry on load)

Comment 26

[Ben Bucksch (:BenB)]

Bryan, sure.

Comment 27

[Ben Bucksch (:BenB)]

Attachment: Fix. v6

I fixed all the known state bugs.
This is now ready for review.

TODO:
- SSL cert handling is broken (but changes for that should be confined)
- Maybe change layout of manual edit mode
- Maybe set ports in manual edit mode

Comment 28

[Ben Bucksch (:BenB)]

Attachment: v7, Screenshot, Manual edit, one username, on 3rd row

This is another alternative for the manual edit layout.
I like this one best - it's fairly clear, and not too wide.

It has only one field for the username, but so does the current dialog, so no worse than before. And the user can still do "Advanced setup", and we can easily add the 2. username field later, if needed.

I think this is the best option.

Comment 29

[Ben Bucksch (:BenB)]

Attachment: Without patch, Screenshot, Manual edit

This is how the dialog looks currently on trunk without my patch.
Just so that you're not confused by the Linux look :).

Comment 30

[Ben Bucksch (:BenB)]

Attachment: Fix, v7

Comment 31

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 437459 [details] [diff] [review]
Fix, v7

>+++ b/mail/locales/en-US/chrome/messenger/accountCreation.dtd
>@@ -5,49 +5,61 @@
> <!ENTITY email.label                     "Email address:">
> <!ENTITY password.label                  "Password:">
>-<!ENTITY username2.label                 "Username:">
>-<!ENTITY incoming.label                  "Incoming:">
>-<!ENTITY outgoing.label                  "Outgoing:">
>+<!ENTITY imapLong.label                  "IMAP (remote folders)">
>+<!ENTITY pop3Long.label                  "POP3 (keep mail on your computer)">
>+
>+<!ENTITY incoming.label                  "Incoming">
>+<!ENTITY outgoing.label                  "Outgoing">
>+<!ENTITY hostname.label                  "Server hostname">
>+<!ENTITY username.label                  "Username">
>+<!ENTITY port.label                      "Port">

If Email and Password have colons in them, why don't Incoming, Outgoing, and Username?

>+<!ENTITY sslAndAuth.label                "SSL and Authentication">
>+<!ENTITY ssl.label                       "SSL">
>+<!ENTITY auth.label                      "Authentication">
>+<!ENTITY autoPort.label                  "Auto">
>+<!ENTITY imap.label                      "IMAP">
>+<!ENTITY pop3.label                       "POP3">

This is mis-aligned.

>-<!ENTITY stop.label                      "Stop">
>+<!ENTITY stop.label                      "Stop detection">

I wonder if this makes the button too large/too obvious.

>-<!-- LOCALIZATION NOTE (retest.label): This is the text that is
>-     displayed on the button which will re-guess the account configuration,
>-     taking into account the settings the user has changed. -->
>-<!ENTITY retest.label                    "Re-test Configuration">
>-<!ENTITY retest.accesskey                "R">
>-<!ENTITY edit.label                      "Edit">
>-<!ENTITY edit.accesskey                  "E">
>+<!-- LOCALIZATION NOTE (half-manual-test.label): This is the text that is
>+     displayed on the button in manual edit mode which will re-guess
>+     the account configuration, taking into account the settings that
>+     the user has manually changed. -->
>+<!ENTITY half-manual-test.label          "Re-test">
>+<!ENTITY half-manual-test.accesskey      "t">
>+<!ENTITY manual-edit.label               "Manual config">

If we're renaming it to "Advanced Setup" above, shouldn't this be "Advanced config"?  Or, actually "Advanced Config", since we seem to be capitalizing both words.

>+++ b/mail/locales/en-US/chrome/messenger/accountCreation.properties
>@@ -14,22 +14,24 @@ default_server_tag= (default)
> # LOCALIZATION NOTE(found_settings_disk): Referring to Thunderbird installation folder on user's harddisk. %1$S will be the brandShortName.
>+looking_up_settings_halfmanual=Looking up configuration: Probing server
> found_settings_disk=The following settings were found on: %1$S installation

I think the localization note should be immediately above the thing it's about, not separated by another property.  ;)

> found_settings_guess=The following settings were found by trying common server names
>+found_settings_halfmanual=The following settings were found by probing the server

I think we can come up with better wording here, perhaps with Bryan's input.

>@@ -50,15 +52,35 @@ password_ok=Password ok!
>+# LOCALIZATION NOTE(resultNoEncryption): Neither SSL/TLS nor STARTTLS. Transmission of emails in cleartext over the Internet.
>+resultNoEncryption=No Encryption
>+resultSSL=SSL

Should this be "SSL/TLS", or should the note above just be "SSL"?

>+resultSTARTTLS=STARTTLS
>+resultSSLCertWeak=_(Warning: Could not verify server)
>+resultSSLCertOK=

These two look kinda odd to me.  Can you explain what the _(…) and the empty property are about?

>+# LOCALIZATION NOTE(resultIncoming):
>+# %1$S will be replaced with either resultIMAP, resultPOP3 or resultSMTP.
>+# %2$S will be replaced with the server hostname. The domain part may be made bold.
>+# %3$S will be replaced with the port (a number between 1 and 65535).
>+# %4$S will be replaced with either resultNoEncryption or resultSSL or resultSTARTTLS.
>+# %5$S will be replaced with either resultSSLCertWeak or resultSSLCertOK (which should normally be empty)
>+# %6$S will be replaced with one of the auth* strings (e.g. authPasswordEncrypted) in messenger.properties .
>+# You may adjust the strings to be a real sentence, if that is nicer in your language.
>+resultIncoming=%1$S, %2$S, Port %3$S, %4$S%5$S, %6$S

Holy ouch.  I once heard that if you've got a function with six parameters, you're missing one.  I suspect the same applies to properties.  ;)

>+++ b/mail/locales/en-US/chrome/messenger/accountCreationUtil.properties
>@@ -13,8 +13,14 @@ boolean.error=Not a boolean
> no_number.error=Not a number
> number_too_large.error=Number too large
> number_too_small.error=Number too small
> 
> 
> # fetchhttp.js
> cannot_contact_server.error=Cannot contact server
> bad_response_content.error=Bad response content
>+
>+#verifyConfig.js
>+authFailedGeneric=Login failed. Are username/email address and password correct?
>+authFailedWithReason=Login failed. The server said: %S
>+verificationFailed=Login verification failed for an unknown reason.
>+verificationFailedWithException=Login verification failed: %S

The other things in this file seem to end in ".error", and these strings look like errors.  I think we want them to end in .error.

>+++ b/mailnews/base/prefs/content/accountcreation/accountConfig.js
>@@ -229,27 +229,24 @@ function replaceVariables(account, realn
>-  account.incoming.hostname =
>-    sanitize.hostname(_replaceVariable(account.incoming.hostname,
>-                                       otherVariables));
>-  if (account.outgoing.hostname) // will be null if user picked existing server.
>-    account.outgoing.hostname =
>-      sanitize.hostname(_replaceVariable(account.outgoing.hostname,
>-                                         otherVariables));
>+  account.incoming.hostname = _replaceVariable(account.incoming.hostname,
>+                                               otherVariables);
>+  account.outgoing.hostname = _replaceVariable(account.outgoing.hostname,
>+                                               otherVariables);

Why aren't we sanitizing the hostname here?

What happens if an ISP mistakenly puts in something bad?

Do we display these values?

>+++ b/mailnews/base/prefs/content/accountcreation/createInBackend.js
>@@ -37,22 +37,22 @@
>- * @ret - the account created.
>+ * @return - the account created.
>+ * (public function)

I've never seen this before, and I think all the functions are public by default.  (We'ld name them _createAccountInBackend if we wanted them to be private.)

> function createAccountInBackend(config)
> {
>-  const Cc = Components.classes;
>-  const Ci = Components.interfaces;
>+  //config.validate();

I think we can remove this if we're not going to call it.  ;)

>@@ -238,18 +238,75 @@ function rememberPassword(server, passwo
>-// Check if there already is a "Local Folders". If not, create it. This routine
>-//  is copied from AccountWizard.js with minor updates.
>+/**
>+ * @param config {AccountConfig} filled in (no placeholders)
>+ * @return {Boolean} user's setup already has an incoming server
>+ * which matches (hostname, port, username) the primary one
>+ * in the config.
>+ * (public function)
>+ */
>+function checkIncomingAccountIsNew(config)

I love that you've turned this into a docstring, but we should use the previous comment (or something like it) as the first line or two.

>+  assert(config instanceof AccountConfig);
>+  //config.validate();

We can remove this too.

>+  var incoming = config.incoming;

We prefer to use let instead of var these days, or so I'm told.  :)

>+  var isNew = gAccountMgr.findRealServer(incoming.username,
>+                                        incoming.hostname,
>+                                        sanitize.enum(incoming.type,
>+                                                      ["pop3", "imap", "nntp"]),
>+                                        incoming.port) == null;

The indentation on those parameters looks a little off to me.

>+/**
>+ * @param config {AccountConfig} filled in (no placeholders)
>+ * @return {String} Either the nsISmtpServer.key for an existing
>+ * SMTP server (in the user's setup) that matches
>+ * (hostname, port, username) the outgoing server in the config,
>+ * or |null| if no match is found.
>+ * (public function)
>+ */
>+function outgoingServerAlreadyExists(config)

We need a one or two line description at the start of the docstring.

>+{
>+  assert(config instanceof AccountConfig);
>+  //config.validate();
>+  var smtpServers = gSmtpManager.smtpServers;
>+  while (smtpServers.hasMoreElements())
>+  {
>+    let existingServer = smtpServers.getNext()
>+        .QueryInterface(Ci.nsISmtpServer);
>+    if (existingServer.hostname == config.outgoing.hostname &&
>+        existingServer.port == config.outgoing.port &&
>+        existingServer.username == config.outgoing.username)
>+      return existingServer.key;
>+  }
>+  return null;

It seems like we should be able to write a comparator, or a filter, or something more elegant there.

>+/**
>+ * Check if there already is a "Local Folders". If not, create it.
>+ * <copied from="AccountWizard.js"> with minor updates
>+ */

I think we can remove the faux-xml tag there.  ;)

>+++ b/mailnews/base/prefs/content/accountcreation/emailWizard.js

Uh, I'm going to skip this for now, and review it on the next version of the patch, cause it's big, and it's late, and I'm tired.

>+++ b/mailnews/base/prefs/content/accountcreation/emailWizard.xul
>@@ -52,18 +52,22 @@
>     <stringbundle id="bundle_brand" src="chrome://branding/locale/brand.properties"/>
>-    <stringbundle id="strings" src="chrome://messenger/locale/accountCreation.properties"/>
>-    <stringbundle id="utilstrings" src="chrome://messenger/locale/accountCreationUtil.properties"/>
>+    <stringbundle id="strings"
>+          src="chrome://messenger/locale/accountCreation.properties"/>
>+    <stringbundle id="utilstrings"
>+          src="chrome://messenger/locale/accountCreationUtil.properties"/>
>+    <stringbundle id="bundle_messenger"
>+          src="chrome://messenger/locale/messenger.properties"/>

You can't change two of the elements ("strings" and "utilstrings") to avoid wrapping on 80 characters, and not change the third ("bundle_brand").  ;)

>@@ -137,46 +141,47 @@
>+  <spacer id="fullwidth"/>

That's odd, I thought we had one of those already.

>-    <groupbox id="initialSettings">
>+    <groupbox id="initialSettings" style="margin-bottom: 2em;">

Boo!  Style should go in a CSS file, so that the other OSs can do something different.

>@@ -184,229 +189,236 @@
>-      <hbox align="center">
>-        <label class="autoconfigLabel"/>
>+      <hbox align="center" pack="start">
>+        <label class="autoconfigLabel" value="" />

Why have the empty value?  And we shouldn't have the space before the "/>".

>         <checkbox id="remember_password"
>                   label="&rememberPassword.label;"
>                   accesskey="&rememberPassword.accesskey;"
>-                  disabled="true"
>-                  checked="false"
>-                  onclick="gEmailConfigWizard._userChangedPassword=true;"/>
>+                  checked="true"/>

I don't think we want the remember password checkbox checked if the user hasn't entered a password yet.  And I'm fairly sure that we want it disabled.

>-      <hbox id="fullspacer"/>

Hey, there's the fullspacer I was wondering about!  :)

>+      <radiogroup id="result_imappop" hidden="true" orient="horizontal"
>+          style="margin-top: 0.5em;">
>+        <radio id="result_select_imap" label="&imapLong.label;" value="1"
>+              oncommand="gEmailConfigWizard.onResultIMAPOrPOP3();" />
>+        <radio id="result_select_pop3" label="&pop3Long.label;" value="2"
>+              oncommand="gEmailConfigWizard.onResultIMAPOrPOP3();" />
>+      </radiogroup>

I think you can use an onselect on the radiogroup instead.
From http://www-archive.mozilla.org/projects/xul/widgets.html:
>Whenever the selection changes on a radiogroup, a 'select' event fires on
>the radio group. A handler can be attached to the radiogroup to listen for
>changes in selection. The handler will fire after the selection has
>changed.

><radiogroup onselect="alert('The new selected item is: ' + this.selectedItem + '\n');">
>  <radio label="Item One"/>
>  <radio selected="true" label="Item Two"/>
>  <radio label="Item Three"/>
></radiogroup>

And that seems more like what you'll want.

>+    <groupbox id="manual-edit_area" hidden="true" style="margin-bottom: 2em;">
>+      <grid>
>+        <columns>
>+          <column/><column/><column/><column/><column/><column/><column/>
>+        </columns>

No flex in the columns?

>+            <textbox id="incoming_server"
>+                     value=""
>+                     style="width: 15em"
>+                     oninput="gEmailConfigWizard.onInputHostname();"

I think we should have "server" and "Hostname" match.  Also, how do we know whether it's the incoming hostname or the outgoing hostname that has been input?  Shouldn't we pass the event in as an argument?

>+            <menulist id="incoming_ssl"
>+                      class="security"
>+                      value=""
>+                      sizetopopup="always">
>+              <menupopup
>+                      onpopuphidden="gEmailConfigWizard.onChangedSSL();">

Since some of this is STARTTLS, or None, should we make this "onChangedSecurity" instead?  (And fix the id, etc…)

>+              <menupopup
>+                      onpopuphidden="gEmailConfigWizard.onChangedAuth();">

That all fits on one line, so we should probably join it up.

>+++ b/mailnews/base/prefs/content/accountcreation/fetchhttp.js
>@@ -201,37 +201,43 @@ FetchHTTP.prototype =
>     // Callbacks
>     if (success)
>+      try {
>+        this._successCallback(this.result);
>+      } catch (e) { logException(e); this._error(e); }
>     else if (exStored)
>+      this._error(exStored);
>     else
>+      this._error(new ServerException(errorStr, errorCode, this._url));

So, my understanding is that we should put {}s around multi-line if blocks, and so after that, we should put them around the else-ifs and elses on the same level.  I also feel a little wiggy about having the catch block be all on one line like that.  Please break it up onto multiple lines.

>     if (this._finishedCallback)
>+      try {
>+        this._finishedCallback(this);
>+      } catch (e) { logException(e); this._error(e); }
> 
>     } catch (e) {
>+      // error in our fetchhttp._response() code
>+      logException(e);
>+      this._error(e);
>+    }

So, I don't think any of the callback code can throw an error, so we should be able to move this catch block up above it, to make my brain hurt less.  And all that intermediate code should be indented more.

>+  _error : function(e)

I like this function.  Nice encapsulation.

>+++ b/mailnews/base/prefs/content/accountcreation/guessConfig.js
>-    incomingHostDetector.start(domain, !!resultConfig.incoming.hostname,
>-        resultConfig.incoming.type, resultConfig.incoming.port,
>-        resultConfig.incoming.socketType);
>+    incomingHostDetector.start(resultConfig.incoming.hostname ?
>+            resultConfig.incoming.hostname : domain,
>+        !!resultConfig.incoming.hostname, resultConfig.incoming.type,
>+        resultConfig.incoming.port, resultConfig.incoming.socketType);

That's some ugly indenting.  I don't think you can do anything nicer, but it's ugly.  ;)

>@@ -926,17 +882,17 @@ function SSLErrorHandler(thisTry, logger
>   processCertError : function(socketInfo, status, targetSite)
>   {
>-    this._log.warn("Got Cert error for "+ targetSite);
>+    this._log.error("Got Cert error for "+ targetSite);

I'm not completely convinced that getting a cert error is actually an error for us.  I think that there are a depressing number of them out there.

>@@ -1133,17 +1089,17 @@ function SocketUtil(hostname, port, ssl,
> SocketAbortable.prototype =
> {
>-  cancel : function()
>+  cancel : function(e)
>   {
>     try {
>       this._transport.close(Components.results.NS_ERROR_ABORT);
>     } catch (e) {
>       ddump("canceling socket failed: " + e);

You're overriding the "e" parameter here.  I don't think it really matters, but it's not great practice.  ;)  Also, we're not using the "e" parameter.  Why not?

>+++ b/mailnews/base/prefs/content/accountcreation/verifyConfig.js
>@@ -270,38 +274,43 @@ urlListener.prototype =
>+    var me = this;

Just FYI, I've seen a lot of people use "self" for this.  I don't think you should change it, but thought you might want to know.

>+    setTimeout(function () { // TODO why???
>+      try {
>+      me.informUserOfCertError(socketInfo, targetSite);

Missing indentation.

My biggest worry with this patch is that there's a lot of new stuff here, and no tests, so I don't know what might break with some strange interactions, or what sort of stuff you've tested with.

And we also need some try-server builds for Bryan and I (and DMose, and maybe other people) to play with, cause I'm going to have to insist on another ui-r which focuses on interactions instead of just screenshots.  (But feel free to run the code by me first, if you'ld like, and I'll review emailWizard.js on the next patch as well.)

Thanks,
Blake.

Comment 32

[Ben Bucksch (:BenB)]

> +<!ENTITY manual-edit.label               "Manual config">

> If we're renaming it to "Advanced Setup" above, shouldn't this be "Advanced
> config"?  Or, actually "Advanced Config", since we seem to be capitalizing both
> words.

They're intentionally distinct. "Manual Config" allows you to specify the *basic* server properties manually, for whatever reason. The main reason is probably that we didn't detect your server or detected it wrongly. It's more or less what the old wizard was (just that you can now also specify SSL and auth).

The "Advanced Setup" is really just for advanced users who are happy to click themselves through all the detailed UI of our Account Settings, and know how to delete the account when they decide to cancel, because we *immediately* create the account after they click that button. We also don't check the values at all. So, they are distinctive and very different modes, and the Advanced one *should* look scary for users, because it really does remove all seat belts.

We could change the other one to "Advanced config" (instead of setup), though. Up to Bryan.

> Should this be "SSL/TLS"

Technically, yes, but I am trying to make this summary more in colloquial tone, how humans would communicate, because the user just needs to review it.

> +resultSSLCertWeak=_(Warning: Could not verify server)
> +resultSSLCertOK=

> These two look kinda odd to me.  Can you explain what the _(…) and the empty
> property are about?

Yes, it's a cert warning that I want to insert right here in the summary, as a note after the "SSL". If it's OK, there's nothing to insert - in English, I don't know about other languages, e.g. deflection. When there's a warning, I need to add it with a space after SSL, e.g.
"..., Port 465, SSL (Warning: Could not verify server), Encrypted password"
vs., when everything's no, no warning, and no space before the comma:
"..., Port 465, SSL, Encrypted password"

That underscore indeed needs to be replaced. I tried a space, but the parser removed leading (and trailing) spaces. Maybe I can use an escape code or non-breaking space or something like that. Thanks for the reminder.

> +resultIncoming=%1$S, %2$S, Port %3$S, %4$S%5$S, %6$S
> Holy ouch.  I once heard that if you've got a function with six parameters,
> you're missing one.

Because 6 is an unholy number and one more would make it holy? :)
Seriously, this is currently just listing the parameters, but I'd like (or have the option) to make it more colloquial, a real user-understandable sentence instead of just a technical gibberish, like

"An IMAP server, with hostname imap.web.de on port 993, using encryption via STARTTLS to protect your mail, but we could not verify the server, so the protection is voided, and using Normal Password authentication to recognize and verify you."

You can do that, as-is. Given that I use a formatted string and luckily the params are now numbered and can be rearranged, and I made all the parts (apart from auth) also separate strings, you should be able to adjust to most other languages (and you can always fall back to the list :) ).

[properties strings]
> I think we want them to end in .error.

Yes, we do. Thanks.

> @@ replaceVariables

> Why aren't we sanitizing the hostname [anymore] here?

Because I now need to call it as part of manual config, before we know that it's complete.

> What happens if an ISP mistakenly puts in something bad?

We would have already catched it in readFromXML and ignored the XML.

> Do we display these values?

Yes.

> we should use the previous comment (or something like it) as the
> first line or two.

That comment belonged to an entirely different function (just diff oddness), but I'll add an appropriate comment.

BTW: As said on IRC, don't even bother reading emailWizard.js as diff. I changed so much that it's useless. Just put old and new files next to each other and compare like that. (I hope you'll like my new version, esp. in comparison to the old one! :) )

> +  while (smtpServers.hasMoreElements())
> It seems like we should be able to write a comparator, or a filter

For an nsISimpleEnumerator? Does that implement the JS interface that |filter| (or |for each ... in| for that matter) needs? Could be, I don't know.

> +  <spacer id="fullwidth"/>
> That's odd, I thought we had one of those already.

We do, I just renamed it.

> Style should go in a CSS file

Ah, right... I avoided touching the style sheets in the beginning, because I always have to change 3 files for one change, but need to do that now.

> I don't think we want the remember password checkbox checked if
> the user hasn't entered a password yet.

Why? What difference does it make? It makes no sense to me and just complicates the code.

In fact, I think it's a sneaky, unfair surprise to the user, when he looks at the dialog, sees "Password", thinks "but I don't want to have it saved", sees the "remember password" and that it's *not* checked, and relaxes and enters the password, but then doesn't notice that we then suddenly check the checkbox around. I think that's just wrong.

If the user didn't enter a password, there's nothing to save, so we'll ignore the "remember password". That's perfectly logical to a user, too.

> I think you can use an onselect on the radiogroup instead.

OK

> No flex in the columns?

hm, I guess I'll let the hostname/username textfield flex, in case the user manually widens the window.

> how do we know whether it's the incoming hostname or the outgoing hostname
> [that changed]

We don't care.

> Shouldn't we pass the event in as an argument?

If we care, we'll just make 2 functions. onInputIncomingServer() and onInputOutgoingServer(), that's much less error-prone in code than a soft "incoming" param.

> gEmailConfigWizard.onChangedSSL();">
> Since some of this is STARTTLS, or None, should we make this
> "onChangedSecurity"

I want to avoid equating SSL with "Security", because that's just wrong (it's just old Netscape propaganda). Just because you use SSL doesn't mean you're secure. For me, "secure" means "unhackable", and you need a *lot* more for that than transport layer encryption. That's why I crinch every time I see "Security" for "SSL".

STARTTLS is a special form of SSL (or rather TLS, its successor).

Actually, we should consider changing "SSL/TLS" to "Normal SSL" in the UI.

> catch block be all on one line

I do that whenever the catch just captures and forwards the error. I reserve multi-line catch blocks to real error handling (which will otherwise go under unnoticed). The idea is to make code *logic* as readable as possible, and when you read and try to understand the basic flow, you usually don't care about error situations. I find it wrong when constant try/catch have the same code footprint as an |if| which is an important logic branching.

Before I actually avoided the small try/catch here, for exactly that reason (too much footprint), but that led to subtle problems.

But I'll change it anyways.

> I don't think any of the callback code can throw an error

Actually, it did (at least before or during my coding, or other, similar callbacks did), leading to some of the weird bugs that this bug fixes.

> That's some ugly indenting.  I don't think you can do anything nicer, but it's
> ugly.  ;)

Agreed :)

> +    this._log.error("Got Cert error for "+ targetSite);
> I'm not completely convinced that getting a cert error is actually
> an error for us. I think that there are a depressing number of them
> out there.

Just because it happens a lot doesn't make it less serious.
Note that it's just the logger, and the default pref is None.

BTW: Can you tell me some of the broken server with bad certs (with different types of badness: expired, self-signed, good CA but hostname mismatch). I need them for fixing the SSL cert error handling, but I don't actually know bad servers. (Luckily, at least the big ISPs seem to be fine.)

> You're overriding the "e" parameter here.

Thanks, will rename.

> My biggest worry with this patch is that there's a lot of new stuff here, and
> no tests, so I don't know what might break with some strange interactions, or
> what sort of stuff you've tested with.

Well, the tests were your part... ;-)

But the old code is *so* broken that it's going to be hard to make it worse, assuming proper manual testing.

The old code didn't have any tests either. I think it make sense to have some UI tests here, but we can do them later. And, the tests can happen after the beta2 freeze, while the strings here do need to get in ASAP.

> we also need some try-server builds for Bryan and I (and DMose,
> and maybe other people) to play with

I happened to make some last night.

http://s3.mozillamessaging.com/build/try-server/2010-04-06_20:[e-mail]-autoconfig-ui-v7/[e-mail]
http://s3.mozillamessaging.com/build/try-server/2010-04-06_20:[e-mail]-autoconfig-ui-v7/[e-mail]
http://s3.mozillamessaging.com/build/try-server/2010-04-06_20:[e-mail]-autoconfig-ui-v7/[e-mail]2

But the changes are JS+XUL only, so easy to patch a build of yours.

> another ui-r which focuses on interactions instead of just screenshots.

I had included the interactions in the discussion with Bryan. I hope I can find 15 minutes of his time again.

Comment 33

[Ben Bucksch (:BenB)]

Attachment: Fix. v8

Review comments addressed
Window size fixed (large enough for all content, unless user goes to manual edit or we show the security warning)

Comment 34

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 434077 [details]
v1, Screenshot, Detection

The stop detection button needs to be in with the detection text.

See previous mockup:
https://wiki.mozilla.org/File:Thunderbird-Autoconfiguration-Auto-Configurating.png

Likely for the spinner we'll want a slightly different layout.

Perhaps center the text and make it on two lines.

{spinner} Looking up configuration ( Cancel )
    Mozilla ISP database

This way the ( Cancel ) button isn't moving around as the lookup location changes.

Comment 35

[Ben Bucksch (:BenB)]

Attachment: v1, Screenshot, Result, reduced

After discussion with Bryan, I reduced and modified the result screen:
- Added Username, because Bryan says this is a common problem for users.
  If it's the same for both, I show it only once, e.g. just "tom".
  If it's different, I show e.g. "Incoming: tom, Outgoing: tomexample.com"
- Added Incoming/Outgoing/Username labels, dimmed
- Moved the IMAP/POP selection above the result. That's Bryan's suggestion,
  and I agree, that's much more logical.
- Removed server port, if it's one of the standard ports.
  (Removes clutter, without reducing significant information, but still
  shows it when needed, in form "hostname:123")
- Removed authentication. That's the part that hurts me most, because in
  case of e.g. GSSAPI, it's actually important, but I can see why the
  "normal password" and "encrypted password" is not terribly useful
  (We'll later warn against non-SSL configs anyways!) and is just clutter.
  Maybe I'll later re-add it for non-standard cases (i.e. not normal
  password and not encrypted password, but GSSAPI, NTLM etc.),
  same pattern as for port, just need to figure out the string/layout.

I still believe that this organization where the IMAP/POP radio is above the result group (and not part of it) makes sense. First, it removes a lot of clutter and business. Any layout where IMAP/POP is in the "incoming group" *and* as radios must necessarily be asymmetric, spacey and look cluttered.
(The only compromise that I could see be a dropdown, as shown in bug 532590 attachment 435123 [details] ("Dropdown").)
Second, the IMAP/POP selection is a *choice*, while the server names are the *result* (if the user input and queries). So, I think it makes sense to separate them. The result is what the user needs to review and confirm and what we will create immediately when the user clicks "Create Account".

I think it's looking good as-is. We could consider making the encryption an icon, but it would lose the information of SSL vs. STARTTLS (which also implies the port, see above), so the user would no longer see the real config.

As mentioned before, I would like to make it even easier to understand for the user, by bolding the domain name (not the full host), as security review. I haven't figured out how to do that nicely in the code.

Comment 36

[Ben Bucksch (:BenB)]

Attachment: Fix, v9

Also implemented default port setting in manual edit mode (unless user entered a non-standard port manually).

Comment 37

[Ben Bucksch (:BenB)]

Attachment: Fix, v10

Updated to trunk

Comment 38

[Ben Bucksch (:BenB)]

(This also fixes sanitize.integer() which previously didn't catch non-integers properly. Possibly security-relevant.)

Comment 40

[Ben Bucksch (:BenB)]

Builds with patch v10:
http://s3.mozillamessaging.com/build/try-server/2010-04-14_14:[e-mail]-autoconfig-ui-10/[e-mail]
http://s3.mozillamessaging.com/build/try-server/2010-04-14_14:[e-mail]-autoconfig-ui-10/[e-mail]
http://s3.mozillamessaging.com/build/try-server/2010-04-14_14:[e-mail]-autoconfig-ui-10/[e-mail]2

Comment 41

[Ben Bucksch (:BenB)]

dmose has informed me one week ago that he will not allow this into 3.1.
For the record, I disagree and think this would be important to ship with 3.1, given how broken the dialog currently is.

Comment 42

[Kent James (:rkent)]

I happen to agree that this is a critical bug that must make 3.1, so I thought that I would give it a try to see if I could help move it forward. I loaded the "Fix, v10" version of the patch.

Unfortunately, I can't make it work.

First, the "Mail Account Setup" screen has a glitch in it, the text "\#x00A0;" appearing above the Cancel/Continue line. Also, The Cancel button is smaller than the Continue button - maybe that is by design since I see it on other screens as well. (Screen shot to follow).

On the next screen, "Mail Account Setup" I have a technical complaint which also exists in the current dialog. That is, since I am using a shared account, the username should be "[e-mail]" but shows as "kenttest"  The old dialog eventually corrects that, but it is disconcerting to see a wrong user name at this point.

When the screen runs, I see on the DOS console:

JavaScript error: chrome://messenger/content/accountcreation/accountConfig.js,
ine 246: account.incoming is undefined

Now I cannot proceed. I press "Manual config" but it does nothing. The above line appears each time I press the Manual button, and Error console will show the same thing.

Since that does not work, let me try "Create Account" let me try "Create Account" But that does not work either, I get an error screen "Error Creating Account, TypeError: account.incoming is undefined"

So I have to cancel in failure. Let me see if I can figure out why I am getting that error as well.

Comment 43

[Kent James (:rkent)]

Attachment: glitch text in Mail Account Setup

Comment 44

[Ben Bucksch (:BenB)]

> \#x00A0;

I'll take a look, but it may also be the patch application. There are test builds in the comments above, try these too.

> Cancel button is smaller

That's by design and Bryan's idea. I can make them all the same size.

> since I am using a shared account,
> the username should be "[e-mail]" but shows as "kenttest"

How are we to know that? We can't.
As you say, we eventually try both. (We can only try, with an actual login. There's no other way to find out, apart from your hoster supporting autoconfig.)
You may also click "Manual config".

> JavaScript error: chrome://messenger/content/accountcreation/accountConfig.js,
> ine 246: account.incoming is undefined

Please describe steps for reproduction. If possible, please get a stack (catch the error and do logException(e);).

Comment 45

[Ben Bucksch (:BenB)]

FWIW, the misaligned textfields in the screenshot look terrible! That's - to my knowledge - not my patch, though, because I haven't touched that code. I'll try to fix it in a later patch version, though.

Comment 46

[Ben Bucksch (:BenB)]

Attachment: Fix, v11

- Updated to trunk
- Fixes the exception rkent reports
  (Reproduction: start with fresh profile without accounts)
- Fixes the "\x00A0"

Comment 47

[Nomis101]

I was inquisitorial and tested your current patch (v11) on Mac. It looks nice, but at the moment I don't think its a real improvement. I can explain you what I don't like:

1. The initial window is, compared to the current one, very big, with very much empty space in it.
http://img534.imageshack.us/img534/3186/firsti.jpg
You only know why there is so much space after you hit the "Continue" button. In the current wizard this extra space for the settings is generated after you hit the "Continue" button. 

2. You also see in the above screenshot, "Remember password" is checked even though you not have entered any. In the current wizard this box is only checked if you enter a password.

3. This is the worst thing. Its not possible to change from IMAP to POP in "Manual config". The wizard founds imap.web.de for my account. Than I changed the incoming server from IMAP to POP and hit "re-test". Than it takes a long while, but after the process my incoming server was still "imap.web.de". But it should be "pop3.web.de".
And I think it would be a real improvement to current behaviour if I don't need to hit re-test if I change my incoming server. Isn't it possible to search automatically for the new incoming server if I change it, without hiting a re-test button?

Comment 48

[Ben Bucksch (:BenB)]

> 1. The initial window is, compared to the current one, very big

But that was an explicit request from Bryan to avoid resizing the dialog.
I don't like it either, and in my earlier patches, it was smaller.

> "Remember password"

Comment 32, in the middle.

> Its not possible to change from IMAP to POP

1. It is. "Re-test" is optional. If you leave any setting on "Auto", you of course need to "Re-test". If you selected all options manually, you don't need to click "Re-test" and can go to "Create Account" without any second guessing.

> The wizard founds imap.web.de for my account. Than I changed
> the incoming server from IMAP to POP and hit "re-test". Than it takes a long
> while, but after the process my incoming server was still "imap.web.de". But > it should be "pop3.web.de".

2. You mean you selected "POP" as protocol and "imap.web.de" as hostname? That's of course not going to work (imap.web.de doesn't support POP). If you go "manual config", then enter proper values. We're not going to change the server hostname, otherwise other users will rightfully complain that we're not accepting what they entered, even though they are in manual mode.

3. I can't even reproduce what you said. When I have "POP" as protocol and imap.web.de as hostname and SSL (which selects port 995) or STARTTLS (which selects port 110), and click "retest", I get a fail: "Shredded failed to find settings for your account".

Comment 49

[Ben Bucksch (:BenB)]

Note: The normal way to select between IMAP and POP3 is the radiobox as shown in attachment 437846 [details]. To see that, you need to enter [e-mail] as email address, or (for ISP configs like web.de) wait for bug 556729.

Comment 50

[Nomis101]

(In reply to comment #48)
> > "Remember password"
> 
> Comment 32, in the middle.
OK, I understand your philosophy now for doing this. But it still confuses me that Thunderbird purports to save a password that doesn't exist...

> 2. You mean you selected "POP" as protocol and "imap.web.de" as hostname?
No. I proceeded the same way I do it in the current wizard. After the wizard founds the imap settings I've entered into the manual configuration and changed IMAP to POP via the drop down menu. In the current wizard if you than re-test the settings, than your incoming server switches automatically from imap.web.de to pop3.web.de and the Port to 110. In your wizard this doesn't happen. 


(In reply to comment #49)
> Note: The normal way to select between IMAP and POP3 is the radiobox as shown
> in attachment 437846 [details]. To see that, you need to enter [e-mail] as email
> address, or (for ISP configs like web.de) wait for bug 556729.
Ah, thats the trick.  Now I can easily switch between IMAP and POP. OK, thats much better and a nice improvement. :-)


(In reply to comment #48)
> 3. I can't even reproduce what you said. When I have "POP" as protocol and
> imap.web.de as hostname and SSL (which selects port 995) or STARTTLS (which
> selects port 110), and click "retest", I get a fail: "Shredded failed to find
> settings for your account".
Yes, I see what you mean. If I only change IMAP to POP in the manual config, without touching the other settings, and than retest my settings, than I get "The following settings were found by probing the given server", but nothing changes (like in the current wizard). But, if I also change "Normal SSL" to anything else, than I also get "Shredded failed to find settings for your account".

Comment 51

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #48)
> > 1. The initial window is, compared to the current one, very big
> 
> But that was an explicit request from Bryan to avoid resizing the dialog.
> I don't like it either, and in my earlier patches, it was smaller.

No, I didn't ask for a larger dialog with mostly empty space.  You seem to have misunderstood.  If you look at my mockups I have those same original sizes from 1 to 2.  There was a re-sizing I wanted to avoid from 2 to 3.

1: https://wiki.mozilla.org/File:Thunderbird-Autoconfiguration-Initial_View.png
2: https://wiki.mozilla.org/File:Thunderbird-Autoconfiguration-Auto-Configurating.png
3: https://wiki.mozilla.org/File:Thunderbird-Autoconfiguration-Auto-Configured.png

Also we have an initial size problem that I don't like but I didn't have a solution to it.  I proposed that we *could* open the dialog at the required size but we would encounter issues using all that space up correctly; as seen.

Comment 52

[Ben Bucksch (:BenB)]

Bryan, OK, great. I'll happily adjust it again. Just to know I understand:
1. The start mode (3 textfields) is sized minimally.
2. When we try to find the config, we increase the size of the dialog to show the status messages.
3. Once we found a config, we increase the size of the dialog to show the result
4. If we go into manual mode (either because the user clicked the button or we didn't find any config), we increase the size of the dialog.

If you meant that, I totally agree and think that's nicest.

In steps 1-3, I can avoid increases of the width and only increase the height, if you want. The current dialog does that as well. (Step 4 - manual config - will still increase in both directions, because the widgets can be large.)

Comment 53

[Ben Bucksch (:BenB)]

FWIW, we also have to pay attention for the length of status and error messages, including other languages (which are often longer than English). Esp. for error msgs, linebreak can't save us, because a linebreak would mean that the position of the other text fields shifts, which is annoying.

Comment 54

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 437857 [details] [diff] [review]
Fix, v9

I'm not reviewing this version!  ;)

Comment 55

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 439725 [details] [diff] [review]
Fix, v11

So, this patch is about 10 times too big for me to review.

And it doesn't contain any tests.

And it really needs some sort of UI review as well, before I get to it.

But seriously, Ben, you need to break this change up into self-contained patches of at most 500 lines (100-200 would be far preferable) if you want me to review it.  (And I'm probably going to insist on tests, too, since we've got the time to add them before 3.2 ships.)

Thanks,
Blake.

Comment 56

[Blake Winton (:bwinton) (:☕️)]

(And just so you know I'm not pulling these numbers out of the air, <http://smartbear.com/docs/book/code-review-cisco-case-study.pdf> says:

> LOC under review should be under 200, not to exceed
> 400. Anything larger overwhelms reviewers and defects
> are not uncovered.

and

> Total review time should be less than 60 minutes, not ex-
> ceed 90. Defect detection rates plummet after that time.

and

> Given these factors, the single best piece of advice we can
> give is to review between 100 and 300 lines of code at a time and
> spend 30-60 minutes to review it.

Thanks,
Blake.

Comment 57

[Ben Bucksch (:BenB)]

> it doesn't contain any tests.

Yes. You had promised me that you write them. A few weeks ago. We had agreed to work together on this and on the IMAP/POP bug. In fact, you had told me that we'd get this into 3.1, if I finish it in time. I was in time, but denied the commit even before the feature freeze. And now you're refusing to review it?

> too big ... to review ... break this change up

A simple look at the patch will show that it's impossible to break down. There's no way, because it's an overhaul of the code. The old code had bad design and logic patterns (basically no design). This straightens out the code, and makes the modes and data flow more explicit and clear. There's no way to break this down, inherently.

I had already separated out the guessConfig change. This is only the dialog UI code change, so I had already separated it as far as possible. It's not really possible to break it down more.

And, FWIW, to make small patches, I need really tight review loops, in the area of <1h, because I need to continue working on the same code and can't halt for a day just because I finished one change. You could follow my change progress in the hg repository, if you want smaller chunks.

As I said, it doesn't really make sense to review this as diff. Just put old and new code next to each other. You'll see that the new code is more logical. I hope.

Rejecting a patch just because it's overhauling broken code does not make sense and removes all possibility to contribute in a meaningful way.

FYI, there are other projects which don't require *any* review, not even shortly before release. If the result of review is that I cannot replace badly broken, misdesigned and extremely buggy code, then you should reconsider the processes. Sorry, but it's getting silly. I expected you to be happy that there's somebody who's taking the time to fix this mess.

Comment 58

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #57)
> > it doesn't contain any tests.
> Yes. You had promised me that you write them. A few weeks ago.

Okay, that's fair enough.  Back when we were trying to get it into 3.1, I would have tried to write them all.  And I will still help write them, but now that the time pressure has lessened, it hardly seems fair for me to write all the tests anymore.

> We had agreed to
> work together on this and on the IMAP/POP bug. In fact, you had told me that
> we'd get this into 3.1, if I finish it in time.

I do remember saying that I would love to get it into 3.1, and would help you as much as I could, but the final decision was not mine to make, and if I gave you the impression that it was then I sincerely apologize.

> I was in time, but denied the commit even before the feature freeze.

I would suggest that it still isn't done, and that huge changes without tests need much more time than smaller patches, and so this fix wasn't done in time.  (The current patch doesn't even have a ui-r yet.)

> And now you're refusing to review it?

Not because I don't want to.  I just don't feel that it's physically possible for me to review it.

> > too big ... to review ... break this change up
> A simple look at the patch will show that it's impossible to break down.

I don't think that's true.  I think you can get there from here in a series of steps.  Heck, I think you did get there from here in a series of steps, because I can't believe that you deleted all the old code, and wrote all the new code from scratch, and didn't test any of it (manually) until the end.

> I had already separated out the guessConfig change. This is only the dialog UI
> code change, so I had already separated it as far as possible. It's not really
> possible to break it down more.

I don't think that's true.  I see a bunch of utility functions that could be their own patch.  I see some changes to the strings that could be their own patch.  I see some re-indentation that could be its own patch.  I think you could move the existing xul into its new locations in a patch, and then do another patch that changed the behaviour.

Heck, as you mention below, if you just put up the patches from your hg repository, they should be smaller than 4600 lines of code each, and self-contained, to boot.

> And, FWIW, to make small patches, I need really tight review loops, in the
> area
> of <1h, because I need to continue working on the same code and can't halt for
> a day just because I finished one change. You could follow my change progress
> in the hg repository, if you want smaller chunks.

While I would like to be able to review patches within an hour, that would prevent me from doing any other work, including creating patches of my own, which doesn't seem like a win (or like much fun).

> Rejecting a patch just because it's overhauling broken code does not make
> sense and removes all possibility to contribute in a meaningful way.

I'm not rejecting it because it's overhauling broken code, I'm rejecting it because I can't meaningfully review it.  (And according to the data, neither can any normal human.)

> FYI, there are other projects which don't require *any* review, not even
> shortly before release. If the result of review is that I cannot replace badly
> broken, misdesigned and extremely buggy code, then you should reconsider the
> processes. Sorry, but it's getting silly. I expected you to be happy that
> there's somebody who's taking the time to fix this mess.

I am very happy that you're putting in a lot of effort to clean this up, and I do appreciate it more than you might think.

If you want to leave this sitting in my review queue, I guess I can leave it as an r?, since the main reason I gave it the r- it was to get it out of my queue, but I am honestly not able to review it, so having it sit there with the r? is sort of a lie.

(You could also try asking someone else to review the code, but then you'ld be waiting on me to write the tests, and I can understand if you didn't want to be put in that position.)

Sorry about that,
Blake.

Comment 59

[Ben Bucksch (:BenB)]

> I can't meaningfully review it.  (And according to the data, neither
> can any normal human.)

There are tons of projects which take huge patches. See Linux kernel for example. As noted, other popular projects (like Evolution, which is probably the closest to us) don't even require *any* review whatsoever.

You have to realize that this is a refactoring. It was 1-2 weeks of work, full-time, hard work. How do you propose I do this?
I cannot work for 2 hours and produce a 100 line diff, put in in your queue, have it sit there for 2 weeks, and then continue, and repeat that 30 times (60 hours of work). This rework done here would simply be *impossible* this way.

The only way to do this in small patches would be "pair programming", where indeed we both work on it and you do nothing else. I did publish the hg repo, and we spoke about it and you wanted to take a look at it, but couldn't due to other time constraints. I understand and don't blame you, but you had the chance, you can't blame me for not providing small patches, because I did.

The only other alternative is that you get a big patch. "It was hard to write, so it should be hard to use" ;-). Seriously, it took 1-2 weeks to develop, so it's natural that it takes up to a day to review. Smaller loops would have only increased that time.

If it's impossible to review a 1000-line JS file, how can we ever add such code as new code? Obviously, we can. As I said, I suggest you don't look at the emailWizard.js file as diff but as new code. You had already reviewed the rest (everything apart from emailWizard.js). I do believe it's manageable to read and review, if you only look at the new emailWizard.js file.

> I am very happy that you're putting in a lot of effort to clean this up, and I
> do appreciate it more than you might think.

Well, that's not helping me any, if I can't ever commit this, does it?

Comment 60

[Blake Winton (:bwinton) (:☕️)]

> > I can't meaningfully review it.  (And according to the data, neither
> > can any normal human.)
> There are tons of projects which take huge patches. See Linux kernel for
> example. As noted, other popular projects (like Evolution, which is probably
> the closest to us) don't even require *any* review whatsoever.

I'm sure there are.  I don't think Thunderbird is that kind of project, though, and the basic fact is that you asked me for a review, and I don't think I'm able to review the patch you posted.  (I would still like to review patches that drive this bug forward, if you post them.)

> You have to realize that this is a refactoring. It was 1-2 weeks of work,
> full-time, hard work. How do you propose I do this?

That's a good question, and one we need to ask earlier in the process next time.  (Uh, not just you, I also need to ask it earlier.)  Could you post the commits from your repo as separate patches?  Would that split the chunks up enough?

> I cannot work for 2 hours and produce a 100 line diff, put in in your queue,
> have it sit there for 2 weeks, and then continue, and repeat that 30 times (60
> hours of work). This rework done here would simply be *impossible* this way.

One hopes that with small patches of importance, the reviews could be done in less than two weeks.  (I managed to turn a review around in a day just now (now that my review queue is almost clear), and it wasn't even a particularly important bug.)

> The only way to do this in small patches would be "pair programming", where
> indeed we both work on it and you do nothing else. I did publish the hg repo,
> and we spoke about it and you wanted to take a look at it, but couldn't due to
> other time constraints. I understand and don't blame you, but you had the
> chance, you can't blame me for not providing small patches, because I did.

So (after long conversations on IRC), this seems like two ways forward.

1) do pair programming, which is tough, given that this wasn't my highest priority for 3.1, but seems like a good way to get the most important things in.  I'ld like to try this for future large patches, and hopefully we can work the schedule out.

2) post each commit, which I will review separately.

> The only other alternative is that you get a big patch. "It was hard to write,
> so it should be hard to use" ;-). Seriously, it took 1-2 weeks to develop, so
> it's natural that it takes up to a day to review. Smaller loops would have
> only increased that time.

Would that it were just up to a day.

Let's assume I can effectively review 300 lines of diff per hour.  (That's on the high side according to the study, but I'll be generous.)  The patch would take over 15 hours to review.  But it's even worse than that, because after an hour of review, the effectiveness drops precipitously, so I would need to take a rest every hour for, say, an hour (again, probably on the low side).  So now we're looking at a 30-hour review.  At eight hours per day, that's almost four full days of me not doing anything but reviewing a single patch.  Not getting any of my bugs fixed, not reviewing anyone else's patches, nothing but reviewing this one patch.  And god help us if I find bugs in it and need to re-review it.

> If it's impossible to review a 1000-line JS file, how can we ever add such
> code
> as new code? Obviously, we can. As I said, I suggest you don't look at the
> emailWizard.js file as diff but as new code.

Even as new code, it's still 1700+ lines, or over 11 hours straight.

> > I am very happy that you're putting in a lot of effort to clean this up,
> > and I do appreciate it more than you might think.
> Well, that's not helping me any, if I can't ever commit this, does it?

Perhaps not immediately, but I've found that people appreciating my work has lead to good things for me in the past.  And I suspect that this code (or code like it) will eventually make its way into the tree, just not in the form it's in now.

(I think we've reached a resolution on this, after a long conversation on IRC.  Ben will post smaller diffs, containing parts of the changes, and I'll review those.  And he might even fix the should-be-failing tests. ;)

Thanks,
Blake.

Comment 61

[Ben Bucksch (:BenB)]

> the basic fact is that you asked me for a review

I don't have much of a choice.

> > How do you propose I do this?

> That's a good question, and one we need to ask earlier in the process next
> time.

Well, we did, we wanted to work together on this, based on hg. You just didn't have any time, without fault of your own, but neither of me.

> Could you post the commits from your repo as separate patches?

You do have them all right here:
https://hg.mozilla.org/users/mozilla.BenB_bucksch.org/tb-autoconfig-ui-1/
(skip over the trunk merges)

> Would that split the chunks up enough?

I think that's too much to review for you. even if I batch them in 300-line patches, I guess that would end up being 30-50 times 300 line patches. With one per day, that would take you all summer to review. I don't think that would work. (So much for the "small patches" idea.)

> One hopes that with small patches of importance, the reviews could be done in
> less than two weeks.

Even if it's one day turnaround always: 2 hours of work, one day wait, another 2 hours of work, and that over a period of 2 months (60 hours of work) does not work for me.

============================
I worked on this 10 hours per day straight. 3 days in a row. (And more at other days.) That's how I work, and the only way I can work. It's a passion.

You simply won't be able to keep up with me with reviewing that, unless you work full time with me as well. That's why the "small patches" idea fundamentally does not work and never will.
============================

> I think we've reached a resolution on this, after a long conversation on IRC. 

From how I understand, the resolution was:
- I'll post the changes to all files apart from emailWizard.js as diff, preferably as diffs with less than 300 lines each. (Blake had already reviewed them in comment 31, but wants to do it again.)
- I'll post emailWizard.js as plain source file, and put in "stop reviewing" markers in intervals of less than 300 lines, preferably where it makes logically sense
- Blake will review one of these <300 lines batches per day.
- With some luck, we're through after 5-10 days.

Comment 62

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #61)
> > the basic fact is that you asked me for a review
> I don't have much of a choice.

That's true, but that's also something that we should look into fixing.

> > > How do you propose I do this?
> > That's a good question, and one we need to ask earlier in the process next
> > time.
> Well, we did, we wanted to work together on this, based on hg. You just didn't
> have any time, without fault of your own, but neither of me.

Yeah, so that didn't work out so well.  We should figure out a better answer next time.

> > Would that split the chunks up enough?
> I think that's too much to review for you. even if I batch them in 300-line
> patches, I guess that would end up being 30-50 times 300 line patches. With one
> per day, that would take you all summer to review. I don't think that would
> work. (So much for the "small patches" idea.)

We do have all summer before 3.2 comes out.  ;)

> > One hopes that with small patches of importance, the reviews could be done in
> > less than two weeks.
> Even if it's one day turnaround always: 2 hours of work, one day wait, another
> 2 hours of work, and that over a period of 2 months (60 hours of work) does
> not work for me.

If you're using pbranch, you can create new sub-branches as you get to reasonable points and continue working, then post the later patches as the earlier patches get reviewed.

> I worked on this 10 hours per day straight. 3 days in a row. (And more at other
> days.) That's how I work, and the only way I can work. It's a passion.

I'll refrain from posting the study that suggests that you're not producing your best work after working those hours.  ;)

> You simply won't be able to keep up with me with reviewing that, unless you
> work full time with me as well. That's why the "small patches" idea
> fundamentally does not work and never will.

Just to re-state the previous point: You can continue working on it, just branch off periodically, and then I can review each pbranch as its own small patch, and you can merge any review fixes from previous patches up into later patches.

It might also be a good idea to find another reviewer.  One who can work with you on this in some sort of pair-programming fashion.  (I know, I know, there's only me.  I mean that we should get someone else in who can learn the code, and then we won't be so reliant on each other.)

> > I think we've reached a resolution on this, after a long conversation on IRC. 
> From how I understand, the resolution was:
> - I'll post the changes to all files apart from emailWizard.js as diff,
> preferably as diffs with less than 300 lines each. (Blake had already reviewed
> them in comment 31, but wants to do it again.)

They were mainly reviewed for style, not for substance, so a second review is probably necessary.

> - I'll post emailWizard.js as plain source file, and put in "stop reviewing"
> markers in intervals of less than 300 lines, preferably where it makes
> logically sense

(If you want to re-organize the file to make it easier to break, that would work for me, too.)

> - Blake will review one of these <300 lines batches per day.

At least one.  I might get to a second, depending on how my days turn out.

> - With some luck, we're through after 5-10 days.

I agree with that understanding.

Later,
Blake.

Comment 63

[Ben Bucksch (:BenB)]

> > - Blake will review one of these <300 lines batches per day.
> At least one.  I might get to a second, depending on how my days turn out.
> > - With some luck, we're through after 5-10 days.
> I agree with that understanding.

Fantastic! Glad to hear, and looking forward to it. Thanks!

Comment 64

[pclough]

I am chagrined by the exchanges above, and while the fiddling continues, we users burn....

Much of my career was spent developing code and managing projects to do so. From that two observations:

1. structured code allows for unitary functional testing which requires only simple test cases and where it is easy to spot errors
2. artificially limiting code to a specific number of lines with disregard for function is silly - or worse.  If this fix takes a lot of code, so be it.  Breaking code down to call it a patch rather than leaving as functionally integrated risks making it more buggy, not less.  If it needs to be considered as more than a patch, then it should be handled that way.

Please get on with this;  it's a mess left as it is.

Peter

Comment 65

[Ben Bucksch (:BenB)]

Attachment: Fix, v12

Comment 66

[Ben Bucksch (:BenB)]

Attachment: v12, utility functions

util.js, sanitizeDatatypes.js, fetchhttp.js, fetchConfig.js

Comment 67

[Ben Bucksch (:BenB)]

Attachment: v12, basic account creation functions

accountConfig.js, createInBackend.js, verifyConfig.js
(239 lines)

(utility functions above is 222 lines, BTW)

Comment 68

[Ben Bucksch (:BenB)]

Attachment: v12, emailWizard.xul, except middle part

(165 lines, trivial changes)

Comment 69

[Ben Bucksch (:BenB)]

Attachment: v12, emailWizard.xul, middle part

Many changes here, so just the new code

(228 lines)

Comment 70

[Ben Bucksch (:BenB)]

Attachment: v12, UI strings

Comment 71

[Ben Bucksch (:BenB)]

(175 lines of diff)

Comment 72

[Ben Bucksch (:BenB)]

Attachment: v12, guessConfig.js

(411 lines, of which almost 150 are just removing restart() and other cruft)

Comment 73

[Ben Bucksch (:BenB)]

Attachment: v12, emailWizard.js

The majority of this file changed, to not as diff, but as new file.
Some of the old code remained, so please be gentle on that.

As requested, I put markets in the file, with bunches of < 300 lines.

I tried to group them logically (it helped that I tried to organize the file logically anyways, which is part of the reason why the diff is so huge).

== REVIEW part 1 -- start, switchToMode() -- 300 lines
== REVIEW part 2 -- name/email/pw fields, findConfig() -- 266 lines
== REVIEW part 4 -- progress and result display -- 193 lines
== REVIEW part 5 -- manual edit, part 1 -- 168 lines
== REVIEW part 6 -- manual edit, part 2 -- 247 lines
== REVIEW part 7 -- utility and dialog finish -- 221 lines
== REVIEW part 8 -- SecurityWarningDialog -- 277 lines

Comment 74

[Ben Bucksch (:BenB)]

Happy meal! :)

Comment 75

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 442831 [details] [diff] [review]
v12, utility functions

>+++ b/mailnews/base/prefs/content/accountcreation/fetchConfig.js
>@@ -91,27 +91,34 @@ function fetchConfigFromISP(domain, emai
>         function(e2)
>         {
>           ddump("fetchisp 2 <" + url2 + "> took " + (Date.now() - time) +
>               "ms and failed with " + e2);
>-          errorCallback(e1); // return error for primary call
>+          // return error for primary call
>+          errorCallback(e2 instanceof CancelledException ? e2 : e1);

I think we should update the comment to something like:
Return the error for the primary call, unless the fetch was cancelled.

>+++ b/mailnews/base/prefs/content/accountcreation/fetchhttp.js
>@@ -201,37 +201,53 @@ FetchHTTP.prototype =
>     // Callbacks
>     if (success)
>-      this._successCallback(this.result);
>+    {
>+      try {
>+        this._successCallback(this.result);
>+      } catch (e) {
>+        logException(e);
>+        this._error(e);
>+      }
>+    }
>     else if (exStored)
>-      this._errorCallback(exStored);
>+      this._error(exStored);
>     else
>-      this._errorCallback(new ServerException(errorStr, errorCode, this._url));
>+      this._error(new ServerException(errorStr, errorCode, this._url));
> 
>     if (this._finishedCallback)
>-      this._finishedCallback(this);
>+    {
>+      try {
>+        this._finishedCallback(this);
>+      } catch (e) {
>+        logException(e);
>+        this._error(e);
>+      }
>+    }
> 
>     } catch (e) {
>-      // error in callback or our fetchhttp._response() code
>-      try
>-      {
>-        ddump("Error in errorCallback or _response(): " + e);
>-        this._errorCallback(e);
>-      } catch (e) {
>-        //ddump("Error in errorCallback: " + e);
>-        // XXX TODO FIXME BOGUS
>-        alert(e); // error in errorCallback, too!
>-        throw(e); // to error console
>-      }
>+      // error in our fetchhttp._response() code
>+      logException(e);
>+      this._error(e);
>+    }
>+  },
>+  _error : function(e)
>+  {
>+    try {
>+      this._errorCallback(e);
>+    } catch (e) {
>+      // error in errorCallback, too!
>+      logException(e);
>+      alertPrompt("Error in errorCallback for fetchhttp", e);
>     }
>   },

These are all similar enough that I think I'ld like to see them combined into a function like either:
  _attempt : function(cb, data)
  {
    try {
      cb(data);
    } catch (e) {
      logException(e);
      if (cb != this._errorCallback)
        this._attempt(this._errorCallback, e);
      else
        alertPrompt("Error in errorCallback for fetchhttp", e);
    }
  },

Or, if you'ld prefer,
  _attempt : function(cb, data, callError)
  {
    try {
      cb(data);
    } catch (e) {
      logException(e);
      if (callError)
        this._attempt(this._errorCallback, e, false);
      else
        alertPrompt("Error in errorCallback for fetchhttp", e);
    }
  },

(I think the first one makes sense, since you'll be calling the error callback unless you already were.)

>@@ -249,28 +265,38 @@ FetchHTTP.prototype =
>-extend(UserCancelledException, Exception);
>+extend(UserCancelledException, CancelledException);

Do we have non-User Cancelled Exceptions?

>+++ b/mailnews/base/prefs/content/accountcreation/util.js
>@@ -55,17 +55,18 @@ Cu.import("resource:///modules/errUtils.
> function assert(test, errorMsg)
> {
>   if (!test)
>-    throw new NotReached(errorMsg);
>+    throw new NotReached(errorMsg ? errorMsg :
>+          "Programming bug: assertion failed, see log");

Could this just be:
    throw new NotReached(errorMsg || "Assertion failed, see log");
(I shortened the error message partially because it's redundant, and partially to fit it in the 80 character limit.)

>@@ -191,16 +192,17 @@ Exception.prototype =
>   {
>     return this._message;
>   }
> }
> 
> function NotReached(msg)
> {
>   Exception.call(this, msg);
>+  logException(e);
> }

I don't see a definition for "e" in this scope.  Did you mean "logException(this);"?

The changes I've asked for are fairly mechanical, but that second one is kinda big, so I'm going to r- this so that I can take another look, but I'm generally happy with the new code here.

Thanks,
Blake.

Comment 76

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 442834 [details] [diff] [review]
v12, basic account creation functions

>+++ b/mailnews/base/prefs/content/accountcreation/accountConfig.js

This didn't apply cleanly for me, seemingly because we've added a "toUpperCase()" to the incoming and outgoing hostnames before the calls to _replaceVariable

>@@ -244,27 +244,24 @@ function replaceVariables(account, realn
>-  account.incoming.hostname =
>-    sanitize.hostname(_replaceVariable(account.incoming.hostname,
>-                                       otherVariables));
>-  if (account.outgoing.hostname) // will be null if user picked existing server.
>-    account.outgoing.hostname =
>-      sanitize.hostname(_replaceVariable(account.outgoing.hostname,
>-                                         otherVariables));
>+  account.incoming.hostname = _replaceVariable(account.incoming.hostname,
>+                                               otherVariables);
>+  account.outgoing.hostname = _replaceVariable(account.outgoing.hostname,
>+                                               otherVariables);

Why shouldn't we sanitize the hostnames?  That doesn't make a lot of sense to me, cause it seems like we're just hiding errors.

>+++ b/mailnews/base/prefs/content/accountcreation/createInBackend.js
>@@ -32,27 +32,25 @@
> function createAccountInBackend(config)
> {
>-  const Cc = Components.classes;
>-  const Ci = Components.interfaces;

Hmmm…  I thought we needed these when we ran the tests for some reason.  And they're not defined anywhere else in this file, so where do you think they come from, and are you sure they're always there?

Or, put another way, were they causing you problems, or could we leave them in?

>@@ -238,18 +236,77 @@ function rememberPassword(server, passwo
>+/**
>+ * Check whether the user's setup already has an incoming server
>+ * which matches (hostname, port, username) the primary one
>+ * in the config.
>+ * (We also check the email address as username.)

I think this would be clearer if you wrote it as:
/**
 * Check whether the user's setup already has an incoming server which
 * matches (hostname, port, username|email address) the primary one in
 * the config.

>+function checkOutgoingServerAlreadyExists(config)
[snip…]
>+    // TODO check username with full email address, too, like for incoming?

I think that makes the most sense.

>+/**
>+ * Check if there already is a "Local Folders". If not, create it.
>+ * Copied from AccountWizard.js with minor updates.
>+ */
> function verifyLocalFoldersAccount(am) 

Can we just merge these two functions?  Please?

>@@ -263,11 +320,10 @@ function verifyLocalFoldersAccount(am)
>-  catch (ex) {dump("Error in verifyLocalFoldersAccount " + ex + "\n");  }
>-
>+  catch (ex) { dump("Error in verifyLocalFoldersAccount " + ex + "\n");  }

If you're going to fix the indentation, we might as well get rid of the second space after the ";".  ;)

>+++ b/mailnews/base/prefs/content/accountcreation/verifyConfig.js
>@@ -276,38 +280,43 @@ urlListener.prototype =
> 
>   // Suppress any certificate errors
>   notifyCertProblem: function(socketInfo, status, targetSite) {
>     if (!status)
>       return true;
> 
>     this.mCertError = true;
>     this._log.error("cert error");
>-    setTimeout(this.informUserOfCertError, 0, socketInfo, targetSite, this);
>+    var me = this;

I prefer using "self", but I won't insist on the change.

>+    setTimeout(function () { // TODO why???

I'm pretty sure we do this so that the function can run immediately, but after we return, so that we're not blocking anything.  Or was that your question?

I don't think there's anything in here that I don't trust you to be able to fix on your own, so r=me with the nits fixed.

Later,
Blake.

Comment 77

[Ben Bucksch (:BenB)]

> This didn't apply cleanly for me

Surprise :)

> Why shouldn't we sanitize the hostnames?

Because they have been sanitized before, in readFromXML() and getUserConfig().

And it failed here when I used this to generate a stub config (with ".domain.tld" as server hostname) using placeholders (hostname = ".%EMAILDOMAIN%", username = ...) and replaceVariables(), so the check was disturbing, and had been a problem before, thus the toUpperCase() from Kohei, and served no real purpose.

> - const Cc = Components.classes;
> where do you think they come from?

util.js. None of the files here are guaranteed to work without util.js.

> +    // TODO check username with full email address, too, like for incoming?
> I think that makes the most sense.

OK, I'll remove the "?", but don't plan to implement it in this bug.

> > function verifyLocalFoldersAccount(am) 
> Can we just merge these two functions? Please?

With what? AccountWizard.js? I'm not going to import that here, or vise versa. We should probably use functions crom accountcreation/ in AccountWizard.js and AccountManager in the future, but that's for another patch.

Comment 78

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 442840 [details] [diff] [review]
v12, emailWizard.xul, except middle part

>+++ b/mailnews/base/prefs/content/accountcreation/emailWizard.xul
>@@ -137,46 +142,47 @@
>   <vbox id="mastervbox" class="mastervbox" flex="1">
>-    <groupbox id="initialSettings">
>+    <groupbox id="initialSettings" style="margin-bottom: 1em;">

The style should be moved out to a css file.

Other than that, looks good to me.  r=me for this part.

Later,
Blake.

Comment 79

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 442842 [details] [diff] [review]
v12, emailWizard.xul, middle part

I'm having a hard time trying to figure out where this goes in the emailWizard.xul file, but I'll see what I can do for a review.

>      <hbox align="center" pack="start">
>        <label class="autoconfigLabel"/>
>        <checkbox id="remember_password"
>                  label="&rememberPassword.label;"
>                  accesskey="&rememberPassword.accesskey;"
>                  checked="true"/>
>      </hbox>
>    </groupbox>
>    <spacer flex="1" />
>
>     <hbox id="status_area" style="margin-bottom: 1em;">

The style should be moved into a css file.

>      <vbox pack="start">
>        <image id="spinner"
>               src="chrome://global/skin/icons/loading_16.png"
>               hidden="true"/>
>      </vbox>
>      <description id="status_msg"
>                   flex="1" class="title"
>                   style="min-height: 2em;">&#160;</description>

The style should be moved into a css file.

>                   <!-- 160 = A0 is a non-breaking space, to make the element
>                   occupy the full height for at least one line. Without it,
>                   it has no height, an with a just normal space, it somehow
>                   has a smaller (not sufficient) height. -->

I think you could shorten that comment to:
                   <!-- Include 160 = nbsp, to make the element occupy the
                   full height.  With a normal space, it does not have
                   sufficient height. -->
and not lose any clarity.  (If you'ld rather not change it, then at least replace the "an" with an "and".)

>    </hbox>
>
>    <groupbox id="result_area" hidden="true" style="margin-bottom: 1.5em;">
>      <radiogroup id="result_imappop" orient="horizontal"
>          style="margin-bottom: 1.5em;"

Blah blah style blah blah css file.  ;)
(I won't mention this one anymore from here on out.)

>          onselect="gEmailConfigWizard.onResultIMAPOrPOP3();">

Line this up with the id=, please.

>        <radio id="result_select_imap" label="&imapLong.label;" value="1"/>
>        <radio id="result_select_pop3" label="&pop3Long.label;" value="2"/>
>      </radiogroup>
>      <hbox>
>        <label class="textbox-label" value="&incoming.label;"/>
>        <description id="result-incoming" flex="1">&#160;</description>

Do we really need the #160 here, since the incoming.label should give us a lines worth of height?

>    <groupbox id="manual-edit_area" hidden="true" style="margin-bottom: 2em;">

Hmmm…  I think I would like it more if it looked like the user could edit the results directly, instead of being popped into a different groupbox.  While I guess we could mimic that effect by putting the elements of the result-incoming description in the same order as the columns below, that does introduce the risk that we'll change things in one place, but not the other.  And I'm not sure how well it'll work with localized text.

>      <grid>
>        <columns>
>          <column/><column/><column flex="1"/><column/><column/><column/>
>        </columns>
>        <rows>
>          <row id="labels_row" align="center">
>            <spacer/>
>            <spacer/>

It might be nice to leave comments here telling us what the first two columns are for.

>            <label value="&hostname.label;" class="initialDesc"/><!--TODO new class -->

Put the comment on a new line, so that this is less than 80 characters.  Or better yet, do whatever we need to do for the new class.  ;)

>            <label value="&port.label;" class="initialDesc"/>
>            <label value="&ssl.label;" class="initialDesc"/>
>            <label value="&auth.label;" class="initialDesc"/>
>          </row>
>          <row id="incoming_server_area" align="center">
>            <label class="textbox-label"
>                   value="&incoming.label;"
>                   control="incoming_hostname"/>
>            <menulist id="incoming_protocol"
>                      value=""
>                      sizetopopup="always">
>              <menupopup
>                onpopuphidden="gEmailConfigWizard.onChangedIncomingProtocol();">

I don't get the 6-character indent here.  Why not use 2-characters, so that it fits in the 80-character limit?

>                <menuitem label="&imap.label;" value="1"/>
>                <menuitem label="&pop3.label;" value="2"/>
>              </menupopup>
>            </menulist>
>            <textbox id="incoming_hostname"
>                     value=""

If there is no value, why include the value element?

>                     style="width: 15em"
>                     oninput="gEmailConfigWizard.onInputHostname();"

I thought I remembered Bryan running into some times where the oninput didn't fire when we wanted it to, but I might be misremembering.

>                <menuitem label="&autodetect.label;" value="0"/>
>                <menuitem label="&noEncryption.label;" value="1"/>
>                <menuitem label="&starttls.label;" value="3"/>
>                <menuitem label="&sslTls.label;" value="2"/>

It does look kind of odd to have the numbers go 0,1,3,2, but I guess that's the right order.  Those numbers should correspond to constants in a different file, correct?  We might want to make a note of that.

>                <menuitem label="&autodetect.label;" value="0"/>
>                <!-- labels set from messenger.properties to avoid duplication -->
>                <menuitem id="in-authMethod-password-cleartext" value="3"/>
>                <menuitem id="in-authMethod-password-encrypted" value="4"/>
>                <menuitem id="in-authMethod-kerberos" value="5"/>
>                <menuitem id="in-authMethod-ntlm" value="6"/>

And where are these values from?  I'm a little worried that they start at 3, which leads me to believe that they're supposed to continue on from the previous menuitems, but we've got a duplicate entry for 3, which could lead to problems.

>            <menulist id="outgoing_authMethod"
>                      class="auth"
>                      value=""
>                      sizetopopup="always">
>              <menupopup onpopuphidden="gEmailConfigWizard.onChangedAuth();">

Why doesn't this method differentiate between Incoming and Outgoing, like the SSL method does?  Alternatively, since this method, and many others, just call gEmailConfigWizard.onChangedManualEdit(), why bother having these functions at all?

And all of that leads me to a slightly bigger question.  If we autodetect a config, and the user clicks "manual edit", do we fill in everything with the values we detected, or with "autodetect"?  And if we fill them in with autodetect, do we just re-test the previous values, or do we force them to go through the tedious and time consuming step of re-probing?  And what if they change the incoming and/or outgoing host to one we know the values for, like gmail.com?  Do we re-fetch the settings from MoMo's autoconfig?

>                <menuitem label="&autodetect.label;" value="0"/>
>                <!-- labels set from messenger.properties to avoid duplication -->
>                <menuitem id="out-authMethod-no" value="1"/>

Why isn't there a no-auth option for the incoming server?
And what would value="2" correspond to?

>                <menuitem id="out-authMethod-password-cleartext" value="3"/>
>                <menuitem id="out-authMethod-password-encrypted" value="4"/>
>                <menuitem id="out-authMethod-kerberos" value="5"/>
>                <menuitem id="out-authMethod-ntlm" value="6"/>

Which one of those corresponds to CRAM-MD5, by the way?

>          <row id="labels_row" align="center">

I'm fairly sure having two elements with the same id is a bad thing.

>            <label class="textbox-label"
>                   value="&username.label;"
>                   control="incoming_username"/>
>            <spacer/>
>            <textbox id="incoming_username"

Shouldn't that be outgoing_username?  And if not, how will this handle different incoming and outgoing usernames, which are allowed in the XML spec?

>    <hbox id="buttons_area">
>      <hbox align="center" pack="start">
>        <button id="manual-edit_button"
>                label="&manual-edit.label;"
>                accesskey="&manual-edit.accesskey;"
>                class="smaller-button"
>                hidden="true"
>                oncommand="gEmailConfigWizard.onManualEdit();"/>
>        <button id="advanced-setup_button"
>                label="&advancedSetup.label;"
>                accesskey="&advancedSetup.accesskey;"
>                class="smaller-button"
>                disabled="true"
>                hidden="true"
>                oncommand="gEmailConfigWizard.onAdvancedSetup();"/>

I wonder if we really need two different buttons anymore.
If so, I'm guessing that the Advanced Setup should really be something more like "Create Account with these Settings, and Allow me to Modify It.", or something a little less wordy.  ;)

>      <!--<spacer flex="1"/>
>      <hbox align="center" pack="center">
>      </hbox>-->

Please just remove the code instead of commenting it out.

>      <hbox align="center" pack="end">
>        <button id="stop_button"
>                label="&stop.label;"
>                accesskey="&stop.accesskey;"
>                class="smaller-button"
>                hidden="true"
>                oncommand="gEmailConfigWizard.onStop();"/>
>        <button id="cancel_button"
>                label="&cancel.label;"
>                class="smaller-button"
>                accesskey="&cancel.accesskey;"
>                oncommand="gEmailConfigWizard.onCancel();"/>
>        <button id="half-manual-test_button"
>                label="&half-manual-test.label;"
>                accesskey="&half-manual-test.accesskey;"
>                class="larger-button"
>                hidden="true"
>                oncommand="gEmailConfigWizard.onHalfManualTest();"/>

Really?  "half-manual-test"?  Can't we come up with a better name for that?  Give me a description of what it does, and I'll take a shot.

>        <button id="next_button"
>                label="&continue.label;"
>                accesskey="&continue.accesskey;"

Perhaps we should take this opportunity to make the next button use the next.label, and the next.accesskey.

>        <button id="create_button"
>                label="&createAccount.label;"
>                accesskey="&createAccount.accesskey;"
>                class="larger-button"
>                hidden="true"
>                oncommand="gEmailConfigWizard.onOK();"/>

Ditto the create button with the create.label, and the gEmailConfigWizard.onCreate function.

Apart from that, r=…  Wait, what am I saying, I complained way too much to give it an r+.  ;)  I do look forward to seeing the next version of this patch, though.

Thanks,
Blake.

Comment 80

[Ben Bucksch (:BenB)]

> I think I would like it more if it looked like the user could edit the
> results directly, instead of being popped into a different groupbox.

That's the whole idea of this bug: The result display is just a *display*, and not at all editable or even widgets at all. It's just us informing the user what we found and what we'll create. The style in the result is aimed at getting the information which is critical for security straight to the user, with minimal clutter, visual business and distraction. It should be mostly readable by "mom and dad".

The manual config, OTOH, is for people who already know the mail server config, and put all the technical details in manually. It's a totally different user group. It's also different information needed: e.g. the authentication, the port fields etc.

In the result, we don't even need to show the port, not even the Port textfield, if it's the standard port. Similarly, the authentication method won't be understood by most users, and doesn't need to be mentioned, given that we'll explicitly and boldly warn about non-SSL connections. So, for the result screen, we only need to show:
IMAP, imap.orange.fr, SSL
SMTP, smtp.orange.fr, SSL
fred
That's a *whole* lot less than what we need to show in manual config.

On the coding side, that also makes things *considerably* easier to not have to deal with different modes of the widgets, as the old dialog does. That's what led to all the state bugs, and what this bug fixes. OTOH, of course I don't want to duplicate all widgets either.
For both reasons - simpler to implement as text, and easier to read for user and less mental burden for him, I chose this approach of a separate manual config screen (aimed technical users) and an absolutely minimal result screen (aimed at average users).

Comment 81

[Ben Bucksch (:BenB)]

> I'm having a hard time trying to figure out where this goes in the
> emailWizard.xul file

See the previous review chunk "v12, emailWizard.xul, except middle part" (diff):
 ============
 The middle part has too many changes and is in emailWizardxul-middle.txt
 ============

> The style should be moved into a css file.

Yes.

> Those numbers should correspond to constants in a different file, correct?

Yes, in nsMsgSocketType and nsMsgAuthMethod
<http://mxr.mozilla.org/comm-central/source/mailnews/base/public/MailNewsTypes2.idl>
That should also explain your other comments about the numbers.
I'll add an XML comment.

> If we autodetect a config, and the user clicks "manual edit", do we
> fill in everything with the values we detected, or with "autodetect"?

We prefill the config we found.

> what if they change the incoming and/or outgoing host to one we know
> the values for, like gmail.com?  Do we re-fetch the settings from
> MoMo's autoconfig?

No, once in manual mode, the user takes control.
All we do in manual mode is:
- If we and/or the user left any fields at "Auto", and the user clicks
  [Re-test], fill them in.
- Check the config (on [Re-Test] and [Create]) whether it works at all.

(For manual config mode:)
> Really?  "half-manual-test"?  Can't we come up with a better name for that? 
> Give me a description of what it does, and I'll take a shot.

Values that the user entered will be accepted as-is. (This is manual mode. The user said so, we'll do it. Yes, sir! But we'll still test whether they work at all.)
Values which are left at "Auto" (or empty) will be probed, on request by button click.

> I'm guessing that the Advanced Setup should really be something more
> like "Create Account with these Settings, and Allow me to Modify It.", or
> something a little less wordy.  ;)

Good idea. I'd call it "Create unverified and modify advanced settings", but that's not any shorter :).
It is the mode where
1) we take the hands of entirely, don't even check whether it will work
   (may be broken config)
2) the user can edit fields like POP keep/delete on server, IMAP Root
   folder, IMAP Sent folder name etc., which we can't all replicate in the
   wizard.

Comment 82

[Ben Bucksch (:BenB)]

Blake, any progress with the further patches, esp. emailWizard.js? (Note the
"== REVIEW" markers, as requested)

Comment 83

[Ben Bucksch (:BenB)]

(see comment 73 for overview)

Comment 84

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #82)
> Blake, any progress with the further patches, esp. emailWizard.js? (Note the
> "== REVIEW" markers, as requested)

Yeah, I got caught up in some other stuff, but I plan to get back to it soon.

Thanks,
Blake.

Comment 85

[Ben Bucksch (:BenB)]

ping :-((

Comment 86

[Blake Winton (:bwinton) (:☕️)]

Yup, they're in my queue.  I haven't forgotten about them.

Comment 87

[Ben Bucksch (:BenB)]

I thought we agreed one 300-line chunk per day.

Comment 88

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 442844 [details] [diff] [review]
v12, UI strings

>+++ b/mail/locales/en-US/chrome/messenger/accountCreation.dtd
>@@ -5,52 +5,58 @@
>+<!ENTITY pop3.label                       "POP3">

One space too many of indent here.

>-<!ENTITY sslTls.label                    "SSL/TLS">
>+<!ENTITY sslTls.label                    "Normal SSL">

What do you mean by "Normal SSL"?  Also, couldn't this be TLS, which isn't _really_ SSL?

>-<!ENTITY manualSetup.label               "Manual Setup…">
>-<!ENTITY manualSetup.accesskey           "S">
>+<!ENTITY advancedSetup.label             "Advanced config">

I think this should continue to have the elipsis, since it opens up a new dialog.

>+<!ENTITY manual-edit.label               "Manual config">
>+<!ENTITY manual-edit.accesskey           "M">

How, other than accesskey, does this differ from the Advanced config above?

>+
>

I think we could do without two blank lines here.

>+++ b/mail/locales/en-US/chrome/messenger/accountCreation.properties
>@@ -4,32 +4,38 @@
>+looking_up_settings_halfmanual=Looking up configuration: Probing server

I'm really not liking the term "halfmanual" (which should probably be "half_manual" in this case).  Surely we can come up with something less confusing.  Something like "probing", maybe, which is the term you use several times in the text itself.

>@@ -50,15 +56,42 @@ password_ok=Password ok!
>+#config result display
>+# LOCALIZATION NOTE(resultUnknown): Displayed instead of resultIncoming,
>+# resultOutgoing or resultUsername when we don't have a proper value.
>+resultUnknown=Unknown

I'ld like to get a translator's comment on whether we can use the same term here for these three things, or whether they need different terms.  (For example, if username is femine, but incoming servers are masculine.) 

>+# LOCALIZATION NOTE(resultIncoming):
>+# %1$S will be replaced with either resultIMAP, resultPOP3 or resultSMTP.
>+# %2$S will be replaced with the server hostname
>+#   with possibly a port appended as ":"+port.
>+#   The domain part may be made bold.
>+# %3$S will be replaced with either resultNoEncryption or resultSSL or
>+#    resultSTARTTLS.
>+# %4$S will be replaced with either resultSSLCertWeak or resultSSLCertOK
>+#    (which should normally be empty)
>+# You may adjust the strings to be a real sentence.

I like this.  Nice, explanatory, good suggestion at the end.

>+resultNoEncryption=no encryption
>+resultSSL=SSL
>+resultSTARTTLS=STARTTLS

It looks a little odd to me to have "no encryption" be entirely in lowercase, but SSL and STARTTLS be entirely in uppercase.

>+# LOCALIZATION NOTE(resultSSLCertWeak): \u0020 is just a space
>+resultSSLCertWeak=\u0020(Warning: Could not verify server)

Okay, why is it there, then?

r=me, if a translator says it should be fine, and with halfmanual changed, and the rest of the nits addressed.

Later,
Blake.

Comment 89

[Ben Bucksch (:BenB)]

> What do you mean by "Normal SSL"?

Not STARTTLS.

> Also, couldn't this be TLS, which isn't _really_ SSL?

Well, TLS is just the new technical name for the current version of SSL, but almost nobody uses it like this.
Most people use TLS == STARTTLS, which is wrong.
That's why I avoid "TLS" entirely and just use "Normal SSL" and "STARTTLS".
I say "Normal SSL", not just "SSL", because "STARTTLS" is also SSL.

> >+<!ENTITY advancedSetup.label             "Advanced config">

> I think this should continue to have the elipsis, since it opens up
> a new dialog.

Yes. But it also closes the current dialog, i.e. with no way to go back. And more importantly, it immediately creates the account, before we open the Account Manager dialog.
I think the ellipsis denotes a command that does not execute immediately, but shows a dialog first which then, after confirmation, executes some change.
Here, we do something significant (creating the account permanently) immediately when the user clicks the button. That's why I removed the ellipsis as an (ever so subtle) hint.

> >+<!ENTITY manual-edit.label               "Manual config">
> >+<!ENTITY manual-edit.accesskey           "M">

> How, other than accesskey, does this differ from the Advanced config above?

Yes! "Manual config" is the mode in our dialog where you enter hostname and port etc., and can retest etc. "Advanced config" creates the account immediately and sends you to Account Manager, where you can modify further details like IMAP delete model, Sent folder etc..
"Manual config" is for cases where we can't autodetect settings or detect the wrong ones and need the user to enter hostnames etc..
"Advanced config" is for cases where the user knows he needs to change some other, advanced option that we can't offer here.

Comment 90

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 442845 [details] [diff] [review]
v12, guessConfig.js

>+++ b/mailnews/base/prefs/content/accountcreation/guessConfig.js
>@@ -203,114 +198,66 @@ function guessConfig(domain, progressCal
> GuessAbortable.prototype =
> {
>+  cancel : function(ex)
>   {
>-    switch (which)
>-    {
>-      case "incoming":
>-      default:
>-        if (this._incomingHostDetector)
>-          this._incomingHostDetector.cancel();
>-      case "outgoing":
>-        if (which != "incoming")
>-        {
>-          if (this._outgoingHostDetector)
>-            this._outgoingHostDetector.cancel();
>-          outgoingDone = true;
>-        }
>-    }
>+    this._incomingHostDetector.cancel(ex);
>+    this._outgoingHostDetector.cancel(ex);
>   },

So, I don't think this is quite right.  It looks like we call cancel from _checkFinished, which would cancel both the incoming and outgoing host detectors if either of them finished.

Later,
Blake.

Comment 91

[Ben Bucksch (:BenB)]

Comment on attachment 442845 [details] [diff] [review]
v12, guessConfig.js

> It looks like we call cancel from _checkFinished,
> which would cancel both the incoming and outgoing host

I think you confused the 2 cancel() functions. _checkFinished() is a function of the HostDetector, and it calls this.cancel(), i.e. HostDetector.cancel().
The cancel() function you cite is on GuessAbortable. This is mainly for the caller (whoever called guessConfig()), and e.g. when the user cancels, directly or indirectly.

Resetting to r? for reconsideration.

Comment 92

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 442845 [details] [diff] [review]
v12, guessConfig.js

Sure, whatever.

You're going to need to post a new patch anyways, and I'll be checking through all the comments again when I review that one, so I don't _really_ care what this piece gets marked as.

Later,
Blake.

Comment 93

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 442854 [details] [diff] [review]
v12, emailWizard.js

REVIEW part 1:

>/*********************
>TODO for bug 549045
>- autodetect protocol
>Polish
>- margins into style sheet
>- bold status
>- remove status when user edited in manual edit
>- put protocol dropdown back after hostname?
>- add and adapt test from bug 534588
>Bugs
>- SSL cert errors
>  - invalid cert (hostname mismatch) doesn't trigger warning dialog as it should
>  - accept self-signed cert (e.g. imap.mail.ru) doesn't work (works without my patch), verifyConfig.js line 124 has no inServer, for whatever reason, although I didn't change verifyConfig.js at all (the change you see in that file is irrelevant: that was an attempt to fix the bug and clean up the code).

These should all be fixed.  After all, we've got the time to fix them now, before the next deadline.

Also, that last line is more than 80 characters.

>Things to test (works for me):
>- state transitions, buttons enable, status msgs
>  - stop button
>    - showes up again after stopping detection and restarting it
>    - when stopping [retest]: buttons proper?
>  - enter nonsense domain. guess fails, (so automatically) manual, change domain to real one (not in DB), guess succeeds. former bug: goes to manual first shortly, then to result

We should add automated tests for these, since I don't think these descriptions are good enough to turn into Litmus tests.  (They're missing the steps, and the expected results.)

>kDebug = true; // TODO remove

We should remove this.

>function _hide(id)
>function _show(id)
>function _enable(id)
>function _disable(id)

Blank lines between these functions, please.

>function setText(id, value)
>{
>  var element = document.getElementById(id);
>  if (!element)
>    return;

I think if you're asking to set the text of a non-existant element, we should throw an exception, because that seems like programmer error.

>  if (element.localName == "textbox" || element.localName == "label")
>    element.value = value;
>  else if (element.localName == "description")
>    element.textContent = value;

Why not just set the value of the description, too?  Or why not set the textContent of the textbox and label?

>  else
>    throw new NotReached("XUL element type not supported");
>}

I'm also a little worried that this is limiting ourselves to XUL elements, and when all of Thunderbird gets rewritten in HTML, we won't be able to use this code.  ;)

>function setLabelFromStringBundle(elementID, stringName)
>{
>  document.getElementById(elementID).label =
>      gMessengerBundle.getString(stringName);
>};

I would prefer seeing you pass in the bundle, since you make shockingly similar calls with a different bundle in a bunch of other places.  (Or just get rid of the function, since it doesn't really make the code any easier for me to read.)

>EmailConfigWizard.prototype =
>  onLoad : function()
>     * This is the config we got either from the XML file or from guessing
>     * or from the user. Unless it's from the user, it contains placeholders
>     * like %EMAILLOCALPART% in username and other fields.

This is confused me for a while.  I think it would be better stated as "this._currentConfig is the config we got…".  Also, I don't believe we stop the user from entering their name as "%EMAILLOCALPART%"…

>    this._currentConfig = null;
>    this._domain = "";
>    this._email = "";
>    this._realname = "";
>    this._password = "";

So, why have these, instead of just storing them in the _currentConfig?

>    this._verifiedConfig = false;
>
>    this._okCallback = null;

This should be part of the preceeding block, with the blank line coming after.

>    // Add the entry for the new host to the menulist
>    let menuitem = menulist.insertItemAt(0, "", "-new-"); // pos,label,value
>    menuitem.serverKey = null;

What will happen if we don't have an outgoing server, because we stopped the auto-detection?  Also, what would happen if the outgoing server was "mail.%EMAILDOMAIN%"?  Finally, do we need to set the serverKey to null here, or would leaving it as undefined work just as well?

>    document.getElementById("mastervbox").setAttribute("style", "min-width: " + document.width + "px");
>    document.getElementById("mastervbox").setAttribute("style", "min-height: " + document.height + "px");

I'm sure we can split these two lines up to less than 80 characters each.

>  /**
>   * Changes the window configuration to the different modes we have.
>   * Shows/hides various window parts and buttons.
>   * @param modename {String-enum}
>   *    "start" : Just the realname, email address, password fields
>   *    "find-config" : detection step, adds the progress message/spinner
>   *    "result" : We found a config and display it to the user.
>   *       The user may create the account.
>   *    "manual-edit" : The user wants (or needs) to manually enter their
>   *       the server hostname and other settings. We'll use them as provided.
>   * Additionally, there are the following sub-modes which can be entered after
>   * you entered the main mode:
>   *    "manual-edit-have-hostname" : user entered a hostname for both servers
>   *        that we can use
>   *    "manual-edit-testing" : User pressed the [Re-test] button and
>   *         we're currently detecting the "Auto" values
>   *    "manual-edit-complete" : user entered (or we tested) all necessary
>   *         values, and we're ready to create to account

How is this state different from the result state?

>   * Currently, this doesn't cover the warning dialogs etc.. It may later.
>   */
>  switchToMode : function(modename)
>  {
>    if (modename == this._currentModename)
>      return;

I would suggest that that's programmer error, and should be turned into an exception.  (Or at least an assert.)

>    //_show("initialSettings"); always visible
>    //_show("cancel_button"); always visible
>    if (modename == "start")
>    {
>      _hide("status_area");
>      _hide("result_area");
>      _hide("manual-edit_area");
>
>      _show("next_button");
>      _disable("next_button"); // will be enabled by code
>      _hide("half-manual-test_button");
>      _hide("create_button");
>      _hide("stop_button");
>      _hide("manual-edit_button");
>      _hide("advanced-setup_button");
>    }
>    else if (modename == "find-config")
>    {
>      _show("status_area");
>      _hide("result_area");
>      _hide("manual-edit_area");
>
>      _show("next_button");
>      _disable("next_button");
>      _hide("half-manual-test_button");
>      _hide("create_button");
>      _show("stop_button");
>      this.onStop = this.onStopFindConfig;
>      _show("manual-edit_button");
>      _hide("advanced-setup_button");
>    }
>    else if (modename == "result")
>    {
>      _show("status_area");
>      _show("result_area");
>      _hide("manual-edit_area");
>
>      _hide("next_button");
>      _hide("half-manual-test_button");
>      _show("create_button");
>      _hide("stop_button");
>      _show("manual-edit_button");
>      _hide("advanced-setup_button");
>    }
>    else if (modename == "manual-edit")
>    {
>      _show("status_area");
>      _hide("result_area");
>      _show("manual-edit_area");
>
>      _hide("next_button");
>      _show("half-manual-test_button");
>      _disable("half-manual-test_button");
>      _show("create_button");
>      _disable("create_button");
>      _hide("stop_button");
>      _hide("manual-edit_button");
>      _show("advanced-setup_button");
>      _disable("advanced-setup_button");
>    }
>    else if (modename == "manual-edit-have-hostname")
>    {
>      _show("status_area");
>      _hide("result_area");
>      _show("manual-edit_area");
>      _hide("manual-edit_button");
>      _hide("next_button");
>      _show("create_button");
>
>      _show("half-manual-test_button");
>      _enable("half-manual-test_button");
>      _disable("create_button");
>      _hide("stop_button");
>      _show("advanced-setup_button");
>      _disable("advanced-setup_button");
>    }
>    else if (modename == "manual-edit-testing")
>    {
>      _show("status_area");
>      _hide("result_area");
>      _show("manual-edit_area");
>      _hide("manual-edit_button");
>      _hide("next_button");
>      _show("create_button");
>
>      _show("half-manual-test_button");
>      _disable("half-manual-test_button");
>      _disable("create_button");
>      _show("stop_button");
>      this.onStop = this.onStopHalfManualTesting;
>      _show("advanced-setup_button");
>      _disable("advanced-setup_button");
>    }
>    else if (modename == "manual-edit-complete")
>    {
>      _show("status_area");
>      _hide("result_area");
>      _show("manual-edit_area");
>      _hide("manual-edit_button");
>      _hide("next_button");
>      _show("create_button");
>
>      _show("half-manual-test_button");
>      _enable("half-manual-test_button");
>      _enable("create_button");
>      _hide("stop_button");
>      _show("advanced-setup_button");
>      _enable("advanced-setup_button");
>    }

These big if/else statements are kind of tedious.  I think we should replace them with a table, so that we have less chance of making typos on the values we pass in.
modes = {
           // status, result, manual,  next,  test, create,  stop,  edit, setup
    "start": [ _hide,  _hide,  _hide, _show, _hide,  _hide, _hide, _hide, _hide],    "find-config": […],
    …
};

Later,
Blake.

Comment 94

[Ben Bucksch (:BenB)]

(In reply to comment #93)
> We should add automated tests for these

I agree that would be useful.
I may add mozmill tests once they work reliably and nice on Linux. Currently, that testsuite is unusable.

> >function setText(id, value)
...
> >  if (element.localName == "textbox" || element.localName == "label")
> >    element.value = value;
> >  else if (element.localName == "description")
> >    element.textContent = value;
> 
> Why not just set the value of the description, too?  Or why not set the
> textContent of the textbox and label?

Because that's how labels and descriptions work.
It's <label value="foo"/> and <description>foo</description>.
If a <description value="foo"/> doesn't wrap, so it's the same as <label>. Likewise label vise versa.

> when all of Thunderbird gets rewritten in HTML

Say again?

I surely hope you're joking.

> >function setLabelFromStringBundle(elementID, stringName)
> >{
> >  document.getElementById(elementID).label =
> >      gMessengerBundle.getString(stringName);
> >};
> 
> I would prefer seeing you pass in the bundle

I call this over a dozen times in row, see caller. The point of the function is to shorten these dozen lines to one a single line each and thus make them more readable. I just didn't want to have these silly setters all be 2-3 lines long.

> I don't believe we stop the user from entering their name as "%EMAILLOCALPART%"…

Yeah, right. So what?

> >    this._currentConfig = null;
> >    this._domain = "";
> >    this._email = "";
> >    this._realname = "";
> >    this._password = "";
> 
> So, why have these, instead of just storing them in the _currentConfig?

The text explains that.

> >    // Add the entry for the new host to the menulist
> >    let menuitem = menulist.insertItemAt(0, "", "-new-"); // pos,label,value
> >    menuitem.serverKey = null;
> 
> What will happen if we don't have an outgoing server, because we stopped the
> auto-detection?  Also, what would happen if the outgoing server was
> "mail.%EMAILDOMAIN%"?  Finally, do we need to set the serverKey to null here,
> or would leaving it as undefined work just as well?

I don't really know what you mean, but:
Self-Reminder: Test.

> >   *    "manual-edit-complete" : user entered (or we tested) all necessary
> >   *         values, and we're ready to create to account
> 
> How is this state different from the result state?

That the manual edit fields are displayed.

> >  switchToMode : function(modename)
> >  {
> >    if (modename == this._currentModename)
> >      return;
> 
> I would suggest that that's programmer error

No, it's not. I do that a lot to ensure we're in a certain mode, no matter which mode we're in. Most of the time, it will be already in that mode, so that's just a short-cut.

> These big if/else statements are kind of tedious.  I think we should replace
> them with a table

> modes = {
>            // status, result, manual,  next,  test, create,  stop,  edit, setup
>     "start": [ _hide,  _hide,  _hide, _show, _hide,  _hide, _hide, _hide,
> _hide],    "find-config": […],

I'm not sure that that's easier to read, but I may give it a try.

Comment 95

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #94)
> (In reply to comment #93)
> > We should add automated tests for these
> I agree that would be useful.
> I may add mozmill tests once they work reliably and nice on Linux. Currently,
> that testsuite is unusable.

Really?  Cause http://tinderbox.mozilla.org/showbuilds.cgi?tree=Thunderbird doesn't show any problems…  (We've done a lot of work recently cleaning up the tests, so if you haven't tried them in the last, say, 12 hours, you might want to try again.)

> > >function setText(id, value)
> ...
> > >  if (element.localName == "textbox" || element.localName == "label")
> > >    element.value = value;
> > >  else if (element.localName == "description")
> > >    element.textContent = value;
> > 
> > Why not just set the value of the description, too?  Or why not set the
> > textContent of the textbox and label?
> 
> Because that's how labels and descriptions work.
> It's <label value="foo"/> and <description>foo</description>.
> If a <description value="foo"/> doesn't wrap, so it's the same as <label>.
> Likewise label vise versa.

So, why don't we want the labels to wrap?  (Also, that should really go in a comment.  I knew what you were going for, but only because I banged my head against it before.)

> > when all of Thunderbird gets rewritten in HTML
> Say again?
> I surely hope you're joking.

Yup.  Did I forget to include the smiley?  ;)

> > >function setLabelFromStringBundle(elementID, stringName)
> > >{
> > >  document.getElementById(elementID).label =
> > >      gMessengerBundle.getString(stringName);
> > >};
> > I would prefer seeing you pass in the bundle
> I call this over a dozen times in row, see caller. The point of the function is
> to shorten these dozen lines to one a single line each and thus make them more
> readable. I just didn't want to have these silly setters all be 2-3 lines long.



> > I don't believe we stop the user from entering their name as "%EMAILLOCALPART%"…
> Yeah, right. So what?

So the comment is slightly off.  It suggests that we would only see the placeholders if the config didn't come from the user, but we could actually see them anytime.

> > >    this._currentConfig = null;
> > >    this._domain = "";
> > >    this._email = "";
> > >    this._realname = "";
> > >    this._password = "";
> > So, why have these, instead of just storing them in the _currentConfig?
> The text explains that.

Which text?  I didn't get an explanation out of the comment above, so perhaps it could be made more clear…

> > >  switchToMode : function(modename)
> > >  {
> > >    if (modename == this._currentModename)
> > >      return;
> > I would suggest that that's programmer error
> No, it's not. I do that a lot to ensure we're in a certain mode, no matter
> which mode we're in. Most of the time, it will be already in that mode, so
> that's just a short-cut.

But aren't the state transitions fairly well defined?  We should know which state we're coming from, and which state we're going to, and if they're the same, why is that button even shown?

Later,
Blake.

Comment 96

[Ben Bucksch (:BenB)]

> So, why don't we want the labels to wrap? 

Because that's the main difference between <label> and <description>:
label doesn't wrap, descr does.

> Which text?  I didn't get an explanation out of the comment above, so perhaps
> it could be made more clear…

The comment above _currentConfig . Basically, _currentConfig is potentially with placeholders, and intentionally and necessarily so (and the comment explains why). That's why we need the realname etc. stored separately, so that we can fill them in.

> But aren't the state transitions fairly well defined?

There is code which is called from 2 different modes: within the mode, and from another mode. If within the mode, we don't need to change it. If we're coming from another mode, we have to change it to a certain mode.
The alternative would be to make 2 function that only differ in having the _switchMode() line or not. I think the unconditional switchMode() and then having the shortcut conditional is both nicer, and safer.

Comment 97

[Ben Bucksch (:BenB)]

Blake: Ping reviews.

Comment 99

[Nik Conwell]

Will these bug fixes fix the following I'm encountering on TB 3.1.1 on Mac OS?

Account wizard adding an account for [e-mail] gets stuck on Incoming: bu.edu POP 110.  If I let the wizard discover various servers and then press the Stop button (which is still clickable) nothing happens and I can't change the protocol away from POP (POP does not turn into a menu item and "Manual Setup..." doesn't let me change from POP to IMAP.  The Error console has nothing logged.

Note that if I press the Stop button pretty much immediately it does change the POP protocol text to a menu item.

Comment 101

[Matt]

Could the enhancement in bug 531099 be included in this patch?

Comment 104

[Ben Bucksch (:BenB)]

Had a long phone meeting with Bryan Clark and David Eaves. We went over the screenshots at the top, and Byran's mockups. It was very productive.

Here are the changes needed, in comparison to my screenshots above:
- start screen
  attachment 434068 [details]
  https://wiki.mozilla.org/File:Thunderbird-Autoconfiguration-Initial_View.png
  - cancel button
    - make it same size as Continue button
    - put it directly to the left of the Continue button
  none-changes:
  - "skip to manual config" stays out.
    I believe the main reason for the user complaints is that the
    old wizard's stop etc. doesn't work right, and I'm fixing that here.
    And we often know a better config than even users who know a working
    config, e.g. with SSL. And they can still trivially stop the detection
    by clicking "Manual config" later.
    If after 3.2, users still complain, we can still revisit this decision.
  - text field labels stay on the left.
- detection screen (progress bar)
  attachment 434077 [details]
  https://wiki.mozilla.org/File:Thunderbird-Autoconfiguration-Auto-Configurating.png
  - Move text and spinner to the horizontal center of the screen
  - "Stop detection" and "manual config" buttons are moved to the
    right of the progress text.
  none-changes:
  - "manual config" stays a button, not a link (style), same elsewhere.
  - no indeterminate progress bar, but we keep spinner.
- result screen
  attachment 437846 [details]
  https://wiki.mozilla.org/File:Thunderbird-Autoconfiguration-Auto-Configured.png
  That's where we differ most.
  My feedback to Bryan: I consider bryan's mockup to visually "busy",
  to hard to process mentally. Esp. given that it's just a confirmation and
  the only choice is IMAP or POP.
  Username: I prefer not to repeat the username for incoming and outgoing,
  but only show both when they are actually different. He prefers to show
  them both to make users of guessed configs aware that they might need
  to change this.
  IMAP/POP3 hostnames: not needed to show both, adds to clutter.
  Bryan's feedback to mine: Bryan doesn't like that things don't align
  in my screenshot, it would be better to be a grid. I agreed on that.
  We then both moved towards each other.
  Mine:
  - change to a grid: label, protocol, hostname, encryp.
  - replace "no encryption" and "SSL" / "STARTTLS" to icons
  - make a mockup where the username is shown for both, in the 5th column
    (makes them directly underneath each other, which makes mental
    processing much easier.) The other mockup will keep the username as-is.
  Bryan:
  - remove the POP3 server hostname line
  - remove the group boxes
  - then play around how to make this nice and visually easy to parse
  Hopefully, both will then more or less convene.
- manual config screen
  attachment 437446 [details]
  https://wiki.mozilla.org/images/6/61/Thunderbird-Autoconfiguration-Sport_Configuring.png
  - same issues as result screen, should tackle afterwards
  - my screen is actually fairly close to the 3.1 screen (latter shown in
    screenshot attachment 437448 [details])
  - retest button renamed to "test" and moved to the right, close to the
    form fields
  - note: most values are "auto" by default, and "test" or "create account"
    will autodetect them and then fill them out. Values which are filled
    out by the user will *not* be changed by "test", but accepted as-is.

Bryan, does that reflect your view? Do you have corrections/additions?

Comment 105

[Matt]

(In reply to comment #104)
>   none-changes:
>   - "skip to manual config" stays out.
>     I believe the main reason for the user complaints is that the
>     old wizard's stop etc. doesn't work right, and I'm fixing that here.
>     And we often know a better config than even users who know a working
>     config, e.g. with SSL. And they can still trivially stop the detection
>     by clicking "Manual config" later.
>     If after 3.2, users still complain, we can still revisit this decision.
 
Does not user choice come into this at all?   Perhaps you do have a 'better' set of config settings than the user, but the user need a choice in the process.  The decision to convert all new accounts to IMAP if possible has worked for many but has been a nightmare for those that did not want to use IMAP.  Here we are almost a year down the track fighting off allowing the user to step directly into a manual setup if they so choose.

I can not follow the workflows on this thread, but any relience of the stop button to bypass auto-detection of account settings is ludicrous. It currently appears for mere seconds and users are have been resorting to 'pulling the plug' to manage to click stop (now the new bug caused by pulling the plug).  Lets see this item fixed once and for all without the view that the development/planning group knows more than the user.  Whilst many users as clueless, the ones that are not are aggravated by this hand holding which is getting in the way of their productivity.

Comment 106

[Matt]

http://getsatisfaction.com/mozilla_messaging/topics/new_account_setup_wizard_is_too_aggressive

Comment 107

[Reindl Harald]

The way this wizard starts and the attitude here is simply unacceptable.

As advanced user the first question has to be "do you want manually or automatic setup", it may be ok that automatic is default but it has to start after "OK"-Click and no second before.

The automatic and IMAp-Default ends up in users who does not know about imap/pop3 are ending in imap and endless mailboxes on the server without understanding what happens behind.

Finally they are not able to change to pop3 months later and in the worst-case they have more than one computer, using outlook on another one with pop3 and fetching thousands of mails after setup.

Automatic wizards are fine but they have NEVER to start without confirmation, again NEVER

Comment 108

[Ben Bucksch (:BenB)]

> The way this wizard starts

... will change a lot to the better here. It's pointless to comment without having tried it out. Please wait how this will work before drawing conclusions based on the current wizard.

> [the stop button] currently appears for mere seconds and users are
> have been resorting to 'pulling the plug' to manage to click stop
> (now the new bug caused by pulling the plug). 

I know, that's precisely what this bug fixes. I've been working hard on that.

There will be a "manual config" button, and it will actually work, and it will change into the former "Edit" mode, not go into the Account Manager with random hostname and POP/IMAP as the current wizard.

unicorn wrote:
> The decision to convert all new accounts to IMAP if possible

If there is POP available, we offer it, even with detection. If we for some reason don't know about a POP option, you can still manually config it. We're not taking any options away.

Comment 109

[Matt]

(In reply to comment #108)
Ben, I am just hoping for a nice friendly interface that does not get in the way of user choice and that works.  I think I must have answered 500 get satisfaction problems where the unknowing user has found themselves in an IMAP account and deleted everything because they don't understand IMAP. Then had a very difficult time of it trying to change.  Because it is such a support headache it is one  of my most wished for fixes.

Is there a build that I can try it out on?  More than happy to be a guinea pig for this.

Comment 110

[rsx11m]

Ben, the try servers should be operational again, you can check with gozer if you want to offer any patched builds for broader testing.

Comment 111

[A.P. Veening]

(In reply to comment #109)
> (In reply to comment #108)
> Ben, I am just hoping for a nice friendly interface that does not get in the
> way of user choice and that works.  I think I must have answered 500 get
> satisfaction problems where the unknowing user has found themselves in an IMAP
> account and deleted everything because they don't understand IMAP. Then had a
> very difficult time of it trying to change.  Because it is such a support
> headache it is one  of my most wished for fixes.
> 
> Is there a build that I can try it out on?  More than happy to be a guinea pig
> for this.
Just download and install Thunderbird 2.0.0.24, set everything up like you want it and only then "upgrade" to 3.*.

Comment 112

[Reindl Harald]

> Just download and install Thunderbird 2.0.0.24, set everything 
> up like you want it and only then "upgrade" to 3.*.

Should be the answer or solution to what?
To "Is there a build that I can try it out on?" it is not and for first install for endusers it is no solution, especially on modern operation systems with package-managment

I wonder that it needs so much months to get fixed, throw away this **** and let settings as in 2.0.x - Change wizards if they are finished and working and not in such a state

Comment 114

[Neville Hillyer]

Further to my earlier comment at bug 531099#c52 I request that consideration be given to an initial window with these options:

1 - Automatic setup
2 - Manual setup
3 - No setup at this time

Comment 115

[Neville Hillyer]

I see that ' bug 531099#c52' does not work here - pity. The link should be:

https://bugzilla.mozilla.org/show_bug.cgi?id=531099#c52

Comment 116

[Ben Bucksch (:BenB)]

We have extensively discussed and answered this exact question in the last days here: <http://getsatisfaction.com/mozilla_messaging/topics/new_account_setup_wizard_is_too_aggressive>

Comment 117

[Neville Hillyer]

I am aware others have asked for and discussed 'Manual setup' but I am asking for 'No setup at this time' in addition to manual.

This would be particularly useful for experienced users and those who want a very simple mass deployment solution.

Comment 118

[Ben Bucksch (:BenB)]

Neville, answered in 531099 comment 60.

Comment 119

[Ben Bucksch (:BenB)]

NO COMMENTS: Please do not comment in this bug unless you are a developer. Thank you.

Comment 120

[Neville Hillyer]

I can see no mention of 'No setup at this time' or anything like it in comment 60.

Comment 121

[rsx11m]

Neville: There is a "Cancel" button on the Account Wizard which you can use for this purpose in a first-run scenario. According to comment #104 it will still be there in the redesigned version, which obviously makes sense.

Comment 122

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 442854 [details] [diff] [review]
v12, emailWizard.js

REVIEW part 2 -- name/email/pw fields, findConfig() -- 266 lines

>  getConcreteConfig : function()

This function needs documentation, cause I’m not really sure what it’s being used for.

>  /*
>   * This checks if the email address is at least possibly valid, meaning it
>   * has an '@' before the last char.
>   */
>  validateEmailWeakly : function(emailAddr)
>  {
>    let atPos = emailAddr.lastIndexOf("@");
>    return  atPos > 0 && atPos + 1 < emailAddr.length;
>  },

I think you should take this a little further, and assert that the address is at least of the form "a@b.c".  Or maybe just test it against ".+@.+\..+".

>  /*
>   * This checks if the email address is at least possibly valid, meaning it
>   * has an '@' before the last char.
>   */
>  validateEmail : function(emailAddr)
>  {
>    return emailRE.test(emailAddr);
>  },

Hmm, or there’s this.  What are the differences in times between validateEmailWeakly and validateEmail?  Because having both is more complicated, and if we don’t really need the extra speed, I’ld prefer to remove validateEmailWeakly.

>  /**
>   * onEmailInput and onRealnameInput are called onInput, and

ITYM "onInputEmail", and "onInputRealname".  And I think "onInput" should be "on input".

>   * en/disable the next button based on whether there's a semi-proper

I’ld prefer "enable/disable" here.  Or "enable the next button (or not)".

>   * e-mail address and nonblank realname to start with.

"non-blank"

>   * A change to the email address also automatically restarts the
>   * whole process.

Does this mean that the whole process gets restarted for every character I type in my email address?

>  /**
>   * This check is only done as an informative warning.
>   * We don't want to block the person, if they've entered an email address
>   * that doesn't conform to our regex.

Well, we do if we can’t do anything with it, like if it doesn’t contain an @.  ;)

>  checkStartDone : function()

We could use a little documentation on this function.

>  {
>    if (this.validateEmailWeakly(this._email) &&
>        this._realname)

I wonder if it would make more sense to check the error attributes of the emailEl and realnameEl, since that way the checks here could never get out of sync with the checks on the elements, and it would likely be faster.

>    {
>      this._domain = this._email.split("@")[1].toLowerCase();
>      _enable("next_button");
>    }
>    else
>      _disable("next_button");

I think we need {}s around the else clause, cause the if clause has them.

>  /**
>   * When the [Continue] button is clicked, we move from the initial account
>   * information stage to using that information to configure account details.
>   */
>  onNext : function()

If it’s the continue button now, and we’re changing everything anyways, would it make sense to rename this to "onContinue" (and the button to "continue_button")?

>  /**
>   * Just a continuation of findConfig()
>   */
>  _guessConfig : function(domain, initialConfig)
>  {
>    this.startSpinner("looking_up_settings_guess")
>    let me = this;
>    me._abortable = guessConfig(domain,
>          function(type, hostname, port, ssl, done, config) // progress

This should have a two-space indent, I think, not a six-space indent.

>  /**
>   * When findConfig() was successful, it calls this.

"When findConfig() or _guessConfig() was successful…"

>  foundConfig : function(config)
>  {
>    gEmailWizardLogger.info("foundConfig()");
>    assert(config instanceof AccountConfig,
>        "BUG: Arg 'config' needs to be an AccountConfig object");
>
>    this._haveValidConfigForDomain = this._email.split("@")[1];;
>
>    if (!this._realname || !this._email)
>      return;
>    try {
>    return this._foundConfig2(config);
>    } catch (e) { logException(e); throw e; }

I’ld prefer it if this was indented like this:
    try {
      return this._foundConfig2(config);
    }
    catch (e) {
      logException(e);
      throw e;
    }

>  },
>  // Continuation of foundConfig2() after custom fields.

Put a blank like before this line, please.

Other than that, it looks good, so r=me with the nits fixed.

(Five more to go, and most of them are smaller than this.)

Later,
Blake.

Comment 123

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 442854 [details] [diff] [review]
v12, emailWizard.js

REVIEW part 3 -- actually, there’s no part 3.

Well, that was easy.  ;)


REVIEW part 4 -- progress and result display -- 193 lines

>  onStopAll : function()

I would rather this be named "onStopCommon", because for a while there, I thought it would stop all the things, not be called by all the things that wanted to stop.

>    this.stopSpinner("");

Could this just be "this.stopSpinner();"?

>  /**
>   * Displays a (probed) config to the user,
>   * in the result config details area.
>   *
>   * @param config {AccountConfig} The config to present to user
>   */
>  updateConfigResult : function(config)

Since it’s displaying a probed config, what do you think about renaming the function "displayProbedConfig", or "displayConfigResult"?

>    this._currentConfig = config;
>    var configFilledIn = this.getConcreteConfig();

I’m starting to think that we want getConcreteConfig to take a config, and return the filled in version, instead of having it depend on this._currentConfig.  I’m not going to insist on it, but I do think it would be really nice.

>    /* TODO
>    const boldStart = "<html:strong>";
>    const boldEnd = "</html:strong>";
>    const warnStart = "<span style='color: red'>";
>    const warnEnd = "</span>";
>    */

Perhaps you could do something with these, or remove them?

>    var unknownString = gStringsBundle.getString("resultUnknown");
>
>    function _makeHostDisplayString(server, stringName)
>    {
>      return gStringsBundle.getFormattedString(stringName,
>        [ gStringsBundle.getString(sanitize.translate(server.type,  // %1
>              { imap : "resultIMAP", pop3 : "resultPOP3",
>                 smtp : "resultSMTP" }), unknownString),
>          server.hostname + // %2
>              (isStandardPort(server.port) ? "" : ":" + server.port),
>          gStringsBundle.getString(sanitize.translate(server.socketType,  // %3
>              { 1 : "resultNoEncryption",  2 : "resultSSL",
>                 3 : "resultSTARTTLS" }), unknownString),
>          gStringsBundle.getString(server.badCert ? // %4
>              "resultSSLCertWeak" : "resultSSLCertOK"),
>        ]);

Ewwww…  ;)

I’ld far prefer it it you used some temporary variables here.
Something like:
      let type = sanitize.translate(server.type,
                   { imap : "resultIMAP",
                     pop3 : "resultPOP3",
                     smtp : "resultSMTP" });
      type = gStringsBundle.getString(type, unknownString);

      let host = server.host + (isStandardPort(server.port) ?
                                "" : (":" + server.port));

      let secure = sanitize.translate(server.socketType,
                     { 1 : "resultNoEncryption",
                       2 : "resultSSL",
                       3 : "resultSTARTTLS" });
      secure = gStringsBundle.getString(secure, unknownString),
      secure += gStringsBundle.getString(server.badCert ?
                  "resultSSLCertWeak" : "resultSSLCertOK");

      return gStringsBundle.getFormattedString(stringName,
                                               [type, host, secure, ]);


Cause then I could see what’s happening in my debugger.  Also because I find that way easier to follow.

>    ddumpObject(config, "config"); // TODO

You should really do this, whatever it is you meant to do here.

>    // IMAP / POP dropdown
>    var lookForAltType =
>        configFilledIn.incoming.type == "imap" ? "pop3" : "imap";

Do you really need this, or could you just check that "alt.type != configFilledIn.incoming.type" later on?

>    for (let i = 0; i < configFilledIn.incomingAlternatives.length; i++)
>    {

I believe the Mozilla style is to put the {s on the same line as the for/if statements, and if you’re changing the whole file, I see no reason to deviate from that.

>  onResultIMAPOrPOP3 : function()
[snip…]
>    config.incomingAlternatives = config.incomingAlternatives.filter(
>        function(e) { return e != config.incoming; }); // TODO doesn't work

We should probably fix this, if it doesn’t work.

So, these are mostly small things.  r=me with them fixed.

Later,
Blake.

Comment 124

[Ben Bucksch (:BenB)]

> What are the differences in times between
> validateEmailWeakly and validateEmail?

You answered that yourself, I think (will have to double-check):

> >   * This check is only done as an informative warning.
> >   * We don't want to block the person, if they've entered an email address
> >   * that doesn't conform to our regex.
> Well, we do if we can’t do anything with it, like if it doesn’t contain an @. 
;)

Style:

> This should have a two-space indent, I think, not a six-space indent.

FWIW, I use a 2-space for block indentions, and 4+-space for line indentions.

>   } catch (e) { logException(e); throw e; }

This isn't actually handling the exception, just logging it. I try to keep these catches (and those which just show the error) with minimal code footprint, to make the other catches stand out which actually handle the exception in a meaningful way with fallbacks.

Many other comments were good, thanks for them.

Comment 125

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 442854 [details] [diff] [review]
v12, emailWizard.js

REVIEW part 5 -- manual edit, part 1 -- 168 lines

>  getUserConfig : function()
[snip…]
>    // Incoming server
>    var inProtocolDropdown = document.getElementById("incoming_protocol");
>    var inHostnameField = document.getElementById("incoming_hostname");
>    var inPortField = document.getElementById("incoming_port");
>    var inUsernameField = document.getElementById("incoming_username");
>    var inSSLDropdown = document.getElementById("incoming_ssl");
>    var inAuthDropdown = document.getElementById("incoming_authMethod");
>    try {
>      config.incoming.hostname = sanitize.hostname(field.value);
>      field.value = config.incoming.hostname;

Where does “field” come from here?

>    } catch (e) {}

And why is it safe to ignore any exception?  What do you think will fail, and can’t we test for that instead?

>    try {
>      config.incoming.port = sanitize.integerRange(inPortField.value,
>            1, kHighestPort);

I think we can line this up with the “inPortField.value”.

>    } catch (e) { config.incoming.port = undefined } // incl. default "Auto"

Please put this on more than one line.

>    config.incoming.type = sanitize.translate(inProtocolDropdown.value,
>        { 1: "imap", 2 : "pop3", 0 : null });

I think I’ld rather see the various vars up top inlined, unless they’re used in more than one place.  Then we can make sure we didn’t forget to use any, like “inHostnameField”.  ;)

>    // Did the user select one of the already configured SMTP servers from the
>    // drop-down list? If so, use it.
>    var outHostnameCombo = document.getElementById("outgoing_host");
>    var outMenuitem = outHostnameCombo.selectedItem;
>    if (outMenuitem && outMenuitem.serverKey)
>    {

Braces on the same line, please.  (Because you do it for the try blocks really close to this if-else.)

>    else
>    {

Ditto.

>        config.outgoing.hostname = sanitize.hostname(
>              inHostnameCombo.inputField.value);
>        inHostnameCombo.inputField.value = config.outgoing.hostname;

These three lines really don’t look right to me.  ;)

>  /* [Manual Config] button click handler.  This turns the config details area into an
>   * editable form and makes the (Go) button appear.  The edit button should
>   * only be available after the config probing is completely finished,
>   * replacing what was the (Stop) button.

Wrap before 80 characters, please.

>  onManualEdit : function()
>  {
>    if (this._abortable)
>      this.onStop();
>    this.editConfigDetails();
>  },

I think it might be easier to read if we just rolled this code into the editConfigDetails function, and made that the manual config’s onClick handler.  Is there any reason we shouldn’t call onStop if we’re trying to edit the config details, and we have an _abortable?

>  /**
>   * Setting the config details form so it can be edited.  We also disable

(Just some English tweaks: “Set the config details form…”  “Also disable”.)

>    if (!this._currentConfig)
>    {
>      this._currentConfig.incoming.hostname = ".%EMAILDOMAIN%";
>      this._currentConfig.outgoing.hostname = ".%EMAILDOMAIN%";

Why “.%EMAILDOMAIN%” instead of “%EMAILDOMAIN%”?

>  _fillManualEditFields : function(config)
[snip…]
>    // populate fields even if existingServerKey, in case user changes back
>    if (config.outgoing.existingServerKey)
>    {
>      let menulist = document.getElementById("outgoing_host");
>      //menulist.value = config.outgoing.existingServerKey; overwrites textfield

Should this still be commented out, or can we remove it entirely?

One more thing:
editConfigDetails calls _fillManualEditFields which calls onChangedOutgoingDropdown which calls onChangedManualEdit which calls validateManualEditComplete.

But just below the call to _fillManualEditFields, editConfigDetails then calls validateManualEditComplete directly, which I think we don’t need to do.  If I’m right, please remove that second call.  (If I’m wrong, please let me know where.  ;)

These are mostly minor, so I’m going to say r=me with them fixed.

Thanks,
Blake.

Comment 126

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 442854 [details] [diff] [review]
v12, emailWizard.js

REVIEW part 6 -- manual edit, part 2 -- 247 lines

>  setPort : function(config, setIncoming)
[snip…]
>    var newInPort;
>    if (setIncoming && (!incoming.port || isStandardPort(incoming.port)))
>    {
>      if (incoming.type == "imap")
>      {
>        if (incoming.socketType == 1 || incoming.socketType == 3)
>          newInPort = 143;
>        else if (incoming.socketType == 2) // Normal SSL
>          newInPort = 993;
>        else // auto
>          newInPort = autoPort;
>      }
>      else if (incoming.type == "pop3")
>      {
>        if (incoming.socketType == 1 || incoming.socketType == 3)
>          newInPort = 110;
>        else if (incoming.socketType == 2) // Normal SSLs
>          newInPort = 995;
>        else // auto
>          newInPort = autoPort;
>      }
>    }

I think I’ld rather see that in a table, like so:
ports = {
  "imap,1": 143,
  "imap,2": 993,
  "imap,3": 143,
  "pop3,1": 110,
  "pop3,2": 995,
  "pop3,3": 110,
}

And then do something like:
  let key = incoming.type + "," + incoming.socketType;
  if (key in ports)
    newInPort = ports[key];
  else
    newInPort = autoPort;

>    if (!setIncoming && (!outgoing.port || isStandardPort(outgoing.port)))

ITYM setOutgoing.

>      if (outgoing.socketType == 1 || outgoing.socketType == 3)
>      {
>        // standard port is 587 *or* 25, so set to auto
>        // unless user or config already entered one of these two ports.
>        if (outgoing.port != 25 && outgoing.port != 587)
>          newOutPort = autoPort;
>      }
>      else if (outgoing.socketType == 2) // Normal SSL
>        newOutPort = 465;
>      else // auto
>        newOutPort = autoPort;

Ditto with the table lookup.  Heck, you could even add these cases in to the ports list as "smtp,1", etc…

>    if (newInPort)
>    {
>      document.getElementById("incoming_port").value = newInPort;
>      document.getElementById("incoming_authMethod").value = 0; // auto
>    }
>    if (newOutPort)
>    {
>      document.getElementById("outgoing_port").value = newOutPort;
>      document.getElementById("outgoing_authMethod").value = 0; // auto
>    }

Could we set the authMethod the the authMethod from the config?

>  onChangedPort : function()
>  {
>    this.onChangedManualEdit();
>  },
>  onChangedAuth : function()
>  {
>    this.onChangedManualEdit();
>  },
>  onInputUsername : function()
>  {
>    this.onChangedManualEdit();
>  },
>  onInputHostname : function()
>  {
>    this.onChangedManualEdit();
>  },

Uh, do we really need these functions, or could we just call onChangedManualEdit for the handlers?

>  onAdvancedSetup : function()
>    if (existingAccountManager)
>      existingAccountManager.focus();
>    else
>      window.openDialog("chrome://messenger/content/AccountManager.xul",
>                        "AccountManager", "chrome,centerscreen,modal,titlebar",
>                        { server: newAccount.incomingServer,
>                          selectPage: "am-server.xul" });

One of the things we have a bug (and that I think is a good idea) for is to select the newly-created account when we open this window, as opposed to leaving it sitting on the first account.  I think the re-write should also fix that bug.

>    // TODO check whether the dialog was cancelled. If so, remove the accounts we created.

I don't know if that really makes sense to do.  I think once you're in the advanced setup, keeping or deleting the account is all on you.  But if you insist on doing it, you should wrap the comment before 80 characters.  ;)

>  /**
>   * [Re-test] button click handler.  Restarts the config guessing process after a
>   * person editing the server fields.

And ditto this comment.

>  onHalfManualTest : function()
>  {
>    var newConfig = this.getUserConfig();
>    this.startSpinner("looking_up_settings_halfmanual");
>    this.switchToMode("manual-edit-testing");
>    // if (this._userPickedOutgoingServer) TODO

What are you going to do in this case?

>    var me = this;
>    me._abortable = guessConfig(this._domain,

Please use either "me" or "this" for both of these attributes.  :)

>        function(e, config) // guessconfig failed
>        {
>          if (e instanceof CancelledException)
>            return;
>          me._abortable = null;
>          gEmailWizardLogger.info("guessConfig failed: " + e);
>          me.stopSpinner("failed_to_find_settings");
>          me.switchToMode("manual-edit-have-hostname");

So, does it make sense to switch to the manual-edit-have-hostname mode if we couldn't find any info from the guessconfig?

I think I've asked for quite a bit from this review, so I'm going to give it an r-.

(Phew.  Only two more parts to go. :)

Later,
Blake.

Comment 127

[Ben Bucksch (:BenB)]

> Braces on the same line, please.
> (Because you do it for the try blocks really close to this if-else.)

FYI, the style for this file and most other files in this dir is:
- opening bracket on its own line (functions, if/else/for)
  - expect for try/catch where possible
    (to avoid big code footprint for *standard* error handling)
- indent blocks 2 spaces
- indent line wraps by 4-6 spaces

I don't really want to change all that.

> Wrap before 80 characters, please.

Sure.

> >        config.outgoing.hostname = sanitize.hostname(
> >              inHostnameCombo.inputField.value);
> >        inHostnameCombo.inputField.value = config.outgoing.hostname;

> These three lines really don’t look right to me.  ;)

Why? If you meant that we write back to the original |value|, that's intentional. I take the value from the UI, sanitize it, write the sanitized value into the internal object representing the current config. Then I make the UI match the internal representation again, in case the sanitize did change the var.

Or is there something specific you were/are thinking of?

> I think I’ld rather see that in a table

I'm not sure that's possible, because there were minor differences (esp. for SMTP with 2 ports), but I can try it. I hope it will still be readable.

> >    if (!setIncoming && (!outgoing.port || isStandardPort(outgoing.port)))
> ITYM setOutgoing.

uhm, yes. Thanks.

> Could we set the authMethod the the authMethod from the config?

No. We reset the port, and the server might offer different auth methods on the other port (d.h. SSL doesn't have encrypted auth).

> Uh, do we really need these functions, or could we just call
> onChangedManualEdit for the handlers?

No, we don't strictly need them. That's just style. I hate having to look up the XUL to see what's going on. Having these methods here makes clear that this function is called (and nothing else) for this list of widgets, without having to hunt for it in the XUL file. I had hear-tearout moments when this was violated. I think this logic is complex enough that this ease of reading it useful - I know it did help even myself during coding.

In other words, I try to separate UI handlers from functions.

> I think it might be easier to read if we just rolled this code into the
> editConfigDetails function, and made that the manual config’s onClick

same as above.

> > //menulist.value = config.outgoing.existingServerKey; overwrites textfield
> Should this still be commented out, or can we remove it entirely?

This line was the obvious way to code it, but it caused a bug that I found later. This comment is my way to record the warning not to do that.
If you have suggestions how else to keep that warning, feel free.

> >      window.openDialog("chrome://messenger/content/AccountManager.xul",
> >                        "AccountManager", "chrome,centerscreen,modal,titlebar",
> >                        { server: newAccount.incomingServer,
> >                          selectPage: "am-server.xul" });

> select the newly-created account when we open this window, as opposed to
> leaving it sitting on the first account.  I think the re-write should also fix
> that bug.

Don't I do that here? If not, I think we should leave it to the followup bug.

> Why “.%EMAILDOMAIN%” instead of “%EMAILDOMAIN%”?

If you enter [e-mail], I don't think we should prefill IMAP and SMTP server hostname "example.com", because that may be a host, but is unlikely to have the mail server. Prefilling ".example.com" 1) shows us that the value is invalid as-is 2) shows to the user that he has to change this and that we expect some hostname in front (otherwise "example.com" may look right to non-techy users) 3) saves him typing the dot.

Alternatively, we could use "changeme.%EMAILDOMAIN%", but that would be even clearer. (Ideally, we would localize it, but not strictly necessary.) In fact, I think I like that better and think I will use that.


> editConfigDetails calls _fillManualEditFields which calls
> onChangedOutgoingDropdown which calls onChangedManualEdit which calls
> validateManualEditComplete.

> But just below the call to _fillManualEditFields, editConfigDetails then calls
> validateManualEditComplete directly, which I think we don’t need to do.
> If I’m right, please remove that second call.  (If I’m wrong, please
> let me know where.  ;)

Interesting. I didn't expect that, although I don't think it does harm.

_fillManualEditFields() should not trigger the onChanged...() handlers, I want them to be called for user edits, but not when we fill them out. I'll check.

But as said, I don't think it does harm to call the function twice. The validateManualEditComplete() is important, so I'd like to keep it explicitly in the code, or at least make a comment that it's called implicitly.

Comment 128

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #127)
> > Braces on the same line, please.
> > (Because you do it for the try blocks really close to this if-else.)
> FYI, the style for this file and most other files in this dir is:
> - opening bracket on its own line (functions, if/else/for)
>   - expect for try/catch where possible
>     (to avoid big code footprint for *standard* error handling)
> - indent blocks 2 spaces
> - indent line wraps by 4-6 spaces

Since you're re-writing this whole file, the file's current style doesn't really matter so much.  Please conform to the Mozilla Coding Style.  https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Control_Structures

(I'm sure there are automated tools which will fix it for you.  I could look them up and recommend one, and the appropriate command line flags, if you'ld like.)

> > >        config.outgoing.hostname = sanitize.hostname(
> > >              inHostnameCombo.inputField.value);
> > >        inHostnameCombo.inputField.value = config.outgoing.hostname;
> > These three lines really don’t look right to me.  ;)
> Why? If you meant that we write back to the original |value|, that's
> intentional. I take the value from the UI, sanitize it, write the sanitized
> value into the internal object representing the current config. Then I make
> the UI match the internal representation again, in case the sanitize did
> change the var.
> 
> Or is there something specific you were/are thinking of?

It was more that you're setting the outgoing hostname to the inHostnameCombo's value, and vice versa.

> > Uh, do we really need these functions, or could we just call
> > onChangedManualEdit for the handlers?
> 
> No, we don't strictly need them. That's just style. I hate having to look up
> the XUL to see what's going on. Having these methods here makes clear that
> this function is called (and nothing else) for this list of widgets, without
> having to hunt for it in the XUL file. I had hear-tearout moments when this
> was violated. I think this logic is complex enough that this ease of reading
> it useful - I know it did help even myself during coding.

Okay.  That sounds reasonable.  What do you think about switching them to the slightly shorter:

onChangedPort = onChangedManualEdit;

(Would that work?)

> > > //menulist.value = config.outgoing.existingServerKey; overwrites textfield
> > Should this still be commented out, or can we remove it entirely?
> 
> This line was the obvious way to code it, but it caused a bug that I found
> later. This comment is my way to record the warning not to do that.
> If you have suggestions how else to keep that warning, feel free.

Maybe change it into a comment like:
// We can't say menulist.value = config.outgoing.existingServerKey because
// that will overwrite the text field.

> > select the newly-created account when we open this window.
> Don't I do that here? If not, I think we should leave it to the followup bug.

I don't think so.  Could you test it to see?

Thanks,
Blake.

Comment 129

[Thomas D. (:thomas8)]

As per the new link policy GS -> BMO (https://wiki.mozilla.org/Thunderbird/Support/Workflow#GS_-.3E_Bugzilla), ftr:

http://gsfn.us/t/198nn - Can't set up a Thunderbird account for Roadrunner WAS: Can't set up my account, I can't use it, I'll use something that works. 
http://gsfn.us/t/15tty - My email is pop, but thunderbird wants imap4

Comment 130

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 442854 [details] [diff] [review]
v12, emailWizard.js

REVIEW part 7 -- utility and dialog finish -- 221 lines

I really thought I had done this one already, so these comments will be a little more perfunctory than usual.

>  _prefillConfig: function(initialConfig)
>  {
>    var emailsplit = this._email.split("@");
>    if (emailsplit.length != 2)
>      throw new Exception(gStringsBundle.getString("double_check_email"));

I think we should call one of the other email validators here, so that we can consolidate the code into one place.

>  clearError: function(which) {
[snip…]
>  setError: function(which, msg_name) {

It would be cute/neat if “setError(foo, '')”, or “setError(foo)’ did the same thing as clearError.  Not more maintainable, and I don’t suggest we change it, but it would be neat.  ;)

>  onKeyDown : function(event)
[snip…]
>    if (key == 13) // OK key
>    {
>      if (!document.getElementById("next_button").hidden
>          && !document.getElementById("next_button").disabled)
>      {
>        this.onNext();
>        return true;
>      }
>      if (!document.getElementById("create_button").hidden
>          && !document.getElementById("create_button").disabled)
>      {
>        this.onOK();
>        return true;
>      }
>      if (!document.getElementById("half-manual-test_button").hidden
>          && !document.getElementById("half-manual-test_button").disabled)
>      {
>        this.onHalfManualTest();
>        return true;
>      }

I think I’ld like to see these in a list, and have us iterate through them, calling the first one that’s not hidden and not disabled.  That way we would be certain to not call more that one button, if two happened to be shown at the same time.

>  onOK : function()
>  {
>      gSecurityWarningDialog.open(this._currentConfig, configFilledIn, true,
>      function() // on OK
>      {
>        me.validateAndFinish(configFilledIn);
>      },
>      function() {}); // on cancel, do nothing

I think these functions should both be indented two spaces.

>  // called by onOK()
>  validateAndFinish : function()
>  {
>    if (checkIncomingServerAlreadyExists(configFilledIn))
>    {
>      alertPrompt(gStringsBundle.getString("error_creating_account"),
>                  gStringsBundle.getString("incoming_server_exists"));
>      return;
>    }
>
>    if (configFilledIn.outgoing.addThisServer)
>    {
>      let existingServer = checkOutgoingServerAlreadyExists(configFilledIn);
>      if (existingServer)
>      {
>        configFilledIn.outgoing.addThisServer = false;
>        configFilledIn.outgoing.existingServerKey = existingServer.key;
>      }

So, why are we reporting an error if the incoming server that we want to add already exists, but not throwing an error if the outgoing server that we’ve specified we want to add already exists?

>    var me = this;
>    if (!this._verifiedConfig) // TODO for what use is that? if it was verified, we created the account and closed the dialog

Please put the comment on a new line, and wrap it at 80 characters.

Or, since I agree with the comment, perhaps we should just assert that this._verifiedConfig is false, to make sure.

>          function(e) // failure
>          {
>            // give user something to proceed after fixing
>            _enable("create_button");
>            // hidden in non-manual mode, so it's fine to enable
>            _enable("half-manual-test_button");
>            _enable("advanced-setup_button");

Should this be replaced with a call to “switchToMode”?  (Or why not?)

>  verifyConfig : function(concreteConfig, successCallback, errorCallback)
>  {
>      concreteConfig.source != AccountConfig.kSourceXML,
>      //concreteConfig.source == AccountConfig.kSourceGuess, TODO

I don’t understand this TODO.  Could you remove it, or do it, or add more words so that I can tell what we need to do?  :)

>        me._verifiedConfig = true;
>        if (concreteConfig.incoming.password)
>          me.stopSpinner("password_ok");

What happens if there is no incoming password?  Does the spinner keep on going forever?

>        me.setError("passworderror", "user_pass_invalid"); // TODO wrong error msg, there may be a 1000 other reasons why this failed -- bug 555448

Please put this comment on a new line, and wrap it at 80 characters.

>function serverMatches(a, b)
>{
>  return a.type == b.type && a.hostname == b.hostname && a.port == b.port &&
>      a.socketType == b.socketType && a.auth == b.auth;

I’ld rather this was broken up like this:
  return a.type == b.type &&
         a.hostname == b.hostname &&
         a.port == b.port &&
         a.socketType == b.socketType &&
         a.auth == b.auth;

>function isStandardPort(port)
>{
>  // Protocol doesn't really matter. You have to be luniatic to

“lunatic”.  (Thank you, Firefox spell-checker.  ;)

>  // run an IMAP server on port 25. More likely, it's a user error.
>  switch (port)
>  {
>    case 587:
>    case 25:
>    case 465:
>    case 143:
>    case 993:
>    case 110:
>    case 995:

So, I think we could either order these numerically, or put comments between them stating which are for SMTP, POP, and IMAP.

I’m confident you can resolve all the issues I mentioned without me needing to check it over again, so r=me for this part.

Thanks,
Blake.

Comment 131

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 442854 [details] [diff] [review]
v12, emailWizard.js

REVIEW part 8 -- SecurityWarningDialog -- 277 lines

>/**
> * Warning dialog, warning user about lack of, or inappropriate, encryption.
> *
> * This is effectively a separate dialog, but implemented as part of
> * this dialog. It works by hiding the main dialog part and unhiding
> * the this part, and vice versa, and resizing the dialog.
> */
>function SecurityWarningDialog()
>{
>  this._acklowledged = new Array();

That should be “this._acknowledged”, I think.  (Also in the other places it occurs.)

>}
>SecurityWarningDialog.prototype =
>{
>  /**
>   * {Array of {server (incoming or outgoing) part of {Accountconfig}}

Could you re-phrase this as:
   * {Array of {(incoming or outgoing) server part of {AccountConfig}}
(If not, at least change “Accountconfig” to “AccountConfig”.)

>  /**
>   * Checks whether we need to warn about this config.
>   *
>   * We (currently) warn if
>   * - the mail travels unsecured (no SSL/STARTTLS)
>   * - the SSL certificate is not proper
>   * - (We don't warn about unencrypted passwords specifically,
>   *   because they'd be encrypted with SSL and without SSL, we'd
>   *   warn anyways.)
>   *
>   * We may not warn despite these conditions if we had shown the
>   * warning before and the user acknowledged it.
>   * (Given that this dialog object is static/global and persistent,
>   * we can store that approval state here in this object.)

Do we warn if we had shown the warning for no SSL, and then the user switches to SSL, but we get an improper certificate?  (i.e. same server, different warning case.)

>  needed : function(configSchema, configFilledIn)
>  {
>    var incomingOK = configFilledIn.incoming.socketType > 1 &&

Don’t we have a constant for that magic number “1”?

>      incomingOK = this._acklowledged.some(
>          function(ackServer) {
>            return serverMatches(ackServer, configFilledIn.incoming);
>          });

Nice!  :)

>   * The function is async: it returns immediately, and when the user clicks
>   * OK or Cancel, the callbacks are called. There the callers proceeds as
>   * appropriate.

“proceed as appropriate”.

>   * @param onlyIfNeeded {Boolean}   If there is nothing to warn about,
>   *     call okCallback() immediately (and sync).

So, what do we do if this is set to false?

>  open : function(configSchema, configFilledIn, onlyIfNeeded,
>                okCallback, cancelCallback)

I think this should line up with “configSchema”.

>    if (!needed && onlyIfNeeded)
>    {
>      okCallback();
>      return;
>    }
>    assert(needed, "security dialog opened needlessly");

So, basically you’re asserting that onlyIfNeeded is always true?

>    if (incoming.socketType == 1)
>    {
>      setText("warning_incoming", gStringsBundle.getFormattedString(
>          "cleartext_warning", [incoming.hostname]));

Line this up with “warning_incoming”, please.  Actually, or don’t, since it’s part of the call to getFormattedString.  Your choice.

>      setText("incoming_details", gStringsBundle.getString("cleartext_details"));

Let‘s split this line before “cleartext_details”, to match the other three if blocks.

>  /**
>   * [OK] button pressed.
>   * Implies that the user toggled the acknowledge checkbox,
>   * i.e. approved the config and ignored the warnings,
>   * otherwise the button would have been disabled.
>   */
>  onOK : function()

Since extensions might call this, should we double-check the state of the button, or do you feel that this documentation is enough of a warning?

>  showCertOverrideDialog : function(config)
>   * @returns true, if all certs are fine or the user accepted them.
>   *     false, if the user cancelled.

So, we don’t seem to check the return value of this function, which seems kind of like a bug.  Am I missing something that makes it okay?

>    if (config.incoming.socketType > 1 && // SSL or STARTTLS
>        config.incoming.badCert)
>    {
>      var params = { exceptionAdded : false };
>      params.prefetchCert = true;
>      params.location = config.incoming.targetSite;

Why not set these as part of the object directly?

>      if (params.exceptionAdded) // set by dialog
>        config.incoming.badCert = false;
>      else
>        return false;
>    }
>    if (!config.outgoing.existingServerKey)
>    {

So, if we’re using the same host for incoming and outgoing, and we’ve added an exception for the incoming host, we’ll still see this dialog for the identical outgoing host?  That doesn’t seem quite right to me.

>var gSecurityWarningDialog = new SecurityWarningDialog();

I’m going to r- this, because there are a few biggish questions in it, which I think might lead to a fair bit of extra code.

And with that, I think we’re done this first pass of reviews.  I look forward to seeing the next set of changes.  :)

Later,
Blake.

Comment 132

[Ben Bucksch (:BenB)]

> >   * {Array of {server (incoming or outgoing) part of {Accountconfig}}
> Could you re-phrase this as:
>    * {Array of {(incoming or outgoing) server part of {AccountConfig}}

Yeah, much better.

> >   var incomingOK = configFilledIn.incoming.socketType > 1 &&
> Don’t we have a constant for that magic number “1”?

Yes, but this is ">", so we're matching several values here. Using a constant makes it likely to miss that it's > and not ==. In fact:

[19:28:10] <bwinton> Oh, it's a >...  Nevermind that one, then.
[19:28:24] <bwinton> (I misread it as ==)

:)

Comment 134

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #104)
Just wanted to note that I updated all of my mockups (you may have to hard refresh).

> - start screen
As agreed upon.
https://wiki.mozilla.org/File:Thunderbird-Autoconfiguration-Initial_View.png

> - detection screen (progress bar)
As agreed upon.
https://wiki.mozilla.org/File:Thunderbird-Autoconfiguration-Auto-Configurating.png

> - result screen
I haven't seen the latest screenshots from your code yet but I've merged what we talked about into the mockup and I'd love to get your feedback when you have a chance.
https://wiki.mozilla.org/File:Thunderbird-Autoconfiguration-Auto-Configured.png

So I've also created an alternate take on this dialog in case we continue to differ I have another approach that I think could also work.
https://wiki.mozilla.org/File:Thunderbird-Autoconfiguration-Auto-Configured%28Alternate%29.png

> - manual config screen
Here is the merged version I have that continues the current trend.
https://wiki.mozilla.org/File:Thunderbird-Autoconfiguration-Sport_Configuring.png

I also did an alternate view of the manual mode just in case
https://wiki.mozilla.org/File:Thunderbird-Autoconfiguration-Sport-Config%28Alternate%29.png

Ben, are these more or less inline with what you have now?  Feel free to call or ping me whenever you get a chance to discuss.

Comment 135

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 437446 [details]
v7, Screenshot, Manual edit, one username, on 3rd row

removing this request as we're working the review through the comments.

Comment 136

[r.benzazon]

Hi,
I see you make a ton of work to make the autoconfig working, bravo. A lot of users would prefer clicking on manual config from the start, then click if they feel it, test config. This scenario should always be possible. I see that now it's not, you must go to auto by default, let the config fail, then click manual. What a loss of time.

Comment 137

[Adrien de Croy]

I've had nothing but nightmares with the account creation wizard, it's very frustrating.  my suggestions:

1. Start page of wizard should ask the user whether they wish to manually configure the account, or have thunderbird attempt to automatically configure it.

2. Selecting POP3 should change the port to 110 from default of 143 for IMAP.  In fact, ports should stay out of it, only override default port (143 or 110) on advanced setting.  "Use default port" should be the default setting.

3. Stop button should actually work.  It doesn't work.  The lookups keep coming, and any settings you typed in keep getting replaced.

The heuristic for auto config is really tuned to accessing an ISP account, and is next to useless for corporate.  I'd suggest another question in the wizard: "Is this an ISP account, or an internal corporate account".  If it's internal, do a SRV record lookup.

4. Failure to automatically guess settings should result in allowing the user to manually set up the account.

5. ALLOW PEOPLE TO CHANGE MAILBOX PROTOCOL!!!!!!!  This one is just ridiculous.  If I change my mail account on my mail server from POP3 to IMAP or vice-versa I have to delete my account????? WTF???  How am I supposed to support a company using TB with mindless rules like this?  Try rolling out IMAP in a company of 100 TB users.

Even if you have to recreate some object using the common parameters (servers, usernames, passwords, names, return address etc) it's still miles better than forcing the poor user (or hapless admin) through the account creation nightmare wizard a zillion times.

Comment 138

[Charles]

(In reply to comment #137)
> I've had nothing but nightmares with the account creation wizard, it's very
> frustrating.  my suggestions:

Lots of changes in the works - it will be much better soon... but for one reason or another, the devs simply do not want to make it easy to just select 'Manual' mode...

> 2. Selecting POP3 should change the port to 110 from default of 143 for IMAP. 
> In fact, ports should stay out of it, only override default port (143 or 110)
> on advanced setting.  "Use default port" should be the default setting.
> 
> 3. Stop button should actually work.  It doesn't work.  The lookups keep
> coming, and any settings you typed in keep getting replaced.

Both known issues that will be addressed...

> 4. Failure to automatically guess settings should result in allowing the user
> to manually set up the account.

It does for me...

> 5. ALLOW PEOPLE TO CHANGE MAILBOX PROTOCOL!!!!!!!  This one is just
> ridiculous. If I change my mail account on my mail server from POP3
> to IMAP or vice-versa I have to delete my account????? WTF???

For many reasons changing an account from IMAP to POP is non-trivial, and most other clients will not let you do it either - I know Outlook and Outlook Express won't (not sure about 2010 though)...

The problem you are complaining about is most likely the way the account creation currently works - if you click 'Manual Setup', it actually *creates* the account and takes you to the 'Account Settings' window - but once the account has been created, if you decide you prefer POP to IMAP (or vice-versa) and it was created as an IMAP (or POP) account, then  yes, you have to delete it and start over.

As I said - this process will soon be *much* easier to deal with - and actually the auto-detection has gotten much better, even for our corporate mail system that is *not* oart of Mozilla's ISP database.

> How am I supposed to support a company using TB with mindless rules like
> this?

Maybe by doing a little research before mindlessly yelling and screaming about something without all of the facts?

> Try rolling out IMAP in a company of 100 TB users.

We've been using Thunderbird since about version 0.8 in our smallish (50-60 average) company - which means, yes, since about 2000/2001 (forget when it was released). We were using Netscape 4 before that - and yes, always using IMAP access *only*.

> Even if you have to recreate some object using the common parameters
> (servers, usernames, passwords, names, return address etc) it's still
> miles better than forcing the poor user (or hapless admin) through the
> account creation nightmare wizard a zillion times.

I admittedly will be extremely happy when full GPO support is available.

Comment 139

[Ben Bucksch (:BenB)]

Please see the whiteboard:
> NO COMMENTS: Please do not comment unless you are a developer. Thank you.

Status:
bwinton completed the review, after 5 (!) months.
Byran posted his ui-review comments 1 month ago.

Now, the ball is in my park. I need to adjust the UI a bit and have another UI-review round with Bryan. And I need to fix bwinton's 20 pages of review comments. Then hopefully we come to conclusion here.

Comment 140

[Adrien de Croy]

Ben - thanks.  I am actually a developer, not a mozilla one though which I presume is meant.

As for my comment about changing protocol.  That is a different bug sorry, and probably should not have been raised here.

But for the record, I was talking about migrating a company from POP3 to IMAP where they use TB.  I'm pretty sure TB 2 and earlier allowed you to change protocol when editing an account, and frankly I don't consider it that difficult given the number of shared settings vs the number of different ones.

And having written both an IMAP and POP3 server, I do know a little bit about this.

Cheers Ben, look forward to seeing the result.

Comment 141

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #140)
> As for my comment about changing protocol.  That is a different bug sorry, and
> probably should not have been raised here.
> 
> But for the record, I was talking about migrating a company from POP3 to IMAP
> where they use TB.  I'm pretty sure TB 2 and earlier allowed you to change
> protocol when editing an account, and frankly I don't consider it that
> difficult given the number of shared settings vs the number of different ones.

Adrien,

I would happily review any patch you care to post which would let us switch account from POP3 to IMAP (or back).  Please add me to the bug, when you file it.  :)

(Sadly, I don't have time to write the patch myself, but I would certainly make time to review it, since we get that request quite a bit.)

Thanks,
Blake.

Comment 142

[rsx11m]

> (comment #140)  I'm pretty sure TB 2 and earlier allowed you to change
> protocol when editing an account, ...

No, it didn't. Changing the account type always involved creating a new and deleting the old one.

Comment 143

[rsx11m]

(In reply to comment #141)
> I would happily review any patch you care to post which would let us switch
> account from POP3 to IMAP (or back).  Please add me to the bug, when you file
> it.  :)

Bug 87948 for changing type of existing accounts.

Comment 145

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 442854 [details] [diff] [review]
v12, emailWizard.js

>    document.getElementById("mastervbox").setAttribute("style", "min-width: " + document.width + "px");
>    document.getElementById("mastervbox").setAttribute("style", "min-height: " + document.height + "px");

This is bogus. In the first place, you're overwriting the min-width when you set the min-height. In the second place, document.width and document.height will be removed as soon as Gecko 2.0 branches.

Comment 146

[Ben Bucksch (:BenB)]

Attachment: Fix, v13 - Review comments addressed

Comment 147

[Ben Bucksch (:BenB)]

Comment on attachment 512954 [details] [diff] [review]
Fix, v13 - Review comments addressed

I think the only review comment not addressed is the bracket style.

Comment 148

[Ben Bucksch (:BenB)]

Attachment: Review comment responses

So, my dear Blake. This file contains all your review comments together, which are, if they were print on paper, a whooping 30 pages.

I put all my comments in between, including the existing ones already here in the bug, for better overview. I added some, and marked everything that changed. I adressed all comments - if I didn't do something, the reason is noted here.
I managed to add only 12 pages to your 30 - that includes all the existing comments and the new ones.

Of course, my code changes as result of your review requests are far bigger.

---

I hope that's it for this bug, finally, after almost one year. *Please* don't do a full review again, I can't do any more. I am far beyond my limits of what I can take, and what I can justify as time and nerve investment. I have a day job as freelancer, and I lose money when I work on this. I hope that's understandable.

I will still change the bracket style, as promised. If there's anything else on this bug, I will now draw the joker, namely that you promised me originally to help me :).

Comment 149

[Ben Bucksch (:BenB)]

Attachment: Fix, v14 - Review comments addressed

Also changed bracket style

Comment 150

[Thomas D. (:thomas8)]

Ben, thanks for the incredible work that you have done on this bug!!!

A very small question: Does the current patch fix bug 520964 (focus name entry on load), as requested by comment 25 and confirmed by you in comment 26? If yes, we should set the dependency accordingly.

Comment 151

[Ben Bucksch (:BenB)]

>     this.switchToMode("start");
>     e("realname").focus();

Comment 152

[Ben Bucksch (:BenB)]

Builds (didn't test them myself)
http://stage.mozilla.org/pub/mozilla.org/thunderbird/tryserver-builds/[e-mail]-7b3169e0995b/tryserver-linux/thunderbird-3.3a3pre.en-US.linux-i686.tar.bz2
http://stage.mozilla.org/pub/mozilla.org/thunderbird/tryserver-builds/[e-mail]-7b3169e0995b/tryserver-win32/thunderbird-3.3a3pre.en-US.win32.zip
http://stage.mozilla.org/pub/mozilla.org/thunderbird/tryserver-builds/[e-mail]-7b3169e0995b/tryserver-macosx64/thunderbird-3.3a3pre.en-US.mac.dmg

Comment 153

[Jens Wahnes]

Do the builds mentioned in Comment 152 actually address the issues raised here? I very briefly tried the Windows build and the account creation wizard seems to be quite buggy. For example, the "Continue" button is still much bigger than the others, entering a password isn't required, and when going from autoconfig to manual to advanced and then cancelling the whole thing, an account is still created. I didn't go any further than that, because it really doesn't seem like there is anything fixed in the account creation wizard of this build.

Comment 154

[Reindl Harald]

Why in the world there will not be included a "manual setup" button in RECENT thunderbird-versions since this useless **** does not work since longer than a year and since you guys do not understand that is **** terrible force domain-admins provide a autoconfig-subdomain for broken mailclients from ignorant developers?

Is it funny to release broken **** wiht the reason "it's free"?

Comment 155

[A.P. Veening]

(In reply to comment #154)
> Why in the world there will not be included a "manual setup" button in RECENT
> thunderbird-versions since this useless **** does not work since longer than a
> year and since you guys do not understand that is **** terrible force
> domain-admins provide a autoconfig-subdomain for broken mailclients from
> ignorant developers?
> 
> Is it funny to release broken **** wiht the reason "it's free"?

Because the developers have completely lost contact with ordinary users and how those like things.

Comment 156

[Ben Bucksch (:BenB)]

> useless crap

Reindl, one more comment like that from you - or A.P.Veening or anybody else - here or anywhere else - and you'll be banned from bugzilla. You have been warned before.

I won't put up with this hate speech anymore. Not after sinking lots of my *free time* into it.

This bug is CLOSED FOR COMMENTS from non-developers.

> the "Continue" button is still much bigger than the others

That was intentional. It's just style anyway, just one line of CSS or classes. We can adapt the visual style to whatever Byran wants. I'll stay out of that.

> entering a password isn't required

That's bug 634078.

> and when going from autoconfig to manual to advanced and then cancelling
> the whole thing, an account is still created.

bwinton questioned whether this is even wanted.
I agree with you, but it was never part of this bug. It's a separate bug.

> it really doesn't seem like there is anything fixed

Very qualified comment, yes. I already regret that I attached test builds.

Comment 157

[Ben Bucksch (:BenB)]

As an every so slight and non-obvious hint, the scope of what this bug must fix is defined by the initial description (first comment).

Comment 158

[Reindl Harald]

> Reindl, one more comment like that from you

Nice, now we are at "shut up users we do what we want"

> Not after sinking lots of my *free time* into it

It would be better the WHOLE time anybody spent in this useless 
wizard were used for things which making sense instead break
thunderbird for more than a year 

Throw away the wizard and release a working TB 3.1.x and you 
will get not such comments

why do you not understand taht a simply button to skip the wizard IS NEEDED for endusers and would take away the pressure from developers which are unable to fix this since more than a year?

Comment 159

[A.P. Veening]

(In reply to comment #156)
> > useless ****
> 
> Reindl, one more comment like that from you - or A.P.Veening or anybody else -
> here or anywhere else - and you'll be banned from bugzilla. You have been
> warned before.

I haven't been warned before. And if any of you developers ever bothered looking at the questions and complaints posted at http://forums.mozillazine.org and the amount of time and effort (other) volunteers -of which I am one- put into fixing things you developers just plain broke, you might have any idea about the amount of frustration that is going around.
> 
> I won't put up with this hate speech anymore.

Since when is an honest complaint about a real problem "hate speech"?

> Not after sinking lots of my *free time* into it.

How much time do you think the volunteers at http://forums.mozillazine.org are sinking in fixing things the developers broke?

> 
> This bug is CLOSED FOR COMMENTS from non-developers.
> 
I'll stay out of it again as I have better things to do than discussing this on the internet (like helping Thunderbird users at http://forums.mozillazine.org).

Comment 160

[Mark Banner (:standard8)]

(In reply to comment #158)
> > Reindl, one more comment like that from you
> 
> Nice, now we are at "shut up users we do what we want"

Reindl, A.P.Veening, I appreciate that you may be frustrated. However, please consider how you would feel if someone reacted as you did above to something that you worked hard on.

Please review https://bugzilla.mozilla.org/page.cgi?id=etiquette.html before posting to bugzilla again.

Comment 161

[Reindl Harald]

> However, please consider how you would feel if someone reacted 
> as you did above to something that you worked ****

If it take longer than a year and it seems that nobody needs what i do and i am not able to get this thing that i would like working i would trash it instead of making users crazy with things they do not really need

Comment 162

[Robert Kaiser]

Guys, calm down. Ben has created a 200 kilobyte (!) patch here that improves a lot of things in this UI and its code. That's a real lot of volunteer work he did put into this and it helps with a number of issues.
This surely doesn't fix all problems you might have with it, and one bug can't do everything, but you're quite right that there's more to be done. Please make sure that other bugs are filed on each of those problems (one bug per issue, please) and then people can work on them as further steps building on the good work already happening here.
Of course, you can always speed up things by writing patches yourself, this is an open source project after all (and yes, I emphasize it one more time, Ben also is a user who volunteers his free time to improve things here and come up with patches).

Comment 163

[Ben Bucksch (:BenB)]

Attachment: Fix, v15 - Review comments addressed

One-line change (misspelled variable name), fixes us hanging on successful password verification.

Comment 164

[Nomis101]

(In reply to comment #152)
> Builds (didn't test them myself)
> http://stage.mozilla.org/pub/mozilla.org/thunderbird/tryserver-builds/[e-mail]-7b3169e0995b/tryserver-linux/thunderbird-3.3a3pre.en-US.linux-i686.tar.bz2
> http://stage.mozilla.org/pub/mozilla.org/thunderbird/tryserver-builds/[e-mail]-7b3169e0995b/tryserver-win32/thunderbird-3.3a3pre.en-US.win32.zip
> http://stage.mozilla.org/pub/mozilla.org/thunderbird/tryserver-builds/[e-mail]-7b3169e0995b/tryserver-macosx64/thunderbird-3.3a3pre.en-US.mac.dmg

Today I've tested the Windows and the Mac try build. Sadly the Windows version was unable to find the ISP configuration, but I think this is because of the very strict Firewall settings in my lab. The Mac trybuild, I've tested at home, worked perfect. And I think the improvement for an inexperienced user is great. With this easy setup even my mom can set up a new Thunderbird account now. :-)
The only thing, in the very first window you get, there is no indication that it is still possible to set up the account manually. You see the manual setup button the first time in the second dialog window. So maybe this can seem, that you only have the autoconfig choice.

Comment 165

[rsx11m]

I've given the try-server builds (patched with the v14/v15 delta) a thorough testing on Linux and Windows, and overall this works very well for me. I ran into a couple of minor things or suggestions which I'm summarizing below, and which I'm happy to spin off into follow-up bugs unless they are trivial enough that you (BenB) want to address them here.

Testing scenarios included a ISPDB-listed server (Gmail), an unlisted but "guessable" configuration, and an unlisted and unguessable configuration.
The Gmail setup went just fine, as expected.

While the server name, ports, and connection encryption where guessed correctly in the 2nd scenario, I ran into a regression of bug 491202 (no fallback to password authentication when Kerberos/GSSAPI is advertised but not configured). After going to "Manual Config" and changing to "Normal Password", retesting
and account creation was successful (switching to "Autodetect" just gave me Kerberos again, so that didn't help).

On a side note, the initial error message of "user name or password are wrong" didn't reflect that the Kerberos-ticket failed and may be misleading for the unsuspecting user. This actually seems to be a regression from bug 525238 rather as I see the same behavior in the 3.1.8 release build, so this would probably be a candidate for a separate bug either way.

The "unguessable" configuration was tested twice: One is letting auto-config fail, the other clicking on "Manual Config" immediately. Typing in the correct server names and re-testing provided valid configurations, so that's a pass for both scenarios (and interrupting the guessing with the "Manual Config" button worked without any delay, that's a major improvement over the old wizard).

I didn't test changing from IMAP to POP as I don't use the latter and didn't want to mess up my server-side folders, but I've noticed an inconsistency when the configuration is listed (then you get radio-boxes to switch from IMAP to POP) versus unlisted/guessed configurations (here you don't have radio boxes though the POP ports appear to be tested, you have to go into the "Manual Config" to switch to POP and then retest. This too may be a follow-up bug.

Some trivial things I've noticed:
 - "Manual/Advanced" is disabled when the no connection is present, on purpose?
   This may require re-scoping of bug 583602 given that the behavior is changed.
 - If a configuration has been successfully identified, maybe switch the label
   from "Manual" to "Edit Config", thus making it clear that you won't start
   from scratch.
 - The "SSL/TLS" option is labeled "Normal SSL", which is inconsistent and a
   bit irritating (is "STARTTLS" not normal by that definition?).

Again, it works fine as is and is an improvement over the old wizard, thus IMHO should go into the nightly builds for broader community testing.

Comment 166

[rsx11m]

(In reply to comment #164)
> The only thing, in the very first window you get, there is no indication that
> it is still possible to set up the account manually. You see the manual setup
> button the first time in the second dialog window. So maybe this can seem, that
> you only have the autoconfig choice.

I don't think it would be good to show a disabled "Manual Config" button at this stage. What should help is an introductory label to communicate what's going to happen, something like: "To set up your account, please enter your account information above (note that the password will only be sent to your e-mail provider). Thunderbird will try to automatically find a matching configuration, or you can set up your account manually if this doesn't work."

-> Spin-off bug I'd say, once the main part has passed...

Comment 167

[Adrien de Croy]

If there's going to be an explanatory page about how TB will try to automatically do the account setup, this is a prime place to put a couple of radio buttons.  One giving the option to go straight to manual.

Giving a choice at the start.  Make the default automatic by all means.

Having to wait for the automatic process to give up would be a real pain in the neck and waste of time for people who know what they're doing and just want to type in some servernames etc.

Comment 168

[rsx11m]

Adrien, did you read my comment #165? You don't have to wait for anything to fail, that was the 2nd part of my 3rd testing scenario. Also, there "won't be" any explanatory page in *this* bug, that was a suggestion for a follow-up.

Comment 169

[Ben Bucksch (:BenB)]

Thanks, rsx, for the feedback.

> regression of bug 491202

hm, that's strange, because I do have the same code (in fact more exhaustive) in emailWizard.js:
      this.verifyConfig(function(successfulConfig) { // success
        self._currentConfig.incoming.auth = successfulConfig.incoming.auth;
        self._currentConfig.outgoing.auth = successfulConfig.outgoing.auth;

I checked verifyConfig.js, and I didn't change the relevant code at all there.
The code *did* change very much, though, by me in bug 525238. This is most likely a regression from that bug, not this patch here. Are you sure this works on stock 3.1?

> the initial error message of "user name or password are wrong"
> didn't reflect that the Kerberos-ticket failed and may be misleading for the
> unsuspecting user.

100% agreed, I consider that serious, too, but that's precisely bug 555448. It's not a regression. In fact, I had added a TODO in the code in this patch.

> This actually seems to be a regression from bug 525238

I don't think it's a regression, nor that it's related to the error msg.
(I implemented that bug.)

> - "Manual/Advanced" is disabled when the no connection is present,
> on purpose? This may require re-scoping of bug 583602 given that
> the behavior is changed.

Not on purpose, but I didn't test that scenario at all. It's currently not supported, and I'd leave it to another bug (e.g. the one you mentioned).

> - If a configuration has been successfully identified, maybe switch the label
>   from "Manual" to "Edit Config", thus making it clear that you won't start
>   from scratch.

I'd rather leave it as-is. This "Manual" intentionally expresses that the user overrides us.
If we can't find a working config, we'll typically switch to manual mode automatically.

> - The "SSL/TLS" option is labeled "Normal SSL", which is inconsistent and a
>    bit irritating (is "STARTTLS" not normal by that definition?).

hm, you're right. I thought I used "Normal SSL" as well in the pref dialogs, but indeed they show "SSL/TLS" (maybe that was a review change there), so this should be changed to be consistent, yes.
DONE

> Again, it works fine as is and is an improvement over the old wizard, thus IMHO
> should go into the nightly builds for broader community testing.

Thanks for this vote of confidence! :)

Comment 170

[rsx11m]

You are welcome.

(In reply to comment #169)
> > regression of bug 491202
> The code *did* change very much, though, by me in bug 525238. This is most
> likely a regression from that bug, not this patch here. Are you sure this works
> on stock 3.1?

My suspicion as well. It used to work with 3.0.x, but no longer on 3.1.x already, thus that issue is independent of the account-wizard redesign here.
The idea in bug 491202 was to fall back automatically to "Normal Password" if Kerberos fails, as long as connection security is either SSL/TLS or STARTTLS.

> 100% agreed, I consider that serious, too, but that's precisely bug 555448.
> It's not a regression. In fact, I had added a TODO in the code in this patch.

Ok, thanks. I didn't look into the code in that much detail... ;-)

> I'd rather leave it as-is. This "Manual" intentionally expresses that the user
> overrides us.
> If we can't find a working config, we'll typically switch to manual mode
> automatically.

Good point. Yes, the 3rd test scenario brought me directly into manual setup.

Comment 171

[Ben Bucksch (:BenB)]

> bug 491202 ... It used to work with 3.0.x, but no longer on 3.1.x already

Re-reading that bug, the currently last comment there (comment 54, from 2009-11-21) is from me and mentions this precise issue that you raise, I think.
Good, so it's not relevant here.

Comment 172

[Neville Hillyer]

Notwithstanding the assurances of rx11m I would also like to see initial options as expressed in comment #167. There are occasions when it is expedient to bypass the Wizard and set-up from within TB. This can be useful in both on-line and off-line situations. I have had so much difficulty with multiple POP installations that I resorted to this unorthodox method: http://links.open.ac.uk/thunderbird/seed-1.html

Whilst Ben's work here looks very encouraging I hope we can look forward to the following options at the start of the process:

1 - automatic setup
2 - manual setup
3 - no setup at this time

Clearly option '1' will not work off-line but the other two should.

Many of us are here because merged bugs asked for such things.

Comment 173

[Ben Bucksch (:BenB)]

Neville, this is OFFTOPIC on this bug. rsx already said this in comment 168.
You also know the clear directive "NO COMMENTS unless you are a developer" in the whiteboard. I will not tolerate this further on this bug.

Comment 174

[Ben Bucksch (:BenB)]

(and you're repeating yourself, compare comment 114 ff)

---

New try builds with comment 163 fixed should appear soon at
<http://ftp.mozilla.org/pub/mozilla.org/thunderbird/tryserver-builds/[e-mail]-e2d2a4ddd638>

Comment 175

[Neville Hillyer]

As I said Ben:

"Many of us are here because merged bugs asked for such things".

Comment 176

[rsx11m]

Neville, it's enough. At least test the try-server builds before commenting.

Comment 177

[Ben Bucksch (:BenB)]

Neville, no, bug 531099 was un-duped for this very reason so that you don't have an excuse to spam this bug. That bug was WONTFIX, so you are directly violating 
https://bugzilla.mozilla.org/page.cgi?id=etiquette.html
1.1 (repeated comments) 1.2. (do what I want or I'll harass you until you do) and 2.2 (whining about WONTFIXed bugs).

rsx, no, I don't want ANY comments from users here anymore. I'm totally sickened by the harassment. If you think you found a bug in the test build, people can may email me. But only for bugs.

Comment 178

[rsx11m]

(In reply to comment #169)
> 
> I checked verifyConfig.js, and I didn't change the relevant code at all there.
> The code *did* change very much, though, by me in bug 525238. This is most
> likely a regression from that bug, not this patch here.

Spun off as bug 635628.

Comment 179

[Neville Hillyer]

In reply to comment #176

I would if I could find one which is compatible with my Mac.

Comment 180

[rsx11m]

(In reply to comment #165)
> 
> I didn't test changing from IMAP to POP as I don't use the latter and didn't
> want to mess up my server-side folders, but I've noticed an inconsistency when
> the configuration is listed (then you get radio-boxes to switch from IMAP to
> POP) versus unlisted/guessed configurations (here you don't have radio boxes
> though the POP ports appear to be tested, you have to go into the "Manual
> Config" to switch to POP and then retest. This too may be a follow-up bug.

This is bug 560051, assuming that the IMAP/POP radiobuttons will directly show
up for guessed configurations as well once guessConfig() returns both options.

Comment 181

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 513461 [details] [diff] [review]
Fix, v15 - Review comments addressed

The error text is too long as shown in
http://dl.dropbox.com/u/2301433/Screenshots/Big%20Error.png

The way I see it, we've got three options:
1) Make the dialog bigger.
2) Make the error message wrap.
3) Move the error messages underneath the fields.
And while I have my preference, I'll defer to Bryan's choice as to which we should do.


http://dl.dropbox.com/u/2301433/Screenshots/Big%20Username.png

We need to align the text labels with their values.


In half-manual config, if I set the outgoing SSL to "None", and hit
Re-test, it re-sets it to STARTTLS (which my server offers, but I _did_
explicitly choose "None").


I got
JavaScript error: chrome://messenger/content/accountcreation/verifyConfig.js, line 242: this.mConfig.incoming.authAlternatives is null
When hitting "Create Account" for a Manual Config of a GMail account with
an invalid password.  And the "Checking passwords" spinner spins forever.

Sounds like it might be related to what you ran into when you said:
> During testing, I found an old bug: I have a profile where
> removeIncomingServer in cleanup() in verifyConfig is throwing. No idea
> why. It worked a few days ago in this profile, and still works in the
> another, fresher profile. This causes us to hang during password
> verification. Same happens with the old dialog, without my changes. I
> added a try/catch.


When I create an account ([e-mail]), delete it, try to create it again, I get an "Incoming Server already exists" error.
http://dl.dropbox.com/u/2301433/Screenshots/Which%20Incoming%20Server.png


I still think "Advanced Setup" should let us know that it's going to create
the account for us somehow, but since I can't think of a good way to word
it, I'm happy to push it off to a followup bug.


>>>  onResultIMAPOrPOP3 : function()
>> [snip…]
>>>    config.incomingAlternatives = config.incomingAlternatives.filter(
>>>        function(e) { return e != config.incoming; }); // TODO doesn't work
>> We should probably fix this, if it doesn’t work.
> If I knew why it doesn't work, and how to fix it, I'd be delighted to do so.
> 
> IIRC, it doesn't create an actual problem, it just appends one duplicate object
> to incomingAlternatives each time the user clicks a radio button, but
> it doesn't affect functionality.

Okay, let's put that in the comment, and leave it for later.


The RTL-version doesn't look quite right to me:
http://dl.dropbox.com/u/2301433/Screenshots/RTL%20First%20screen.png
http://dl.dropbox.com/u/2301433/Screenshots/RTL%20Second%20screen.png
but since I sit near Ehsan, I think it makes the most sense to file a
followup bug, and assign it to me, and I'll grab him and fix it later.


>> Bug 506290 - Layout changes to quick account setup
>Impossible to merge this.
>
>This was done after I attached my patch and before my review was done, and
>it's a substantial change. I had explicitly asked not to do that.
>
>Luckily Blake Winton agreed that I may revert this change, if I match the
>requirements. Unfortunately, I am not clear what they are, in comparison
>to what I have or discussed with Bryan. If the new layout does not please,
>please let me know, we'll have to reimplement that.

Yeah, as far as I'm concerned we can file followup bugs to change whatever
layout we want in the new UI.  (The main one I miss are the spinners for
incoming and outgoing servers, which turned into popups like at the bottom
of https://bug506290.bugzilla.mozilla.org/attachment.cgi?id=398758 )

I will ask Bryan for his ui-r, though, to clear up what we think the requirements are (or what we'll need to change after this lands).


So yeah, I'm pretty happy with the changes you've made, and the reasons for
the changes you didn't make, so I'm going to say r=me on this patch, and we
can file new bugs for the outstanding issues.  (Oh, and we should let protz finish fixing the tests so that we can land both patches at the same time, and not have breakage.)

Thank you very much for your work on this, Ben.

Later,
Blake.

Comment 182

[Ben Bucksch (:BenB)]

> The error text is too long as shown in

Can we pick the easiest solution for now (maybe wrap it), and leave the rest to a followup bug?

> We need to align the text labels with their values.

They are already in an <hbox> each, so hopefully that's just a matter of CSS, e.g. -moz-box-align: baseline or something like it.

> When I create an account ([e-mail]), delete it, try to create
> it again, I get an "Incoming Server already exists" error.

You're deleting in the normal Account Manager, right? This is a Thunderbird backend bug. There's nothing we can do about it, and I'd think that the dialog before had the same problem.

> if I set the outgoing SSL to "None", and hit Re-test, it re-sets it
> to STARTTLS

That's a bug, that shouldn't happen.

> I got verifyConfig.js, line 242
> this.mConfig.incoming.authAlternatives is null
> When hitting "Create Account" for a Manual Config of a GMail account with
> an invalid password.

I probably broke that in some of my changes last week.

> I still think "Advanced Setup" should let us know that it's going to create
> the account for us somehow, but since I can't think of a good way to word
> it, I'm happy to push it off to a followup bug.

Likely, it needs more text, so maybe a popup dialog.
A followup bug would be best, given that the problem also exists in the old wizard *and* this patch considerably improves it (by going into "Manual Config" mode first, before you can go to this "Advanced Config").

> RTL

*snicker* Never thought of that. But it looks good to my. The ":" are off, but they are localized, so no problem there.
Only the alignment of the email address and hostname fields looks wrong.

> > Bug 506290 - Layout changes to quick account setup

> Yeah, as far as I'm concerned we can file followup bugs to change whatever
> layout we want in the new UI.

Thanks! That removes a big scary thing for me.

> the spinners for incoming and outgoing servers

They don't exist at all anymore, they are gone without replacement. That's stated part of this bug since the very beginning, see initial description: "I'll probably remove the hostname updates in the fields" and the justification there.

> I'm pretty happy with the changes you've made, and the reasons for
> the changes you didn't make

Thanks! :-)

> I'm going to say r=me on this patch

Whooooo! :-)

Comment 183

[Ben Bucksch (:BenB)]

> they are gone without replacement

(because they are no hostname fields anymore, intentionally)

Comment 184

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #182)
> > The error text is too long as shown in
> Can we pick the easiest solution for now (maybe wrap it), and leave the rest
> to a followup bug?

That was my idea, too!  :)
(Well, I thought it would be easier to just make the dialog bigger, but whatever works.)

> > We need to align the text labels with their values.
> They are already in an <hbox> each, so hopefully that's just a matter of CSS,
> e.g. -moz-box-align: baseline or something like it.

Sounds good.  Would you rather a followup patch on this bug, or should we fix it in the UI-fix bug?

> > When I create an account ([e-mail]), delete it, try to create
> > it again, I get an "Incoming Server already exists" error.
> You're deleting in the normal Account Manager, right? This is a Thunderbird
> backend bug. There's nothing we can do about it, and I'd think that the dialog
> before had the same problem.

Probably.  Do we have a bug for that?  Bienvenu?

> > if I set the outgoing SSL to "None", and hit Re-test, it re-sets it
> > to STARTTLS
> That's a bug, that shouldn't happen.
> > I got verifyConfig.js, line 242
> > this.mConfig.incoming.authAlternatives is null
> > When hitting "Create Account" for a Manual Config of a GMail account with
> > an invalid password.
> I probably broke that in some of my changes last week.

Hurray!  I can still find bugs!  :)

Did you want to fix these here, or in a followup bug?

> > I still think "Advanced Setup" should let us know that it's going to create
> > the account for us somehow, but since I can't think of a good way to word
> > it, I'm happy to push it off to a followup bug.
> Likely, it needs more text, so maybe a popup dialog.
> A followup bug would be best, given that the problem also exists in the old
> wizard *and* this patch considerably improves it (by going into "Manual Config"
> mode first, before you can go to this "Advanced Config").

I'm happy with a followup bug for that.

> > RTL
> *snicker* Never thought of that. But it looks good to my. The ":" are off, but
> they are localized, so no problem there.
> Only the alignment of the email address and hostname fields looks wrong.

Yeah, that was what I was thinking.  Also, the email address and server hostnames should probably be aligned the same as the name and password fields.  It seems like minor stuff, though.

> > > Bug 506290 - Layout changes to quick account setup
> > Yeah, as far as I'm concerned we can file followup bugs to change whatever
> > layout we want in the new UI.
> Thanks! That removes a big scary thing for me.

Well, don't get too comfortable.  We'll see if Bryan agrees with me.  :)

> > the spinners for incoming and outgoing servers
> They don't exist at all anymore, they are gone without replacement. That's
> stated part of this bug since the very beginning, see initial description:
> "I'll probably remove the hostname updates in the fields" and the justification
> there.

Yeah, I do kind of miss those popups though.  (Although I suspect I was the only person to hover over the green and yellow dots…  ;)

Later,
Blake.

Comment 185

[David :Bienvenu]

(In reply to comment #184)

> > > When I create an account ([e-mail]), delete it, try to create
> > > it again, I get an "Incoming Server already exists" error.
> > You're deleting in the normal Account Manager, right? This is a Thunderbird
> > backend bug. There's nothing we can do about it, and I'd think that the dialog
> > before had the same problem.
> 
> Probably.  Do we have a bug for that?  Bienvenu?

I do remember encountering this issue in the initial account wizard development so I do believe it's an existing issue. I don't know if there's a backend bug for it.  The issue is fixed by shutting down and restarting, iirc, so it's an in-memory datastructure issue.

Comment 186

[Ben Bucksch (:BenB)]

> I thought it would be easier to just make the dialog bigger

If you consider French, which can be twice as long as English (even the textfield labels on the same line), that doesn't work very well.

> > > We need to align the text labels with their values.

> > They are already in an <hbox> each

> Would you rather a followup patch on this bug, or should we fix
> it in the UI-fix bug?

I think a followup bug would be best, because it's not trivial to fix - and it's just about a few pixels:

They should be a <grid>, actually. If the labels are longer (French, German etc.!), the alignment in the other direction is off, too.

In fact, the same problem already exists for the name/email/password textfields, even in the old dialog (I intentionally didn't touch that part). Take a look how at TB 3.1 in French - it looks awful!

Part of the problem is that we're using different font sizes in this case. Not even a <grid> would fix that, in fact the <grid> might make it harder. So, it's not trivial and I'd rather deal with that in a followup bug.

> Hurray!  I can still find bugs!  :)
> Did you want to fix these here

Yes, that's really broken, I want to fix that.

> RTL

I'm looking at the code, and these fields have a class="uri-element". I didn't add that, it was there before. It looks quite intentional to me, because even in RTL, these fields have English content. It seems to intentionally force these fields to LTR. In any case, I didn't change that.

> I do kind of miss those popups though

We have (since 3.0) a very reddish dialog, with red and yellow dots, for insecure servers. I hope that's colorful enough. :-)

Comment 187

[Ben Bucksch (:BenB)]

Filed followup bugs:
Bug 636018 - Error text cut of
Bug 636016 - Describe implication of "Advanced Config"
Bug 636014 - Align labels and textfields
Bug 636017 - [Account Manager] Delete and re-create doesn't work,
             account leaves in-memory traces

Comment 188

[Ben Bucksch (:BenB)]

> In half-manual config, if I set the outgoing SSL to "None", and hit
> Re-test, it re-sets it to STARTTLS (which my server offers, but I _did_
> explicitly choose "None").

How could this escape me? It seems to work for in the incoming server, though.

This guy must be hiding somewhere in guessConfig.js getOutgoingTryOrder() or getHostEntry(). But my eyes are too tired right now.


> I got
> JavaScript error: chrome://messenger/content/accountcreation/verifyConfig.js,
> line 242: this.mConfig.incoming.authAlternatives is null
> When hitting "Create Account" for a Manual Config of a GMail account with
> an invalid password.  And the "Checking passwords" spinner spins forever.

Line 242 is a comment in my version of the code. However, I commited a fix that might fix it, see:
<https://hg.mozilla.org/users/mozilla.BenB_bucksch.org/tb-autoconfig-ui-1/rev/34ab5d36376f>

If this is still hanging for you, could you please enable debug (see top of emailWizard.js: To debug, set mail.wizard.logging.dump (or .console)="All" and kDebug = true) and tell me what's you see there? If the last thing you use is "username=...", then it's bug 635241.

Comment 189

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #188)
> > I got
> > JavaScript error: chrome://messenger/content/accountcreation/verifyConfig.js,
> > line 242: this.mConfig.incoming.authAlternatives is null
> > When hitting "Create Account" for a Manual Config of a GMail account with
> > an invalid password.  And the "Checking passwords" spinner spins forever.
> Line 242 is a comment in my version of the code. However, I commited a fix that
> might fix it, see:
> <https://hg.mozilla.org/users/mozilla.BenB_bucksch.org/tb-autoconfig-ui-1/rev/34ab5d36376f>

That seems to have fixed it.  Thanks!

Later,
Blake.

Comment 190

[Jonathan Protzenko [:protz]]

Attachment: Fix, v16: a version with all tests passing on my Linux box

Here's a big updated patch. It was obtained by:
- taking BenB's private branch's tip
- applying modifications on top of it.

Here's a few remarks:
- I noticed an inconsistency in the XUL file between outgoing_host and incoming_hostname. I had to change a control="outgoing_hostname" to control="outgoing_host" to make it match.
- As I told you on IRC, the old version of the wizard, when guessing the parameters for [e-mail], would set [e-mail] for the outgoing SMTP username. The patch now sets john.doe as the outgoing username. This makes more sense in my opinion.
- One thing that bugs me, but it might be too late to chime in, is that the "advanced setup" button does not make it clear that the account *will be created* with the current parameters, and that you will be redirected to the page that allows you to edit the advanced settings. The workflow is great, and works fine, except that it surprised me a little that the account was created.

I understand that technically speaking, there's not much we can do and that UI-wise, it's kinda hard to convey this information in the text of the button, so I'm not sure there's much we can do...

Anyway, I have a great feeling about this patch, and I feel like it will solve most of our problems concerning the account wizard. Thanks for your work!

I'll be sending an interdiff between v15 and v16 shortly.

Comment 191

[Jonathan Protzenko [:protz]]

Attachment: Fix v16b: left some debug mc.sleep

I just realized I left some debug mc.sleep calls in that patch, here's a clean one.

Comment 192

[Jonathan Protzenko [:protz]]

Attachment: v15 to v16b, i.e. the modifications to the tests

And here's my modifications separated from the main patch.

Comment 193

[Jonathan Protzenko [:protz]]

Comment on attachment 513461 [details] [diff] [review]
Fix, v15 - Review comments addressed

Resetting the ui-review flag to its original value.

Comment 194

[Ben Bucksch (:BenB)]

(In reply to comment #190)
> Fix, v16: a version with all tests passing on my Linux box

Thanks, protz! :-)

> - One thing that bugs me, but it might be too late to chime in, is that the
> "advanced setup" button does not make it clear that the account *will be
> created* with the current parameters, and that you will be redirected to the
> page that allows you to edit the advanced settings. The workflow is great, and
> works fine, except that it surprised me a little that the account was created.
> 
> I understand that technically speaking, there's not much we can do and that
> UI-wise, it's kinda hard to convey this information in the text of the button,
> so I'm not sure there's much we can do...

bwinton agrees with you, and so I do. I filed followup-bug 636016 about it. We discussed that above, and as said there, this problem also exists in the old wizard, and is much more severe there than with this patch.

> Anyway, I have a great feeling about this patch, and I feel like it will solve
> most of our problems concerning the account wizard.

Thanks :)

Comment 195

[Ben Bucksch (:BenB)]

I pushed your changes to my branch

Comment 196

[Jonathan Protzenko [:protz]]

I pushed to try. No failures related to my updated tests were found, except on Linux x86-64 tryserver opt test mozmill (timeout). That's just weird since it's the only build that failed, and that also happens to be the platform I'm developing with. I'll check tomorrow see if I did a mistake when updating the test.

Still waiting for the Win test results, I'll leave a comment later on with the results.

Comment 197

[Blake Winton (:bwinton) (:☕️)]

Bryan,
I believe (although protz should feel free to correct me) that you can download his try-builds from http://ftp.mozilla.org/pub/mozilla.org/thunderbird/tryserver-builds/[e-mail]-66b6fc86ccf9/ if you wanted to do the UI-review without re-compiling everything.  :)

Later,
Blake.

Comment 198

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 514779 [details] [diff] [review]
v15 to v16b, i.e. the modifications to the tests

>+++ b/mailnews/base/prefs/content/accountcreation/emailWizard.xul
>@@ -318,7 +318,7 @@
>-                   control="outgoing_hostname"/>
>+                   control="outgoing_host"/>

(See below.)

>+++ b/mail/test/mozmill/account/test-mail-account-setup-wizard.js
>@@ -148,7 +148,7 @@
>     "outgoing server username": {
>-      actual: outgoing.username, expected: user.email
>+      actual: outgoing.username, expected: user.email.split("@")[0]

Just to offer some justification for this change:
bwinton:ispdb_data/ $ egrep -hir "<username>" . | sort | uniq -c
 224       <username>%EMAILADDRESS%</username>
   8       <username>%EMAILLOCALPART%.%EMAILDOMAIN%</username>
 658       <username>%EMAILLOCALPART%</username>

So almost three times as many configs expect just the first part of the email address.

>@@ -209,19 +209,20 @@
>+    assert_equals(awc.e("outgoing_host").value, user.outgoingHost,
>+    assert_equals(awc.e("incoming_hostname").value, user.incomingHost,

Why outgoing_host, but incoming_hostname?

>@@ -254,42 +255,44 @@
>+    // Password checkbox is not disabled anymore, so we can't really check for
>+    //  that...
>+    // awc.assertProperty(rememberPassword, "disabled", true);
>+    // awc.assertNotChecked(rememberPassword);

Feel free to delete these, then.  We've got the hg log if we want them back.

>+    // password field is empty -> disable and uncheck checkbox
>+    // awc.assertProperty(rememberPassword, "disabled", true);
>+    // awc.assertNotChecked(rememberPassword);

(These too.)

>+++ b/mail/test/mozmill/account/test-retest-config.js
>@@ -91,37 +93,40 @@
>-  var incoming_server = awc.e("incoming_server");
>-
>-  var wizard_window = awc.e("autoconfigWizard");
>-
>-  var right = incoming_server.boxObject.y+incoming_server.boxObject.height;
>-  var bottom = incoming_server.boxObject.x+incoming_server.boxObject.width;
>-
>-  if (right > wizard_window.boxObject.height ||
>-      bottom > wizard_window.boxObject.width)
>-    throw new Error("The start over button didn't collapse the window.");
>+  // Previously, we'd switched to the manual editing state. Now we've started
>+  // over, we should make sure the information is presented back in its original
>+  // "automatic" mode.
>+  assert_true(!awc.e("manual-edit_button").hidden,
>+    "We're not back to the original state!");  
>+  assert_true(awc.e("advanced-setup_button").hidden,
>+    "We're not back to the original state!");  

I notice that we're not checking that the box is the correct size anymore…  Any reason for that?

>+++ b/mail/test/mozmill/shared-modules/test-account-manager-helpers.js
>@@ -52,6 +52,8 @@
>+var isLinux = ("@mozilla.org/gnome-gconf-service;1" in Cc);

I'm sure there are KDE people who will cry to see this line.  ;)

Could you use mc.isLinux(), as per https://developer.mozilla.org/en/Mozmill/Mozmill_Base_Object_Interfaces instead?


Other than that, I'm pretty happy with this, so r=me with the things I mentioned fixed or explained.

Thanks,
Blake.

Comment 199

[Ben Bucksch (:BenB)]

> Why outgoing_host, but incoming_hostname?

I have fixed that in the dialog in the meantime, but not in the test.

So, I fixed the above mentioned things (apart from the "justification") in my branch. I did not run the tests, though.

Comment 200

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 513461 [details] [diff] [review]
Fix, v15 - Review comments addressed

Here's the list of a couple of things I noticed as I was testing this.  I'll mark this ui-r- until we can discus or fix them.

* Because I'm on the Mac I happened to lose focus on the setup dialog after getting focus (see "make this dialog a sheet") so I got the error text beside the name entry before I had even started.  This isn't too big of a deal but would be a nice polish to what we have.  Could be a followup bug, doesn't have to block.

* The "looking up configuration" text layout is incorrect.
** It should be centered to the dialog
** It should be on two lines
** The spinner should be to the right of the text
** The look up engine shouldn't be in bold.
***  *Looking up configuration* (%)
***      $CONFIG_LOOKUP

* The success/error text is too long, "The following settings were found by trying common server names"
** "Configuration found using $SETTINGS_USED" e.g. "Configuration found using common server names"
** The success text should be normal font weight and centered
** The error text should be bold and centered and needs an /!\ alert icon preceding it.


* The port value entry needs to be a combo box similar to the outgoing host name.
** If you enter a value other than what is given the only way to reset is to type in the text "Auto" yourself.
** We should have a drop down that includes the guessed value and Auto as possible choices.


These I feel like are follow up bugs:

* ( Advanced Config ) button should have a popup dialog warning people this will create the account.  Also in the Mac it opens as a sheet on top of the dialog.

* This dialog should be a sheet on the mac as it often gets hidden by the size of Thunderbird opening and then lost.

Comment 201

[Jonathan Protzenko [:protz]]

> * ( Advanced Config ) button should have a popup dialog warning people this
> will create the account.  Also in the Mac it opens as a sheet on top of the
> dialog.
This is bug 63601. I think this issue's a thing everyone agrees on :-).

As to me, I'll wait for BenB to upload a new version of the patch that addresses the ui-review comments, and then I'll post an updated version of the patch I did for the tests, if that seems alright for you all.

Comment 202

[Ben Bucksch (:BenB)]

Bryan, thanks for the review!

Your feedback sounds very reasonable, and fair. I'll try to fix it.

> lose focus ... so I got the error text 

Agreed, I saw that, too. Maybe we should check the realname field only when the user presses Continue (but still check the email field as soon as he leaves the field).

> This dialog should be a sheet on the mac as it often gets hidden
> by the size of Thunderbird opening and then lost.

I had the same problem on Linux in fact: I clicked elsewhere in Thunderbird, and that made the main menu come to front, wizard behind (which I didn't see at all), and given that we enforce having only a single instance of the dialog, I could not get the wizard back again and thought stuff is broken.

I think we should just make the dialog modal, on all platforms. That would automatically make it a sheet on Mac, I think.

> port ... dropdown

That's a great idea. I can just populate the dropdown with "Auto" and all the applicable standard ports.

> [status text layout]

OK, will try to change it.

Comment 203

[Ben Bucksch (:BenB)]

> * Because I'm on the Mac I happened to lose focus on the setup dialog after
> getting focus (see "make this dialog a sheet") so I got the error text beside
> the name entry before I had even started.  This isn't too big of a deal but
> would be a nice polish to what we have.  Could be a followup bug, doesn't have
> to block.

I answered:
> Maybe we should check the realname field only when the
> user presses Continue

Actually, he can't press continue, because we disable it, until he enters some realname, so I can't do it like that.

I filed bug 638790 about it, but then had an idea, so I'll just include the fix here.

> * The "looking up configuration" text layout is incorrect.
> ** It should be centered to the dialog

DONE

> ** The look up engine shouldn't be in bold.

FYI, I don't see it bold, so I checked, and this was in the Mac style only, so I assume was intentionally done by you or somebody else earlier. I fixed it nevertheless.
DONE

> ** It should be on two lines
> ***  *Looking up configuration* (%)
> ***      $CONFIG_LOOKUP

Bryan, would you find it acceptable to leave this as one line? The status display code is currently generic (not just for "looking up"), including the string lookup and everything, and just one line. The strings are just one line, too. I'd have to change the code quite a bit to make it 2 lines and/or different bolding in this one case. I hope you can overlook this.

Please note that I was not moving around when it was left-aligned. You can make it left-aligned again with just a CSS change, see below.

> ** The spinner should be to the right of the text

DONE

> * The success/error text is too long, "The following settings were found by
> trying common server names"
> ** "Configuration found using $SETTINGS_USED" e.g. "Configuration found using
> common server names"

DONE, now using:
Configuration found on Thunderbird installation
Configuration found at email provider
Configuration found in Mozilla ISP database
Configuration found by trying common server names
(The "trying" in there is important, we discussed that exact wording before)

> ** The success text should be normal font weight and centered

DONE

> ** The error text should be bold and centered and needs an /!\ alert icon
> preceding it.

DONE

To summarize:
status is now:
- centered
- if loading: bold, and with spinner after the text
- if error: bold, and with /!\ before the text
All this defined in CSS, so feel free to play with it, and even adjust it per platform.

> * The port value entry needs to be a combo box similar to the outgoing host
> name.
> ** If you enter a value other than what is given the only way to reset is to
> type in the text "Auto" yourself.
> ** We should have a drop down that includes the guessed value and Auto as
> possible choices.

I wrote:
> That's a great idea. I can just populate the dropdown with "Auto" and all the
> applicable standard ports.

DONE

> These I feel like are follow up bugs:
> * ( Advanced Config ) button should have a popup dialog warning people this
> will create the account.  Also in the Mac it opens as a sheet on top of the
> dialog.

Indeed, bug 63601, as said

> * This dialog should be a sheet on the mac as it often gets hidden by the size
> of Thunderbird opening and then lost.

I changed the wizard to be a modal dialog.
I think that makes it a sheet on mac, but I can't test it.
DONE

Comment 204

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #203)
Ok, sounds good. Thanks a lot Ben!

Comment 205

[Ben Bucksch (:BenB)]

Thanks, Bryan! :-)
I'm currently trying to adjust the SSL value to port changes (where necessary). I'll make try builds afterwards.

Comment 206

[Ben Bucksch (:BenB)]

> I'm currently trying to adjust the SSL value to port changes (where necessary).

DONE

> I'll make try builds afterwards.

Kicked off. They should appear at
<http://ftp.mozilla.org/pub/mozilla.org/thunderbird/tryserver-builds/[e-mail]-0e3d891da4a8>.

Comment 207

[Ben Bucksch (:BenB)]

Attachment: Fix, v17 - UI review comments addressed

Comment 208

[Ben Bucksch (:BenB)]

Attachment: Interdiff v16 to v17

Comment 209

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 517015 [details] [diff] [review]
Fix, v17 - UI review comments addressed

>+++ b/mailnews/base/prefs/content/accountcreation/emailWizard.js
>@@ -702,22 +707,29 @@
>-  startSpinner: function(actionStrName)
>+  startSpinner : function(actionStrName)
[snip…]
>-  stopSpinner: function(actionStrName)
>+  stopSpinner : function(actionStrName)
[snip…]
>-  _showStatusTitle: function(msgName)
>+  showErrorStatus : function(actionStrName)
[snip…]

The standard, as I understand it, is "startSpinner: function…", but
since the rest of the file has the extra space, I guess we can leave
this change in.

>@@ -929,7 +941,7 @@
>     }
>     this.editConfigDetails();
>   },
>-
>+ 

Trailing space.

>@@ -969,37 +982,38 @@
>-                                                { "imap" : 1, "pop3" : 2}, 1);
>+                                                { "imap" : 1, "pop3" : 2 }, 1);
>-                                            [0,1,2,3], 0);
>+                                            [ 0, 1, 2, 3 ], 0);

I don't think we normally put spaces after {s and [s, but you're
consistent in the file, so I guess we can leave it for now…

>@@ -1057,33 +1080,123 @@
>+  /**
>+   * Sets the prefilled values of the port fields.
>+   * Filled statically with the standard ports for the given protocol,
>+   * plus "Auto".
>+   */
>+  fillPortDropdown : function(protocolType)
>   {
>-    this.setPort(this.getUserConfig(), true);
>+    var menu = e(protocolType == "smtp" ? "outgoing_port" : "incoming_port");
>+
>+    //clear

I think we can remove this line…

>@@ -1444,28 +1554,22 @@
>+var _gStandardPorts = {};
>+_gStandardPorts["imap"] = [ 143, 993 ];
>+_gStandardPorts["pop3"] = [ 110, 995 ];
>+_gStandardPorts["smtp"] = [ 587, 25, 465 ]; // order matters
>+var _gAllStandardPorts = _gStandardPorts["smtp"]
>+    .concat(_gStandardPorts["imap"]).concat(_gStandardPorts["pop3"]);

I think I'ld prefer to see this as:
_gAllStandardPorts = [];
Object.keys(_gStandardPorts).map(function(x) {
  _gAllStandardPorts = _gAllStandardPorts.concat(_gStandardPorts[x]);
});
But it's not a huge preference, and I suspect you prefer it the way it
is.  ;)

So yeah, everything here is okay codewise, I guess.  r=me.


I have hit a couple of UI bugs:

Expanding the screen in manual config doesn't re-center the window, which looks really odd.
http://dl.dropbox.com/u/2301433/Screenshots/Non-Centered.png

If I have a different incoming and outgoing username, like for gmail's POP account, the status box pushes the buttons down too close to the bottom of the screen.
http://dl.dropbox.com/u/2301433/Screenshots/TooTall.png

Later,
Blake.

Comment 210

[Ben Bucksch (:BenB)]

> Trailing space.

Already fixed.
DONE

> +    //clear
> I think we can remove this line…

Yes, Sir, DONE, Sir.

> Expanding the screen in manual config doesn't re-center the window

For me on Linux/GNOME, it does reposition the window on resize(), which is unwanted here. So, this is platform dependent and I don't know what to do about it.

> [wrapping of a random description]
> pushes the buttons down too close to the bottom of the

OK, I guess I'll just add 2em to the height, or something, to account for any wrapping.

Comment 211

[Ben Bucksch (:BenB)]

> For me on Linux/GNOME, it does reposition the window on resize()

Details see bug 638958.

Comment 212

[Ben Bucksch (:BenB)]

> I'll just add 2em to the height

Added 10px
DONE

Comment 213

[Ben Bucksch (:BenB)]

> Ok, sounds good. Thanks a lot Ben!

Bryan, does that count as ui-review+ ?

Comment 214

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 517015 [details] [diff] [review]
Fix, v17 - UI review comments addressed

Looks good.  The only nit that I have is a lack of space between the error icon and the error text.

Right now it looks like this:

/!\Error text

We just need a space in front of the text or more padding around the image.

/!\ Error text

ui-r+ with that change.  Thanks!

Comment 215

[Ben Bucksch (:BenB)]

Attachment: Screenshot, Windows, v18 - Start

Comment 216

[Ben Bucksch (:BenB)]

Attachment: Screenshot, Windows, v18 - Detection

Comment 217

[Ben Bucksch (:BenB)]

Attachment: Screenshot, Windows, v18 - Result

Comment 218

[Ben Bucksch (:BenB)]

Attachment: Screenshot, Windows, v18 - Manual edit

Comment 219

[Ben Bucksch (:BenB)]

> /!\Error text

It looks fine on Linux. I tested on Windows and it looks fine there, too.
I added a margin-right: 1em; to the Mac theme.
DONE

Comment 220

[Ben Bucksch (:BenB)]

Attachment: Fix, v18 - commiting

This is with the last nits.

I am going to commit this now, after talking with Standard8.

Comment 221

[Ben Bucksch (:BenB)]

Commited <http://hg.mozilla.org/comm-central/rev/75875cb17fb0>
I hope the commit sticks and the layout stays and the change helps users.
FIXED

Comment 222

[u60234]

Nit: the string "resultSTARTTLS" is defined twice in accountCreation.properties,  first on http://mxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/messenger/accountCreation.properties#91 and then again on line 95.

Comment 223

[rsx11m]

Thanks for reporting. This bug is closed though, thus I've created bug 641331 on your behalf and added a respective patch to remove the duplicate.

Comment 224

[Vincent (caméléon)]

(In reply to comment #218)
> Created attachment 517570 [details]
> Screenshot, Windows, v18 - Manual edit

I am afraid that "SSL" is a too technical term and should be replace by something more friendly like "Security". Does it worth create a new bug for this?

Comment 225

[rsx11m]

All, as said in comment #223 already, this bug is finished up and done.
For any observations and suggestions, please file new bugs. Thanks.

Comment 226

[Adrien de Croy]

Hi all.

Sorry, but this bug is not done, there are still numerous problems with account creation as at 3.3a3:

1. Can't create IMAP account.  Server is specified as my local IP (which is correct).  I see in logs the client connects, but does NOT issue an auth or login command (CAPABILITY only), but complains that it couldn't find my settings.  It prevents the account from being created.

2. Can't create POP3 account either.  Same issue, it connects to the POP3 server but doesn't test credentials before disconnecting, failing and prohibiting account creation.

Can I PLEASE request that account creation not be blocked just because validation heuristics fail?  PLEASE?  Until the validation heuristics are actually correct and 100%, this just creates problems for no good reason.  I have to open an old TB to create an account.

couple nits as well, changing from POP3 to IMAP and back takes the port selection away from auto, and it ends up on 110.  It also resets the chosen auth methods (minor).

Comment 227

[rsx11m]

Yes, this bug *is* done - please read the whiteboard and file new bugs as necessary, nothing more will be done here.

CLOSED

Comment 228

[Tim Maks van den Broek [:mad_maks]]

Please change the entityname if you change the content. I think that a lot of localizers who only use comparelocale going to miss the changes in accountcreation.properties

Comment 229

[Ben Bucksch (:BenB)]

You mean found_settings_* only? These changes were just cosmetic, I don't think it's critical to translate these changes anew.

> who only use comparelocale going to miss the changes

BTW: *Please* fix your scripts. This is completely broken, and trivial to fix the script to compare contents, too.

Comment 230

[Tim Maks van den Broek [:mad_maks]]

(In reply to comment #229)
> You mean found_settings_* only? These changes were just cosmetic, I don't think
> it's critical to translate these changes anew.
> 

true but if you redesign something is it nice when a translation is close to the original text. but yes this is a trivial one


> > who only use comparelocale going to miss the changes
> 
> BTW: *Please* fix your scripts. This is completely broken, and trivial to fix
> the script to compare contents, too.

i don't think that the l1on drivers are agree with you (about comparing the content)

ps thanks for fixing the manager, this is/was the most asked question in our forum about thunderbird.

Comment 231

[Thomas D. (:thomas8)]

(In reply to comment #229)
> > who only use comparelocale going to miss the changes
> BTW: *Please* fix your scripts. This is completely broken, and trivial to fix
> the script to compare contents, too.

Ben, do we have a bug for that? Could you file one? This would be really useful. It's ridiculous having to use new strings for cosmetic changes, and indeed, this should be trivial to fix as the script only needs to compare en-US old set of strings with en-US new set of strings to find any differences in the values of those strings. I don't know anything about l10n scripts, so I would feel a bit uncomfortable to post the new bug myself.

Comment 233

[Sten Carlsen]

Just a thought - if you discover more than one match in the first search, do I (the user) get to select which one is the right one?
I may have missed the answer in the above few comments - in that case, sorry.

Comment 234

[Ben Bucksch (:BenB)]

There's never more than one match. We have exact matching, and our database is manually vetted and thus authoritative for Thunderbird.

Comment 235

[Sten Carlsen]

I was thinking of Comment 6, "fires off 50 server names ...", so in principle I assume two or more of those could come back and say here it is?

I see this while testing, I have my normal server at mail2.s-carlsen.dk and my new test server some place else, like xxx.s-carlsen.dk

This seems to cause some confusion, maybe because s-carlsen.dk also points to s mail server, the same as mail2. Usually the wizard selects s-carlsen.dk for my server and I have difficulty convincing it that I want a different server. It seems to have been getting better but not there yet.

I was thinking a race condition could exist and maybe easier fixed with user selection, IF the user knows what is correct.

Comment 236

[Ben Bucksch (:BenB)]

Sten, this bug is closed. Please file a new bug about your issue, and describe *exactly* what you do (every click, every keypress, including concrete domains used). cc me.