The default bug view has changed. See this FAQ.

remove some obsolete constructs in /mailnews

RESOLVED FIXED in Thunderbird 23.0

Status

MailNews Core
Backend
--
trivial
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

Trunk
Thunderbird 23.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

16.66 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
There are still some PR_FALSE, PR_TRUE, PRInt32, nsTArray<*>::NoIndex usages in the /mailnews tree.
(Assignee)

Comment 1

4 years ago
Created attachment 731461 [details] [diff] [review]
patch
Attachment #731461 - Flags: review?(mbanner)

Comment 2

4 years ago
Thank you for taking care of the change(s) in 
mailnews/local/src/nsParseMailbox.cpp
Comment on attachment 731461 [details] [diff] [review]
patch

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

::: mailnews/base/src/nsMsgAccountManager.cpp
@@ +3402,5 @@
>            nsCString folderURI;
>            folder->GetURI(folderURI);
>  
>            int32_t index = searchURI.Find(folderURI);
> +          if (index == kNotFound)

I'm pretty sure kNotFound is incompatible with the external API, which is why we don't use it. If its not that, I'm sure there was some reason...

Hopefully Neil will remember.
Attachment #731461 - Flags: review?(mbanner) → review?(neil)
(Assignee)

Comment 4

4 years ago
I still have no idea what is external API.
And we use kNotFound at other places. So what are the rules of its usage? I think it would be good to have such a constant meaning "not found" and the value could potentially change in the future.

Comment 5

4 years ago
Comment on attachment 731461 [details] [diff] [review]
patch

nsMsgUtils.h #defines kNotFound for us if we're using the external API, so that's safe.

>-    if (m_objectClass.IndexOf(oclass) == nsTArray<nsCString>::NoIndex)
>+    if (m_objectClass.IndexOf(oclass) == m_objectClass.NoIndex)
Nit: In those cases where we don't care about the actual index, you could replace this with
if (!m_objectClass.Contains(oclass))
Attachment #731461 - Flags: review?(neil) → review+

Comment 6

4 years ago
(In reply to :aceman from comment #4)
> I still have no idea what is external API.

You build with 

ac_add_options --enable-incomplete-external-linkage
ac_add_options --enable-incomplete-toolkit-ldap-autocomplete

... and among other things don't have to relink libxul when testing mailnews cpp changes (yay!!) - just a fast make -C mailnews is enough. The downside is it's busted every once in a while (like atm, though i here there are patches waiting to go in that will fix it again).
(Assignee)

Comment 7

4 years ago
(In reply to Magnus Melin from comment #6)
> ac_add_options --enable-incomplete-external-linkage
> ac_add_options --enable-incomplete-toolkit-ldap-autocomplete
> 
> ... and among other things don't have to relink libxul when testing mailnews
> cpp changes (yay!!) - just a fast make -C mailnews is enough. The downside
> is it's busted every once in a while (like atm, though i here there are
> patches waiting to go in that will fix it again).

And how does that work?

Comment 8

4 years ago
Dunno what you mean, but the builds work just fine when it builds - and i don't think they are noticeable slower. Personally i find the internal build compile-test cycle way to slow to be productive with c++ work.
(Assignee)

Comment 9

4 years ago
That's why I disable everything possible in the mozconfig :) So that the "internal" build is bearable (think ~3 minutes).

I mean how does it tack on the changed code onto the existing libxul if that one isn't relinked.
(Assignee)

Comment 10

4 years ago
Created attachment 734765 [details] [diff] [review]
patch v2
Attachment #731461 - Attachment is obsolete: true
Attachment #734765 - Flags: review?(neil)

Comment 11

4 years ago
(In reply to :aceman from comment #9)
> That's why I disable everything possible in the mozconfig :) So that the
> "internal" build is bearable (think ~3 minutes).

With an external build it's like < 10 seconds, depending on what you change of course.

> I mean how does it tack on the changed code onto the existing libxul if that
> one isn't relinked.

Build isn't my cup of tee but that would be the "external" part i suppose? That it's kept outside libxul at some level.
(In reply to aceman from comment #9)
> I mean how does it tack on the changed code onto the existing libxul if that
> one isn't relinked.

It doesn't, it creates libmail, libsmime and libimport (although we really should lump them altogether at some point, so as to avoid multiple copies of base/util).
Comment on attachment 734765 [details] [diff] [review]
patch v2

r=me based on code inspection of the interdiff.
Attachment #734765 - Flags: review?(neil) → review+
(Assignee)

Comment 14

4 years ago
Thanks.

I should try that out some time. Is the external build functionally equivalent to normal build? As in, if something compiles and works then it is guaranteed to work also in normal build? On the other hand, I understand external build could fail whereas normal could still succeed with the same patch.
Keywords: checkin-needed

Comment 15

4 years ago
Functionality should be equivalent, if it's not then that's a bug.
Compiling doesn't necessarily work/fail with same things esp. due to the slightly different string apis. OTOH, i think in practice if you're compiling with external linkage and it works it's very likely to work with internal linkage as well.
(Assignee)

Comment 16

4 years ago
OK, one more thing: what gets into libxul and what into the libmail, libsmime and libimport libraries? Is libxul the product of /mozilla (m-c) so if I make changes there it still needs to be relinked? Do the 3 libs cover all of /mail* ?
(In reply to Magnus Melin from comment #15)
> Functionality should be equivalent, if it's not then that's a bug.

Functionality should be the same, but performance is likely to be impacted - startup due to more dlls to be loaded, and generally due to the use of slower APIs. We've never actually measured it though.

(and bugzilla isn't really the place for this discussion...)
https://hg.mozilla.org/comm-central/rev/a3df3ce94ea8
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 23.0
You need to log in before you can comment on or make changes to this bug.