856304 - remove some obsolete constructs in /mailnews

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[:aceman]

There are still some PR_FALSE, PR_TRUE, PRInt32, nsTArray<*>::NoIndex usages in the /mailnews tree.

Comment 1

[:aceman]

Attachment: patch

Comment 2

[ISHIKAWA, Chiaki]

Thank you for taking care of the change(s) in 
mailnews/local/src/nsParseMailbox.cpp

Comment 3

[Mark Banner (:standard8)]

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.

Comment 4

[:aceman]

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

[[e-mail]]

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))

Comment 6

[Magnus Melin [:mkmelin]]

(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).

Comment 7

[:aceman]

(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

[Magnus Melin [:mkmelin]]

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.

Comment 9

[:aceman]

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.

Comment 10

[:aceman]

Attachment: patch v2

Comment 11

[Magnus Melin [:mkmelin]]

(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.

Comment 12

[[e-mail]]

(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 13

[[e-mail]]

Comment on attachment 734765 [details] [diff] [review]
patch v2

r=me based on code inspection of the interdiff.

Comment 14

[:aceman]

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.

Comment 15

[Magnus Melin [:mkmelin]]

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.

Comment 16

[:aceman]

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* ?

Comment 17

[Mark Banner (:standard8)]

(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...)

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/a3df3ce94ea8