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)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.8beta1
People
(Reporter: bugs, Assigned: bugs)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 4 obsolete files)
9.85 KB,
patch
|
Details | Diff | Splinter Review | |
19.01 KB,
patch
|
Biesinger
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•19 years ago
|
Status: NEW → ASSIGNED
Flags: blocking-aviary1.1+
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta1
Comment 1•19 years ago
|
||
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?
Assignee | ||
Comment 2•19 years ago
|
||
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 3•19 years ago
|
||
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-
Assignee | ||
Comment 4•19 years ago
|
||
Attachment #174505 -
Attachment is obsolete: true
Attachment #174548 -
Flags: superreview?(bzbarsky)
Attachment #174548 -
Flags: review?(cbiesinger)
Assignee | ||
Comment 5•19 years ago
|
||
Filed bug 282545 for implementing this on OS X (and dependency 282546 for implementing moz-icon support for app-bundle-icons on OS X).
Assignee | ||
Updated•19 years ago
|
OS: All → Windows XP
Hardware: All → PC
Comment 6•19 years ago
|
||
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-
Comment 7•19 years ago
|
||
> Filed bug 282545 for implementing this on OS X
What about gtk? And OS/2? Those both support -moz-icon channels, no?
Assignee | ||
Comment 8•19 years ago
|
||
Attachment #174548 -
Attachment is obsolete: true
Attachment #174552 -
Flags: superreview?(bzbarsky)
Attachment #174552 -
Flags: review?(cbiesinger)
Assignee | ||
Updated•19 years ago
|
Attachment #174505 -
Flags: review?(cbiesinger)
Assignee | ||
Updated•19 years ago
|
Attachment #174548 -
Flags: review?(cbiesinger)
Assignee | ||
Comment 9•19 years ago
|
||
see 282548, 282549 for GTK, OS/2 bugs, although I believe OS/2 is pretty much dead at this point.
Comment 10•19 years ago
|
||
Comment on attachment 174552 [details] [diff] [review] ... sr=bzbarsky, assuming biesi's ok with the general setup.
Attachment #174552 -
Flags: superreview?(bzbarsky) → superreview+
Comment 11•19 years ago
|
||
Comment 12•19 years ago
|
||
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-
Assignee | ||
Comment 13•19 years ago
|
||
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:...";
Assignee | ||
Updated•19 years ago
|
Attachment #174552 -
Attachment is obsolete: true
Attachment #174957 -
Flags: review?(cbiesinger)
Comment 14•19 years ago
|
||
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?
Assignee | ||
Comment 15•19 years ago
|
||
Attachment #174957 -
Attachment is obsolete: true
Attachment #175393 -
Flags: review?(cbiesinger)
Assignee | ||
Updated•19 years ago
|
Attachment #174957 -
Flags: review?(cbiesinger)
Comment 16•19 years ago
|
||
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+
Assignee | ||
Comment 17•19 years ago
|
||
Comment on attachment 175393 [details] [diff] [review] impl on nsMIMEInfoWin only asking bz to do one last check...
Attachment #175393 -
Flags: superreview?(bzbarsky)
Comment 18•19 years ago
|
||
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+
Assignee | ||
Updated•19 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•