While the spec is not finalized, we are getting somewhat close to agreement on how to do cancellable fetch(). See: https://github.com/whatwg/fetch/issues/447#issuecomment-281731850 We should begin implementing the underlying bits so we can ship relatively quickly after the spec is finalized. Andrew, can we get this on someone's TODO list?

baku, can you also handle this?

I don't want to implement the FetchProgressEvent. I would like to know how the review + the spec go before continuing. It can also be implemented as follow up.

(In reply to Andrea Marchesini [:baku] from comment #6) > I don't want to implement the FetchProgressEvent. I would like to know how > the review + the spec go before continuing. It can also be implemented as > follow up. Yes, priority and progress features should be implemented separately. I expect they will not be spec'd immediately either.

NI myself to review this. Sorry I forgot about it.

Comment on attachment 8844011 [details] [diff] [review] part 1 - FetchController in WebIDL Review of attachment 8844011 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. We should write some WPT tests before enabling on nightly. ::: dom/base/nsGkAtomList.h @@ +887,5 @@ > GK_ATOM(onpopuphiding, "onpopuphiding") > GK_ATOM(onpopuppositioned, "onpopuppositioned") > GK_ATOM(onpopupshowing, "onpopupshowing") > GK_ATOM(onpopupshown, "onpopupshown") > +GK_ATOM(onprioritychange, "onprioritychange") Lets not do the priority bits yet. ::: dom/fetch/FetchController.cpp @@ +53,5 @@ > +} > + > +FetchController::FetchController(nsIGlobalObject* aGlobal) > + : mGlobal(aGlobal) > + , mPriority(0) Remove priority bits. @@ +57,5 @@ > + , mPriority(0) > + , mAborted(false) > +{} > + > +FetchController::~FetchController() = default @@ +77,5 @@ > + return mSignal; > +} > + > +void > +FetchController::SetPriority(uint8_t aPriority) Remove priority bits. ::: dom/fetch/FetchController.h @@ +35,5 @@ > + WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override; > + > + nsIGlobalObject* GetParentObject() const > + { > + return mGlobal; nit: Please put all method bodies in the cpp unless there is a measured perf reason to have it in the header. @@ +42,5 @@ > + FetchSignal* > + Signal(); > + > + void > + SetPriority(uint8_t aPriority); Remove priority bits. @@ +59,5 @@ > + > + nsCOMPtr<nsIGlobalObject> mGlobal; > + RefPtr<FetchSignal> mSignal; > + > + uint8_t mPriority; Remove priority bits. ::: dom/fetch/FetchSignal.cpp @@ +33,5 @@ > + uint8_t aPriority, > + bool aAborted) > + : DOMEventTargetHelper(aController->GetParentObject()) > + , mController(aController) > + , mPriority(aPriority) Remove priority bits. @@ +37,5 @@ > + , mPriority(aPriority) > + , mAborted(aAborted) > +{} > + > +FetchSignal::~FetchSignal() = default @@ +47,5 @@ > + return FetchSignalBinding::Wrap(aCx, this, aGivenProto); > +} > + > +void > +FetchSignal::SetPriority(uint8_t aPriority) Remove priority bits. @@ +78,5 @@ > + EventInit init; > + init.mBubbles = false; > + init.mCancelable = false; > + > + // TODO which kind of event should we dispatch here? Ask this question on the spec issue? ::: dom/fetch/FetchSignal.h @@ +31,5 @@ > + return mPriority; > + } > + > + void > + SetPriority(uint8_t aPriority); Remove priority bits. @@ +36,5 @@ > + > + bool > + Aborted() const > + { > + return mAborted; Please keep method bodies in cpp unless measured perf reason to inline in header. @@ +48,5 @@ > + > +private: > + ~FetchSignal(); > + > + RefPtr<FetchController> mController; It looks like the controller and signal have a cycle? Is that handled by the CC stuff? @@ +49,5 @@ > +private: > + ~FetchSignal(); > + > + RefPtr<FetchController> mController; > + RefPtr<FetchSignal> mSignal; I think mSignal can be removed here? @@ +51,5 @@ > + > + RefPtr<FetchController> mController; > + RefPtr<FetchSignal> mSignal; > + > + uint8_t mPriority; Remove priority bits. ::: dom/fetch/Request.h @@ +11,5 @@ > #include "nsISupportsImpl.h" > #include "nsWrapperCache.h" > > #include "mozilla/dom/Fetch.h" > +#include "mozilla/dom/FetchSignal.h" Do you really need this header here? Nothing else changed in the file ::: dom/tests/mochitest/fetch/test_fetch_controller.html @@ +4,5 @@ > +--> > +<!DOCTYPE HTML> > +<html> > +<head> > + <title>Test FetchController</title> I think you need to update test_interfaces for window and worker. Also, it would be nice to write our tests in WPT so we can upstream them to other browsers. We can set prefs for WPT tests in order to enable the feature. ::: dom/webidl/FetchController.webidl @@ +8,5 @@ > + Func="FetchController::IsEnabled"] > +interface FetchController { > + readonly attribute FetchSignal signal; > + > + void setPriority(octet priority); Please leave out the priority and progress stuff for now. Those parts aren't as far in the spec. We don't want to include in the webidl so people can more easily feature detect the fact they are not there. ::: dom/webidl/FetchSignal.webidl @@ +6,5 @@ > + > +[Exposed=(Window,Worker), > + Func="FetchController::IsEnabled"] > +interface FetchSignal : EventTarget { > + readonly attribute octet priority; Remove priority bits. @@ +10,5 @@ > + readonly attribute octet priority; > + readonly attribute boolean aborted; > + > + attribute EventHandler onabort; > + attribute EventHandler onprioritychange; Remove priority bits. ::: dom/webidl/Request.webidl @@ +48,5 @@ > RequestRedirect redirect; > DOMString integrity; > + > + [Func="FetchController::IsEnabled"] > + FetchSignal signal; A later patch in the bug will implement using this? We shouldn't put it in here unless we are going to support it.

Comment on attachment 8844012 [details] [diff] [review] part 2 - Follow/Unfollow Review of attachment 8844012 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. ::: dom/fetch/FetchController.cpp @@ +11,5 @@ > namespace mozilla { > namespace dom { > > +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(FetchController, mGlobal, mSignal, > + mFollowingSignal) I think its a little confusing to CC a parent class member like this. I'm not sure what the best to handle it is, though. ::: dom/fetch/FetchController.h @@ +56,5 @@ > > + FetchSignal* > + Following() const > + { > + return mFollowingSignal; Please put method bodies in the cpp file unless there is a measured perf reason to have in the header. @@ +61,5 @@ > + } > + > + // FetchFollower > + > + void PriorityChanged(uint8_t aPriority) override; Remove priority bits. ::: dom/fetch/FetchSignal.cpp @@ +102,5 @@ > + > +void > +FetchSignal::AddFollower(FetchFollower* aFollower) > +{ > + MOZ_ASSERT(aFollower); Please use MOZ_DIAGNOSTIC_ASSERT() for cheap checks like this. @@ +103,5 @@ > +void > +FetchSignal::AddFollower(FetchFollower* aFollower) > +{ > + MOZ_ASSERT(aFollower); > + if (!mFollowers.Contains(aFollower)) { Is this because content can try to follow multiple times? @@ +144,5 @@ > +void > +FetchFollower::Follow(FetchSignal* aSignal) > +{ > + MOZ_ASSERT(aSignal); > + Unfollow(); Should we only unfollow if we pass the CanAcceptFollower() check? Otherwise this could unfollow old signal, but not follow new signal. ::: dom/fetch/FetchSignal.h @@ +15,5 @@ > class FetchController; > +class FetchSignal; > + > +// This class must be implemented by objects who want to follow a FetchSignal. > +class FetchFollower Maybe move this into FetchSignal to make it more clear they are associated. So it would be FetchSignal::Follower. @@ +18,5 @@ > +// This class must be implemented by objects who want to follow a FetchSignal. > +class FetchFollower > +{ > +public: > + virtual void PriorityChanged(uint8_t aPriority) = 0; Remove priority bits.

Comment on attachment 8844014 [details] [diff] [review] part 3 - FetchSignal in Fetch API Review of attachment 8844014 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. ::: dom/fetch/Fetch.cpp @@ +601,5 @@ > } > > + if (aReason == FetchDriverObserver::eAborted) { > + RefPtr<WorkerFetchSignalAbortRunnable> r = > + new WorkerFetchSignalAbortRunnable(mPromiseProxy->GetWorkerPrivate(), Why a new, separate runnable here? It seems we should just incorporate this result into the existing WorkerFetchResponseEndRunnable and WorkerFetchResponseEndControlRunnable classes. ::: dom/fetch/FetchDriver.cpp @@ +877,5 @@ > + // TODO > +} > + > +void > +FetchDriver::Aborted() Can you MOZ_ASSERT(NS_IsMainThread()) this? ::: dom/fetch/FetchDriver.h @@ +91,5 @@ > NS_DECL_NSICHANNELEVENTSINK > NS_DECL_NSIINTERFACEREQUESTOR > NS_DECL_NSITHREADRETARGETABLESTREAMLISTENER > > + explicit FetchDriver(InternalRequest* aRequest, nit: This doesn't need explicit any more (and didn't before).

Comment on attachment 8844402 [details] [diff] [review] part 4 - FetchObserver WebIDL Review of attachment 8844402 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed ::: dom/fetch/FetchObserver.cpp @@ +28,5 @@ > +/* static */ bool > +FetchObserver::IsEnabled(JSContext* aCx, JSObject* aGlobal) > +{ > + if (NS_IsMainThread()) { > + return Preferences::GetBool("dom.fetchObserver.enabled", false); Can we just tie this to the same pref as the controller? Not sure its worth enabling them separately. @@ +50,5 @@ > + , mPriority(aPriority) > + , mState(aState) > +{} > + > +FetchObserver::~FetchObserver() = default ::: dom/fetch/FetchObserver.h @@ +29,5 @@ > + > + uint8_t > + Priority() const > + { > + return mPriority; Remove priority bits. @@ +35,5 @@ > + > + FetchState > + State() const > + { > + return mState; Please move method bodies to cpp file.

Comment on attachment 8844403 [details] [diff] [review] part 5 - FetchObserver notifications Review of attachment 8844403 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed ::: dom/fetch/Fetch.cpp @@ +680,5 @@ > + RefPtr<WorkerDataAvailableRunnable> r = > + new WorkerDataAvailableRunnable(mPromiseProxy->GetWorkerPrivate(), this); > + > + if (!r->Dispatch()) { > + NS_WARNING("Could not dispatch fetch data available runnable"); Personally I don't find these warnings helpful. Perhaps just do something like: // If the worker is shutting down its ok if we don't update the // fetch state. Unused << r->Dispatch(); ::: dom/fetch/FetchObserver.cpp @@ +119,5 @@ > + EventInit init; > + init.mBubbles = false; > + init.mCancelable = false; > + > + // TODO which kind of event should we dispatch here? Ask on spec issue?

Thanks for working on this! I think it would be great if we could land this disabled for now. We can then write some WPT tests and enable as an experiment on nightly only.

Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/25d4dfbdd6f9 Implement FetchController and FetObserver - part 1 - FetchController in WebIDL, r=bkelly https://hg.mozilla.org/integration/mozilla-inbound/rev/8424f191ee09 Implement FetchController and FetObserver - part 2 - Follow/Unfollow, r=bkelly https://hg.mozilla.org/integration/mozilla-inbound/rev/d755ad0719ee Implement FetchController and FetObserver - part 3 - FetchSignal in Fetch API, r=bkelly https://hg.mozilla.org/integration/mozilla-inbound/rev/660134649942 Implement FetchController and FetObserver - part 4 - FetchObserver WebIDL, r=bkelly https://hg.mozilla.org/integration/mozilla-inbound/rev/617238ba1988 Implement FetchController and FetObserver - part 5 - Dispatching observer events, r=bkelly

I prefer to keep FetchObserver and FetchController with 2 separate prefs. They could be enable/disable separately.

https://hg.mozilla.org/mozilla-central/rev/25d4dfbdd6f9 https://hg.mozilla.org/mozilla-central/rev/8424f191ee09 https://hg.mozilla.org/mozilla-central/rev/d755ad0719ee https://hg.mozilla.org/mozilla-central/rev/660134649942 https://hg.mozilla.org/mozilla-central/rev/617238ba1988

Note, this is experimental and the spec may finalize slightly differently. I expect the overall structure to match what we implemented, though.

Why doesn't FetchSignal.webidl have a link to the relevant spec? All webidl files that are based on a spec (draft or not) should link to the thing they're based on...

This was speculative based on initial spec discussions at the time. It's since been changed to AbortSignal.webidl and it now does have the spec linked. (As of the last 24 hours or so.)

Removing ddn, as the docs for this were handled in other places, unless I'm mistaken (https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API#Aborting_a_fetch)