Comment 148 for bug 25830

(In reply to comment #121)
> This wants to NS_PRECONDITION aData and NS_PRECONDITION aRequest, right?

fine with me to make aRequest a precondition. aData is allowed to be null though
(see the interface docs).

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

hm... yeah, that makes sense I guess.

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

indeed. will make that change.

> >+ rv = aData->Available(&count);
> >+ // XXX what if that failed?
>
> We cancel aRequest with rv? That would make the most sense...

ok. I guess Cancel will always succeed ;) (it better...)

> 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?

we always pass a null context currently...

> Could you write a comment to that effect?

sure

> I think this would be clearer written as:

ok

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

I think I had a good reason for that change. wish I remembered it...

> >+NS_IMETHODIMP nsExternalAppHandler::GetCanHandleAs(PRBool* aCanHandleAs)
>
> This would be better named GetCanCallHandleAs, I think...

hmm... ok... I'd prefer my name, but sure.

> 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?

Ah... I can do that. my thinking was that the only reference was through necko
-> streamlistener (documentopeninfo) -> exthandler.

> Add a comment explaining that this breaks the cycle?

ok...

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

oh yeah, good point.

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

I dunno, do we? ;) sure, fine with me. in uriloader/base or exthandler? I guess
base is better (I envision possibly passing this to nsIURIContentListener
objects as well, sometime in the future)

> 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? :)

implement it? heh. that's a really good question. personally, I don't expect
anybody to implement it ;)

> >+ * 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.

that's the only object currently which gets an nsIDispatchable? the code would
have to be expanded if we ever pass it to other handlers, sure. I am not sure
that I want to give up this precondition, it seems important to let the current
handler rely on this.

> >+ * *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?

I'd have to check. it probably should get onStopRequest... maybe with a special
status code? (NS_BINDING_RETARGETED?) I suspect onstopr currently doesn't deal
with that very well.

> >+ * 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?

I don't see how that follows... if doContent does call it, we re-enter uriloader
while it's not done with DoContent yet. that seems scary. it would also mean
that the new target listener gets two onstartrequest calls (one from uriloader's
onstartrequest, to which dispatchcontent returns, to which docontent returns;
one from ReDispatch). so this does seem like an nsIDispatchable issue. I can
rephrase it as "Must be called in or after onStartRequest", if you prefer that.

> >+ * 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.

well, the current handler's... I'll mention that.

> >+ * 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...

well, both. interface documentation can be read from both sides :-)
(http://weblogs.asp.net/oldnewthing/archive/2003/12/26/45979.aspx may be an
interesting read)

> >+ * @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?

the real solution may involve storing the request in the DocOpenInfo, but that
increases the risk of cycles...

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

hmm, guess we never store the uricontentlistener we chose? if we do,
DispatchContent could just compare the new one to the current one and fail if
they are equal (after CanHandleContent/IsPreferred returned true)

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

will do