One resource entry per resource

[plehegar]

This comes from #2884 and this test scenario needs to be addressed by the specification first.

[wpt-pr-bot]

Reviewers for this pull request are: @haoxli, @plehegar, and @zqzhang.

[hoppipolla-critic-bot]

Critic review: https://critic.hoppipolla.co.uk/r/6450

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

[plehegar]

@igrigorik do you know where we stand on this front? Is the test appropriate and is the case covered by the spec?

[igrigorik]

 +    observer.observe({entryTypes: ["resource"]});
 +    var img = document.createElement("img");
 +    img.src = img_location;
 +    document.body.appendChild(img);
 +    img = document.createElement("img");
 +    img.src = img_location;
 +    document.body.appendChild(img);
 +  }, "Only one resource entry per resource");

It's still not clear to me if this is correct. If you follow the RT processing model verbatim, then there should be multiple records, since the second image should (at least in theory) initiate a fetch, which should then hit the appropriate caches and pull out the right data.

However, in practice, in Chrome you only see one request because the request is fulfilled via the ~"renderer memory cache", which is not specified anywhere... I think we should solve that first (probably within Fetch), and then we'll be in a position to answer this.

/cc @toddreifsteck @annevk

[toddreifsteck]

Said another way.. if there was a ServiceWorker behind the fetches, you'd see 2 entries. If there isn't, you may only see 1 today. This is probably not correct and we should consider updating the spec. (We will also need to keep in mind that web devs are used to this behavior today and changing this could break some telemetry processing.)

[annevk]

HTML defines an image cache. Forgot the name atm but I think that is defined to some extent. Not well, since not subject to CSP atm, but…

[gsnedders]

15:01 < aryehgregor> Um, I might have done something wrong by mistake.
15:01 < aryehgregor> I think I pushed to the wrong repo?

[yoavweiss]

We are not testing here a scenario where there will be a second entry added, only in some slight delay that will cause it to be added to the next PerfObs event.

[yoavweiss]

HTML defines an image cache. Forgot the name atm but I think that is defined to some extent. Not well, since not subject to CSP atm, but…

@annevk - I think you're referring to the list of available images.

In any case, I disagree that this case should show two separate entries, as there's no actual fetch triggered for MemoryCache reused images. I'd argue that if we want 2 separate entries here, we should also have 2 entries for every resource the preloader is triggering a preload of. There's no conceptual difference between the two.

Said another way.. if there was a ServiceWorker behind the fetches, you'd see 2 entries. If there isn't, you may only see 1 today. This is probably not correct and we should consider updating the spec.

@toddreifsteck - Why is that? Can you point to the relevant parts of the SW/fetch specs that result in resource reuse triggering new fetch events?

[annevk]

In any case, I disagree that this case should show two separate entries, as there's no actual fetch triggered for MemoryCache reused images. I'd argue that if we want 2 separate entries here, we should also have 2 entries for every resource the preloader is triggering a preload of. There's no conceptual difference between the two.

When are you planning on getting this defined in detail?

[igrigorik]

Discussed this at TPAC yesterday with Todd & Yoav...

It's still not clear to me if this is correct. If you follow the RT processing model verbatim, then there should be multiple records, since the second image should (at least in theory) initiate a fetch, which should then hit the appropriate caches and pull out the right data.

Ignore the above. Step 6 of the RT processing model covers this case:

If the user agent is to reuse the data from another existing or completed fetch initiated from the current document, abort the remaining steps.

So, the behavior that is being tested here is correct -- lgtm for that. For the test structure: it's not clear to me that this is the best way to test the actual behavior, as there is no guarantee that UA must dispatch a single PO event. Ideally, we should check that only one event was delivered some time after the onload event of the second test fired.

[toddreifsteck]

@plehegar Is updating this test per @igrigorik feedback something you can take or should we move it to Xiaoqian?

[igrigorik]

@siusin is this something you'd be able to update (see #2919 (comment)) while you're working on #402?

[plehegar]

Sorry, I missed that earlier :(
To be clear, the proposal is to change the test as follows:
1- attach an observer before the load event
2- add two img tags in the markup, pointing to the same location
3- once the load event fired, check that we only got one entry for the image?

[igrigorik]

Yep! For (1), make sure we register it at the top of the page, well ahead of the images..

[plehegar]

(the pull request needs to be rebased)

[plehegar]

At this point, I'm thinking to wait for w3c/resource-timing#83 ...

[igrigorik]

@plehegar wait for...? As in, tests to cover w3c/resource-timing#87?

[plehegar]

No. I did mean w3c/resource-timing#83 . For the purpose of this test, I need to be able to better predict when the entry will be reported and that's not the case with Firefox at the moment.

[igrigorik]

Err, still confused. What does responseStart have to do with when entry is reported (I assume you mean queued to PerfObserver)?

[igrigorik]

@siusin any chance we can stage this test?

[yoavweiss]

Resubmitted at #6567

[igrigorik]

Thanks Yoav. Closing this, let's continue in the new PR.