1637562 - Expose an API on `ResourceWatcher` in order to only listen to "future" resources

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Alexandre Poirot [:ochameau]]

Currently, ResourceWatcher.watch is expected to call the onAvailable callback for resources which already exists. i.e. for resources that were created before the call to watch().
But:

1. There is no way to only listen for "future" resources. i.e. only the one which are created after the call to watch(). And bug 1626494 highlighted a case where that would be useful.
2. Bug 1625961 highlighted that watch was inconsistant, and while the first call would notify about past and future resources, all other calls after the first one would only notify about the future ones. We are currently relying on this inconsistancy in order to only retrieve future resources for DOCUMENT_EVENTS resource. Such flag would help addressing this inconsistancy by being explicit about it. The current codebase uses a whitelist, but that is imperfect as the inconsistancy is still here for DOCUMENT_EVENTS.

I imagine we could probably first introduce a dictionnary argument in ResourceWatcher.watch (bug 1637561), then, here we would expose new optional boolean: watchOnlyFutureResources, or onlyFuture, ... (or any naming that makes sense).

Then, regarding how to implement this attribute, we might try to convey this boolean down to the legacy listeners, so that we avoid fetching "cached"/"past" resources.
For example, we could avoid executing these lines.
Later on in bug 1626647, we may pass this flag to the WatcherActor so that it knows if it should emit past resources or not.
Note that depending on how we implement that, we may have inconsistancy depending on the call order of watch({ onlyFuture: false }) versus watch({ onlyFuture: true })

Also, I think that this behavior "only listen for future resources" is most likely going to be rare and should be considered as an edge case.
We may add various restriction to it, like:

• only accept listening for future resource after a call to watch has been made listening for everything?
• only accept listening for only future for a few special resources?
  Given this, we may expose this feature through an API other than watch.
  We could for example expose:
• onNextResource(resourceType) returning a promise, a bit like eventEmitter.once
• onNextResource(resourceType, resource => resource.name == "dom-complete") similar, but with an optional callback to filter out the resource, in order to match a precise one.
• or some other naming or interface that makes sense!
  This function could throw if we weren't already watching for this resource type. Or automagically start calling watch, but be careful of inconsistancy of call order between watch and onNextResource.

Comment 1

[Alexandre Poirot [:ochameau]]

FYI, Daisuke already started prototyping something in https://phabricator.services.mozilla.com/D67438.

Comment 2

[Chris Peterson [:cpeterson]]

Tracking dt-fission-m2-mvp bugs for Fission Nightly milestone (M6c)

Comment 3

[Alexandre Poirot [:ochameau]]

From: https://phabricator.services.mozilla.com/D67438#inline-436437

Alex:
I was wondering if we could, instead of modifying ResourceWatcher.watch, have a dedicated new method on ResourceWatcher, which would better fit what we need here.
I thought about ResourceWatcher.onNextResource(resourceType, testFunction), a method we would resolve once we receive the next future resource of a given type, and we can also use a testFunction which would help filter out the one resource we are waiting for
I first thought about returning a promise, but we can tweak that accordingly to the needs.

Daisuke:
I have one question.
Will this onNextResource enable only once as same as EventEmitter.once()?
If so, as we want to catch all future resources in this case, we take the events by like this?

async function observe() {
  const resource = await watcher.onNextResource(DOCUMENT_EVENT);
  … do something
  observe();
}

Or, similar to watch but get future resources only?

function handler(resource) {
  ... do something
}
await watcher.onNextResource(DOCUMENT_EVENT, handler);

Sorry, I thought that WebExtension codebase was only listening to the next resource and not to all the following.
So, yes it makes sense to have a listener/handler instead of returning a promise like EventEmitter.once.
Then it looks like watch again. So the choice is between:
watcher.watch(DOCUMENT_EVENT, { onAvailable: listener, onlyFuture: true });
versus
watcher.onNextResources(DOCUMENT_EVENT, listener);

Nicolas, Julian, do you have any opinion?
(See comment 0 for much more context)

Comment 4

[Julian Descottes [:jdescottes]]

I slightly prefer the first option. I think it's pretty clear that if we watch for onlyFuture (or another arg name if we can think of something else), it will behave exactly like watch, except you won't get the already existing targets. With a new separate method onNextResources, it could be slightly harder to understand for someone new to these APIs.

But as I said, I "slightly" prefer it, we could go either way.

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

I non-slightly prefer the first option too :)
A flag that limit what we consume is enough, and it has the benefit of us keeping the same function, which is better in the long run, as with 2 functions, we'd have to keep them "in-sync" or they'll diverge at some point.

I'd like something like watcher.watch(DOCUMENT_EVENT, onAvailable, onUpdate, { ignoreExistingResources: true } so we keep the current call as they are now (watcher.watch(CONSOLE_MESSAGE, onResourceAvailable)), and if a callpoint need to add an option they can do it.

Comment 6

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1637562: Introduce 'onlyFuture' flag in order to listen to future resources only.

Depends on D75482

Comment 7

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1637562: Make the future resources feature available for document events.

Depends on D75746

Comment 8

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1637562: Add test for ignoreExistingResources flag for document events.

Depends on D75747

Comment 9

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1637562: Add test for ignoreExistingResources flag for console messages.

Depends on D75748

Comment 10

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1637562: Add test for ignoreExistingResources flag for error messages.

Depends on D75749

Comment 11

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1637562: Add test for ignoreExistingResources flag for platform messages.

Depends on D75750

Comment 12

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1637562: Introduce 'ignoreExistingResources' flag in order to ignore existing resources.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3d2af0d65f39
Introduce 'ignoreExistingResources' flag in order to ignore existing resources. r=ochameau,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/c5b2ed837172
Add test for ignoreExistingResources flag for document events. r=ochameau,jdescottes
https://hg.mozilla.org/integration/autoland/rev/6761e905feaf
Add test for ignoreExistingResources flag for console messages. r=ochameau,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/44167342f7f0
Add test for ignoreExistingResources flag for error messages. r=ochameau,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/366f83055635
Add test for ignoreExistingResources flag for platform messages. r=ochameau,nchevobbe

Comment 14

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/3d2af0d65f39
https://hg.mozilla.org/mozilla-central/rev/c5b2ed837172
https://hg.mozilla.org/mozilla-central/rev/6761e905feaf
https://hg.mozilla.org/mozilla-central/rev/44167342f7f0
https://hg.mozilla.org/mozilla-central/rev/366f83055635