Comment 147 for bug 25830

(From update of attachment 165908)
>Index: uriloader/base/nsURILoader.cpp
>+NS_IMETHODIMP nsDocumentOpenInfo::ReDispatch(const nsACString& aMIMEType,

This wants to NS_PRECONDITION aData and NS_PRECONDITION aRequest, right?

Also, why is it ok to have an aRequest here that's not a channel? When would
that happen, exactly? URILoader in general only handles channels, and we
really do want a channel here to be able to do SetContentType...

>+ // Save type and stream listener

"save old type and stream listener in case we fail to redispatch", right?

>+ rv = aData->Available(&count);
>+ // XXX what if that failed?

We cancel aRequest with rv? That would make the most sense...

>+ rv = m_targetStreamListener->OnDataAvailable(aRequest, nsnull, aData, 0, count);

Hmm... I guess this is an OK context to pass, since there's really no better
one in sight, and we _did_ open this channel ourselves with a null context,
huh?

Could you write a comment to that effect?

> nsCOMPtr<nsIExternalHelperAppService> helperAppService =
> do_GetService(NS_EXTERNALHELPERAPPSERVICE_CONTRACTID, &rv);
>- if (helperAppService) {
>+ if (!aAllowExt) {
>+ rv = NS_ERROR_NOT_AVAILABLE;
>+ } else if (helperAppService) {

I think this would be clearer written as:

nsCOMPtr<nsIExternalHelperAppService> helperAppService;
if (allowExt) {
  helperAppService =
    do_GetService(NS_EXTERNALHELPERAPPSERVICE_CONTRACTID, &rv);
} else {
  rv = NS_ERROR_NOT_AVAILABLE;
}
if (helperAppService) {
...

> nsDocumentOpenInfo::TryContentListener(nsIURIContentListener* aListener,
>- nsIChannel* aChannel)
>+ nsIRequest* aRequest)

I'm not so enamored of that change, for the same reasons as apply to the first
comment on this patch...

>- NS_PRECONDITION(aChannel, "Must have a channel");
>+ NS_PRECONDITION(aRequest, "Must have a channel");

If the change _is_ made, please fix the assert text.

>Index: uriloader/exthandler/nsExternalHelperAppService.cpp

>+NS_IMETHODIMP nsExternalAppHandler::GetCanHandleAs(PRBool* aCanHandleAs)

This would be better named GetCanCallHandleAs, I think...

> NS_IMETHODIMP nsExternalAppHandler::OnStopRequest(nsIRequest *request, nsISupports *aCtxt,

>- mRequest = nsnull;

You sure this doesn't introduce any cycles? Maybe we want to null it out here
only if we've got disposition info or something?

>@@ -2115,12 +2126,15 @@ nsresult nsExternalAppHandler::CreatePro
> mDialog = nsnull;
>+
>+ mDispatchable = nsnull;
>+

Add a comment explaining that this breaks the cycle?

>+NS_IMETHODIMP nsExternalAppHandler::HandleAs(const nsACString& aMIMEType)

This needs to enforce the constraints of GetCanHandleAs() and throw if that
would return false, I think....

>Index: uriloader/exthandler/nsIExternalHelperAppService.idl

>+interface nsIDispatchable : nsISupports

Could we please put this in a different file? Especially if we expect to allow
embeddors to implement nsIDispatchable?

Then again, do we expect people to either call nsIExternalHelperAppService
directly or to implement it? If not, perhaps none of these need be public
apis? I suppose that's for another bug, huh? :)

>+ * In no case will this re-enter the original content handler.

This can only be guaranteed if the original content handler is the helper app
service, no? I think this should be clarified.

>+ * *When this function returns successfully, the current handler WILL NOT
>+ * get an onStopRequest call!*

Maybe it should? Would that actually break anything in this case?

>+ * THIS MUST NOT BE CALLED FROM doContent!

This is an internal implementation notes for nsExternalHelperAppService, no? I
don't see how it would apply in general... Again, do we expect embeddors to
implement nsIExternalHelperAppService?

>+ * This method may be called event after onStopRequest has been called,

"even", not "event". Which onStopRequest? That's non-obvious from looking at
this interface.

>+ * in which case the new listener will receive an onStopRequest call before
>+ * this method returns.

Will or must? That is, is this documentation for implementors of
nsIDispatchable, or for coders in nsExternalHelperAppService? The latter
reaslly does not belong here...

>+ * @param aRequest
>+ * The request to dispatch as. Must be the same as passed to doContent.
>+ * XXX this sucks.

I rather agree.... I'm not sure what the best way to fix this is,
conceptually. Perhaps some verbiage indicating the overall interaction of
nsIExternalHelperAppService and nsIDispatchable is in order?

>+ * No content handler for the registered type was registered; or the
>+ * current handler is the handler for the requested type.

Do we ever enforce the latter condition? I don't believe we do, and don't see
a good way to do it without assuming that the current handler is always the
externalhelperappservice.

>+ void handleAs(in ACString aMIMEType);

This should document that it throws anytime canHandleAs is false or something
like that. At the very least, canHandleAs needs to be mentioned here.