Closed Bug 52441 Opened 24 years ago Closed 23 years ago

System and user mailcap and mime.types should be used

Categories

(Core :: Networking, defect, P1)

x86
Linux
defect

Tracking

()

VERIFIED FIXED
mozilla0.9.4

People

(Reporter: ewv, Assigned: bzbarsky)

References

(Depends on 1 open bug, Blocks 4 open bugs)

Details

(Keywords: platform-parity)

Attachments

(16 files)

(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), text/plain
Details
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
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
Target Milestone: --- → Future
Setting bug status to New. Bugs whaich have been triaged to a target milestone should not be Unconfirmed.
Status: UNCONFIRMED → NEW
Ever confirmed: true
*** Bug 3128 has been marked as a duplicate of this bug. ***
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.*.
this is uriloader. ->mscott
Assignee: gagan → mscott
triaging.
Keywords: mozilla1.0
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.
done.
Keywords: 4xp, pp
Depends on: 58811
Still broken as of build 2001032614. I strace'd mozilla and found no evidence that it ever even attempts to open these files.
I straced Mozilla-0.8.1 with the same result - it does not seem to try to open these files.
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.
Depends on: 56662, 57420
Blocks: 57342
Blocks: 78106
Depends on: 78919
Depends on: 54940
Attached patch partial implementation (deleted) — Splinter Review
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.
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
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?
Depends on: 81165
Attached patch Better patch (deleted) — Splinter Review
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.
ccing jag for wisdom on strings
Depends on: 51246
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.
Well, last i heard bz was trying to find a good way to handle \\\\\%s and stuff like that ... strings and fun foo.
Assignee: mscott → bzbarsky
Keywords: nsbeta1patch
Target Milestone: Future → ---
mass move, v2. qa to me.
QA Contact: tever → benc
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?
Priority: P3 → P1
Target Milestone: --- → mozilla0.9.2
Blocks: 83305
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
Comments from bovik@best.com: > Should extension comparisons also be case-insensitive? Yes; .doc = .DOC
*** Bug 84522 has been marked as a duplicate of this bug. ***
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.
Keywords: review
Oops. The last patch I attached causes a crash on trying to open a context menu. Please disregard it. Attaching safer patch.
Attached patch non-crashing patch (deleted) — Splinter Review
nominating for beta1 and 0.9.2
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.
Attached patch Patch version 5 (deleted) — Splinter Review
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...
Target Milestone: mozilla0.9.2 → mozilla0.9.3
Attached patch Patch version 6 (deleted) — Splinter Review
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.
*** Bug 80285 has been marked as a duplicate of this bug. ***
*** Bug 64586 has been marked as a duplicate of this bug. ***
*** Bug 79821 has been marked as a duplicate of this bug. ***
Attached patch Patch version 7 (deleted) — Splinter Review
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...
Status: NEW → ASSIGNED
Attached patch Patch version 8 (deleted) — Splinter Review
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.
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.
Akkana, you're probably seeing bug 88473...
Filed bug 88927 on the font crash.
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.
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?)
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.
> 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.
Oh. Another thing... > In GetTypeAndDescriptionFromMimetypesFile, is aFileExtension ASCII or UTF8? It's probably safer to assume UTF8. Changed that code to use NS_ConvertUTF8toUCS2
>> 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.
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, " .... ");
*** Bug 90688 has been marked as a duplicate of this bug. ***
+ 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 ...
> 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....
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... :)
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.
I'd rather you use an (inline) function than a macro, since that's basically what you seem to want here. More later.
Looks like this won't make 0.9.3 unless a miracle of review happens today. Pushing out.
Target Milestone: mozilla0.9.3 → mozilla0.9.4
Blocks: 95091
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.
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.
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
Attached file [email] Comments I e-mailed to bz (deleted) —
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.
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.
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?
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? :)
r=jag
sr=scc, given that you've made the changes and jag has investigated to his satisfaction your exact iterator manipulations.
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.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Blocks: 95504
No longer blocks: 95504
Blocks: 95594
No longer blocks: 95594
Blocks: 95504
No longer blocks: 95504
No longer depends on: 51246
Blocks: 95504
No longer depends on: 54940
Blocks: 54940
Blocks: 115041
*** Bug 144290 has been marked as a duplicate of this bug. ***
-> File handling
QA Contact: benc → sairuh
rs vrfy.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: