(From update of attachment 176937) >Index: exthandler/nsExternalHelperAppService.cpp >+NS_IMETHODIMP nsExternalAppHandler::GetCanCallHandleAs(PRBool* aCanHandleAs) >+{ >+ *aCanHandleAs = mDispatchable != nsnull && mApplyingDecoding; >+ return NS_OK; >+} please add a comment explaining why mApplyingDecoding modifies the value of this attribute. also, it might read a little better as: *aCanHandleAs = mDispatchable && mApplyingDecoding; > NS_IMETHODIMP nsExternalAppHandler::OnStopRequest(nsIRequest *request, nsISupports *aCtxt, > nsresult aStatus) > { >+ if (aStatus == NS_BINDING_REDIRECTED) >+ return NS_OK; NS_BINDING_REDIRECTED is normally not seen by a nsIStreamListener unless someone refused to allow a HTTP redirect through. This status code is seen by a loadgroup observer, however. Do you maybe mean NS_BINDING_RETARGETED? At the very least this deserves a comment to explain what's going on. Also, I don't think we should overload the meaning of NS_BINDING_REDIRECTED. >+NS_IMETHODIMP nsExternalAppHandler::HandleAs(const nsACString& aMIMEType) >+{ >+ // Ensure that this is allowed >+ PRBool canHandleAs; >+ nsresult rv = GetCanCallHandleAs(&canHandleAs); >+ if (NS_FAILED(rv) || !canHandleAs) >+ return NS_ERROR_UNEXPECTED; So, it is okay to call HandleAs without checking GetCanCallHandleAs? I'm asking it because it seems to be the case that I could write code that doesn't check but rather handles the NS_ERROR_UNEXPECTED error. >+ >+ NS_PRECONDITION(!aMIMEType.IsEmpty() && IsASCII(aMIMEType), >+ "Invalid MIME Type argument"); >+ >+ nsCOMPtr channel(do_QueryInterface(mRequest)); >+ if (!channel) >+ return NS_ERROR_NOT_AVAILABLE; should this perhaps be checked in canCallHandleAs? >Index: exthandler/nsIDispatchable.idl >+/** >+ * This interface allows re-dispatching a request to a different content >+ * handler. It is useful to allow a user to choose to view a file as a >+ * different type than it really is. >+ */ >+[scriptable, uuid(69314C16-EDAD-4c9d-931C-DE2D438F9CF7)] >+interface nsIDispatchable : nsISupports I'm not crazy about the name of this interface. The name suggests that this would have a method named dispatch or something. How about coming up with a name that is more specific. nsIDocumentOpenInfo is not great of course. What it seems like to me is that you are retargeting the channel. Maybe that is because I am necko-centric, but nsIChannel does have a LOAD_TARGETED flag that corresponds to when a channel has a final consumer. Part of the problem here I think is that you are forced into exposing this as a callback interface because of nsIExternalHelperAppService:: DoContent. But, in reality, no one but nsURILoader.cpp should ever care to pass a nsIDispatchable to that DoContent. It seems to me that consumers outside this module will instead focus on the HandleAs API. So, it seems to me that we might be unnecessarily introducing a XPCOM interface for communication between nsDocumentOpenInfo and nsExternalAppHandler. Why not deCOMtaminate this relationship? I'm also not fond of canCallHandleAs. That just doesn't read well. allowsHandleAs, permitsHandleAs, or supportsHandleAs might be better.