Unify 'request' object interface for DNS and PPS asyncResolve methods nsIProtocolProxyService::asyncResolve returns a nsISupports request object that can be canceled via nsIProtocolProxyService::cancelAsyncResolve. nsIDNSService::asyncResolve returns a nsIDNSReqeuest object that can be canceled via nsIDNSRequest::cancel. These should be handled similarly. Maybe they should each return an object with a cancel method, or maybe they should each move the cancel method onto the service interface. Another approach involves implementing nsIRequest, but that is very heavy weight for these objects. See bug 282442 comment #43.

OK, I went ahead with the nsIProtocolProxyRequest solution. I think this is cleaner and easier for consumers than cancelAsyncResolve. The extra interface is not that bad. As for a unified interface shared between the DNS and PPS: *shrug* At least the programming paradigm is the same.

Comment on attachment 178888 [details] [diff] [review] v1 patch + // Provided we haven't been canceled... + if (mStatus == NS_OK) { not NS_SUCCEEDED?

> (From update of attachment 178888 [details] [diff] [review] [edit]) > + // Provided we haven't been canceled... > + if (mStatus == NS_OK) { > > not NS_SUCCEEDED? This code is testing to see if Cancel was called. Since we initialize mStatus to NS_OK, I figured I'd test for NS_OK. There's no point in testing for the more generic NS_SUCCEEDED. The alternative would be to test for NS_ERROR_ABORT.

Comment on attachment 178888 [details] [diff] [review] v1 patch >Index: base/public/nsIProtocolProxyRequest.idl I almost wonder whether we should use the same nsIBasicCancelableRequest interface for both this and the DNS service... It seems silly to have two interfaces with the same exact functionality and different names. If we really do want the two separate interfaces for some reason, perhaps they should both inherit from nsIBasicCancelableRequest or something? >Index: base/src/nsProtocolProxyService.cpp > nsresult DispatchCallback() It's probably best to null out mCallback if this method ends up throwing; otherwise I don't see any way for the reference cycle to be broken... > void OnQueryComplete(nsresult status, const nsCString &pacString) >+ // If we've already called DoCallback then, nothing more to do. Comma should be before "then", not after. sr=bzbarsky with the two nits and if we're happy with the interfaces setup as-is....

bz: thanks for the feedback. any better name than nsIBasicCancelableRequest?

nsICancelable?

Spoke to bz over IRC, and he seemed happy with nsICancelable. This patch implements that, but only for the PPS at this time. I'm going to wack the DNS interfaces with a subsequent patch (need to get rid of init and shutdown as well in that patch).

interdiff for easier reviewing.

Comment on attachment 179300 [details] [diff] [review] v1.1 patch given: + * being canceled. It is an error to pass a success code. can this: + NS_ENSURE_ARG(NS_FAILED(reason)); just be an NS_ASSERTION? r=me either way

The v1.1 patch landed. I'm preparing a followup patch that makes the DNS service use nsICancelable as well.

Comment on attachment 179785 [details] [diff] [review] patch: make DNS service use nsICancelable (v1) looks good. should nsPIDNSService maybe be nonscriptable? + // TODO(darin): make XPCOM proxies support any nsIEventTarget impl wanna file a bug on this?

yeah, i'll file a bug on that. no reason in my mind to make the interface non-scriptable. we might want to use it from a unit test for instance.

fixed-on-trunk