Last Comment Bug 801383 - Build is failing in comm-central MailNews code since m-c Bug 779473
: Build is failing in comm-central MailNews code since m-c Bug 779473
Status: RESOLVED FIXED
: regression
Product: MailNews Core
Classification: Components
Component: Database (show other bugs)
: Trunk
: x86_64 Linux
: -- blocker (vote)
: Thunderbird 19.0
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
: 801389 (view as bug list)
Depends on: nsresult-enum-class 801559
Blocks: 801541 801916 802135
  Show dependency treegraph
 
Reported: 2012-10-14 02:46 PDT by Ian Neal
Modified: 2012-11-29 02:16 PST (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
[checked in] Fix nsMsgSend (1.62 KB, patch)
2012-10-14 06:06 PDT, Ian Neal
standard8: review+
Details | Diff | Splinter Review
Fix nsMsgPrompts with static cast (2.97 KB, patch)
2012-10-14 07:00 PDT, Ian Neal
no flags Details | Diff | Splinter Review
Fix nsMsgPrompts without static cast (9.27 KB, patch)
2012-10-14 09:49 PDT, Ian Neal
no flags Details | Diff | Splinter Review
[checked in] Fix nsMsgPrompts without static cast v2 (16.00 KB, patch)
2012-10-15 03:24 PDT, Mark Banner (:standard8)
neil: review+
Details | Diff | Splinter Review
[checked in] Get db/mork compiling (11.00 KB, patch)
2012-10-15 03:27 PDT, Mark Banner (:standard8)
neil: review+
Details | Diff | Splinter Review
Partial patch to fix OSX and Linux builds... (10.15 KB, patch)
2012-10-15 08:44 PDT, Mike Conley (:mconley) - (needinfo me!)
irving: feedback+
Details | Diff | Splinter Review
WIP: fix nsresult enum class bustage under Clang (83.70 KB, patch)
2012-10-15 13:28 PDT, :Irving Reid (No longer working on Firefox)
standard8: feedback+
Details | Diff | Splinter Review
Updated WIP patch (96.75 KB, patch)
2012-10-15 17:57 PDT, Ian Neal
irving: feedback+
Details | Diff | Splinter Review
[checked in] Fix clang build errors caused by nsresult enum class - v3 (99.75 KB, patch)
2012-10-15 20:40 PDT, :Irving Reid (No longer working on Firefox)
standard8: review+
Details | Diff | Splinter Review
[checked in] Fix all errors in nsNNTPProtocol (82.83 KB, patch)
2012-10-15 20:57 PDT, Joshua Cranmer [:jcranmer]
standard8: review+
Details | Diff | Splinter Review
[checked in] Fix error in NNTP search caused by missed function signature change (2.31 KB, patch)
2012-10-16 13:32 PDT, :Irving Reid (No longer working on Firefox)
standard8: review+
Details | Diff | Splinter Review

Description Ian Neal 2012-10-14 02:46:27 PDT
Recent builds on comm-central are failing in the mork code:
../../../../db/mork/src/morkEnv.cpp: In member function ‘void morkEnv::NewErrorAndCode(const char*, mork_u2)’:
../../../../db/mork/src/morkEnv.cpp:363:59: error: operands to ?: have different types ‘mork_u2’ and ‘tag_nsresult’
../../../../db/mork/src/morkEnv.cpp: In member function ‘void morkEnv::NewError(const char*)’:
../../../../db/mork/src/morkEnv.cpp:375:30: error: cannot convert ‘tag_nsresult’ to ‘mork_u4’ in assignment
../../../../db/mork/src/morkEnv.cpp: In member function ‘virtual nsresult morkEnv::GetHeap(nsIMdbHeap**)’:
../../../../db/mork/src/morkEnv.cpp:592:48: error: no match for ‘operator==’ in ‘heap->nsIMdbHeap::HeapAddStrongRef((&((morkEnv*)this)->morkEnv::<anonymous>)) == 0’

This appears have started landing since the most recent merge of mozilla-inbound / mozilla-central at Sat Oct 13 16:26:43
d750d39139d1	Ryan VanderMeulen — Merge the last PGO-green inbound changeset to m-c.
Comment 1 Aryeh Gregor (:ayg) (away until October 25) 2012-10-14 03:29:04 PDT
This is because of the enum class change, not the enum change.  It should be relatively easy to fix -- don't try to implicitly convert other things to an nsresult.  As a quick workaround, you can just add appropriate static_cast<>'s everywhere to the expected type while you work on proper fixes, if there are too many to properly fix right away.
Comment 2 ojab 2012-10-14 03:50:45 PDT
*** Bug 801389 has been marked as a duplicate of this bug. ***
Comment 3 Philip Chee 2012-10-14 04:46:47 PDT
c:\t1\hg\comm-central\config\rules.mk:1195:0$ c:/DEV/mozilla-build/python/python2.6.exe -O c:/t1/hg/
comm-central/mozilla/build/cl.py cl -FonsMsgSend.obj -c -D_HAS_EXCEPTIONS=0 -I../../../mozilla/dist/
stl_wrappers  -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GF
X -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DZLIB_INTERNAL -DMOZ_SUITE=1 -DOSTYP
E=\"WINNT6.1\" -DOSARCH=WINNT  -Ic:/t1/hg/comm-central/mailnews/compose/src -I. -I../../../mozilla/d
ist/include -I../../../mozilla/dist/include/nsprpub  -Ic:/t1/hg/objdir-sm/mozilla/dist/include/nspr
-Ic:/t1/hg/objdir-sm/mozilla/dist/include/nss        -TP -nologo -W3 -Gy -Fdgenerated.pdb -wd4800 -G
R-  -DNDEBUG -DTRIMMED -Zi -Zi -UDEBUG -DNDEBUG -O1 -Oy -MD            -FI ../../../comm-config.h -D
MOZILLA_CLIENT c:/t1/hg/comm-central/mailnews/compose/src/nsMsgSend.cpp
nsMsgSend.cpp
c:/t1/hg/comm-central/mailnews/compose/src/nsMsgSend.cpp(3779) : error C2664: 'NS_ERROR_GET_CODE' :
cannot convert parameter 1 from 'int32_t' to 'nsresult'
        Conversion to enumeration type requires an explicit cast (static_cast, C-style cast or function-style cast)
Comment 4 Ian Neal 2012-10-14 06:06:20 PDT
Created attachment 671198 [details] [diff] [review]
[checked in] Fix nsMsgSend

Not tested as yet.
Comment 5 Ian Neal 2012-10-14 07:00:05 PDT
Created attachment 671206 [details] [diff] [review]
Fix nsMsgPrompts with static cast
Comment 6 Ian Neal 2012-10-14 07:04:36 PDT
(In reply to Ian Neal from comment #5)
> Created attachment 671206 [details] [diff] [review]
> Fix nsMsgPrompts with static cast

To fix without static cast, all the mailnews NS_ERROR type defines would probably have to be converted to nsresult along the lines of nsErrorList.h I guess.
Comment 7 Philip Chee 2012-10-14 07:55:30 PDT
Note: static_cast probably needed here:
http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsAbLDAPAutoCompFormatter.cpp#222
Comment 8 Ian Neal 2012-10-14 09:49:39 PDT
Created attachment 671229 [details] [diff] [review]
Fix nsMsgPrompts without static cast

This version doesn't use a static cast, instead it changes the relevant defines to provide nsresults.
Comment 9 Mark Banner (:standard8) 2012-10-14 12:27:38 PDT
Comment on attachment 671198 [details] [diff] [review]
[checked in] Fix nsMsgSend

Looks right.
Comment 10 neil@parkwaycc.co.uk 2012-10-14 13:34:37 PDT
Comment on attachment 671229 [details] [diff] [review]
Fix nsMsgPrompts without static cast

> #define SMTP_DELIV_MAIL                             12521
Anyone think it might be a good idea to follow up with all of these to make all of these nsresults and then have the consumer extract the code, or seeing as we already have a weird mixture, it makes no difference to add a few more into the mix?

Actually thinking about it we should probably switch these to string IDs.

>-#define NS_MSG_FOLLOWUPTO_ALERT                     12564
>+#define NS_MSG_FOLLOWUPTO_ALERT                     NS_MSG_GENERATE_FAILURE(12564)
I was in two minds over some of these as to whether they should be SUCCESS or FAILURE...

>-  bundle->GetStringFromID(NS_IS_MSG_ERROR(msgID) ? NS_ERROR_GET_CODE(msgID) : msgID, getter_Copies(msg));
>+  nsMsgGetMessageByID(msgID, msg);
Apart from this change I think I ended up making all the same other changes locally. (For this one I had just changed it to unconditionally get the code.)
Comment 11 Mark Banner (:standard8) 2012-10-15 01:07:17 PDT
(In reply to neil@parkwaycc.co.uk from comment #10)
> Comment on attachment 671229 [details] [diff] [review]
> Fix nsMsgPrompts without static cast
> 
> > #define SMTP_DELIV_MAIL                             12521
> Anyone think it might be a good idea to follow up with all of these to make
> all of these nsresults and then have the consumer extract the code, or
> seeing as we already have a weird mixture, it makes no difference to add a
> few more into the mix?

Yes, I think we should make all of them nsresults in a follow-up.

> Actually thinking about it we should probably switch these to string IDs.

I think we need to keep some of the nsresult values, but change the string IDs themselves from "12564" to "msgFollowuptoAlert" (or something like).

> >-#define NS_MSG_FOLLOWUPTO_ALERT                     12564
> >+#define NS_MSG_FOLLOWUPTO_ALERT                     NS_MSG_GENERATE_FAILURE(12564)
> I was in two minds over some of these as to whether they should be SUCCESS
> or FAILURE...

Urgh, yeah, that's definitely a string-only ID.
Comment 12 Mark Banner (:standard8) 2012-10-15 03:24:19 PDT
Created attachment 671365 [details] [diff] [review]
[checked in] Fix nsMsgPrompts without static cast v2

Ok, I've taken Ian's patch and updated it with two things:

- More changes to make mailnews/compose compile on Mac.
- For NS_MSG_FOLLOWUPTO_ALERT change this string to use a string name rather than an ID, and provide appropriate functions which we can use for future additional replacements.
Comment 13 Mark Banner (:standard8) 2012-10-15 03:27:29 PDT
Created attachment 671368 [details] [diff] [review]
[checked in] Get db/mork compiling

This is what it takes to get db/mork compiling on Mac. There's various inconsistencies here like what AddWeakRef etc return, but I think that's a future clean up if they are needed at all.

Note that HeapAddStrongRef, HeapCutStrongRef and morkEnv::NewErrorAndCode are all unused functions.
Comment 14 Mark Banner (:standard8) 2012-10-15 05:28:06 PDT
Comment on attachment 671198 [details] [diff] [review]
[checked in] Fix nsMsgSend

Checked in: https://hg.mozilla.org/comm-central/rev/af0697548fb2
Comment 15 neil@parkwaycc.co.uk 2012-10-15 05:52:37 PDT
Comment on attachment 671365 [details] [diff] [review]
[checked in] Fix nsMsgPrompts without static cast v2

>-      bundle->GetStringFromID(NS_MSG_FAILURE_ON_OBJ_EMBED_WHILE_SENDING, getter_Copies(msg));
>+      bundle->GetStringFromID(NS_ERROR_GET_CODE(NS_MSG_FAILURE_ON_OBJ_EMBED_WHILE_SENDING), getter_Copies(msg));
As this is the only use of NS_MSG_FAILURE_ON_OBJ_EMBED_WHILE_SENDING we should change it to be a string ID instead of an nsresult which would make it clearer especially for any subsequent string name conversion. r=me with that fixed.

>-      if (SendQuit(SMTP_ERROR_DONE) < 0)
>+      if (NS_FAILED(SendQuit(SMTP_ERROR_DONE)))
Eek! Does this need to land on aurora, as it's basically a regression fix from bug 779130? (nsresult is unsigned to the check wasn't working.)
Comment 16 Neil Lee [:neilio] 2012-10-15 06:13:48 PDT
Guys, please make sure you're flagging the correct person for review.
Comment 17 :Irving Reid (No longer working on Firefox) 2012-10-15 07:12:13 PDT
(In reply to neil@parkwaycc.co.uk from comment #15)
> Comment on attachment 671365 [details] [diff] [review]

> >-      if (SendQuit(SMTP_ERROR_DONE) < 0)
> >+      if (NS_FAILED(SendQuit(SMTP_ERROR_DONE)))
> Eek! Does this need to land on aurora, as it's basically a regression fix
> from bug 779130? (nsresult is unsigned to the check wasn't working.)

Good catch; we should back port this - it also overlaps bug 780124.
Comment 18 Mark Banner (:standard8) 2012-10-15 07:28:43 PDT
Comment on attachment 671365 [details] [diff] [review]
[checked in] Fix nsMsgPrompts without static cast v2

https://hg.mozilla.org/comm-central/rev/b9df7f0be3cd
Comment 19 Mike Conley (:mconley) - (needinfo me!) 2012-10-15 08:44:56 PDT
Created attachment 671457 [details] [diff] [review]
Partial patch to fix OSX and Linux builds...

So I started working on cleaning up some more of our nsresult abusage.

It turns out that I'm not 100% familiar with how a lot of our back-end works. So I'm kind of wary of messing with this stuff.

This patch fixes some of the issues I felt comfortable fixing, but somebody who's more familiar with our backend should probably take the ball here.
Comment 20 :Irving Reid (No longer working on Firefox) 2012-10-15 09:37:14 PDT
Comment on attachment 671457 [details] [diff] [review]
Partial patch to fix OSX and Linux builds...

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

Other than removing the NS_ENSURE_SUCCESS noted below, looks good.

::: mailnews/addrbook/src/nsAddrDatabase.cpp
@@ +624,1 @@
>    return ret;

All the functions we call above *should* be returning valid nsresult values now (I looked at a few, and they seem OK). We shouldn't need the NS_ENSURE_SUCCESS macro here, we can just return ret.
Comment 21 neil@parkwaycc.co.uk 2012-10-15 09:51:58 PDT
Comment on attachment 671368 [details] [diff] [review]
[checked in] Get db/mork compiling

>-  mEnv_ErrorCode = morkEnv_kGenericError;
>+  mEnv_ErrorCode = (mork_u4)morkEnv_kGenericError;
[Ideally you would make mEnv_ErrorCode an mdb_err instead.]
Comment 22 Mark Banner (:standard8) 2012-10-15 10:01:52 PDT
(In reply to neil@parkwaycc.co.uk from comment #21)
> Comment on attachment 671368 [details] [diff] [review]
> Get db/mork compiling
> 
> >-  mEnv_ErrorCode = morkEnv_kGenericError;
> >+  mEnv_ErrorCode = (mork_u4)morkEnv_kGenericError;
> [Ideally you would make mEnv_ErrorCode an mdb_err instead.]

Filed bug 801718.
Comment 23 Mark Banner (:standard8) 2012-10-15 10:08:54 PDT
Comment on attachment 671368 [details] [diff] [review]
[checked in] Get db/mork compiling

https://hg.mozilla.org/comm-central/rev/eac22e3d7ba3
Comment 24 :aceman 2012-10-15 13:03:21 PDT
Is anybody fixing these?
mailnews/base/util/nsMsgKeySet.cpp: In member function 'int nsMsgKeySet::Add(int32_t)':
mailnews/base/util/nsMsgKeySet.cpp:712:24: error: cannot convert 'tag_nsresult' to 'int' in return
mailnews/base/util/nsMsgKeySet.cpp: In member function 'int nsMsgKeySet::Remove(int32_t)':
mailnews/base/util/nsMsgKeySet.cpp:829:41: error: cannot convert 'tag_nsresult' to 'int' in return
mailnews/base/util/nsMsgKeySet.cpp: In member function 'int nsMsgKeySet::AddRange(int32_t, int32_t)':
mailnews/base/util/nsMsgKeySet.cpp:929:30: error: cannot convert 'tag_nsresult' to 'int' in return

mailnews/base/util/nsMsgLineBuffer.cpp: In member function 'int32_t nsMsgLineBuffer::ConvertAndSendBuffer()':
mailnews/base/util/nsMsgLineBuffer.cpp:233:85: error: cannot convert 'nsresult {aka tag_nsresult}' to 'int32_t {aka int}' in return
mailnews/base/util/nsMsgLineBuffer.cpp: In member function 'virtual int32_t nsMsgLineBuffer::FlushLastLine()':
mailnews/base/util/nsMsgLineBuffer.cpp:242:85: error: cannot convert 'nsresult {aka tag_nsresult}' to 'int32_t {aka int}' in return
mailnews/base/util/nsMsgLineBuffer.cpp:245:1: error: control reaches end of non-void function [-Werror=return-type]
mailnews/base/util/nsMsgLineBuffer.cpp: In member function 'int32_t nsMsgLineBuffer::ConvertAndSendBuffer()':
mailnews/base/util/nsMsgLineBuffer.cpp:234:1: error: control reaches end of non-void function [-Werror=return-type]

etc.
Comment 25 :Irving Reid (No longer working on Firefox) 2012-10-15 13:28:32 PDT
Created attachment 671571 [details] [diff] [review]
WIP: fix nsresult enum class bustage under Clang

This began with mconley's patch and has covered many more files. I need to step out of the office so I wanted to get my WIP up.

There are still compile errors to fix, and after the compile errors are fixed we probably also want to clean up the following...


Sweep for == NS_OK and != NS_OK and replace with NS_SUCCEEDED / NS_FAILED

Go through Pop3 and NNTP and replace -1 returns with more descriptive #defined error codes where definitions exist
Comment 26 :aceman 2012-10-15 14:05:23 PDT
(In reply to Irving Reid (:irving) from comment #25)
> Sweep for == NS_OK and != NS_OK and replace with NS_SUCCEEDED / NS_FAILED
If this does not prevent building and can be done after this bug is closed then I can do it, just file the bug and assign it to me.
Comment 27 Ian Neal 2012-10-15 14:58:00 PDT
(In reply to Irving Reid (:irving) from comment #25)
> Created attachment 671571 [details] [diff] [review]
> WIP: fix nsresult enum class bustage under Clang
> 
I presume nsNntpIncomingServer.cpp has yet to be looked at as that has a HandleLine too?
Comment 28 Ian Neal 2012-10-15 15:24:21 PDT
Comment on attachment 671571 [details] [diff] [review]
WIP: fix nsresult enum class bustage under Clang

>+++ b/mailnews/addrbook/src/nsAbLDAPAutoCompFormatter.cpp
>@@ -214,17 +214,17 @@ nsAbLDAPAutoCompFormatter::FormatExcepti

>+        errCodeNum.AppendInt(static_cast<uint32_t>aErrorCode, 16);    
Missing () round aErrorCode
Comment 29 Mark Banner (:standard8) 2012-10-15 15:25:51 PDT
Comment on attachment 671571 [details] [diff] [review]
WIP: fix nsresult enum class bustage under Clang

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

This is looking reasonable, I'd almost give it r+ except for wanting to look at a couple of files a bit more when I'm more awake. I've not tested it as I'm assuming there's more to come.

::: mailnews/local/src/nsPop3Protocol.cpp
@@ +1331,2 @@
>    }
>    else

nit: we can drop the else here as we're returning early.
Comment 30 Ian Neal 2012-10-15 17:57:32 PDT
Created attachment 671677 [details] [diff] [review]
Updated WIP patch

Changes since Irving's patch:
* Fixed Mark's comment
* Corrected a couple of typos
* Added fixes to more files

Build completes locally with this patch now, I can start the application but no thorough testing done.
Comment 31 Joshua Cranmer [:jcranmer] 2012-10-15 19:12:38 PDT
Egads! On the nsNNTPProtocol.cpp, it's far better to change all of the int32_t methods called by ProcessProtocolState to nsresults instead of changing all the defines to be int32_t--it's what I want to do in the long run anyways (change -1 to NS_ERROR_FAILURE), and I don't think it'd be substantively more lines of changes
Comment 32 :Irving Reid (No longer working on Firefox) 2012-10-15 20:16:15 PDT
Comment on attachment 671677 [details] [diff] [review]
Updated WIP patch

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

I have a couple more fixes so I'll include the changes I mention below and put a new version of the patch up

::: mailnews/addrbook/src/nsAbLDAPAutoCompFormatter.cpp
@@ +218,5 @@
>  
>          // put the entire nsresult in string form
>          //
>          errCodeNum.AppendLiteral("0x");
> +        errCodeNum.AppendInt(static_cast<uint32_t>(aErrorCode), 16);    

Trailing white space

::: mailnews/news/src/nsNntpIncomingServer.cpp
@@ +1208,4 @@
>  nsNntpIncomingServer::HandleLine(const char* line, uint32_t line_size)
>  {
>    NS_ASSERTION(line, "line is null");
> +  if (!line) return NS_OK;

Return on its own line

@@ +1212,3 @@
>  
>    // skip blank lines and comments
> +  if (line[0] == '#' || line[0] == '\0') return NS_OK;

Return on its own line
Comment 33 :Irving Reid (No longer working on Firefox) 2012-10-15 20:40:07 PDT
Created attachment 671714 [details] [diff] [review]
[checked in] Fix clang build errors caused by nsresult enum class - v3

Fix the last of the clang build errors (and a couple of warnings). Still fails a lot of xpcshell tests, and could use a review of compile warning messages - I only paid thorough attention to errors.
Comment 34 Joshua Cranmer [:jcranmer] 2012-10-15 20:57:05 PDT
Created attachment 671719 [details] [diff] [review]
[checked in] Fix all errors in nsNNTPProtocol

This replaces the nsNNTPProtocol-relevant portions of the last patch to make the entire state machine use nsresults instead of int32_t, combined with a few style changes locally (nsNNTPProtocol is in horrible shape, stylistically).

Basic rules of thumb:
1. AlertError ignores its first parameter, so the one place that calls it with an nsresult got a 0 shoved in instead because I didn't want to mutilate another dozen callsites.

2. nsresult rv's added and used where appropriate, as is NS_ENSURE_SUCCESS

3. nsMsgLineBuffer is an annoying interface. Anywhere where we did "return status" where status is passed in as the second parameter to the ReadLine function there becomes a return rv, where rv is passed in the appropriate parameter. The "status" is really "how many bytes are in the line", which should be -1 only when the return value gets set to some error code (I've read it three times, I think that's what happens...).

4. MK_NNTP_LOADED is equivalent to NS_OK (it's #define'd 1 IIRC).

5. All of the other MK_NNTP_* stuff should be negative values; these (and -1 returns) all became NS_ERROR_FAILURE (or rv if one looked more appropriate).

6. I gave up with renaming the status variable in ProcessProtocolState. It's really an rv, but there's too many uses of it. All other nsresults should be named rv, though.

... I guess I forgot about how much cleaning up this code needed.

I'm currently attempting to test these changes to see if they pass...
Comment 35 Joshua Cranmer [:jcranmer] 2012-10-15 21:24:32 PDT
Confirmation: all NNTP tests pass with my change.
Comment 36 Mark Banner (:standard8) 2012-10-16 01:21:18 PDT
Comment on attachment 671719 [details] [diff] [review]
[checked in] Fix all errors in nsNNTPProtocol

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

Looks good, I see failures in test_internalUris.js and test_nntpPassword2.js, but I think the password one could be related to something else and is covered by bug 801916. The test_internalUris.js we could have a look at once this lands.

::: mailnews/news/src/nsNNTPProtocol.cpp
@@ +1369,5 @@
>    {
>      AlertError(MK_NNTP_ERROR_MESSAGE, m_responseText);
>  
>      m_nextState = NNTP_ERROR;
> +    return NS_ERROR_FAILURE;

The define for MK_BAD_NNTP_CONNECTION can be removed with this change.

@@ +1437,1 @@
>    uint32_t status = 0;

nit: I'd prefer status to be moved inside the if, next to where it is really used.
Comment 37 Mark Banner (:standard8) 2012-10-16 03:15:57 PDT
Comment on attachment 671714 [details] [diff] [review]
[checked in] Fix clang build errors caused by nsresult enum class - v3

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

This looks generally fine. I do wonder if we should change the nsPop3Protocol items to use nsresult as Joshua did with nsNNTPProtocol, the other option would be to change status to a boolean there, as I think that's what it is essentially. We can cover those in follow-ups anyway.

r=me without the nsNNTPProtocol changes.
Comment 38 Mark Banner (:standard8) 2012-10-16 03:26:38 PDT
Comment on attachment 671714 [details] [diff] [review]
[checked in] Fix clang build errors caused by nsresult enum class - v3

https://hg.mozilla.org/comm-central/rev/65b8e2fa4eba
Comment 39 Mark Banner (:standard8) 2012-10-16 03:26:58 PDT
Comment on attachment 671719 [details] [diff] [review]
[checked in] Fix all errors in nsNNTPProtocol

https://hg.mozilla.org/comm-central/rev/cc0ff188f5bc
Comment 40 Mark Banner (:standard8) 2012-10-16 03:30:07 PDT
We should now be compiling on all platforms.

There are some test failures still, most of these are covered by bug 801916, but test_internalUris.js and test_nntpPassword2.js do not appear to be.
Comment 41 :Irving Reid (No longer working on Firefox) 2012-10-16 13:32:01 PDT
Created attachment 671995 [details] [diff] [review]
[checked in] Fix error in NNTP search caused by missed function signature change

We changed the IDL signature and the parent class definition of CurrentUrlDone() but missed the override in nsMsgSearchNews

Thanks to jcranmer for spotting the error.

This fixes test_internalUris.js; other tests are still failing
Comment 42 Mark Banner (:standard8) 2012-10-16 14:29:22 PDT
Comment on attachment 671995 [details] [diff] [review]
[checked in] Fix error in NNTP search caused by missed function signature change

Looks fine, minor nit, can you remove the spaces before the open brackets on the lines you're changing please.
Comment 43 Mark Banner (:standard8) 2012-10-17 04:36:15 PDT
Comment on attachment 671995 [details] [diff] [review]
[checked in] Fix error in NNTP search caused by missed function signature change

https://hg.mozilla.org/comm-central/rev/a64aee9cd17e
Comment 44 Mark Banner (:standard8) 2012-10-17 04:39:36 PDT
I believe this is all fixed now apart from the outstanding issues in bug 801916.
Comment 45 Mark Banner (:standard8) 2012-10-17 05:03:17 PDT
(In reply to Mark Banner (:standard8) from comment #43)
> Comment on attachment 671995 [details] [diff] [review]
> [checked in] Fix error in NNTP search caused by missed function signature
> change
> 
> https://hg.mozilla.org/comm-central/rev/a64aee9cd17e

That was the bustage fix, this was the full fix landing:

https://hg.mozilla.org/comm-central/rev/6ad31d127ae2

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