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)
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
Comment 1•24 years ago
|
||
Setting bug status to New. Bugs whaich have been triaged to a target milestone
should not be Unconfirmed.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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 5•24 years ago
|
||
triaging.
Updated•24 years ago
|
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.
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.
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.
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.
Updated•23 years ago
|
Keywords: helpwanted,
nsbeta1
Assignee | ||
Comment 11•23 years ago
|
||
Assignee | ||
Comment 12•23 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.
Assignee | ||
Comment 13•23 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
Comment 14•23 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?
Assignee | ||
Comment 15•23 years ago
|
||
Assignee | ||
Comment 16•23 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.
Assignee | ||
Comment 17•23 years ago
|
||
ccing jag for wisdom on strings
Comment 18•23 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.
Comment 19•23 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.
Assignee | ||
Comment 21•23 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?
Priority: P3 → P1
Target Milestone: --- → mozilla0.9.2
Reporter | ||
Comment 22•23 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
Assignee | ||
Comment 23•23 years ago
|
||
Comments from bovik@best.com:
> Should extension comparisons also be case-insensitive?
Yes; .doc = .DOC
Comment 24•23 years ago
|
||
*** Bug 84522 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 25•23 years ago
|
||
Assignee | ||
Comment 26•23 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.
Keywords: review
Assignee | ||
Comment 27•23 years ago
|
||
Oops. The last patch I attached causes a crash on trying to open a context
menu. Please disregard it. Attaching safer patch.
Assignee | ||
Comment 28•23 years ago
|
||
Assignee | ||
Comment 30•23 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.
Assignee | ||
Comment 31•23 years ago
|
||
Assignee | ||
Comment 32•23 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...
Target Milestone: mozilla0.9.2 → mozilla0.9.3
Assignee | ||
Comment 33•23 years ago
|
||
Assignee | ||
Comment 34•23 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.
Comment 35•23 years ago
|
||
*** Bug 80285 has been marked as a duplicate of this bug. ***
Comment 36•23 years ago
|
||
*** Bug 64586 has been marked as a duplicate of this bug. ***
Comment 37•23 years ago
|
||
*** Bug 79821 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 38•23 years ago
|
||
Assignee | ||
Comment 39•23 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...
Status: NEW → ASSIGNED
Assignee | ||
Comment 40•23 years ago
|
||
Assignee | ||
Comment 41•23 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.
Comment 42•23 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.
Assignee | ||
Comment 43•23 years ago
|
||
Akkana, you're probably seeing bug 88473...
Comment 44•23 years ago
|
||
Filed bug 88927 on the font crash.
Comment 45•23 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.
Comment 46•23 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?)
Comment 47•23 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.
Assignee | ||
Comment 48•23 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.
Assignee | ||
Comment 49•23 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
Assignee | ||
Comment 50•23 years ago
|
||
Comment 51•23 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.
Comment 52•23 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, " .... ");
Assignee | ||
Comment 53•23 years ago
|
||
*** Bug 90688 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 54•23 years ago
|
||
Comment 55•23 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 ...
Assignee | ||
Comment 56•23 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....
Assignee | ||
Comment 57•23 years ago
|
||
Assignee | ||
Comment 58•23 years ago
|
||
Assignee | ||
Comment 59•23 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... :)
Comment 60•23 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.
Comment 61•23 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.
Assignee | ||
Comment 62•23 years ago
|
||
Assignee | ||
Comment 63•23 years ago
|
||
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
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.
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.
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
Comment 67•23 years ago
|
||
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.
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.
Comment 70•23 years ago
|
||
Assignee | ||
Comment 71•23 years ago
|
||
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?
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? :)
Comment 74•23 years ago
|
||
r=jag
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.
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.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 77•22 years ago
|
||
*** Bug 144290 has been marked as a duplicate of this bug. ***
You need to log in
before you can comment on or make changes to this bug.
Description
•