Open Bug 1617887 Opened 6 months ago Updated 1 month ago

dismantle mozmill.jsm and friends

Categories

(Thunderbird :: Testing Infrastructure, task)

task
Not set
normal

Tracking

(Not tracked)

REOPENED
Thunderbird 75.0

People

(Reporter: mkmelin, Assigned: mkmelin)

Details

(Keywords: leave-open)

Attachments

(17 files, 1 obsolete file)

5.60 KB, patch
khushil324
: review+
Details | Diff | Splinter Review
7.43 KB, patch
khushil324
: review+
Details | Diff | Splinter Review
4.33 KB, patch
khushil324
: review+
Details | Diff | Splinter Review
10.85 KB, patch
khushil324
: review+
Details | Diff | Splinter Review
3.06 KB, patch
khushil324
: review+
Details | Diff | Splinter Review
5.25 KB, patch
khushil324
: review+
Details | Diff | Splinter Review
3.30 KB, patch
khushil324
: review+
Details | Diff | Splinter Review
7.80 KB, patch
khushil324
: review+
Details | Diff | Splinter Review
11.41 KB, patch
khushil324
: review+
Details | Diff | Splinter Review
5.71 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
17.29 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
22.35 KB, patch
khushil324
: review+
Details | Diff | Splinter Review
31.58 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
3.81 KB, patch
khushil324
: review+
Details | Diff | Splinter Review
11.44 KB, patch
khushil324
: review+
Details | Diff | Splinter Review
1.33 KB, patch
khushil324
: review+
Details | Diff | Splinter Review
2.89 KB, patch
khushil324
: review+
Details | Diff | Splinter Review

Inspired by the recent bustage, I notices mozmill.jsm contains mostly junk nowadays. Most of the stuff isn't used, and it even exports some things that do not exist :/
Time to clean out.

Attachment #9129399 - Flags: review?(khushil324)
Attachment #9129400 - Flags: review?(khushil324)
Attachment #9129401 - Flags: review?(khushil324)
Attachment #9129402 - Flags: review?(khushil324)
Attachment #9129403 - Flags: review?(khushil324)
Attachment #9129404 - Flags: review?(khushil324)
Attachment #9129405 - Flags: review?(khushil324)
Attachment #9129406 - Flags: review?(khushil324)

Successful tryrun (have fixed the linting after) - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=039ed6d33065f320334af488e647d4493bcfc484&selectedJob=290575559

It's almost entirely removing unused things.

Attachment #9129408 - Flags: review?(khushil324)
Comment on attachment 9129399 [details] [diff] [review]
bug1617887_dismantle_mozmil.part1.patch

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

Everything else looks good. But we are using "platform" export here: https://searchfox.org/comm-central/source/mail/test/browser/shared-modules/frame.jsm#559
Is this of any use?
Attachment #9129399 - Flags: review?(khushil324)
Comment on attachment 9129400 [details] [diff] [review]
bug1617887.part2.patch

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

Looks good. r=khushil. One question here. Can we replace all the controller i.e. mozmill.getMail3PaneController(); with mc? It will make things less confusing.
Attachment #9129400 - Flags: review?(khushil324) → review+
Comment on attachment 9129401 [details] [diff] [review]
bug1617887.part3.patch

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

Looks good to me. r=khushil.
Attachment #9129401 - Flags: review?(khushil324) → review+
Comment on attachment 9129402 [details] [diff] [review]
bug1617887.part4.patch

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

Looks good to me. r=khushil.
Attachment #9129402 - Flags: review?(khushil324) → review+
Comment on attachment 9129403 [details] [diff] [review]
bug1617887.part5.patch

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

Looks good to me. r=khushil
Attachment #9129403 - Flags: review?(khushil324) → review+
Comment on attachment 9129404 [details] [diff] [review]
bug1617887.part6.patch

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

Looks good to me. r=khushil
Attachment #9129404 - Flags: review?(khushil324) → review+
Comment on attachment 9129405 [details] [diff] [review]
bug1617887.part7.patch

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

Looks good to me. r=khushil
Attachment #9129405 - Flags: review?(khushil324) → review+
Comment on attachment 9129406 [details] [diff] [review]
bug1617887.part8.patch

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

Looks good to me. r=khushil
Attachment #9129406 - Flags: review?(khushil324) → review+
Comment on attachment 9129408 [details] [diff] [review]
bug1617887.part9.patch

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

Looks good to me. r=khushil
Attachment #9129408 - Flags: review?(khushil324) → review+

Got rid of mozmill.platform calls. Also force_skip and EXCLUDED_PLATFORMS which we don't use in Mochitests.

Just sent to try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=32f7b87d548d426ff36474f9a642913c2c19bdd9

Attachment #9129399 - Attachment is obsolete: true
Attachment #9131446 - Flags: review?(khushil324)
Comment on attachment 9131446 [details] [diff] [review]
bug1617887_dismantle_mozmil.part1.patch

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

Looks good to me. r=khushil.
Attachment #9131446 - Flags: review?(khushil324) → review+

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/bbcd9eee9226
part1 - remove unsed mozmill exports. r=khushil
https://hg.mozilla.org/comm-central/rev/d0a04e52dbd5
part2 - remove unnecessary getMail3PaneController. r=khushil
https://hg.mozilla.org/comm-central/rev/d5f2f34c0d01
part3 - stop exporting mozmill.controller. r=khushil
https://hg.mozilla.org/comm-central/rev/db70bd44f6e2
part4 - stop exporting mozmill.utils. r=khushil
https://hg.mozilla.org/comm-central/rev/fb950872cc0f
part5 - inline mozmill init.jsm. r=khushil
https://hg.mozilla.org/comm-central/rev/163fe0f39164
part6 - remove unused dom.jsm. r=khushil
https://hg.mozilla.org/comm-central/rev/89e4f92cad8f
part6 - inline strings.jsm usage. r=khushil
https://hg.mozilla.org/comm-central/rev/71e02e54a7f3
part8 - remove unused arrays.jsm. r=khushil
https://hg.mozilla.org/comm-central/rev/9979881cf01c
part9 - remove jum.jsm usage. r=khushil DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/50fa3d8d13ff
part1 followup - assign module.mozmill late. rs=bustage-fix

Leaving open for some more removals.

Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Target Milestone: --- → Thunderbird 75.0
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/22c665f600c7
part 1 follow-up - Fix tests that depend on isMac or isWindows. rs=bustage-fix

Perhaps not strictly a mozmill one... but this one can also go.

Attachment #9143448 - Flags: review?(geoff)
Attachment #9143448 - Flags: review?(geoff) → review+
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/9f5657fd0e4e
get rid of MessageHelpers.jsm. r=darktrojan

This is a but yucky. I don't know why CI uses a different path than local runs. Anyway...

Attachment #9145072 - Flags: review?(geoff)
Attachment #9145073 - Flags: review?(khushil324)
Comment on attachment 9145073 [details] [diff] [review]
bug1617887_less_osjsm.patch

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

Looks good to me. r=khushil.
Attachment #9145073 - Flags: review?(khushil324) → review+
Comment on attachment 9145072 [details] [diff] [review]
bug1617887_rm_load_via_src_path.patch

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

We could probably take this improvement further, but it's a good start. I think the use of the term "module" is misleading (these days, it probably made sense at the time) and should be replaced with something like "scope".

::: mail/test/browser/shared-modules/FolderDisplayHelpers.jsm
@@ +174,5 @@
>  var { MailUtils } = ChromeUtils.import("resource:///modules/MailUtils.jsm");
>  var { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
> +var { MessageGenerator, MessageScenarioFactory } = ChromeUtils.import(
> +  "resource://testing-common/mailnews/MessageGenerator.jsm"
> +);

It didn't occur to me when I first converted all of this stuff, but we should use ChromeUtils.defineModuleGetter instead of ChromeUtils.import within the helper modules, so that we don't load a lot of things we don't need for every test.

@@ +3383,4 @@
>  
>  /**
> + * Load a file in its own 'module' based on the effective location of the staged
> + * the FolderDisplayHelpers.jsm module

There's one too many "the"s in this sentence, and it's missing the full stop.
Attachment #9145072 - Flags: review?(geoff) → review+
Comment on attachment 9145075 [details] [diff] [review]
bug1617887_rm_runner.patch

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

::: mail/test/browser/shared-modules/frame.jsm
@@ +40,5 @@
>  var arrayRemove = function(array, from, to) {
>    var rest = array.slice((to || from) + 1 || array.length);
>    array.length = from < 0 ? array.length + from : from;
>    return array.push.apply(array, rest);
>  };

It's probably about time we used .splice!
Attachment #9145075 - Flags: review?(geoff) → review+

(In reply to Geoff Lankow (:darktrojan) from comment #31)

It didn't occur to me when I first converted all of this stuff, but we
should use ChromeUtils.defineModuleGetter instead of ChromeUtils.import
within the helper modules, so that we don't load a lot of things we don't
need for every test.

Will save this for later. There's a lot of things to clean up yet. Taking things out one at a time helps seeing what's left.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/89648b88a0e3
Rework load_via_src_path. r=darktrojan
https://hg.mozilla.org/comm-central/rev/ad8b33dcd11e
remove unused (mozmill) os.jsm imports. r=khushil
https://hg.mozilla.org/comm-central/rev/6e0dee99d40a
remove Runner and other things from frame.jsm. r=darktrojan DONTBUILD

The mozmill os.jsm is now unused and can be removed.

Attachment #9146734 - Flags: review?(khushil324)
Comment on attachment 9146734 [details] [diff] [review]
bug1617887_rm_osjsm.patch

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

Looks good to me. r=khushil
Attachment #9146734 - Flags: review?(khushil324) → review+
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/92b5978bce31
remove now unused mozmill os.jsm. r=khushil
Comment on attachment 9155742 [details] [diff] [review]
bug1617887_utils_export_less.patch

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

Looks good to me. r=khushil

::: mail/test/browser/shared-modules/controller.jsm
@@ -39,5 @@
>  // ***** END LICENSE BLOCK *****
>  
> -var EXPORTED_SYMBOLS = [
> -  "MozMillController",
> -  "MozMillAsyncTest",

MozMillAsyncTest related functions can be removed now.
Attachment #9155742 - Flags: review?(khushil324) → review+
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/c6b42eb37520
remove exports never used from utils.jsm. r=khushil DONTBUILD
Attachment #9156517 - Flags: review?(khushil324)
Comment on attachment 9156517 [details] [diff] [review]
bug1617887_rm_MozMillAsyncTest.patch

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

Looks good to me.
Attachment #9156517 - Flags: review?(khushil324) → review+
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/d204dd91ce6a
remove MozMillAsyncTest. r=khushil DONTBUILD
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/5b03f99338e2
followup to fix linting. rs=eslint DONTBUILD
Comment on attachment 9160284 [details] [diff] [review]
bug1617887_rm_controllerAdditions.patch

Look good to me. r=khushil.
We can also remove this comment: https://searchfox.org/comm-central/source/mail/test/browser/shared-modules/WindowHelpers.jsm#1740
Attachment #9160284 - Attachment description: bug1617887_rm_controllerAdditions.path → bug1617887_rm_controllerAdditions.patch
Attachment #9160284 - Attachment filename: bug1617887_rm_controllerAdditions.path → bug1617887_rm_controllerAdditions.patch
Attachment #9160284 - Attachment is patch: true
Attachment #9160284 - Attachment mime type: text/x-systemd-unit → text/plain
Attachment #9160284 - Flags: review?(khushil324) → review+
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/ee5eb95ccf70
remove controllerAdditions and friends. r=khushil
You need to log in before you can comment on or make changes to this bug.