Status

()

Core
Networking
P5
normal
RESOLVED WONTFIX
16 years ago
8 years ago

People

(Reporter: Chak Nanga, Unassigned)

Tracking

(Depends on: 1 bug, {arch, topembed})

Trunk
arch, topembed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

16 years ago
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.

Comment 1

16 years ago
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.
(Reporter)

Updated

16 years ago
Blocks: 157137

Updated

16 years ago
OS: Windows XP → All
Hardware: PC → All

Comment 4

16 years ago
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?  

Comment 5

16 years ago
jjmata has the complete list.

Comment 6

16 years ago
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.

Comment 8

16 years ago
what is the plan here: 78919 is futured.

Comment 9

16 years ago
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?

Comment 10

16 years ago
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).

Comment 12

16 years ago
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....

Comment 14

16 years ago
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)).

Comment 16

16 years ago
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.  ;)

Comment 18

16 years ago
-> 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
Keywords: arch, mozilla1.2, topembed
Priority: -- → P2
Summary: Freeze additional necko interfaces(nsIMimeInfo, nsICacheService,...) → Freeze nsIMimeInfo
Target Milestone: --- → mozilla1.2beta

Comment 19

16 years ago
bz: yeah, good points re: system.  i hadn't carefully thought it through.

Comment 20

16 years ago
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.

Comment 22

16 years ago
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.  ;)

Comment 24

16 years ago
of course!  that works for me :)

Comment 25

16 years ago
Created attachment 102210 [details] [diff] [review]
v0 - preliminary patch; lacks JS changes and nsIHelperApp fu

i'm just attaching this patch here as a backup of the work i've been doing.

Comment 26

16 years ago
Created attachment 102552 [details] [diff] [review]
v0.4 - complete patch for XP_WIN and XP_UNIX

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

Comment 28

16 years ago
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.

Updated

16 years ago
Depends on: 86640

Comment 29

16 years ago
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 :(

Comment 30

16 years ago
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 ...

Comment 31

16 years ago
Created attachment 102744 [details]
discussion thread between timeless and bz on the subject of our mime / helperapp architecture

Comment 32

16 years ago
Marking as topembed+/nsbeta1+.
Keywords: topembed → nsbeta1+, topembed+

Comment 33

16 years ago
-> moz 1.3
Target Milestone: mozilla1.2beta → mozilla1.3alpha

Comment 34

16 years ago
Created attachment 104396 [details]
prototype interfaces

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?

Comment 37

16 years ago
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....
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).

Comment 41

16 years ago
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.

Comment 42

16 years ago
Created attachment 106534 [details]
comment from bz via email about benefits of getFromMIMETypeAndExtension

Updated

16 years ago
Target Milestone: mozilla1.3alpha → mozilla1.3beta

Comment 43

16 years ago
-> API tester.
QA Contact: benc → depstein

Updated

16 years ago
Target Milestone: mozilla1.3beta → mozilla1.4alpha

Updated

16 years ago
QA Contact: depstein → ashishbhatt

Updated

15 years ago
Keywords: mozilla1.2
Target Milestone: mozilla1.4alpha → mozilla1.4beta

Updated

15 years ago
Severity: major → normal
Priority: P2 → P5

Comment 44

15 years ago
not going to happen for 1.4
Target Milestone: mozilla1.4beta → mozilla1.5alpha

Updated

15 years ago
Depends on: 205895

Comment 45

15 years ago
dropping plus since this isnt going to happen for 1.4
Keywords: nsbeta1+, topembed+ → nsbeta1, topembed

Comment 46

15 years ago
adt: nsbeta1-
Keywords: nsbeta1 → nsbeta1-

Updated

15 years ago
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

Updated

14 years ago
Blocks: 270895

Updated

14 years ago
Blocks: 268520

Updated

12 years ago
Assignee: darin → nobody
Status: ASSIGNED → NEW
QA Contact: ashshbhatt → networking
Target Milestone: Future → ---
We no longer freeze interfaces.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.