1493513 - Clean up mail migrator

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Jorg K (CEST = GMT+2)]

+++ This bug was initially created as a clone of Bug #1371898 +++

A new profile is created with 'mail.ui-rdf.version' at 0 and then runs through all the upgrade steps for no good reason.

Note that in step "currentUIVersion < 5" the menubar is hidden depending on pref mail.main_menu.collapse_by_default. We have two tests that
- ensure the menubar is hidden in a new profile
  (startup-firstrun/test-menubar-collapsed.js)
- ensure the menubar is NOT hidden in a new profile if the pref is false
  (override-main-menu-collapse/test-override-mainmenu-collapse.js)

To clean up the migrator it would be useful
- not to run all steps on a new profile (if that can be detected somehow,
  note that our forebears didn't have a good idea).
- run the "currentUIVersion < 5" outside the upgrade
- remove steps < v17, see bug 1371898 comment #37.

Comment 1

[Jorg K (CEST = GMT+2)]

Magnus said in bug 1371898 comment #39: "I like the pref". So that shouldn't be removed although you really need a user.js trick to set it to become active on a *new* profile.

Comment 2

[Magnus Melin [:mkmelin]]

I'm sure there are many organizations that set up some defaults for their users.

Comment 3

[:aceman]

TB17 is at UI version 5 (https://dxr.mozilla.org/comm-esr17/source/mail/base/modules/mailMigrator.js#103) so it seems we already killed exactly those older versions.

Comment 4

[:aceman]

Attachment: 1493513.patch

This should do it.
If UI version in pref is 0, it is a new profile, skip all migration except the 2 blocks we identified via tests (main menu hiding and expanding of "All Addressbooks" node). Existing accounts will have a different UI version in pref and will run through migration as usual.

There are a few changes to tests because the fix to properly hide main menu not basing it on zero accounts in the profile fixes also mozmill to run without the main menu. Previously it had the menu shown as it was not considered a new profile due to its predefined test accounts.

So some tests that operated the main menu failed and I update them to use the app menu.

There are many other tests clicking items on the main menu, but that somehow does not fail yet (but may in the future). Those tests may need to be gradually migrated to appmenu.

Or do we show the menu for mozmill? The default the user sees in a new profile is without main menu, so that is probably what we should test.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=517dd2efe5086c53449483fb406f1aad77c1a651

Comment 5

[Jorg K (CEST = GMT+2)]

Comment on attachment 9011329 [details] [diff] [review]
1493513.patch

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

This looks fine and we've discussed all of it on IRC already. To avoid the MozMill changes and to avoid having MozMill artificially click on a non-visible menu via, for example |mc.click(new elib.Elem(mc.menus.menu_File.menu_New.newMailAccountMenuItem));|, we could just exempt MozMill from the menubar collapsing as indicated below.

Either way is fine by me, but let's hear what Magnus thinks.

::: mail/base/modules/mailMigrator.js
@@ +117,5 @@
> +      // This is a new profile.
> +      newProfile = true;
> +
> +      // Collapse the main menu by default if the override pref
> +      // "mail.main_menu.collapse_by_default" is set to true.

Here we could do:

// For MozMill we require the menu and we detect this since there are predefined accounts.

@@ +118,5 @@
> +      newProfile = true;
> +
> +      // Collapse the main menu by default if the override pref
> +      // "mail.main_menu.collapse_by_default" is set to true.
> +      if (Services.prefs.getBoolPref("mail.main_menu.collapse_by_default")) {

&& MailServices.accounts.accounts.length == 0

Comment 6

[:aceman]

(In reply to Jorg K (GMT+2) from comment #5)
> > +      if (Services.prefs.getBoolPref("mail.main_menu.collapse_by_default")) {
> 
> && MailServices.accounts.accounts.length == 0

Companies may pre-define accounts for new profiles and we may still want them to have the default hidden menu bar.

We should be able to detect presence of mozmill extension properly (like ExtensionSupport.loadedLegacyExtensions.has("[e-mail]")).

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9011329 [details] [diff] [review]
1493513.patch

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

::: mail/base/modules/mailMigrator.js
@@ +112,5 @@
>        return;
>  
>      let xulStore = Services.xulStore;
>  
> +    if (currentUIVersion == 0) {

I think it would be clearer if you just moved let newProfile down before the if like

let newProfile = (currentUIVersion == 0);

::: mail/test/mozmill/shared-modules/test-folder-display-helpers.js
@@ +2856,5 @@
> +function toggle_main_menu(aEnabled = true) {
> +  let menubar = mc.e("mail-toolbar-menubar2");
> +  let state = menubar.getAttribute("autohide") != "true";
> +  menubar.setAttribute("autohide", !aEnabled);
> +  mc.sleep(0);

Needed?
If needed I think you need to really wait for it to be visible

Comment 8

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #6)
> We should be able to detect presence of mozmill extension properly (like
> ExtensionSupport.loadedLegacyExtensions.has("[e-mail]")).

Let's not detect automation stuff in the main code.

Comment 9

[:aceman]

(In reply to Magnus Melin [:mkmelin] from comment #7)
> ::: mail/test/mozmill/shared-modules/test-folder-display-helpers.js
> @@ +2856,5 @@
> > +function toggle_main_menu(aEnabled = true) {
> > +  let menubar = mc.e("mail-toolbar-menubar2");
> > +  let state = menubar.getAttribute("autohide") != "true";
> > +  menubar.setAttribute("autohide", !aEnabled);
> > +  mc.sleep(0);
> 
> Needed?
> If needed I think you need to really wait for it to be visible

I think it was better with the sleep, in a run that has shown and immediatelly hidden the toolbar, the toolbar appeared with sleep, but didn't without the sleep. I mean it was working, but wasn't rendered for the split second it should be visible.

(In reply to Magnus Melin [:mkmelin] from comment #8)
> (In reply to :aceman from comment #6)
> > We should be able to detect presence of mozmill extension properly (like
> > ExtensionSupport.loadedLegacyExtensions.has("[e-mail]")).
> 
> Let's not detect automation stuff in the main code.

OK. We could also set the prefs specially in the mozmill profile instead of having such code in the main code. So I agree.
But if you agree with the current patch, we don't need to bother further.

Comment 10

[Jorg K (CEST = GMT+2)]

I much prefer to use that pref for MozMill if possible rather than doing those phantom clicks.

Comment 11

[:aceman]

Attachment: 1493513.patch v1.1

OK, this explicitly forces the menu bar shown for mozmill so the tests can safely click it. Except for the tests that play with the menubar and want it at the default value or set their own.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=201794273&revision=6b5902541af15626acc991f655e9c38308bb6cb9

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 9012398 [details] [diff] [review]
1493513.patch v1.1

Thanks, better this way. I've checked the interdiff:
https://bugzilla.mozilla.org/attachment.cgi?oldid=9011329&action=interdiff&newid=9012398&headers=1

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/60dfc2e2e18c
clean up migration of UI features. r=jorgk,mkmelin