Closed Bug 282196 Opened 19 years ago Closed 19 years ago

Need to access arbitrary metadata on default application handler

Categories

(Core Graveyard :: File Handling, defect, P1)

x86
Windows XP
defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.8beta1

People

(Reporter: bugs, Assigned: bugs)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

nsIMIMEInfo exposes a nsILocalFile property for the user's custom selected
application handler, and using things like moz-icon:// and other interfaces
(e.g. the new nsILocalFileWin) I can get the icon for the custom app, or other
information in that executable's VERSIONINFO block. 

I would like to be able to get the same info from the _default_ application
handler. This means I need a way to get at the file associated with the default
app from nsIMIMEInfo. Boris has pointed out that the default app handler does
not necessarily give you all the information you need to launch the file - e.g.
it does not take into account any command line flags required, etc. 

For my purposes, and other metadata harvesting that might be done by other apps,
knowledge of the command line params is not necessary; however it might be for
others. The purpose of this bug is to come up with and implement the ideal set
of additions to nsIMIMEInfo to allow for my use case and others, and also to
craft something reasonable enough so that after the interface is frozen we won't
be kicking ourselves about some oversight.
Status: NEW → ASSIGNED
Flags: blocking-aviary1.1+
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta1
So here is the basic problem.  The default app handler is just something handled
by the OS, on a per-platform basis.  There may be no associated executable file
at all, in some cases (for example, consider the default handler for .exe files
on Windows).  On a lot of our platforms, when we need to use the default handler
we just pass the temp filename to the platform and say "hey, do something with
this" (using, say, nsILocalFile.launch).

So given that, what's a reasonable way to expose properties of the default
handler, in those cases when we may happen to know them?  I'm really not so
happy about using an nsIPropertyBag or something, but maybe that's the way to go?
Attached patch a stab at something like this (obsolete) — Splinter Review
make the mime info objs impl nsIPropertyBag. This must include 252189 since I
use the code from 252189 to initialize members used by the property bag impl.
("SetDefaultApplicationHandler")

I can then have code in js that looks like this:

var fallbackURL = "moz-icon://some.default";
var iconURL = "";
if (mi instanceof Components.interfaces.nsIPropertyBag)
  iconURL = mi.getProperty("defaultApplicationIconURL");
filefield.image = iconURL || fallbackURL;

which will show the appropriate app icon on windows:

[ W Microsoft Word	    ]

or something generic on Linux:

[ @ foo -bar		    ]
Attachment #174505 - Flags: superreview?(bzbarsky)
Attachment #174505 - Flags: review?(cbiesinger)
Comment on attachment 174505 [details] [diff] [review]
a stab at something like this

>+nsMIMEInfoImpl::GetProperty(const nsAString& aName, nsIVariant* *_retval)
>+{
>+  *_retval = nsnull;
>+  return NS_OK;

That's not what the nsIPropertyBag API says should happen for properties that
don't exist...

>Index: uriloader/exthandler/nsMIMEInfoImpl.h
>+    // nsIPropertyBag methods
>+    NS_IMETHOD GetEnumerator(nsISimpleEnumerator* *_retval);
>+    NS_IMETHOD GetProperty(const nsAString& aName, nsIVariant* *_retval);

Why not NS_DECL_NSIPROPERTYBAG?

>Index: uriloader/exthandler/win/nsMIMEInfoWin.cpp
> +nsMIMEInfoWin::GetProperty(const nsAString& aName, nsIVariant* *_retval)

This doesn't follow the nsIPropertyBag api when the property is not available.

Also, NS_GetURLSpecFromFile crashes if the object passed to it is null.  That
could be an issue here, since we don't always have a default or preferred
handler.

>Index: uriloader/exthandler/win/nsMIMEInfoWin.h
>+    NS_IMETHOD GetEnumerator(nsISimpleEnumerator* *_retval);
>+    NS_IMETHOD GetProperty(const nsAString& aName, nsIVariant* *_retval);

Same here.

I think you want the property name defines in nsMIMEInfoImpl.h

With those fixed looks ok to me if biesi's ok with it, but please file followup
bugs on the other mime info implementations to hand out icon URLs.  I'm pretty
sure they can do a reasonable job of it in most cases.
Attachment #174505 - Flags: superreview?(bzbarsky) → superreview-
Attached patch patch, revised (obsolete) — Splinter Review
Attachment #174505 - Attachment is obsolete: true
Attachment #174548 - Flags: superreview?(bzbarsky)
Attachment #174548 - Flags: review?(cbiesinger)
Filed bug 282545 for implementing this on OS X (and dependency 282546 for
implementing moz-icon support for app-bundle-icons on OS X). 
OS: All → Windows XP
Hardware: All → PC
Comment on attachment 174548 [details] [diff] [review]
patch, revised

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

>+nsMIMEInfoWin::GetProperty(const nsAString& aName, nsIVariant* 
>+      variant = do_CreateInstance("@mozilla.org/variant;1");
...
>+      variant->SetAsACString(iconURLSpec);

Crash on out of memory....  Probably want an early return here if variant
didn't get created.

>+    else if (aName.EqualsLiteral(PROPERTY_CUSTOM_APP_ICON_URL)) {
>+      variant = do_CreateInstance("@mozilla.org/variant;1");

Same here.  Should have caught these the first time....

You also didn't fix the crash if mPreferredApplication is null (and I'm not
sure why lack of an mDefaultApplication should keep us from returning the icon
for the preferred app).
Attachment #174548 - Flags: superreview?(bzbarsky) → superreview-
> Filed bug 282545 for implementing this on OS X 

What about gtk?  And OS/2?  Those both support -moz-icon channels, no?
Attached patch ... (obsolete) — Splinter Review
Attachment #174548 - Attachment is obsolete: true
Attachment #174552 - Flags: superreview?(bzbarsky)
Attachment #174552 - Flags: review?(cbiesinger)
Attachment #174505 - Flags: review?(cbiesinger)
Attachment #174548 - Flags: review?(cbiesinger)
see 282548, 282549 for GTK, OS/2 bugs, although I believe OS/2 is pretty much
dead at this point. 
Comment on attachment 174552 [details] [diff] [review]
...

sr=bzbarsky, assuming biesi's ok with the general setup.
Attachment #174552 - Flags: superreview?(bzbarsky) → superreview+
Blocks: 282548
Comment on attachment 174552 [details] [diff] [review]
...

uriloader/exthandler/nsMIMEInfoImpl.cpp

I don't think nsMIMEInfoBase should implement this interface, if it doesn't
provide an implementation of it.

So... this way of implementing it (with a QI to propertybag) doesn't scale well
if many properties exist... but I guess that's ok for now.

uriloader/exthandler/nsMIMEInfoImpl.h
+ * moz-icon URI for the default handler application's icon, if available.

Can you mention the type of the property here?

uriloader/exthandler/win/nsMIMEInfoWin.cpp

ok, I'm not such a fan of using strings for URIs, but I guess this is OK (since
we rarely need an origin charset here)...

But, NS_GetURLSpecFromFile returns an UTF-8 string. So please use
setAsUTF8String, unless you want to go for nsIURI and setAsInterface.

hm... the code the two properties looks pretty identical, maybe it should be
factored into a helper function?
Attachment #174552 - Flags: review?(cbiesinger) → review-
Attached patch How about this (obsolete) — Splinter Review
use a helper function, SetAsAUTFString...

I still return the icon URI as a string, since moz-icon: URIs are almost never
used as URIs themselves anywhere in the system, most often to initialize UI
elements, e.g. button.image = "moz-icon:...";
Attachment #174552 - Attachment is obsolete: true
Attachment #174957 - Flags: review?(cbiesinger)
Comment on attachment 174957 [details] [diff] [review]
How about this

So does nsMIMEInfoBase really need to implement this interface? why not
implement it only in nsMIMEInfoWin?
Attachment #174957 - Attachment is obsolete: true
Attachment #175393 - Flags: review?(cbiesinger)
Attachment #174957 - Flags: review?(cbiesinger)
Comment on attachment 175393 [details] [diff] [review]
impl on nsMIMEInfoWin only

uriloader/exthandler/nsMIMEInfoImpl.cpp
-  return NS_NewUTF8StringEnumerator(aResult, &mExtensions, this);
+  nsIMIMEInfo* mi = NS_STATIC_CAST(nsIMIMEInfo*, this);
+  return NS_NewUTF8StringEnumerator(aResult, &mExtensions, mi);

seems to me like this cast should no longer be necessary.

r=biesi with that change
Attachment #175393 - Flags: review?(cbiesinger) → review+
Comment on attachment 175393 [details] [diff] [review]
impl on nsMIMEInfoWin only

asking bz to do one last check...
Attachment #175393 - Flags: superreview?(bzbarsky)
Comment on attachment 175393 [details] [diff] [review]
impl on nsMIMEInfoWin only

>Index: uriloader/exthandler/nsMIMEInfoImpl.cpp
> nsMIMEInfoBase::GetFileExtensions(nsIUTF8StringEnumerator** aResult)
> {
>-  return NS_NewUTF8StringEnumerator(aResult, &mExtensions, this);
>+  nsIMIMEInfo* mi = NS_STATIC_CAST(nsIMIMEInfo*, this);
>+  return NS_NewUTF8StringEnumerator(aResult, &mExtensions, mi);

Nuke that change.

>Index: uriloader/exthandler/nsMIMEInfoImpl.h
>     NS_IMETHOD GetDefaultDescription(nsAString& aDefaultDescription);
>-
>+    

And this one.

>Index: uriloader/exthandler/win/nsMIMEInfoWin.cpp
>+static nsresult GetIconURLVariant(nsIFile* aApplication, nsIVariant* *_retval)

Toss in an NS_PRECONDITION(aApplication, "Must have file") here?

>Index: uriloader/exthandler/win/nsOSHelperAppService.cpp
>+nsOSHelperAppService::GetDefaultAppInfo(nsAString& aTypeName, 
>+    PRBool found = GetValueString(handlerKey, NULL, handlerCommand);
>+    if (found) {
...
>+      ::RegCloseKey(handlerKey);
>+    }

The key needs to be closed even if GetValueString returned false; move it out
one level.

sr=bzbarsky with those (especially the last change!)
Attachment #175393 - Flags: superreview?(bzbarsky) → superreview+
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.