Eric Vaandering (no email) Reporter
	Description • 24 years ago

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

	Asa Dotzler [:asa]
	Comment 1 • 24 years ago

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

	Chris McAfee
	Comment 2 • 24 years ago

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

	R.K.Aa.
	Comment 3 • 24 years ago

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.*.

	Gagan
	Comment 4 • 24 years ago

this is uriloader. ->mscott

	Scott MacGregor
	Comment 5 • 24 years ago

triaging. 

	robbe
	Comment 6 • 24 years ago

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.

	Ben Bucksch (:BenB)
	Comment 7 • 24 years ago

done.

	Mark H. Wood
	Comment 8 • 24 years ago

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

	Aleksey Nogin
	Comment 9 • 24 years ago

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

	Aleksey Nogin
	Comment 10 • 24 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 12 • 24 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 13 • 24 years ago

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

	robbe
	Comment 14 • 24 years ago

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?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 16 • 24 years ago

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"
robbe@orcus.priv.at 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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 17 • 24 years ago

ccing jag for wisdom on strings

	robbe
	Comment 18 • 24 years ago

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.

	timeless
	Comment 19 • 24 years ago

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

	benc
	Comment 20 • 24 years ago

mass move, v2.
qa to me.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 21 • 24 years ago

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?

	Eric Vaandering (no email) Reporter
	Comment 22 • 24 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 23 • 24 years ago

Comments from bovik@best.com:

> Should extension comparisons also be case-insensitive?

Yes; .doc = .DOC

	timeless
	Comment 24 • 24 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 26 • 24 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 27 • 24 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 29 • 24 years ago

nominating for beta1 and 0.9.2

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 30 • 24 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 32 • 24 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 34 • 24 years ago

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.

	timeless
	Comment 35 • 24 years ago

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

	benc
	Comment 36 • 24 years ago

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

	benc
	Comment 37 • 24 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 39 • 24 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 41 • 24 years ago

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.

	Akkana Peck
	Comment 42 • 24 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 43 • 24 years ago

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

	Akkana Peck
	Comment 44 • 24 years ago

Filed bug 88927 on the font crash.

	jag (Peter Annema)
	Comment 45 • 24 years ago

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.

	timeless
	Comment 46 • 24 years ago

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

	Colin Blake
	Comment 47 • 24 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 48 • 24 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 49 • 24 years ago

Oh.  Another thing...

> In GetTypeAndDescriptionFromMimetypesFile, is aFileExtension ASCII or UTF8?

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

	jag (Peter Annema)
	Comment 51 • 24 years ago

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

	jag (Peter Annema)
	Comment 52 • 24 years ago

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, " .... ");

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 53 • 24 years ago

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

	timeless
	Comment 55 • 24 years ago

+  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 ...

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 56 • 24 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 59 • 24 years ago

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... :)

	Akkana Peck
	Comment 60 • 24 years ago

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.

	jag (Peter Annema)
	Comment 61 • 24 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 63 • 24 years ago

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

	kgonzale
	Comment 64 • 23 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 65 • 23 years ago

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.

	Scott Collins
	Comment 66 • 23 years ago

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

	jag (Peter Annema)
	Comment 68 • 23 years ago

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.

	Scott Collins
	Comment 69 • 23 years ago

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.

	Akkana Peck
	Comment 72 • 23 years ago

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?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 73 • 23 years ago

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?  :)

	jag (Peter Annema)
	Comment 74 • 23 years ago

r=jag

	Scott Collins
	Comment 75 • 23 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 76 • 23 years ago

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.

	timeless
	Comment 77 • 23 years ago

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

	benc
	Comment 78 • 22 years ago

-> File handling

	sairuh (rarely reading bugmail)
	Comment 79 • 22 years ago

rs vrfy.