Last Comment Bug 217089 - Fix compiler warnings: "unused variable" and "defined but not used"
: Fix compiler warnings: "unused variable" and "defined but not used"
Status: RESOLVED FIXED
[build_warning] bugday0420
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: x86 Windows XP
: -- trivial (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
: 198785 211429 (view as bug list)
Depends on: 90906 214063 214199 219908 224018 228780
Blocks: buildwarning
  Show dependency treegraph
 
Reported: 2003-08-23 11:34 PDT by Matthias Bockelkamp
Modified: 2015-12-18 04:44 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for CNavDTD.cpp (391 bytes, patch)
2003-08-23 11:36 PDT, Matthias Bockelkamp
timeless: review+
hjtoi-bugzilla: superreview+
Details | Diff | Splinter Review
</modules/plugin/samples/default/windows/dialogs.cpp> [Checked in: Comment 153] (376 bytes, patch)
2003-08-23 11:41 PDT, Matthias Bockelkamp
timeless: review+
alecf: superreview+
Details | Diff | Splinter Review
</modules/oji/src/jvmmgr.cpp> v1 (251 bytes, patch)
2003-08-23 11:43 PDT, Matthias Bockelkamp
timeless: review-
Details | Diff | Splinter Review
Patch for lcglue.cpp (500 bytes, patch)
2003-08-23 11:46 PDT, Matthias Bockelkamp
jst: review+
jst: superreview+
Details | Diff | Splinter Review
Patch for ns4xPlugin.cpp (482 bytes, patch)
2003-08-23 11:48 PDT, Matthias Bockelkamp
timeless: review-
Details | Diff | Splinter Review
Patch for nsBoxObject.cpp (334 bytes, patch)
2003-08-23 11:49 PDT, Matthias Bockelkamp
bryner: review-
bryner: superreview-
Details | Diff | Splinter Review
</widget/src/windows/nsClipboard.cpp> [Checked in: Comment 113] (1.04 KB, patch)
2003-08-23 11:50 PDT, Matthias Bockelkamp
mikepinkerton: review+
sfraser_bugs: superreview+
tor: approval1.6-
Details | Diff | Splinter Review
</widget/src/windows/nsDataObj.cpp>, v1 (1.63 KB, patch)
2003-08-23 11:51 PDT, Matthias Bockelkamp
emaijala+moz: review+
Details | Diff | Splinter Review
Patch for nsDocumentViewer.cpp (478 bytes, patch)
2003-08-23 11:53 PDT, Matthias Bockelkamp
timeless: review+
dbaron: superreview+
Details | Diff | Splinter Review
<nsFileSystemDataSource.cpp> [Checked in: Comment 77] (401 bytes, patch)
2003-08-23 11:56 PDT, Matthias Bockelkamp
timeless: review+
alecf: superreview+
Details | Diff | Splinter Review
Patch for nsFontMetricsWin.cpp (1.15 KB, patch)
2003-08-23 11:58 PDT, Matthias Bockelkamp
rbs: review+
rbs: superreview+
Details | Diff | Splinter Review
</gfx/src/windows/nsGfxFactoryWin.cpp> (448 bytes, patch)
2003-08-23 11:59 PDT, Matthias Bockelkamp
rbs: review-
rbs: superreview-
Details | Diff | Splinter Review
Patch for nsHTMLContentSink.cpp (525 bytes, patch)
2003-08-23 12:01 PDT, Matthias Bockelkamp
timeless: review-
Details | Diff | Splinter Review
Patch for nsImageWin.cpp (678 bytes, patch)
2003-08-23 12:02 PDT, Matthias Bockelkamp
timeless: review+
roc: superreview+
Details | Diff | Splinter Review
Patch for nsJVMManager.cpp (462 bytes, patch)
2003-08-23 12:04 PDT, Matthias Bockelkamp
timeless: review-
Details | Diff | Splinter Review
</widget/src/windows/nsNativeDragTarget.cpp> [Checked in: Comment 173] (590 bytes, patch)
2003-08-23 12:05 PDT, Matthias Bockelkamp
timeless: review+
rbs: superreview+
Details | Diff | Splinter Review
</gfx/src/windows/nsNativeThemeWin.cpp> [Checked in: Comment 135] (1.84 KB, patch)
2003-08-23 12:07 PDT, Matthias Bockelkamp
rbs: review+
rbs: superreview+
Details | Diff | Splinter Review
Patch for nsOSHelperAppService.cpp (616 bytes, patch)
2003-08-23 12:09 PDT, Matthias Bockelkamp
timeless: review+
darin.moz: superreview+
Details | Diff | Splinter Review
</modules/plugin/base/src/nsPluginsDirWin.cpp> [Checked in: Comment 155] (535 bytes, patch)
2003-08-23 12:13 PDT, Matthias Bockelkamp
timeless: review+
alecf: superreview+
Details | Diff | Splinter Review
</gfx/src/windows/nsPrintOptionsWin.cpp> [Checked in: Comment 114] (444 bytes, patch)
2003-08-23 12:17 PDT, Matthias Bockelkamp
timeless: review+
kinmoz: superreview+
tor: approval1.6-
Details | Diff | Splinter Review
Patch for nsRDFXMLDataSource.cpp [Checked in: Comment 112] (197 bytes, patch)
2003-08-23 12:20 PDT, Matthias Bockelkamp
timeless: review+
darin.moz: superreview+
Details | Diff | Splinter Review
Patch for nsScanner.cpp (288 bytes, patch)
2003-08-23 12:21 PDT, Matthias Bockelkamp
timeless: review-
Details | Diff | Splinter Review
Patch for nsScreenManagerWin.cpp (615 bytes, patch)
2003-08-23 12:23 PDT, Matthias Bockelkamp
timeless: review-
Details | Diff | Splinter Review
Patch for nsSelection.cpp (479 bytes, patch)
2003-08-23 12:24 PDT, Matthias Bockelkamp
timeless: review+
Details | Diff | Splinter Review
Patch for nsToolkit.cpp (418 bytes, patch)
2003-08-23 12:26 PDT, Matthias Bockelkamp
bryner: review+
bryner: superreview+
Details | Diff | Splinter Review
<nsWinCharset.cpp>, v1-r (for review only) [Check in: See v1-ci] (450 bytes, patch)
2003-08-23 12:27 PDT, Matthias Bockelkamp
smontagu: review+
jag-mozilla: superreview+
Details | Diff | Splinter Review
Patch for nsWindow.cpp (2.54 KB, patch)
2003-08-23 12:28 PDT, Matthias Bockelkamp
emaijala+moz: review-
Details | Diff | Splinter Review
Patch for nsXMLContentSink.cpp (388 bytes, patch)
2003-08-23 12:29 PDT, Matthias Bockelkamp
timeless: review-
Details | Diff | Splinter Review
</layout/xul/base/src/nsXULTooltipListener.cpp>, v1 (844 bytes, patch)
2003-08-23 12:31 PDT, Matthias Bockelkamp
bryner: review+
jag-mozilla: superreview+
Details | Diff | Splinter Review
</modules/plugin/samples/default/windows/plugin.cpp> v1 (946 bytes, patch)
2003-08-23 12:32 PDT, Matthias Bockelkamp
timeless: review-
Details | Diff | Splinter Review
Patch for nsBoxObject.cpp, v2 (392 bytes, patch)
2003-08-23 15:05 PDT, Matthias Bockelkamp
bryner: review+
bryner: superreview+
Details | Diff | Splinter Review
</modules/oji/src/jvmmgr.cpp> v2 [Checked in: Comment 166] (536 bytes, patch)
2003-08-24 04:47 PDT, Matthias Bockelkamp
yuanyi21: review+
Henry.Jia: superreview+
Details | Diff | Splinter Review
Patch for nsXMLContentSink.cpp, v2 (843 bytes, patch)
2003-08-24 04:50 PDT, Matthias Bockelkamp
hjtoi-bugzilla: review+
hjtoi-bugzilla: superreview+
Details | Diff | Splinter Review
<plugin.cpp>, v2 (1.15 KB, patch)
2003-08-24 04:52 PDT, Matthias Bockelkamp
no flags Details | Diff | Splinter Review
<os2/dialogs.cpp> [Checked in: Comment 107] (632 bytes, patch)
2003-08-24 11:46 PDT, Matthias Bockelkamp
mozilla: review+
mozilla: superreview+
Details | Diff | Splinter Review
Patch for nsHTMLContentSink.cpp, v2 (1.52 KB, patch)
2003-08-24 12:53 PDT, Matthias Bockelkamp
jst: review+
jst: superreview+
Details | Diff | Splinter Review
Patch for nsScanner.cpp, v2 (621 bytes, patch)
2003-08-24 22:10 PDT, Matthias Bockelkamp
jst: review+
jst: superreview+
Details | Diff | Splinter Review
Patch for nsScreenManagerWin.cpp, v2 (2.56 KB, patch)
2003-08-24 22:51 PDT, Matthias Bockelkamp
no flags Details | Diff | Splinter Review
Patch for nsDocumentViewer.cpp, v2 (1.03 KB, patch)
2003-08-24 23:04 PDT, Matthias Bockelkamp
timeless: review+
jst: superreview+
Details | Diff | Splinter Review
Patch for nsScreenManagerWin.cpp, v3 (2.49 KB, patch)
2003-09-07 12:32 PDT, Matthias Bockelkamp
timeless: review-
Details | Diff | Splinter Review
Patch for nsScreenManagerWin.cpp, v4 (3.58 KB, patch)
2003-09-11 06:48 PDT, Matthias Bockelkamp
timeless: review-
Details | Diff | Splinter Review
Patch for nsScreenManagerWin.cpp, v5 (3.58 KB, patch)
2003-09-11 08:20 PDT, Matthias Bockelkamp
emaijala+moz: review-
Details | Diff | Splinter Review
<nsScreenManagerWin.*> (v6) [+/- Checked in: Comment 110] (3.87 KB, patch)
2003-12-20 16:40 PST, Serge Gautherie (:sgautherie)
emaijala+moz: review+
rbs: superreview+
Details | Diff | Splinter Review
</layout/xul/base/src/nsXULTooltipListener.cpp>, v2 [Checked in: Comment 119] (747 bytes, patch)
2004-01-19 14:58 PST, Serge Gautherie (:sgautherie)
bryner: review+
jag-mozilla: superreview+
Details | Diff | Splinter Review
</widget/src/windows/nsDataObj.cpp>, v1b [Checked in: Comment 142] (2.82 KB, patch)
2004-04-22 15:31 PDT, Serge Gautherie (:sgautherie)
emaijala+moz: review+
Details | Diff | Splinter Review
<nsFrame.cpp> [Checked in: Comment 147] (1.50 KB, patch)
2004-04-23 17:56 PDT, Serge Gautherie (:sgautherie)
dbaron: review+
dbaron: superreview+
Details | Diff | Splinter Review
<nsMsgThread.cpp>, v1 (7.67 KB, patch)
2004-04-23 18:33 PDT, Serge Gautherie (:sgautherie)
neil: review-
Details | Diff | Splinter Review
<*/plugin.cpp> ++, v3 (3.65 KB, patch)
2004-04-30 16:31 PDT, Serge Gautherie (:sgautherie)
no flags Details | Diff | Splinter Review
<nsMsgThread.cpp>, v2 [Checked in: Comment 161] (7.77 KB, patch)
2004-05-01 07:35 PDT, Serge Gautherie (:sgautherie)
mozilla: review+
mozilla: superreview+
Details | Diff | Splinter Review
<nsWinCharset.cpp>, v1-ci (for check in only) [Checked in: Comment 177] (853 bytes, patch)
2004-05-26 17:13 PDT, Serge Gautherie (:sgautherie)
no flags Details | Diff | Splinter Review
<*/plugin.cpp> ++, v3b [Checked in: Comment 180] (3.57 KB, patch)
2004-05-27 10:36 PDT, Serge Gautherie (:sgautherie)
bugzillamozillaorg_serge_20140323: review+
bugzillamozillaorg_serge_20140323: superreview+
Details | Diff | Splinter Review

Description Matthias Bockelkamp 2003-08-23 11:34:10 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5b) Gecko/20030820
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5b) Gecko/20030820

I'll attach a few patches for "warning: unused variable" and "... defined but
not used" in different files using the MinGW compiler.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
Comment 1 Matthias Bockelkamp 2003-08-23 11:36:53 PDT
Created attachment 130273 [details] [diff] [review]
Patch for CNavDTD.cpp
Comment 2 Matthias Bockelkamp 2003-08-23 11:41:13 PDT
Created attachment 130274 [details] [diff] [review]
</modules/plugin/samples/default/windows/dialogs.cpp>
[Checked in: Comment 153]
Comment 3 Matthias Bockelkamp 2003-08-23 11:43:43 PDT
Created attachment 130275 [details] [diff] [review]
</modules/oji/src/jvmmgr.cpp> v1
Comment 4 Matthias Bockelkamp 2003-08-23 11:46:13 PDT
Created attachment 130276 [details] [diff] [review]
Patch for lcglue.cpp
Comment 5 Matthias Bockelkamp 2003-08-23 11:48:01 PDT
Created attachment 130277 [details] [diff] [review]
Patch for ns4xPlugin.cpp
Comment 6 Matthias Bockelkamp 2003-08-23 11:49:08 PDT
Created attachment 130278 [details] [diff] [review]
Patch for nsBoxObject.cpp
Comment 7 Matthias Bockelkamp 2003-08-23 11:50:37 PDT
Created attachment 130280 [details] [diff] [review]
</widget/src/windows/nsClipboard.cpp>
[Checked in: Comment 113]
Comment 8 Matthias Bockelkamp 2003-08-23 11:51:52 PDT
Created attachment 130281 [details] [diff] [review]
</widget/src/windows/nsDataObj.cpp>, v1
Comment 9 Matthias Bockelkamp 2003-08-23 11:53:08 PDT
Created attachment 130283 [details] [diff] [review]
Patch for nsDocumentViewer.cpp
Comment 10 Matthias Bockelkamp 2003-08-23 11:56:09 PDT
Created attachment 130284 [details] [diff] [review]
<nsFileSystemDataSource.cpp>
[Checked in: Comment 77]
Comment 11 Matthias Bockelkamp 2003-08-23 11:58:28 PDT
Created attachment 130286 [details] [diff] [review]
Patch for nsFontMetricsWin.cpp
Comment 12 Matthias Bockelkamp 2003-08-23 11:59:41 PDT
Created attachment 130287 [details] [diff] [review]
</gfx/src/windows/nsGfxFactoryWin.cpp>
Comment 13 Matthias Bockelkamp 2003-08-23 12:01:17 PDT
Created attachment 130288 [details] [diff] [review]
Patch for nsHTMLContentSink.cpp
Comment 14 Matthias Bockelkamp 2003-08-23 12:02:34 PDT
Created attachment 130289 [details] [diff] [review]
Patch for nsImageWin.cpp
Comment 15 Matthias Bockelkamp 2003-08-23 12:04:07 PDT
Created attachment 130290 [details] [diff] [review]
Patch for nsJVMManager.cpp
Comment 16 Matthias Bockelkamp 2003-08-23 12:05:52 PDT
Created attachment 130291 [details] [diff] [review]
</widget/src/windows/nsNativeDragTarget.cpp> [Checked in: Comment 173]
Comment 17 Matthias Bockelkamp 2003-08-23 12:07:18 PDT
Created attachment 130292 [details] [diff] [review]
</gfx/src/windows/nsNativeThemeWin.cpp>
[Checked in: Comment 135]
Comment 18 Matthias Bockelkamp 2003-08-23 12:09:55 PDT
Created attachment 130294 [details] [diff] [review]
Patch for nsOSHelperAppService.cpp
Comment 19 Matthias Bockelkamp 2003-08-23 12:13:57 PDT
Created attachment 130295 [details] [diff] [review]
</modules/plugin/base/src/nsPluginsDirWin.cpp>
[Checked in: Comment 155]
Comment 20 Matthias Bockelkamp 2003-08-23 12:17:51 PDT
Created attachment 130296 [details] [diff] [review]
</gfx/src/windows/nsPrintOptionsWin.cpp>
[Checked in: Comment 114]
Comment 21 Matthias Bockelkamp 2003-08-23 12:20:33 PDT
Created attachment 130297 [details] [diff] [review]
Patch for nsRDFXMLDataSource.cpp
[Checked in: Comment 112]
Comment 22 Matthias Bockelkamp 2003-08-23 12:21:59 PDT
Created attachment 130298 [details] [diff] [review]
Patch for nsScanner.cpp
Comment 23 Matthias Bockelkamp 2003-08-23 12:23:26 PDT
Created attachment 130299 [details] [diff] [review]
Patch for nsScreenManagerWin.cpp
Comment 24 Matthias Bockelkamp 2003-08-23 12:24:22 PDT
Created attachment 130300 [details] [diff] [review]
Patch for nsSelection.cpp
Comment 25 Matthias Bockelkamp 2003-08-23 12:26:00 PDT
Created attachment 130301 [details] [diff] [review]
Patch for nsToolkit.cpp
Comment 26 Matthias Bockelkamp 2003-08-23 12:27:10 PDT
Created attachment 130302 [details] [diff] [review]
<nsWinCharset.cpp>, v1-r (for review only) [Check in: See v1-ci]
Comment 27 Matthias Bockelkamp 2003-08-23 12:28:23 PDT
Created attachment 130303 [details] [diff] [review]
Patch for nsWindow.cpp
Comment 28 Matthias Bockelkamp 2003-08-23 12:29:42 PDT
Created attachment 130304 [details] [diff] [review]
Patch for nsXMLContentSink.cpp
Comment 29 Matthias Bockelkamp 2003-08-23 12:31:50 PDT
Created attachment 130305 [details] [diff] [review]
</layout/xul/base/src/nsXULTooltipListener.cpp>, v1
Comment 30 Matthias Bockelkamp 2003-08-23 12:32:59 PDT
Created attachment 130306 [details] [diff] [review]
</modules/plugin/samples/default/windows/plugin.cpp> v1
Comment 31 Brian Ryner (not reading) 2003-08-23 13:37:00 PDT
Comment on attachment 130305 [details] [diff] [review]
</layout/xul/base/src/nsXULTooltipListener.cpp>, v1

There's a call to this function in nsXULTooltipListener::LaunchTooltip, #ifdef
DEBUG_crap.  I'm not quite sure what the status is of that #ifdef... jag, you
tweaked it a bit, are you using it for something?
Comment 32 Brian Ryner (not reading) 2003-08-23 13:44:22 PDT
Comment on attachment 130278 [details] [diff] [review]
Patch for nsBoxObject.cpp

Uh, no.  You can't just remove the call to GetPrimaryFrameFor, as it fills in
|frame|.  Just remove the "nsresult rv = " instead.
Comment 33 Matthias Bockelkamp 2003-08-23 15:05:22 PDT
Created attachment 130314 [details] [diff] [review]
Patch for nsBoxObject.cpp, v2
Comment 34 Matthias Versen [:Matti] 2003-08-23 16:29:24 PDT
-> mbockelkamp@web.de 
Comment 35 timeless 2003-08-23 21:19:28 PDT
please stop for a while.
Comment 36 timeless 2003-08-23 21:21:18 PDT
Comment on attachment 130275 [details] [diff] [review]
</modules/oji/src/jvmmgr.cpp> v1

Functions have side effects, in this case GetRunningJVM() is *responsible* for
starting the jvm. so you're making the startupjvm function never start a jvm.
Comment 37 timeless 2003-08-23 21:46:12 PDT
Comment on attachment 130277 [details] [diff] [review]
Patch for ns4xPlugin.cpp

this sure looks like an exported function. don't go killing things that could
be used externally.
Comment 38 timeless 2003-08-23 22:03:58 PDT
Comment on attachment 130280 [details] [diff] [review]
</widget/src/windows/nsClipboard.cpp>
[Checked in: Comment 113]

fwiw pinkerton removed the consumer in rev 1.53
Comment 39 timeless 2003-08-23 23:03:31 PDT
Comment on attachment 130304 [details] [diff] [review]
Patch for nsXMLContentSink.cpp

look at the patch, see how ref is used two statements later?
Comment 40 timeless 2003-08-23 23:06:49 PDT
Comment on attachment 130306 [details] [diff] [review]
</modules/plugin/samples/default/windows/plugin.cpp> v1

well, i wouldn't mind killing plugin finder, but this isn't the way to do it.
Comment 41 timeless 2003-08-23 23:16:29 PDT
he future, please use cvs to generate diffs, cvs diff -up5 or something would be
good. Ideally for small functions you should diff enough to show that a local
variable really isn't used.

also if you're going to split patches up, you should pick people who have done
stuff in the areas recently.

however some people think you should put together a single patch and get a big
overall review.

i'm not sure i agree, since i think that only a few people would recognize the
damage your patches would cause. unfortunately they aren't the people you
selected for reviews.
Comment 42 Matthias Bockelkamp 2003-08-24 04:47:23 PDT
Created attachment 130327 [details] [diff] [review]
</modules/oji/src/jvmmgr.cpp> v2
[Checked in: Comment 166]
Comment 43 Matthias Bockelkamp 2003-08-24 04:50:04 PDT
Created attachment 130328 [details] [diff] [review]
Patch for nsXMLContentSink.cpp, v2
Comment 44 Matthias Bockelkamp 2003-08-24 04:52:00 PDT
Created attachment 130329 [details] [diff] [review]
<plugin.cpp>, v2
Comment 45 timeless 2003-08-24 09:40:50 PDT
http://www.mozilla.org/hacking/reviewers.html lists people who are srs and the
areas in which they specialize, i'm not on the list, so i can't sr.
Comment 46 timeless 2003-08-24 11:09:20 PDT
Comment on attachment 130274 [details] [diff] [review]
</modules/plugin/samples/default/windows/dialogs.cpp>
[Checked in: Comment 153]

this change should be made to both windows and os2 plugins.
Comment 47 Matthias Bockelkamp 2003-08-24 11:46:15 PDT
Created attachment 130332 [details] [diff] [review]
<os2/dialogs.cpp>
[Checked in: Comment 107]
Comment 48 timeless 2003-08-24 12:09:49 PDT
Comment on attachment 130287 [details] [diff] [review]
</gfx/src/windows/nsGfxFactoryWin.cpp>

personally i think this is the wrong fix.

hyatt: why do you need NS_NewNativeTheme? why not use the generic factory
constructor.

i'd rather get rid of the extra exported function and use the normal factory
system.
Comment 49 timeless 2003-08-24 12:17:43 PDT
Comment on attachment 130288 [details] [diff] [review]
Patch for nsHTMLContentSink.cpp

this is paired with code in 4093-4102

if you're going to mess with the code please locate pairs and deal equally with
them.
Comment 50 timeless 2003-08-24 12:34:29 PDT
Comment on attachment 130290 [details] [diff] [review]
Patch for nsJVMManager.cpp

#include <stdio.h>
int a(char c,int d){
printf("hello world [%c:%d]\n",c,d);return 1;
}
int main(void){
for(int i=0;i<5;i++)
{static int y=a('s',i);int z=a('!',i);}
return 0;
}

compile this c++ program. you're changing the code from 's' to '!'.
Comment 51 Matthias Bockelkamp 2003-08-24 12:53:46 PDT
Created attachment 130336 [details] [diff] [review]
Patch for nsHTMLContentSink.cpp, v2
Comment 52 Matthias Bockelkamp 2003-08-24 13:11:18 PDT
Comment on attachment 130290 [details] [diff] [review]
Patch for nsJVMManager.cpp

> compile this c++ program. you're changing the code from 's' to '!'.

OK. I didn't understand it until I saw its output.
Comment 53 timeless 2003-08-24 13:16:26 PDT
Comment on attachment 130292 [details] [diff] [review]
</gfx/src/windows/nsNativeThemeWin.cpp>
[Checked in: Comment 135]

i can't imagine that we would want to remove the prototypes.

for my reference because you didn't include enough context, isDisabled probably
was meant to store IsDisabled(aFrame). since it is just used inline w/ ifs it
makes sense to remove the PRBools.
Comment 54 timeless 2003-08-24 13:26:41 PDT
Comment on attachment 130295 [details] [diff] [review]
</modules/plugin/base/src/nsPluginsDirWin.cpp>
[Checked in: Comment 155]

note to self: info.fName in the beos version mismatches allocator/deallocators
(for some paths)
Comment 55 timeless 2003-08-24 13:35:05 PDT
Comment on attachment 130296 [details] [diff] [review]
</gfx/src/windows/nsPrintOptionsWin.cpp>
[Checked in: Comment 114]

note to self: that assertion is bogus, it should be replaced with:
if (!printSettings) return NS_ERROR_OUT_OF_MEMORY;
(for all the impls)
Comment 56 timeless 2003-08-24 13:42:56 PDT
Comment on attachment 130297 [details] [diff] [review]
Patch for nsRDFXMLDataSource.cpp
[Checked in: Comment 112]

RDFXMLDataSourceImpl::rdfXMLFlush
Comment 57 timeless 2003-08-24 13:44:12 PDT
Comment on attachment 130297 [details] [diff] [review]
Patch for nsRDFXMLDataSource.cpp
[Checked in: Comment 112]

RDFXMLDataSourceImpl::rdfXMLFlush
Comment 58 timeless 2003-08-24 13:58:01 PDT
Comment on attachment 130298 [details] [diff] [review]
Patch for nsScanner.cpp

Peek() changes theChar, you can't not call it.
Comment 59 timeless 2003-08-24 14:01:35 PDT
Comment on attachment 130299 [details] [diff] [review]
Patch for nsScreenManagerWin.cpp

how about making the code work w/ mingw instead?
Comment 60 timeless 2003-08-24 15:11:28 PDT
Comment on attachment 130302 [details] [diff] [review]
<nsWinCharset.cpp>, v1-r (for review only) [Check in: See v1-ci]

i'm not certain the rv should be ignored...
Comment 61 timeless 2003-08-24 15:25:44 PDT
Comment on attachment 130303 [details] [diff] [review]
Patch for nsWindow.cpp

are we supposed to do something w/ the HCURSOR/HPALETTE from
SetCursor/SelectPalette?
Comment 62 Johnny Stenback (:jst, jst@mozilla.com) 2003-08-24 16:25:30 PDT
Comment on attachment 130276 [details] [diff] [review]
Patch for lcglue.cpp

r+sr=jst
Comment 63 timeless 2003-08-24 17:40:50 PDT
Comment on attachment 130336 [details] [diff] [review]
Patch for nsHTMLContentSink.cpp, v2

i don't know if someone might want this code.
Comment 64 Matthias Bockelkamp 2003-08-24 22:10:56 PDT
Created attachment 130349 [details] [diff] [review]
Patch for nsScanner.cpp, v2
Comment 65 Simon Montagu :smontagu 2003-08-24 22:17:38 PDT
Comment on attachment 130302 [details] [diff] [review]
<nsWinCharset.cpp>, v1-r (for review only) [Check in: See v1-ci]

This looks OK to me: in all cases where a failure code is returned by
MapToCharset() a sensible default is set.

r=smontagu
Comment 66 Johnny Stenback (:jst, jst@mozilla.com) 2003-08-24 22:21:40 PDT
Comment on attachment 130336 [details] [diff] [review]
Patch for nsHTMLContentSink.cpp, v2

r+sr=jst
Comment 67 Johnny Stenback (:jst, jst@mozilla.com) 2003-08-24 22:27:24 PDT
Comment on attachment 130349 [details] [diff] [review]
Patch for nsScanner.cpp, v2

r+sr=jst
Comment 68 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2003-08-24 22:43:11 PDT
Comment on attachment 130283 [details] [diff] [review]
Patch for nsDocumentViewer.cpp

sr=dbaron if you remove the 

#ifdef DEBUG
#defined EXTENDED_DEBUG_PRINTING
#endif

as well, since that should now be in a header file (although it happens to be
copied into two other cpp files instead) -- but in any case it's not needed
here.
Comment 69 Matthias Bockelkamp 2003-08-24 22:51:12 PDT
Created attachment 130352 [details] [diff] [review]
Patch for nsScreenManagerWin.cpp, v2

It compiles fine, but I cannot test it with my single monitor equipment.
Comment 70 Matthias Bockelkamp 2003-08-24 23:04:16 PDT
Created attachment 130353 [details] [diff] [review]
Patch for nsDocumentViewer.cpp, v2

Changed as dbaron said.
r=timeless, sr=dbaron from previous patch.
Comment 71 rbs 2003-08-25 00:23:07 PDT
Comment on attachment 130286 [details] [diff] [review]
Patch for nsFontMetricsWin.cpp

r+sr=rbs
Comment 72 Ere Maijala (slow) 2003-08-25 05:24:02 PDT
Comment on attachment 130303 [details] [diff] [review]
Patch for nsWindow.cpp

I think it's ok not to care about the results of SetCursor/SelectPalette, but
nsServiceManager::GetService definitely needs error checking. Although the
return value wasn't checked before either, I don't like it being completely
disregarded.
Comment 73 Christopher Aillon (sabbatical, not receiving bugmail) 2003-08-25 06:14:27 PDT
Comment on attachment 130300 [details] [diff] [review]
Patch for nsSelection.cpp

Note that jst has this fixed as part of his patch for bug 215891.
Comment 74 Christopher Aillon (sabbatical, not receiving bugmail) 2003-08-25 06:57:35 PDT
I meant bug 215981.
Comment 75 Mike Kaply [:mkaply] 2003-08-25 08:00:54 PDT
Comment on attachment 130332 [details] [diff] [review]
<os2/dialogs.cpp>
[Checked in: Comment 107]

r=mkaply
sr=blizzard (platform specific code)
Comment 76 jag (Peter Annema) 2003-08-25 09:23:43 PDT
Comment on attachment 130305 [details] [diff] [review]
</layout/xul/base/src/nsXULTooltipListener.cpp>, v1

That's the code we'll need for making tooltips for truncated text in trees
become titletips. *shrug*, I guess it can be removed, bonsai will remember it
for us if/when we fix that.
Comment 77 Alec Flett 2003-08-26 07:10:50 PDT
Comment on attachment 130284 [details] [diff] [review]
<nsFileSystemDataSource.cpp>
[Checked in: Comment 77]

sr=alecf
Comment 78 Christopher Blizzard (:blizzard) 2003-09-03 12:04:56 PDT
Why do you want to remove some of the old MSVC support?  Have we dropped support
for those older compilers?
Comment 79 Matthias Bockelkamp 2003-09-07 12:32:22 PDT
Created attachment 131036 [details] [diff] [review]
Patch for nsScreenManagerWin.cpp, v3
Comment 80 timeless 2003-09-09 23:24:06 PDT
Comment on attachment 131036 [details] [diff] [review]
Patch for nsScreenManagerWin.cpp, v3

>-#if _MSC_VER >= 1200
>+#if defined(__MINGW32__) || _MSC_VER >= 1200
> typedef HMONITOR (WINAPI *MonitorFromRectProc)(LPCRECT inRect, DWORD inFlag); 
> typedef BOOL (WINAPI *EnumDisplayMonitorsProc)(HDC, LPCRECT, MONITORENUMPROC, LPARAM);
> 
> BOOL CALLBACK CountMonitors ( HMONITOR, HDC, LPRECT, LPARAM ioCount ) ;
this change isn't a correct transformation:
>-#elif !defined(__MINGW32__)
>-typedef void* HMONITOR;
> #endif

you want
#else
Comment 81 Matthias Bockelkamp 2003-09-11 06:48:47 PDT
Created attachment 131230 [details] [diff] [review]
Patch for nsScreenManagerWin.cpp, v4
Comment 82 timeless 2003-09-11 08:09:56 PDT
Comment on attachment 131230 [details] [diff] [review]
Patch for nsScreenManagerWin.cpp, v4

>-#elif !defined(__MINGW32__)
>+#elif
#elif expects a condition, this should just be "#else"
Comment 83 Matthias Bockelkamp 2003-09-11 08:20:26 PDT
Created attachment 131244 [details] [diff] [review]
Patch for nsScreenManagerWin.cpp, v5
Comment 84 Christian :Biesinger (don't email me, ping me on IRC) 2003-09-14 13:07:42 PDT
Comment on attachment 130294 [details] [diff] [review]
Patch for nsOSHelperAppService.cpp

this patch is checked in:
09/10/2003 20:18	timeless%mozdev.org	mozilla/ uriloader/ exthandler/
win/ nsOSHelperAppService.cpp	1.42	0/2	Bug 217089 Compiler warnings
patch by mbockelkamp@web.de r=timeless sr=darin
Comment 85 timeless 2003-09-15 15:02:39 PDT
Comment on attachment 131244 [details] [diff] [review]
Patch for nsScreenManagerWin.cpp, v5

>@@ -225,15 +225,15 @@
> nsScreenManagerWin :: GetNumberOfScreens(PRUint32 *aNumberOfScreens)
(EnumDisplayMonitorsProc)mEnumDisplayMonitorsProc;
>-      BOOL result = (*proc)(nsnull, nsnull, (MONITORENUMPROC)CountMonitors, (LPARAM)&count);
>+      (*proc)(nsnull, nsnull, (MONITORENUMPROC)CountMonitors, (LPARAM)&count);
>       *aNumberOfScreens = mNumberOfScreens = count;      

I'm very uncomfortable using a value in a failure case. the winapi doesn't
suggest how it might fail, but does of course allow it to fail. mozilla xpcom
is the same in that respect and for xpcom failures you aren't supposed to use 
values if the function failed...
Comment 86 Ere Maijala (slow) 2003-09-16 12:50:59 PDT
Comment on attachment 131244 [details] [diff] [review]
Patch for nsScreenManagerWin.cpp, v5

Timeless is right. The function could fail and the correct fix is to check the
return value and act accordingly.
Comment 87 Mike Pinkerton (not reading bugmail) 2003-10-21 21:40:48 PDT
Comment on attachment 130280 [details] [diff] [review]
</widget/src/windows/nsClipboard.cpp>
[Checked in: Comment 113]

if you say this isn't used, i guess i believe you

r=pink
Comment 88 kinmoz 2003-11-23 00:27:44 PST
Comment on attachment 130296 [details] [diff] [review]
</gfx/src/windows/nsPrintOptionsWin.cpp>
[Checked in: Comment 114]

sr=kinmoz@netscape.net
Comment 89 Serge Gautherie (:sgautherie) 2003-12-14 08:48:34 PST
*** Bug 211429 has been marked as a duplicate of this bug. ***
Comment 90 Serge Gautherie (:sgautherie) 2003-12-17 08:31:22 PST
Comment on attachment 130280 [details] [diff] [review]
</widget/src/windows/nsClipboard.cpp>
[Checked in: Comment 113]


'approval1.6=?': Trivial unused code removal.
(if not too late)
Comment 91 Serge Gautherie (:sgautherie) 2003-12-17 08:36:11 PST
Comment on attachment 130296 [details] [diff] [review]
</gfx/src/windows/nsPrintOptionsWin.cpp>
[Checked in: Comment 114]


'approval1.6=?': Trivial unused code removal.
(if not too late)

Note: even if it's not in the "Blamed Build Warnings" page :-<
Comment 92 Serge Gautherie (:sgautherie) 2003-12-17 08:39:55 PST
Comment on attachment 130302 [details] [diff] [review]
<nsWinCharset.cpp>, v1-r (for review only) [Check in: See v1-ci]


This patch is out of sync.: needs an update.

Note: even if it's not in the "Blamed Build Warnings" page :-<
Comment 93 Serge Gautherie (:sgautherie) 2003-12-17 08:42:31 PST
Comment on attachment 130305 [details] [diff] [review]
</layout/xul/base/src/nsXULTooltipListener.cpp>, v1


This patch is out of sync.: needs an update.

Note: even if it's not in the "Blamed Build Warnings" page :-<
Comment 94 Serge Gautherie (:sgautherie) 2003-12-17 08:48:16 PST
Comment on attachment 130314 [details] [diff] [review]
Patch for nsBoxObject.cpp, v2


This patch was included in attachment 131865 [details] [diff] [review] of bug 219908 :-)
Comment 95 Serge Gautherie (:sgautherie) 2003-12-17 08:55:49 PST
Comment on attachment 130328 [details] [diff] [review]
Patch for nsXMLContentSink.cpp, v2


This was fixed in v1.288,
apparently from bug 215981 :-)
Comment 96 Serge Gautherie (:sgautherie) 2003-12-17 09:19:35 PST
Comment on attachment 130353 [details] [diff] [review]
Patch for nsDocumentViewer.cpp, v2


Superseeded by attachment 134374 [details] [diff] [review] in bug 224018 :-)
Comment 97 tor 2003-12-18 08:57:35 PST
Comment on attachment 130280 [details] [diff] [review]
</widget/src/windows/nsClipboard.cpp>
[Checked in: Comment 113]

Too late in the 1.6 timeframe for changes that aren't serious
regressions, security, or crash related.
Comment 98 tor 2003-12-18 08:57:43 PST
Comment on attachment 130296 [details] [diff] [review]
</gfx/src/windows/nsPrintOptionsWin.cpp>
[Checked in: Comment 114]

Too late in the 1.6 timeframe for changes that aren't serious
regressions, security, or crash related.
Comment 99 Serge Gautherie (:sgautherie) 2003-12-20 14:51:51 PST
Comment on attachment 130280 [details] [diff] [review]
</widget/src/windows/nsClipboard.cpp>
[Checked in: Comment 113]


matthias (or anyone):
Can you check in your r+/sr+ patch ? Thanks.
Comment 100 Serge Gautherie (:sgautherie) 2003-12-20 14:54:03 PST
Comment on attachment 130296 [details] [diff] [review]
</gfx/src/windows/nsPrintOptionsWin.cpp>
[Checked in: Comment 114]


Matthias (or anyone):
Can you check in your r+/sr+ patch ? Thanks.
Comment 101 Serge Gautherie (:sgautherie) 2003-12-20 15:51:52 PST
Re comment 55:

MXR search for "NS_ASSERTION\(printSettings":

timeless:
Would you like me to prepare a patch to replace theses 2
{
/gfx/src/nsPrintOptionsImpl.cpp, line 859 -- NS_ASSERTION(printSettings, "Can't
be NULL!");
/gfx/src/windows/nsPrintOptionsWin.cpp, line 68 -- NS_ASSERTION(printSettings,
"Can't be NULL!");
}
by
|if (!printSettings) return NS_ERROR_OUT_OF_MEMORY;|
?

There are theses 4 also, but they are not returning a |nsresult|, then I don't know
{
/embedding/qa/mozembed/src/mozEmbed.cpp, line 663 -- NS_ASSERTION(printSettings,
"You can't PrintPreview without a PrintSettings!");
/embedding/tests/os2Embed/os2Embed.cpp, line 673 -- NS_ASSERTION(printSettings,
"You can't PrintPreview without a PrintSettings!");
/embedding/tests/winEmbed/winEmbed.cpp, line 624 -- NS_ASSERTION(printSettings,
"You can't PrintPreview without a PrintSettings!");
/content/base/src/nsDocumentViewer.cpp, line 2152 -- NS_ASSERTION(printSettings,
"You can't PrintPreview without a PrintSettings!");
}
Comment 102 Serge Gautherie (:sgautherie) 2003-12-20 16:40:33 PST
Created attachment 137772 [details] [diff] [review]
<nsScreenManagerWin.*> (v6)
[+/- Checked in: Comment 110]

This is v5, with comment 85 suggestions.
Comment 103 Serge Gautherie (:sgautherie) 2003-12-20 16:43:50 PST
Comment on attachment 137772 [details] [diff] [review]
<nsScreenManagerWin.*> (v6)
[+/- Checked in: Comment 110]


'r=?':
I have no compiler: Could you compile/test/review it ? Thanks.


The only call to |nsScreenManagerWin::GetNumberOfScreens()| is at
<http://landfill.bugzilla.org/mxr-test/seamonkey/source/gfx/src/windows/nsDevic
eContextWin.cpp#189>.
Should something be done there too ? (What ?)
Comment 104 Ere Maijala (slow) 2003-12-22 11:00:56 PST
Comment on attachment 137772 [details] [diff] [review]
<nsScreenManagerWin.*> (v6)
[+/- Checked in: Comment 110]

I don't have MinGW so I'll just rely on them being correct. Otherwise ok. r=ere
Comment 105 rbs 2003-12-22 16:34:17 PST
Comment on attachment 137772 [details] [diff] [review]
<nsScreenManagerWin.*> (v6)
[+/- Checked in: Comment 110]

sr=rbs
Comment 106 Serge Gautherie (:sgautherie) 2003-12-23 15:48:41 PST
Comment on attachment 130274 [details] [diff] [review]
</modules/plugin/samples/default/windows/dialogs.cpp>
[Checked in: Comment 153]

Re comment 46:

timeless:
What do you mean by "this change should be made to both windows and os2
plugins." ?

MXR result:
{
szDefaultPluginFinderURL
Defined as a variable in:

    * modules/plugin/samples/default/os2/plugin.cpp, line 54
    * modules/plugin/samples/default/windows/dialogs.cpp, line 47
    * modules/plugin/samples/default/windows/plugin.cpp, line 57 

Referenced (in 3 files total) in:

    * modules/plugin/samples/default/os2/plugin.cpp:
	  o View change log or Blame annotations line 54
	  o line 324 
    * modules/plugin/samples/default/windows/dialogs.cpp:
	  o View change log or Blame annotations line 47 
    * modules/plugin/samples/default/windows/plugin.cpp:
	  o View change log or Blame annotations line 57
	  o line 369 
}
Comment 107 Serge Gautherie (:sgautherie) 2003-12-23 15:54:07 PST
Comment on attachment 130332 [details] [diff] [review]
<os2/dialogs.cpp>
[Checked in: Comment 107]


Check in: { 1.5 timeless%mozdev.org	Sep 10 20:18 }
Comment 108 Serge Gautherie (:sgautherie) 2003-12-23 15:59:54 PST
Comment on attachment 130274 [details] [diff] [review]
</modules/plugin/samples/default/windows/dialogs.cpp>
[Checked in: Comment 153]

Disregard comment 106:

I read "dialogs.cpp" where it is written "plugin.cpp" :-<
(I believe the 2 plugin.cpp files are correct as they are.)
Comment 109 Serge Gautherie (:sgautherie) 2003-12-23 16:18:45 PST
Updating:
+(CC) brendan

I'm changing r?/sr? from beard#netscape.com to you,
since the owner and his 3 peers have @netscape.com email in <owners.html>,
and I don't know to who/what this 'modules/plugin' fits in <reviewers.html>.

Can you help to find a (super-)reviewer ?

Thanks.
Comment 110 Serge Gautherie (:sgautherie) 2003-12-24 16:29:11 PST
Comment on attachment 137772 [details] [diff] [review]
<nsScreenManagerWin.*> (v6)
[+/- Checked in: Comment 110]


Check in: { 12/24/2003 15:45	neil%parkwaycc.co.uk	nsScreenManagerWin.cpp 
1.16 }
"I only checked in nsScreenManagerWin.cpp because nsScreenManagerWin.h change
gave me an *extra* warning..."
Comment 111 Chase Tingley 2004-01-14 20:49:05 PST
*** Bug 198785 has been marked as a duplicate of this bug. ***
Comment 112 Serge Gautherie (:sgautherie) 2004-01-15 04:55:15 PST
Comment on attachment 130297 [details] [diff] [review]
Patch for nsRDFXMLDataSource.cpp
[Checked in: Comment 112]


Check in: { 1.145	timeless%mozdev.org	Sep 10 2003 }
Comment 113 Boris Zbarsky [:bz] 2004-01-17 12:11:26 PST
Comment on attachment 130280 [details] [diff] [review]
</widget/src/windows/nsClipboard.cpp>
[Checked in: Comment 113]

Checking in nsClipboard.cpp;
/cvsroot/mozilla/widget/src/windows/nsClipboard.cpp,v  <--  nsClipboard.cpp
new revision: 1.86; previous revision: 1.85

Next time, please for the love of God use cvs diff or at least diff from
toplevel -- there are 8 different nsClipboard.cpp files in the tree, and _how_
do you expect people to figure out which one to apply your patch to?
Comment 114 Boris Zbarsky [:bz] 2004-01-17 20:40:59 PST
Comment on attachment 130296 [details] [diff] [review]
</gfx/src/windows/nsPrintOptionsWin.cpp>
[Checked in: Comment 114]

Checking in nsPrintOptionsWin.cpp;
/cvsroot/mozilla/gfx/src/windows/nsPrintOptionsWin.cpp,v  <-- 
nsPrintOptionsWin.cpp
new revision: 1.8; previous revision: 1.7
Comment 115 Serge Gautherie (:sgautherie) 2004-01-19 14:47:39 PST
Comment on attachment 130305 [details] [diff] [review]
</layout/xul/base/src/nsXULTooltipListener.cpp>, v1


This is what I asked jag
{
> I guess the warning used to be "defined but not used", but this is no longer the case !!?
> See
> http://bugzilla.mozilla.org/show_bug.cgi?id=219908
> http://bugzilla.mozilla.org/attachment.cgi?id=131865&action=view
> with "fixed" the warning (which appeared in v1.23) in v1.28
}

And his answer was
{ (1/2)
I think I'd rather keep it in there
}
Comment 116 Serge Gautherie (:sgautherie) 2004-01-19 14:58:14 PST
Created attachment 139448 [details] [diff] [review]
</layout/xul/base/src/nsXULTooltipListener.cpp>, v2
[Checked in: Comment 119]

{ (2/2)
perhaps adding a comment saying "this stuff inside DEBUG_crap could be used to
make tree tooltips work in the future."
}
Comment 117 Serge Gautherie (:sgautherie) 2004-01-19 14:59:56 PST
Comment on attachment 139448 [details] [diff] [review]
</layout/xul/base/src/nsXULTooltipListener.cpp>, v2
[Checked in: Comment 119]

'(s)r=?': (see comment 116)
I have no compiler: Could you compile/test/review it ? Thanks.
Comment 118 jag (Peter Annema) 2004-01-19 16:30:28 PST
Comment on attachment 139448 [details] [diff] [review]
</layout/xul/base/src/nsXULTooltipListener.cpp>, v2
[Checked in: Comment 119]

sr=jag
Comment 119 Serge Gautherie (:sgautherie) 2004-01-22 13:50:35 PST
Comment on attachment 139448 [details] [diff] [review]
</layout/xul/base/src/nsXULTooltipListener.cpp>, v2
[Checked in: Comment 119]


Check in: { 01/22/2004 08:14	neil%parkwaycc.co.uk	mozilla/ layout/ xul/
base/ src/ nsXULTooltipListener.cpp	  1.32 }
Comment 120 Serge Gautherie (:sgautherie) 2004-04-20 14:07:46 PDT
Comment on attachment 130287 [details] [diff] [review]
</gfx/src/windows/nsGfxFactoryWin.cpp>


No (super-)review from <hyatt@mozilla.org> since '2003-08-24' :-(

Also, see comment 48 !
Comment 121 Serge Gautherie (:sgautherie) 2004-04-20 14:11:13 PDT
Comment on attachment 130292 [details] [diff] [review]
</gfx/src/windows/nsNativeThemeWin.cpp>
[Checked in: Comment 135]


No review from <tim@prismelite.com>, no super-review from <hyatt@mozilla.org>,
since '2003-08-24' :-(

Also, see comment 53 !
Comment 122 Serge Gautherie (:sgautherie) 2004-04-20 14:13:58 PDT
Comment on attachment 130281 [details] [diff] [review]
</widget/src/windows/nsDataObj.cpp>, v1


No (super-)review from <blizzard@mozilla.org> since '2003-08-23' :-(
Comment 123 Serge Gautherie (:sgautherie) 2004-04-20 14:21:52 PDT
Comment on attachment 130302 [details] [diff] [review]
<nsWinCharset.cpp>, v1-r (for review only) [Check in: See v1-ci]


No super-review from <blizzard@mozilla.org> since '2003-08-23' :-(

Also, see comment 92 !
(If sr+, I'll post an updated patch for check-in.)
Comment 124 Serge Gautherie (:sgautherie) 2004-04-20 14:27:20 PDT
Comment on attachment 130327 [details] [diff] [review]
</modules/oji/src/jvmmgr.cpp> v2
[Checked in: Comment 166]


No (super-)review from <scc@mozilla.org> since '2003-08-24' :-(
Comment 125 Serge Gautherie (:sgautherie) 2004-04-20 14:34:38 PDT
Comment on attachment 130329 [details] [diff] [review]
<plugin.cpp>, v2

No (super-)review from <brendan@mozilla.org> since '2003-12-23' :-(
Comment 126 Serge Gautherie (:sgautherie) 2004-04-20 14:37:51 PDT
Comment on attachment 130295 [details] [diff] [review]
</modules/plugin/base/src/nsPluginsDirWin.cpp>
[Checked in: Comment 155]


No super-review from <brendan@mozilla.org> since '2003-12-23' :-(
Comment 127 Serge Gautherie (:sgautherie) 2004-04-20 14:39:30 PDT
Comment on attachment 130274 [details] [diff] [review]
</modules/plugin/samples/default/windows/dialogs.cpp>
[Checked in: Comment 153]


No super-review from <brendan@mozilla.org> since '2003-12-23' :-(

Also, see comment 108 !
Comment 128 Brendan Eich [:brendan] 2004-04-20 15:21:14 PDT
Serge, if you are removing unused code, and the module owner or peer stamps his
or her r= on your patch, and someone tests it, then you can assume
rs=brendan@mozilla.org in lieu of sr=.

But attachment 130329 [details] [diff] [review] gets rid of set-once-never-used variables, when I would
rather see a change to check for errors, if errors are possible (they seem to be
in this code).  Don't just paper over warnings; if you are going to touch the
file, you may as well improve it substantively.

/be
Comment 129 Serge Gautherie (:sgautherie) 2004-04-20 16:03:19 PDT
(In reply to comment #128)
> Serge, if you are removing unused code, and the module owner or peer stamps his
> or her r= on your patch, and someone tests it, then you can assume
> rs=brendan@mozilla.org in lieu of sr=.

I'm not familiar with |rs=| ... I guess it means that no super-review is needed !?

> But attachment 130329 [details] [diff] [review] gets rid of set-once-never-used variables, when I would
> rather see a change to check for errors, if errors are possible (they seem to be
> in this code).  Don't just paper over warnings; if you are going to touch the
> file, you may as well improve it substantively.

I agree, and do it as much as I can;
In these cases, the patches are not mine (untill I would take and modify them);
I'm only trying to get them dealt with, as a first step:
depending on the reviewer comment, I might either update them myself, or ask for
 helpwanted...


Matthias Bockelkamp:
Can/Will you check in the patches which should be (current / future) ?
Or should I do it "myself" ? (Let's say this is the default answer after the
next 7 days...)
Comment 130 Brendan Eich [:brendan] 2004-04-20 16:11:59 PDT
Serge: rs= means "rubber-stamp=", amusingly inverting the sr= "super-review="
letter order.  Don't abuse it -- use it only with module owner or peer review,
only with testing, and only for changes that remove dead code.

In that light, you have rs= on attachment 130329 [details] [diff] [review] as well, but I'd appreciate it
if you could try to get a followup fix to check for errors done.  If that's
beyond what you're willing or able to do, please help find an owner or peer, or
a new peer, who can help.  (Any patch that adds error checks would need real
sr=, as usual.)  Thanks,

/be
Comment 131 rbs 2004-04-20 23:04:24 PDT
Comment on attachment 130287 [details] [diff] [review]
</gfx/src/windows/nsGfxFactoryWin.cpp>

r+sr=rbs
Comment 132 rbs 2004-04-20 23:05:00 PDT
Comment on attachment 130292 [details] [diff] [review]
</gfx/src/windows/nsNativeThemeWin.cpp>
[Checked in: Comment 135]

r+sr=rbs
Comment 133 Matthias Bockelkamp 2004-04-21 05:49:41 PDT
(In reply to comment #129)
> Can/Will you check in the patches which should be (current / future) ?
> Or should I do it "myself" ? (Let's say this is the default answer after the
> next 7 days...)

I cannot check anything in. If you have the necessary rights, please check it in.

(In reply to comment #130)
> In that light, you have rs= on attachment 130329 [details] [diff] [review] as well, but I'd appreciate it
> if you could try to get a followup fix to check for errors done.  If that's

I thought about something like:

int iSize = LoadString(m_hInst, IDS_GOING2HTML, buf, sizeof(buf));

NPError error = NPN_NewStream(m_pNPInstance, "text/html", "asd_plugin_finder",
&pStream);

if (error == NPERR_NO_ERROR) {

  //char buf[] = "<html>\n<body>\n\n<h2 align=center>NPN_NewStream / NPN_Write -
This seems to work.</h2>\n\n</body>\n</html>";

  int32 iBytes = NPN_Write(m_pNPInstance, pStream, iSize, buf);  // iSize was
lstrlen(buf) before!!!
  
  NPN_DestroyStream(m_pNPInstance, pStream, NPRES_DONE);
  
}  

Two questions, since I'm not familar with the plugin code:
- What should happen if error!=NPERR_NO_ERROR?
- What should happen if after NPN_Write iSize!=iBytes?
- Should CPlugin::URLNotify perhaps be changed from void to NPError?
Comment 134 Serge Gautherie (:sgautherie) 2004-04-21 07:48:56 PDT
(In reply to comment #131)
> (From update of attachment 130287 [details] [diff] [review])
> r+sr=rbs

I'd like to have your comment on comment 48 before proceeding with check-in.
Thanks.
Comment 135 Serge Gautherie (:sgautherie) 2004-04-21 09:43:16 PDT
Comment on attachment 130292 [details] [diff] [review]
</gfx/src/windows/nsNativeThemeWin.cpp>
[Checked in: Comment 135]


Check in: { 2004-04-21 08:34	neil%parkwaycc.co.uk	mozilla/ gfx/ src/
windows/ nsNativeThemeWin.cpp	     3.38    0/8     Removing unused variables
b=130292 p=mbockelkamp@web.de r/sr=rbs }
Comment 136 Serge Gautherie (:sgautherie) 2004-04-21 09:50:25 PDT
Comment on attachment 130327 [details] [diff] [review]
</modules/oji/src/jvmmgr.cpp> v2
[Checked in: Comment 166]


Resurrect sr=?:
I have no compiler, neil (my check-in partner) has no jvm;
Or can someone test this ? (kyle ?)
Comment 137 rbs 2004-04-21 15:39:01 PDT
> I'd like to have your comment on comment 48

I also prefer using the generic constructor, unless some special-casing prevents
otherwise. See nsGfxFactoryGTK/Mac. I hadn't see that comment and assumed that
you guys did your homework properly while waiting all this time. Don't just toss
review requests in the air. Seeing '-' lines is tempting.
Comment 138 Ere Maijala (slow) 2004-04-22 11:17:41 PDT
Comment on attachment 130281 [details] [diff] [review]
</widget/src/windows/nsDataObj.cpp>, v1

r=ere
Comment 139 Serge Gautherie (:sgautherie) 2004-04-22 15:31:00 PDT
Created attachment 146811 [details] [diff] [review]
</widget/src/windows/nsDataObj.cpp>, v1b
[Checked in: Comment 142]

v1,
with an additional |status| removal.
Comment 140 Serge Gautherie (:sgautherie) 2004-04-22 15:35:05 PDT
Comment on attachment 146811 [details] [diff] [review]
</widget/src/windows/nsDataObj.cpp>, v1b
[Checked in: Comment 142]


I have no compiler: Could you compile/test/review this patch ? Thanks.

Sorry for asking a second time:
the first patch was not mine; (and may be MinGW did not complain)
Now, I ran an LXR search on |GlobalUnlock| :-)
Comment 141 Ere Maijala (slow) 2004-04-22 22:24:20 PDT
Comment on attachment 146811 [details] [diff] [review]
</widget/src/windows/nsDataObj.cpp>, v1b
[Checked in: Comment 142]

Compiles, works, r=ere
Comment 142 Serge Gautherie (:sgautherie) 2004-04-23 11:32:30 PDT
Comment on attachment 146811 [details] [diff] [review]
</widget/src/windows/nsDataObj.cpp>, v1b
[Checked in: Comment 142]


Check in: { 2004-04-23 02:01	neil%parkwaycc.co.uk	mozilla/ widget/ src/
windows/ nsDataObj.cpp	  1.61 }
Comment 143 Serge Gautherie (:sgautherie) 2004-04-23 17:56:00 PDT
Created attachment 146892 [details] [diff] [review]
<nsFrame.cpp>
[Checked in: Comment 147]

Fix for:

{{ <http://tinderbox.mozilla.org/SeaMonkey/warn1082753340.30566.html>
1173.	layout/html/base/src/nsFrame.cpp:4352 (See build log excerpt)
	`nsIFrame* GetNextSiblingAcrossLines(nsIPresContext*, nsIFrame*)'
defined but not used
}}

{{ <http://lxr.mozilla.org/mozilla/search?string=GetNextSiblingAcrossLines>
GetNextSiblingAcrossLines
/layout/html/base/src/nsFrame.cpp, line 4352 --
GetNextSiblingAcrossLines(nsIPresContext *aPresContext, nsIFrame *aFrame)
}}
Comment 144 Serge Gautherie (:sgautherie) 2004-04-23 18:00:27 PDT
Comment on attachment 146892 [details] [diff] [review]
<nsFrame.cpp>
[Checked in: Comment 147]


(In reply to comment #142)
> (From update of Comment 142]">attachment 146811 [details] [diff] [review])

It removes a useless |thisBlock| too.

***

I have no compiler: Could you (super-)review/compile/test/check in this patch ?
Thanks.
Comment 145 Serge Gautherie (:sgautherie) 2004-04-23 18:33:50 PDT
Created attachment 146894 [details] [diff] [review]
<nsMsgThread.cpp>, v1

Fix for:

{{ <http://tinderbox.mozilla.org/SeaMonkey/warn1082753340.30566.html>
4.	mailnews/db/msgdb/src/nsMsgThread.cpp:871 (See build log excerpt)
	`nsresult nsMsgThreadUnreadFilter(nsIMsgDBHdr*, void*)' defined but not
used
}}

{{ <http://lxr.mozilla.org/mozilla/search?string=nsMsgThreadUnreadFilter>
nsMsgThreadUnreadFilter
/mailnews/db/msgdb/src/nsMsgThread.cpp, line 870 --
nsMsgThreadUnreadFilter(nsIMsgDBHdr* msg, void* closure)
}}

And some cleanup, mostly related to |ret|.
Comment 146 Serge Gautherie (:sgautherie) 2004-04-23 18:35:12 PDT
Comment on attachment 146894 [details] [diff] [review]
<nsMsgThread.cpp>, v1


I have no compiler: Could you (super-)review/compile/test/check in this patch ?
Thanks.
Comment 147 Serge Gautherie (:sgautherie) 2004-04-24 08:00:23 PDT
Comment on attachment 146892 [details] [diff] [review]
<nsFrame.cpp>
[Checked in: Comment 147]


Check in: { 2004-04-24 06:30	neil%parkwaycc.co.uk	mozilla/ layout/ html/
base/ src/ nsFrame.cpp	 3.481 }
Comment 148 neil@parkwaycc.co.uk 2004-04-25 16:32:12 PDT
Comment on attachment 146894 [details] [diff] [review]
<nsMsgThread.cpp>, v1

>-  ret = m_mdbDB->UInt32ToRowCellColumn(m_metaRow, m_mdbDB->m_threadIdColumnToken, threadKey);
>+  nsresult ret = m_mdbDB->UInt32ToRowCellColumn(
>+                   m_metaRow, m_mdbDB->m_threadIdColumnToken, threadKey);
>   // gotta set column in meta row here.
>   return ret;
Thought: return m_mdbDB->UInt32ToRowCellColumn( [etc] here (and similar cases
below)?

>-  if (ret == 0)
>+  if (ret == NS_OK)
Still wrong, should be if (NS_SUCCEEDED(ret))

>-  nsresult ret = NS_OK;
If ret gets assigned into so that the NS_OK is never used, then just declaring
nsresult ret; should be sufficient cleanup.

>-  ret = m_mdbTable->GetTableRowCursor(m_mdbDB->GetEnv(), pos, &rowCursor);
>-  if (ret != 0)
>+  nsresult ret = m_mdbTable->GetTableRowCursor(
>+                               m_mdbDB->GetEnv(), pos, &rowCursor);
>+  if (ret != NS_OK)
>     return  NS_ERROR_FAILURE;
This should actually say
if (NS_FAILED(ret))
  return ret;
Comment 149 Alec Flett 2004-04-29 07:14:44 PDT
Comment on attachment 130274 [details] [diff] [review]
</modules/plugin/samples/default/windows/dialogs.cpp>
[Checked in: Comment 153]

sr=alecf
Comment 150 Alec Flett 2004-04-29 07:15:10 PDT
Comment on attachment 130295 [details] [diff] [review]
</modules/plugin/base/src/nsPluginsDirWin.cpp>
[Checked in: Comment 155]

sr=alecf
Comment 151 Alec Flett 2004-04-29 07:17:23 PDT
Comment on attachment 130329 [details] [diff] [review]
<plugin.cpp>, v2

huh. I'm wondering if this is really the right thing. I wonder if iSize should
be passed in instead of lstrlen(buf) and if "rc" should be checked before the
call to NPN_Write and NPN_Destroy, instead of just throwing away the return
values.

cancelling my sr request until we get proper reviews from the module owner.
Comment 152 Matthias Bockelkamp 2004-04-29 07:54:24 PDT
Gerv:
Please have a look at Attachment 130329 [details] [diff] and Comments 113, 151.
I cannot find the email address of the initial developer (av@netscape.com). 
Comment 153 Serge Gautherie (:sgautherie) 2004-04-30 10:27:58 PDT
Comment on attachment 130274 [details] [diff] [review]
</modules/plugin/samples/default/windows/dialogs.cpp>
[Checked in: Comment 153]


Check in: { 2004-04-29 15:58	neil%parkwaycc.co.uk	mozilla/ modules/
plugin/ samples/ default/ windows/ dialogs.cpp	      1.9 }
Comment 154 Serge Gautherie (:sgautherie) 2004-04-30 10:53:09 PDT
(In reply to comment #46)
> (From update of Comment 153]">attachment 130274 [details] [diff] [review])
> this change should be made to both windows and os2 plugins.

I filed bug 242215 on this.
Comment 155 Serge Gautherie (:sgautherie) 2004-04-30 15:10:38 PDT
Comment on attachment 130295 [details] [diff] [review]
</modules/plugin/base/src/nsPluginsDirWin.cpp>
[Checked in: Comment 155]


Check in: { 2004-04-30 11:28	neil%parkwaycc.co.uk	mozilla/ modules/
plugin/ base/ src/ nsPluginsDirWin.cpp	      1.34 }
Comment 156 Serge Gautherie (:sgautherie) 2004-04-30 16:31:43 PDT
Created attachment 147420 [details] [diff] [review]
<*/plugin.cpp> ++, v3

v2, with comment 151 suggestion(s).
Comment 157 Serge Gautherie (:sgautherie) 2004-04-30 16:35:58 PDT
Comment on attachment 147420 [details] [diff] [review]
<*/plugin.cpp> ++, v3

I have no compiler: Could you compile/test/review this patch ? Thanks.

(In reply to comment #152)
> Please have a look at Attachment 130329 [details] [diff] and Comments 113.

See also comment 133 (not 113).
Comment 158 Serge Gautherie (:sgautherie) 2004-05-01 07:35:43 PDT
Created attachment 147444 [details] [diff] [review]
<nsMsgThread.cpp>, v2
[Checked in: Comment 161]

v1, with comment 148 suggestion(s).
Comment 159 Serge Gautherie (:sgautherie) 2004-05-01 07:37:58 PDT
Comment on attachment 147444 [details] [diff] [review]
<nsMsgThread.cpp>, v2
[Checked in: Comment 161]

I have no compiler: Could you (super-)review/compile/test/check in this patch ?
Thanks.
Comment 160 Gervase Markham [:gerv] 2004-05-01 08:34:17 PDT
Matthias: what do you want me to look at about that attachment? I can't work it out.

Gerv
Comment 161 David :Bienvenu 2004-05-03 06:56:09 PDT
Comment on attachment 147444 [details] [diff] [review]
<nsMsgThread.cpp>, v2
[Checked in: Comment 161]

r/sr=bienvenu, checked in.
Comment 162 David :Bienvenu 2004-05-03 07:03:49 PDT
Comment on attachment 147444 [details] [diff] [review]
<nsMsgThread.cpp>, v2
[Checked in: Comment 161]

whoa, didn't mean to grant 1.7 approval (don't think I did either...) running
trun seamonkey build...
Comment 163 Matthias Bockelkamp 2004-05-03 08:57:46 PDT
(In reply to comment #160)
> Matthias: what do you want me to look at about that attachment? I can't work
it out.

The reviewer wrote that the rc value should not be removed but used for an error
check. Serge has already done that in attachment 147420 [details] [diff] [review].
Comment 164 Serge Gautherie (:sgautherie) 2004-05-24 14:14:35 PDT
(In reply to comment #136)
> (From update of attachment 130327 [details] [diff] [review])
> 
> Resurrect sr=?:
> I have no compiler, neil (my check-in partner) has no jvm;
> Or can someone test this ? (kyle ?)

Kyle ? We're looking for a tester or a super-reviewer ... Can you help ? Thanks.
Comment 165 Henry Jia 2004-05-24 21:14:31 PDT
Comment on attachment 130327 [details] [diff] [review]
</modules/oji/src/jvmmgr.cpp> v2
[Checked in: Comment 166]

sr=Henry
Comment 166 Serge Gautherie (:sgautherie) 2004-05-25 11:19:38 PDT
Comment on attachment 130327 [details] [diff] [review]
</modules/oji/src/jvmmgr.cpp> v2
[Checked in: Comment 166]


Check in: { 2004-05-25 03:34	neil%parkwaycc.co.uk	mozilla/ modules/ oji/
src/ jvmmgr.cpp  1.44 }
Comment 167 Serge Gautherie (:sgautherie) 2004-05-25 11:28:23 PDT
Comment on attachment 130291 [details] [diff] [review]
</widget/src/windows/nsNativeDragTarget.cpp> [Checked in: Comment 173]

No super-review from <hyatt@mozilla.org> since "2003-08-24" :-(
Comment 168 Serge Gautherie (:sgautherie) 2004-05-25 11:32:00 PDT
Comment on attachment 130302 [details] [diff] [review]
<nsWinCharset.cpp>, v1-r (for review only) [Check in: See v1-ci]

No super-review from <dveditz@cruzio.com> since "2004-04-20" :-(
Comment 169 Serge Gautherie (:sgautherie) 2004-05-25 11:39:30 PDT
Comment on attachment 147420 [details] [diff] [review]
<*/plugin.cpp> ++, v3

No review from <peterlubczynski-bugs@peterl.com> since "2004-04-30" :-(

I have no compiler: Could you compile/test/(super-)review/check in this patch ?
Thanks.

***

(In reply to comment #152)
> Please have a look at Attachment 130329 [details] [diff] and Comments 113.

See also comment 133 (not 113).
Comment 170 Alec Flett 2004-05-25 23:25:12 PDT
Comment on attachment 147420 [details] [diff] [review]
<*/plugin.cpp> ++, v3

I will review this patch when it has been tested.
Comment 171 Serge Gautherie (:sgautherie) 2004-05-26 05:10:46 PDT
(In reply to comment #170)
> (From update of attachment 147420 [details] [diff] [review])
> I will review this patch when it has been tested. 

helpwanted:
Who can test this patch ? (Matthias ?)
Comment 172 Matthias Bockelkamp 2004-05-26 06:49:58 PDT
Comment on attachment 147420 [details] [diff] [review]
<*/plugin.cpp> ++, v3

(In reply to comment #171)
> Who can test this patch ? (Matthias ?)

Compiles (with mingw) and works (with Shockwave Flash).
Comment 173 Matthias Bockelkamp 2004-05-26 07:05:27 PDT
Comment on attachment 130291 [details] [diff] [review]
</widget/src/windows/nsNativeDragTarget.cpp> [Checked in: Comment 173]

Check in: { 2004-05-26 04:48	neil%parkwaycc.co.uk	mozilla/ widget/ src/
windows/ nsNativeDragTarget.cpp   1.28	  3/3	  Removing unused variable
b=217089 p=mbockelcamp@web.de r=timeless sr=rbs }
Comment 174 jag (Peter Annema) 2004-05-26 13:39:49 PDT
Comment on attachment 130302 [details] [diff] [review]
<nsWinCharset.cpp>, v1-r (for review only) [Check in: See v1-ci]

sr=jag
Comment 175 Serge Gautherie (:sgautherie) 2004-05-26 17:13:20 PDT
Created attachment 149385 [details] [diff] [review]
<nsWinCharset.cpp>, v1-ci (for check in only)
[Checked in: Comment 177]

v1-r, "diffed" against current trunk,
with an added white space cleanup.

Keeping
{{
<nsWinCharset.cpp>, v1-r (for review only)	 patch		2003-08-23
12:27 PDT    smontagu: review+
jag: superreview+ 
}}
Comment 176 Alec Flett 2004-05-26 17:43:18 PDT
Comment on attachment 147420 [details] [diff] [review]
<*/plugin.cpp> ++, v3

thanks. 

comments:
+// static char szDefaultFileExt[] = "*";

remove this line entirely, don't just comment it out, goes for both instances

r/sr=alecf with that fixed
Comment 177 Serge Gautherie (:sgautherie) 2004-05-27 10:20:40 PDT
Comment on attachment 149385 [details] [diff] [review]
<nsWinCharset.cpp>, v1-ci (for check in only)
[Checked in: Comment 177]


Check in: { 2004-05-27 02:00	neil%parkwaycc.co.uk	mozilla/ intl/ uconv/
src/ nsWinCharset.cpp	  1.33 }
Comment 178 Serge Gautherie (:sgautherie) 2004-05-27 10:36:00 PDT
Created attachment 149425 [details] [diff] [review]
<*/plugin.cpp> ++, v3b
[Checked in: Comment 180]

v3b, with comment 176 suggestion(s).
Comment 179 Serge Gautherie (:sgautherie) 2004-05-27 10:37:48 PDT
Comment on attachment 149425 [details] [diff] [review]
<*/plugin.cpp> ++, v3b
[Checked in: Comment 180]

Keeping
{{
(comment #176)
> 
> r/sr=alecf
}}
Comment 180 Serge Gautherie (:sgautherie) 2004-05-27 15:00:56 PDT
Comment on attachment 149425 [details] [diff] [review]
<*/plugin.cpp> ++, v3b
[Checked in: Comment 180]


Check in: { 2004-05-27 12:32	neil%parkwaycc.co.uk }
Comment 181 Serge Gautherie (:sgautherie) 2008-09-30 18:03:50 PDT
Comment on attachment 130287 [details] [diff] [review]
</gfx/src/windows/nsGfxFactoryWin.cpp>

(In reply to comment #137)
> I also prefer using the generic constructor

Eventually, this file was removed by
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/gfx/src/windows/Attic/nsGfxFactoryWin.cpp&rev=3.46
Comment 182 Philip Chee 2008-12-25 06:19:36 PST
Serge, can this bug be closed?
Comment 183 Ed Morley [:emorley] 2011-08-01 17:43:07 PDT
All patches checked in, marking fixed.
Rest of the work is carrying on in bug 187528 and dependants.

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