The external helper app service has a method, GetProtocolHandlerInfo, for retrieving an nsIHandlerInfo object for a protocol, but it's not exposed to frontend code via any API. Exposure would be useful, since code in the Download Actions dialog (which I'm modifying over in bug 377784) would benefit from being able to retrieve nsIHandlerInfo objects for protocols via an API rather than directly from the datasource. dmose: GetProtocolHandlerInfo says "XXX before we expose this to the UI, we need sort out our strategy re the "warning" and "exposed" prefs" <myk> dmose: do you know what the issues are there? <dmose> myk: i just wanted to make sure we didn't break existing semantics <dmose> myk: i talked to beltzner, and he said he wanted to break the warning semantics <dmose> and that we'd go ahead and do that now and discuss more during security review <dmose> as far as i know, we haven't broken the "exposed" semantics or the forbidden semantics <dmose> some automated tests in that area would be a fine thing <myk> dmose: so you're comfortable with me exposing that method to the UI? <dmose> i think so, yes. maybe paste the stuff i said above into the bug, and add qa-wanted

This patch makes GetProtocolHandlerInfo be a method of the nsIExternalProtocolService interface. Note that the method behaves the same as nsIMIMEService::GetByTypeAndExtension, in that it returns an nsIHandlerInfo object whether or not there's a known handler for the given type. To minimize the amount of code that needed to change, I left the aProtocolScheme parameter to GetProtocolHandlerInfo as an ACString, even though the externalProtocolHandlerExists and isExposedProtocol methods of the external protocol service take a string. If I'd made GetProtocolHandlerInfo consistent with the other two methods, I would have had to modify all its existing callers. Plus, given that nsIURI::scheme is an ACString, it seems to me that the parameter for all three methods should be that type. I've noted that in comments on the two existing methods.

Comment on attachment 275510 [details] [diff] [review] patch v1: nsIExternalProtocolService::GetProtocolHandlerInfo Looks good. Nit: I don't think moving the GetProtocolHandlerInfo method to be next to the other NS_IMETHODs is a good trade for making the cvsblame harder to navigate. How about leaving it where it is? > /** >+ * Retrieve the handler for the given protocol, if any. >+ * @param aProtocolScheme the scheme from a url: http, ftp, mailto, etc. >+ * @return the handler, if any; otherwise null >+ * FIXME: specify whether or not the scheme should include a trailing colon. The current implementation expects there to be no trailing colon, which I think is correct. The colon is part of the URI/URL syntax, not part of the scheme. Let's document that. sr=dmose

(In reply to comment #2) > (From update of attachment 275510 [details] [diff] [review]) > Looks good. Nit: I don't think moving the GetProtocolHandlerInfo method to be > next to the other NS_IMETHODs is a good trade for making the cvsblame harder to > navigate. How about leaving it where it is? I like the code tidyness of putting it in the right place, but I see your point about cvsblame. I'll put it back where it was in the patch I check in (or in a followup patch, if one is required). > The current implementation expects there to be no trailing colon, which I > think is correct. The colon is part of the URI/URL syntax, not part of the > scheme. Let's document that. Sounds good, I'll fix this comment to specify that the scheme should not include a trailing colon.

(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 275510 [details] [diff] [review] [details]) > > Looks good. Nit: I don't think moving the GetProtocolHandlerInfo method > > to be next to the other NS_IMETHODs is a good trade for making the cvsblame > > harder to navigate. How about leaving it where it is? > > I like the code tidyness of putting it in the right place, but I see your > point about cvsblame. I'll put it back where it was in the patch I check in > (or in a followup patch, if one is required). Ok, it's back where it was. > > The current implementation expects there to be no trailing colon, which I > > think is correct. The colon is part of the URI/URL syntax, not part of the > > scheme. Let's document that. > > Sounds good, I'll fix this comment to specify that the scheme should not > include a trailing colon. Fixed. I also fixed the comment to specify that GetProtocolHandlerInfo always returns a valid handler info object, even if neither the application nor the OS knows about a specific handler for the scheme. Now it reads as follows: /** * Retrieve the handler for the given protocol. If neither the application * nor the OS knows about a handler for the protocol, the object this method * returns will represent a default handler for unknown content. * * @param aProtocolScheme the scheme from a URL: http, ftp, mailto, etc. * * Note: aProtocolScheme should not include a trailing colon, which is part * of the URI syntax, not part of the scheme itself (i.e. pass "mailto" not * "mailto:"). * * @return the handler, if any; otherwise a default handler */ Carrying forward superreview.

> If neither the application > nor the OS knows about a handler for the protocol, the object this method > returns will represent a default handler for unknown content. It'd be good to clarify what the implications of this are: that this object shouldn't be stored or used unless the caller first fills enough appropriate data to make launchWith* actually do something. (That is the actual semantic here, right?)

(In reply to comment #5) > > If neither the application > > nor the OS knows about a handler for the protocol, the object this method > > returns will represent a default handler for unknown content. > > It'd be good to clarify what the implications of this are: that this object > shouldn't be stored or used unless the caller first fills enough appropriate > data to make launchWith* actually do something. (That is the actual semantic > here, right?) Actually, I think in this case the object represents a reasonable default (like "save to disk" for MIME handlers) and it doesn't matter whether or not we store it, as the stored values will be equivalent to the values obtained when the object is not stored.

Checking in uriloader/exthandler/nsExternalHelperAppService.cpp; /cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp,v <-- nsExternalHelperAppService.cpp new revision: 1.335; previous revision: 1.334 done Checking in uriloader/exthandler/nsExternalHelperAppService.h; /cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.h,v <-- nsExternalHelperAppService.h new revision: 1.86; previous revision: 1.85 done Checking in uriloader/exthandler/nsIExternalProtocolService.idl; /cvsroot/mozilla/uriloader/exthandler/nsIExternalProtocolService.idl,v <-- nsIExternalProtocolService.idl new revision: 1.12; previous revision: 1.11 done