86036 - widget/src/qt/nsMime.cpp uses non-standard for init scope

Status: VERIFIED FIXED

(SeaMonkey :: General, defect)

Description

[John Mark Vandenberg]

widget/src/qt/nsMime.cpp uses c++ coding standard regarding for initialisation 
variable scope which differs from rest of Mozilla source.

Comment 1

[John Mark Vandenberg]

Attachment: Patch to fix 'for' variable scope

Comment 2

[timeless]

Attachment: my editor ate someone's whitespace. patch adds a search function, fixes spelling errors and complains about string copying

Comment 3

[timeless]

*shrug* i actually don't think that the reporter's assertion is correct. it is 
kindof correct for javascript but that's a different ball of wax.

One thing about C++ is that it's prefered that variables be declared in a tight 
scope to their usage.

so
+  PRInt32 c;
   for (msd = mMimeStore.first(); msd != 0; msd = mMimeStore.next()) {
before the for loop worries me a bit, if i were to move it at all, i'd put it 
at least inside:
  if (strcmp(name, kUnicodeMime) == 0) {
and after the |for (msd = mMimeStore.first(); msd != 0; msd = 
mMimeStore.next()) {|...|}| loop.

I might even change
-  nsMimeStoreData* msd;
-  for (msd = mMimeStore.first(); msd != 0; msd = mMimeStore.next()) {
+  for (nsMimeStoreData* msd = mMimeStore.first(); msd; msd = 
mMimeStore.next()) {

What the reporter's bug/patch really show is that the nsMimeStore class needs a 
search function. So I wrote one for my patch, however it isn't great since 
there are still two more potential clients if only it returned an index.

In order for me to write that, I'd have to read the documentation on the Qt 
classes.  I think format() takes ints from 0..count-1 
in which case the better search function would return -1 on failure or 
0..count-1 on success.

scc: I'm sure i should know what to do by now, but i'm not sure :( partly 
because i don't know what a QString does, but would you agree there's a better 
way to go from PRUnichar* to char*?

QString ascii(PromiseFlatCString((PRUnichar*)data).get(), len); should make the 
code slightly shorter although it's probably way too expensive since i'm almost 
guaranteed to have 3 copies.

len /= 2; //**this line given for context to match an existing line of code.
const nsAFlatCString& flat = PromiseFlatCString((PRUnichar*)data);
char *as = new char[len];
for (PRInt32 c = 0; c < len; c++)
  as[c] = flat[c];
// let's text/plain be first for stupid programs //see **
This should probably save me at least one copy but i have many doubts that it 
would even compile, and i'm fishing for a better solution...  I do suspect that 
this would save loop time.

since we were on the subject of cleanup, i fixed a few spelling errors.

One of the few frivilous changes i might try to make across the codebase:
Switch array loop indices (where appropriate) from PRInt32 to PRUInt32 
unfortunately making that change here would give me a type warning because 
PRBool nsMimeStore::AddFlavorData(const char* name, void* data, PRInt32 len)
len is a PRInt32.

So we have the other approach which is to replace the code w/ something 
cleaner.

These comments were rearranged, so I hope they're readable. If not I 
appologize.

I've temporarily taken the position as qa contact, however that's rather 
meaningless and mostly to save doron from the bugmail.  For a bug like this, 
the QA merely needs to verify that the app still works and that whatever code 
was finally selected was checked in.

Comment 4

[timeless]

scc suggested ToNewCString(nsDependentString((PRUnichar*)data)), which sounds 
good to me. so i'll try to get that working after i get my Solaris box 
compiling moz again [it's in limbo Forte6unpatched instead of Forte5 or 
Forte6patched :-(]

Comment 5

[John Mark Vandenberg]

MIPSpro is farily particular about ansi for initialisation scope ... and the 
rest of the Mozilla code base compiles.  This is the first piece of code which 
has broken on this.  

http://www.mozilla.org/hacking/portable-cpp.html#variables_in_for

Comment 6

[John C. Griggs]

Attachment: Updated patch for widget/src/qt/nsMime.cpp

Comment 7

[John C. Griggs]

Attachment: Updated patch for widget/src/qt/nsMime.h

Comment 8

[John C. Griggs]

I have updated the patches for nsMime.cpp and nsMime.h, using the suggestion to
use ToNewCString and nsDependentString.  This cuts down on the copying and
removes the assumption about the length of PRUnichar. I can also confirm that a
QByteArray member of nsMimeStoreData takes ownership of the buffer passed to the
nsMimeStoreData constructor.  I haven't had any problem passing the buffer
returned from ToNewCString directly, but I guess there is a possibility of Qt
and Mozilla clashing over memory management, in which case an additional copy
would be required.  These patches remove the for loops that started the whole
discussion 8^).

Comment 9

[timeless]

r=timeless

Comment 10

[Christopher Blizzard (:blizzard)]

sr=blizzard

Comment 11

[John C. Griggs]

The patches have been checked into CVS.

Comment 12

[timeless]

fixed per JCG

Comment 13

[timeless]

and verified per bonsai, changes were in version 1.2 of both files.