243888 - stream converter tidy up

Status: VERIFIED FIXED

(MailNews Core :: MIME, defect)

Description

[Brodie]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040514
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040514

While investigating the nsStreamConverter class to see if I could use it in
something else that I was doing I found that the code was more complex than
necessary and had a lot of unnecessary calls (like alloc/free/strlen) which
could be eliminated.

I have created a patch to simplify this class. 

Reproducible: Always
Steps to Reproduce:

Comment 1

[Brodie]

Attachment: first cut

This is first draft of the patch to tidy up the class.	

Also, I searched all of mailnews for code that uses the "outformat=" header and
couldn't find any. Is this now unnecessary?

Comment 2

[Stephen Donner [:stephend] Not actively reading bugmail]

Confirming bug since it has a patch.

Comment 3

[Brodie]

Attachment: final patch (ignore whitespace)

Comment 4

[Brodie]

Attachment: final patch

Final patch. There were a number of tabs in the final which I removed. Review
is probably easier using the ignore whitespace version of this patch. Otherwise
patches are identical.

Comment 5

[Brodie]

Attachment: patch (ignore whitespace)

Comment 6

[Brodie]

Attachment: patch

I messed up last time and uploaded an old version of the patch. This is the
version that I had meant to upload last time.

Comment 7

[Jean-Francois Ducarroz]

Comment on attachment 148927 [details] [diff] [review]
patch

very good cleanup. For the story, this code originated from Netscape 3 and was
written in C. That might explain why it's sound so odd sometime...

I have only 2 comments:
1) +	    for (int n = 0; n < NS_ARRAY_LENGTH(rgTypes); ++n)
This might cause some trouble on compiler which considers the scoop of the
variable n to be the parent scoop for the for loop. Therefore it would be safer
to declare n outside the for loop.

2) While parsing the header param, it's possible to detect the value belonging
to another query element:
...?header=src&outformat=none
In that case, we will interpret header=none! But as the old code has the same
problem, feel free to either fix it of file another bug.

Apart that, R=ducarroz.

Comment 8

[Brodie]

Yes, now I can understand it a little better if it was originally C.

For your comments:

1)
the scoping of n should still be okay even if considered in parent scope (which 
VC does by default?). The first n is scoped within the if(mustInit) { ... } 
block which separates the definition from the second one.

2)
I'm afraid I don't understand how this problem can occur. The function that 
finds the query string looks for "header=" in the URL, ensures that & or ? 
precede it and then returns a pointer to the string following the = sign. Am I 
looking in the wrong place?

Any suggestions for who to ask for sr?

Comment 9

[Jean-Francois Ducarroz]

R1) Yes, I saw that. but the potential problem could come later one when
somebody will move the code around. That's why I think is better to not use that
kind of pattern.

R2) In my understanding (just by reading the code), the code will correctly find
the beginning of the value for a specific query parameter but it will not
truncate the url. Therefore, in the example I gave above, when you look for
header=, you get a pointer to the "src&outformat=none\0". Then when you look for
a particular value, you might get the value for the next parameter (in the
example "none" instead of "src")

Ask David Bienvenu <[e-mail]> or Seth Spitzer
<[e-mail]> for the SR

Comment 10

[Brodie]

Attachment: patch 2

1) ok, I've moved the var decl to the enclosing scope.

2) We can't actually fail the way that you are thinking. The old code was using
PL_strcasestr which certainly had that problem, but I'm using PL_strncasecmp so
we are guaranteed to look at the data for the current query element only.
However, I wasn't checking that a match was for the whole string though, so
previously it would have matched 'none' for 'noneofyourbusiness' too. That
problems is now fixed in this patch.

I changed the order of the function to be more readable, but kept the same
semantics as before. It now reads as:

if (bad args)
  default
  return

if (outformat)
  ...
  return

if (part)
  ...
  return

if (header)
  ...
  return

default
return

Also, as per my comment re: outformat. I've done a file contents search of
every file (*.*) in the mozilla directory tree and I didn't find a single
location where the string "outformat=" is added to any URL. It is possible that
this code has been orphaned off by a change. It dates back to version 1.1 of
the file. Do you know if this code is still being used?

Comment 11

[Jean-Francois Ducarroz]

Comment on attachment 149010 [details] [diff] [review]
patch 2

You are totally correct, I missed the n in PL_strncasecmp.

About the outformat thing,your are also right, it's probably a left over. But
we cannot be 100% sure that another   external project uses it. Therefore I
would suggest to keep it. R=ducarroz

Comment 12

[Brodie]

Attachment: patch 3

I just noticed that the following private member variables are initialized and
either a) read but not written, or b) written but not read. It seems that they
could all be removed safely (being private there are no other locations that
they could be accessed).

-  PRUnichar *			 mDesiredOutputType; // the output content type
passed into AsyncConvertData..
-  PRInt32			 mTotalRead;	 // Counter variable
-  PRBool			 mDoneParsing;	   // If this is true, we've
already been told by libmime to stop sending
-						   // data so don't feed the
parser any more!
-  nsIChannel * 		 mPendingChannel;  //Will be use whenn we need
to delay to fire onStartRequest
-  nsISupports *		 mPendingContext;  //Will be use whenn we need
to delay to fire onStartRequest

mDesiredOutputType = init + write
mTotalRead = init + write
mDoneParsing = init + write
mPendingChannel = init + read
mPendingChannel = init + read

if mPendingChannel and mPendingContext are removed, then the following method
has no body. It currently has no effect anyhow because these two member
variables were always NULL which it checks before doing anything. It is called
once by mime_output_fn in mimemoz2.cpp.
   NS_IMETHOD FirePendingStartRequest(void);

This patch removes all of them as well as includes the changes from patch 2. Do
you forsee any problems if I also remove them?

Comment 13

[David :Bienvenu]

format=header is used by the bayesian spam filter, isn't it?

can you attach a -uwp diff so I can skip the whitespace changes? thx...

Comment 14

[Brodie]

Attachment: patch 3 (ignore whitespace)

The spam filter uses "header=filter".

Comment 15

[Brodie]

Comment on attachment 149015 [details] [diff] [review]
patch 3

I know its been a pain with 3 revisions of the patch, but it is now ready to
go.

Comment 16

[David :Bienvenu]

I believe outFormat is obsolete - lxr doesn't show any hits either...and
cvsblame said it was part of the very first checkin of this file, so it could
easily be obsolete.

+    struct HeaderType {
+      char * headerType;
+      size_t headerTypeLen; // set to 0, automatically calculated
+      char * outputFormat;
+      nsMimeOutputType mimeOutputType;
+    };

the char * fields can be const char *, can't they?

+    static PRBool mustInitTable = PR_TRUE;
+    if (mustInitTable)
+    {
+      mustInitTable = PR_FALSE;
+      for (n = 0; n < NS_ARRAY_LENGTH(rgTypes); ++n)
+        rgTypes[n].headerTypeLen = strlen(rgTypes[n].headerType);
     }
 
if you switch the sense of this global var to "haveInitedTable", then you don't
need to init it, because it will automatically be 0'd, which will generate less
code.

But, better, you can use sizeof() the static string to that the lengths are
computed at compile time, which will save some code. (not sure if that includes
the null byte or not - you can just check...)

I'm not sure why you can just remove the code that fires the OnStartRequest -
can you elaborate?

Comment 17

[Jean-Francois Ducarroz]

I agree with removing those variables except for mPendingChannel &
mPendingContext. When I look at the history (thanks to lxr as I don't remember
anything), some of my changes in version 1.76 has been overwritten by apparently
a poor merge from Doug in version 1.78. You can really see it's a merge issue as
the  slepp error in the comment line "// forward the start rquest to any
listeners" which I corrected in 1.76 came back in version 1.78!

Now, I need to figure out if we still need my original fix...

Comment 18

[Jean-Francois Ducarroz]

the missing code is change 907 & 909 in the following:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsStreamConverter.cpp&branch=&root=/cvsroot&subdir=mozilla/mailnews/mime/src&command=DIFF_FRAMESET&rev1=1.75&rev2=1.76

Comment 19

[Brodie]

Jean-Francois ->
I'll add the appropriate code back in.

David -> 
re: outformat
Jean-Francois was concerned that we might have external parties using the
outformat query. If I put an assert inside the if statement and no-one ever
complains about it then the could can be removed later. However if you are
happing with removing it now then I'll do that.

re: array/init
Using sizeof()-1 on another copy of the string will clutter up the table and
leave it just as susceptible to errors as just writing in the length ourselves. 
 { "filter", sizeof("filter")-1, "text/html", ... }
or
 { "filter", 6, "text/html", ... }, 
i.e. just hard code the length. Once it has been set then it should be fine.
That will avoid the init.

Comment 20

[Brodie]

Attachment: patch 4 (ignore whitespace)

Comment 21

[Brodie]

Attachment: patch 4

New patches (including ignore whitespace) addressing the review comments. I've
hardcoded the lengths, but could also write it as:

+      { "filter",    sizeof("filter")-1,    "text/html", 
nsMimeOutput::nsMimeMessageFilterSniffer },
+      { "quotebody", sizeof("quotebody")-1, "text/html", 
nsMimeOutput::nsMimeMessageBodyQuoting },
+      { "print",     sizeof("print")-1,     "text/html", 
nsMimeOutput::nsMimeMessagePrintOutput },
+      { "only",      sizeof("only")-1,      "text/xml",  
nsMimeOutput::nsMimeMessageHeaderDisplay },
+      { "none",      sizeof("none")-1,      "text/html", 
nsMimeOutput::nsMimeMessageBodyDisplay },
+      { "quote",     sizeof("quote")-1,     "text/html", 
nsMimeOutput::nsMimeMessageQuoting },
+      { "saveas",    sizeof("saveas")-1,    "text/html", 
nsMimeOutput::nsMimeMessageSaveAs },
+      { "src",       sizeof("src")-1,	     "text/plain",
nsMimeOutput::nsMimeMessageSource }

I don't feel that this would be any clearer or greatly less error prone. If
anything having the length hard-coded is going to scare people into double
checking whereas a spelling error might be missed with the sizeof() scenario.

I'm happy to go with whatever you'd prefer though.

Comment 22

[David :Bienvenu]

Comment on attachment 149317 [details] [diff] [review]
patch 4

looks good - one nit:

+    static size_t contentTypeLen = strlen(UNKNOWN_CONTENT_TYPE) + 1;
+    *aOutputContentType = (char *) nsMemory::Clone(UNKNOWN_CONTENT_TYPE,
contentTypeLen);

you can just use sizeof(UNKNOWN_CONTENT_TYPE) here, right, and avoid the
strlen...

Comment 23

[Brodie]

Attachment: patch 5

Fixed UNKNOWN_CONTENT_TYPE. 

I've also fixed the table so that we don't need the length now. To do this I
added a small function which checks for a prefix:

// case-sensitive test for string prefixing. If |string| is prefixed
// by |prefix| then a pointer to the next character in |string| following
// the prefix is returned. If it is not a prefix then |nsnull| is returned.
static const char * 
SkipPrefix(const char *aString, const char *aPrefix)
{
  while (*aPrefix)
    if (*aPrefix++ != *aString++)
       return nsnull;
  return aString;
}

and then use it like:

    const char * remainder;
    for (int n = 0; n < NS_ARRAY_LENGTH(rgTypes); ++n)
    {
      remainder = SkipPrefix(header, rgTypes[n].headerType);
      if (remainder && (*remainder == '\0' || *remainder == '&'))
      {

Note that this means we now check the URL query case-sensitive whereas before
it was case-insensitive. I could make the test case-insensitive, but I can't
find anywhere in the tree where non-lowercase forms are being used and there is
nothing to be gained from case-insensitivity.

These are the only 2 changes compared to the last patch.

Comment 24

[Jean-Francois Ducarroz]

Comment on attachment 149467 [details] [diff] [review]
patch 5

looks good.

Comment 25

[Brodie]

Would someone please check this in for me. I don't have write access to CVS so 
I can't do it myself. 

Comment 26

[David :Bienvenu]

reassign...

Comment 27

[David :Bienvenu]

fixed

Comment 28

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using LXR:

1.122	bienvenu%nventure.com	2004-06-01 10:22	 	clean up stream converter code,
243888, patch by [e-mail], r=ducarroz, sr=bienvenu