52441 - System and user mailcap and mime.types should be used

Status: VERIFIED FIXED

(Core :: Networking, defect, P1)

Description

[Eric Vaandering (no email)]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux 2.2.14 i686; en-US; m18) Gecko/20000908
BuildID:    2000090808

Mozilla should use, or at least allow importing data from, the system mailcap
and user's mailcap files. Having to re-enter all that information is a huge
waste of time.

If this is possible to do now, I could find no mention of how

Comment 1

[Asa Dotzler [:asa]]

Setting bug status to New.  Bugs whaich have been triaged to a target milestone
should not be Unconfirmed.

Comment 2

[Chris McAfee]

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

Comment 3

[R.K.Aa.]

In my ~/.mailcap both Acrobat reader, Realaudio and other are defined.
But even if /usr/local/mozilla/defaults/pref/unix.js have these line by default:


pref("helpers.global_mime_types_file", "/usr/local/lib/netscape/mime.types");
pref("helpers.global_mailcap_file", "/usr/local/lib/netscape/mailcap");
pref("helpers.private_mime_types_file", "~/.mime.types");
pref("helpers.private_mailcap_file", "~/.mailcap");

mozilla doesn't see a single of these plugins or helper apps. Nothing.
Which makes me believe it doesn't even attempt to read mailcap files.

The syntax in the files i have on disk are untampered with, and AFAIK all
created by Netscape Communicator 4.*.

Comment 4

[Gagan]

this is uriloader. ->mscott

Comment 5

[Scott MacGregor]

triaging. 

Comment 6

[robbe]

This has worked quite fine in NS4 (Linux). => 4xp
The corresponding functionality kinda works on Windows, too - i.e. Mozilla uses
the system registry there, to determine the right helper app. => pp

Please someone add these keywords ... I'm not allowed to.

Comment 7

[Ben Bucksch (:BenB)]

done.

Comment 8

[Mark H. Wood]

Still broken as of build 2001032614.  I strace'd mozilla and found no evidence
that it ever even attempts to open these files.

Comment 9

[Aleksey Nogin]

I straced Mozilla-0.8.1 with the same result - it does not seem to try to open
these files.

Comment 10

[Aleksey Nogin]

Reading mailcap files would not be as useful if Mozilla can not run the helper
apps specified there properly. It needs to be able to use $PATH correctly as
well as understand the command line arguments.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: partial implementation

Comment 12

[Boris Zbarsky [:bzbarsky]]

I have this partly working.  Patch is attached.  What it does:

1) Implements the nsOSHelperAppService::GetFromExtension and 
   nsOSHelperAppService::GetFromMIMEType methods on Unix.
2) Implements reading of mime.types files of both the Netscape and "other"
   variety.  The "other" variety is used by things like Apache and pine and so
   forth, apparently, and is often found in /etc/mime.types or
   /usr/local/etc/mime.types
3) Implements reading of mailcap files and extraction of the shell command to
   execute (still in mailcap syntax) and description (if any)

So with this patch file:// urls suddenly have correct mimetype mappings (if your
prefs are set up right).  http:// urls pick up pretty descriptions for the mime
types.  With the new "what to do" dialog that allows you to easily edit/save the
information mapping, this makes life much better.

What this does _not_ do is implement correctly launching the programs extracted
from mime.types.  That depends on bug 78919 getting fixed.  We may want to spin
complete mailcap support off into a separate bug.

Known problems:

1)  There is a lot of string stuff in this patch.  I was learning strings as I
    went along, so chances are there are places where things are pretty
    inefficient.  Any advice on improving it is welcome.
2)  The code uses nsIFileSpec, because I could not find a way to do a readline()
    with nsIFile and did not have the time to figure out how to work around that
    limitation.  Again, any advice, even just of the "figure out how to do this
   without readline()" variety is appreciated.
3)  The code has some variable-naming issues.  Those will get fixed as it gets
    closer to its final form as regards problems #1 and #2.

Comment 13

[Boris Zbarsky [:bzbarsky]]

More known problems:

4)  GetExtensionsFromRegularFile returns a whitespace-separated extension list.
    It needs to return a comma-separated list
5)  This patch does not handle things like 'image/*' for the mimetype in mailcap
    files

Comment 14

[robbe]

Great that someone is working on this! I'm still compiling a test build.

> What this does _not_ do is implement correctly launching the programs extracted
> from mime.types.  That depends on bug 78919 getting fixed.  We may want to spin
> complete mailcap support off into a separate bug.

Hmm, many of my mailcap entries are of the "/some/app '%s'" format. Maybe this
case could be (dirtily) hacked to work until 78919 is squashed?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Better patch

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment 35036 [details] [diff] addresses issues #2,#3,#4, and #5 with the first patch and
makes some progress on issue #1.  It also includes the "dirty hack"
[e-mail] mentions.  For now we assume that all the mailcap handlers
are of the form 'foo %s' where foo is either an absolute path or an executable
in $PATH.

If people decide to test this patch (which I would greatly appreciate), they
will also need to apply the patch from bug 81165 since I use that ReadLine()
function.

I'm going out of town till June 10.  So if someone else wants to start off with
this, clean it up a bit, and get it checked in.  If we do that, please spin off
a bug on taking out the "dirty hack" once bug 78919 is fixed.

Comment 17

[Boris Zbarsky [:bzbarsky]]

ccing jag for wisdom on strings

Comment 18

[robbe]

The 2nd patch works wonders! Type detection on non-http sources is fine, as is
selecting of the right application to start. (Apps needing commandline switches
have problems, of course, resolution depending on bug 78919.)

(It works so well that I'm now asked to start "wine" when opening    
ftp://.../README.dll
<bg> Maybe we need an "open in mozilla, dammit" option, but that's for another bug.)

Mozilla people: please consider un-futuring, in the light of
* 4xp and pp
* its #50 on the browser vote-charts
* an unpolished, but working patch is here

So this basically needs review now.

Comment 19

[timeless]

Well, last i heard bz was trying to find a good way to handle \\\\\%s and stuff 
like that ... strings and fun foo.

Comment 20

[benc]

mass move, v2.
qa to me.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Actually, the \\\\\%s stuff is needed for bug 83305 and is not an issue till bug 
78919 is fixed.  The problems _I_ still have with my second patch (going to 
keep consecutive numbering of problems going):

6)  Some of the comparisons should be case-insensitive.  In particular mime
    types are case-insensitive.  Should extension comparisons also be 
    case-insensitive?  (My gut instinct is they should be).
7)  The current parsing of mime.types and mailcap files is very likely
    inefficient.  Would using iterators and a sink make it more efficient?  
    Ideally we should have a one-pass parse of each entry with minimal string
    creation as we go along.
8)  There are two functions to parse each type of mime.types file, for four
    parsing functions total.  It would be better to write two functions (one for 
    each type of file) that parse a mime.types entry and return a struct with
    the info from the entry.  That way the parsing code could be separated from
    the code to read the file and the logic for comparing what you have to the
    entry.  This may well also be faster if the "parse an entry" function is
    well-written.

I'll get back to work on this on June 10.  In the meantime, do people see any 
other problems with the patch?

Comment 22

[Eric Vaandering (no email)]

6)  Some of the comparisons should be case-insensitive.  In particular mime
    types are case-insensitive.  Should extension comparisons also be 
    case-insensitive?  (My gut instinct is they should be).


This (the mime-types) is the topic of bug 59619

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comments from [e-mail]:

> Should extension comparisons also be case-insensitive?

Yes; .doc = .DOC

Comment 24

[timeless]

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

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch that uses case-insensitive comparisons

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment 38087 [details] [diff] uses case-insensitive comparisons and cleans up the parsing a
bit.  It has some debugging fprintfs that need to be removed, but other than
that, I'm fairly happy with it.  Reviews?  Again, you'll need the patch from bug
81165 to get this to compile/run.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Oops.  The last patch I attached causes a crash on trying to open a context
menu.  Please disregard it.  Attaching safer patch.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: non-crashing patch

Comment 29

[Boris Zbarsky [:bzbarsky]]

nominating for beta1 and 0.9.2

Comment 30

[Boris Zbarsky [:bzbarsky]]

That last patch has a known crash associated with it.  Opening a file from a
file:// url will crash if we find no info on it in mime.types.  I've fixed the
crash in my tree and I'll post an updated patch once I also iron out some string
stuff that jag pointed out to me.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch version 5

Comment 32

[Boris Zbarsky [:bzbarsky]]

Attachment 38929 [details] [diff] has the following changes:

* Builds against the latest patch in bug 81165
* Has some parsing improvements
* #ifdefs all the debug printfs
* uses nsAReadableString and nsAWritableString throughout for argument types
* fixes a bunch of return values to be more meaningful (fixing the crash I
  mentioned in my 2001-06-13 20:48 comment in the process)

Comments?  This is looking like it won't make 0.9.2, so pushing out to 0.9.3...

Comment 33

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch version 6

Comment 34

[Boris Zbarsky [:bzbarsky]]

Attachment 39149 [details] [diff] has the following changes: 

* more parsing improvements (for mailcap files)
* start the helper without asking the user for confirmation from the user if a
  helper is found and does not have x-mozilla-flags=prompt set.  This would make 
  us consistent with NS 4.x behavior (change made per discussion with mpt).  If
  the prompt flag is present, we bring up the helper app dialog.

Comment 35

[timeless]

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

Comment 36

[benc]

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

Comment 37

[benc]

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

Comment 38

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch version 7

Comment 39

[Boris Zbarsky [:bzbarsky]]

Attachment 40105 [details] [diff] has the following changes:

* works against the latest patch in bug 81165
* converts most of the string stuff to using iterators
* improves option parsing for mailcap files to make it more extensible
* adds support for the "test=" clause in mailcap entries

Jag, could you please take a look at this?  I'd really like to get it in by 0.9.3

A general question.  Right now I'm using Unicode strings throughout this patch.
 Would it make more sense to use CStrings?   mime.types and mailcap files don't
really support Unicode anyway as far as I can tell...

Comment 40

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch version 8

Comment 41

[Boris Zbarsky [:bzbarsky]]

Attachment 40797 [details] [diff] has the following changes:

* Make the application description not be an empty string -- otherwise the fix to
  bug 88066 makes things fail badly
* Adds a (for now empty) hash table to deal with mimetype options and substituting 
  them into commands.

Comment 42

[Akkana Peck]

I tried patch version 8, but I"m having trouble testing it.  I made new .mailcap
and .mime.types entries corresponding to a new extension, went to a page and
clicked on a link with that extension, and I crashed.  The crash wasn't in this
code, but in font code:
#0  0x41642d7e in nsFontGTKNormal::GetWidth (this=0x8841c60, 
    aString=0x8569a50, aLength=1) at nsFontMetricsGTK.cpp:2012
#1  0x41651884 in nsRenderingContextGTK::GetWidth (this=0x8541c38, 
    aString=0x85699b8, aLength=143, aWidth=@0xbfffb2dc, aFontID=0x0)
    at nsRenderingContextGTK.cpp:1327
So I don't think this had anything to do with this patch, but I may not be able
to test it until I get past it.  I'll file a bug to bstell on the font crash.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Akkana, you're probably seeing bug 88473...

Comment 44

[Akkana Peck]

Filed bug 88927 on the font crash.

Comment 45

[jag (Peter Annema)]

Instead of doing a #define of those two strings and their length, you could
#define FOO NS_LITERAL_STRING("foo") and then do a == (or .Equals()) on your
string and FOO, since in this case "all" you're doing with them is
aStr.EqualsWithConversion(FOO, PR_FALSE, FOO_LEN).

Also, in LookUpTypeAndDescription, you have:

+  nsXPIDLCString mimeFileNameStr;
+  const char* mimeFileName;

Then you getter_Copies into mimeFileNameStr and put the result of its .get()
into mimeFileName. You could actually get rid of const char* mimeFileName and
directly use the nsXPIDLCString and pass in the .get() to
GetTypeAndDescriptionFromMimetypesFile, or even better yet you make that
function take a nsACString (or nsAFlatCString) and directly pass in the
nsXPIDLCString. If you make it take a nsACString, you could use
PromiseFlatCString() to be able to do .get() so you can use it with
InitWithPath(). PromiseFlatCString is (iirc) cheap if the nsAString passed in
already is a flat string.

Hrm, actually, I think you should be using CopyUnicharPref and nsXPIDLStrings,
and InitWithUnicodePath, to support non-ascii filenames.

I would model FindSemicolon after FindInReadable, where the begin iterator is
non-const and adjusted to point at what you were looking for, or point beyond
the end of the string and be equal to the end iterator. Also, I would drop the
Distance check at the start of that function, since you're only using it
internally and should not be able to run into a case where your begin iter comes
after your end iter, or should pretty much be able to avoid it in the call sites
(by putting the check where it's really needed, instead of burdening everyone
with it).

Generally I would group the "in" and "out" iterators on functions together,
e.g.:

nsresult
ParseMIMEType(const nsReadingIterator<PRUnichar>& aStart_iter,
              const nsReadingIterator<PRUnichar>& aEnd_iter,
              nsReadingIterator<PRUnichar>& aMajorTypeStart,
              nsReadingIterator<PRUnichar>& aMajorTypeEnd,
              nsReadingIterator<PRUnichar>& aMinorTypeStart,
              nsReadingIterator<PRUnichar>& aMinorTypeEnd) {

though I guess there's something to be said for indicating internal order of
iterators by having the aEnd_iter as the last param. *shrug* Up to you I guess,
unless someone can make a compelling case one way or the other.

+  while (aMinorTypeEnd != aEnd_iter &&
+         (thechar = *aMinorTypeEnd) != ' ' &&
+         thechar != 't' &&
+         thechar != ';') {
+    ++aMinorTypeEnd;

I'm wondering if any gain from this optimization is worth making the code
(slightly) less readable and if a decent compiler couldn't make this
optimization itself.

In GetTypeAndDescriptionFromMimetypesFile, is aFileExtension ASCII or UTF8? If
the latter, you'll want to use a different conversion (CopyUTF8toUCS2, if such a
beast exists yet, NS_ConvertUTF8toUCS2 otherwise) than AssignWithConversion
(which is ASCIItoUCS2).


+      localFile->InitWithUnicodePath(PromiseFlatString(Substring(start_iter,
colon_iter) +
+                                                       NS_LITERAL_STRING("/") +
+                                                       appPath).get());

I guess this will always work on Unix, though technically you should be doing
InitWithUnicodePath for the PromiseFlatString(Substring()), then
AppendRelativeUnicodePath for appPath.

Comment 46

[timeless]

colin: openvms is slightly different from normal unices, can you check to see 
if this code would work? (does openvms even have mailcap/mime.types?)

Comment 47

[Colin Blake]

OpenVMS doesn't have mailcap and mime.types files, BUT, what we've done on
previous versions of the browser is to just leave that code in. So, if a user or 
an application creates the files, then the browser can happily use them.

Comment 48

[Boris Zbarsky [:bzbarsky]]

> you could #define FOO NS_LITERAL_STRING("foo") and then do a == (or .Equals())

Well, I could.... would that do the same thing, though?  That is, what if there
are trailing spaces on that first line?  (I'd want equality in that case).
|.Equals()| does not handle that 

> Hrm, actually, I think you should be using CopyUnicharPref and nsXPIDLStrings,
> and InitWithUnicodePath, to support non-ascii filenames.

So I should. Done.  Also passing in the XPIDLString as an nsAReadableString and
calling PromiseFlatString + get() on that.

> I would model FindSemicolon after FindInReadable, where the begin iterator is
> non-const and adjusted to point at what you were looking for. Also, I would
> drop the Distance check at the start of that function

OK.  Done.

> +         (thechar = *aMinorTypeEnd) != ' ' &&
> I'm wondering if any gain from this optimization is worth making the code
> (slightly) less readable

Probably not.  I've gotten rid of all the code that looked like that.
> I guess this will always work on Unix, though technically you should be doing
> InitWithUnicodePath for the PromiseFlatString(Substring()), then
> AppendRelativeUnicodePath for appPath.

Yikes. Right you are.  Fixed that.

Comment 49

[Boris Zbarsky [:bzbarsky]]

Oh.  Another thing...

> In GetTypeAndDescriptionFromMimetypesFile, is aFileExtension ASCII or UTF8?

It's probably safer to assume UTF8.  Changed that code to use NS_ConvertUTF8toUCS2

Comment 50

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch version 9 -- updated based on jag's comments

Comment 51

[jag (Peter Annema)]

>> you could #define FOO NS_LITERAL_STRING("foo") and then do a == (or
>> .Equals())
>>
> Well, I could.... would that do the same thing, though?  That is, what
> if there are trailing spaces on that first line?  (I'd want equality in
> that case). |.Equals()| does not handle that

Doh, you're right. I keep messing up with that one :-)

So that would then become (until |StartsWith(aStr, FOO)| is written):

Substring(aStr, 0, FOO.Length()).Equals(FOO)

In which case FOO will be created twice, which I guess you'd want to prevent.

Let me talk to some people to see if there's a cleaner way to do what you want
to do.

Comment 52

[jag (Peter Annema)]

Hrm, on second thought, I think the best solution here is to refactor
|GetTypeAndDescriptionFromMimetypesFile(...)| and
|GetExtensionsAndDescriptionFromMimetypesFile(...)|, since these functions
essentially do the same kind of thing and share a large chunk of code.

Once that's done, you can replace those #defines in the header with this chunk
of code in the refactored function:

NS_NAMED_LITERAL_STRING(netscapeHeader, " .... ");
NS_NAMED_LITERAL_STRING(MCOMHeader, " .... ");

Comment 53

[Boris Zbarsky [:bzbarsky]]

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

Comment 54

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch version 10 -- refactored the two Get*FromMimetypesFile functions and got rid of the header #defines

Comment 55

[timeless]

+  while (aMinorTypeEnd != aEnd_iter &&
+         *aMinorTypeEnd != ' ' &&
+         *aMinorTypeEnd != 't' &&
+         *aMinorTypeEnd != ';') {
+    ++aMinorTypeEnd;
+  }

+    if (!buf.IsEmpty() && buf.First() != '#') {
+      entry.Append(buf);
+      if (! entry.IsEmpty()) {
Can entry.IsEmpty() be true here?

+  } while (end_iter != start_iter &&
+           (*end_iter == ' ' ||
+            *end_iter == '\t'));
you use this construct a lot (or the reverse) can you use a function or macro 
to make this cleaner?

perhaps NOT_WHITE_AND_NOTPAST(end_iter,start_iter)

+    if (!buffer.IsEmpty() && buffer.First() != '#') {
+      entry.Append(buffer);
+      if (!entry.IsEmpty() && entry.Last() == '\\') {  // entry continues on 
next line
same question about entry being empty ...

Comment 56

[Boris Zbarsky [:bzbarsky]]

> Can entry.IsEmpty() be true here?

Actually, no.  That seems to be left over from when I went through and added
empty-checks to all places where I called First() or Last()

> you use this construct a lot (or the reverse) can you use a function or macro
> to make this cleaner?

That makes sense.  Macro it is....

Comment 57

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch version 11 -- add the macros timeless suggests, eliminate redundant tests, make it actually compile (unlike 10). Oops.

Comment 58

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch version 11 -- replace while(1) loops with do { } while() loops

Comment 59

[Boris Zbarsky [:bzbarsky]]

OK.  That patch fixes up some code style nitpicks that timeless found.  Jag,
Akkana, could you give this another look?  I think I'll wait for comments from
you both before creating another patch... :)

Comment 60

[Akkana Peck]

Boris and I worked through some problems I was seeing (didn't work for the last
line in the file, which coincidentally happened to be the type I was testing),
and he made a few changes and now it's working fine for me.  I'll run with it
and comment if I see any problems, but it looks good at this point.

Comment 61

[jag (Peter Annema)]

I'd rather you use an (inline) function than a macro, since that's basically
what you seem to want here. More later.

Comment 62

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch version 12 (at jag's request) -- use functions, not macros. Also fixes the "last line in file" thing

Comment 63

[Boris Zbarsky [:bzbarsky]]

Looks like this won't make 0.9.3 unless a miracle of review happens today. 
Pushing out.

Comment 64

[kgonzale]

Is this going to make it into Mozilla 0.9.4?  It would be VERY helpful if it was.

Also, what additional testing needs to be done on this?  I would be glad to help
to get this pushed out.

Comment 65

[Boris Zbarsky [:bzbarsky]]

Here's the deal.  I'm leaving this Thursday for about two weeks.  If I can get
reviews and whatnot before then, great.  If not, I'll attach the then-current
state of this patch to this bug and hope that someone pushes it through.

Once I get back, if this is not done yet I'll try some more to get reviews for it.

Comment 66

[Scott Collins]

Where possible, when working on some string class, |T|, prefer the type
|T::const_iterator|  (or |T::iterator| as appropriate) over some more specific
declaration like |nsReadingIterator<PRUnichar>|.  Worst case, you get the same
thing; best case the world has changed, you're working on a single-fragment
string, and suddenly you're working with pointers instead of big class instances.

You might find the types |const nsAString&| and |nsAString&| (or the `C' string
equivalents) easier to type than |nsAReadableString&| and |nsAWritableString&|.
 The readable and writable types are |typedef|s for those incantations which I
provide for compatibility with older code that used those names when readable
and writable really were distinct types.  New code should prefer the simpler and
more idiomatic |const nsAString&| and |nsAString&| forms.

Not illegal, but your |DistinctAnd...| routines look heavily over-parenthesized
:-)  I would have written, e.g.,

  return iter1 != iter2 && (*iter1 == ' ' || *iter1 == '\t');

Good use of |inline| functions, by the way.


  thePrefsService->CopyUnicharPref((char*) "helpers.private_mime_types_file",

Ugh!  I assume you're doing this because |CopyUnicharPref| is not
|const|-correct.  Avoid old-style casts.  The new casts tell the reader what
you're really doing, and report warnings or errors if there's a problem. 
Old-style casts always compile, even when wrong, because they turn into a
|reinterpret_cast| in the worst case.  For this instance, if my assumption is
correct, you probably meant to say something like

  NS_CONST_CAST(char*, "helpers...")

Good use of |PromiseFlatString|, though, since we own |InitWithUnicodePath|,
ideally that code should be updated to accept a |const nsAString&|.  Not your
code, I know.  Somebody should file a bug.

Wow, you are really wanting |StartsWith| to exist, I see.  Hmm.  To me this
looks like another good case for a nearby |inline| which contains the two named
literal declaration and returns a |PRBool|.  Then your code just says

  *aNetscapeFormat = IsNetscapeFormat(aBuffer);

...with no loss in efficiency.

In |GetTypeAndDescriptionFromMimetypesFile|, is |entry| likely to end up being
64 characters or less?  If not, you probably want to use the heap based string
and |SetCapacity| in advance to something large enough.  If it _is_ likely to be
short, you are doing exactly the right thing.  |Append| is cheap for stack
buffers like |nsAutoString|, and lets you slide around the Avoid `modify
in-place' Rule that I mention below.

Oh my.  I hate to see |.Trim| but I don't have a working substitute for you yet.
 |Trim| is inefficient among other things.  The general rule is, if you can,
avoid modifying strings in-place.  Try to build up your answer as one big
expression, since that will do the minimal amount of shifting around of
characters.  If I had a generator in place for |Trim|, you would probably want
to use the generator instead.  In the meantime, keep the `avoid in-place
modifications' rule in mind.

The iterators are simple to use (when treating them like a pointer), but bear in
mind the overhead of advancing them (about twice the cost of advancing a raw
pointer).  You'll want to profile your code at some point to determine (in loops
heavy with iterator manipulation) whether to exploit string iterator's `chunky'
properties with |.size_forward()| and |.get()|, or even to rewrite certain loops
as character sinks or as generators.  What you have now is fine, and the cost
may be negligible, but it'll be worth measuring.

There is an |RFindInReadable| you might want to use, though it takes a string,
not a character.

You really love |do|..|while| loops :-)  Please be careful.  |do|..|while| loops
are the most fragile of loops for maintainance, since they always execute at
least once.  _You_ might be terrific at never missing the edge case, but people
who come later to enhance your code probably won't be.  Prefer the more
idiomatic |for| when applicable.  See "The Practice of Programming" for more on
this topic.


I see lots of places where you say the equivalent of

  if ( expr )
    DoSomething(PR_TRUE);
  else
    DoSomething(PR_FALSE);

To my eye, these seem better as

  DoSomething( expr );

or if you're really concerned about using the exact values (typically not a problem)

  DoSomething( expr ? PR_TRUE : PR_FALSE );

Or if the result would be _really_ long and/or complicated, use a temporary
|PRBool|, e.g.,

  PRBool shouldPrompt = (mozillaFlags == NS_LITERAL_STRING("prompt"));
  mimeInfo->SetAlwaysAskBeforeHandling(shouldPrompt);

I did not dive down into the machinery of your exact iterator manipulations.  If
you need me to do that, I will (though it will take time).  My overall
impression is that this a very mature patch.  Good use of strings; I wish I had
some better tools for you (and I will, soon).  Sorry for the delay in this review.

Hope this helps

Comment 67

[jag (Peter Annema)]

Attachment: [email] Comments I e-mailed to bz

Comment 68

[jag (Peter Annema)]

For the record: those comments were sent a while back, some of them have been
addressed (iirc), some of them are superceded by scc's more recent comments.

Comment 69

[Scott Collins]

At 11:32 AM -0400 8/15/01, Boris Zbarsky wrote:
  >nsresult DoSomething(nsReadingIterator<PRUnichar> arg1,
  >                     nsReadingIterator<PRUnichar> arg2);
  >
  >nsString::const_iterator foo1, foo2;
  >DoSomething(foo1, foo2);
  >
  >Is this a bad idea?  That is, what should I declare function
  >arguments as if I want to be able to pass arbitrary iterators
  >to them while using class-specific iterators in my code?

(In general) You'll want to declare the formal arguments to be the general
iterators, i.e., |nsAString::const_iterator|.  However, that won't satisfy the
problem of making |DoSomething| applicable to both raw pointer iterators and
chunky iterators.  To do that you'll end up using function overloading, when it
matters, e.g.,

  nsresult DoSomething( nsAString::const_iterator&,
                  const nsAString::const_iterator& );

  nsresult DoSomething( nsASingleFragmentString::const_iterator&,
                        nsASingleFragmentString::const_iterator );

Until such time as it matters, only write the former, and ensure you get the
thing you want by, in callers, also using the general type,

  nsAString::const_iterator foo1, foo2;
  // ...
  DoSomething(foo1, foo2);

Your final option is templatize |DoSomething| on the type of the iterator, but
this is not recommended.  It's a complicated route to make something accept
either raw pointers or chunky iterators efficiently (see |copy_string| for an
example).

To summarize: my advice for now is to use |nsAString::const_iterator| in
argument lists and for your local declarations.  Later, if and when it pays to
do so, you may write specialized versions of the functions to take
|nsASingleFragmentString::const_iterator|.  At that time, callers who have a
known single fragment string can fix their local declarations to exploit the
more specialized routines.  This advice will soon apply to substrings as well. 
I'll paste this advice into the bug for other readers.

Comment 70

[jag (Peter Annema)]

Attachment: [patch] This is the patch I commented on. Sent in private e-mail, never attached here apparently.

Comment 71

[Boris Zbarsky [:bzbarsky]]

Attachment: [patch] Patch updated to reflect scc's and jag's comments

Comment 72

[Akkana Peck]

All this string class discussion is useful in general, but nobody's gonna find
it here.  I wonder if anyone is capturing this advice to make a new string
document on mozilla.org, findable via the search page, since the old docs are
out of date?

Comment 73

[Boris Zbarsky [:bzbarsky]]

OK.  I've gotten rid of nsReadingIterator<PRUnichar> in favor of
nsAString::const_iterator throughout (the iterators involved _are_ being passed
around, so I can't make them nsString::const_iterator).

> Not illegal, but your |DistinctAnd...| routines look heavily
> over-parenthesized

I've replaced them with nsCRT::IsAsciiSpace in any case, per jag's recommendation

> thePrefsService->CopyUnicharPref((char*) "helpers.private_mime_types_file",

That's an artifact of using a different pref-getter before I switched to
CopyUnicharPref... and also possibly an artifact of pre-pref-landing prefs.

I removed the cast and things are peachy.

> *aNetscapeFormat = IsNetscapeFormat(aBuffer);

Good idea.  Done.

> In |GetTypeAndDescriptionFromMimetypesFile|, is |entry| likely to end up being
> 64 characters or less? 

Using nsString with SetCapacity(100) now.  That's plenty, based on my survey of
several mime types files.

This code could use some profiling, yes.  I would bet the vast majority of the
time is spent actually reading data off the hard drive, though.  Seeing as we
reread the files every time though...

Eventually, the mime system needs an architectural solution to allow the
contents of these files to be cached or something like that.

> You really love |do|..|while| loops :-)  Please be careful.

Actually, this is the first time I've really used them.  I feel that they fit
the logic flow a lot better than anything else (I could use an equivalent for
loop, but it would feel forced).  If you really feel a for loop would be better
I can change it, of course.

> To my eye, these seem better as DoSomething( expr );

So they do.  Fixed.

so... if people are happy with this one, would someone mind putting r= or sr= on
it?  :)

Comment 74

[jag (Peter Annema)]

r=jag

Comment 75

[Scott Collins]

sr=scc, given that you've made the changes and jag has investigated to his
satisfaction your exact iterator manipulations.

Comment 76

[Boris Zbarsky [:bzbarsky]]

Checked in.  Further enhancements (ui for editing the file locations, sane
defaults for the file locations in non-migrated profiles, improvements in
handling of mailcap entries) are either already filed or should be filed as
separate bugs.  resolving this baby fixed.

Comment 77

[timeless]

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

Comment 78

[benc]

-> File handling

Comment 79

[sairuh (rarely reading bugmail)]

rs vrfy.