Last Comment Bug 856304 - remove some obsolete constructs in /mailnews
: remove some obsolete constructs in /mailnews
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: Thunderbird 23.0
Assigned To: :aceman
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-30 04:44 PDT by :aceman
Modified: 2013-04-13 05:12 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (12.39 KB, patch)
2013-03-30 04:45 PDT, :aceman
neil: review+
Details | Diff | Splinter Review
patch v2 (16.66 KB, patch)
2013-04-08 12:11 PDT, :aceman
neil: review+
Details | Diff | Splinter Review

Description :aceman 2013-03-30 04:44:26 PDT
There are still some PR_FALSE, PR_TRUE, PRInt32, nsTArray<*>::NoIndex usages in the /mailnews tree.
Comment 1 :aceman 2013-03-30 04:45:50 PDT
Created attachment 731461 [details] [diff] [review]
patch
Comment 2 ISHIKAWA, Chiaki 2013-03-31 04:38:48 PDT
Thank you for taking care of the change(s) in 
mailnews/local/src/nsParseMailbox.cpp
Comment 3 Mark Banner (:standard8, afk until Dec) 2013-04-07 13:45:29 PDT
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 2013-04-07 13:58:37 PDT
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 neil@parkwaycc.co.uk 2013-04-07 15:58:47 PDT
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 2013-04-07 22:32:24 PDT
(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 2013-04-07 23:47:10 PDT
(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 2013-04-07 23:52:26 PDT
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 2013-04-08 00:03:47 PDT
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 2013-04-08 12:11:36 PDT
Created attachment 734765 [details] [diff] [review]
patch v2
Comment 11 Magnus Melin 2013-04-08 12:40:39 PDT
(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 neil@parkwaycc.co.uk 2013-04-08 14:33:21 PDT
(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 neil@parkwaycc.co.uk 2013-04-08 14:39:48 PDT
Comment on attachment 734765 [details] [diff] [review]
patch v2

r=me based on code inspection of the interdiff.
Comment 14 :aceman 2013-04-08 23:18:12 PDT
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 2013-04-08 23:34:10 PDT
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 2013-04-08 23:39:24 PDT
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, afk until Dec) 2013-04-09 00:47:44 PDT
(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] 2013-04-13 05:12:29 PDT
https://hg.mozilla.org/comm-central/rev/a3df3ce94ea8

Note You need to log in before you can comment on or make changes to this bug.