217089 - Fix compiler warnings: "unused variable" and "defined but not used"

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Matthias Bockelkamp]

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]

Attachment: Patch for CNavDTD.cpp

Comment 2

[Matthias Bockelkamp]

Attachment: </modules/plugin/samples/default/windows/dialogs.cpp> [Checked in: Comment 153]

Comment 3

[Matthias Bockelkamp]

Attachment: </modules/oji/src/jvmmgr.cpp> v1

Comment 4

[Matthias Bockelkamp]

Attachment: Patch for lcglue.cpp

Comment 5

[Matthias Bockelkamp]

Attachment: Patch for ns4xPlugin.cpp

Comment 6

[Matthias Bockelkamp]

Attachment: Patch for nsBoxObject.cpp

Comment 7

[Matthias Bockelkamp]

Attachment: </widget/src/windows/nsClipboard.cpp> [Checked in: Comment 113]

Comment 8

[Matthias Bockelkamp]

Attachment: </widget/src/windows/nsDataObj.cpp>, v1

Comment 9

[Matthias Bockelkamp]

Attachment: Patch for nsDocumentViewer.cpp

Comment 10

[Matthias Bockelkamp]

Attachment: <nsFileSystemDataSource.cpp> [Checked in: Comment 77]

Comment 11

[Matthias Bockelkamp]

Attachment: Patch for nsFontMetricsWin.cpp

Comment 12

[Matthias Bockelkamp]

Attachment: </gfx/src/windows/nsGfxFactoryWin.cpp>

Comment 13

[Matthias Bockelkamp]

Attachment: Patch for nsHTMLContentSink.cpp

Comment 14

[Matthias Bockelkamp]

Attachment: Patch for nsImageWin.cpp

Comment 15

[Matthias Bockelkamp]

Attachment: Patch for nsJVMManager.cpp

Comment 16

[Matthias Bockelkamp]

Attachment: </widget/src/windows/nsNativeDragTarget.cpp> [Checked in: Comment 173]

Comment 17

[Matthias Bockelkamp]

Attachment: </gfx/src/windows/nsNativeThemeWin.cpp> [Checked in: Comment 135]

Comment 18

[Matthias Bockelkamp]

Attachment: Patch for nsOSHelperAppService.cpp

Comment 19

[Matthias Bockelkamp]

Attachment: </modules/plugin/base/src/nsPluginsDirWin.cpp> [Checked in: Comment 155]

Comment 20

[Matthias Bockelkamp]

Attachment: </gfx/src/windows/nsPrintOptionsWin.cpp> [Checked in: Comment 114]

Comment 21

[Matthias Bockelkamp]

Attachment: Patch for nsRDFXMLDataSource.cpp [Checked in: Comment 112]

Comment 22

[Matthias Bockelkamp]

Attachment: Patch for nsScanner.cpp

Comment 23

[Matthias Bockelkamp]

Attachment: Patch for nsScreenManagerWin.cpp

Comment 24

[Matthias Bockelkamp]

Attachment: Patch for nsSelection.cpp

Comment 25

[Matthias Bockelkamp]

Attachment: Patch for nsToolkit.cpp

Comment 26

[Matthias Bockelkamp]

Attachment: <nsWinCharset.cpp>, v1-r (for review only) [Check in: See v1-ci]

Comment 27

[Matthias Bockelkamp]

Attachment: Patch for nsWindow.cpp

Comment 28

[Matthias Bockelkamp]

Attachment: Patch for nsXMLContentSink.cpp

Comment 29

[Matthias Bockelkamp]

Attachment: </layout/xul/base/src/nsXULTooltipListener.cpp>, v1

Comment 30

[Matthias Bockelkamp]

Attachment: </modules/plugin/samples/default/windows/plugin.cpp> v1

Comment 31

[Brian Ryner (not reading)]

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

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]

Attachment: Patch for nsBoxObject.cpp, v2

Comment 34

[Matthias Versen [:Matti]]

-> [e-mail] 

Comment 35

[timeless]

please stop for a while.

Comment 36

[timeless]

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]

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]

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]

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]

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]

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]

Attachment: </modules/oji/src/jvmmgr.cpp> v2 [Checked in: Comment 166]

Comment 43

[Matthias Bockelkamp]

Attachment: Patch for nsXMLContentSink.cpp, v2

Comment 44

[Matthias Bockelkamp]

Attachment: <plugin.cpp>, v2

Comment 45

[timeless]

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]

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]

Attachment: <os2/dialogs.cpp> [Checked in: Comment 107]

Comment 48

[timeless]

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]

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]

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]

Attachment: Patch for nsHTMLContentSink.cpp, v2

Comment 52

[Matthias Bockelkamp]

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]

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]

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]

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]

Comment on attachment 130297 [details] [diff] [review]
Patch for nsRDFXMLDataSource.cpp
[Checked in: Comment 112]

RDFXMLDataSourceImpl::rdfXMLFlush

Comment 57

[timeless]

Comment on attachment 130297 [details] [diff] [review]
Patch for nsRDFXMLDataSource.cpp
[Checked in: Comment 112]

RDFXMLDataSourceImpl::rdfXMLFlush

Comment 58

[timeless]

Comment on attachment 130298 [details] [diff] [review]
Patch for nsScanner.cpp

Peek() changes theChar, you can't not call it.

Comment 59

[timeless]

Comment on attachment 130299 [details] [diff] [review]
Patch for nsScreenManagerWin.cpp

how about making the code work w/ mingw instead?

Comment 60

[timeless]

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]

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

Comment on attachment 130276 [details] [diff] [review]
Patch for lcglue.cpp

r+sr=jst

Comment 63

[timeless]

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]

Attachment: Patch for nsScanner.cpp, v2

Comment 65

[Simon Montagu :smontagu]

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

Comment on attachment 130336 [details] [diff] [review]
Patch for nsHTMLContentSink.cpp, v2

r+sr=jst

Comment 67

[Johnny Stenback (:jst)]

Comment on attachment 130349 [details] [diff] [review]
Patch for nsScanner.cpp, v2

r+sr=jst

Comment 68

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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]

Attachment: Patch for nsScreenManagerWin.cpp, v2

It compiles fine, but I cannot test it with my single monitor equipment.

Comment 70

[Matthias Bockelkamp]

Attachment: Patch for nsDocumentViewer.cpp, v2

Changed as dbaron said.
r=timeless, sr=dbaron from previous patch.

Comment 71

[rbs]

Comment on attachment 130286 [details] [diff] [review]
Patch for nsFontMetricsWin.cpp

r+sr=rbs

Comment 72

[Ere Maijala (slow)]

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

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

I meant bug 215981.

Comment 75

[Mike Kaply [:mkaply]]

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

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]

Comment on attachment 130284 [details] [diff] [review]
<nsFileSystemDataSource.cpp>
[Checked in: Comment 77]

sr=alecf

Comment 78

[Christopher Blizzard (:blizzard)]

Why do you want to remove some of the old MSVC support?  Have we dropped support
for those older compilers?

Comment 79

[Matthias Bockelkamp]

Attachment: Patch for nsScreenManagerWin.cpp, v3

Comment 80

[timeless]

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]

Attachment: Patch for nsScreenManagerWin.cpp, v4

Comment 82

[timeless]

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]

Attachment: Patch for nsScreenManagerWin.cpp, v5

Comment 84

[Christian :Biesinger (don't email me, ping me on IRC)]

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 [e-mail] r=timeless sr=darin

Comment 85

[timeless]

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

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

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]

Comment on attachment 130296 [details] [diff] [review]
</gfx/src/windows/nsPrintOptionsWin.cpp>
[Checked in: Comment 114]

sr=[e-mail]

Comment 89

[Serge Gautherie (:sgautherie)]

*** Bug 211429 has been marked as a duplicate of this bug. ***

Comment 90

[Serge Gautherie (:sgautherie)]

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

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

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

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

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

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

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]

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]

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

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

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

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

Attachment: <nsScreenManagerWin.*> (v6) [+/- Checked in: Comment 110]

This is v5, with comment 85 suggestions.

Comment 103

[Serge Gautherie (:sgautherie)]

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

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]

Comment on attachment 137772 [details] [diff] [review]
<nsScreenManagerWin.*> (v6)
[+/- Checked in: Comment 110]

sr=rbs

Comment 106

[Serge Gautherie (:sgautherie)]

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

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

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

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

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]

*** Bug 198785 has been marked as a duplicate of this bug. ***

Comment 112

[Serge Gautherie (:sgautherie)]

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 [:bzbarsky]]

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 [:bzbarsky]]

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

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

Attachment: </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)]

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

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

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

Comment on attachment 130287 [details] [diff] [review]
</gfx/src/windows/nsGfxFactoryWin.cpp>


No (super-)review from <[e-mail]> since '2003-08-24' :-(

Also, see comment 48 !

Comment 121

[Serge Gautherie (:sgautherie)]

Comment on attachment 130292 [details] [diff] [review]
</gfx/src/windows/nsNativeThemeWin.cpp>
[Checked in: Comment 135]


No review from <[e-mail]>, no super-review from <[e-mail]>,
since '2003-08-24' :-(

Also, see comment 53 !

Comment 122

[Serge Gautherie (:sgautherie)]

Comment on attachment 130281 [details] [diff] [review]
</widget/src/windows/nsDataObj.cpp>, v1


No (super-)review from <[e-mail]> since '2003-08-23' :-(

Comment 123

[Serge Gautherie (:sgautherie)]

Comment on attachment 130302 [details] [diff] [review]
<nsWinCharset.cpp>, v1-r (for review only) [Check in: See v1-ci]


No super-review from <[e-mail]> since '2003-08-23' :-(

Also, see comment 92 !
(If sr+, I'll post an updated patch for check-in.)

Comment 124

[Serge Gautherie (:sgautherie)]

Comment on attachment 130327 [details] [diff] [review]
</modules/oji/src/jvmmgr.cpp> v2
[Checked in: Comment 166]


No (super-)review from <[e-mail]> since '2003-08-24' :-(

Comment 125

[Serge Gautherie (:sgautherie)]

Comment on attachment 130329 [details] [diff] [review]
<plugin.cpp>, v2

No (super-)review from <[e-mail]> since '2003-12-23' :-(

Comment 126

[Serge Gautherie (:sgautherie)]

Comment on attachment 130295 [details] [diff] [review]
</modules/plugin/base/src/nsPluginsDirWin.cpp>
[Checked in: Comment 155]


No super-review from <[e-mail]> since '2003-12-23' :-(

Comment 127

[Serge Gautherie (:sgautherie)]

Comment on attachment 130274 [details] [diff] [review]
</modules/plugin/samples/default/windows/dialogs.cpp>
[Checked in: Comment 153]


No super-review from <[e-mail]> since '2003-12-23' :-(

Also, see comment 108 !

Comment 128

[Brendan Eich [:brendan]]

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=[e-mail] 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)]

(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=[e-mail] 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]]

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]

Comment on attachment 130287 [details] [diff] [review]
</gfx/src/windows/nsGfxFactoryWin.cpp>

r+sr=rbs

Comment 132

[rbs]

Comment on attachment 130292 [details] [diff] [review]
</gfx/src/windows/nsNativeThemeWin.cpp>
[Checked in: Comment 135]

r+sr=rbs

Comment 133

[Matthias Bockelkamp]

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

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

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=[e-mail] r/sr=rbs }

Comment 136

[Serge Gautherie (:sgautherie)]

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]

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

Comment on attachment 130281 [details] [diff] [review]
</widget/src/windows/nsDataObj.cpp>, v1

r=ere

Comment 139

[Serge Gautherie (:sgautherie)]

Attachment: </widget/src/windows/nsDataObj.cpp>, v1b [Checked in: Comment 142]

v1,
with an additional |status| removal.

Comment 140

[Serge Gautherie (:sgautherie)]

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

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

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

Attachment: <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)]

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

Attachment: <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)]

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

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

[[e-mail]]

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]

Comment on attachment 130274 [details] [diff] [review]
</modules/plugin/samples/default/windows/dialogs.cpp>
[Checked in: Comment 153]

sr=alecf

Comment 150

[Alec Flett]

Comment on attachment 130295 [details] [diff] [review]
</modules/plugin/base/src/nsPluginsDirWin.cpp>
[Checked in: Comment 155]

sr=alecf

Comment 151

[Alec Flett]

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]

Gerv:
Please have a look at Attachment 130329 [details] [diff] and Comments 113, 151.
I cannot find the email address of the initial developer ([e-mail]). 

Comment 153

[Serge Gautherie (:sgautherie)]

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

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

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

Attachment: <*/plugin.cpp> ++, v3

v2, with comment 151 suggestion(s).

Comment 157

[Serge Gautherie (:sgautherie)]

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

Attachment: <nsMsgThread.cpp>, v2 [Checked in: Comment 161]

v1, with comment 148 suggestion(s).

Comment 159

[Serge Gautherie (:sgautherie)]

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

Matthias: what do you want me to look at about that attachment? I can't work it out.

Gerv

Comment 161

[David :Bienvenu]

Comment on attachment 147444 [details] [diff] [review]
<nsMsgThread.cpp>, v2
[Checked in: Comment 161]

r/sr=bienvenu, checked in.

Comment 162

[David :Bienvenu]

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]

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

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

Comment on attachment 130327 [details] [diff] [review]
</modules/oji/src/jvmmgr.cpp> v2
[Checked in: Comment 166]

sr=Henry

Comment 166

[Serge Gautherie (:sgautherie)]

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

Comment on attachment 130291 [details] [diff] [review]
</widget/src/windows/nsNativeDragTarget.cpp> [Checked in: Comment 173]

No super-review from <[e-mail]> since "2003-08-24" :-(

Comment 168

[Serge Gautherie (:sgautherie)]

Comment on attachment 130302 [details] [diff] [review]
<nsWinCharset.cpp>, v1-r (for review only) [Check in: See v1-ci]

No super-review from <[e-mail]> since "2004-04-20" :-(

Comment 169

[Serge Gautherie (:sgautherie)]

Comment on attachment 147420 [details] [diff] [review]
<*/plugin.cpp> ++, v3

No review from <[e-mail]> 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]

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

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

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]

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=[e-mail] r=timeless sr=rbs }

Comment 174

[jag (Peter Annema)]

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

Attachment: <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]

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

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

Attachment: <*/plugin.cpp> ++, v3b [Checked in: Comment 180]

v3b, with comment 176 suggestion(s).

Comment 179

[Serge Gautherie (:sgautherie)]

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

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

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]

Serge, can this bug be closed?

Comment 183

[Ed Morley [:emorley]]

All patches checked in, marking fixed.
Rest of the work is carrying on in bug 187528 and dependants.