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

RESOLVED FIXED in Thunderbird 60.0

Status

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: jorgk, Assigned: jorgk)

Tracking

unspecified
Thunderbird 60.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Assignee

Comment 2

Last year
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).
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8945619 - Flags: review?(valentin.gosu)
Attachment #8945619 - Flags: review?(philipp)
Assignee

Comment 3

Last year
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.
Assignee

Comment 5

Last year
(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.
Flags: needinfo?(frgrahl)
(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)
Assignee

Comment 7

Last year
(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.
> 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 :)
Flags: needinfo?(frgrahl)
Attachment #8945619 - Flags: review?(philipp) → review+

Comment 9

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c5f308f257eb
Port bug 1432257 to C-C: Replace use of nsIStandardURL::Init. r=valentin,philipp
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Assignee

Updated

Last year
Target Milestone: --- → Thunderbird 60.0
You need to log in before you can comment on or make changes to this bug.