Boris Zbarsky [:bzbarsky] Reporter
	Description • 24 years ago

nsIMIMEInfo uses an nsIFile for preferredApplicationHandler

This means that:

1) A full pathname must be used in preferences for the application.  No $PATH
   resolution is done (bug 56662)
2) No arguments can be passed to the application (bug 57420)
3) There is no way to support the standard Unix mailcap syntax for application
   handlers, which allows arbitrary shell expressions.  Eg "grep foo %s | lpr"
   would be a perfectly valid application handler, where %s is replaced by the
   temp filename.  (bug 52441)

Maybe create a new interface (nsIPreferredContentHandler) and use that in
nsIMimeInfo.   That would allow us to be a lot more flexible about what we
support...

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 1 • 24 years ago

Adding dependencies.  This is pp and 4xp -- we fully support the system ways of
doing helpers on Mac and Win and 4.x works just fine on Linux.  Nominating for
1.0 -- this is core functionality and we should try to at least get the
architecture right for 1.0 even if we do not fix the various bugs blocked by
this.

ccing mscott.  ccing mpt since he expressed an interest in this stuff.

	Aleksey Nogin
	Comment 2 • 24 years ago

Are you sure about mozilla1.0? Personally, I believe that this is such a major
annoyance, that it might be reasoable to try to have it fixed by, say, 0.9.2 so
that all the dependent bugs could be fixed for 1.0.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 3 • 24 years ago

OK.  After some more research we have:

1)  nsIFile is no good on Unix.  We have to be able to store a semi-arbitrary
    string as a helper

2)  a string is no good on Mac.  A single path may well point to two different
    files.  This means that on Mac we _have_ to use nsIFile or something along
    those lines.

I'm not sure anymore that the idea of creating an nsIFile-derived thing is such
a great one.  It may be better to have a totally different class that has two
member variables -- an nsIFile, and a string that was used to initialize the
nsIFile...  ccing dougt.  Doug, is there a simpler solution I'm missing?

	Doug Turner (:dougt)
	Comment 4 • 24 years ago

Is there any reason that this interface needs to be XP?  Why not have a UNIX
only interface so that we can set mailcap syntax?

	Stuart Ballard
	Comment 5 • 24 years ago

1) XP is a good idea in general because there is an XP concept of "get me the
thing to do with this mime type". If it simplifies to an nsIFile on some
platforms, it still seems to me that it should be an XP wrapper around an
nsIFile because there are XP *semantics* involved.

2) Windows also supports command-line parameters, even if you don't see them
terribly often. Take a look at the shortcut in your start menu for 4.x mail - it
says '"c:\path\to\netscape.exe" -mail'. I guess it's rare on windows for
command-line parameters to be necessary for mime-type support, but the base OS
supports it, and Mozilla should too.

3) Mac OS X, being unix, will also support command-line parameters. That means
it may need a combination of the two sets of semantics, perhaps?

This is a big ugly thing that can't be platform-specific-ifdeffed away...

Btw, dougt, this isn't only for mailcap parsing, but also for the helper app UI;
see bug 57420 (no support for helper app command-line args).

	timeless
	Comment 6 • 24 years ago

take a look at netscape3's registry info for real player. it heavily abuses 
command line switches for mime types.  [Yes I don't like the fact that 
realplayer is using the old fassioned n3 entries when it should just use system 
entries, but that's far afield.]

	Gagan
	Comment 7 • 24 years ago

this is more of a URI loader issue and not necessarily critical for 0.9.1... but
I'd let him decide.

	benc
	Comment 8 • 24 years ago

mass move, v2.
qa to me.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 9 • 23 years ago

nsIHelperAppLauncher also assumes the helper app is an nIFile... this may or may
not be a good assumption there...

	Chris P. Ross
	Comment 10 • 23 years ago

I don't think I understand why this can't be a string.  Something about
it needing to be an nsIFile on MacOS <10?  I guess I don't see why
a single path may point to more than on thing on a Mac.  Sorry, been a while
since I've been deep into MacOS programming (but I have been, so I at
least think I *should* understand this)
It's becomming really annoying for me because of this blocking bug #57420.
I'd really like to be able to "properly" invoke helper app's that need
command line args.
Is there strong opposition to maknig an XP wrapper that can contain either
the string (including %s's and such a la NS 4.x) or an nsIFile if, in fact,
that is needed for the Mac?  I guess it'll be easier, if such a thing
is constructed, to tear apart the input string into some sort of nsIFile
holding the application, and references (in some form) for the arguments
and position for the temp file name.  Later, this can be improved so that
if the input string isn't a fully qualified path, you could do that path
work when going to fill in the nsIFile...
Anyway.  Mostly just looking into current state and feelings...

	timeless
	Comment 11 • 23 years ago

the simple answer is that you can have two drives with the same name.

absolute paths on macos (which are absolutely taboo) are of the form
<volumename>:<folderpath>:<file>
where <folderpath> is of the form ""|<folder>:<folderpath>
secondly, i should be able to tell mozilla to use DDE or AppleScript or
BMessages and i can't do any of those w/ a commandline.

Heck on macos there is no commandline so the appropriate way to launch something
w/ parameters would probably be applescript, and not whatever you thought you
were suggesting. (it's true you can just stick a file onto the stack and pray it
behaves, but i don't think you can stick a url like that).

so for the grand scheme of things, (and i'm 99% sure i'm confusing bugs, but i'm
tired -- sorry) this what i think i'm reading isn't good enough.

	Chris P. Ross
	Comment 12 • 23 years ago

Right.  Someone else explained that to me off-line.  Thanks.
And, aren't Apple Events still the way to invoke things on the
mac?  An OAPP (or ODOC if you need to specify the doc to open)
event?  If that's what you meant my AppleScript, then I apologize.
Haven't worked with AppleScript myself...  Anyway, no need for
furthur spam/commentary in the bug.  I've already been straightened
out.  :-)

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 13 • 23 years ago

Over to file handling....  I doubt mscott is ever going to even look at this.

I'm still thinking of doing this myself at some point, but not till summer....

	Bill Law
	Comment 14 • 23 years ago

Future-ing.

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 15 • 22 years ago

(marking All/All, since this seems to be an architecture bug, therefore affects
all platforms)

Wouldn't the easiest solution be something along these lines:
o HelperAppService gets a method "findHelperApp(mimeType)" or something like
that, it probably has such a method
o the resulting interface (let's call it nsIHelperApp) has a method like this:
run(nsIFile aFile);
which launches the helper with the file.
and maybe an attribute "wstring humanReadableDesc"

In other words: The caller does not care at all how the helper app is invoked.
It just knows that the interface can somehow run the helper with all the
platform-specific stuff it has to do, and with the platform-specific stuff
stored in it.


Comments on that approach?

Also, if what I said above is complete nonsense given how this stuff currently
works, please tell me, and I'll do some reading of how this stuff works and make
a better suggestion :)

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 16 • 22 years ago

Yeah, that's sorta how things would need to work...

Which still means we need an nsIHelperApp which can be usefully created
somehow... from outside the helper app service. (Eg the filepicker on the helper
app dialog needs to be able to do it.)

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 17 • 22 years ago

Well, you could do it like this:
give it a contract id and Init(nsIFile) function and use that from filepicker.
have the HelperAppService use a normal C++ constructor or something like that to
initialize it.

_or_, give it two Init functions, one taking a file, another one taking a mimetype.
The former would just use that file as helper app, the latter would look it up
from the helperappservice.

I would prefer the first approach, I guess.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 18 • 22 years ago

Yeah... how does this differ from what I suggest in comment 0?  ;)

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 19 • 22 years ago

bz: hey, you didn't mention how the interface would look like or be used :)

taking bug

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 20 • 22 years ago

Another approach (the current Windows one) is to allow the "system" application
to be null and only set the app description.  Then when LaunchWithApplication is
called with a null app we do whatever it is we need to do (in the case of
Windows, ShellExecute the file being viewed).  This _could_ be extended to other
OSes if we specify in nsIMIMEInfo that that is what should happen...

That would, of course, mean that the only thing that could launch the app would
be the external helper app service.  Not necessarily a good idea, imo....

So yeah, an nsIHelperApp that can be inited with an nsIFile, that encapsulates
its description, etc would be great.

We could have an XP superclass doing all the nsIFile/description stuff and
having a Launch(in nsIFile) impl, and then subclass it as needed on platforms
that need special stuff to happen in Launch().  I'd say that the contractid
should give an instance of the base class, but maybe we want to ifdef the
component registration...

Anyway, I'd love to see what you come up with.  Thanks for doing this!

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 21 • 22 years ago

status update: linux (well, unix) part of the patch done
if anyone is interested, I can attach a work-in-progress.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 22 • 22 years ago

"yes"  ;)

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 23 • 22 years ago

ok, I hope I diffed all changed files
please let me know if this fails to compile

Note: nsMIMEInfoImpl got some changes which are probably not very visible in
this patch, sorry about that, I suggest a manual "diff -u" between the currnet
MIMEInfoImpl and the one in this patch.

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 24 • 22 years ago

oh, and what this patch does _not_ do:
actually subclass the mimeinfo implementation for good mailcap support
that's partly because I don't really know how the mailcap stuff works :)

But this patch does make it possible to subclass mimeinfoimpl to easily add such
support. that's bug 83305 I guess.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 25 • 22 years ago

OK, based on a quick skim I have one question... you only use launchWithFile to
mean "launch using the default handler"?   Is that the plan?  Is the code that
launches with the "preferred" handler sufficiently similar across the OSes that
it could be moved into the mimeinfo base class impl?

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 26 • 22 years ago

bz: Indeed, that was my plan. That's not set in stone, of course.
I'll check how similar the other code is. If it is very similar, we can probably
get rid of LaunchAppWithTempFile (and replace it with
mimeInfo->LaunchWithFile(...)), which I would like, I guess.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 27 • 22 years ago

Yeah... I really don't like the nsIHelperAppLauncher interface, to be truthful;
anything we can do to clean it up would be great.

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 28 • 22 years ago

errr that has its own _interface_?
oh indeed...

Anyway, I had a short look at the implementations:
o Unix just creates an nsIProcess and runs it
o Windows and BeOS does the same, except if the file should be opened with the
default app, in which case nsILocalFile::Launch is called, unless the file is
executable (imho, that executable check does not belong here, but ok...)
o Mac uses nsILocalFileMac to launch the file
o OS/2 does the same as Unix, with the addition of a few lines of OS/2 specific
black magic.

Now, that stuff is mostly the same no matter if the default app or preferred app
should be used. So just using LaunchWithFile for both default app and preferred
app sounds like a good solution to me. It will require some ifdefs, at least
until this is actually subclassed, and maybe even then.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 29 • 22 years ago

> imho, that executable check does not belong here

It's a last-ditch check.  There are checks before this point, so this check
should never be needed, but it's there "just in case".  Paranoia is good in the
security business.

I was thinking we could have the superclass handle launching of "preferred"
handlers (since that sounds like it's pretty uniform across platforms, except
mac) and have the subclasses handle the "default" handler.... But yes, at first
we may have to ifdef it (though I would prefer we did this right to start with).

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 30 • 22 years ago

oh, I see, so every platform would have its own MIMEInfoImpl?
Hm, that probably sounds like a good idea. That idea hadn't occured to me, I was
only thinking of one impl. for nsIFile-based launching, and another one for
string-based launching (like for mailcap)

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 31 • 22 years ago

> oh, I see, so every platform would have its own MIMEInfoImpl?

Yep.  Exactly.  The base class could do some default stuff and then platforms
could override as needed.

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 32 • 22 years ago

I uploaded a plan for implementing this at:
http://stud4.tuwien.ac.at/~e0225227/plan78919.txt

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 34 • 22 years ago

Comment on attachment 127208 [details] [diff] [review]
part 1 (checked in)

r=me on all but the ldap changes... ;)

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 35 • 22 years ago

Comment on attachment 127208 [details] [diff] [review]
part 1 (checked in)

er... don't know how the ldap changes got in there... please ignore them, I
won't check them in

	Darin Fisher
	Comment 36 • 22 years ago

Comment on attachment 127208 [details] [diff] [review]
part 1 (checked in)

sr=darin

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 37 • 22 years ago

Comment on attachment 127208 [details] [diff] [review]
part 1 (checked in)

part 1 checked in.

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 39 • 21 years ago

Comment on attachment 130028 [details] [diff] [review]
Part 2

bz, feel free to take your time with this review. I don't want to land this
before 1.6alpha anyway.

sigh, this includes the patch for bug 120045... tell me if you'd rather have a
patch without that other patch mixed in

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 40 • 21 years ago

No need to split those changes out......

I'll try to get to this, but only once I have a non-modem net connection (could 
be a week or two).

	Jerry Baker
	Comment 41 • 21 years ago

Re: attachment 130028 [details] [diff] [review]

Defining BIN extensions as a binary executable on the Windows platform might not
be a good idea. A lot of Windows CD imaging programs create CD images as BIN
files. I don't think there is a such thing as an executable BIN file anyway (on
Windows).

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 42 • 21 years ago

Jerry, that has nothing to do with this bug; that's the part of the patch that
ended up in there by mistake as pointed out in comment 39 (and in fact does not
change the existing behavior at all; if you feel that the existing behavior
needs changing, feel free to file a separate bug on that).

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 43 • 21 years ago

biesi, could you attach a patch against current trunk when you get a chance? 
I'll try to review this tomorrow (that's about 18 hours from now)...

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 44 • 21 years ago

updated to trunk

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 45 • 21 years ago

Comment on attachment 132608 [details] [diff] [review]
Part 2, v2

>Index: nsExternalHelperAppService.cpp

>+  aMIMEInfo->SetApplicationDescription(nsnull);
>+  aMIMEInfo->SetPreferredApplicationHandler(nsnull);

Add a brief comment as to why the two lines are there?	I'm not sure why they
are needed...

>   if (NS_SUCCEEDED(rv) && exists)
...
>+  else if (NS_SUCCEEDED(rv) && !exists) {

"else if (NS_SUCCEEDED(rv))" is enough (since if "exists" is also true, you
would never make it to the else here).

>+already_AddRefed<nsIMIMEInfo> nsExternalHelperAppService::GetMIMEInfoFromOS(const char * aContentType, const char * aFileExt, PRBool& aFound)

Please use a pointer for an out param... (that makes it clear that it is in
fact an out param at the caller end of things).

> NS_IMETHODIMP nsExternalHelperAppService::GetFromTypeAndExtension(const char *aMIMEType, const char *aFileExt, nsIMIMEInfo **_retval) 

>+    rv = GetMIMEInfoForMimeTypeFromDS(aMIMEType, *_retval);

This is only safe because the DS will not override the info we want from the
OS.  comment to that effect.

Also, the DS _will_ override the description with an empty string.... In
FillTopLevelProperties, we want to only set the description if it's nonempty.

>+  if (NS_FAILED(rv)) {
>     // No type match, try extension match
>     nsCOMPtr<nsIMIMEInfo> dsInfoExt;

dsInfoExt is no longer used, right?

>+  if (!found) {

found will be false if no OS-level info for this type.	But there may be info
from the DS, no?  You probably want (!found && NS_FAILED(rv)) or something to
that effect...

Then you can probably just pass in _retval as the MIME info, and not have to do
that GetMIMEInfoFromOS thing in the *Extras functions....

>+  if (LOG_ENABLED()) {

That should be in #ifdef PR_LOGGING

> nsresult nsExternalHelperAppService::GetMIMEInfoForExtensionFromExtras(const char* aExtension, nsIMIMEInfo ** aMIMEInfo )
>+              nsCOMPtr<nsIMIMEInfo> mimeInfo =
>+                         GetMIMEInfoFromOS(extraMimeEntries[index].mMimeType,
>+                                           nsnull, ignored);

How come this is done like this, while the similar Type function just assigns
to the out param?

>Index: nsExternalHelperAppService.h

>+   * @param aFound

"@param aFound [out]" or some such (not sure what the right javadoc syntax is)

>+   * @return A MIMEInfo. This function is supposed to always return a MIMEInfo;
>+   *         should only be returned in failures like out-of-memory.

"null should only be returned"?  Maybe something more like:

"This function must return a MIMEInfo object if it can allocate one.  The only
justifiable reason for not returning one is an out-of-memory error."

>Index: os2/nsOSHelperAppService.cpp
> nsOSHelperAppService::GetMIMEInfoFromOS(const char *aType,

>+    // If we found nothing, and had no type, just return
>+    if (!retval)
>+      return nsnull;

The only way retval can be null there is if we ran out of memory (or were
otherwise unable to instantiate the contractid).  Should this be testing aType
or something?  Or should this test just not exist?

>Index: unix/nsOSHelperAppService.cpp

Same issue.

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 46 • 21 years ago

>The only way retval can be null there is if we ran out of memory (or were
>otherwise unable to instantiate the contractid).  Should this be testing aType
>or something?  Or should this test just not exist?

hm, that's not true actually. this seems unreachable. (the |return retval| above
is not inside an if (retval))

>You probably want (!found && NS_FAILED(rv)) or something to
>that effect...

I did it differently and set found to true if getting something from the ds worked

attaching a new patch in a minute, once I get it to compile...

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 48 • 21 years ago

Comment on attachment 134162 [details] [diff] [review]
part 2, v3

>Index: nsExternalHelperAppService.cpp

> NS_IMETHODIMP nsExternalHelperAppService::GetFromTypeAndExtension(const char *aMIMEType, const char *aFileExt, nsIMIMEInfo **_retval) 

>+  PRBool found;
>+  *_retval = GetMIMEInfoFromOS(aMIMEType, aFileExt, &found).get();

>+  if (aMIMEType && *aMIMEType) {
>+    rv = GetMIMEInfoForMimeTypeFromDS(aMIMEType, *_retval);
>+    found = NS_SUCCEEDED(rv);
>   }
>+  if (!found) {
>+      rv = GetMIMEInfoForExtensionFromDS(aFileExt, *_retval);
>+      found = NS_SUCCEEDED(rv);
>     }
>   }
> 
>   // (3) No match yet. Ask extras.
>+  if (!found) {
>+    rv = NS_ERROR_FAILURE;

So if we get an OS-level match but don't find the type/extension in our DS we
will hit the extras?  That seems wrong...   I think we should only hit the
extras if there is nothing from the OS or from the DS at all for that
type/extension.

>Index: os2/nsOSHelperAppService.cpp
> already_AddRefed<nsIMIMEInfo>
> nsOSHelperAppService::GetMIMEInfoFromOS(const char *aType,
>+    // If we found nothing, and had no type, just return
>+    if (!retval)
>+      return nsnull;

Like you said, this return is unreachable.  Please remove it?

>Index: unix/nsOSHelperAppService.cpp
> nsOSHelperAppService::GetMIMEInfoFromOS(const char *aType,

>+    // If we found nothing, and had no type, just return
>+    if (!retval)
>+      return nsnull;

Same.

r=bzbarsky with those changes.	I'm sorry it took me so long to get to this...
:(

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 49 • 21 years ago

ok, I changed the code to:
  if (aMIMEType && *aMIMEType) {
    // This will not overwrite the OS information that interests us
    // (i.e. default application, default app. description)
    rv = GetMIMEInfoForMimeTypeFromDS(aMIMEType, *_retval);
    found = found || NS_SUCCEEDED(rv);
  }
so if Get...FromDS fails, but found was true, found will stay true.
will attach a new patch in a second

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 51 • 21 years ago

So if we get OS-level info, but don't get anything out of the DS based on the
type we will skip looking in the DS based on the extension?  That's wrong too....

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 52 • 21 years ago

Comment on attachment 135066 [details] [diff] [review]
part 2, v4

you're correct...

	Darin Fisher
	Comment 54 • 21 years ago

Comment on attachment 135073 [details] [diff] [review]
part 2, v5 (checked in)

+  NS_ENSURE_ARG_POINTER( aExtension );
+  NS_ENSURE_ARG( *aExtension );

how about using NS_ASSERTION instead?  we shouldn't have to
null check input args like this.

+    if (!miByExt && !mi) {
+      *aFound = PR_FALSE;
+      CallCreateInstance(NS_MIMEINFO_CONTRACTID, &mi);
+      if (mi) {
+	 if (aMIMEType && *aMIMEType)
+	   mi->SetMIMEType(aMIMEType);
+	 if (aFileExt && *aFileExt)
+	   mi->AppendExtension(aFileExt);
+      }

this blob of code seems to be added to all the platform
specific files.  should it be factored into the cross-
platform code?


sr=darin

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 55 • 21 years ago

Comment on attachment 135073 [details] [diff] [review]
part 2, v5 (checked in)

part 2 v5 checked in

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 56 • 21 years ago

move nsMIMEInfoImpl from netwerk/mime/src to uriloader/exthandler; create it
using |new nsMIMEInfoImpl| instead of using the component manager.

this does not allow creating mimeinfos via CreateInstance anymore. (nothing in
cvs.mozilla.org depends on that, except exthandler and that's patched here).

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 57 • 21 years ago

oh yeah, I will of course get nsMIMEInfoImpl.* to be repository-copied.

I know that for bug 190413, people need a way to create mimeinfos to pass it to
the nsIXMLMIMEService; but they can just ask the mimeservice for one.

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 58 • 21 years ago

well, this is the missing netwerk/mime/src/Makefile.in change.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 59 • 21 years ago

Comment on attachment 139620 [details] [diff] [review]
part3 v1 addition (checked in)

r=bzbarsky

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 60 • 21 years ago

Comment on attachment 139613 [details] [diff] [review]
part 3 v1 (checked in)

>Index: uriloader/exthandler/beos/nsOSHelperAppService.cpp
>+	nsCOMPtr<nsIMIMEInfo> mimeInfo = new nsMIMEInfoImpl;

new nsMIMEInfoImpl(), please (just makes it clearer that a constructor call
happens there.... ;))

Same in the other places where you have |new nsMIMEInfoImpl|.

r=bzbarsky with that.

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 61 • 21 years ago

Comment on attachment 139613 [details] [diff] [review]
part 3 v1 (checked in)

ok, I will make that change

	Darin Fisher
	Comment 62 • 21 years ago

Comment on attachment 139613 [details] [diff] [review]
part 3 v1 (checked in)

sr=darin

	Darin Fisher
	Comment 63 • 21 years ago

Comment on attachment 139620 [details] [diff] [review]
part3 v1 addition (checked in)

sr=darin

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 64 • 21 years ago

OK, so the following files need to be moved in cvs:
netwerk/mime/src/nsMIMEInfoImpl.cpp
netwerk/mime/src/nsMIMEInfoImpl.h

To:
uriloader/exthandler, unchanged name

darin: could you give sr for that move?

	Darin Fisher
	Comment 65 • 21 years ago

sr=darin on the move

	Dave Miller [:justdave]
	Comment 66 • 21 years ago

Done.

dave@emac [21:58 exthandler 29] tcsh> cvs -q stat -v nsMIMEInfoImpl.cpp
===================================================================
File: nsMIMEInfoImpl.cpp        Status: Up-to-date

   Working revision:    1.46
   Repository revision: 1.46   
/cvsroot/mozilla/uriloader/exthandler/nsMIMEInfoImpl.cpp,v
   Sticky Tag:          (none)
   Sticky Date:         (none)
   Sticky Options:      (none)

   Existing Tags:
        No Tags Exist

dave@emac [21:58 exthandler 30] tcsh> cvs -q stat -v nsMIMEInfoImpl.h
===================================================================
File: nsMIMEInfoImpl.h  Status: Up-to-date

   Working revision:    1.17
   Repository revision: 1.17   
/cvsroot/mozilla/uriloader/exthandler/nsMIMEInfoImpl.h,v
   Sticky Tag:          (none)
   Sticky Date:         (none)
   Sticky Options:      (none)

   Existing Tags:
        No Tags Exist

You may cvs remove the old versions whenever you're ready.

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 67 • 21 years ago

Comment on attachment 139613 [details] [diff] [review]
part 3 v1 (checked in)

part 3 checked in, including removal of old nsMIMEInfoImpl.{cpp,h}

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 68 • 21 years ago

this basically gives each os its own nsMIMEInfo implementation. except unix,
which uses nsMIMEInfoImpl.

Most functions of nsMIMEInfoImpl moved to nsMIMEInfoBase, which doesn't know
about default applications. I didn't want to make the windows impl inherit
functions (and variables) it had no use for, especially as that may mislead
people into thinking they are used.
This means that nsMIMEInfoBase has two pure virtual functions -
LaunchDefaultWithFile and GetHasDefaultHandler (the latter from nsIMIMEInfo).

The new function launchWithFile calls LaunchDefaultWithFile when
mPreferredAction is set to useSystemDefault (by default. can be overridden,
OS/2 does that, mac too iirc).

patch was tested on all affected platforms. testcases: compile, open a file
with the default application, open it with a specified application.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 69 • 21 years ago

Comment on attachment 140758 [details] [diff] [review]
part 4

> ? uriloader/exthandler/os2/nsMIMEInfoOS2.cpp
> ? uriloader/exthandler/os2/nsMIMEInfoOS2.h

CVS add these so I can take a look?  ;)

>Index: netwerk/mime/public/nsIMIMEInfo.idl

>+     * a pretty name description of the default application

Mention that this is only usable if hasDefaultHandler is set?

>+     * Launches the application with the specified file, depending on the
>+     * preferredAction attribute.

"in a way that depends on the value of preferredAction"

> action must be useHelperApp or
>+     * useSystemDefault.

"preferredAction must be..."

Also, add a @throws line to say what we throw if it's not?

>Index: uriloader/exthandler/nsExternalHelperAppService.h

>-protected:

How come?

>Index: uriloader/exthandler/nsMIMEInfoImpl.cpp
>-nsMIMEInfoImpl::nsMIMEInfoImpl() {
>+nsMIMEInfoBase::nsMIMEInfoBase() {
>     mPreferredAction = nsIMIMEInfo::saveToDisk;
>     mAlwaysAskBeforeHandling = PR_TRUE;

Make those initializers while you're here?

>+nsMIMEInfoBase::nsMIMEInfoBase(const char *aMIMEType) :mMIMEType( aMIMEType 
){
>     mPreferredAction = nsIMIMEInfo::saveToDisk;
>     mAlwaysAskBeforeHandling = PR_TRUE;

Same.

>Index: uriloader/exthandler/nsMIMEInfoImpl.h

>+ * be set using the public SetDefaultApplication (not exposed on any interface).
>+ * If this is not true for a platform, they should create their own subclass of
>+ * this interface

I'd say they should subclass nsMIMEInfoBase, not "this interface"....  They can
still override LaunchWithFile if they had to.

> with an appropriate implementation of launchWithFile, or use
>+ * nsMIMEInfoBase.

They can't "use" it -- it has virtual methods.	Also, we're talking about
LaunchDefaultWithFile, not LaunchWithFile, are we not?

>+ * Alternatively, people can accept the base class's implementation of launching
>+ * the preferred application (which is to use LaunchWithIProcess) and just
>+ * override LaunchDefaultWithFile, which will be called by launchWithFile if the
>+ * action is set to useSystemDefault.

Oh, I see.  This comment is pretty confusing.  I would just tell people to
subclass nsMIMEInfoImpl if their default app is an nsIFile, and otherwise to
subclass nsMIMEInfoBase and implement LaunchDefaultWithFile... anyone who
really needs more will figure out for themselves that they can override virtual
functions.  ;)

>+ *
>+ * Another assumption is that hasDefaultApp is true iff mDefaultApplication is
>+ * non-null and existant.

hasDefaultHandler, no?

You never check its existence... Should you?

>Index: uriloader/exthandler/mac/nsOSHelperAppService.cpp

>-#ifdef XP_MACOSX
>-  else
>-  { // We didn't get an application to handle the file from aMIMEInfo, ask LaunchServices directly

We probably want to keep that code (and make it actually reachable, in the case
when neither preferred nor default app handlers are set....  Don't we have a
bug on this code not being reachable already?

In any case, this should move into nsMIMEInfoMac::LaunchWithFile (possibly in a
followup patch if you prefer).

>@@ -266,34 +215,60 @@ nsOSHelperAppService::GetMIMEInfoFromOS(
>+      // Copy the attributes of miByType onto miByExt

Wouldn't it be simpler to just copy the default app in the other direction?  We
know what class miByType and miByExt are, so we can do said copy easily, no? 
May need to add GetDefaultApplication to nsMIMEInfoImpl (but there's no reason
not to, right?)

I realize those come from the IC service, but maybe we can cheat and cast them?
 The current code is really a little excessive...

On an unrelated note, we should add some calls to
SetFileTypeAndCreatorFromMIMEType in the appropriate spots in the helper app
code (eg when saving), imo.

>Index: uriloader/exthandler/os2/nsOSHelperAppService.cpp
>@@ -1514,23 +1404,22 @@ nsOSHelperAppService::GetFromExtension(c
>+  nsMIMEInfoOS2* mimeInfo = new nsMIMEInfoOS2(NS_ConvertUCS2toUTF8(mimeType.get()).get());

Why mimeType.get() instead of just mimeType?

>@@ -1698,32 +1585,51 @@ nsOSHelperAppService::GetMIMEInfoFromOS(
>+    // Copy the attributes of retval onto miByExt, to return it

Again, a little silly, esp since this class owns both the method that produced
miByExt and the method that produced retval.  Just make that method return
alredy_AddRefed<nsMIMEInfoOS2> (like you did with Windows), then just copy the
default handler and description in this code.

>Index: uriloader/exthandler/unix/nsOSHelperAppService.cpp

In the usual way, same comments as for OS/2.  This time only on 

>@@ -1644,20 +1594,39 @@ nsOSHelperAppService::GetMIMEInfoFromOS(
>+    // Copy the attributes of retval onto miByExt, to return it

>Index: uriloader/exthandler/win/nsMIMEInfoWin.h

>+    // The default impl of this is wrong for us, as we don't use
>+    // mDefaultApplication
>+    NS_IMETHOD GetHasDefaultHandler(PRBool * _retval);

Ditch the comment; it's not true anyway, since there is no default impl (in the
baseclass, which is nsMIMEInfoBase).

>Index: uriloader/exthandler/win/nsOSHelperAppService.h

>+class nsMIMEInfoWin;

>+  already_AddRefed<nsMIMEInfoBase> GetByExtension(const char *aFileExt, const char *aTypeHint = nsnull);

Shouldn't the forward decl be for the type we're returning here?  I'm surprised
this compiled....

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 71 • 21 years ago

Comment on attachment 140817 [details] [diff] [review]
part 4: OS/2 mime info impls

r=bzbarsky.   One more thing.  For classes that override LaunchWithFile,
perhaps they should also (debug-only) override LaunchDefaultWithFile and put
NS_NOTREACHED in it? Just to make sure that none of the callers call the wrong
method?

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 72 • 21 years ago

(I'll make the changes that I don't comment on)

> (From update of attachment 140758 [details] [diff] [review])
> > ? uriloader/exthandler/os2/nsMIMEInfoOS2.cpp
> > ? uriloader/exthandler/os2/nsMIMEInfoOS2.h
> 
> CVS add these so I can take a look?  ;)

"oops" :) attached.

> >Index: uriloader/exthandler/nsExternalHelperAppService.h
> 
> >-protected:
> 
> How come?

There is already a protected: line, further up in the file. I saw no point in
repeating it.

> Oh, I see.  This comment is pretty confusing.

Yeah, I'll just rewrite it I guess. Originally, there was no nsMIMEInfoBase, and
I left the comment pretty untouched.

> >+ * Another assumption is that hasDefaultApp is true iff mDefaultApplication is
> >+ * non-null and existant.
> 
> hasDefaultHandler, no?

details ;)

> You never check its existence... Should you?

nsIProcess/ShellExecute will fail if it doesn't exist... but true,
GetHasDefaultHandler should check. Will make that change.

> We probably want to keep that code (and make it actually reachable, in the case
> when neither preferred nor default app handlers are set....  Don't we have a
> bug on this code not being reachable already?

I'd like to keep making this code reachable in bug 183848.

> >@@ -266,34 +215,60 @@ nsOSHelperAppService::GetMIMEInfoFromOS(
> >+      // Copy the attributes of miByType onto miByExt
> 
> Wouldn't it be simpler to just copy the default app in the other direction?  We
> know what class miByType and miByExt are, so we can do said copy easily, no? 
> May need to add GetDefaultApplication to nsMIMEInfoImpl (but there's no reason
> not to, right?)

This is probably doable on OS/2. But MacOS asks Internet Config... oh, you know
that:

> I realize those come from the IC service, but maybe we can cheat and cast them?
>  The current code is really a little excessive...

If we had dynamic_cast available, I'd maybe be willing to. As it is, I fear too
much that someone would make internet config return another impl of nsIMIMEInfo,
and that would be a _bad_ thing if I static_cast this.

IC has its own IDL file, so it's hard to change what it returns...

scratch that. it turns out I have dynamic_cast on the mac.

> On an unrelated note, we should add some calls to
> SetFileTypeAndCreatorFromMIMEType in the appropriate spots in the helper app
> code (eg when saving), imo.

eh, we don't do that already? will file a bug on that.

> >@@ -1698,32 +1585,51 @@ nsOSHelperAppService::GetMIMEInfoFromOS(
> >+    // Copy the attributes of retval onto miByExt, to return it
> 
> Again, a little silly, esp since this class owns both the method that produced
> miByExt and the method that produced retval.

actually that's not true. parts come from nsGNOMERegistry.

And once real support for mailcap entries is implemented, this code will fall
apart - mime infos from gnome registry will probably still be nsMIMEInfoImpl,
those from mailcap would be a new subclass of nsMIMEInfoBase.

OS/2 could probably just be changed to copy the mailcap state then, as it has no
gnomeregistry equivalent...

>  Just make that method return
> alredy_AddRefed<nsMIMEInfoOS2> (like you did with Windows), then just copy the
> default handler and description in this code.

oh, all of that was about os/2? ah well. I'll make this change for os2.

> >Index: uriloader/exthandler/unix/nsOSHelperAppService.cpp
> 
> In the usual way, same comments as for OS/2. 

see above.

> >Index: uriloader/exthandler/win/nsMIMEInfoWin.h
> 
> >+    // The default impl of this is wrong for us, as we don't use
> >+    // mDefaultApplication
> >+    NS_IMETHOD GetHasDefaultHandler(PRBool * _retval);
> 
> Ditch the comment; it's not true anyway, since there is no default impl (in the
> baseclass, which is nsMIMEInfoBase).

right... forgot to remove this comment.

> >Index: uriloader/exthandler/win/nsOSHelperAppService.h
> 
> >+class nsMIMEInfoWin;
> 
> >+  already_AddRefed<nsMIMEInfoBase> GetByExtension(const char *aFileExt,
const char *aTypeHint = nsnull);
> 
> Shouldn't the forward decl be for the type we're returning here?  I'm surprised
> this compiled....

this has a full decl already, by way of nsExternalHelperAppService.h.

I don't use nsMIMEInfoWin because I assign this into a nsRefPtr<nsMIMEInfoBase>.
hm, why didn't I make that a nsRefPtr<nsMIMEInfoWin>? Will change this to use
nsMIMEInfoWin.

(In reply to comment #71)
> (From update of attachment 140817 [details] [diff] [review])
> r=bzbarsky.   One more thing.  For classes that override LaunchWithFile,
> perhaps they should also (debug-only) override LaunchDefaultWithFile and put
> NS_NOTREACHED in it? Just to make sure that none of the callers call the wrong
> method?

sounds like a good idea, will do that.

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 73 • 21 years ago

note to self: add a function "public: void CopyBasicData(nsMIMEInfoBase*
target)" to nsMIMEInfoBase, for use on os/2,mac,unix

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 74 • 21 years ago

(I filed bug 233375 about setting mac type and creator on downloaded files)

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 76 • 21 years ago

Comment on attachment 140827 [details] [diff] [review]
part 4 v2

changes made

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 77 • 21 years ago

Comment on attachment 140827 [details] [diff] [review]
part 4 v2

>Index: uriloader/exthandler/nsMIMEInfoImpl.cpp

>+nsMIMEInfoImpl::GetHasDefaultHandler(PRBool * _retval)
>+{
...
>+    if (NS_SUCCEEDED(mDefaultApplication->Exists(&exists)) && exists)
>+      *_retval = PR_TRUE;

How about 

  *_retval = NS_SUCCEEDED(mDefaultApplication->Exists(&exists)) && exists;

?

>Index: uriloader/exthandler/nsMIMEInfoImpl.h

>+    void CopyBasicDataTo(nsMIMEInfoBase* aOther);

The impl didn't copy the mac stuff.

>Index: uriloader/exthandler/beos/nsMIMEInfoBeOS.h
>+class nsMIMEInfoBeOS : public nsMIMEInfoImpl {

Could make this inherit from nsMIMEInfoBase, actually...  Separate patch, if
you want.

>Index: uriloader/exthandler/mac/nsOSHelperAppService.cpp

>+#include "nsIStringEnumerator.h"

Don't need that, do you?

>Index: uriloader/exthandler/os2/nsOSHelperAppService.cpp

>+#include "nsIStringEnumerator.h"

Same.

>Index: uriloader/exthandler/unix/nsOSHelperAppService.cpp

>+#include "nsIStringEnumerator.h"

And here.

r=bzbarsky with those nits picked.

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 78 • 21 years ago

all done, except the beos change for which I will file a new bug

	Darin Fisher
	Comment 79 • 21 years ago

Comment on attachment 140830 [details] [diff] [review]
part 4 v3

>+    nsAutoString           mDescription; // human readable description

nsAutoString is not meant to be used as a member variable.  why are you doing
that?

>+    static NS_HIDDEN_(nsresult)

talk to bryner... i don't know if we're ready to start using NS_HIDDEN...


>+    NS_IMETHODIMP GetDefaultDescription(PRUnichar ** aDefaultDescription);

why is this declared NS_IMETHODIMP instead of NS_IMETHOD?  seems like you might
have a reason... care to share that?


> virtual NS_HIDDEN_(nsresult)

isn't this what NS_IMETHOD corresponds to?


> + * The Initial Developer of the Original Code is 
+ * Paul Ashford. 
+ * Portions created by the Initial Developer are Copyright (C) 2002
+ * the Initial Developer. All Rights Reserved. 

bad license file?  or is this really his code?


hmm.. BeOS header file has your name as the initial developer.


> virtual nsresult LaunchDefaultWithFile

why not marked HIDDEN here?


more to come....

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 80 • 21 years ago

(In reply to comment #79)
> (From update of attachment 140830 [details] [diff] [review])
> >+    nsAutoString           mDescription; // human readable description
> 
> nsAutoString is not meant to be used as a member variable.  why are you doing
> that?

Indeed, why am I... Was a mistake I guess, I'll switch to nsString.

> >+    static NS_HIDDEN_(nsresult)
> 
> talk to bryner... i don't know if we're ready to start using NS_HIDDEN...

bryner tells me it's ok to use it.

> >+    NS_IMETHODIMP GetDefaultDescription(PRUnichar ** aDefaultDescription);
> 
> why is this declared NS_IMETHODIMP instead of NS_IMETHOD?  seems like you might
> have a reason... care to share that?

the reason is that I copied the line from the .cpp file and forgot to change this :(
will change this to NS_IMETHOD

> > virtual NS_HIDDEN_(nsresult)
> 
> isn't this what NS_IMETHOD corresponds to?

NS_IMETHOD also uses stdcall (on windows)... here, I don't really care about the
calling convention.

> > + * The Initial Developer of the Original Code is 
> + * Paul Ashford. 
> + * Portions created by the Initial Developer are Copyright (C) 2002
> + * the Initial Developer. All Rights Reserved. 
> 
> bad license file?  or is this really his code?

it is a copy&paste from nsOSHelperAPpService, so this is really his code.

> hmm.. BeOS header file has your name as the initial developer.

yes - this file is newly created by me.

> > virtual nsresult LaunchDefaultWithFile
> 
> why not marked HIDDEN here?

no good reason :(
(god, was I asleep when I wrote this patch?)

	Darin Fisher
	Comment 81 • 21 years ago

Comment on attachment 140830 [details] [diff] [review]
part 4 v3

nit: indentation is funky in nsMIMEInfoMac.cpp


> +      nsMIMEInfoBase* byType = dynamic_cast<nsMIMEInfoBase*>(miByType.get());

hmm... we don't compile with RTTI support, so is this really of any value?



there's a lot of string converions happening in this code block (maybe convert
major/minorType just once?):
	   NS_LossyConvertUCS2toASCII(minorType).get(),
	   NS_LossyConvertUCS2toASCII(mime_types_description).get()));

   if (majorType.IsEmpty() && minorType.IsEmpty()) {
     // we didn't get a type mapping, so we can't do anything useful
     return nsnull;
   }

-  nsIMIMEInfo* mimeInfo = new nsMIMEInfoImpl();
+  mimeType = majorType + NS_LITERAL_STRING("/") + minorType;
+  nsMIMEInfoOS2* mimeInfo = new
nsMIMEInfoOS2(NS_ConvertUCS2toUTF8(mimeType).get());



sr=darin

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 82 • 21 years ago

>hmm... we don't compile with RTTI support, so is this really of any value?

mac code does that in other places too, random example:
http://lxr.mozilla.org/seamonkey/source/gfx/src/mac/nsPrintSettingsMac.cpp#230
(lxr for dynamic_cast finds a lot more places)

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 83 • 21 years ago

(In reply to comment #81)
> there's a lot of string converions happening in this code block (maybe convert
> major/minorType just once?):

I added a local variable NS_LossyConvertUTF16toASCII asciiMajorType and
asciiMinorType to only convert those once.

Checked in.

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 84 • 21 years ago

filed Bug 235350 for the beos suggestion from comment 77.