Closed Bug 700536 Opened 13 years ago Closed 12 years ago

Account provisioner polish, and x.browser bug in uriListener.js

Categories

(Thunderbird :: Mail Window Front End, defect)

x86
All
defect
Not set
normal

Tracking

(thunderbird9-, thunderbird10+ fixed)

RESOLVED FIXED
Thunderbird 11.0
Tracking Status
thunderbird9 - ---
thunderbird10 + fixed

People

(Reporter: mconley, Assigned: mconley)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 24 obsolete files)

98.68 KB, image/png
Details
61.00 KB, image/png
Details
227.02 KB, patch
Details | Diff | Splinter Review
65.67 KB, image/png
Details
Going through the account provisioner process, and setting a default search engine, this error shows up in the console:

Warning: reference to undefined property x.browser
Source File: chrome://messenger/content/newmailaccount/uriListener.js
Line: 97

Likely, we just need to make sure x has a browser before doing the equality check on line 97.

There's also some room for polish on some of the provisioner code.  I'll try to take care of some of that here.
Blocks: 686347
Target Milestone: --- → Thunderbird 11.0
Here are some issues I've brought over from bug 686347:

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

potentially-delayable-to-fix-in-aurora-things:
  * We _really_ need to sanitize the input to handle "<b>Blake</b> Winton",
    and "<!--".
  * Clicking on the line between two suggested addresses doesn't expand the list.
  * I suspect we want to split accountProvisioner.css up into Mac, Linux, and
    Windows (and possible Windows Aero) version.
  * Test with the Windows High-Contrast theme.
  * I think what we really want to do is use system colours instead.
  * We're also not testing the failure cases, which is where a lot of the
    bugs with the current account wizard have cropped up.
  * Check that the price is what we expect it to be?
  * Check that the appropriate things get returned for various locales?
Two things for the polish list:
1) IMO, the text "Click here to show all providers." should be styled like a button or link. Right now it's just a plain paragraph, and it feels a bit awkward to click on it and expect some action.
2) I find it a bit strange that the providers' TOS and Privacy Policy links are only displayed when I "Click here to show all providers" or after I search. Perhaps they should always be shown? There's plenty of white space in that dialog initially.
Another issue - it appears as if the ToS is being displayed twice.
A few more problems:
* I have a problem with using user's name in the Hello paragraph. In my locale, the natural grammatical case for Hello Rimas Kudelis would not be nominative. But there are no means to generate different grammatical cases, so I'd consider removing name in my locale instead of showing it in an incorrect grammatical case. I'll try to rewrite the sentence, but haven't come up with a good option yet.
* The first paragraph below the table would make more sense above the "Hello" paragraph IMO (though the word Hello would probably have to be removed then).
* The paragraph listing partners and their privacy policies currently lists even those providers that are currently hidden. If possible, it would probably make sense to hide those providers from this paragraph too, until they are shown above.
Warnings when I start up a Nov 9 Daily build:

JS Component Loader: WARNING chrome://messenger/content/accountcreation/emailWizard.js:695
                     anonymous function does not always return a value
JS Component Loader: WARNING chrome://messenger/content/accountcreation/verifyConfig.js:66
                     function verifyConfig does not always return a value
<!ENTITY partnership.description "In partnership with several providers, &brandShortName; can offer you a new email account. Just fill in your first and last name, or any other words you’d like, in the fields above to get started.">

This says "fields above" but there's only one field now. I suppose the string should be updated.
Also:
* choosing AOL as email provider opens the "Blog-o!" page instead of aol.com. I hope this won't slip into final release as is.
* By the way, I suppose the list of providers will have to become localizable at some point.
* the list of providers shown instead of <span class="placeholder"></span> is not localizable enough. For example, I should really use quotes around provider names, which is currently not possible
* nit: the LOCALIZATION NOTE for sepAnd (in accountProvisioner.properties) says it's actually for another string called price. This can be corrected in aurora too, because it doesn't involve string changes.
(In reply to Rimas Kudelis from comment #7)
> Also:
> * choosing AOL as email provider opens the "Blog-o!" page instead of
> aol.com. I hope this won't slip into final release as is.

That is all controlled server side, so we won't need to change anything in TB for that.

> * By the way, I suppose the list of providers will have to become
> localizable at some point.

Again this is controlled server side. We are working with a range of providers and will offer different options on a locale basis.
Attached patch WIP Patch 1 (obsolete) — Splinter Review
Here's what I've got so far - still needs some cleaning, and I've broken some tests by making the account provisioner modal.  Dealing with that fallout now.

List of things fixed by this WIP patch:

-Disclaimer is no longer displayed twice.                                       
-x.browser undefined is no longer a problem                                     
-Clicking on an entire row both uses the pointer cursor and expands that row    
-Click here to show more providers looks more like a link now.                  
-Split out themes to each platform                                              
-Correctly assigning current search engine to Services.search.currentEngine     
-For names with HTML elements, these are escaped when rendering the name in     
 the page, and when they're sent to the server.                                 
  -For addresses returned from the server that jQuery can't render in a template
   (for example, names with invalid XML in them), we skip displaying those.     
-Clicking on the header of a result group displays all results.                 
-Only selected providers show links for TOS and Privacy Policy                  
-Now displays errors when search results are empty, slow, or broken.            
-Names are restricted to 150 characters now.
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Andreas, I've split out the skins into the gnomestripe/pinstripe/qute folders.  The CSS stuff is, at this point, likely non-volatile from me.  If you could start making Account Provisioner pretty and native looking, that'd be totally awesome. :)
I've got some try-builds of this stuff ready, if anybody (I'm looking at you Rimas! :)) wants to give this new stuff a shot.

Here's the build directory:  https://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/mconley@mozilla.com-712193079fd5/
Attached patch Patch v1 (obsolete) — Splinter Review
Alright, small changes here - I've added some more documentation, reorganized the code a little, and I've moved the instructions for using the account provisioner a bit higher in the interface.

Sorry Andreas, it involved me changing accountProvisioner.css for all three platforms.  :/

Try builds for testing should be available here soon:  http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/mconley@mozilla.com-c423c246ff8a

Try results coming in here:  http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=c423c246ff8a
Attachment #574293 - Attachment is obsolete: true
Rimas:

"1) IMO, the text "Click here to show all providers." should be styled like a button or link. Right now it's just a plain paragraph, and it feels a bit awkward to click on it and expect some action."

Fixed.

"2) I find it a bit strange that the providers' TOS and Privacy Policy links are only displayed when I "Click here to show all providers" or after I search. Perhaps they should always be shown? There's plenty of white space in that dialog initially."

Fixed.

"* I have a problem with using user's name in the Hello paragraph. In my locale, the natural grammatical case for Hello Rimas Kudelis would not be nominative. But there are no means to generate different grammatical cases, so I'd consider removing name in my locale instead of showing it in an incorrect grammatical case. I'll try to rewrite the sentence, but haven't come up with a good option yet."

I'm not sure what you need here.  There are l10n strings before and after the name...is that not sufficient?

"* The first paragraph below the table would make more sense above the "Hello" paragraph IMO (though the word Hello would probably have to be removed then)."

Fixed.

"* The paragraph listing partners and their privacy policies currently lists even those providers that are currently hidden. If possible, it would probably make sense to hide those providers from this paragraph too, until they are shown above."

Fixed.

"* nit: the LOCALIZATION NOTE for sepAnd (in accountProvisioner.properties) says it's actually for another string called price. This can be corrected in aurora too, because it doesn't involve string changes."

Whoops - missed that one in the last patch I put up.  I'll update it soon.
The following issues are still unresolved:

l10n things:
  * the list of providers shown instead of <span class="placeholder"></span> is not 
    localizable enough. For example, I should really use quotes around provider names,
    which is currently not possible
  * <!ENTITY partnership.description> says "fields above" but there's only one field now. I suppose the string should be updated.

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

potentially-delayable-to-fix-in-aurora-things:
  * Test with the Windows High-Contrast theme.
  * I think what we really want to do is use system colours instead.
  * Check that the price is what we expect it to be?
  * Check that the appropriate things get returned for various locales?

Warnings that still crop up:

JS Component Loader: WARNING chrome://messenger/content/accountcreation/emailWizard.js:695
                     anonymous function does not always return a value
JS Component Loader: WARNING chrome://messenger/content/accountcreation/verifyConfig.js:66
                     function verifyConfig does not always return a value
(In reply to Mike Conley (:mconley) from comment #13)
> Rimas:
> 
> "1) IMO, the text "Click here to show all providers." should be styled like
> a button or link. Right now it's just a plain paragraph, and it feels a bit
> awkward to click on it and expect some action."
> 
> Fixed.

The link looks a bit too long though. Perhaps only the second sentence should be clickable?


> "2) I find it a bit strange that the providers' TOS and Privacy Policy links
> are only displayed when I "Click here to show all providers" or after I
> search. Perhaps they should always be shown? There's plenty of white space
> in that dialog initially."
> 
> Fixed.

I think it makes more sense now. Although, this whole dialog looks way too textual...


> "* I have a problem with using user's name in the Hello paragraph. In my
> locale, the natural grammatical case for Hello Rimas Kudelis would not be
> nominative. But there are no means to generate different grammatical cases,
> so I'd consider removing name in my locale instead of showing it in an
> incorrect grammatical case. I'll try to rewrite the sentence, but haven't
> come up with a good option yet."
> 
> I'm not sure what you need here.  There are l10n strings before and after
> the name...is that not sufficient?

Like I said on IRC, if possible, I would like to have one string with a placeholder for a name (Hello #1, here's what we can offer).
Attached patch Patch v2 (obsolete) — Splinter Review
Alright, fixed the sepAnd comment nit that Rimas pointed out.
Attachment #574327 - Attachment is obsolete: true
Comment on attachment 574372 [details] [diff] [review]
Patch v2

Alright, here's my first official run at account provisioner polish.

Try builds are here:  https://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/mconley@mozilla.com-c423c246ff8a/

Try results are here:  http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=c423c246ff8a
Attachment #574372 - Flags: ui-review?(bwinton)
Attachment #574372 - Flags: review?(mbanner)
Attachment #574372 - Flags: approval-mozilla-aurora?
Attachment #574372 - Flags: approval-mozilla-aurora?
Just a quick note that the account provisioner, unlike the other dialogs, don't close when hitting ESC.
Attached patch Patch v3 (obsolete) — Splinter Review
Ok, I've fixed a few things in here.  Specifically:

1)  Escape key now closes the AP window
2)  Clicking directly on the "+X More" now works properly.
3)  I've reorganized some of the elements to help make the dialog seem less text-y.  Andreas is going to do some theme-ing work to help solidify this.

-Mike
Attachment #574372 - Attachment is obsolete: true
Attachment #574372 - Flags: ui-review?(bwinton)
Attachment #574372 - Flags: review?(mbanner)
Attached patch small alignment and color fixes (obsolete) — Splinter Review
Some small fixes
Attached patch more fixes (obsolete) — Splinter Review
Some improvements from latest patch.
* Alignment as in bwintons mockup [1]
* Buttons in lower box are now beside each other
* Removed header in lower box
Attachment #575202 - Attachment is obsolete: true
Attached patch and some more fixes (obsolete) — Splinter Review
And this version of the patch takes care of the flex box issue so that the buttons are the same size (as far as I can see).
Attachment #575288 - Attachment is obsolete: true
Forgot to add the new style fixes to Windows and Mac too.
Should be all good now.
Attachment #575332 - Attachment is obsolete: true
Thanks Andreas - I took your patch and made a few small modifications after some suggestions that Blake made last night.  This patch is your patch with my modifications.
Attachment #575430 - Attachment is obsolete: true
And here's what the AP looks like after Andreas' and my work.
Comment on attachment 575549 [details]
Appearance after applying attachment 575547 [details] [diff] [review]

What do you think of the spacing here?
Attachment #575549 - Flags: feedback?(nisses.mail)
Attachment #575549 - Flags: feedback?(bwinton)
Perhaps the search button font should be restored to normal now?
Comment on attachment 575549 [details]
Appearance after applying attachment 575547 [details] [diff] [review]

Looks good!
I also agree with bwinton's comment on IRC last night that we should put the third provider on the same line as the two others.
Attachment #575549 - Flags: feedback?(nisses.mail) → feedback+
A few more tweaks based on attachment 575547 [details] [diff] [review], to be applied on top of attachment 574905 [details] [diff] [review].
Attachment #575547 - Attachment is obsolete: true
Attached patch long labels workaround (obsolete) — Splinter Review
This is mainly for something Mike and I discussed on IRC for fixing the issue with long labels, and not something I'm sure should go in.
Needs patches 574905 and 576166 applied first.
Attached patch Patch v4 (obsolete) — Splinter Review
This patch combines the patch from attachment 576166 [details] [diff] [review].  We punted on the patch for attachment 576478 [details] [diff] [review], due to the fact that we can control the length of provider labels to a certain degree.

This patch is also a little more proactive in ensuring that the required fields are met in the data it gets back from the server.

Try builds coming in here:  http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=50c9490c43b5
Attachment #575549 - Attachment is obsolete: true
Attachment #576166 - Attachment is obsolete: true
Attachment #576478 - Attachment is obsolete: true
Attachment #575549 - Flags: feedback?(bwinton)
Attachment #574905 - Attachment is obsolete: true
Attached patch Patch v5 (obsolete) — Splinter Review
Whoops, forgot the binary portions of the patch.
Attachment #576580 - Attachment is obsolete: true
Attached patch Small align fix to patch v5 (obsolete) — Splinter Review
This fixes two issues with Mike's latest patch in the case that the provider name is Reallyreallyreallylonglikethis or "Really really really long like this" to make sure it don't starts doing stuff like switching to new rows or something crazy like that.
Attachment #576589 - Attachment is obsolete: true
Attached patch Patch v6 (obsolete) — Splinter Review
Took Andreas's align fix and propagated it across pinstripe and qute.

Try builds coming in here:  http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=232b9ddc95c3
Attachment #576794 - Attachment is obsolete: true
Attachment #576798 - Flags: ui-review?(bwinton)
We're getting pretty close - here are my notes of things that bwinton says we want fixed before this goes through:

1)  The search button should be disabled if the search input is empty, or if no providers are selected.

2)  If the price for an address is "0", then we should display the "Free" string.

3)  For new profiles, the Test Pilot tab is being loaded up *after* the modal has finished closing.  If the user has clicked on ordering an address, that's really confusing, because the Test Pilot tab will pull focus away from the order form.
Attached patch Patch v7 (obsolete) — Splinter Review
I've updated the patch so that:

1)  The search button is only enabled if there are some providers checked AND if there is something in the search input.  I've added a test for this.

2)  When an address is available for $0, we say it's "Free", instead of "0 a year".

Things to note:

- The disabled button doesn't *look* disabled.  Andreas, can we do some styling magic here?
- I need to talk to squib about what to do with the TestPilot tab that opens up on a fresh profile.  I'll CC him on this bug, if he's not already on it.
Attachment #576798 - Attachment is obsolete: true
Attachment #576798 - Flags: ui-review?(bwinton)
Attached patch patch v8 (obsolete) — Splinter Review
Same as Mike's patch above, but adds styling for the disabled button state.

This will probably come up in the code review, but don't we usually call these disabled="true" rather than disabled="disabled"?
Attachment #576981 - Attachment is obsolete: true
Attached patch Patch v9 (obsolete) — Splinter Review
Switching instances of disabled="disabled" to disabled="true".  Propagated changes in gnomestripe to pinstripe and qute.
Attachment #577223 - Attachment is obsolete: true
Attached patch Patch v10 (obsolete) — Splinter Review
Fixes a small bug where the 3pane can pull focus immediately after opening up a provisioner order form.
Attachment #577262 - Attachment is obsolete: true
Depends on: 705808
Attached patch Patch v11 (obsolete) — Splinter Review
A few last changes here:

1)  Provider names are now labels, and when clicked, toggle the associated checkbox.
2)  The "search engine by default" window mode has been collapsed into the success window mode, in that the "select this provider as the default search engine" button-y thing is now included in the final window at the top (and is, of course, hidden if there are no search engines associated with this provider, or the provider is already the default search engine).
3)  The "search engine by default" button-y thing can be toggled by clicking any portion of the button-y thing, as opposed to just the checkbox.

I'll also attach a screenshot to show the new location of the button-y thing.
Attachment #577275 - Attachment is obsolete: true
Attachment #577645 - Flags: ui-review?(bwinton)
Oh, and I forgot to mention - this last patch also makes it so that when the user chooses to compose a message or get add-ons from the final window state, the window does not actually close until the user clicks on "close".
Comment on attachment 577645 [details] [diff] [review]
Patch v11

Standard8:  directing the code review request to you - let me know if someone else would be more appropriate.

Thanks,

-Mike
Attachment #577645 - Flags: review?(mbanner)
A few suggestions for Comment 43, if I may.

1. A checkbox inside a button is a bit odd. What happens if I click on it? Will the checkbox get unchecked or will the button get clicked?

2. "Attach a personal signature to my email" should say "... signature to your email" for consistency. 

3. The period in "Close this window." looks a bit out of place.
Comment on attachment 577645 [details] [diff] [review]
Patch v11

"The search terms used are sent to Mozilla (Privacy Policy) and to 3rd party email providers Hover.com (Privacy Policy, Terms of Service) to find available email addresses."

That reads a little oddly, but I don't think we want to change the string for this release.  Followup bug, perhaps?

The space before the search box (12px) is much less than the space after the search box (31px).

When I selected "Write some email", the identity it used was the default identity, not the newly-created identity.

Similarly, "Attach a personal signature to my email" took me to the Local Folders section of the preferences, not the newly-created account's settings.

Finally, there seems to be a scrollbar in the final window, which is kinda silly, since we should be able to determine the size of that window.  :)

So, those are all kind of minor things, so I'm going to say ui-r=me with those fixed.

Thank you!
Blake.
Attachment #577645 - Flags: ui-review?(bwinton) → ui-review+
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #47)
> Comment on attachment 577645 [details] [diff] [review] [diff] [details] [review]
> Patch v11
> 
> "The search terms used are sent to Mozilla (Privacy Policy) and to 3rd party
> email providers Hover.com (Privacy Policy, Terms of Service) to find
> available email addresses."
> 
> That reads a little oddly, but I don't think we want to change the string
> for this release.  Followup bug, perhaps?

Filed as bug 708030.

> The space before the search box (12px) is much less than the space after the
> search box (31px).

Fixed.

> When I selected "Write some email", the identity it used was the default
> identity, not the newly-created identity.

Fixed.

> Similarly, "Attach a personal signature to my email" took me to the Local
> Folders section of the preferences, not the newly-created account's settings.

Fixed. 

> Finally, there seems to be a scrollbar in the final window, which is kinda
> silly, since we should be able to determine the size of that window.  :)

Fixed.
 
> So, those are all kind of minor things, so I'm going to say ui-r=me with
> those fixed.
> 
> Thank you!
> Blake.

No, thank you!
Attached patch Patch v12 (ui-r'd by bwinton) (obsolete) — Splinter Review
I've fixed the issues that Blake brought up.  Ready for some serious code review.  Bring it on!
Attachment #577645 - Attachment is obsolete: true
Attachment #577645 - Flags: review?(mbanner)
Attachment #579392 - Flags: review?(mbanner)
Attachment #579392 - Flags: review?(mbanner) → review?(bwinton)
Comment on attachment 579392 [details] [diff] [review]
Patch v12 (ui-r'd by bwinton)

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

UX Idea: If the user chose only one provider, we should probably just expand the list of suggestions for that one.  If it's simple, please implement it, if not, please file a bug.

Okay, so, there's a bunch of stuff below, but it's all pretty simple, so I'm going to say r=me, with the things below resolved, cause I trust you.  ;)

Thanks,
Blake.

::: mail/base/content/msgMail3PaneWindow.js
@@ -326,2 +326,9 @@
> > -  if (gPrefBranch.getBoolPref("mail.provider.enabled"))
> > +  if (gPrefBranch.getBoolPref("mail.provider.enabled")) {
> > -    NewMailAccountProvisioner(msgWindow, { okCallback: okCallback });
> > +    // We need to let the event loop pump a little so that the 3pane finishes
> > +    // opening, so we use setTimeout. The 100ms is a bit arbitrary, but seems
> > +    // to be enough time to let the 3pane do it's thing, and not pull focus
NaN more ...

Since you only use openProvisioner once, I think you can inline the function.

::: mail/components/newmailaccount/content/accountProvisioner.js
@@ -164,3 +93,3 @@
> >  
> > -function tryToPopulateProviderList() {
> > +var EmailAccountProvisioner = {
> > -  // If we're already in the middle of this, bail out.
> > +  /* The Email Account Provisioner window can be in several states.  The first

I think most of these should start with "/**\n", so that they get picked up as documentation comments.  (See, for instance, mailnews/db/gloda/modules/explattr.js)

Commenting the object would also be nice.  ;)

@@ -191,14 +113,22 @@
> > -  $.getJSON(providerList, function(data) {
> > +  // The SEARCH state is where we start.
> > -    providers = {};
> > +  _state: this.SEARCH,
NaN more ...
> > -        ;
> > +  _spinning: false,
> > -      else if (i == data.length - 2)
> > +  _searchEnabled: false,
NaN more ...

I don't think we ever use this getter.  Perhaps we should remove it?

@@ -191,14 +113,81 @@
> > -  $.getJSON(providerList, function(data) {
> > +  // The SEARCH state is where we start.
> > -    providers = {};
> > +  _state: this.SEARCH,
NaN more ...
> > -        ;
> > +  _spinning: false,
> > -      else if (i == data.length - 2)
> > +  _searchEnabled: false,
NaN more ...

I don't think this is true, because we could also be in SET_SEARCH_ENGINE.

Or, actually, I'm wrong, because we don't ever use SET_SEARCH_ENGINE.  And we don't seem to ever go back to the SEARCH state, so you could probably remove this whole else block, too, I think.

And then, since we only ever start in one state, and transition to a second by showing and hiding things, and never get to go back, I think maybe we don't want a state variable at all, and should just rename this method "showSuccessPage".

@@ -191,14 +113,91 @@
> > -  $.getJSON(providerList, function(data) {
> > +  // The SEARCH state is where we start.
> > -    providers = {};
> > +  _state: this.SEARCH,
NaN more ...
> > -        ;
> > +  _spinning: false,
> > -      else if (i == data.length - 2)
> > +  _searchEnabled: false,
NaN more ...

We never seem to use this get property, either.

@@ -191,14 +113,103 @@
> > -  $.getJSON(providerList, function(data) {
> > +  // The SEARCH state is where we start.
> > -    providers = {};
> > +  _state: this.SEARCH,
NaN more ...
> > -        ;
> > +  _spinning: false,
> > -      else if (i == data.length - 2)
> > +  _searchEnabled: false,
NaN more ...

Nor this variable, so perhaps we can get rid of it, too?

@@ -191,14 +113,108 @@
> > -  $.getJSON(providerList, function(data) {
> > +  // The SEARCH state is where we start.
> > -    providers = {};
> > +  _state: this.SEARCH,
NaN more ...
> > -        ;
> > +  _spinning: false,
> > -      else if (i == data.length - 2)
> > +  _searchEnabled: false,
NaN more ...

Same here...

@@ -191,14 +113,121 @@
> > -  $.getJSON(providerList, function(data) {
> > +  // The SEARCH state is where we start.
> > -    providers = {};
> > +  _state: this.SEARCH,
NaN more ...
> > -        ;
> > +  _spinning: false,
> > -      else if (i == data.length - 2)
> > +  _searchEnabled: false,
NaN more ...

If we _really_ needed to know the value of searchEnabled, I bet we could just use searchButtonEnabled.  (He says until he runs across that code and asks that it be removed, too.  ;)

@@ -191,14 +113,148 @@
> > -  $.getJSON(providerList, function(data) {
> > +  // The SEARCH state is where we start.
> > -    providers = {};
> > +  _state: this.SEARCH,
NaN more ...
> > -        ;
> > +  _spinning: false,
> > -      else if (i == data.length - 2)
> > +  _searchEnabled: false,
NaN more ...

We don't ever seem to use this property.

@@ -191,14 +113,166 @@
> > -  $.getJSON(providerList, function(data) {
> > +  // The SEARCH state is where we start.
> > -    providers = {};
> > +  _state: this.SEARCH,
NaN more ...
> > -        ;
> > +  _spinning: false,
> > -      else if (i == data.length - 2)
> > +  _searchEnabled: false,
NaN more ...

Given that we have the list of providers in providers, this seems like a confusing name.  How about providerListUrl instead?  And, since we seem to only use it in one place, could we just inline this call into that place?

@@ -191,14 +113,173 @@
> > -  $.getJSON(providerList, function(data) {
> > +  // The SEARCH state is where we start.
> > -    providers = {};
> > +  _state: this.SEARCH,
NaN more ...
> > -        ;
> > +  _spinning: false,
> > -      else if (i == data.length - 2)
> > +  _searchEnabled: false,
NaN more ...

Ditto.

@@ -191,14 +113,193 @@
> > -  $.getJSON(providerList, function(data) {
> > +  // The SEARCH state is where we start.
> > -    providers = {};
> > +  _state: this.SEARCH,
NaN more ...
> > -        ;
> > +  _spinning: false,
> > -      else if (i == data.length - 2)
> > +  _searchEnabled: false,
NaN more ...

Perhaps this could be "saveName"?  or at least "saveField"...  :)

@@ -191,14 +113,200 @@
> > -  $.getJSON(providerList, function(data) {
> > +  // The SEARCH state is where we start.
> > -    providers = {};
> > +  _state: this.SEARCH,
NaN more ...
> > -        ;
> > +  _spinning: false,
> > -      else if (i == data.length - 2)
> > +  _searchEnabled: false,
NaN more ...

Please inline this into the following expression.

@@ -191,14 +113,208 @@
> > -  $.getJSON(providerList, function(data) {
> > +  // The SEARCH state is where we start.
> > -    providers = {};
> > +  _state: this.SEARCH,
NaN more ...
> > -        ;
> > +  _spinning: false,
> > -      else if (i == data.length - 2)
> > +  _searchEnabled: false,
NaN more ...

I really like this comment!

@@ -191,14 +113,212 @@
> > -  $.getJSON(providerList, function(data) {
> > +  // The SEARCH state is where we start.
> > -    providers = {};
> > +  _state: this.SEARCH,
NaN more ...
> > -        ;
> > +  _spinning: false,
> > -      else if (i == data.length - 2)
> > +  _searchEnabled: false,
NaN more ...

We never seem to set this to true...

@@ -191,14 +113,218 @@
> > -  $.getJSON(providerList, function(data) {
> > +  // The SEARCH state is where we start.
> > -    providers = {};
> > +  _state: this.SEARCH,
NaN more ...
> > -        ;
> > +  _spinning: false,
> > -      else if (i == data.length - 2)
> > +  _searchEnabled: false,
NaN more ...

Does this still make sense in the click-to-web world?  (Leave it for now, but perhaps think about it as part of that bug...)

@@ -191,14 +113,244 @@
> > -  $.getJSON(providerList, function(data) {
> > +  // The SEARCH state is where we start.
> > -    providers = {};
> > +  _state: this.SEARCH,
NaN more ...
> > -        ;
> > +  _spinning: false,
> > -      else if (i == data.length - 2)
> > +  _searchEnabled: false,
NaN more ...

I think we could use a newline before this comment.

@@ -191,14 +113,274 @@
> > -  $.getJSON(providerList, function(data) {
> > +  // The SEARCH state is where we start.
> > -    providers = {};
> > +  _state: this.SEARCH,
NaN more ...
> > -        ;
> > +  _spinning: false,
> > -      else if (i == data.length - 2)
> > +  _searchEnabled: false,
NaN more ...

This function, and the notifications delegate below, are getting big enough that we might want to consider making them their own functions in the object.

@@ -191,14 +113,294 @@
> > -  $.getJSON(providerList, function(data) {
> > +  // The SEARCH state is where we start.
> > -    providers = {};
> > +  _state: this.SEARCH,
NaN more ...
> > -        ;
> > +  _spinning: false,
> > -      else if (i == data.length - 2)
> > +  _searchEnabled: false,
NaN more ...

I wonder if we want to have some hooks for TestPilot here (and some other places), so that we can see how people use this feature...

Also, having some sort of (toggle-able) local logging will help us a lot when things go wrong.  Check the Account Wizard for a sample of how to implement that.

@@ -191,14 +113,299 @@
> > -  $.getJSON(providerList, function(data) {
> > +  // The SEARCH state is where we start.
> > -    providers = {};
> > +  _state: this.SEARCH,
NaN more ...
> > -        ;
> > +  _spinning: false,
> > -      else if (i == data.length - 2)
> > +  _searchEnabled: false,
NaN more ...

Missing space after the ":".

@@ -191,14 +113,304 @@
> > -  $.getJSON(providerList, function(data) {
> > +  // The SEARCH state is where we start.
> > -    providers = {};
> > +  _state: this.SEARCH,
NaN more ...
> > -        ;
> > +  _spinning: false,
> > -      else if (i == data.length - 2)
> > +  _searchEnabled: false,
NaN more ...

Could this get inlined into
.error(EmailAccountProvisioner.showSearchError)
?

@@ -191,14 +113,326 @@
> > -  $.getJSON(providerList, function(data) {
> > +  // The SEARCH state is where we start.
> > -    providers = {};
> > +  _state: this.SEARCH,
NaN more ...
> > -        ;
> > +  _spinning: false,
> > -      else if (i == data.length - 2)
> > +  _searchEnabled: false,
NaN more ...

Does this delete the original data's provider?  Oh, wait, I think we don't care at this point.

@@ -191,14 +113,353 @@
> > -  $.getJSON(providerList, function(data) {
> > +  // The SEARCH state is where we start.
> > -    providers = {};
> > +  _state: this.SEARCH,
NaN more ...
> > -        ;
> > +  _spinning: false,
> > -      else if (i == data.length - 2)
> > +  _searchEnabled: false,
NaN more ...

We might want to mention that we're closing _this_ window, not the tab we just opened.

@@ -191,14 +113,363 @@
> > -  $.getJSON(providerList, function(data) {
> > +  // The SEARCH state is where we start.
> > -    providers = {};
> > +  _state: this.SEARCH,
NaN more ...
> > -        ;
> > +  _spinning: false,
> > -      else if (i == data.length - 2)
> > +  _searchEnabled: false,
NaN more ...

What do you mean by "both selectors"?

@@ -191,14 +113,455 @@
> > -  $.getJSON(providerList, function(data) {
> > +  // The SEARCH state is where we start.
> > -    providers = {};
> > +  _state: this.SEARCH,
NaN more ...
> > -        ;
> > +  _spinning: false,
> > -      else if (i == data.length - 2)
> > +  _searchEnabled: false,
NaN more ...

So, I think I'ld like to see some error reporting here.  Not to the user, but logged somewhere so that when Sancus adds a new provider, he can check whether he's got all the stuff it needs (preferably all at once, instead of one at a time).

And there are only seven things to check, so I'm not _too_ worried about the speed.

@@ -191,14 +113,482 @@
> > -  $.getJSON(providerList, function(data) {
> > +  // The SEARCH state is where we start.
> > -    providers = {};
> > +  _state: this.SEARCH,
NaN more ...
> > -        ;
> > +  _spinning: false,
> > -      else if (i == data.length - 2)
> > +  _searchEnabled: false,
NaN more ...

This seems like it could be documented a little more.  I mean, don't get me wrong, I'm a fan of the functional style, but there was already a bug in this code.  ;)

@@ -215,9 +679,14 @@
> > -        )
> > +        ).append($("<span />").text(")"));
> > -        .append($("<span />").text(")"+sep));
> > +      termsAndPrivacyLinks.push(providerLinks);
NaN more ...
> > +    if (termsAndPrivacyLinks.length <= 0) {
> > -        inputs.before('<span class="provider">'+
> > +      // Something went really wrong - we shouldn't have gotten here. Bail out.
> > -          '<input type="checkbox" value="' + provider.id + '" checked="true"/>' +
NaN more ...

I think this might be a very English-specific thing, and I worry about how well it'll localize.  Hopefully the localizers will have some input on how we can fix it for the next version.

@@ -215,9 +679,57 @@
> > -        )
> > +        ).append($("<span />").text(")"));
> > -        .append($("<span />").text(")"+sep));
> > +      termsAndPrivacyLinks.push(providerLinks);
NaN more ...
> > +    if (termsAndPrivacyLinks.length <= 0) {
> > -        inputs.before('<span class="provider">'+
> > +      // Something went really wrong - we shouldn't have gotten here. Bail out.
> > -          '<input type="checkbox" value="' + provider.id + '" checked="true"/>' +
NaN more ...

I wonder if we shouldn't create the elements and inject them into the DOM _before_ expanding the search pane...

@@ -215,9 +679,66 @@
> > -        )
> > +        ).append($("<span />").text(")"));
> > -        .append($("<span />").text(")"+sep));
> > +      termsAndPrivacyLinks.push(providerLinks);
NaN more ...
> > +    if (termsAndPrivacyLinks.length <= 0) {
> > -        inputs.before('<span class="provider">'+
> > +      // Something went really wrong - we shouldn't have gotten here. Bail out.
> > -          '<input type="checkbox" value="' + provider.id + '" checked="true"/>' +
NaN more ...

Oh, perhaps not, because we'll want to show the error if we get one?

@@ -215,9 +679,77 @@
> > -        )
> > +        ).append($("<span />").text(")"));
> > -        .append($("<span />").text(")"+sep));
> > +      termsAndPrivacyLinks.push(providerLinks);
NaN more ...
> > +    if (termsAndPrivacyLinks.length <= 0) {
> > -        inputs.before('<span class="provider">'+
> > +      // Something went really wrong - we shouldn't have gotten here. Bail out.
> > -          '<input type="checkbox" value="' + provider.id + '" checked="true"/>' +
NaN more ...

I wonder if we should also show an error if we get back results for providers we _didn't_ select?

::: mail/components/newmailaccount/content/accountProvisioner.xhtml
@@ -53,3 +54,3 @@
> >    <link rel="stylesheet"
> >          type="text/css"
> > -        href="chrome://messenger/content/newmailaccount/accountProvisioner.css" />
> > +        href="/skin/newmailaccount/accountProvisioner.css" />

Huh, you don't need the "chrome://messenger" at the start of that?

@@ -98,2 +102,3 @@
> > -      <div class="header">
> > +        <form id="search" onsubmit="return false;">
> > -        <h2>&header.label;</h2>
> > +          <div id="searchFields" class="hbox">
> > +            <input id="name" type="text" placeholder="&input.namePlaceholder;" maxlength="150" class="boxFlex"/>

Feel free to break this up into:
<input id="name" type="text" placeholder="&input.namePlaceholder;"
       maxlength="150" class="boxFlex"/>
(with "maxlength" lined up under "id".)

@@ -98,2 +102,8 @@
> > -      <div class="header">
> > +        <form id="search" onsubmit="return false;">
> > -        <h2>&header.label;</h2>
> > +          <div id="searchFields" class="hbox">
> > +            <input id="name" type="text" placeholder="&input.namePlaceholder;" maxlength="150" class="boxFlex"/>
> > +            <input type="submit" value="&input.search;" class="search boxFlex" id="searchSubmit"/>
NaN more ...

Does this add extra spaces before and after the partnership description?

@@ -139,6 +155,6 @@
> > -      <div class="description">
> > +    </div>
> > -        <p>
> > +
NaN more ...
> > -        </p>
> > +      <div class="tinyheader hbox">
> > -        <p class="commentary">
> > +        <button class="existing boxFlex">&tinyheader.existing;</button>
NaN more ...

This should have a couple more spaces of indent.

::: mail/test/mozmill/instrumentation/wrapper.py
@@ -1,1 +1,18 @@
> > -NO_ACCOUNTS = True
> > +# ***** BEGIN LICENSE BLOCK *****
> > +# Version: MPL 1.1/GPL 2.0/LGPL 2.1
> > +#
> > +# The contents of this file are subject to the Mozilla Public License Version
NaN more ...

"2011"?

::: mail/test/mozmill/newmailaccount/html/providerList
@@ +2,5 @@
>    "label": "foo",
>    "paid": true,
>    "languages" : ["en-US"],
>    "api": "http://www.example.com/tbReg?first={firstname}&last={lastname}&email={email}",
> +  "tos_url": "http://foo.com/tos",

So, example.com is an officially-sanctioned "does not exist host".  "foo.com", on the other hand resolves to 216.234.246.165, and is actually live on the internet, and might not appreciate it if we hit their url on every test run...

@@ +11,5 @@
>    "label": "bar",
>    "paid": false,
>    "languages" : ["en-US", "fr-FR"],
>    "api": "http://localhost:43336/registration.html",
> +  "tos_url": "http://bar.com/tos",

"bar.com" is 216.250.183.108.  ;)

@@ +20,5 @@
> +  "label": "French Provider",
> +  "paid": false,
> +  "languages" : ["fr-FR"],
> +  "api": "http://localhost:43336/registration.html",
> +  "tos_url": "http://french.fr/tos",

94.23.128.154

::: mail/test/mozmill/newmailaccount/test-newmailaccount.js
@@ -71,3 +83,4 @@
> >  var gProvisionerEnabled = Services.prefs.getBoolPref(kProvisionerEnabledPref);
> >  
> > -var setupModule = function(module) {
> > +// Record what the original value of the mail.provider.enabled pref is so
> > +// that we can put it back once the tests are done.

Do we ever test that we _don't_ show the Provisioner when the pref is set to false?

@@ -85,0 +105,7 @@
> > +  // First, restrict the user's language to just en-US
> > +  let langs = Cc["@mozilla.org/pref-localizedstring;1"].createInstance(Ci.nsIPrefLocalizedString);
> > +  langs.data = "en-US";
> > +  Services.prefs.setComplexValue(kAcceptLangs, Ci.nsIPrefLocalizedString, langs);
NaN more ...

We don't undo this change on teardown, do we?

@@ -101,8 +129,8 @@
> > -function wait_for_provisioner_window() {
> > +/* This tests the basic workflow for Account Provisioner - it spawns the
> > -  let w = null;
> > + * Provisioner window, fills in the search input, gets the results, clicks
NaN more ...
> > -    let docShell = w.QueryInterface(Ci.nsIInterfaceRequestor)
> > + *
> > -        .getInterface(Ci.nsIWebNavigation)
> > + * It gets a little complicated, since the modal dialog for the provisioner
NaN more ...

Which three functions?  (Would be worth a mention here.)

@@ +419,5 @@
> +  let $ = w.window.$;
> +
> +  // Type a name with some HTML tags and an ampersand in there
> +  // to see if we can trip up account provisioner.
> +  const CLEVER_STRING = "<i>Hey, I'm clever & smart!<!-- Ain't I a stinkah? --></i>";

I'ld like to see 's and "s int there, too, in case we put stuff in attributes.

@@ -136,1 +196,288 @@
> > -  // First, make sure the page is loaded.
> > +  // Clicking this button should close the modal dialog.
> > +  $('button.create[address="green@bar.com"]').click();
> > +}
> > +
NaN more ...

Trailing space on this line.

::: mail/test/mozmill/shared-modules/test-window-helpers.js
@@ -176,1 +176,5 @@
> > -  return windowType;
> > +
> > +  if (windowType)
> > +    return windowType;
> > +
> > +  // As a last resort, use the name given to the DOM window.

This seems odd.  Why do we need it?

::: mail/themes/gnomestripe/mail/newmailaccount/accountProvisioner.css
@@ +35,5 @@
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +* {

Just as a question, did you go through these to make sure we were using all the styles?  If not, please file a bug so that we do sometime.

@@ +257,5 @@
> +#FirstAndLastName {
> +  font-weight: bold;
> +}
> +
> +.hbox.row.th.displayNone { display: none; }

Multiple lines here, please.
Attachment #579392 - Flags: review?(bwinton) → review+
Comment on attachment 579392 [details] [diff] [review]
Patch v12 (ui-r'd by bwinton)

(Oh, it would also be good if Sid could review the Mozmill tests before checkin.)
Attachment #579392 - Flags: review?(sagarwal)
Blocks: 709809
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #50)
> Comment on attachment 579392 [details] [diff] [review]
> Patch v12 (ui-r'd by bwinton)
> 
> Review of attachment 579392 [details] [diff] [review]:
> -----------------------------------------------------------------

Blake:

Hey - so I've taken care of a good chunk of your comments - but yeah, Splinter screwed up, and I'm missing context on a number of your comments.  So I'm blocked on those until we can map those comments to the code you were originally referring to.

Here's what I've fixed so far:

> 
> UX Idea: If the user chose only one provider, we should probably just expand
> the list of suggestions for that one.  If it's simple, please implement it,
> if not, please file a bug.
> 

Filed as bug 709809

> 
> Since you only use openProvisioner once, I think you can inline the function.
> 

Done.

> 
> I think most of these should start with "/**\n", so that they get picked up
> as documentation comments.  (See, for instance,
> mailnews/db/gloda/modules/explattr.js)

Done.

> 
> Commenting the object would also be nice.  ;)
> 

Done.

> 
> I don't think this is true, because we could also be in SET_SEARCH_ENGINE.
> 
> Or, actually, I'm wrong, because we don't ever use SET_SEARCH_ENGINE.  And
> we don't seem to ever go back to the SEARCH state, so you could probably
> remove this whole else block, too, I think.
> 
> And then, since we only ever start in one state, and transition to a second
> by showing and hiding things, and never get to go back, I think maybe we
> don't want a state variable at all, and should just rename this method
> "showSuccessPage".

Done.


> Could this get inlined into
> .error(EmailAccountProvisioner.showSearchError)
> ?

Done.

> We might want to mention that we're closing _this_ window, not the tab we
> just opened.

Done.


> 
> What do you mean by "both selectors"?
> 

Fragment of an ancient comment.  Replaced with something more sensical.

> 
> Huh, you don't need the "chrome://messenger" at the start of that?
> 

Fixed.

> Feel free to break this up into:
> <input id="name" type="text" placeholder="&input.namePlaceholder;"
>        maxlength="150" class="boxFlex"/>
> (with "maxlength" lined up under "id".)
> 

Done.

> 
> "2011"?
> 

Fixed.

> So, example.com is an officially-sanctioned "does not exist host". 
> "foo.com", on the other hand resolves to 216.234.246.165, and is actually
> live on the internet, and might not appreciate it if we hit their url on
> every test run...

Ah, good catch!  Fixed for foo, bar, French, German, corrupt.

> 
> Do we ever test that we _don't_ show the Provisioner when the pref is set to
> false?
> 

Nope, we don't.  I'll add a test.


> We don't undo this change on teardown, do we?

We do indeed.

> 
> Which three functions?  (Would be worth a mention here.)
> 

Done.

> 
> I'ld like to see 's and "s int there, too, in case we put stuff in
> attributes.
> 

Done.

> 
> This seems odd.  Why do we need it?
> 

So in order to use our wait/plan_for_modal_dialog functions, we need to pass some kind of window identifier.  Normally, that's a window type or ID, like "mail:3pane".  The problem is that the Account Provisioner modal dialog is an XHTML page, which cannot set the id or type of the window - instead, the window is given a name at spawn time.  So this change allows us to use wait/plan_for_modal_dialog based on the name of the window at spawn time.

> 
> Multiple lines here, please.

Fixed.
Half-way done fixups from bwinton's review.
Attachment #579392 - Attachment is obsolete: true
Attachment #579392 - Flags: review?(sagarwal)
Comment on attachment 581011 [details] [diff] [review]
WIP Patch v13 (ui-r+'d by bwinton)

I've added the test that Blake requested, but I don't imagine I'll be modifying the Mozmill stuff any more for this bug.  Review away.

Thanks Sid,

-Mike
Attachment #581011 - Flags: review?(sagarwal)
Blocks: 709945
Comment on attachment 581011 [details] [diff] [review]
WIP Patch v13 (ui-r+'d by bwinton)

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

So I've only looked at the tests, not at the code. This means that I'm not very familiar with what the account provisioner code is capable of doing, so I wouldn't be able to identify anything missing from the tests. I also see that Blake's looked at the tests too. As such, I'm giving it feedback+ modulo the comments below. 

If you'd like me to review this for real, I think I'll have to go through all the code too. Given the size of the code and my unfamiliarity with it, I expect a full review to take a week minimum.

::: mail/test/mozmill/instrumentation/wrapper.py
@@ +34,5 @@
> +# the terms of any one of the MPL, the GPL or the LGPL.
> +#
> +# ***** END LICENSE BLOCK *****
> +
> +# For test-instrumentation.js, we need to disable the account provisioner.

Why? Could you add a comment here explaining?

::: mail/test/mozmill/newmailaccount/html/registrationCorrupt.html
@@ +11,5 @@
> +      <form action="configCorrupt.xml" method="GET">
> +         <p>
> +         First name: <input value="Green" id="first" name="firstname" type="text"><br>
> +         Last name: <input value="Llama" id="last" name="lastname" type="text"><br>
> +         Email: <input value="da.green.llama@bar.com" id="email" name="email" type="text"><br>

Can we use one of the example domains here? (or .nul, that works too I believe).

::: mail/test/mozmill/newmailaccount/html/suggestFromName
@@ +5,5 @@
>  "personalized_email", "addresses": ["green@llama.com", "me@greenllama.com",
>  "green@greenllama.com", "green@greenllama.net", "green@greenllama.org",
>  "green@greenllama.me", "me@greenllama.me"], "succeeded": true, "quote":
> +"3f93e48679ab46a49475", "price": "20.00", "provider": "foo"},
> +{"product": "personalized_email", "addresses": ["corrupt@corrupt.com"],

And here.

::: mail/test/mozmill/newmailaccount/test-newmailaccount.js
@@ +108,5 @@
> +  Services.prefs.setComplexValue(kAcceptLangs, Ci.nsIPrefLocalizedString, langs);
> +
> +  // Add a "bar" search engine that we can switch to be the default.
> +  Services.search.addEngineWithDetails("bar", null, null, null, "post",
> +                                       "http://www.foo.com/search");

example domain?

@@ +194,4 @@
>    $(".address:first").click();
>    mc.waitFor(function () $("button.create:visible").length > 0);
>  
> +  // Pick the email address green@bar.com

example domain?

@@ +199,3 @@
>  
> +  // Clicking this button should close the modal dialog.
> +  $('button.create[address="green@bar.com"]').click();

here too.

::: mail/test/mozmill/shared-modules/test-window-helpers.js
@@ +178,5 @@
> +    return windowType;
> +
> +  // As a last resort, use the name given to the DOM window.
> +  let domWindow = aXULWindow.docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                    .getInterface(Ci.nsIDOMWindow);

The dot's slightly misaligned.
Attachment #581011 - Flags: review?(sagarwal) → feedback+
Thanks Sid - I've made the corrections you asked for.

Just waiting on some final review feedback from bwinton, and then we're all set to go with this.
Attachment #581011 - Attachment is obsolete: true
Comment on attachment 579392 [details] [diff] [review]
Patch v12 (ui-r'd by bwinton)

>+++ b/mail/components/newmailaccount/content/accountProvisioner.js
>@@ -75,496 +74,786 @@ function getLocalStorage(page) {
>+  set state(aVal) {
>+    // If we're already in this state, there's nothing to do.
>+    if (aVal == this._state)
>+      return;
>+
>+    if (aVal == this.SUCCESS) {
[snip]
>+    } else {
>+      // We're in SEARCH

I don't think this is true, because we could also be in SET_SEARCH_ENGINE.

Or, actually, I'm wrong, because we don't ever use SET_SEARCH_ENGINE.
And we don't seem to ever go back to the SEARCH state, so you could
probably remove this whole else block, too, I think.

And then, since we only ever start in one state, and transition to a
second by showing and hiding things, and never get to go back, I think
maybe we don't want a state variable at all, and should just rename this
method "showSuccessPage".

>+      $("#window").show();
>+      $("#successful_account").hide();
>+    }
>+
>+    this._state = aVal;
>+  },
>+
>+  /* Returns whether or not the spinner is currently visible.
>+   */
>+  get spinning() {
>+    return this._spinning;
>+  },

We never seem to use this get property, either.


>+  /* A setter for showing / hiding the spinner.
>+   */
>+  set spinning(aVal) {
>+    if (aVal) {
>+      $("#notifications .spinner").css('display', 'block');
>+    } else {
>+      $("#notifications .spinner").css('display', 'none');
>+    }
>+    this._spinning = aVal;

Nor this variable, so perhaps we can get rid of it, too?

>+  },
>+
>+  /* Returns whether or not the search fields are enabled.
>+   */
>+  get searchEnabled() {
>+    return this._searchEnabled;
>+  },

Same here...

>+  /* A setter for enabling / disabling the search fields.
>+   */
>+  set searchEnabled(aVal) {
>+    if (aVal) {
>+      $("#name").removeAttr("disabled");
>+    } else {
>+      $("#name").attr("disabled", "true");
>+    }
>+    this.searchButtonEnabled = aVal;
>+    this._searchEnabled = aVal;

If we _really_ needed to know the value of searchEnabled, I bet we could
just use searchButtonEnabled.  (He says until he runs across that code
and asks that it be removed, too.  

>+  get someProvidersChecked() {
>+    return this._someProvidersChecked;
>+  },

We don't ever seem to use this property.

>+  /* A collection of provider result objects returned after a search.
>+   */
>+  get providers() {
>+    return this._providers;
>+  },
>+
>+  /* A setter for the provider result objects returned after a search.
>+   */
>+  set providers(aVal) {
>+    this._providers = aVal;
>+  },

I don't see the advantage of this over just using a variable.

>+
>+  /* Returns the URL for retrieving the provider list.
>+   */
>+  get providerList() {

Given that we have the list of providers in providers, this seems like a
confusing name.  How about providerListUrl instead?  And, since we seem
to only use it in one place, could we just inline this call into that place?

>+  /**
>+   * Save the fields in this page to localstorage, so we can reconstitute it
>+   * later.
>+   */
>+  saveFields: function EAP_saveFields() {
>+    var name = String.trim($("#name").val());
>+    this.storage.setItem("name", name);
>+  },

Perhaps this could be "saveName"?  or at least "saveField"...  

>+  onSearchInputOrProvidersChanged: function EAP_onSearchInputOrProvidersChanged(event) {
>+    let emptyName = $("#name").val() == "";
>+    let someProvidersChecked = EmailAccountProvisioner.someProvidersChecked;

Please inline this into the following expression.

>+    EmailAccountProvisioner.searchButtonEnabled = !emptyName
>+                                                  && someProvidersChecked;
>+  },



>+  /* Hook up our events, populate the DOM, set our hooks, do all of our
>+   * prepatory work.  Since this is called via jQuery on document ready,
>+   * the scope for "this" is the actual window document, hence the need
>+   * to explicitly refer to EmailAccountProvisioner.
>+   */

I really like this comment!

>+  init: function EAP_init() {
>+    // We can only init once, so bail out if we've been called again.
>+    if (EmailAccountProvisioner._inited)

We never seem to set this (_inited) to true...

>+    // For any anchor element that gets the "external" class, make it so that
>+    // when we click on that element, instead of loading up the href in the
>+    // window itself, we open up the link in the default browser.

Does this still make sense in the click-to-web world?  (Leave it for
now, but perhaps think about it as part of that bug...)

>+    // populate the search field with it.
>+    let name = EmailAccountProvisioner.storage.getItem("name") ||
>+               $("#name").text();
>+    $("#name").val(name);
>+    EmailAccountProvisioner.saveFields();
>+    // Pretend like we've typed something into the search input to set the
>+    // initial enabled/disabled state of the search button.

I think we could use a newline before this comment.

>+    $("#search").submit(function() {

This function, and the notifications delegate below, are getting big
enough that we might want to consider making them their own functions in
the object.

>+      $.ajax({
>+        url: EmailAccountProvisioner.suggestFromName,

I wonder if we want to have some hooks for TestPilot here (and some
other places), so that we can see how people use this feature...

Also, having some sort of (toggle-able) local logging will help us a lot
when things go wrong.  Check the Account Wizard for a sample of how to
implement that.

>+        dataType: 'json',
>+        data: {"first_name": firstname,
>+               "last_name": lastname,
>+               "providers":providerList},

Missing space after the ":".

>+      // And add the extra data.
>+      let data = storedData[provider.id];
>+      delete data.provider;

Does this delete the original data's provider?  Oh, wait, I think we
don't care at this point.

>+  providerHasCorrectFields: function EAP_providerHasCorrectFields(provider) {
>+    let result = true;
>+
>+    let required = ["id", "label", "paid", "languages", "api", "tos_url",
>+                    "privacy_url"];
>+
>+    for (let [index, aField] in Iterator(required)) {
>+      result &= (aField in provider);
>+
>+      if (!result) {
>+        break;

So, I think I'ld like to see some error reporting here.  Not to the
user, but logged somewhere so that when Sancus adds a new provider, he
can check whether he's got all the stuff it needs (preferably all at
once, instead of one at a time).

And there are only seven things to check, so I'm not _too_ worried about
the speed.

>+      EmailAccountProvisioner.providers[provider.id] = provider;
>+      let supportsSomeUserLang = provider
>+                                 .languages
>+                                 .some(function (x) {
>+                                   return EmailAccountProvisioner
>+                                          .userLanguages
>+                                          .indexOf(x.toLowerCase()) >= 0
>+                                 });

This seems like it could be documented a little more.  I mean, don't get
me wrong, I'm a fan of the functional style, but there was already a bug
in this code.  

>+  /* Once we've received search results from the server, create some
>+   * elements to display those results, and inject them into the DOM.
>+   */
>+  onSearchResults: function(data) {
>+    // Expand the search pane if it hasn't been expanded yet.
>+    EmailAccountProvisioner.expandSearchPane();

I wonder if we shouldn't create the elements and inject them into the
DOM _before_ expanding the search pane...

>+    // Empty any old results.
>+    let results = $("#results").empty();
>+
>+    if (!data || !data.length) {
>+      // If we've gotten back nonsense, display the generic
>+      // error message, and bail out.
>+      EmailAccountProvisioner.showSearchError();

Oh, perhaps not, because we'll want to show the error if we get one?

>+    // Filter out any results that don't match our requirements...
>+    if (returnedProviders.length == 0) {
>+      // Display the generic error message, and bail out.
>+      EmailAccountProvisioner.showSearchError();

I wonder if we should also show an error if we get back results for
providers we _didn't_ select?

>+++ b/mail/components/newmailaccount/content/accountProvisioner.xhtml
>@@ -86,125 +87,124 @@
>+          <div id="providers">
>+            <div id="instructions">
>+              &partnership.description;
>+            </div>

Does this add extra spaces before and after the partnership description?

>+++ b/mail/test/mozmill/newmailaccount/test-newmailaccount.js
>+function subtest_show_tos_privacy_links_for_selected_providers(w) {

[snip]

>+  // Now show the providers from different locales...
>+  w.click(w.eid("otherLangDesc"));
>+  wait_for_element_invisible(w, "otherLangDesc"); 

Trailing space on this line.


Comments in context, also curse Splinter!
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #58)
> Comment on attachment 579392 [details] [diff] [review]
> Patch v12 (ui-r'd by bwinton)
> 
> 
> I don't think this is true, because we could also be in SET_SEARCH_ENGINE.
> 
> Or, actually, I'm wrong, because we don't ever use SET_SEARCH_ENGINE.
> And we don't seem to ever go back to the SEARCH state, so you could
> probably remove this whole else block, too, I think.
> 
> And then, since we only ever start in one state, and transition to a
> second by showing and hiding things, and never get to go back, I think
> maybe we don't want a state variable at all, and should just rename this
> method "showSuccessPage".

Done.


> >+  /* Returns whether or not the spinner is currently visible.
> >+   */
> >+  get spinning() {
> >+    return this._spinning;
> >+  },
> 
> We never seem to use this get property, either.
> 
> ...
> 
> Nor this variable, so perhaps we can get rid of it, too?
> 

Removed (or rather, switched to a spinning function)

> >+  },
> >+
> >+  /* Returns whether or not the search fields are enabled.
> >+   */
> >+  get searchEnabled() {
> >+    return this._searchEnabled;
> >+  },
> 
> Same here...

Removed.

> 
> If we _really_ needed to know the value of searchEnabled, I bet we could
> just use searchButtonEnabled.  (He says until he runs across that code
> and asks that it be removed, too.  

Done.

> 
> >+  get someProvidersChecked() {
> >+    return this._someProvidersChecked;
> >+  },
> 
> We don't ever seem to use this property.

Actually, we do use this one - in the "onSearchInputOrProvidersChanged" function.  Do you still want me to remove it?

> 
> >+  /* A collection of provider result objects returned after a search.
> >+   */
> >+  get providers() {
> >+    return this._providers;
> >+  },
> >+
> >+  /* A setter for the provider result objects returned after a search.
> >+   */
> >+  set providers(aVal) {
> >+    this._providers = aVal;
> >+  },
> 
> I don't see the advantage of this over just using a variable.
> 

Good point - removed.

> >+  get providerList() {
> 
> Given that we have the list of providers in providers, this seems like a
> confusing name.  How about providerListUrl instead?  And, since we seem
> to only use it in one place, could we just inline this call into that place?
> 

Done.

> >+  /**
> >+   * Save the fields in this page to localstorage, so we can reconstitute it
> >+   * later.
> >+   */
> >+  saveFields: function EAP_saveFields() {
> >+    var name = String.trim($("#name").val());
> >+    this.storage.setItem("name", name);
> >+  },
> 
> Perhaps this could be "saveName"?  or at least "saveField"...  

Switched to saveName.

> 
> >+  onSearchInputOrProvidersChanged: function EAP_onSearchInputOrProvidersChanged(event) {
> >+    let emptyName = $("#name").val() == "";
> >+    let someProvidersChecked = EmailAccountProvisioner.someProvidersChecked;
> 
> Please inline this into the following expression.
> 
> >+    EmailAccountProvisioner.searchButtonEnabled = !emptyName
> >+                                                  && someProvidersChecked;
> >+  },
> 
> 

Done.

> 
> >+  init: function EAP_init() {
> >+    // We can only init once, so bail out if we've been called again.
> >+    if (EmailAccountProvisioner._inited)
> 
> We never seem to set this (_inited) to true...

Good eye!  Fixed.

> 
> >+    // For any anchor element that gets the "external" class, make it so that
> >+    // when we click on that element, instead of loading up the href in the
> >+    // window itself, we open up the link in the default browser.
> 
> Does this still make sense in the click-to-web world?  (Leave it for
> now, but perhaps think about it as part of that bug...)

Yeah, if / when CTW lands, we'll want to use the URIUtils Javascript module in that patch, instead of manually opening up the links in the external browser.

> 
> >+    // populate the search field with it.
> >+    let name = EmailAccountProvisioner.storage.getItem("name") ||
> >+               $("#name").text();
> >+    $("#name").val(name);
> >+    EmailAccountProvisioner.saveFields();
> >+    // Pretend like we've typed something into the search input to set the
> >+    // initial enabled/disabled state of the search button.
> 
> I think we could use a newline before this comment.

Done.

> 
> >+    $("#search").submit(function() {
> 
> This function, and the notifications delegate below, are getting big
> enough that we might want to consider making them their own functions in
> the object.
> 

Done.

> >+      $.ajax({
> >+        url: EmailAccountProvisioner.suggestFromName,
> 
> I wonder if we want to have some hooks for TestPilot here (and some
> other places), so that we can see how people use this feature...

I believe TestPilot can hook into button click events, and observe when DOM nodes get added, etc.  I don't think we have to do anything special here.

> 
> Also, having some sort of (toggle-able) local logging will help us a lot
> when things go wrong.  Check the Account Wizard for a sample of how to
> implement that.
> 

Done.

> >+        dataType: 'json',
> >+        data: {"first_name": firstname,
> >+               "last_name": lastname,
> >+               "providers":providerList},
> 
> Missing space after the ":".

Fixed

> 
> >+  providerHasCorrectFields: function EAP_providerHasCorrectFields(provider) {
> >+    let result = true;
> >+
> >+    let required = ["id", "label", "paid", "languages", "api", "tos_url",
> >+                    "privacy_url"];
> >+
> >+    for (let [index, aField] in Iterator(required)) {
> >+      result &= (aField in provider);
> >+
> >+      if (!result) {
> >+        break;
> 
> So, I think I'ld like to see some error reporting here.  Not to the
> user, but logged somewhere so that when Sancus adds a new provider, he
> can check whether he's got all the stuff it needs (preferably all at
> once, instead of one at a time).
> 
> And there are only seven things to check, so I'm not _too_ worried about
> the speed.

Done.

> 
> >+      EmailAccountProvisioner.providers[provider.id] = provider;
> >+      let supportsSomeUserLang = provider
> >+                                 .languages
> >+                                 .some(function (x) {
> >+                                   return EmailAccountProvisioner
> >+                                          .userLanguages
> >+                                          .indexOf(x.toLowerCase()) >= 0
> >+                                 });
> 
> This seems like it could be documented a little more.  I mean, don't get
> me wrong, I'm a fan of the functional style, but there was already a bug
> in this code.  

Okie doke, I've put in some documentation for this block of code.


> 
> >+  /* Once we've received search results from the server, create some
> >+   * elements to display those results, and inject them into the DOM.
> >+   */
> >+  onSearchResults: function(data) {
> >+    // Expand the search pane if it hasn't been expanded yet.
> >+    EmailAccountProvisioner.expandSearchPane();
> 
> I wonder if we shouldn't create the elements and inject them into the
> DOM _before_ expanding the search pane...
> 
> >+    // Empty any old results.
> >+    let results = $("#results").empty();
> >+
> >+    if (!data || !data.length) {
> >+      // If we've gotten back nonsense, display the generic
> >+      // error message, and bail out.
> >+      EmailAccountProvisioner.showSearchError();
> 
> Oh, perhaps not, because we'll want to show the error if we get one?

Correct.

> 
> >+    // Filter out any results that don't match our requirements...
> >+    if (returnedProviders.length == 0) {
> >+      // Display the generic error message, and bail out.
> >+      EmailAccountProvisioner.showSearchError();
> 
> I wonder if we should also show an error if we get back results for
> providers we _didn't_ select?

Done.

> 
> >+++ b/mail/components/newmailaccount/content/accountProvisioner.xhtml
> >@@ -86,125 +87,124 @@
> >+          <div id="providers">
> >+            <div id="instructions">
> >+              &partnership.description;
> >+            </div>
> 
> Does this add extra spaces before and after the partnership description?
> 

Good catch - I've made the tag a single line.

> >+++ b/mail/test/mozmill/newmailaccount/test-newmailaccount.js
> >+function subtest_show_tos_privacy_links_for_selected_providers(w) {
> 
> [snip]
> 
> >+  // Now show the providers from different locales...
> >+  w.click(w.eid("otherLangDesc"));
> >+  wait_for_element_invisible(w, "otherLangDesc"); 
> 
> Trailing space on this line.
> 

Fixed.

> 
> Comments in context, also curse Splinter!

Hear hear!  Review Board forever! :)
Attachment #581262 - Attachment is obsolete: true
Attachment #581371 - Attachment description: Patch v15 → Patch v15 (r+'d by bwinton, ui-r+'d by bwinton, feedback+ from sid0)
Ok, pushed to try here: http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=1684cd6fb961

If I see all green, well, up she goes.
So, uh, not so green, eh?  :(
Yay!  I greened it (well, kinda - it`s orange, but it`s a known random orange from an unrelated test).  Turns out I needed to use the close_popup function from test-folder-display-helpers to close the File menu popup.  Go figure.

Committed to comm-central as http://hg.mozilla.org/comm-central/rev/5d1ce217c593
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment on attachment 581371 [details] [diff] [review]
Patch v15 (r+'d by bwinton, ui-r+'d by bwinton, feedback+ from sid0)

Requesting approval for comm-aurora.
Attachment #581371 - Flags: approval-comm-aurora?
Depends on: 710912
Attached image screenshot
Not sure if i need to file a new bug....

The text need to be wrapped or the localizers need the possibility to made the window bigger, see screenshot
Thanks Tim for finding / reporting this bug,

I've filed a new bug 711968.

-Mike
Attachment #581371 - Flags: approval-comm-aurora? → approval-comm-aurora+
Blocks: 711968
You need to log in before you can comment on or make changes to this bug.