Closed Bug 162116 Opened 22 years ago Closed 14 years ago

Freeze nsIMimeInfo

Categories

(Core :: Networking, defect, P5)

defect

Tracking

()

RESOLVED WONTFIX

People

(Reporter: chak, Unassigned)

References

(Depends on 1 open bug)

Details

(Keywords: arch, topembed)

Attachments

(4 files, 1 obsolete file)

Freeze the following interfaces: nsIMIMEInfo nsICacheService nsICookieService Darin's comments regarding freezing nsICacheService from an email thread: "basically, if you want to freeze nsICacheService, then it means freezing all of the cache interfaces because nsICacheService either directly or indirectly references all of the other cache interfaces. do we want to freeze all of the cache interfaces at this time? i hope we don't have to." We need to sort out all of this stuff in the Mozilla 1.2 timeframe.
afaik nsIMimeInfo needs work. as for cookie, i suspect i won't like it. i'll look at it later.
nsIMIMEInfo is in no state to be frozen. It needs a complete revamp instead. See bug 78919 and bug 86640 (this latter requires changes to the interface and is _the_ biggest problem in our helper app UI at the moment, imo). Freezing this would simply mean that our helper app code would need to move to nsIMIMEInfo2 to get anything useful done (like command-line arguments on helpers, non-sucky UI, etc). If we decide this interface _does_ need freezing, then we should address the issue now and redesign it before freezing it. The implementation need not be complete, but the interface needs to be if not correct, at least bearable. The current one is not.
Why does this need to be frozen? Do we really want to freeze all the cacheservice stuff? Who will be using it? As well as the mimeinfo stuff, the cookieservice needs to be changed to use nsAStrings rather than char*s before it is frozen.
Blocks: 157137
OS: Windows XP → All
Hardware: PC → All
darin said that nsIMimeInfo *had* to be frozen. I don't doubt him, but what exact functionality needs to be exposed. how do we need to clean up the current nsIMimeInfo or do we cut a new interface?
jjmata has the complete list.
From the meeting notes: [09/26/02] * Use an Enumerator rather than the array * Naming of functions to intercaps with lowercase first letters. * Run by plugins people to figure out if there are any new features that need to be supported. * Clean up MIME service that is inside Necko. * Clean up the DataURI member since it is probably not used. * Needs work if OS MIME type handling is necessary.
And the issues covered by bug 78919
what is the plan here: 78919 is futured.
Of the three issues mentioned in bug 78919 only #2 was talked about at the API review on 9/27/02. Darin mentioned some way this could be done (passing command-line arguments to the nsIFile) though I don't remember what it was. Darin? The third one is already fixed (which obviously did not require changing the nsIFile in nsIMIMEInfo, though I didn't read the whole bug description ...) and I am not sure of what the implications of #1 on nsIMIMEInfo would be. Any ideas?
the issues with nsIFile have to do with the fact that it is not always possible to represent a helper app with a nsIFile. bz has some examples, but basically nsIFile has no way of representing command line arguments. so, if we freeze nsIMIMEInfo with the helper app represented as a nsIFile, it means that we might end up having to create a new class type that aggregates nsLocalFile to allow support for command line arguments that may be required by a helper app (e.g., xdvi -paper us filename.dvi). so, we just need to think a bit about how we want to eventually support helper apps like this. nsIHelperApp perhaps?
jj, I've mentioned all three issues repeatedly... I've stressed #2, since that one is an issue on Windows as well, not just on Unix. #1 is also an issue on Windows, albeit a smaller one. #3 is most emphatically not fixed, leading to some correctness and security problems; if something gave you the idea that it is, please let me know what that was. nsIHelperApp is what I was thinking of... Note that while the _api_ for nsIHelperApp needs to support all this stuff the back end could, initially, not be implemented for some of these things (eg issue #3 from my list).
BZ: The bug related to issue #3 is marked fixed, therefore my comment. I also mentioned that I had not read the bug report, so it is entirely possible that the fix (I believe it was yours) avoided the actual root of the problem and fixed it elsewhere as a side-effect. Regardless, don't take my comments as indication that we are any closer to freezing nsIMIMEInfo than we actually are, I was just trying to relay what I had found our in reading two or three of the related bugs mentioned. I'll get out of the way and let you folks figure things out.
Actually, the bug on issue #3 is bug 83305 and is blocked by bug 78919. ;) We "support" mailcap. Sorta... but not correctly. So one possible suggestion. interface nsIHelperApp : nsISupports { attribute nsAString prettyName; void initWithFile(in nsIFile file); void setArguments(/* Need a function signature here */); void initWithString(in nsAString string); void launchOnFile(in nsIFile file); } Then we can move the prettyName logic into here and not have it living on nsIMIMEInfo where it's a little odd anyway. Not sure what the best way to setArguments would be....
yeah, this looks promising. how about if we say that setArguments takes a string that will be inserted between the executable name and the file name, like so: exec + [" " + args +] " " filename brackets meant to denote optional part.
Well... nsIProcess expects an arguments array. So if we want to launch nsIHelperApp via nsIProcess (and I think we do) we will need to convert arguments into an array at some point. Which will require parsing the string.... It _is_ better to have the parsing code localized, but if someone has separate arguments, they should not need to concatenate them only to have them (probably buggily) reparsed. Maybe we want "setArgumentsToString" and "setArgumentsToArray" (and eliminate the initFromString completely? I'm not sure I like that as much, since the unix impl of nsIHelperApp would set some bools when inited from a string (it would assume the string is to be passed to shell and mailcap-expansion is to be performed)).
should nsIProcess be modified? windows impl builds a single string from the args, mac impl ignores the args, and default impl passes arg array to NSPR. i guess we wouldn't want to invoke the "system" function under unix, though that would imply that our argument systax could take advantage of shell quoting syntax. or maybe that's not such a hot idea :-/
The OS2 impl builds a string as well. But the Unix one does not, and with good reason. You want to fork() and exec() instead of using system() for the following reasons: 1) system() is synchronous. 99% of the time we want asyncronous process spawning. 2) system() calls the shell on the string. This is a security hole waiting to happen when someone forgets to escape something. Add to that the fact that the escaping syntax will actually vary based on shell (yes, it should all be "sh" but in practice bash and vanilla sh have some subtle incompatibilities) and life sucks all around. So no, I don't think changing nsIProcess is a good idea. ;)
-> moz 1.2, changing summary to only include nsIMIMEInfo, since we're not freezing nsICacheService. i'm also introducing nsIMIMEInfoMac for the mac specific fields on nsIMIMEInfo. does nsIMIMEInfoMac need to be frozen as well?
Severity: normal → major
Status: NEW → ASSIGNED
Priority: -- → P2
Summary: Freeze additional necko interfaces(nsIMimeInfo, nsICacheService,...) → Freeze nsIMimeInfo
Target Milestone: --- → mozilla1.2beta
bz: yeah, good points re: system. i hadn't carefully thought it through.
bz: i'm not sure why initWithString would be necessary. how about this: interface nsIHelperApp : nsISupports { /* get/set "display" name of application */ attribute AString name; /* get/set location of helper app */ attribute nsIFile file; /* set additional arguments to be passed to the helper app */ void setArguments(in unsigned long count, [array, size_is(count)] in string args); /* set filename to be passed to the helper app; appended to arguments * unless arguments contain a %s in which case the filename replaces * the %s. */ void launchOnFile(in nsIFile file); }; does this interface seem sufficient? some questions: 1) should a nsIHelperApp returned from an immutable nsIMIMEInfo be mutable? seems ok given that helper app info is a global property. 2) should nsIMIMEInfo reference this interface directly, or should nsIMIMEInfo only expose a nsISupports parameter? seems like it would be nice to expose nsIHelperApp directly, but that means this IDL will be included in necko, which is ok, i guess. it also obviously means that nsIHelperApp will also need to be frozen.
The reason I sorta wanted "initWithString" is that that way the helper app implementation could itself decide what to do with the string. Eg Unix would take the string and spawn "/bin/sh", "-c", string But that knowledge could live in the OS-specific MIME services; I guess that makes more sense. I'm not sure I like that %s thing... For one thing, are we planning to do that on Windows? Or just on Unix? That feels like a system-dependent implementation detail, I guess... (this is the other reason I wanted initWithString -- that would allow the impl to "know" that the %s should be replaced as opposed to being meant literally). I'm torn about nsISupports.... I sort of like the idea, since we're writing this new interface and haven't had a good chance to see how usable it really is... but at the same time, we'd need to include some comment like "can be QIed to nsIHelperApp", no? Otherwise, it wouldn't be usable... I agree that the nsIHelperApp returned by an immutable nsIMIMEInfo should be mutable. That will allow people to change what helper is being used for a type even though they cannot change the type-to-extension mapping, which makes some sense to me.
seems to me that "/bin/sh", "-c", string could easily be configured using SetFile and SetArguments. if we allow an initWithString, then don't we have to parse out the arguments? doesn't that have the same problem that changing the API of nsIProcess would have?
It can. Just have to remember to escape %s in the argument string as needed... The idea I had initially was for the helper app object to _internally_ set itself to "/bin/sh", "-c", "string"; I would just pass in the "string". Like I said, I buy ditching initWithString; if we ever _really_ feel like we need it, we derive nsIHelperAppMailcap from nsIHelperApp. ;)
of course! that works for me :)
i'm just attaching this patch here as a backup of the work i've been doing.
bz and i discussed the nsIHelperApp thing some more. looks like there is some code that sets applicationDescription w/o setting preferredApplicationHandler. the XP_WIN GetTypeFromExtension code does this if it is going to use the default system handler. in other words, it becomes difficult to move the application description onto a nsIHelperApp interface. we sort of agreed that we might be able to keep the current interface and still allow for future expansion of the helper app by subclassing nsIFile, e.g.: nsIHelperAppFile : nsIFile. that way we could QI the preferredApplicationHandler to nsIHelperAppFile in order to set arguments and do other things. this isn't the greatest solution, but given the amount of time available it seems reasonable. it is the least variation from what we currently have implemented. so, now i just have to make this thing work on the mac, which is looking to be a bitch due to the fact that i'm trying to separate the mac specific nsIMIMEInfo fields off onto a separate nsIMIMEInfoMac interface. did you know nsLocalFileMac actually utilizes nsIMIMEInfo?? yeah, believe it! very sad, very sad :(
Attachment #102210 - Attachment is obsolete: true
As I mentioned yesterday (not sure if you got that message), see also the changes Bill had to make to the interface in bug 86640 (not yet checked in, but needs to be checked in at some point).
yeah, i noticed the changes to nsIMIMEInfo. i haven't incorporated those changes yet. i shouldn't have called the v0.4 patch complete... i only meant that it is completely functional. i still have work to do on it.
Depends on: 86640
from discussions with bz and timeless today, it would appear that we are far from being able to freeze this interface. or, looking at it from another point of view, freezing this interface would leave us with a nasty, legacy interface to support for ever and ever ;-) perhaps we need to re-evaluate the goal of freezing this interface. i don't much like rushing to redesign an interface just in time to freeze it. that just doesn't sit right. if the helper app / mime system must be redesigned, from the ground up it would seem, then what are we doing talking about freezing the existing interfaces? perhaps we need to consider adding higher level and much simpler embedding APIs for this stuff. i'm starting to see this as less of a reality for the mozilla 1.2 timeframe :(
Not trying to rush things. Can you include the content of the conversation with timeless and Boris here for reference? At least the outline of the areas that need addressing ...
Marking as topembed+/nsbeta1+.
Keywords: topembednsbeta1+, topembed+
-> moz 1.3
Target Milestone: mozilla1.2beta → mozilla1.3alpha
Attached file prototype interfaces (deleted) —
this attachment contains some rough cut stuff based on some discussions we've had. it isn't anywhere near complete. i still have a lot of investigation to do before finalizing something. i'm posting it here to get broader feedback. it turns out that most embedders only really need immutable access to nsIMIMEInfo. modifications should really come through a different interface. when i get more time, i want to look to see how to better incorporate some of the mapping done from mime type to internal handler / plugin handler to see if some of that couldn't be pulled into the mime service. at any rate, the prototype still assumes that mime handlers are external apps.
Still thinking about most of it, but a comment on GetFromFile/GetFromURI. First, it may be enough to only have the latter, unless consumers having an nsIFile but not an nsIFileURL are common. Second, I feel we should at least have GetFromFile -- while GetFromURI for non-file urls is identical to GetFromExtension, for file urls we can do a lot better. On mac it's type/creator, but on all OSes we could try actually opening the file and reading a few bytes to sniff the type; on Unix we could maybe run the "file" command; on BeOS iirc the filesystem contains metadata like the content type, etc.
Also, even the Windows version of GetFromFile/GetFromURI (for the case when the uri is a file url) could be a win... it would know that it need not bother with the content-type or anything and just use the extension... In fact, maybe the fuction on this interface should be: getMimeInfo(in ACString aMIMEType, in ACString aFileExt /* should this be AUTF8String? */ in nsIURI aURI, in nsIInputStream aData); again, leaving null those parts that are not relevant. That way if you have an actual URI you don't have to bother getting the extension and just pass the URI... Thoughts?
yes, i was thinking about something very similar myself, and yes... the extension argument should have been AUTF8String :-)
And if we discover that there's a perf-sensitive place that really only needs the content-type and not the full mime info, then we deal... I'm not aware of such places at the moment... (the unknown content decoder would be the only real contender).
Could we move forward on the nsIMIMEService part of this even if we're not totally happy with the nsIMIMEInfo? That part is a lot simpler and we're close to agreement on it, I think. And that part blocks a slew of helper app bugs, including the infamous extension-twiddling....
Blocks: 147679
One other thing... the current suggestion for nsIMIMEService assumes that aData is a buffered stream that can be read with ReadSegments. Is there an interface that we could demand that guarantees that? Because nsIInputStream does not.... If not, we should at least make it very clear in the comments (and specify whether the service will throw NS_ERROR_INVALID_ARG or the error code returned by ReadSegments or what if the stream is not segmented).
bz: actually, i now believe that passing a nsIInputStream is bad. the problem is that even if there is a first segment, it might not be large enough. for example, what if the first segment is only 1 byte in length (very unlikely, but possible). for this reason the "sniffer" API should take a simple byte array.
Target Milestone: mozilla1.3alpha → mozilla1.3beta
-> API tester.
QA Contact: benc → depstein
Target Milestone: mozilla1.3beta → mozilla1.4alpha
QA Contact: depstein → ashishbhatt
Keywords: mozilla1.2
Target Milestone: mozilla1.4alpha → mozilla1.4beta
Severity: major → normal
Priority: P2 → P5
not going to happen for 1.4
Target Milestone: mozilla1.4beta → mozilla1.5alpha
Depends on: 205895
dropping plus since this isnt going to happen for 1.4
adt: nsbeta1-
Keywords: nsbeta1nsbeta1-
Target Milestone: mozilla1.5alpha → Future
adding dependencies to some bugs that would rqeuire api changes to nsIMIMEInfo..
No longer blocks: 147679
Depends on: 56662, 57420, 147679, 221708, 235505
Blocks: 270895
Blocks: 268520
Assignee: darin → nobody
Status: ASSIGNED → NEW
QA Contact: ashshbhatt → networking
Target Milestone: Future → ---
We no longer freeze interfaces.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: