Comment 156 for bug 25830

(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<nsIChannel> 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.