545563 - migration assistant / new features popup window

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

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

The migration assistant tab has too much content in one page so people have been missing what is new and different that they need to look at.

A new migration assistant should popup a dialog which can use an iframe to display local content and actions to help you configure Thunderbird.  I would think we want 1 page per section (of the current migration assistant) each page living in some kind of "new-features" directory.

Comment 1

[Ludovic Hirlimann [:Usul]]

Something looking like the installer  on windows ?

Comment 2

[Ludovic Hirlimann [:Usul]]

(In reply to comment #1)
> Something looking like the installer  on windows ?

Or the old setup account ?

Comment 3

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

Similar to the install on windows but I've actually been looking at how the Firefox major update window seems to have an iframe in it for displaying richer content.  Something like that with ( Previous ) ( Next ) buttons for transitioning between pages.

Comment 4

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

Attachment: incorrect size but a concept mockup

Here's a quick mockup of a modal dialog I made editing the existing upgrade dialog to add an iframe for rich content in the center.  The ( Previous ) and ( Next ) buttons would just change the src attribute of the iframe.  The dialog itself needs to be a bit larger than this one demonstrates.  Also we'll need a "[ ] Don't show me this again" option in there so when people close the window it doesn't come back.

Comment 5

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

We need the dialog to be callable (and point-to-a-specific-page-able), and access a directory of chrome-html files.
Start with Autosync stuff.
  How much space TB is using now
  How much space TB will use with autosync on.

Could have this page lead them into the autoconfig, if there are no accounts.

Comment 6

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

I'm pushing my WIP to http://hg.mozilla.org/users/bwinton_latte.ca/prefsui/
So far, I've got the infrastructure in place, and the autosync page working, albeit without the space.

Comment 7

[David :Bienvenu]

nsIMsgFolder.sizeOnDisk for an imap folder will tell you the sum of the sizes of the imap messages in a folder, independent of whether they're download or not. For a local folder, it really is the sizeOnDisk; for imap folders, it's how big the folder is on the server.

To determine how much space is being used for an offline store for an imap folder, you could use nsIMsgFolder.filePath to get the nsILocalFile for the offline store, and then use nsIFile.fileSize to find the size of the offline store. The filePath could potentially not exist, if there is no offline store.

Free disk space - get an nsILocalFile, any nsILocalFile on that drive, and use the diskSpaceAvailable attribute.

Comment 8

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

Hey dmose, could you check out http://hg.mozilla.org/users/bwinton_latte.ca/prefsui/rev/af0a6652cf09 and let me know if that's along the lines of what you were asking for?

Thanks,
Blake.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Built from that repo, and I'm not actually seeing a dialog pop-up at all, so I can't comment based on experience, but briefly skimming those bits of code makes me think that that's pretty much what I had in mind.

Comment 10

[Timo Pietilä]

Just a comment:

One thing that must be set for migration wizard is that it should prevent person from logging to his accounts until settings have been made unless it keeps the settings that users has set. If anything changes during upgrade and user logs in before going thru those settings we get a situation where program is possibly doing some unwanted things at the background while user ponders what to set and what not.

Comment 12

[rsx11m]

> (In reply to comment #10) migration wizard is that it should prevent
> person from logging to his accounts until settings have been made

That would be a generalization of bug 522631 I've filed for IMAP accounts.
No problem extending the scope of that bug, or handling it here either.

Comment 13

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

Attachment: WIP patch.

Here's what we've got so far.

Thanks,
Blake.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 435960 [details] [diff] [review]
WIP patch.

After a bunch of discussion, the current plan is for bwinton to generate try-server builds for the test day that will be built to auto-update to 3.1b2. 

Additionally, he'll work on some tests, and I'm continuing to explore reviewing options.

Comment 15

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

bienvenu mentioned in bug 534072 comment 7 that we might want to set mail.server.default.autosync_offline_stores to false in order to turn off autosync.

Comment 16

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

Good call.  I'm setting it now when the user clicks "Sync All" or "Sync None".

Thanks,
Blake.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

sid0 has graciously agreed to review the forthcoming patch once it's ready; thanks Sid!

Comment 18

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

Attachment: Newer WIP, for Thursday's Test Day.

The build is kicked off.

It's got all the changes we could fit in, including my change to set the default autosync behaviour for new servers to whatever you choose in the autosync page.

Some other things:
We don't show autosync page if there are no IMAP accounts.
You can re-show the Migration Assistant if you go to Help » Migration Assistant (on a Mac, other platforms probably differ).

I'll be interested in hearing what the testers find.

Linux: http://s3.mozillamessaging.com/build/try-server/2010-03-31_18:[e-mail]-bw-MigAsst/[e-mail]2

Windows: http://s3.mozillamessaging.com/build/try-server/2010-03-31_18:[e-mail]-bw-MigAsst/[e-mail]

Windows Installer: http://s3.mozillamessaging.com/build/try-server/2010-03-31_18:[e-mail]-bw-MigAsst/[e-mail]

And I'm really freaking tired, so I'm going to head to bed, but the Mac build should be ready for you by tomorrow morning at http://tinderbox.mozilla.org/showbuilds.cgi?tree=ThunderbirdTry

Thanks,
Blake.

Comment 19

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

(And just for completeness, Mac:
http://s3.mozillamessaging.com/build/try-server/2010-03-31_18:[e-mail]-bw-MigAsst/[e-mail]
)

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

Blake, is this ready to have a patch generated for review?

Comment 21

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

Not yet, but I could probably clean it up enough by tomorrow to have something to post.  My only concern is that we might get some feedback by Wednesday which could involve larger changes, necessitating a re-review, but if you're fine with that, then I'm happy to post a patch sooner.

Comment 22

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

Here are my notes from our recent call for items left to do on this bug:

* autosync
** Ensure the proper states are correct (the default should be no... bug ???)
*** no offline settings = default to sync all
*** offline set to none = default to sync none
*** some offline settings = account specific options
* add-on pages
** feedback upon clicking install
*** reuse the add-on manager install system where we replace the button with a spinner and the text "Installing..."
** Already installed feedback
*** Remove the install button if the add-on is already installed and replace it with the text "Installed"
** Show a warning section for whom the add-on is very likely desired
*** detect adv. columns in folder pane and show a warning section
*** detect collapsed header view and show a warning section

Comment 23

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

So, comment 22 would be some of the feedback which is going to push this patch back a little.  ;)  I'll try to finish off the autosync changes by tomorrow morning sometime, and post the patch so that at least those bits can start through the review process.

Sorry about that,
Blake.

Comment 24

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

(In reply to comment #22)
> * autosync
> ** Ensure the proper states are correct (the default should be no... bug ???)
> *** no offline settings = default to sync all

So, we've got three different ways to say this.  As I understand it, from most general to most specific, they are:
The "mail.server.default.autosync_offline_stores" pref.
The "server.offlineDownload" property.
and the "nsMsgFolderFlags.Offline" flag on a folder.

Currently, if none of the folders have the Offline flag, we check "sync none"; if all of the folders have the Offline flag, we check "sync all"; and if some of the folders have the Offline flag, we check "sync some".

> *** offline set to none = default to sync none

Check, as per preceding paragraph.

> *** some offline settings = account specific options

Check, as per preceding paragraph.

> * add-on pages
> ** feedback upon clicking install
> *** reuse the add-on manager install system where we replace the button with a
> spinner and the text "Installing..."

Done.

> ** Already installed feedback
> *** Remove the install button if the add-on is already installed and replace it
> with the text "Installed"

Check.

> ** Show a warning section for whom the add-on is very likely desired
> *** detect adv. columns in folder pane and show a warning section
> *** detect collapsed header view and show a warning section

Check and check.  Well, we just replace the text with a stronger version, but you can style it as you like.

The changes are pushed to http://hg.mozilla.org/users/bwinton_latte.ca/prefsui/
 in the 545563-new-migration-assistant branch, so unless there's something else you want to add, I think we should start getting this reviewed.  Sound good?

Later,
Blake.

Comment 25

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

(In reply to comment #24)
> > ** Show a warning section for whom the add-on is very likely desired
> > *** detect adv. columns in folder pane and show a warning section
> > *** detect collapsed header view and show a warning section
> 
> Check and check.  Well, we just replace the text with a stronger version, but
> you can style it as you like.

Excellent, thanks!

> The changes are pushed to http://hg.mozilla.org/users/bwinton_latte.ca/prefsui/
>  in the 545563-new-migration-assistant branch, so unless there's something else
> you want to add, I think we should start getting this reviewed.  Sound good?

Ok, I'm going over it now but I don't think I have anything more than possible grammar or style corrections.  Lets start getting this reviewed.

Comment 26

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

Attachment: The first cut at the new migration assistant.

Okay, okay, I know it's a huge patch, but the code changes are only 750-ish lines, or 700-ish if you don't include the test, or 550-ish if you ignore the license blocks.  ;)

Bryan, I'm asking you for the ui-r, which feels kind of silly, since you've been involved with most of the UI work, but I figured better safe than sorry.

Thanks,
Blake.

Comment 27

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

Attachment: patch to fix the aero issues

here's a quick patch to fix up the issues with Vista.  The style sheets weren't propagated down to the aero section so Vista users were getting the unstyled pages we saw comments about.

Other than this fix the only issue I see right now is that the autosync reported 0.0MB of space for my system, which isn't true.  I'm wondering what we can do to mitigate this kind of error or if it's just a bug.

Comment 28

[Rain [:rain1] (inactive)]

Comment on attachment 438537 [details] [diff] [review]
The first cut at the new migration assistant.

Rich review at http://reviews.visophyte.org/r/438537/ -- sorry about the snafu with duplicate names. ReviewBoard isn't too happy with renames it seems.

This is a first-pass review -- I've only glanced at the HTML and CSS so far,
and I haven't run with the patch either.


on file: mail/base/content/featureConfigurator.js line 46
>   subpages: ["others", "autosync", "toolbar", "compactheader",
>              "folderpanecolumns"],

As discussed on IRC, please rename others to something more suitable -- intro
maybe?


on file: mail/base/content/featureConfigurator.js line 230
>   previousButton: function(e) {
>     e.preventDefault();
>     this.changePage(this.index - 1);
>   },

Please name this function and all the other functions you're touching or
creating.


on file: mail/base/content/featureConfigurator.js line 245
>   changePage: function(index) {
>     this.index = index;
>     let url = "chrome://messenger/content/featureConfigurators/" +
>               this.subpages[this.index] +
>               ".xhtml";
>     document.getElementById("contentFrame").setAttribute("src", url);
>     document.getElementById("prevButton").disabled = (this.index == 0);
>     document.getElementById("nextButton").disabled =
>         (this.index == this.subpages.length - 1);
>   },

I think it makes more sense to make this a setter for a private property
called _index (and have a corresponding getter).


on file: mail/base/content/featureConfigurator.js line 256
>   setup: function(parentWin) {
>     this.parentWin = parentWin;

I think our usual name for such functions is init(), but I don't really care
:)


on file: mail/base/content/featureConfigurator.js line 258
>     parentWin.previousSyncSettings = {};
>     parentWin.previousSyncPref = Cc["@mozilla.org/preferences-service;1"]
>         .getService(Ci.nsIPrefBranch)
>         .getBoolPref("mail.server.default.autosync_offline_stores");

Oh, I don't really like invading the parent window's namespace like this. I
think a global dictionary of dictionary of arguments, keyed by the subpage
name, would work here? You could pass it in to each page's onLoad function as
a second argument.


on file: mail/base/content/featureConfigurator.js line 268
>       let allImap = true;

This isn't used.


on file: mail/base/content/featureConfigurator.js line 270
>         if (server.type != "imap")
>           continue;
>         server = server.QueryInterface(Ci.nsIImapIncomingServer);

I don't think you need the check for type any more. You can write this as:

if (!(server instanceof Ci.nsIImapIncomingServer))
  continue;


on file: mail/base/content/featureConfigurator.js line 277
>       if (!foundImap)
>         this.subpages = this.subpages.filter(function(item) {
>           return item != "autosync";
>         });

this.subpages.filter(function(item) item != "autosync");


on file: mail/base/content/featureConfigurator.js line 288
> /**
>  * addEventListener betrayals compel us to establish our link with the
>  * outside world from inside.  NeilAway suggests the problem might have
>  * been the registration of the listener prior to initiating the load.  Which
>  * is odd considering it works for the XUL case, but I could see how that might
>  * differ.  Anywho, this works for now and is a delightful reference to boot.
>  */
> function reachOutAndTouchFrame(parentWin) {

Please add a doc comment saying that parentWin is an optional parameter (this
confused me for a while).


on file: mail/base/content/featureConfigurator.js line 295
> function reachOutAndTouchFrame(parentWin) {

Is the name valid any more? Seems like this is now just another boring onload.


on file: mail/base/content/featureConfigurators/autosync.js line 18
>  * Portions created by the Initial Developer are Copyright (C) 2009

2010 I believe?


on file: mail/base/content/featureConfigurators/autosync.js line 50
>   updateStatus: function as_updateStatus() {

A doc comment for this, please.


on file: mail/base/content/featureConfigurators/autosync.js line 54
>       let currentsize = 0;
>       let estimatedsize = 0;
>       let freespace = null;
>       let sizedifference = 0;

All this should be camelCase.


on file: mail/base/content/featureConfigurators/autosync.js line 63
>         if (server.type != "imap")
>           continue;
>         server = server.QueryInterface(Ci.nsIImapIncomingServer);

Same as before, you can do both with a single instanceof call.


on file: mail/base/content/featureConfigurators/autosync.js line 76
>           filePath.initWithPath(filePath.path+".msf");

Spaces around the + please


on file: mail/base/content/featureConfigurators/autosync.js line 80
>           if (freespace == null && filePath.exists()) {
>             freespace = filePath.diskSpaceAvailable;
>           }

This will not be able to compute the free space if none of the files exists. 
In any case, I don't know whether it's worth bothering about the case where
the imap mail directory is on a separate drive from the profile directory.
Seems to me that we should just compute the free space on the drive the
profile directory is in -- anyone who knows enough to poke around in
about:config would know enough to not trust the value there.


on file: mail/base/content/featureConfigurators/autosync.js line 87
>       // Display the current status
>       // TODO: Replace this with something from bug 516787, hopefully.
>       let formatSize = function as_formatSize(size) {
>         let mbs = size / (1024*1024);
>         if (mbs > 1000)
>           return (mbs / 1024).toFixed(1) + " GB";
>         return (mbs).toFixed(1) + " MB";
>       }

We have code in DownloadUtils.jsm for this. DownloadUtils.convertByteUnits()


on file: mail/base/content/featureConfigurators/autosync.js line 100
>       // If we are within 50 MB of space then warn
>       if (freespace < sizedifference + (1024 * 1024 * 50))
>         $("#disk-space-warning").show()
>       else
>         $("#disk-space-warning").hide()

50 MB seems an awfully small amount to leave on the hard drive. Could we make
it so that we warn if (freeSpace - sizeDifference) is some percentage (< 10%)
of freeSpace, with sensible lower and upper limits?


on file: mail/base/content/featureConfigurators/autosync.js line 102
>         $("#disk-space-warning").show()
>       else
>         $("#disk-space-warning").hide()
> 
>       // Fix the issue where compacting will actually save you space
>       if (sizedifference <= 0)
>         $("#disk-space-required").hide()

Semicolons after the three statements, please.


on file: mail/base/content/featureConfigurators/autosync.js line 132
>   setSyncStatus: function as_setSyncStatus(sync) {

- A doc comment for this, please.

- Heh, our usual style is "aSync", but that makes it look too much like
"async". I guess aSync is still better, though.


on file: mail/base/content/featureConfigurators/autosync.js line 142
>       for each (let server in fixIterator(servers, Ci.nsIMsgIncomingServer)) {
>         if (server.type != "imap")
>           continue;
>         server = server.QueryInterface(Ci.nsIImapIncomingServer);

instanceof


on file: mail/base/content/featureConfigurators/autosync.js line 149
>         newSync = (sync == "some") ?
>                   this.previousSyncSettings[server.prettyName] :
>                   (sync == "true");
>         server.offlineDownload = newSync;

I'm not following the logic here. What happens if, say, someone chooses some
folders, changes the selection around a little, changes his mind and goes to
sync all/none, and then comes back and chooses sync some again? Will we
clobber his choice? That doesn't sound like a particularly good idea...


on file: mail/base/content/featureConfigurators/autosync.js line 160
>         $(document.getElementById(server.prettyName))
>                   .toggleClass("syncing", server.offlineDownload);

No need to access server.offlineDownload -- you can simply use newSync here.


on file: mail/base/content/featureConfigurators/autosync.js line 186
>                     .attr("id", server.prettyName)

http://mxr.mozilla.org/comm-central/source/mailnews/base/public/nsIMsgIncomingServer.idl#65
says that the key is guaranteed to be unique across servers, so please use
that instead of prettyName.


on file: mail/base/content/featureConfigurators/autosync.js line 192
>         let aServer = server;
>         li.click(function() {

Yeah, sucks that this is necessary. Could you please add a comment explaining
why?


on file: mail/base/content/featureConfigurators/compactheader.js line 48
> CompactHeaderConfigurator.prototype = {
>   __proto__: ExtensionConfigurator.prototype,
> 
>   shouldShowStrongMessage: function ch_shouldShowStrongMessage() {
>       return this.parentWin.document.getElementById("msgHeaderViewDeck")
>                                     .usedCompactHeader;
>   },
> }
> 
> var compactHeaderConfigurator = new CompactHeaderConfigurator();

- Why have CompactHeaderConfigurator/ExtensionConfigurator as a class and not
just as a singleton? Bonus, since "this" is dynamically bound in JS, you can
have extensionId/extensionUrl as properties of CompactHeaderConfigurator
directly.

- There needs to be a semicolon at the end of the definition.



on file: mail/base/content/featureConfigurators/compactheader.js line 52
>       return this.parentWin.document.getElementById("msgHeaderViewDeck")
>                                     .usedCompactHeader;

Instead of this, could you please figure this out in FeatureConfigurator.js
and pass it in as an element of the arguments dictionary?


on file: mail/base/content/featureConfigurators/compactheader.xhtml line 30
>    - Portions created by the Initial Developer are Copyright (C) 2009

2010, and there are a bunch of other files which also have 2009.


on file: mail/base/content/featureConfigurators/folderpanecolumns.js line 51
> FolderPaneConfigurator.prototype = {
>   __proto__: ExtensionConfigurator.prototype,
> 
>   shouldShowStrongMessage: function fp_shouldShowStrongMessage() {
>       // If the user had shown folder pane columns.
>       return gPrefBranch.getBoolPref("mail.showFolderPaneColumns");
>   },
> }
> 
> var folderPaneConfigurator = new FolderPaneConfigurator();

This could also be a singleton I think.


on file: mail/base/content/featureConfigurators/toolbar.xhtml line 59
>   <!-- Global Context -->
>   <!-- script type="application/javascript;version=1.8"
>       src="chrome://messenger/content/featureConfigurator.js"></script -->

Please remove this entirely.


on file: mail/base/content/msgMail3PaneWindow.js line 765
>       // Open a dialog explaining the major changes between 2 and 3.
>       if (gPrefBranch.getBoolPref("mailnews.ui.show.migration.on.upgrade"))
>         window.setTimeout(openFeatureConfigurator, 300, [true,]);

- This isn't 3 any more. In general I think it'd be really hard to keep the
version number updated, so I'd prefer something like "the major changes from
2".

- Do we need the setTimeout any more?

- openFeatureConfigurator doesn't take any parameters, so I don't think the
[true,] at the end is necessary.

[I'm not going to ask you to move this code to mailMigrator.js, because
there's a lot of surrounding code that'll also need to be moved. :) ]


on file: mail/base/content/utilityOverlay.js line 351
>                     "chrome,dialog=yes,all,centerscreen,width=704,height=416",

I'm not sure about the normal thing to do here -- do we really have hard-coded
dimensions like this? And do we care about 640x480?


on file: mail/base/content/utilityOverlay.js line 352
>                     window);

This is window.arguments[0] for the feature configurator window, right? Please
add a comment saying so.


on file: mail/locales/en-US/chrome/messenger/featureConfigurator.dtd line 5
> <!ENTITY featureConfigurator.heading "Thunderbird 3 features worth knowing about">

s/3/3.1/ or a better fix please. "New Thunderbird features" maybe? Also I
believe you'll have to rename the entities for any text strings you modify.


on file: mail/locales/en-US/chrome/messenger/featureConfigurator.dtd line 8
> <!ENTITY featureConfigurator.para1 "Thunderbird 3 includes powerful new features which we're quite proud of, and think most people will love. People's tastes and needs vary, so we made it easy to control and customize Thunderbird.">

s/3/3.1/ or a better fix if possible. Maybe "this version of Thunderbird"?


on file: mail/locales/en-US/chrome/messenger/featureConfigurator.dtd line 28
> <!ENTITY featureConfigurator.headerParaBeforeLink "Thunderbird 3 has improved the detailed message header view, including buttons for message-related actions. There are alternative designs available as ">

Another reference to "Thunderbird 3".


on file: mail/locales/en-US/chrome/messenger/featureConfigurator.dtd line 36
> <!-- LOCALIZATION NOTE (featureConfigurator.addOn.installing): TODO: lorem ipsum -->

This needs a note.


on file: mail/locales/en-US/chrome/messenger/featureConfigurator.dtd line 39
> <!-- LOCALIZATION NOTE (featureConfigurator.addOn.installed): TODO: lorem ipsum -->

So does this, and there are a large number of other localization notes below
this that are also TODOs.


on file: mail/locales/en-US/chrome/messenger/featureConfigurator.dtd line 46
> <!ENTITY featureConfigurator.collapsedHeaders.para.weak "The compact message reader view has been moved into it's own add-on where it is now getting much more attention and options.">

s/it's/its/


on file: mail/locales/en-US/chrome/messenger/featureConfigurator.dtd line 67
> <!ENTITY featureConfigurator.autoSync.para "Thunderbird 3 comes with a new and better search functionality.  To experience this new search you'll need to synchronize your email messages.">

Yet another reference to Tb3.


on file: mail/test/mozmill/migration/test-shell-and-first-page.js line 42
> var MODULE_NAME = "test-migration-assistant-others";

Please fix the module name.


on file: mail/test/mozmill/migration/test-shell-and-first-page.js line 64
> function test_open_and_close_migration_assistant() {
>   plan_for_new_window("mailnews:featureconfigurator");
>   mc.click(new elib.Elem(mc.menus.helpMenu.featureConfigurator));
>   let fc = wait_for_new_window("mailnews:featureconfigurator");
>   fc.click(fc.eid("closeButton"));

Please wrap this with a plan_for_window_close/wait_for_window_close.


on file: mailnews/mailnews.js line 712
> // Thunderbird wants to show the migration assistant on upgrade by default.
> pref("mailnews.ui.show.migration.on.upgrade", true);

You should have this in all-thunderbird.js instead, because nothing else in
the patch touches mailnews. Also the pref should probably be called
"mail.ui.show.migration.on.upgrade".

Comment 29

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

Attachment: A patch to add some skeleton tests.

And here's the patch with the tests.

They all pass, but there's not a lot of testing there.  This is more a place to hang further tests on, but I wanted to get it out there and reviewed, so that I could roll the tests in to the next version of the main patch.

Some notes: I couldn't seem to get at the contents of the iframe.  I'm working on it, but I wanted to post this before making any of the changes from the previous review.

Thanks,
Blake.

Comment 30

[Rain [:rain1] (inactive)]

Comment on attachment 438999 [details] [diff] [review]
A patch to add some skeleton tests.

The tests look fine, but they don't test anything non-trivial yet :)

on file: mail/test/mozmill/shared-modules/test-migration-helpers.js line 42
> var mozmill = {};
> Cu.import("resource://mozmill/modules/mozmill.js", mozmill);
> var controller = {};
> Cu.import("resource://mozmill/modules/controller.js", controller);
> var elib = {};
> Cu.import("resource://mozmill/modules/elementslib.js", elib);
> var frame = {};
> Cu.import("resource://mozmill/modules/frame.js", frame);

I think you only really need elib out of these.


on file: mail/test/mozmill/shared-modules/test-migration-helpers.js line 72
>  * Call this to open the migration assistant, and navigate to a specific
>  * pane

Minor nit: please end this with a full stop.


on file: mail/test/mozmill/shared-modules/test-migration-helpers.js line 76
>  * @param aPane the pane to navigate to.  This value should be one of the
>  *     ones contained  in the "subpages" array of
>  *     mail/base/content/featureConfigurator.js.

The usual way we represent optional parameters is with [brackets] (e.g.
[aPane]), with an explanation of what happens if the parameters isn't
specified.


on file: mail/test/mozmill/shared-modules/test-migration-helpers.js line 114
> function close_migration_assistant(fc) {
>   fc.click(fc.eid("closeButton"));
> }

Please wrap a plan_for_window_close/wait_for_window_close around this.

Comment 31

[Rain [:rain1] (inactive)]

Rich review of the HTML/CSS parts at <http://reviews.visophyte.org/r/438537/>. You'll need to scroll down a little.

I still haven't run the patch, but the HTML and CSS seems to mostly be good.

on file: mail/base/content/featureConfigurators/subpage.css line 3
> html {
>   background: url("http://www.mozillamessaging.com/img/thunderbird/start/main-feature.jpg") no-repeat top right white;
> }

So what happens if the Internet or Mozilla Messaging servers are down? I hope
we don't get an ugly red X on the top left of the screen. (Why not embed this
instead of relying on downloading data from the Internet for it?)


on file: mail/base/content/featureConfigurators/subpage.css line 45
>   font-family: georgia,serif;

Capital G, please.

[I'm assuming overriding default fonts is what we want here.]


on file: mail/base/content/featureConfigurators/subpage.css line 124
>   color: #898378;

The color here seems redundant.


on file: mail/base/content/featureConfigurators/subpage.css line 125
>   background-color: #fff;

So in case the image extends to this point, we have a white box overlaid on
the image? Do we want that? (I guess it's better than the alternative, where
the disk space used is unreadable.)


on file: mail/base/content/featureConfigurator.xhtml line 54
>     windowtype="mailnews:featureconfigurator">

I'm not sure about this, but it seems like windowtype should be in the XUL
namespace.


on file: mail/base/content/featureConfigurators/autosync.xhtml line None

I think I'd prefer current-size for this.


on file: mail/base/content/featureConfigurators/autosync.xhtml line None

...free-space for this.


on file: mail/base/content/featureConfigurators/autosync.xhtml line None

...size-difference for this.


on file: mail/base/content/featureConfigurators/autosync.js line 97
>       $("#estimatedsize").text(formatSize(estimatedsize));

This id doesn't exist.


on file: mail/base/content/featureConfigurators/others.xhtml line 59
>   <!-- Global Context -->
>   <!-- script type="application/javascript;version=1.8"
>       src="chrome://messenger/content/featureConfigurator.js"></script -->

If you don't need it, please remove it entirely.


on file: mail/base/content/featureConfigurators/others.xhtml line 74
>         <h1>&featureConfigurator.heading;</h1>
>         <p>&featureConfigurator.para1;</p>
>         <p>&featureConfigurator.para2;</p>

These seem to be missing one level of indentation.


on file: mail/base/content/featureConfigurators/toolbar.xhtml line 74
>         <form>
>           <h1>&featureConfigurator.toolbar.heading;</h1>
>           <img class="screenshot" src="chrome://messenger/skin/featureConfigurators/toolbars.png"/>
>           <label for="toolbar-new">
>             <input type="radio" id="toolbar-new" name="toolbar" checked="true"/>
>             <div>&featureConfigurator.toolbar.new.status;</div>
>             <div class="explanation">&featureConfigurator.toolbar.new.status.explanation;</div>
>           </label>
>           <label for="toolbar-original">
>             <input type="radio" id="toolbar-original" name="toolbar"/>
>             <div>&featureConfigurator.toolbar.original.status;</div>
>             <div class="explanation">&featureConfigurator.toolbar.original.status.explanation;</div>
>           </label>
>         </form>
>         <p>&featureConfigurator.toolbarBenefits.para;</p>

So do all these.


on file: mail/themes/pinstripe/jar.mn line 4
>   skin/classic/messenger/featureConfigurators/subpage.css        (mail/featureConfigurators/subpage.css)
>   skin/classic/messenger/featureConfigurators/animation.png      (mail/featureConfigurators/animation.png)
>   skin/classic/messenger/featureConfigurators/compact-header.png (mail/featureConfigurators/compact-header.png)
>   skin/classic/messenger/featureConfigurators/folder-columns.png (mail/featureConfigurators/folder-columns.png)
>   skin/classic/messenger/featureConfigurators/toolbars.png (mail/featureConfigurators/toolbars.png)

[You need to make these changes to the aero section too, but I see Bryan's
already done that.]


on file: mail/themes/pinstripe/mail/featureConfigurator.css line 2
> 
> #buttons {
>   -moz-appearance: statusbar;
> }
> 
> #buttons button {
>   -moz-appearance: none;
>   border: 1px solid #5F5F5F;
>   background: #A09E9D;
>   min-height: 18px;
>   min-width: 0;
>   padding: 0 2px;
>   margin: 0 2px;
>   text-shadow: rgba(255, 255, 255, 0.4) 0 1px;
>   border: 1px solid rgba(0, 0, 0, 0.6);
>   -moz-border-radius: 3px;
>   -moz-box-shadow: inset 0 20px 16px -10px #FFF, 0 1px rgba(255, 255, 255, 0.4);
> }
> 
> #buttons button:hover:active:not([disabled="true"]) {
>   background: #B5B5B5;
>   text-shadow: rgba(255, 255, 255, 0.4) 0 1px;
>   -moz-box-shadow: inset rgba(0, 0, 0, 0.4) 0 -5px 12px,
>                    inset rgba(0, 0, 0, 1) 0 1px 3px,
>                    0 1px rgba(255, 255, 255, 0.4);
> 
> }
> 
> #buttons button:-moz-window-inactive {
>   color: #7C7C7C !important; /* remove this when we support click-through */
>   border-color: rgba(0, 0, 0, 0.25);
>   background-color: #CCC;
> }

Is all this intended to be pinstripe-only?


on file: mail/themes/pinstripe/mail/featureConfigurator.css line 31
>   color: #7C7C7C !important; /* remove this when we support click-through */

What is "click-through" referring to here?


on file: mail/themes/pinstripe/mail/featureConfigurators/subpage.css line 5
> }
> 

I think that should be height: 57px. Anyway, why do you need to specify the
dimensions here...


on file: mail/themes/pinstripe/mail/featureConfigurators/subpage.css line 9
>   border: 1px solid #5F5F5F;
>   background: #A09E9D;
>   min-height: 18px;
>   min-width: 0;
>   padding: 0 2px;

...and here? Also it seems like gnomestripe should have this CSS added to it.


on file: mail/themes/qute/mail/featureConfigurators/subpage.css line 3
> image.compactheader-image {
> background: url("chrome://messenger/skin/featureConfigurators/compact-header.png") no-repeat;
> width: 514px;
> width: 57px;
> }
> 
> image.folderpanecolumns-image {
> background: url("chrome://messenger/skin/featureConfigurators/folder-columns.png") no-repeat;
> width: 279px;
> height: 110px;
> }

See comment on the pinstripe version.

Comment 32

[Rain [:rain1] (inactive)]

One more global nit that I missed mentioning: please switch to aParam for parameters everywhere (except perhaps with lambdas you pass to filter and so on). While I don't generally care too much about code style, I think being able to distinguish between local variables and parameters is important.

Comment 33

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

Attachment: the previous patch, with Sid0's changes.

I think we got all the changes you requested in here.

Thanks,
Blake.

Comment 34

[Rain [:rain1] (inactive)]

Comment on attachment 439506 [details] [diff] [review]
the previous patch, with Sid0's changes.

Rich review at <http://reviews.visophyte.org/r/439506/>. This patch is pretty close to r+, but I want to have one more look before it's checked in.

I haven't run with this patch yet, but I'll do that now.

on file: mail/base/content/featureConfigurators/subpage.css line 4
>   background: url("chrome://messenger/skin/featureConfigurators/main-feature.jpg") no-repeat top right white;

[Andreas said this needs to be moved into branding?]


on file: mail/base/content/featureConfigurator.js line 64
>   get index() {
>     return this._index;
>   },

Please name this function, something like

get index fc_get_index() {


on file: mail/base/content/featureConfigurator.js line 68
>   set index(aIndex) {

Please name this function too, and add a doc comment here.


on file: mail/base/content/featureConfigurator.js line 98
>       for each (let server in fixIterator(servers, Ci.nsIMsgIncomingServer)) {
>         if (!(server instanceof Ci.nsIImapIncomingServer))
>           continue;
>         foundImap = true;
>       }

OK, so since you don't need the QI any more, I'm going to ask you to go back
to checking the type :)

In fact, since we aren't doing anything else here now, you can use
Array.some/every to demonstrate the intent of the code more clearly. So you'll
need to:

- fix iteratorUtils.jsm's toArray function, maybe in a dependent bug.
- use Array.some/every.


on file: mail/base/content/featureConfigurator.js line 109
>     aSubpageData.usedCompactHeader = aParentWin.document
>                                         .getElementById("msgHeaderViewDeck")
>                                         .usedCompactHeader;

Please fix the indentation here.


on file: mail/base/content/featureConfigurator.xhtml line 54
>     windowtype="mailnews:featureconfigurator">

[unaddressed from previous review] I'm not sure about this, but it seems like
windowtype should be in the XUL namespace.


on file: mail/base/content/featureConfigurators/autosync.xhtml line 30
>    - Portions created by the Initial Developer are Copyright (C) 2009

This shouldn't be 2010.


on file: mail/base/content/featureConfigurators/autosync.xhtml line 79
>   <button id="closeButton" onclick="FeatureConfigurator.closeButton(event)"
>           >&featureConfigurator.closeButton;</button>

I'm wondering if all instead of true and none instead of false would be better
here, both for consistency with the id and to avoid any sort of confusion
between the boolean values and the strings.


on file: mail/base/content/featureConfigurators/autosync.js line 61
>       let freeSpace = Cc["@mozilla.org/file/directory_service;1"]
>                         .getService(Ci.nsIProperties)
>                         .get("ProfD", Ci.nsILocalFile)
>                         .diskSpaceAvailable;

So, uh, we could use the ImapMail folder here --

Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch).getComplexValue("mail.root.imap-rel",
Components.interfaces.nsIRelativeFilePref).file.diskSpaceAvailable;

I don't care at all about this, though, so it's up to you to decide.


on file: mail/base/content/featureConfigurators/autosync.js line 112
>       let sync = $("#some-sync");
>       if (allSync)
>         sync = $("#all-sync");
>       else if (noneSync)
>         sync = $("#none-sync");
>       sync.click();
>       sync.change();
> 

There's an extra blank line at the end here.

[Excellent way to solve this problem. Thanks.]


on file: mail/base/content/featureConfigurators/autosync.js line 149
>         server.offlineDownload = newSync;
>         server.rootFolder.ListDescendents(allFolders);
>         for each (let folder in fixIterator(allFolders, Ci.nsIMsgFolder)) {
>           if (newSync)
>             folder.setFlag(Ci.nsMsgFolderFlags.Offline);
>           else
>             folder.clearFlag(Ci.nsMsgFolderFlags.Offline);
>         }
>         $("#"+server.key).toggleClass("syncing", newSync);

Please confirm with bienvenu whether the backend can handle rapidly setting
and clearing the flag. I'm a little worried about autosync triggering because
the offline flag is set.


on file: mail/base/content/featureConfigurators/autosync.js line 157
>         $("#"+server.key).toggleClass("syncing", newSync);

Spaces around the + please


on file: mail/base/content/featureConfigurators/autosync.js line 185
>     this.previousSyncSettings = aSubpageData.previousSyncSettings;
>     this.previousSyncPref = aSubpageData.previousSyncPref;

- So previousSyncSettings is a dictionary, which means that switching pages
and coming back won't change it, but previousSyncPref is a boolean, which
means that we'll lose the value if we do so. I think the best way is to
compute the sync pref within onload instead of in the feature configurator.

- "previous" is confusing now that we keep it in sync with what the user has
chosen. Please rename it to something like "syncSettings" and "syncPref",
something which makes it clear that we're keeping the values in sync.


on file: mail/base/content/utilityOverlay.js line 349
>   window.openDialog("chrome://messenger/content/featureConfigurator.xhtml",
>                     "_blank", "chrome,dialog=yes,all,centerscreen",
>                     // Below are window.arguments for featureConfigurator.js
>                     window, {});

While this is definitely better, this looks really strange. Have you tried
having the dictionary in the main featureConfigurator scope (gSubpageData) and
passing it into the individual onloads using parent.gSubpageData?


on file: mail/locales/en-US/chrome/messenger/featureConfigurator.dtd line 77
> <!-- LOCALIZATION NOTE (featureConfigurator.diskSpace.warning): The label shown when your available hard disk space is less than 10% of the total and less than required by your IMAP accounts. -->
> <!ENTITY featureConfigurator.diskSpace.warning "You're running low on space!">

This localization note is out of date.


on file: mail/test/mozmill/migration/test-autosync.js line 38
> /*
>  * Test that the migration assistant works properly.
>  */

For all the tests -- please make these comments more specific.


on file: mail/test/mozmill/migration/test-autosync.js line 51
> function setupModule(module) {
>   elib = {};
>   Cu.import("resource://mozmill/modules/elementslib.js", elib);
> 
>   let wh = collector.getModule("window-helpers");
>   wh.installInto(module);
>   let mh = collector.getModule("migration-helpers");
>   mh.installInto(module);
> 
>   mc = wait_for_existing_window("mail:3pane");
> }

A remark about all the tests -- I think we should use folder-display-helpers,
even though we don't really need to. Two reasons:
1. Importing the folder display helper causes a cleanup routine to be executed
at test teardown. The cleanup routine prevents test failures causing a cascade
of unrelated failures.
2. You'll get mc for free.


on file: mail/test/mozmill/migration/test-autosync.js line 52
>   elib = {};
>   Cu.import("resource://mozmill/modules/elementslib.js", elib);

Again, this goes for all the tests: you don't really need elib anywhere but in
test-migration-helpers.js.


on file: mail/test/mozmill/shared-modules/test-migration-helpers.js line 45
> Cu.import("resource:///modules/iteratorUtils.jsm");
> 

Doesn't seem like you need this.


on file: mail/themes/pinstripe/jar.mn line 8
>   skin/classic/messenger/featureConfigurators/toolbars.png (mail/featureConfigurators/toolbars.png)
>   skin/classic/messenger/featureConfigurators//main-feature.jpg (mail/featureConfigurators/main-feature.jpg)

The opening ( for these two columns aren't aligned properly.


on file: mail/themes/pinstripe/jar.mn line 9
>   skin/classic/messenger/featureConfigurators//main-feature.jpg (mail/featureConfigurators/main-feature.jpg)

There's an extra / here.


on file: mail/themes/pinstripe/mail/featureConfigurator.css line 2
> 
> #buttons {
>   -moz-appearance: statusbar;
> }
> 
> #buttons button {
>   -moz-appearance: none;
>   border: 1px solid #5F5F5F;
>   background: #A09E9D;
>   min-height: 18px;
>   min-width: 0;
>   padding: 0 2px;
>   margin: 0 2px;
>   text-shadow: rgba(255, 255, 255, 0.4) 0 1px;
>   border: 1px solid rgba(0, 0, 0, 0.6);
>   -moz-border-radius: 3px;
>   -moz-box-shadow: inset 0 20px 16px -10px #FFF, 0 1px rgba(255, 255, 255, 0.4);
> }
> 
> #buttons button:hover:active:not([disabled="true"]) {
>   background: #B5B5B5;
>   text-shadow: rgba(255, 255, 255, 0.4) 0 1px;
>   -moz-box-shadow: inset rgba(0, 0, 0, 0.4) 0 -5px 12px,
>                    inset rgba(0, 0, 0, 1) 0 1px 3px,
>                    0 1px rgba(255, 255, 255, 0.4);
> 
> }
> 
> #buttons button:-moz-window-inactive {
>   color: #7C7C7C !important;
>   border-color: rgba(0, 0, 0, 0.25);
>   background-color: #CCC;
> }

[unaddressed from the previous patch] Why isn't this in either gnomestripe or
qute?


on file: mail/themes/pinstripe/mail/featureConfigurators/subpage.css line 2
> 
> #buttons {
>   -moz-appearance: statusbar;
> }
> 
> #buttons button {
>   -moz-appearance: none;
>   border: 1px solid #5F5F5F;
>   background: #A09E9D;
>   min-height: 18px;
>   min-width: 0;
>   padding: 0 2px;

These dimensions are different from the ones in gnomestripe.


on file: mail/themes/qute/jar.mn line 49
>   skin/classic/messenger/featureConfigurators/toolbars.png  (mail/featureConfigurators/toolbars.png)
>   skin/classic/messenger/featureConfigurators/main-feature.jpg  (mail/featureConfigurators//main-feature.jpg)

The opening ( isn't aligned properly).


on file: mail/themes/qute/jar.mn line 270
>   skin/classic/aero/messenger/featureConfigurators/subpage.css     (mail/featureConfigurators/subpage.css)
>   skin/classic/aero/messenger/featureConfigurators/animation.png   (mail/featureConfigurators/animation.png)
>   skin/classic/aero/messenger/featureConfigurators/compact-header.png  (mail/featureConfigurators/compact-header.png)
>   skin/classic/aero/messenger/featureConfigurators/folder-columns.png  (mail/featureConfigurators/folder-columns.png)
>   skin/classic/aero/messenger/featureConfigurators/toolbars.png    (mail/featureConfigurators/toolbars.png)

main-feature.jpg isn't present.

Comment 35

[Rain [:rain1] (inactive)]

on file: mail/themes/qute/mail/featureConfigurators/subpage.css line 2
> 
> image.compactheader-image {
>   background: url("chrome://messenger/skin/featureConfigurators/compact-header.png") no-repeat;
>   width: 514px;
>   width: 57px;
> }
> 
> image.folderpanecolumns-image {
>   background: url("chrome://messenger/skin/featureConfigurators/folder-columns.png") no-repeat;
>   width: 279px;
>   height: 110px;
> }

These dimensions are also different.

Comment 36

[Rain [:rain1] (inactive)]

About testing:

- Since we can't test autosync ourselves, we need to add Litmus tests for it. Blake, since you understand the code best, could you write some up?
- Hm, I wonder what else we can test other than just opening and closing pages. I notice that a test for the toolbar is missing. I think we could possibly do some testing there, maybe making use of a custom localStore.rdf we get from a Tb2 install.

Comment 37

[Rain [:rain1] (inactive)]

(In reply to comment #34)
> on file: mail/themes/qute/jar.mn line 49
> >   skin/classic/messenger/featureConfigurators/toolbars.png  (mail/featureConfigurators/toolbars.png)
> >   skin/classic/messenger/featureConfigurators/main-feature.jpg  (mail/featureConfigurators//main-feature.jpg)
> 
> The opening ( isn't aligned properly).

And there's an extra / as well.

Comment 38

[Rain [:rain1] (inactive)]

Attachment: incorrect fonts and alignment for Previous/Next/Close buttons

OK, I'm running with the patch now. UI nits:

- The font for the buttons is incorrect. I don't know whether this can actually be fixed, given that the previous/next buttons are HTML, but it's worth a try.

- Note how the text of the Close button is slightly below that of the Previous and Next buttons.

Comment 39

[Rain [:rain1] (inactive)]

Attachment: autosync: text cut off

Note how the text is strangely cut off. (main-feature.jpg isn't packaged for the Aero theme, which is why there's no background.)

Also the location of the Free Space and Required labels looks a little strange.

Comment 40

[Rain [:rain1] (inactive)]

Attachment: toolbar window

The text is completely unclear.

- "All your messaging buttons at your finger tips"? Does that mean that if I select the other option the buttons will disappear? And there shouldn't be a space between finger and tips.
- The image seems oddly stretched.
- Neither the "message buttons toolbar" nor the "original toolbar" have anything to do with the image, really.
- The page is just a pixel or two larger than will fit into the box.

Comment 41

[Rain [:rain1] (inactive)]

Attachment: compact header page

I wasn't using the compact message header. I think the logic you use only works if the user upgraded from Thunderbird 2.0. (Please add a test for this!)

I tried installing it, but canceled it because it was incompatible with the version of Thunderbird I was testing with. The text remained stuck at "Installing…". Looking at the code I don't see where we change the "Installing…" to "Installed", and where we do have it, it seems like saying something like "This extension is already installed" would make more sense.

Comment 42

[Rain [:rain1] (inactive)]

Also the text uses the word "add-on" when it means "extension". All Mozilla products seem to use the word "add-on" to refer to extensions or themes or plugins.

Comment 43

[Rain [:rain1] (inactive)]

Ugh, the window is resizable but the actual content doesn't resize with it. Please fix this one way or the other -- hopefully by making the content resizable.

Comment 44

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

(In reply to comment #38)
> Created an attachment (id=439585) [details]
> incorrect fonts and alignment for Previous/Next/Close buttons
> 
> OK, I'm running with the patch now. UI nits:
> 
> - The font for the buttons is incorrect. I don't know whether this can actually
> be fixed, given that the previous/next buttons are HTML, but it's worth a try.
> 
> - Note how the text of the Close button is slightly below that of the Previous
> and Next buttons.

I can easily get the buttons to the correct margins however getting them to style like the XUL buttons could be difficult.  I'll give it a go.

Comment 45

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

Issues from comment #38 are fixed in the branch

(In reply to comment #39)
> Created an attachment (id=439586) [details]
> autosync: text cut off
> 
> Note how the text is strangely cut off. (main-feature.jpg isn't packaged for
> the Aero theme, which is why there's no background.)

This is fixed now.  We needed a line-height for the disk space used text and to give some padding at the bottom so we don't run under it.

> Also the location of the Free Space and Required labels looks a little strange.

The spacing is a bit off, I'll work on that a bit, but otherwise I'm not sure what you mean.

Comment 46

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

(In reply to comment #42)
> Also the text uses the word "add-on" when it means "extension". All Mozilla
> products seem to use the word "add-on" to refer to extensions or themes or
> plugins.

Fixed in the branch.  I used extension instead of add-on when we are referring to a specific extension.  I left one reference to add-on in there because it was referring to all types of extensions, themes, etc. aka add-ons

Comment 47

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

(In reply to comment #43)
> Ugh, the window is resizable but the actual content doesn't resize with it.
> Please fix this one way or the other -- hopefully by making the content
> resizable.

This is because the window sizing is now in CSS instead of the window.open() call.  A fixed CSS size means we can't use width: 100%; height: 100%; to resize the content as the window size changes.

I'll play around with it some more but even using the min-(width|height) elements and height: 100%; width: 100%; means you can't resize smaller and see everything.

Comment 48

[Rain [:rain1] (inactive)]

(In reply to comment #34) 
> on file: mail/base/content/featureConfigurator.js line 64
> >   get index() {
> >     return this._index;
> >   },
> 
> Please name this function, something like
> 
> get index fc_get_index() {

Sorry, my bad, turns out that this is only unintentionally accepted:

<http://whereswalden.com/2010/04/16/more-spidermonkey-changes-ancient-esoteric-very-rarely-used-syntax-for-creating-getters-and-setters-is-being-removed>

and while it still seems to work post Waldo's patch, it is at serious risk of being dropped. Please revert the change.

Comment 49

[Andreas Nilsson (:andreasn)]

(In reply to comment #34)

> on file: mail/themes/pinstripe/mail/featureConfigurator.css line 2
> > 
> > #buttons {
> >   -moz-appearance: statusbar;
> > }
> > 
> > #buttons button {
> >   -moz-appearance: none;
> >   border: 1px solid #5F5F5F;
> >   background: #A09E9D;
> >   min-height: 18px;
> >   min-width: 0;
> >   padding: 0 2px;
> >   margin: 0 2px;
> >   text-shadow: rgba(255, 255, 255, 0.4) 0 1px;
> >   border: 1px solid rgba(0, 0, 0, 0.6);
> >   -moz-border-radius: 3px;
> >   -moz-box-shadow: inset 0 20px 16px -10px #FFF, 0 1px rgba(255, 255, 255, 0.4);
> > }
> > 
> > #buttons button:hover:active:not([disabled="true"]) {
> >   background: #B5B5B5;
> >   text-shadow: rgba(255, 255, 255, 0.4) 0 1px;
> >   -moz-box-shadow: inset rgba(0, 0, 0, 0.4) 0 -5px 12px,
> >                    inset rgba(0, 0, 0, 1) 0 1px 3px,
> >                    0 1px rgba(255, 255, 255, 0.4);
> > 
> > }
> > 
> > #buttons button:-moz-window-inactive {
> >   color: #7C7C7C !important;
> >   border-color: rgba(0, 0, 0, 0.25);
> >   background-color: #CCC;
> > }
> 
> [unaddressed from the previous patch] Why isn't this in either gnomestripe or
> qute?

This takes care of rendering the specific buttons used for pinstripe's harder looking buttons for the migration assistant. As qute and gnomestripe goes for more "normal" looking buttons here we don't need that for those.

Comment 50

[Andreas Nilsson (:andreasn)]

(In reply to comment #35)
> on file: mail/themes/qute/mail/featureConfigurators/subpage.css line 2
> > 
> > image.compactheader-image {
> >   background: url("chrome://messenger/skin/featureConfigurators/compact-header.png") no-repeat;
> >   width: 514px;
> >   width: 57px;
> > }
> > 
> > image.folderpanecolumns-image {
> >   background: url("chrome://messenger/skin/featureConfigurators/folder-columns.png") no-repeat;
> >   width: 279px;
> >   height: 110px;
> > }
> 
> These dimensions are also different.

I realized that these (and it's counterparts in the other themes) are actually obsolete and removed them. It was from when the images was referred to as backgrounds, but we later realized we could ask for them with the img tag (as it's html anyway). Removed these and made sure the bitmap images are the same size across all 3 themes in a bundle Blake will move in today or tomorrow.

Comment 51

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

(Just an update of what's in the repo so far.)

(In reply to comment #48)
> > >   get index() {
> > Please name this function, something like
> > get index fc_get_index() {
> Sorry, my bad, turns out that this is only unintentionally accepted:
> and while it still seems to work post Waldo's patch, it is at serious risk of
> being dropped. Please revert the change.

Reverted.

(In reply to comment #47)
> > Ugh, the window is resizable but the actual content doesn't resize with it.
> > Please fix this one way or the other -- hopefully by making the content
> > resizable.
> This is because the window sizing is now in CSS instead of the window.open()
> call.  A fixed CSS size means we can't use width: 100%; height: 100%; to
> resize the content as the window size changes.
> 
> I'll play around with it some more but even using the min-(width|height)
> elements and height: 100%; width: 100%; means you can't resize smaller and see
> everything.

For now I'm just going to put the width/height back in the call to window.open.
 Sure, it's weird, but it works, and none of the other stuff I've tried has
worked.  (Bryan, please feel free to continue playing around with it, and let
me know if you get something working.)

(In reply to comment #41)
> Created an attachment (id=439593) [details]
> compact header page
> 
> I wasn't using the compact message header. I think the logic you use only
> works if the user upgraded from Thunderbird 2.0. (Please add a test for this!)

Fixed, and added a test!  Woo!  :)

And Andreas's bundle is in the repo, too.

Later,
Blake.

Comment 52

[Andreas Nilsson (:andreasn)]

(In reply to comment #34)
> on file: mail/themes/pinstripe/mail/featureConfigurators/subpage.css line 2
> > 
> > #buttons {
> >   -moz-appearance: statusbar;
> > }
> > 
> > #buttons button {
> >   -moz-appearance: none;
> >   border: 1px solid #5F5F5F;
> >   background: #A09E9D;
> >   min-height: 18px;
> >   min-width: 0;
> >   padding: 0 2px;
> 
> These dimensions are different from the ones in gnomestripe.

These are not present in gnomestripe any more, and if I recall things correctly, this is to ensure the specifically-drawn pinstripe buttons (mentioned in comment 49) render correctly, something gnomestripe and qute don't need.

Comment 53

[Timo Pietilä]

Wouldn't it be easier to just remove buttons from header panel if user selects "old toolbar" than recommending them to install add-on for that feature? I'm afraid that this add-on doesn't keep up-to-date with all of the future upgrades, and I consider compact and neat header panel core feature of TB. Current design just takes too much space. Even just moving all those buttons on single line would be improvement (that dropdown-button takes one extra line bloating the panel).

If buttons are removed it is simple matter of selecting which headers are shown in that panel and resize accordingly.

With that fixed add-on recommendation in migration wizard becomes obsolete.

Comment 54

[Rain [:rain1] (inactive)]

Blake: where's the code that deals with switching between the old and the new toolbars? I don't see isNewToolbar being used anywhere.

Comment 55

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

(In reply to comment #54)
> Blake: where's the code that deals with switching between the old and the new
> toolbars? I don't see isNewToolbar being used anywhere.

Yeah, it got lost in the shuffle.  I added back in, and committed to the repo.

New patch coming soon.

Comment 56

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

Attachment: Patch v3.

I believe this has most of the changes you asked for, but you might want to ask Bryan before starting the review.

The only intentional omission is that the text remains stuck at "Installing…".  I don't know if we get any sort of page refresh after the extension is (or isn't) installed, so I'm not sure if we can change this.  I guess we could flip it back to the install button after a couple of seconds, or something.  Bryan, thoughts?

Thanks,
Blake.

Comment 57

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

(In reply to comment #56)
> The only intentional omission is that the text remains stuck at "Installing…". 
> I don't know if we get any sort of page refresh after the extension is (or
> isn't) installed, so I'm not sure if we can change this.  I guess we could flip
> it back to the install button after a couple of seconds, or something.  Bryan,
> thoughts?

I think we could (on a timeout) check if it was actually installed and change to "Installed" text.  But all people have to do is change pages so I'm not too worried about the "Installing..." text sticking around after they cancel out the install.  It's not ideal but it works well enough to not worry too much about.

Comment 58

[Rain [:rain1] (inactive)]

All these fixes are extremely minor. I'm minusing mainly because I'd like to
see the toolbar/compact header upgrade tests.


on file: mail/base/content/featureConfigurator.js line 91
>     this.gSubpageData = {};
> 

Two choices -- either make gSubpageData a global or rename this to subpageData
(and make it a property instead of initializing it in init). Either one's fine
with me.


on file: mail/base/content/featureConfigurator.js line 94
>     this.gSubpageData.dom = aParentWin.document;
>     let toolbar = this.gSubpageData.dom.getElementById("mail-bar3");
>     this.gSubpageData.useSmartFolders =
>       aParentWin.gFolderTreeView.mode == "smart";
>     this.gSubpageData.isNewToolbar =
>       toolbar.currentSet == toolbar.getAttribute("defaultset");
>     this.gSubpageData.fakebar = aParentWin.document.getElementById("mail-bar2");
>     this.gSubpageData.newbar = aParentWin.document.getElementById("mail-bar3");

This won't work if the 3-pane window's closed. Please XXX this for now and
we'll revisit it with RC1.


on file: mail/base/content/featureConfigurator.js line 95
>     let toolbar = this.gSubpageData.dom.getElementById("mail-bar3");

aParentWin.document.getElementById please.


on file: mail/base/content/featureConfigurator.js line 124
>     // We used the compactHeader if we're upgrading, and the index was 0;

A full stop or nothing at the end.


on file: mail/base/content/featureConfigurators/autosync.js line 213
>             window.openDialog("chrome://messenger/content/AccountManager.xul",
>                                  "AccountManager",
>                                  "chrome,centerscreen,modal,titlebar",
>                                  {server: aServer, selectPage: "am-offline.xul"});

The indentation is slightly off -- please align the opening quotes/brace.


on file: mail/base/content/featureConfigurators/extensions.js line 70
>   install: function ex_install() {
>     location.href = this.extensionUrl;
>     $("#addon-install-button").hide();
>     $("#installing").fadeIn();
>   },

We could possibly load the extension inside another iframe, and possibly hook
into the extension manager to get to know when the extension's been installed.
Blake's going to look at this later -- this is fine for now. Please add an XXX
though.


on file: mail/base/content/featureConfigurators/toolbar.js line 58
>       if (! aUseNew) {

Please remove the space after the !.


on file: mail/base/content/featureConfigurators/toolbar.js line 69
>         for (let i = 0; i < bits.length; i++) {
>           let bit = bits[i];
>           switch (bit) {
>             case "search-container":
>             case "gloda-search":
>               newbits.push("gloda-search");
>               foundSearch = true;
>               break;
>             default:
>               newbits.push(bit);
>           }
>         }

for each (let [, bit] in Iterator(bits)) please.


on file: mail/base/content/featureConfigurators/toolbar.js line 89
>         if (labelalign)
>           this.newbar.parentNode.setAttribute("labelalign", labelalign);

Is the if check necessary?


on file: mail/base/content/featureConfigurators/toolbar.js line 96
>         if (iconsize) {
>           this.newbar.setAttribute("iconsize", iconsize);
>           this.newbar.parentNode.setAttribute("iconsize", "large");
>         }

Is the if check necessary here?


on file: mail/base/content/utilityOverlay.js line 351
>  * @param upgrade whether this is being opened as a result of upgrading
>  *     from an earlier version of Thunderbird.
>  */
> function openFeatureConfigurator(upgrade) {
>   window.openDialog("chrome://messenger/content/featureConfigurator.xhtml",
>                     "_blank",
>                     "chrome,dialog=yes,all,centerscreen,width=704,height=416",
>                     // Below are window.arguments for featureConfigurator.js
>                     window, upgrade);

aIsUpgrade, please.


on file: mail/base/jar.mn line 66
> *   content/messenger/featureConfigurators/toolbar.js               (content/featureConfigurators/toolbar.js)

Ew, can we use Application.platformIsMac instead please?


on file: mail/locales/en-US/chrome/messenger/featureConfigurator.dtd line 23
> <!-- LOCALIZATION NOTE (featureConfigurator.addOn.installing): The text to display while installing the addon. -->
> <!ENTITY featureConfigurator.addOn.installing "Installing…">
> 
> <!-- LOCALIZATION NOTE (featureConfigurator.addOn.installed): The label to display when the addon is already installed. -->
> <!ENTITY featureConfigurator.addOn.installed "This extension is already installed.">

Please add a string for "Installed", and change 'installed' to
alreadyInstalled.


on file: mail/test/mozmill/migration/test-compactheader.js line 80
> // TODO: Figure out how to test the upgrade case, where the strong message
> //       is displayed by default.

With a Thunderbird 2 profile, I guess?


on file: mail/test/mozmill/migration/test-folderpanecolumns.js line 58
> function test_open_and_close_folderpanecolumns() {
>   // Open the migration assistant, and navigate to the folderpanecolumns page.
>   let fc = open_migration_assistant(mc, "folderpanecolumns");
>   close_migration_assistant(fc);
> }
> 

nit: blank line at the end.


on file: mail/test/mozmill/migration/test-introduction.js line 58
> function test_open_and_close_migration_assistant() {
>   // Open the migration assistant, and navigate to the autosync page.
>   let fc = open_migration_assistant(mc, "introduction");
>   close_migration_assistant(fc);
> }
> 

and here too.


on file: mail/test/mozmill/shared-modules/test-folder-display-helpers.js line 2476
> 
> // something less sucky than do_check_true
> function assert_true(aBeTrue, aWhy) {
>   if (!aBeTrue)
>     throw new Error(aWhy);
> }

If you add this here you'll need to fix whichever other places in the code use
assert_true.


on file: mail/test/mozmill/shared-modules/test-migration-helpers.js line 117
> function get_subpage(fc) {
>   let contentWindow = fc.e("contentFrame").contentWindow;
>   let aController = new controller.MozMillController(contentWindow);
>   return wh.augment_controller(aController);
> }

Please add a comment for this.

on file: mail/base/content/featureConfigurators/toolbar.js line 101
>         // reset to factory defaults (TB3)
>         let defaultset = this.newbar.getAttribute("defaultset")
>         // this makes it show up
>         this.newbar.currentSet = defaultset;
>         // this makes it persist ...
>         this.newbar.setAttribute("currentset", defaultset);
>         this.newbar.parentNode.setAttribute("labelalign", "end");
> #ifdef XP_MACOSX
>         this.newbar.setAttribute("iconsize", "small");
>         this.newbar.parentNode.setAttribute("iconsize", "small");
> #else
>         this.newbar.setAttribute("iconsize", "large");
>         this.newbar.parentNode.setAttribute("iconsize", "large");
> #endif
>       }

So, well, I customized my toolbar, then fired this up.

- "Original Toolbar" was selected. I don't know how much sense that makes,
given the text beneath it. You might want to consider adding a third option
for people like me, called "Keep your customizations" or something similar, or
merge it into the original toolbar option somehow.
- I switched to "Message Buttons Toolbar", which switches the toolbar to the
default set, then back to the "Original Toolbar", which switched it back to...
the Thunderbird 2 set! I certainly didn't expect to lose my customizations :(

Please add tests for this, whichever way you fix it.

Comment 59

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

(In reply to comment #58)
> on file: mail/base/content/featureConfigurator.js line 91
> >     this.gSubpageData = {};
> Two choices -- either make gSubpageData a global or rename this to subpageData
> (and make it a property instead of initializing it in init). Either one's fine
> with me.

Fixed.

> on file: mail/base/content/featureConfigurator.js line 94
> >     this.gSubpageData.dom = aParentWin.document;
> >     let toolbar = this.gSubpageData.dom.getElementById("mail-bar3");
> This won't work if the 3-pane window's closed. Please XXX this for now and
> we'll revisit it with RC1.

Commented.

> on file: mail/base/content/featureConfigurator.js line 95
> >     let toolbar = this.gSubpageData.dom.getElementById("mail-bar3");
> aParentWin.document.getElementById please.

Changed, although I'm not entirely sure why we wouldn't use the stored copy.

> on file: mail/base/content/featureConfigurator.js line 124
> >     // We used the compactHeader if we're upgrading, and the index was 0;
> A full stop or nothing at the end.

Fixed.

> on file: mail/base/content/featureConfigurators/autosync.js line 213
> >             window.openDialog("chrome://messenger/content/AccountManager.xul",
> >                                  "AccountManager",
> >                                  "chrome,centerscreen,modal,titlebar",
> >                                  {server: aServer, selectPage: "am-offline.xul"});
> The indentation is slightly off -- please align the opening quotes/brace.

Fixed.

> on file: mail/base/content/featureConfigurators/extensions.js line 70
> >   install: function ex_install() {
> >     location.href = this.extensionUrl;
> >     $("#addon-install-button").hide();
> >     $("#installing").fadeIn();
> >   },
> 
> We could possibly load the extension inside another iframe, and possibly hook
> into the extension manager to get to know when the extension's been installed.
> Blake's going to look at this later -- this is fine for now. Please add an XXX
> though.

Commented.

> on file: mail/base/content/featureConfigurators/toolbar.js line 58
> >       if (! aUseNew) {
> Please remove the space after the !.

Fixed.

> on file: mail/base/content/featureConfigurators/toolbar.js line 69
> >         for (let i = 0; i < bits.length; i++) {
> >           let bit = bits[i];
> for each (let [, bit] in Iterator(bits)) please.

Changed.

> on file: mail/base/content/featureConfigurators/toolbar.js line 89
> >         if (labelalign)
> >           this.newbar.parentNode.setAttribute("labelalign", labelalign);
> Is the if check necessary?

https://developer.mozilla.org/en/DOM/element.setAttribute says:
"Even though getAttribute()  returns null for missing attributes, you should use removeAttribute()  instead of elt.setAttribute(attr, null) to remove the attribute."

> on file: mail/base/content/featureConfigurators/toolbar.js line 96
> >         if (iconsize) {
> >           this.newbar.setAttribute("iconsize", iconsize);
> >           this.newbar.parentNode.setAttribute("iconsize", "large");
> >         }
> Is the if check necessary here?

Fixed.

> on file: mail/base/content/utilityOverlay.js line 351
> >  * @param upgrade whether this is being opened as a result of upgrading
> >  *     from an earlier version of Thunderbird.
> >  */
> > function openFeatureConfigurator(upgrade) {
> >   window.openDialog("chrome://messenger/content/featureConfigurator.xhtml",
> >                     "_blank",
> >                     "chrome,dialog=yes,all,centerscreen,width=704,height=416",
> >                     // Below are window.arguments for featureConfigurator.js
> >                     window, upgrade);
> aIsUpgrade, please.

Fixed.

> on file: mail/base/jar.mn line 66
> > *   content/messenger/featureConfigurators/toolbar.js               (content/featureConfigurators/toolbar.js)
> Ew, can we use Application.platformIsMac instead please?

Fixed!  (And thanks!  It looks much nicer.)

> on file: mail/locales/en-US/chrome/messenger/featureConfigurator.dtd line 23
> > <!-- LOCALIZATION NOTE (featureConfigurator.addOn.installed): The label to display when the addon is already installed. -->
> > <!ENTITY featureConfigurator.addOn.installed "This extension is already installed.">
> Please add a string for "Installed", and change 'installed' to
> alreadyInstalled.

Fixed.

> on file: mail/test/mozmill/migration/test-compactheader.js line 80
> > // TODO: Figure out how to test the upgrade case, where the strong message
> > //       is displayed by default.
> With a Thunderbird 2 profile, I guess?

That's the plan.

> on file: mail/test/mozmill/migration/test-folderpanecolumns.js line 58
> > function test_open_and_close_folderpanecolumns() {
> >   // Open the migration assistant, and navigate to the folderpanecolumns page.
> >   let fc = open_migration_assistant(mc, "folderpanecolumns");
> >   close_migration_assistant(fc);
> > }
> > 
> nit: blank line at the end.

Fixed.

> on file: mail/test/mozmill/migration/test-introduction.js line 58
> > function test_open_and_close_migration_assistant() {
> >   // Open the migration assistant, and navigate to the autosync page.
> >   let fc = open_migration_assistant(mc, "introduction");
> >   close_migration_assistant(fc);
> > }
> > 
> and here too.

Fixed.

> on file: mail/test/mozmill/shared-modules/test-folder-display-helpers.js line
> 2476
> > // something less sucky than do_check_true
> > function assert_true(aBeTrue, aWhy) {
> >   if (!aBeTrue)
> >     throw new Error(aWhy);
> > }
> If you add this here you'll need to fix whichever other places in the code use
> assert_true.

I had already fixed them.  :)
(They were in folder-display/test_message_commands.js.)

> on file: mail/test/mozmill/shared-modules/test-migration-helpers.js line 117
> > function get_subpage(fc) {
> >   let contentWindow = fc.e("contentFrame").contentWindow;
> >   let aController = new controller.MozMillController(contentWindow);
> >   return wh.augment_controller(aController);
> > }
> Please add a comment for this.

Added.

And pushed to my repo.  I'm working on the last part with Bryan now, and will post the patch when it's done.

Thanks,
Blake.

Comment 60

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

Attachment: Patch, v4.

> on file: mail/base/content/featureConfigurators/toolbar.js line 101
> >         // reset to factory defaults (TB3)
> >         let defaultset = this.newbar.getAttribute("defaultset")
> >         // this makes it show up
> >         this.newbar.currentSet = defaultset;
> >         // this makes it persist ...
> >         this.newbar.setAttribute("currentset", defaultset);
> >         this.newbar.parentNode.setAttribute("labelalign", "end");
> > #ifdef XP_MACOSX
> >         this.newbar.setAttribute("iconsize", "small");
> >         this.newbar.parentNode.setAttribute("iconsize", "small");
> > #else
> >         this.newbar.setAttribute("iconsize", "large");
> >         this.newbar.parentNode.setAttribute("iconsize", "large");
> > #endif
> >       }
> 
> So, well, I customized my toolbar, then fired this up.
> 
> - "Original Toolbar" was selected. I don't know how much sense that makes,
> given the text beneath it. You might want to consider adding a third option
> for people like me, called "Keep your customizations" or something similar, or
> merge it into the original toolbar option somehow.
> - I switched to "Message Buttons Toolbar", which switches the toolbar to the
> default set, then back to the "Original Toolbar", which switched it back to...
> the Thunderbird 2 set! I certainly didn't expect to lose my customizations :(
> 
> Please add tests for this, whichever way you fix it.

And the wording has been changed, and the bug has been fixed, and tested, and here's the new patch.

Thanks,
Blake.

Comment 61

[Rain [:rain1] (inactive)]

Comment on attachment 440102 [details] [diff] [review]
Patch, v4.

on file: mail/locales/en-US/chrome/messenger/featureConfigurator.dtd line 26
> <!-- LOCALIZATION NOTE (featureConfigurator.addOn.installed): The text to display after the user installs the addon from the configurator. -->
> <!ENTITY featureConfigurator.addOn.installing "Installed">

featureConfigurator.addOn.installed. Thanks to Standard8 for pointing this
out!

Also, Standard8 proposes inserting a hidden xul element using the string even
if it doesn't get shown. "That and given the localization comment already
present
should at least help localisers pick up where the string will be and the
appropriate contexts."


on file: mail/base/content/featureConfigurators/toolbar.js line 101
>         // Save off what we currently have to the fakebar, in case the user
>         // customized it.
>         this.fakebar.setAttribute("currentset",
>                                   this.newbar.getAttribute("currentset"));
>         this.fakebar.setAttribute("labelalign",
>                                   this.newbar.parentNode
>                                       .getAttribute("labelalign"));
>         this.fakebar.setAttribute("iconsize",
>                                   this.newbar.getAttribute("iconsize"));
>         this.fakebar.setAttribute("parenticonsize",
>                                   this.newbar.parentNode
>                                       .getAttribute("iconsize"));

Very clean and elegant solution. Thanks!


on file: mail/locales/en-US/chrome/messenger/featureConfigurator.dtd line 1
> <!-- LOCALIZATION NOTE (featureConfigurator.title): Title for the Migration Assistant page. It should match featureConfiguratorCmd.label in baseMenuOverlay.dtd -->
> <!ENTITY featureConfigurator.title "Migration Assistant">
> 
> <!-- LOCALIZATION NOTE (featureConfigurator.heading1): Heading for the page -->
> <!ENTITY featureConfigurator.heading1 "New Thunderbird features worth knowing about">
> 
> <!-- LOCALIZATION NOTE (featureConfigurator.para.1): The tone of this paragraph is important: we tried for a tone that promotes the new features (which we believe are good and worth users learning about), but also emphasizing that we think it's important to preserve user choice. -->
> <!ENTITY featureConfigurator.para.1 "This new version of Thunderbird includes powerful new features which we're quite proud of, and think most people will love. People's tastes and needs vary, so we made it easy to control and customize Thunderbird.">

I didn't realize it because the current migration assistant also has
Thunderbird in it, but you should use &brandShortName; everywhere.



A couple things I noticed while testing this out on Mac --
1. disabled buttons are also press-able
2. for a Thunderbird 2 to 3 upgrade, it's a little annoying to have the system
integration sheet and the migration assistant show up at the same time. I
think you'll need to handle this somehow, possibly by having some sort of
callback at close that causes the system integration dialog to be displayed.

And you still don't have Tb2 -> Tb3 upgrade tests :)

I'd prefer if you uploaded a separate patch for bugs 1 and 2, along with Tb2
-> Tb3 upgrade tests, layered on top of this one. r+, but please don't land
before getting r+ on a patch fixing whatever I've mentioned here.

Comment 62

[Rain [:rain1] (inactive)]

OK, couple of failures with the tests:

TEST-UNEXPECTED-FAIL |  test_new_toolbar_with_default_tb3
  EXCEPTION: The currentset is incorrect.: 'button-getmsg,button-newmsg,button-address,separator,button-t
ag,spring,gloda-search' != 'button-getmsg,button-newmsg,button-address,spacer,button-tag,spring,gloda-sea
rch,throbber-box'.
    at: test-folder-display-helpers.js line 2487
       Error("The currentset is incorrect.: 'button-getmsg,button-newmsg,button-address,separator,button-
tag,spring,gloda-search' != 'button-getmsg,button-newmsg,button-address,spacer,button-tag,spring,gloda-se
arch,throbber-box'.")  0
       assert_true(false,"The currentset is incorrect.: 'button-getmsg,button-newmsg,button-address,separ
ator,button-tag,spring,gloda-search' != 'button-getmsg,button-newmsg,button-address,spacer,button-tag,spr
ing,gloda-search,throbber-box'.") test-folder-display-helpers.js 2487
       assert_equals("button-getmsg,button-newmsg,button-address,separator,button-tag,spring,gloda-search
","button-getmsg,button-newmsg,button-address,spacer,button-tag,spring,gloda-search,throbber-box","The cu
rrentset is incorrect.") test-folder-display-helpers.js 2481
       assert_default_tb3_settings([object XULElement]) test-toolbar.js 84
       test_new_toolbar_with_default_tb3() test-toolbar.js 146
            frame.js 468
            frame.js 520
            frame.js 562
            frame.js 411
            frame.js 568
            server.js 164
            server.js 168
TEST-UNEXPECTED-FAIL |  test_new_toolbar_with_custom_tb3
  EXCEPTION: The currentset is incorrect.: 'button-getmsg,button-newmsg,button-address,separator,button-t
ag,spring,gloda-search' != 'button-getmsg,button-newmsg,button-address,spacer,button-tag,spring,gloda-sea
rch,throbber-box'.
    at: test-folder-display-helpers.js line 2487
       Error("The currentset is incorrect.: 'button-getmsg,button-newmsg,button-address,separator,button-
tag,spring,gloda-search' != 'button-getmsg,button-newmsg,button-address,spacer,button-tag,spring,gloda-se
arch,throbber-box'.")  0
       assert_true(false,"The currentset is incorrect.: 'button-getmsg,button-newmsg,button-address,separ
ator,button-tag,spring,gloda-search' != 'button-getmsg,button-newmsg,button-address,spacer,button-tag,spr
ing,gloda-search,throbber-box'.") test-folder-display-helpers.js 2487
       assert_equals("button-getmsg,button-newmsg,button-address,separator,button-tag,spring,gloda-search
","button-getmsg,button-newmsg,button-address,spacer,button-tag,spring,gloda-search,throbber-box","The cu
rrentset is incorrect.") test-folder-display-helpers.js 2481
       assert_default_tb3_settings([object XULElement]) test-toolbar.js 84
       test_new_toolbar_with_custom_tb3() test-toolbar.js 187
            frame.js 468
            frame.js 520
            frame.js 562
            frame.js 411
            frame.js 568
            server.js 164
            server.js 168
TEST-PASS |  teardownModule

I think the Windows and Mac default sets are different? You'll need to have an if-then for this.

Comment 63

[Andreas Nilsson (:andreasn)]

(In reply to comment #61)

> I didn't realize it because the current migration assistant also has
> Thunderbird in it, but you should use &brandShortName; everywhere.

Fixed and made a bundle for Blake to merge. I should probably lobby for myself to get a hg account some more bug 546547

Comment 64

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

Attachment: Patch v5.

(In reply to comment #62)
> OK, couple of failures with the tests:
> 
> I think the Windows and Mac default sets are different? You'll need to have an
> if-then for this.

Yup.  Fixed.

(In reply to comment #63)
> > I didn't realize it because the current migration assistant also has
> > Thunderbird in it, but you should use &brandShortName; everywhere.
> Fixed and made a bundle for Blake to merge. I should probably lobby for myself
> to get a hg account some more bug 546547

Merged.

(In reply to comment #61)
> (From update of attachment 440102 [details] [diff] [review])
> on file: mail/locales/en-US/chrome/messenger/featureConfigurator.dtd line 26
> > <!-- LOCALIZATION NOTE (featureConfigurator.addOn.installed): The text to display after the user installs the addon from the configurator. -->
> > <!ENTITY featureConfigurator.addOn.installing "Installed">
> featureConfigurator.addOn.installed. Thanks to Standard8 for pointing this
> out!
> Also, Standard8 proposes inserting a hidden xul element using the string even
> if it doesn't get shown. "That and given the localization comment already
> present
> should at least help localisers pick up where the string will be and the
> appropriate contexts."

Fixed.

Also fixed was the bug you mentioned where we could mess up the TB2 profile if we switched back and forth in the TB3 migration assistant.


Carrying forward r=sid0 from the previous patch.

Comment 65

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

Attachment: Review fixes patch.

This patch fixes
1. (disabled buttons are also press-able), and
2. (the system integration sheet and the migration assistant show up at the same time.)

Thanks,
Blake.

Comment 66

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

Attachment: Review fixes patch, v2. Undoing the modal migration assistant.

Yeah, having the migration assistant as a modal dialog just caused too many problems, I think.

So here's a patch which just disables the buttons.

Thanks,
Blake.

Comment 67

[Rain [:rain1] (inactive)]

Comment on attachment 440304 [details] [diff] [review]
Review fixes patch, v2.  Undoing the modal migration assistant.

Please add a comment about why this is needed (the CSS wants the disabled attribute set to true).

The toolbar tests in the previous patch still need fixing on Windows and Linux -- Blake's currently working on fixing them.

There are a bunch of things that still need to be fixed post b2, and Tb2 to Tb3 upgrade tests need to land. Blake will file bugs for whatever needs to be done.

Comment 68

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

Comment on attachment 440304 [details] [diff] [review]
Review fixes patch, v2.  Undoing the modal migration assistant.

good good

Comment 69

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

Pushed as http://hg.mozilla.org/comm-central/rev/0ac24075232c

Comment 70

[Pavel Cvrcek [:JasnaPaka]]

Just one feedback from L10n point of view. In one step of this migration assistant I see some strings which were reused from old migration assistent from TB 3.0. You can see it on this screenshot:

http://www.jasnapaka.com/temp/thunderbird31-migration.png

Two strings (which are highlighted on screenshot above) were used on different place in old migration assistent. We ("cs" locale) should translate these strings in different way but translators cannot recognize that these strings should by changed. Not sure how many locales are affected but I talked with slovak locale leader ("sk") and he noticed the same thing.