1520643 - Port bug 1514594: Change all call sites of ChromeUtils.import() to the "new" scheme

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Jorg K (CEST = GMT+2)]

See bug 1514594 comment #13. Looks like a job for a sed script.

Comment 1

[Magnus Melin [:mkmelin]]

From that comment
"Since the two behaviors are mutually incompatible, this patch will land with a
scripted rewrite to update all existing callers to use the new model rather
than the old."

Kris, is there a script for this you can share?

Comment 2

[Tomislav Jovanovic :zombie]

The script is on bitbucket [1], though it seems fairly tied to code in m-c from my reading, and you could perhaps do with a simpler one (I wrote a 20-line script that successfully rewrote 99% of Web Extension code in m-c).

It largely depends on your situation (I'm not familiar with the amount and state of JS code in c-c), specifically:

• Do you use a lot of unexported symbols from JSM module's global?
• Do you have the modules.json which list explicitly exported symbols (used for eslint prior to this)?
• If not, could you easily generate one?

[1] https://bitbucket.org/kmaglione/m-c-rewrites/

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2c2d2cd9c181
No bug - Disable Nightly builds (bug 1520643, bug 1523384). a=jorgk DONTBUILD

Comment 4

[Geoff Lankow (:darktrojan)]

Attachment: 1520643-import-exports-1.diff

I've taken the automated script as far as it will go. I think all the remaining stuff is where we haven't declared which variables we want from the module.

There will be a whole lot of linting errors in this, due to calendar requiring spaces in curly brackets, and possibly other stuff. Also I'm suspicious of some of the places the script used const.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: [landed] 1520643-import-exports-1.diff - Calendar only

I've added spaces so we get const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm"); and avoid heaps of linting errors.

Now there are 458 linting errors left in calendar/:
Cu.import imports into variables and into global scope. mozilla/no-import-into-var-and-global (eslint)

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: [landed] 1520643-import-exports-non-cal.diff

Non-calendar part.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6e514874b326
Port bug 1514594: Change import call sites using kmag's script (calendar/). rs=bustage-fix,jorgk
https://hg.mozilla.org/comm-central/rev/985a367b6423
Port bug 1514594: Change import call sites using kmag's script. rs=bustage-fix,jorgk

Comment 8

[Jorg K (CEST = GMT+2)]

Not looking terribly good after that:

JavaScript error: chrome://chat/content/conversation-browser.js, line 181: ReferenceError: smileTextNode is not defined
JavaScript error: chrome://messenger/content/foldersummary.js, line 1: SyntaxError: redeclaration of const Services
JavaScript error: resource:///modules/gloda/datastore.js, line 3950: ReferenceError: GlodaAttributeDBDef is not defined
JavaScript error: chrome://messenger/content/messenger.xul, line 1: ReferenceError: OnLoadMessenger is not defined
JavaScript error: chrome://messenger/content/chat/chat-messenger.js, line 1129: ReferenceError: Notifications is not defined
JavaScript error: chrome://messenger/content/threadPane.js, line 352: ReferenceError: TreeOnMouseDown is not defined
JavaScript error: chrome://messenger/content/messenger.xul, line 1: ReferenceError: messagePaneOnResize is not defined

Comment 9

[Geoff Lankow (:darktrojan)]

Attachment: [landed] lint.diff

This cleans up the lint errors (read: actual errors) from the last round. Straight from my queue without a commit message because I'm tired.

Comment 10

[Geoff Lankow (:darktrojan)]

Attachment: [landed] imports.diff

This cleans up the obvious imports by pattern-matching, and some of the non-obvious ones by hand. I've skipped chat for now, and made it as far as the start of mailnews.

I'm afraid this is where it gets annoying. You'll have to figure out what's used from each import and list it. None of the remaining code is linted so it's not going to tell you if something's missing. :(

Comment 11

[Geoff Lankow (:darktrojan)]

I've basically given up using const for now, because of the redeclaration problem.

Comment 12

[Magnus Melin [:mkmelin]]

Going to look at this now.

Comment 13

[Jorg K (CEST = GMT+2)]

Wait, I'll do a try and I'll fix gloda. ETA: 10 min.

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: 1520643-gloda.patch

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: [landed] 1520643-gloda.patch (v1b)

Missed some. Try with Geoff's two patches + Gloda:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a34539a3f1cef1aa2eeff80c449c10c8a995ff33

Comment 16

[Magnus Melin [:mkmelin]]

Attachment: bug1520643_imports.patch (WIP)

Still some to go.

Comment 17

[Magnus Melin [:mkmelin]]

^^ is on top of the others

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fc28b70545a1
Port bug 1514594: Change import call sites manually to address linting errors. rs=bustage-fix,jorgk
https://hg.mozilla.org/comm-central/rev/b4291fd24315
Port bug 1514594: Change import call sites manually which were missed. rs=bustage-fix,jorgk
https://hg.mozilla.org/comm-central/rev/7f0addfafb82
Port bug 1514594: Change import call sites manually in Gloda. rs=bustage-fix DONTBUILD

Comment 19

[Jorg K (CEST = GMT+2)]

Magnus, why are you removing const {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm"); form files where Services is used, just taking FilterEditor.js at random.

Comment 20

[Jorg K (CEST = GMT+2)]

Looking through the landed patches, does this one make sense?
https://hg.mozilla.org/comm-central/rev/b4291fd243157744625ee9da830c2204416968c9#l81.12

We've mostly remove the import from the constructors, or even the entire constructor:
https://hg.mozilla.org/comm-central/rev/b4291fd243157744625ee9da830c2204416968c9#l14.49
That wasn't in Geoff's patch, I did this since the linter complained about it being unused, see
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a34539a3f1cef1aa2eeff80c449c10c8a995ff33

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: [landed] bug1520643_imports.patch

Restored the removal of services. Some mic. fixes. 3pane works now and other tabs, too.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1061eef33580
Port bug 1514594: correct previous changesets by changing 'const' to 'var', various misc. fixes. rs=bustage-fix,jorgk

Comment 23

[Magnus Melin [:mkmelin]]

Attachment: bug1520643_imports_v2auto.patch

A bunch of automated conversions. (Scripts are in the hg comment).

Comment 24

[Jorg K (CEST = GMT+2)]

Comment on attachment 9039919 [details] [diff] [review]
[landed] bug1520643_imports.patch

Sorry that landed.

Comment 25

[Magnus Melin [:mkmelin]]

Attachment: bug1520643_imports_v2auto.patch

My manual edits were accidentally merged to this same patch, so I'll just upload the complete patch now.

Still some work to do especially in chat.

Comment 26

[Jorg K (CEST = GMT+2)]

Comment on attachment 9039928 [details] [diff] [review]
bug1520643_imports_v2auto.patch

Sorry, I fixed your previous patch and landed it.

Yes, chat doesn't work still.

Also, some JS errors:
JavaScript error: about:preferences, line 1: ReferenceError: gAdvancedPane is not defined
JavaScript error: about:preferences, line 1: TypeError: previewObserver is undefined
JavaScript error: chrome://messenger/content/commandglue.js, ReferenceError: SetGetMsgButtonTooltip is not defined
JavaScript error: chrome://messenger/content/messageDisplay.js, ReferenceError: ClearPendingReadTimer is not defined
JavaScript error: chrome://messenger/content/messenger.xul, line 1: ReferenceError: goUpdateMailMenuItems is not defined
JavaScript error: chrome://messenger/content/msgMail3PaneWindow.js, ReferenceError: MsgGetMessagesForAllServers is not defined
JavaScript error: chrome://messenger/content/preferences/advanced.js, line 1: SyntaxError: redeclaration of var DownloadUtils
JavaScript error: chrome://messenger/content/preferences/messagestyle.js, line 25: TypeError: Services.core is undefined
JavaScript error: imConversations.js, line 22: ReferenceError: ClassInfo is not defined
JavaScript error: resource:///modules/gloda/index_msg.js, ReferenceError: IndexingJob is not defined
JavaScript strict warning: chrome://global/content/elements/menulist.js, ReferenceError: reference to undefined property "mSelectedInternal"
JavaScript strict warning: chrome://messenger/content/searchBar.js, line 58: ReferenceError: reference to undefined property "glodaSearchBox"
JavaScript strict warning: chrome://messenger/content/tabmail.xml, ReferenceError: reference to undefined property "moving"
ReferenceError: reference to undefined property "STATE_SECURE_HIGH"
imCore.js, line 39: ReferenceError: ClassInfo is not defined

Comment 27

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin [:mkmelin] from comment #25)

My manual edits were accidentally merged to this same patch, so I'll just upload the complete patch now.

Hmm, this wasn't teamwork at its best. I fixed your initial patch, and then some manual things to TB started at least. Can you repeat those manual edits?

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/82286364894a
Add missing import of BrowserUtils. rs=bustage-fix

Comment 29

[:aceman]

Attachment: 1520643.patch -3pane

Some more conversions that make 3-pane window show up without errors now.
Except chat where each fix uncovers even more problems and you need to enumerate all the imported symbols.
And one strange case
TypeError: can't redefine non-configurable property "BrowserUtils" in XPCOMUtils.jsm:316:7

Comment 30

[:aceman]

Attachment: 1520643-am.patch

Account manager.

Comment 31

[Jorg K (CEST = GMT+2)]

Comment on attachment 9039941 [details] [diff] [review]
1520643-am.patch

This looks OK, Geoff can also take a look.

Comment 32

[Jorg K (CEST = GMT+2)]

Comment on attachment 9039939 [details] [diff] [review]
1520643.patch -3pane

Hmm, not sure why FeedUtils.jsm can't keep the const.

I think Magnus had the chat hunks, but they got lost in action :-(

Comment 33

[Geoff Lankow (:darktrojan)]

Comment on attachment 9039939 [details] [diff] [review]
1520643.patch -3pane

The JSMs could keep their consts, and I don't know why you're killing ChromeUtils.defineModuleGetter, but it doesn't matter, you've done it now.

Comment 34

[:aceman]

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

Comment on attachment 9039939 [details] [diff] [review]
1520643.patch -3pane

The JSMs could keep their consts, and I don't know why you're killing

What? All are jsms and some couldn't stay const, so what do you mean?

ChromeUtils.defineModuleGetter, but it doesn't matter, you've done it now.

It seems to be the same as ChromeUtils.import() and also gave the redefining errors.

Comment 35

[:aceman]

If anybody has a script that can magically tell which ones should be const and which vars and what does not need to be converted and why then finally use it.
For some reason m-c can use 'const {Services}' but we can't. But those that remember know that we already went through this and had to convert most toplevel consts to vars due to some m-c change (and we also didn't have the time and script to magically check which ones can stay const). So I don't know why the wholesale conversion to 'const' was allowed to blindly land (and not reverted) and now everything is busted.

Comment 36

[Jorg K (CEST = GMT+2)]

We're saying that FeedUtils.jsm and three more "JSM" files in the patch could keep the const. The JS filed get var. I thought the ChromeUtils.defineModuleGetter is some "just in time" thing, so get it when needed, no?

Comment 37

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #35)

So I don't know why the wholesale conversion to 'const' was allowed to blindly land (and not reverted) and now everything is busted.

My fault. Geoff ran the script, I landed. Well, and instead of backing it out, we've been reverting it bit by bit :-(

Comment 38

[:aceman]

For reference, it was bug 1209777.

Comment 39

[:aceman]

Attachment: 1520643.patch -3pane v2

OK, if you mean keeping 'const' INSIDE of jsms, that seems to work too.

Comment 40

[:aceman]

Attachment: 1520643-chat.patch

Some start in /chat

Comment 41

[Geoff Lankow (:darktrojan)]

This try build is aceman's previous patches plus everything I've done today. I think it should pass almost everything unless I made some mistake in the most recent change. If it does I will push it to comm-central (currently busted by bug 1523846) and somebody else can take it from there.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&group_state=expanded&revision=65e9f2f02d105383efa2784e330aa9a1c17e44b5

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ca94d6b0a917
convert some more 'const' imports to 'var'. r=darktrojan
https://hg.mozilla.org/comm-central/rev/e7d070f75357
convert 'const' imports to 'var' in account manager. r=darktrojan
https://hg.mozilla.org/comm-central/rev/4dbbf88b2d37
Port bug 1514594: Change all call sites of ChromeUtils.import() to the "new" scheme; rs=bustage-fix DONTBUILD

Comment 43

[Magnus Melin [:mkmelin]]

Just to try to explain the situation: in modules const should be used, as this definition doesn't leak out to global scope in that case. In other .js files which we include many of into the window, there you can't use const for any common services, since then it will easily clash with a previous declaration, which is not allowed for const. Therefore we use var instead so that there wouldn't be a clash. When using var the last definition "wins". In that regard, I think it's questionable if it's really correct at all to have the var {Services} declarations. Perhaps it doesn't matter much and the js engine will just ignore as needed.

It's not that m-c can use const {Services} more than once either, but much more of their code is in modules (as opposed to just included .js files).

Comment 44

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+1) from comment #27)

Hmm, this wasn't teamwork at its best.

Indeed. I'm not sure why you spent time fixing up the patch when I wrote I had the replacement commands to re-do and fix it up.

Anyway, good to see this is mostly done. On a general level it's very welcome improvement IMO.

Comment 45

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin [:mkmelin] from comment #44)

Indeed. I'm not sure why you spent time fixing up the patch when I wrote I had the replacement commands to re-do and fix it up.

Anyway, good to see this is mostly done. On a general level it's very welcome improvement IMO.

Sadly, you never wrote that anywhere. I was begging for the "script" on IRC since it would have been far easier to re-run the script that to edit the patch and turn faulty removals into replacements.

In situations like this, communication of what people are doing is key, and there wasn't any. You attached the patch at 3 PM, didn't respond to any IRC questions at 20:45 although being online:
20:45:05 - jorgk: mkmelin: Hi, why do you remove all those {Services} = ...
20:49:24 - jorgk: mkmelin: Hi, why do you remove all those {Services} = ...
21:03:11 - mkmelin: jorgk: so far because just making the var didn't help. it's possible it's wrong.
21:23:30 - jorgk: mkmelin: do you have a script
21:23:42 - jorgk: removing the services is wrong
21:23:48 - jorgk: and editing the patch is a pain
21:24:07 - jorgk: making the services var instead of removing it helps
21:25:12 - mkmelin: it's mostly commands, so I can run them again once things work
21:26:13 - mkmelin: though I'm not sure. When we include many js files after one another, and declare e.g. Services in more than one, that can't be good either.
21:26:56 - mkmelin: if the file were modules, it would be ok, but as is everything is put into the global scope

Was I'm going to sit there and do nothing while you didn't not say anywhere what you did or were doing??

Magnus, please communicate better. We've done double work and ended up with an inferior solution of jumbling up the order or imports due to my editing of your WIP patch.

Comment 46

[Geoff Lankow (:darktrojan)]

Okay, that Try push doesn't look brilliant but it's a lot better than anything in the last 24ish hours.

Here's a summary of what's still broken:

XPCShell:
mailnews/db/gloda tests are busted by who-knows-what.
common/test/xpcshell/test_bootstrap.js is busted by bug 1523535

Mozmill:
All calendar tests broken by a busted datepicker (bug 1518932?)
composition tests … also with a menulist, I think
folder-display tests … I'm not sure, but I can't see any errors pointing to this bug
folder-widget/test-message-filters.js … there's a menulist, so I'm guessing that
junk-commands/test-junk-commands.js … same message as some of the folder-display tests "Timed out waiting for message display completion."
search-window/test-search-window.js … fixed
session-store/test-session-store.js … no idea
tabmail/test-tabmail-closing.js … same message as some of the folder-display tests "Timed out waiting for message display

Comment 47

[Jorg K (CEST = GMT+2)]

Comment on attachment 9040028 [details] [diff] [review]
1520643.patch -3pane v2

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

If it works, fine, I think it's preferable. Sadly the other patch already got landed, so I'll back that out :-(

Comment 48

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+1) from comment #45)

Was I'm going to sit there and do nothing while you didn't not say anywhere
what you did or were doing??

But you didn't ask for the scripts, just if I had a script [to generate]. I'd assumed you asked to send/attach them if you needed them. I suppose there's room for improvement in communication from both sides.

Comment 49

[Jorg K (CEST = GMT+2)]

Comment on attachment 9040031 [details] [diff] [review]
1520643-chat.patch

This doesn't apply any more after rev 4dbbf88b2d37. Looks like all exported symbols got imported wholesale which we will have to fix once the linter starts complaining about them.

Comment 50

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin [:mkmelin] from comment #48)

But you didn't ask for the scripts, just if I had a script [to generate]. I'd assumed you asked to send/attach them if you needed them. I suppose there's room for improvement in communication from both sides.

Magnus, you didn't communicate what you were doing and didn't answer questions. That's what IRC is for in such situation. It's only natural that other people are trying to fix the situation when they they see no other activities.

With your WIP you should already have said: Generated like so ... Then it would have been easy for anyone to rerun.

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2b4d619ce6e4
Revert changes to JSM files from rev ca94d6b0a917. a=backout

Comment 52

[Jorg K (CEST = GMT+2)]

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

XPCShell:
mailnews/db/gloda tests are busted by who-knows-what.

All failing with:
[JavaScript Warning: "ReferenceError: reference to undefined property "headerMessageID"" {file: "resource:///modules/gloda/index_msg.js" line: 2923}]

That line is: query.headerMessageID.apply(query, aMessageIDs);

I had no joy trying to work out where this is supposed to be set but isn't (any more?).

Comment 53

[Jorg K (CEST = GMT+2)]

This Gloda stuff is the weirdest thing. I dumped out the query, and there are all sort of things, but no headerMessageID. I wonder how that ever worked. headerMessageID has a getter in datamodel.js and that applies to a GlodaMessage object. No idea why that query object that got created via let query = Gloda.newQuery(Gloda.NOUN_MESSAGE, {... should have received that property.

Comment 54

[Jorg K (CEST = GMT+2)]

What is more, .apply is essentially a function call. headerMessageID looks like a scalar value to me. query at that point has many properties of type function, for example id. So I changed it to query.id.apply(query, aMessageIDs); and that runs past that line and then into problems since the correct query result is not returned.

Comment 55

[Jorg K (CEST = GMT+2)]

OK, here's a try run with M-C at d305772af471766618393c01065556e739 and C-C at 5e2e2c63c2cd when things were still working with some debug:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b2b4e7fbbd817ec6e91272062046430ec884c9a6

Comment 56

[Jorg K (CEST = GMT+2)]

That run didn't print anything, so here a new one with throw new Error(); at the end of one Gloda test. Let's see whether we get output this time:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=56844d654024325f2a7dce3c776bcdfb68dd41be

Comment 57

[Magnus Melin [:mkmelin]]

Seems query.headerMessageID is undefined now.

It used to be

query.headerMessageID=function(...aArgs) {
let constraint = [GlodaDatastore.kConstraintIn, aAttrDef, ...aArgs];
this._constraints.push(constraint);
return this;
}

Comment 58

[Jorg K (CEST = GMT+2)]

Yes, dumping out the object, there is no headerMessageID property. Where did you get the "used to be" value from? Some older build? My try did show some output now, but still not what I wanted to see. So thanks!

Comment 59

[Jorg K (CEST = GMT+2)]

OK, this is populated in
https://searchfox.org/comm-central/rev/738fdd7fcbeb646bbbad9f0ee69e7b1f35e41919/mailnews/db/gloda/modules/gloda.js#1526
and that's controlled by if (aSubjectNounDef.queryClass !== undefined) {. So I bet that thing is now undefined and the query constraint isn't constructed any more.

Comment 60

[Magnus Melin [:mkmelin]]

That's from an older build I have around yes. I'll take a further look soon now. Gloda sure like abstractions.

Comment 61

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/55636f848d82
Follow-up: Restore line overzealously removed in rev 7f0addfafb82 (found by darktrojan). r=darktrojan DONTBUILD

Comment 62

[Magnus Melin [:mkmelin]]

https://searchfox.org/comm-central/source/mailnews/db/gloda/modules/public.js#24

In the new world we're importing public.js as Gloda in some places, and GlodaIndexer as well as GlodaMsgIndexer. But now, we're also setting those up (as var), later, like at https://searchfox.org/comm-central/source/mailnews/db/gloda/test/unit/resources/glodaTestHelper.js#93.

But that indexer wouldn't be set up properly, and not have the expected state/characteristics. This might be the root of the gloda issues.

Comment 63

[Jorg K (CEST = GMT+2)]

Geoff has a few things going on try, sadly he didn't hand them over before disappearing into the night. I'll land one of them now.

Comment 64

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/708ef5e44efd
Port bug 1514594: Fix some imports with two arguments. rs=bustage-fix,jorgk

Comment 65

[Jorg K (CEST = GMT+2)]

Magnus, I browsed the patch before landing, so can you please also check whether something doesn't look right to you. The try run didn't appear to be better than one without the patch, in fact, worse with more tests failing, but I think the changes are still correct.

Unless more bustage came from the latest M-C merge, the X's should be green apart from bug 1524197.

Comment 66

[Jorg K (CEST = GMT+2)]

OK, a heap of bustage will come from
https://searchfox.org/comm-central/search?q=ChromeUtils.import%28%22chrome%3A%2F%2Fmozmill%2Fcontent%2Fmodules&path=

So stuff like:

var mozmill = {};
ChromeUtils.import("chrome://mozmill/content/modules/mozmill.js", mozmill);
var controller = {};
ChromeUtils.import("chrome://mozmill/content/modules/controller.js", controller);
var elib = {};
ChromeUtils.import("chrome://mozmill/content/modules/elementslib.js", elib);

I think I'll fix these with some sed.

Comment 67

[Kris Maglione [:kmag]]

(In reply to Jorg K (GMT+1) from comment #66)

OK, a heap of bustage will come from
https://searchfox.org/comm-central/search?q=ChromeUtils.import%28%22chrome%3A%2F%2Fmozmill%2Fcontent%2Fmodules&path=

So stuff like:

var mozmill = {};
ChromeUtils.import("chrome://mozmill/content/modules/mozmill.js", mozmill);
var controller = {};
ChromeUtils.import("chrome://mozmill/content/modules/controller.js", controller);
var elib = {};
ChromeUtils.import("chrome://mozmill/content/modules/elementslib.js", elib);

I think I'll fix these with some sed.

Those should continue to work.

Comment 68

[Jorg K (CEST = GMT+2)]

Attachment: 1520643-mozmill.patch

First part of the Mozmill patch, but apparently not needed.

Comment 69

[Jorg K (CEST = GMT+2)]

Attachment: 1520643-mozmill.patch

Full MozMill patch. So according to Kris, this won't make any difference :-(

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d9cb33f86a42cf75ea52ce1b65bb2e92eb57ba94

Comment 70

[:aceman]

Comment on attachment 9040532 [details] [diff] [review]
1520643-mozmill.patch

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

I want it :)

Comment 71

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5bc55a8b1780
Switch imports in Mozmill to "new" scheme. r=aceman

Comment 72

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/812a3ac8b6e5
Cache CIDs when registering mock objects to prevent NS_ERROR_FACTORY_NOT_REGISTERED; rs=bustage-fix
https://hg.mozilla.org/comm-central/rev/92d2f7730194
Fix missed import of fixIterator; rs=bustage-fix a=me

Comment 73

[:aceman]

Attachment: 1520643-source.patch

Fixes view source action and MAY fix mozmill/content-policy/test-view-source.js (I can't tell, it fails locally for unknown reason yet).

Comment 74

[alta88]

I believe this all has broken the feed subscribe dialog:
JavaScript error: chrome://messenger-newsblog/content/feed-subscriptions.js, line 651: ReferenceError: Feed is not defined

Comment 75

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7118b2ffae48
fix imports in viewSource.js. r=jorgk
https://hg.mozilla.org/comm-central/rev/63837a14a04d
Fix import for feed subscription. r+a=me DONTBUILD

Comment 76

[Jorg K (CEST = GMT+2)]

Thanks, Alta88, should be fixed now. So you dared to run TB in its current state ;-)

Comment 77

[:aceman]

Attachment: 1520643-constGloda.patch

Put back const imports in gloda files as those seem to actually be jsms just without proper file extension. So we agreed to use const in jsms and var imports elsewhere.
Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f6a62eca5bbe09cbe54d014252f2f59d35ec35b2

Comment 78

[Jorg K (CEST = GMT+2)]

Comment on attachment 9040581 [details] [diff] [review]
1520643-constGloda.patch

OK, I'd still prefer to remove the unneeded imports like I had it here:
https://hg.mozilla.org/comm-central/rev/4dbbf88b2d37d3347e73ba7c5f7c055d6d687994#l288.1
in all of mailnews/db/gloda/modules before the whole-sale replace too place. Up to you, but if we're polishing, we might as well.

Comment 79

[:aceman]

If you know which ones are unused, please remove them. Maybe eslint finds them for us one day.

Comment 80

[alta88]

(In reply to Jorg K (GMT+1) from comment #76)

Thanks, Alta88, should be fixed now. So you dared to run TB in its current state ;-)

heh, pulled a bit too soon..

Comment 81

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ede5d521aad1
Fix missing import in test-session-store.js; rs=bustage-fix a=me

Comment 82

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+1) from comment #65)

Magnus, I browsed the patch before landing, so can you please also check
whether something doesn't look right to you. The try run didn't appear to be

https://hg.mozilla.org/comm-central/rev/708ef5e44efd#l7.12 looks pretty weird. I'll do a patch for it.

Also the everybody.js file is funky, but maybe this didn't make things worse. I'd think the module loading into (formerly) global scope are unnecessary now https://hg.mozilla.org/comm-central/rev/708ef5e44efd#l9.52

Comment 83

[Jorg K (CEST = GMT+2)]

(In reply to Pulsebot from comment #75)

https://hg.mozilla.org/comm-central/rev/63837a14a04d
Fix import for feed subscription. r+a=me DONTBUILD

I've checked manually and all the other import sites
https://searchfox.org/comm-central/search?q=ChromeUtils.import(%22resource%3A%2F%2F%2Fmodules%2FFeedUtils.jsm%22)%3B&case=false&regexp=false&path=
only use FeedUtils. However, something is wrong in the linter, it should have seen me missing import in feed-subscriptions.js.

I'm closing this bug now, we can still land a follow-up for comment #82.

Comment 84

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/comm-central/rev/56f6fda8a274
Backed out changeset 2c2d2cd9c181 to re-enable Nightly builds (bug 1520643, bug 1523384). a=jorgk DONTBUILD

Comment 85

[Magnus Melin [:mkmelin]]

Attachment: bug1520643_gloda_cleanup.patch

Use normal importing in glautocomp.js. LOG was unused, so let's not import it either. gloda xpcshell tests pass locally

Comment 86

[Jorg K (CEST = GMT+2)]

Comment on attachment 9040637 [details] [diff] [review]
bug1520643_gloda_cleanup.patch

Let's find a more competent reviewer.

Comment 87

[Jorg K (CEST = GMT+2)]

Attachment: 1520643-constGloda.patch (v2)

Removed unneeded imports.

Comment 88

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/586599e38b9b
restore some 'const' imports in Gloda where possible and remove unneeded imports. r=jorgk

Comment 89

[Geoff Lankow (:darktrojan)]

I'm guessing the four added "dump" lines in that last patch were unintentional?

Comment 90

[Jorg K (CEST = GMT+2)]

Grrr, I saw those and forgot to mention them :-( - I'll fix it. Thanks for checking.

Comment 91

[:aceman]

Yes, sorry.

Comment 92

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/198dd3e78c9c
Follow-up: remove accidentally landed dump() calls. r=me DONTBUILD

Comment 93

[Geoff Lankow (:darktrojan)]

Comment on attachment 9040637 [details] [diff] [review]
bug1520643_gloda_cleanup.patch

I suspect there's a good reason those imports are not called until needed. With this patch they'd run when the component loads at startup, and I don't think we want that.

Comment 94

[Geoff Lankow (:darktrojan)]

Fixed some more redefinitions.

https://hg.mozilla.org/comm-central/rev/c910db1352fb91160026e4b2dc1f3281fabf4040

Comment 95

[Jorg K (CEST = GMT+2)]

We erroneously removed three dots here:
https://hg.mozilla.org/comm-central/rev/708ef5e44efd#l3.47

We'll add them back in bug 1497041 where we detected it.

Comment 96

[:aceman]

Attachment: 1520643-VFW.patch

Strangely, there is no virtualFolderWrapper exported from virtualFolderWrapper.js and you get an error about it when opening virtual folder properties dialog.

Comment 97

[Geoff Lankow (:darktrojan)]

Comment on attachment 9050217 [details] [diff] [review]
1520643-VFW.patch

Ugh, the script strikes again. I wish I hadn't bothered with it.

Comment 98

[Geoff Lankow (:darktrojan)]

I think perhaps it's time for a new bug when stuff like this is found.

Comment 99

[Jorg K (CEST = GMT+2)]

Certainly when we move to TB 68.

Comment 100

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c0209418ca41
remove reference to non-existent virtualFolderWrapper from import. r=darktrojan DONTBUILD

Comment 101

[Jorg K (CEST = GMT+2)]

Re. the suggested commit message: "remove non-existent virtualFolderWrapper reference"
Umm, the reference did exist, hence we removed it ;-)

Comment 102

[musiquegraeme]

I fixed my addon for this bug but still have the error
"Services.policies is undefined"

I fixed my own imports of modules including Services but this seems to be in ext-storage.js.

Comment 103

[:aceman]

I think policies do not exist in Thunderbird. Code using them should check if they exist, e.g. if (Services.policies && Services.policies.methodCall()). You can see examples of it at https://searchfox.org/comm-central/search?q=Services.policies+%26%26&case=false&regexp=false&path= .

Comment 104

[musiquegraeme]

The code that caused this error was
Thisdomain = await browser.storage.managed.get("domain");
It worked until TB67 came out. Because it refers to Services it suggested to me that this was caused by the change in calling modules but I can't look at ext-storage.js.

This is not to do with whether policies should be there but how managed storage works.

Comment 105

[Geoff Lankow (:darktrojan)]

I'm not sure why you're trying to use browser.storage.managed in Thunderbird, but it doesn't work and isn't meant to work. And this is certainly the wrong bug for it.

Comment 106

[musiquegraeme]

It worked until this week. I don't see why it shouldn't work. It broke at exactly the same time that this bug broke my addon. Can you prove that everything in ext-storage.js was completely fixed in relation to the import of modules?

I originally wrote to maildev and was told by Magnus that I was refering to this bug.

Comment 107

[Kris Maglione [:kmag]]

The error you're seeing clearly has nothing to do with this bug.