Port bug 1456035: Change XPCOMUtils.generateQI to ChromeUtils.generateQI in (mail, mailnew and chat) and convert QueryInterface(aIID)

RESOLVED FIXED in Thunderbird 62.0

Status

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: jorgk, Assigned: jorgk)

Tracking

Trunk
Thunderbird 62.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 9 obsolete attachments)

3.60 KB, patch
aceman
: review+
Fallen
: review+
Details | Diff | Splinter Review
6.68 KB, patch
frg
: review+
Details | Diff | Splinter Review
51.47 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
1.34 KB, patch
aceman
: review+
Details | Diff | Splinter Review
63.65 KB, patch
aceman
: review+
Details | Diff | Splinter Review
66.83 KB, patch
aceman
: review+
Details | Diff | Splinter Review
35.02 KB, patch
aceman
: review+
Details | Diff | Splinter Review
See
https://hg.mozilla.org/mozilla-central/rev/c58f0b4dd849
https://hg.mozilla.org/mozilla-central/rev/8db81ce4d3a2

Also, having Ci.nsISupports will assert, making debug runs real red:
Assertion failure: !ifaces.Contains((nsISupports::COMTypeInfo<nsISupports, void>::kIID), IIDComparator()), at /builds/worker/workspace/build/src/dom/base/MozQueryInterface.cpp:101

One appears to be related to
TEST-UNEXPECTED-FAIL | comm/mailnews/jsaccount/test/unit/test_componentsExist.js | xpcshell return code: 1
but all Mozmill runs also failed on debug.
Attachment #8971814 - Flags: review?(acelists)
Comment on attachment 8971814 [details] [diff] [review]
1457713-remove-nsISupports.patch

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

Thanks. But we could do the optimization of QueryInterface implementations (per the m-c patch) too in the next patch. I'll look at it.
Attachment #8971814 - Flags: review?(acelists) → review+
I sadly missed half of it :-(
Attachment #8971814 - Attachment is obsolete: true
Attachment #8971827 - Flags: review?(acelists)
Comment on attachment 8971827 [details] [diff] [review]
1457713-remove-nsISupports.patch

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

::: chat/content/browserRequest.js
@@ -27,5 @@
>  
>    QueryInterface: function(aIID) {
> -    if (aIID.equals(Ci.nsIWebProgressListener)   ||
> -        aIID.equals(Ci.nsISupportsWeakReference) ||
> -        aIID.equals(Ci.nsISupports))

This is wrong. The reason we removed nsISupports from the generateQI calls is because it's implicitly included, and having an extra copy is bad for code style and performance. In manually-generated implementations, it should still be included (though C++ will never call a JS QueryInterface method for nsISupports).
Attachment #8971827 - Flags: review-
Comment on attachment 8971828 [details] [diff] [review]
1457713-fix-test.patch

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

Kris, what about this?
Attachment #8971828 - Flags: feedback?(kmaglione+bmo)
Comment on attachment 8971828 [details] [diff] [review]
1457713-fix-test.patch

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

This looks fine, but the test_componentsExist changes are probably unnecessary, and it seems incomplete:

https://dxr.mozilla.org/comm-central/search?q=regexp%3A%5B%5E(%5DCi%5C.nsISupports%5Cb%5B%5E.%3B)%5Cs%5D+-path%3Amozilla%2F&redirect=false

You're probably better off just starting with running the script in comment 2, and then cleaning up the nsISupports references it misses, where they're not passed as immediate arguments to generateQI.
Attachment #8971828 - Flags: feedback?(kmaglione+bmo) → feedback+
Posted patch 1457713-QI.patch (obsolete) — Splinter Review
OK, here I'm only removing Ci.nsISupports from calls to generateQI().
The changes in test_componentsExist.js are necessary or the test will fail.

Kris, how does one run your script?
Attachment #8971827 - Attachment is obsolete: true
Attachment #8971828 - Attachment is obsolete: true
Attachment #8971827 - Flags: review?(acelists)
Attachment #8971828 - Flags: review?(acelists)
Posted patch 1457713-QI.patch (v2) (obsolete) — Splinter Review
Actually, test_componentsExist.js only needed one small change.
Attachment #8971833 - Attachment is obsolete: true
(In reply to Jorg K (GMT+1) from comment #10)
> Created attachment 8971833 [details] [diff] [review]
> 1457713-QI.patch
> 
> OK, here I'm only removing Ci.nsISupports from calls to generateQI().
> The changes in test_componentsExist.js are necessary or the test will fail.
> 
> Kris, how does one run your script?

Huh. I thought I had a README...

Basically:

  %cd /path/to/comm-central
  %sh /path/to/m-c-rewrites/run-replacer.sh chromeutils-generateQI

You might want to add "mozilla/" to the list of exclusions in chromeutils-generateQI.jsm first, though.
Keywords: leave-open
OK, let's not mess with the test, just address the problem at hand.
Attachment #8971834 - Attachment is obsolete: true
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/e51241c644f8
Port bug 1456035: Remove nsISupports from calls to XPCOMUtils.generateQI(). rs=bustage-fix
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #9)
> This looks fine, but the test_componentsExist changes are probably
> unnecessary, and it seems incomplete:
> https://dxr.mozilla.org/comm-central/search?q=regexp%3A%5B%5E(%5DCi%5C.
> nsISupports%5Cb%5B%5E.%3B)%5Cs%5D+-path%3Amozilla%2F&redirect=false

Thanks for the feedback.

For now, I've removed Ci.nsISupports from calls to generateQI(). There was no doubt that this would now assert.

That JS Account stuff is just mysterious, again, I discarded the test changes and just fixed the problem at hand.

https://dxr.mozilla.org/comm-central/search?q=regexp%3A%5B%5E(%5DCi%5C.nsISupports%5Cb%5B%5E.%3B)%5Cs%5D+-path%3Amozilla%2F&redirect=false
As for being incomplete: I don't know.

For example, I can't tell what that code does with the interfaces:
https://dxr.mozilla.org/comm-central/rev/8d636ad5810b5586433294c16cb2376d826fcca8/chat/components/src/imContacts.js#245
  getInterfaces: function(countRef) {
    let interfaces = [Ci.nsIClassInfo, Ci.nsISupports, Ci.imITag];
    countRef.value = interfaces.length;
    return interfaces;
  },
  getHelperForLanguage: language => null,
  flags: 0,
  QueryInterface: XPCOMUtils.generateQI([Ci.imITag, Ci.nsIClassInfo])
It doesn't use 'interfaces' to pass into generateQI().

We'll run your script in peace once the immediate bustage is solved, hopefully now.
Attachment #8971837 - Flags: review+
Comment on attachment 8971837 [details] [diff] [review]
1457713-QI.patch (v3) [landed comment #14]

Philipp, you want to see all Calendar changes, so here you go.
Attachment #8971837 - Flags: review?(philipp)
This should cover suite/
Attachment #8971855 - Flags: review?(frgrahl)
Attachment #8971859 - Flags: review?(philipp)
Comment on attachment 8971859 [details] [diff] [review]
1457713-cal.patch QI rewrite - calendar

># HG changeset patch
># User aceman <acelists@atlas.sk>
># Parent  59fcdd0ef581a44c954ff0722f1914b41aa2d058
>Bug 1457713 - rewrite QueryInterface JS implementations the same way as m-c bug 1456035 does - calendar. r?Fallen
>
[...]
>diff --git a/calendar/base/src/calCachedCalendar.js b/calendar/base/src/calCachedCalendar.js
>--- a/calendar/base/src/calCachedCalendar.js
>+++ b/calendar/base/src/calCachedCalendar.js
[...]
>@@ -224,17 +221,17 @@ calCachedCalendar.prototype = {
>             Components.utils.reportError(exc);
>         }
>     },
> 
>     getOfflineAddedItems: function(callbackFunc) {
>         let self = this;
>         self.offlineCachedItems = {};
>         let getListener = {
>-            QueryInterface: XPCOMUtils.generateQI([Components.interfaces.calIOperationListener]),
>+            QueryInterface: ChromeUtilsgenerateQI(["calIOperationListener"]),
>             onGetResult: function(aCalendar, aStatus, aItemType, aDetail, aCount, aItems) {
>                 for (let item of aItems) {
>                     self.offlineCachedItems[item.hashId] = item;
>                     self.offlineCachedItemFlags[item.hashId] = cICL.OFFLINE_FLAG_CREATED_RECORD;
>                 }
>             },
> 
>             onOperationComplete: function(aCalendar, aStatus, aOpType, aId, aDetail) {

It should be "ChromeUtils.generateQI" instead of "ChromeUtilsgenerateQI".
Thanks, of course:)
Attachment #8971859 - Attachment is obsolete: true
Attachment #8971859 - Flags: review?(philipp)
Attachment #8971861 - Flags: review?(philipp)
Comment on attachment 8971860 [details] [diff] [review]
1457713-mail patch QI rewrite - TB [landed comment #25]

Thanks.
Attachment #8971860 - Flags: review?(jorgk) → review+
Comment on attachment 8971855 [details] [diff] [review]
1457713-suite.patch [landed comment #24]

Looks good. Thanks. SeaMonkey needs some additional rewrites as in the cal patches. I will put these in a followup bug.
Attachment #8971855 - Flags: review?(frgrahl) → review+
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/3a02fb80639b
Port bug 1456035 [Remove nsISupports from calls to XPCOMUtils.generateQI()] suite/ part. r=frg DONTBUILD CLOSED TREE
Attachment #8971837 - Attachment description: 1457713-QI.patch (v3) → 1457713-QI.patch (v3) [landed comment #14]
Attachment #8971855 - Attachment description: 1457713-suite.patch → 1457713-suite.patch [landed comment #24]
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/46c62a2fcc9d
rewrite QueryInterface JS implementations the same way as M-C bug 1456035 does - chat/ mail/ mailnews/. r=jorgk
Attachment #8971860 - Attachment description: 1457713-cal.patch QI rewrite - TB → 1457713-mail patch QI rewrite - TB [landed comment #25]
Attachment #8971837 - Flags: review?(philipp) → review+
Comment on attachment 8971861 [details] [diff] [review]
1457713-cal.patch QI rewrite - calendar v1.1

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

Thanks for the patches! calendar uses .generateQI([Ci.calITheInterface] instead of .generateQI(["calITheInterface"]), I'd appreciate if you could change this throughout the patch. r+ with that changed.
Attachment #8971861 - Flags: review?(philipp) → review+
Hmm, the ones I can see are all like:
  QueryInterface: XPCOMUtils.generateQI(calTimezoneInterfaces), ...
OK, then we can drop most of the patch.
There are still some manual 'QueryInterface: function(aIID) {}' remaining but those are implemented specially (call some more functions internally) so I didn't replace those.
Assignee: nobody → acelists
Attachment #8971861 - Attachment is obsolete: true
Attachment #8972276 - Flags: review+
Aceman, is there going to be a patch to change XPCOMUtils.generateQI to ChromeUtils.generateQI?

I don't understand why you've already changed some of those in the Calendar patch and now you've dropped those hunks again.
(In reply to Jorg K (GMT+1) from comment #29)
> Aceman, is there going to be a patch to change XPCOMUtils.generateQI to
> ChromeUtils.generateQI?

Nothing more.

> I don't understand why you've already changed some of those in the Calendar
> patch and now you've dropped those hunks again.

Because I made those changes seeing there was a manual 'QueryInterface: function(aIID) {}' function to convert and then I found out I don't know how to convert it;) So I just didn't backtrack those other changes. Now I did.
(In reply to :aceman from comment #30)
> (In reply to Jorg K (GMT+1) from comment #29)
> > Aceman, is there going to be a patch to change XPCOMUtils.generateQI to
> > ChromeUtils.generateQI?
> Nothing more.
Well, the bug is about changing over to ChromeUtils.generateQI() which is partly done in the rewrites of the manual QIs. So are you saying you're not going to do that patch? We should try to get Kris' script going for that.
Blocks: 1458370
No longer blocks: 1458370
Filed bug 1458370 for the Calendar part.
Summary: Port bug 1456035: Change XPCOMUtils.generateQI to ChromeUtils.generateQI and convert QueryInterface(aIID) → Port bug 1456035: Change XPCOMUtils.generateQI to ChromeUtils.generateQI in (mail, mailnew and chat) and convert QueryInterface(aIID)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/25cf4e4c34f5
replace 'QueryInterface: function()' with 'XPCOMUtils.generateQI()' - calendar. r=philipp
Attachment #8972276 - Attachment description: 1457713-cal.patch QI rewrite - calendar v1.2 → 1457713-cal.patch QI rewrite - calendar v1.2 [landed comment #33]
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #12)
> Huh. I thought I had a README...
> Basically:
>   %cd /path/to/comm-central
>   %sh /path/to/m-c-rewrites/run-replacer.sh chromeutils-generateQI
> You might want to add "mozilla/" to the list of exclusions in
> chromeutils-generateQI.jsm first, though.

Thunderbird is moving to a setup where comm-central lives in a subdirectory "comm" of mozilla-central. This suits the script since it expects to run 'mach' from the top directory. Before it was in mozilla/.

I downloaded run-replacer.sh. main.js, Processor.jsm, Replacer.jsm and chromeutils-generateQI.jsm from
https://bitbucket.org/kmaglione/m-c-rewrites/src/3acc52e49ab4?at=default,
I've added all m-c directories to the path to be ignored, like "dom", "layout", "netwerk", etc., only leaving "comm" to be processed. I wasn't sure where the script automatically excludes .hg, .cargo and the object directory, so I added them as well, better safe than very sorry in case it modified .hg.

Running the script I now get:
JavaScript error: ./main.js, line 13: NS_ERROR_FILE_UNRECOGNIZED_PATH: Component returned failure code: 0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsIFile.initWithPath]
That line is: const baseDir = nsFile(arguments[0]);

BTW, I'm running this on Windows in Mozilla's development environment which provides a bash shell.
Flags: needinfo?(kmaglione+bmo)
Posted file replacer-script.7z (obsolete) —
Contains modified chromeutils-generateQI.jsm. Aceman, perhaps you can give it a try.
Flags: needinfo?(acelists)
Posted patch 1457713-generateQI-mail.patch (obsolete) — Splinter Review
Here's a patch created using 'sed' with indentation corrected manually.
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(acelists)
Attachment #8975974 - Flags: review?(acelists)
Straight 'sed', no manual fix needed.
Assignee: acelists → jorgk
Status: NEW → ASSIGNED
Attachment #8975975 - Flags: review?(acelists)
'sed' and manual indentation fixes where necessary. So we're done here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3748f8572ea8aebbb3351a60431f0e0249e2095e
Attachment #8972787 - Attachment is obsolete: true
Attachment #8975981 - Flags: review?(acelists)
Grrr, that hit jQuery :-( - Now fixed.
Attachment #8975974 - Attachment is obsolete: true
Attachment #8975974 - Flags: review?(acelists)
Attachment #8975983 - Flags: review?(acelists)
Attachment #8975975 - Flags: review?(acelists) → review+
Comment on attachment 8975981 [details] [diff] [review]
1457713-generateQI-mailnews.patch

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

Thanks.
It seems ChromeUtils.generateQI() also takes string names of the interfaces (Ci.X and "X"). Do we know which version is better to use?

::: mailnews/base/search/src/nsMsgTraitService.js
@@ +34,1 @@
>        Ci.nsIMsgTraitService]),

Maybe this does not need to be wrapped.

::: mailnews/base/src/newMailNotificationService.js
@@ +56,5 @@
>  NewMailNotificationService.prototype = {
>    classDescription: "Maintain counts of new and unread messages",
>    classID:              Components.ID("{740880E6-E299-4165-B82F-DF1DCAB3AE22}"),
>    contractID:           "@mozilla.org/newMailNotificationService;1",
> +  QueryInterface:       ChromeUtils.generateQI([Ci.nsIObserver, Ci.nsIFolderListener, Ci.mozINewMailNotificationService]),

This seems to need some wrapping.
Attachment #8975981 - Flags: review?(acelists) → review+
Comment on attachment 8975983 [details] [diff] [review]
1457713-generateQI-mail.patch

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

Thanks.

::: mail/base/content/msgHdrView.js
@@ +404,1 @@
>        [Ci.nsIMsgHeaderSink]),

unwrap
Attachment #8975983 - Flags: review?(acelists) → review+
(In reply to :aceman from comment #40)
> It seems ChromeUtils.generateQI() also takes string names of the interfaces
> (Ci.X and "X"). Do we know which version is better to use?
Looks like a string causes more processing:
https://searchfox.org/mozilla-central/rev/8affe6e83188787eb61fe0528eeb6eef6081ba06/dom/base/MozQueryInterface.cpp#62

But I won't change strings to Ci.X now.
Keywords: leave-open
Target Milestone: --- → Thunderbird 62.0
(In reply to Jorg K (GMT+2) (bustage-fix only, NI for urgent reviews) from comment #42)
> (In reply to :aceman from comment #40)
> > It seems ChromeUtils.generateQI() also takes string names of the interfaces
> > (Ci.X and "X"). Do we know which version is better to use?
> Looks like a string causes more processing:
> https://searchfox.org/mozilla-central/rev/
> 8affe6e83188787eb61fe0528eeb6eef6081ba06/dom/base/MozQueryInterface.cpp#62
> 
> But I won't change strings to Ci.X now.

Not really. The two are about equally efficient. If it was in JITted code and we had IC support for Ci property lookups, that form might be cheaper, but in the current regime, if anything, it's probably slightly more expensive.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f378ff5bbc89
Port bug 1456035: Change XPCOMUtils.generateQI to ChromeUtils.generateQI in mail. r=aceman
https://hg.mozilla.org/comm-central/rev/bf2c27b81cae
Port bug 1456035: Change XPCOMUtils.generateQI to ChromeUtils.generateQI in chat. r=aceman
https://hg.mozilla.org/comm-central/rev/148c2d27abc3
Port bug 1456035: Change XPCOMUtils.generateQI to ChromeUtils.generateQI in mailnews. r=aceman
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.