Last Comment Bug 705573 - Make mailnews compile with frozen linkage on debug Windows
: Make mailnews compile with frozen linkage on debug Windows
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All Windows XP
: -- normal (vote)
: Thunderbird 11.0
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
Depends on:
Blocks: mailnews-libxul
  Show dependency treegraph
 
Reported: 2011-11-27 16:07 PST by neil@parkwaycc.co.uk
Modified: 2011-12-23 01:22 PST (History)
1 user (show)
bugzillamozillaorg_serge_20140323: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed patch (6.26 KB, patch)
2011-11-27 16:18 PST, neil@parkwaycc.co.uk
mozilla: review+
Details | Diff | Splinter Review
(Bv1) GetCPUTime(): Report error when not MOZILLA_INTERNAL_API too [Checked in: See comment 9] (1.64 KB, patch)
2011-12-04 08:21 PST, Serge Gautherie (:sgautherie)
standard8: review+
standard8: approval‑comm‑aurora-
Details | Diff | Splinter Review

Description neil@parkwaycc.co.uk 2011-11-27 16:07:57 PST
+++ This bug was initially created as a clone of Bug #705553 +++

Just a few changes to help with the compilation process with frozen linkage.
Comment 1 neil@parkwaycc.co.uk 2011-11-27 16:18:01 PST
Created attachment 577171 [details] [diff] [review]
Proposed patch

I wasn't actually able to get the Thunderbird version of nsMessengerWinIntegration to compile --with-libxul-sdk so these changes are only sufficient to get SeaMonkey to compile.

I didn't think it was worthwhile implementing nsPrintfCString. (I thought we'd done it somewhere else but I couldn't find it.)

The Makefile.in changes are because debug builds don't like to inline stuff and sometimes want to link to things that aren't even called. Sigh.
Comment 2 Serge Gautherie (:sgautherie) 2011-11-28 10:30:06 PST
Comment on attachment 577171 [details] [diff] [review]
Proposed patch

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

::: mailnews/base/util/nsStopwatch.cpp
@@ +162,3 @@
>    if (!ret)
>      NS_ERROR(nsPrintfCString("GetProcessTimes() failed, error=0x%lx.", GetLastError()).get());
>  #endif

Nit: Could that become
{{
  if (!ret) {
#ifdef MOZILLA_INTERNAL_API
    ... with GetLastError()
#else
    // A comment about nsPrintfCString() not being available.
    ... without GetLastError()
#end
  }
}}
fwiw ?
Or could the full error string be created in a different manner?
Comment 3 neil@parkwaycc.co.uk 2011-11-28 11:44:55 PST
(In reply to Serge Gautherie from comment #2)
> (From update of attachment 577171 [details] [diff] [review])
>     // A comment about nsPrintfCString() not being available.
Well, I could just NS_ERROR("GetProcessTimes() failed"); I suppose...

> Or could the full error string be created in a different manner?
Not really, because then you have to start fiddling around with #ifdef DEBUG.
Comment 4 David :Bienvenu 2011-12-01 12:34:20 PST
Comment on attachment 577171 [details] [diff] [review]
Proposed patch

debug builds and runs...
Comment 5 neil@parkwaycc.co.uk 2011-12-04 06:47:48 PST
Pushed changeset 9b9931394d81 to comm-central.
Comment 6 Serge Gautherie (:sgautherie) 2011-12-04 08:21:44 PST
Created attachment 578905 [details] [diff] [review]
(Bv1) GetCPUTime(): Report error when not MOZILLA_INTERNAL_API too
[Checked in: See comment 9]

Per comment 2 and 3.
This should additionally fix a new "unused var ret" warning ;->
Comment 7 Serge Gautherie (:sgautherie) 2011-12-17 07:32:30 PST
David, ping for review.
Comment 8 Mark Banner (:standard8) 2011-12-19 02:59:14 PST
Comment on attachment 578905 [details] [diff] [review]
(Bv1) GetCPUTime(): Report error when not MOZILLA_INTERNAL_API too
[Checked in: See comment 9]

> #ifdef DEBUG
>+  if (!ret)
> #ifdef MOZILLA_INTERNAL_API
>-  if (!ret)
>     NS_ERROR(nsPrintfCString("GetProcessTimes() failed, error=0x%lx.", GetLastError()).get());
>+#else
>+    // nsPrintfCString() is unavailable to report GetLastError().
>+    NS_ERROR("GetProcessTimes() failed.");
> #endif
> #endif

nit: I'd prefer for the if (!ret) to be there twice, as then it doesn't mess up indentation for editors that don't understand ifdefs.

r=me with that fixed.
Comment 9 Serge Gautherie (:sgautherie) 2011-12-22 08:50:42 PST
Comment on attachment 578905 [details] [diff] [review]
(Bv1) GetCPUTime(): Report error when not MOZILLA_INTERNAL_API too
[Checked in: See comment 9]

http://hg.mozilla.org/comm-central/rev/bc7c26552fe4
Bv1, with comment 8 suggestion(s).


"approval-comm-aurora=?":
AURORA_BASE_20111220 happened in the meantime :-/ No risk.
Comment 10 Mark Banner (:standard8) 2011-12-23 01:22:44 PST
Comment on attachment 578905 [details] [diff] [review]
(Bv1) GetCPUTime(): Report error when not MOZILLA_INTERNAL_API too
[Checked in: See comment 9]

Default configurations will have this as internal API, and we don't even completely support external API on what is aurora now, plus it is debug only. Sorry, but this just isn't necessary on branch.

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