Closed Bug 1434737 Opened 6 years ago Closed 6 years ago

Move to ChromeUtils.import()

Categories

(MailNews Core :: Backend, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 60.0

People

(Reporter: Fallen, Assigned: Fallen)

References

Details

Attachments

(5 files, 2 obsolete files)

Cu.import is no more, long live ChromeUtils.import(). Changing this also fixes the eslint checks.
Attached patch Seamonkey Part - v1 (obsolete) β€” β€” Splinter Review
Totally untested. Also removes XPCOMUtils import if there was no further occurrence in that file. This might break if other files rely on it being imported implicitly, but I'll leave that to you to check.
Attachment #8947252 - Flags: review?(frgrahl)
Attached patch IM/Chat - v1 β€” β€” Splinter Review
Attachment #8947261 - Flags: review?(florian)
Comment on attachment 8947261 [details] [diff] [review]
IM/Chat - v1

I have not tested this, but looks great by code inspection. Thanks!
Attachment #8947261 - Flags: review?(florian) → review+
(In reply to Philipp Kewisch [:Fallen]  from comment #0)
> Cu.import is no more, long live ChromeUtils.import().
Can you please clarify? When/where will this be withdrawn? The Mozmill part of your try is all orange, so something isn't right yet.
Not sure when it will be withdrawn, but I noticed due to the eslint rule. I'll look into the mozmill tests for calendar soon.

Note also the remaining patches are untested, and the try run is calendar only because I only later decided to go ahead with the other patches.

Esentially this is s/(Components.utils|Cu).import(/ChromeUtils.import(/g and s/XPCOMUtils.defineLazyModuleGetter/ChromeUtils.defineModuleGetter/g plus some indent changes and removing some imports. If you want to be on the safe side, we could keep the imports.
I likely fixed the ChromeUtils in Mozmill part with this try run for the calendar patch: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=949a144da49bdeed43bd6d14ecf1e0a8ca8bb362
Sorry, caught in bustage, now it's ready to go again.

> Note also the remaining patches are untested, and the try run is calendar only
> because I only later decided to go ahead with the other patches.
So can you please do a try for all patches.
Thanks saw the underlying change yesterday and thought we should follow. You beat me to it :)

> So can you please do a try for all patches.

SeaMonkey tests are busted. I will need to check the removed imports. Hope I manage this weekend.
What's the status here? Comment 8 suggests that you will update the patch, right?
Comment on attachment 8947252 [details] [diff] [review]
Seamonkey Part - v1

Need to be SeaMonkey instead of Seamonkey in comment but otherwise r+. Thanks

Let me check it in personally because it might need a rebase.

Next stop bug 767640 followup for c-c I assume :)
Attachment #8947252 - Flags: review?(frgrahl) → review+
Comment on attachment 8947242 [details] [diff] [review]
Calendar Part - v1

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

I just noticed that you only added two changes in mail in your last push, so codewise it looks good for calendar, r=me.
Attachment #8947242 - Flags: review?(makemyday) → review+
Here is a push that I believe will work, I forgot one more Sandbox change. I'm splitting the sandbox change out into a separate patch as well.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=86c0ef1d2d0f4f1eaa807bbaffa9e602380ef512
Feel free to delegate to e.g. Magnus if you don't want to review this.
Attachment #8948278 - Flags: review?(jorgk)
Attached patch Calendar Part - v2 β€” β€” Splinter Review
Attachment #8947242 - Attachment is obsolete: true
Attachment #8948279 - Flags: review+
Comment on attachment 8948278 [details] [diff] [review]
Mozmill Sandbox Changes - v1

Usually I refer all matters related to Mozmill to Aceman. I'm also happy to rs this as long as it works. I'll check the try later. Thanks for taking this bug!
Attachment #8948278 - Flags: review?(jorgk) → review?(acelists)
Comment on attachment 8948278 [details] [diff] [review]
Mozmill Sandbox Changes - v1

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

Thanks, the try run seems fine.
This only changes some sandbox calls. When do we migrate all normal Cu.import() calls throughout c-c?

Also, can we have the bug number from m-c where they decide on dropping Cu.import() ?
Attachment #8948278 - Flags: review?(acelists) → review+
(In reply to :aceman from comment #18)
> This only changes some sandbox calls. When do we migrate all normal
> Cu.import() calls throughout c-c?
That's done in all the other patches in the bug ;-)
Comment on attachment 8947262 [details] [diff] [review]
Mail/Common/Ldap/Editor - v1

Thanks. I'll land the whole lot now before it rots.
Attachment #8947262 - Flags: review?(jorgk) → review+
> Thanks. I'll land the whole lot now before it rots.

Let me checkin the SeaMonkey part. Will do it tomorrow. Suite building is broken and waiting for an r+ wich will bitrot the fix or the patch whatever is checked in first.
The rot set in already. This is the SM part rebased.

Philipp, the calendar part also doesn't apply. For example in one failed hunk I see:
this.mProperties = new cal.data.PropertyMap();
when the current code is:
this.mProperties = new cal.calPropertyBag();

That change happened a while ago, so I don't know what's going on there.
Attachment #8947252 - Attachment is obsolete: true
Attachment #8948285 - Flags: review+
Can you check in the parts separately? Why did Mozmill fail then?
Flags: needinfo?(philipp)
Ah, sorry. This was on top of bug 1433802 which I haven't sent an updated patch for. I've done that now, then you should be able to land everything correctly.
Flags: needinfo?(philipp)
Yep, I've just worked that out myself ;-)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1ac48db6177e
Move to ChromeUtils.import() - Chat part. r=florian
https://hg.mozilla.org/comm-central/rev/932a8eee8f8c
Move to ChromeUtils.import() - Mail/Common/Ldap/Editor part. r=jorgk
https://hg.mozilla.org/comm-central/rev/f4ec71bab936
Move to ChromeUtils.import() - Mozmill Sandbox changes. r=aceman
https://hg.mozilla.org/comm-central/rev/0250e2262e3c
Move to ChromeUtils.import() - Calendar part. r=MakeMyDay
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
The target should really be TB 60.
Target Milestone: --- → 6.2
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/87ca394913b1
Move to ChromeUtils.import() - SeaMonkey part. r=frg
Fallen, can we change the Product and Component of the bug as this now covered all of c-c not just calendar?
Component: Internal Components → Backend
Product: Calendar → MailNews Core
Target Milestone: 6.2 → Thunderbird 60.0
Version: Lightning 6.2 → Trunk
Depends on: 1438452
You need to log in before you can comment on or make changes to this bug.