1432908 - Port bug 1432257 to LDAP and Calendar (and potentially other places)

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Jorg K (CEST = GMT+2)]

Bug 1432257 will remove nsIStandardURL::Init().

We use it here
https://searchfox.org/comm-central/source/calendar/base/src/calProtocolHandler.js#36

and here:
https://dxr.mozilla.org/comm-central/rev/1f4e1c314fac5e97f4efa989e2731104d5e144a7/ldap/xpcom/src/nsLDAPURL.cpp#50

The replacement is described here:
https://github.com/valenting/gecko/wiki/Migrating-nsIURI-code-to-use-mutators#js-call-init-or-other-methods-on-a-uri

or examples can be found in bug 1432187:
https://hg.mozilla.org/integration/autoland/rev/42c5d8b64356

Comment 1

[Jorg K (CEST = GMT+2)]

One more:
https://dxr.mozilla.org/comm-central/source/calendar/lightning/components/calItipProtocolHandler.js#81

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1432908-replace-init.patch

I hope I got this right.

Philipp, to test this, you need bug 1432187 locally. Valentin asked me to get this done soon since it's blocking bug 1432257.

This is independent of bug 1432204 which is a long-term project (with a few bits landed already).

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 8945619 [details] [diff] [review]
1432908-replace-init.patch

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

::: ldap/xpcom/src/nsLDAPURL.cpp
@@ -40,5 @@
> -  {
> -    mBaseURL = do_CreateInstance(NS_STANDARDURL_CONTRACTID);
> -    if (!mBaseURL)
> -      return NS_ERROR_OUT_OF_MEMORY;
> -  }

I forgot to say: I checked this code and mBaseURL is only ever set here, so the block will always run. So we might as well "integrate" the creation and the init in the call below.

Comment 4

[Valentin Gosu [:valentin]]

Comment on attachment 8945619 [details] [diff] [review]
1432908-replace-init.patch

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

This looks good, but you missed a few:
https://searchfox.org/comm-central/source/ldap/xpcom/src/nsLDAPProtocolHandler.js#26
https://searchfox.org/comm-central/source/suite/common/src/nsGopherProtocolStubHandler.js#40
https://searchfox.org/comm-central/source/mozilla/extensions/irc/js/lib/chatzilla-service.js#232

Comment 5

[Jorg K (CEST = GMT+2)]

(In reply to Valentin Gosu [:valentin] from comment #4)
> This looks good, but you missed a few:
Umm, have I?

> https://searchfox.org/comm-central/source/ldap/xpcom/src/
> nsLDAPProtocolHandler.js#26
No, that's not missed, that calls into the C++ code I changed.

> https://searchfox.org/comm-central/source/suite/common/src/
> nsGopherProtocolStubHandler.js#40
Yes, well, sometimes we (TB people) fix suite, sometimes we don't. Currently it's broken anyway due to bug 1431913.

> https://searchfox.org/comm-central/source/mozilla/extensions/irc/js/lib/
> chatzilla-service.js#232
Wow, where did you dig this out, the link doesn't even work.
That looks like it's in the Chatzilla, and we also don't mess with this.

So as far as TB is concerned, this should be it, don't you agree?

FRG, here's another one for you. You said in bug 1431913 that you will take care of the SM part, that bug even has a patch for two suite files (attachment 8944314 [details] [diff] [review]). Do you have a bug for SM already? If so, please add this to it.

Comment 6

[Valentin Gosu [:valentin]]

(In reply to Jorg K (GMT+1) from comment #5)
> (In reply to Valentin Gosu [:valentin] from comment #4)
> > This looks good, but you missed a few:
> Umm, have I?
> 
> > https://searchfox.org/comm-central/source/ldap/xpcom/src/
> > nsLDAPProtocolHandler.js#26
> No, that's not missed, that calls into the C++ code I changed.

Oh, I thought ldap also extended nsIStandardURL since it gets passed that in init. My bad :)

> > https://searchfox.org/comm-central/source/suite/common/src/
> > nsGopherProtocolStubHandler.js#40
> Yes, well, sometimes we (TB people) fix suite, sometimes we don't. Currently
> it's broken anyway due to bug 1431913.

I assume it's useless anyways. I don't know how many users gopher has.

> > https://searchfox.org/comm-central/source/mozilla/extensions/irc/js/lib/
> > chatzilla-service.js#232
> Wow, where did you dig this out, the link doesn't even work.
> That looks like it's in the Chatzilla, and we also don't mess with this.

It just showed up on search fox when searching for nsIStandardURL

> So as far as TB is concerned, this should be it, don't you agree?

Yes. I think that's it for now.
Also, I don't know if this is in some of the other patches or not, but classes extending nsMsgMailNewsUrl (nsSmtpUrl, nsImapUrl, etc) need to override the Mutate() method, and have their own mutator implementation (and correct Clone() or StartClone() implementations)

Comment 7

[Jorg K (CEST = GMT+2)]

(In reply to Valentin Gosu [:valentin] from comment #6)
> Also, I don't know if this is in some of the other patches or not, but
> classes extending nsMsgMailNewsUrl (nsSmtpUrl, nsImapUrl, etc) need to
> override the Mutate() method, and have their own mutator implementation (and
> correct Clone() or StartClone() implementations)
Yes, I'm aware of that, that's meant to happen in bug 1432204. You pointed out that something was fishy in the nsMsgMailNewsUrl::CloneInternal(), but it's been hard to work out why it's doing what it's doing and how to replace it without causing breakage. But that's for bug 1432204 to discuss. Please comment there and give us more background. Currently everything appears to be working even without the deficiencies we detected.

Comment 8

[Frank-Rainer Grahl (:frg)]

> FRG, here's another one for you.

Thanks. Just put it in our current breakage bug 1433370 as a reminder. cZ will probably no longer be fixed by the owners. Not sure if we are able to do it.

Need to do some other things first then see that we get the mailpane. Then it is cleanup time :)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c5f308f257eb
Port bug 1432257 to C-C: Replace use of nsIStandardURL::Init. r=valentin,philipp