Expose initiatorUrl in resourceTiming for html initiators [chromium/src : main]
Guohui Deng (Gerrit)
Guohui Deng added 1 comment![]( "Open in Gerrit")
This CL calculates the resource initiator using
BuildInitiatorObejct() that's used by devTool, Except for using "TaskAttribution" instead of "V8_Inspector_StackTrace".
This CL already achieves more than
https://chromium-review.googlesource.com/c/chromium/src/+/4812813 and
https://chromium-review.googlesource.com/c/chromium/src/+/5242518 combined.
And I think I am very close to solve the "JS initiator" and "CSS initiator".
Other unsovled cases are:
1. A main html initiates another html file in a subframe;
2. Worker thread fetchs (not sure if this requires an additional code path)
--
I would like to highlight the following: (that I think there may be possible objects)
1. There is some overlap with Devtool code. Could potentially consolidate. For our purpose "TaskAttribution" is better because it seems faster and it does tell the script url. (whereas StackTrace seems to tell the file name only), so it looks to me a complete consolidate is not likely though.
2. I propose that the `initiator info` is only provided for the resource loaded from the network. This reduces overhead of the Chromium(When not loading from network, resources load fast so it's important to reduce overhead), simplifies both implementation and how the information is used to construct resource dependency tree. We could further discuss at
https://github.com/w3c/resource-timing/issues/263
Thanks for reviewing!
Guohui
Related details
- Ian Clelland
- Noam Rosenthal
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 1 comment![]( "Open in Gerrit")
Thank you so much for working on this!! I'll review next week (traveling this week)
Related details
- Guohui Deng
- Ian Clelland
- Noam Rosenthal
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 1 comment![]( "Open in Gerrit")
Apologies. I failed to review this and I'm traveling this week..
Noam - could you take a look?
Noam Rosenthal (Gerrit)
Noam Rosenthal added 5 comments![]( "Open in Gerrit")
Nice work, did a first pass
Expose initiatorUrl in resourceTiming for html initiatorsCan you explain in one sentence what this is, andd add a link to spec/chromestatus? Is there an I2P for this?
while (document && !document->GetScriptableDocumentParser()) {Why are we going up the frame owner chain?
../../web_tests/external/wpt/fonts/Lato-Medium-Liga.ttfWhat are these for?
Related details
- Guohui Deng
- Ian Clelland
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guohui Deng (Gerrit)
Guohui Deng added 4 comments![]( "Open in Gerrit")
Expose initiatorUrl in resourceTiming for html initiatorsCan you explain in one sentence what this is, andd add a link to spec/chromestatus? Is there an I2P for this?
I rewrote the commit message. I added the link to chromestatus. No spec PR though :(
I sent the I2P email and updated the chromestatus page with the link to the I2P email.
while (document && !document->GetScriptableDocumentParser()) {Why are we going up the frame owner chain?
Thank you for catching that! I removed the code that goes up the frame owner chain.
I think the devTool code goes up the frame owner chain because it requires a "line position". For ResourceTiming we need the most accurate resource identification so we shouldn't go up the frame owner chain.
../../web_tests/external/wpt/fonts/Lato-Medium-Liga.ttfGuohui DengWhat are these for?
Sorry I should have made some explanation here. Back then when this CL was running CQ, something else was broken so I had to add the two lines. Later someone else added the two lines. After I rebase, the two lines are no longer part of this CL.
initiator_url("") {}Unnecessary line
Removed. Additionally, I added "checking null value" in performance_resource_timing.cc
Related details
- Ian Clelland
- Noam Rosenthal
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guohui Deng (Gerrit)
Guohui Deng added 1 comment![]( "Open in Gerrit")
// TODO(guohu...@microsoft.com): Find out if the initiator is a script
// resource. If yes, fill |initiator_ulr| accordingly and return.Update:
In Yosh's CL, `TaskAttribution` was used to determine if the JS is loading resource dynamically, so I was using the same code here before.
Then I tried the next step: using the TaskAttribution to distinguish between:
1) iframe loaded in html statically
2) iframe loaded by javascript dynamically
I found out that TaskAttribution no longer works for this purpose. I contacted Scott (shaseley@). The TaskAttribution is still under development, and we could extend TaskAttribution to support this. So there is some work to do in TaskAttribution for the "initiator info" project.
Related details
- Ian Clelland
- Noam Rosenthal
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal voted and added 3 comments![]( "Open in Gerrit")
Votes added by Noam Rosenthal
| Code-Review | +1 |
3 comments
#include "third_party/blink/renderer/platform/scheduler/public/task_attribution_tracker.h"Check if all these includes are needed
while (document && !document->GetScriptableDocumentParser()) {Guohui DengWhy are we going up the frame owner chain?
Thank you for catching that! I removed the code that goes up the frame owner chain.
I think the devTool code goes up the frame owner chain because it requires a "line position". For ResourceTiming we need the most accurate resource identification so we shouldn't go up the frame owner chain.
Acknowledged
initiator_url("") {}Guohui DengUnnecessary line
Removed. Additionally, I added "checking null value" in performance_resource_timing.cc
- Guohui Deng
- Ian Clelland
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guohui Deng (Gerrit)
Guohui Deng added 1 comment![]( "Open in Gerrit")
#include "third_party/blink/renderer/platform/scheduler/public/task_attribution_tracker.h"Check if all these includes are needed
- Ian Clelland
- Noam Rosenthal
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal voted and added 1 comment![]( "Open in Gerrit")
Votes added by Noam Rosenthal
| Code-Review | -1 |
1 comment
Please link to at least a public explainer before submitting further code changes.
Related details
- Guohui Deng
- Ian Clelland
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guohui Deng (Gerrit)
Guohui Deng added 1 comment![]( "Open in Gerrit")
Please link to at least a public explainer before submitting further code changes.
- Ian Clelland
- Noam Rosenthal
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guohui Deng (Gerrit)
Guohui Deng added 1 comment![]( "Open in Gerrit")
Guohui DengPlease link to at least a public explainer before submitting further code changes.
Done.
I mean I added the explainer link in the commit message. There is also an explainer link in third_party/blink/renderer/core/timing/performance_resource_timing.idl.
I updated the link to the one in MicrosoftEdge repo because they require that I put the explainer there. That should be accessible to the public. Is it O.K.?
Noam Rosenthal (Gerrit)
Noam Rosenthal voted and added 2 comments![]( "Open in Gerrit")
Votes added by Noam Rosenthal
| Code-Review | -1 |
Guohui DengPlease link to at least a public explainer before submitting further code changes.
Guohui DengDone.
I mean I added the explainer link in the commit message. There is also an explainer link in third_party/blink/renderer/core/timing/performance_resource_timing.idl.
I updated the link to the one in MicrosoftEdge repo because they require that I put the explainer there. That should be accessible to the public. Is it O.K.?
Acknowledged
Expose initiatorUrl in resourceTiming for html initiatorsGuohui DengCan you explain in one sentence what this is, andd add a link to spec/chromestatus? Is there an I2P for this?
I rewrote the commit message. I added the link to chromestatus. No spec PR though :(
I sent the I2P email and updated the chromestatus page with the link to the I2P email.
- Guohui Deng
- Ian Clelland
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal voted and added 2 comments![]( "Open in Gerrit")
Votes added by Noam Rosenthal
| Code-Review | +0 |
2 comments
// resource. If yes, fill |initiator_ulr| accordingly and return.ulr -> url
```suggestion
// resource. If yes, fill |initiator_url| accordingly and return.
```
Resource* resource) {Hmm not sure about this. I don't know about the design of filling the initiator info in an "observer". It seems surprising because obsrver usually don't change what they observe.
Related details
- Guohui Deng
- Ian Clelland
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guohui Deng (Gerrit)
Guohui Deng added 2 comments![]( "Open in Gerrit")
// resource. If yes, fill |initiator_ulr| accordingly and return.ulr -> url
```suggestion
// resource. If yes, fill |initiator_url| accordingly and return.
```
Done
Resource* resource) {Hmm not sure about this. I don't know about the design of filling the initiator info in an "observer". It seems surprising because obsrver usually don't change what they observe.
It's indeed not common.
The logic is here because it is not possible to implement it in platform/loader. The logic satisfies all the conditions listed here:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/loader/fetch/resource_load_observer.h;l=28;drc=c76e4f83a8c5786b463c3e55c070a21ac751b96b
There is actually a precedence in `ResourceLoadObserverForFrame` that the observer modifies the data it receives.
Notice the ` (... BlobDataHandle* blob) ` isn't "const" in `DidDownloadToBlob(...)`
(https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/resource_load_observer_for_frame.cc;l=286)
Eventually, the ownership of `blob` is transferred to a collection of inspector agents, see:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/inspector/inspector_network_agent.cc;l=1663;drc=c76e4f83a8c5786b463c3e55c070a21ac751b96b;bpv=1;bpt=1
I think the most important reason for `ResourceLoadObserver` is that it's a place where we can add react logic to individual "load" that cannot happen in platform/loader folder. Modification to the observed data should be allowed here, although it's not common.
The main problem here is actually the name "observer", rather than the design that code in `platform/loader` calls into `core/loader` for something that cannot be in `platform/loader`. The alternative is to rename "observer" to something else. But then someone may object, reasoning that observer can modify things occasionally if it has to, and the name "observer" is suitable except for that we don't modify the data usually.
What do you think?
Related details
- Ian Clelland
- Noam Rosenthal
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guohui Deng (Gerrit)
Guohui Deng added 1 comment![]( "Open in Gerrit")
Resource* resource) {Guohui DengHmm not sure about this. I don't know about the design of filling the initiator info in an "observer". It seems surprising because obsrver usually don't change what they observe.
It's indeed not common.
The logic is here because it is not possible to implement it in platform/loader. The logic satisfies all the conditions listed here:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/loader/fetch/resource_load_observer.h;l=28;drc=c76e4f83a8c5786b463c3e55c070a21ac751b96bThere is actually a precedence in `ResourceLoadObserverForFrame` that the observer modifies the data it receives.
Notice the ` (... BlobDataHandle* blob) ` isn't "const" in `DidDownloadToBlob(...)`
(https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/resource_load_observer_for_frame.cc;l=286)
Eventually, the ownership of `blob` is transferred to a collection of inspector agents, see:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/inspector/inspector_network_agent.cc;l=1663;drc=c76e4f83a8c5786b463c3e55c070a21ac751b96b;bpv=1;bpt=1I think the most important reason for `ResourceLoadObserver` is that it's a place where we can add react logic to individual "load" that cannot happen in platform/loader folder. Modification to the observed data should be allowed here, although it's not common.
The main problem here is actually the name "observer", rather than the design that code in `platform/loader` calls into `core/loader` for something that cannot be in `platform/loader`. The alternative is to rename "observer" to something else. But then someone may object, reasoning that observer can modify things occasionally if it has to, and the name "observer" is suitable except for that we don't modify the data usually.
What do you think?
For this CL, I think I can avoid this problem by making a separate callback/call_back_class that's explicitly named `InitiatorInfoCalculator`.(Which is parallel to this `ResourceLoadObserverForFrame` class). In future if we have more callbacks that modify the observed data, we can have a container class called `ResourceLoadCallbacksForFrame`.
But I don't quite like it because it's very verbose and it's duplicate to this `ResourceLoadObserverForFrame` class.
Alternatively, we can put all the callbacks in the same container(i.e., the current `ResourceLoadObserverForFrame`). Because some modifies the observed data, we don't call this container "observer". So we could rename this class `ResourceLoadObserverForFrame` to `ResourceLoadCallbacksForFrame`. This approach is simpler, but it's a big change. I don't know if someone would object.
Would appreciate any thoughts shared! Thanks!
Guohui Deng (Gerrit)
Guohui Deng added 1 comment![]( "Open in Gerrit")
Resource* resource) {Guohui DengHmm not sure about this. I don't know about the design of filling the initiator info in an "observer". It seems surprising because obsrver usually don't change what they observe.
Guohui DengIt's indeed not common.
The logic is here because it is not possible to implement it in platform/loader. The logic satisfies all the conditions listed here:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/loader/fetch/resource_load_observer.h;l=28;drc=c76e4f83a8c5786b463c3e55c070a21ac751b96bThere is actually a precedence in `ResourceLoadObserverForFrame` that the observer modifies the data it receives.
Notice the ` (... BlobDataHandle* blob) ` isn't "const" in `DidDownloadToBlob(...)`
(https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/resource_load_observer_for_frame.cc;l=286)
Eventually, the ownership of `blob` is transferred to a collection of inspector agents, see:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/inspector/inspector_network_agent.cc;l=1663;drc=c76e4f83a8c5786b463c3e55c070a21ac751b96b;bpv=1;bpt=1I think the most important reason for `ResourceLoadObserver` is that it's a place where we can add react logic to individual "load" that cannot happen in platform/loader folder. Modification to the observed data should be allowed here, although it's not common.
The main problem here is actually the name "observer", rather than the design that code in `platform/loader` calls into `core/loader` for something that cannot be in `platform/loader`. The alternative is to rename "observer" to something else. But then someone may object, reasoning that observer can modify things occasionally if it has to, and the name "observer" is suitable except for that we don't modify the data usually.
What do you think?
For this CL, I think I can avoid this problem by making a separate callback/call_back_class that's explicitly named `InitiatorInfoCalculator`.(Which is parallel to this `ResourceLoadObserverForFrame` class). In future if we have more callbacks that modify the observed data, we can have a container class called `ResourceLoadCallbacksForFrame`.
But I don't quite like it because it's very verbose and it's duplicate to this `ResourceLoadObserverForFrame` class.
Alternatively, we can put all the callbacks in the same container(i.e., the current `ResourceLoadObserverForFrame`). Because some modifies the observed data, we don't call this container "observer". So we could rename this class `ResourceLoadObserverForFrame` to `ResourceLoadCallbacksForFrame`. This approach is simpler, but it's a big change. I don't know if someone would object.
Would appreciate any thoughts shared! Thanks!
Sorry I cannot edit my previous comment. Let me make some corrections and clarifications. Below are two options that I am considering:
Option 1. Rename `ResourceLoadObserver' class to `ResourceLoadClient`
Option 2. Use a partially bound `RepeatingCallback` named `fill_initiator_info_cb_`. This callback is set from `core/loader` and called in `platform/loader`
1) has larger footprint, while 2) is a little hacky.
Kouhei Ueno (Gerrit)
Kouhei Ueno added 1 comment![]( "Open in Gerrit")
yyanagisawa: Would you review this CL since this is a significant change to core/loading stack?
nidhi: Would you review the performance impact of the change (since you are looking into URL handling optimizations)?
Related details
- Guohui Deng
- Ian Clelland
- Nidhi Jaju
- Noam Rosenthal
- Yoshisato Yanagisawa
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nidhi Jaju (Gerrit)
Nidhi Jaju added 2 comments![]( "Open in Gerrit")
UrlWithoutFragment(document->Url()).GetString();Can we just store the KURL, similar to initial_url/name? Or even wait to remove the fragment identifier until the property is accessed, so that we don't need to scan the entire URL if not necessary?
resource->Options().initiator_info.initiator_url,It seems like this is the only place where you pass in a non-empty value for initiator_url. Do we need to pass it in, or can we just call FillInitiatorInfo() or just a setter when we need to?
Related details
- Guohui Deng
- Ian Clelland
- Noam Rosenthal
- Yoshisato Yanagisawa
Guohui Deng (Gerrit)
Guohui Deng added 2 comments![]( "Open in Gerrit")
UrlWithoutFragment(document->Url()).GetString();Can we just store the KURL, similar to initial_url/name? Or even wait to remove the fragment identifier until the property is accessed, so that we don't need to scan the entire URL if not necessary?
Yes, that's a great idea. I Will update accordingly in the patchset after I receive more feedback from yyanagisawa. (continued in the next comment).
resource->Options().initiator_info.initiator_url,It seems like this is the only place where you pass in a non-empty value for initiator_url. Do we need to pass it in, or can we just call FillInitiatorInfo() or just a setter when we need to?
Yes, I will do that(using a setter) in the next patchset.
I will do the following:
1) the url is stored in `KURL` before added to `ResourceTimingInfo`. It will be added using a setter.
2) I will not process the URL here. The sites can process it in a worker thread or on there server.
(FYI in my next step I plan to add a couple of more locations where the `initiator_url` is added to `ResourceTimingInfo`. I will follow the same rules stated above.)
Related details
- Ian Clelland
- Nidhi Jaju
- Noam Rosenthal
- Yoshisato Yanagisawa
Guohui Deng (Gerrit)
Guohui Deng added 1 comment![]( "Open in Gerrit")
@yyana...@chromium.org: Hi, would you please take a look once you have a chance? Thanks a lot.
Related details
- Ian Clelland
- Noam Rosenthal
- Yoshisato Yanagisawa
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa added 1 comment![]( "Open in Gerrit")
Do you have a CL to describe the final shape of the changes?
Or, is it possible to chain the CL to what happens next?
The newly added initiator_url fields usually looks to take "", which I suppose will be updated later.
Related details
- Guohui Deng
- Ian Clelland
- Noam Rosenthal
Guohui Deng (Gerrit)
Guohui Deng added 1 comment![]( "Open in Gerrit")
Do you have a CL to describe the final shape of the changes?
Or, is it possible to chain the CL to what happens next?
The newly added initiator_url fields usually looks to take "", which I suppose will be updated later.
There are only two spots where the `initiator_url` is expected to be updated later. They are `resource_load_observer_for_worker.cc` and `html_frame_owner_element.cc`.
The reason is that I plan to only track the network downloaded resources, and I may not track all of them.
The `resource_load_observer_for_worker.cc` is similar to this one. `html_frame_owner_element.cc` change can be see here:
https://chromium-review.googlesource.com/c/chromium/src/+/6357989/17/third_party/blink/renderer/core/html/html_frame_owner_element.cc
(It's a draft but it shows what I plan to do)
It would be much easier for me to review this one without making all the rest ready. I have a long way to go :) The next step is blocked by some work I need to do in `TaskAttribution`, which I am working on right now.
If you think we need all the rest of the CLs to be ready, I will do. Maybe I can make some CLs that are not complete?(with `TODO`s because we cannot decide on Javascript initiators)
Please let me know. Thanks!
Related details
- Ian Clelland
- Noam Rosenthal
- Yoshisato Yanagisawa
Guohui Deng (Gerrit)
Guohui Deng added 1 comment![]( "Open in Gerrit")
Guohui DengDo you have a CL to describe the final shape of the changes?
Or, is it possible to chain the CL to what happens next?
The newly added initiator_url fields usually looks to take "", which I suppose will be updated later.
There are only two spots where the `initiator_url` is expected to be updated later. They are `resource_load_observer_for_worker.cc` and `html_frame_owner_element.cc`.
The reason is that I plan to only track the network downloaded resources, and I may not track all of them.
The `resource_load_observer_for_worker.cc` is similar to this one. `html_frame_owner_element.cc` change can be see here:
https://chromium-review.googlesource.com/c/chromium/src/+/6357989/17/third_party/blink/renderer/core/html/html_frame_owner_element.cc
(It's a draft but it shows what I plan to do)It would be much easier for me to review this one without making all the rest ready. I have a long way to go :) The next step is blocked by some work I need to do in `TaskAttribution`, which I am working on right now.
If you think we need all the rest of the CLs to be ready, I will do. Maybe I can make some CLs that are not complete?(with `TODO`s because we cannot decide on Javascript initiators)
Please let me know. Thanks!
Actually, I think, I can do the following:
1) Make a separate file, somewhere under `/render/core`, in the file I implement`FillInitiatorInfo` functions (two versions, for resource vs page)
2) Use these functions in `/render/core/loader/resource_load_observer_for_frame.cc`, `/renderer/core/loader/resource_load_observer_for_worder.cc` and `/renderer/core/html/html_frame_owner_element.cc`
What do you think?
Additionally, what do you think that I rename `ResourceLoadObserver` to `ResourceLoadClient`?
Guohui Deng (Gerrit)
Guohui Deng added 1 comment![]( "Open in Gerrit")
I modified this CL according to Nidhi's suggestion; and there are no more places `CreateResourceTimingInfo` taking "" parameter. I am not sure if this is what you mean by it takes "" in many places, but they are gone.
I made a new CL that is chained after this one to illustrate what the final shape of this project would be. It's better than the old link I posted before that combines two steps.
This CL has been up for more than two months already, and I am eager to move this forward. Would you please take a look as soon as possible? Especially, can you advise on whether I can rename `ResourceLoadObserverForFrame`? This is a big change and it really needs someone who owns this folder to decide.
Thanks a lot.
Related details
- Ian Clelland
- Noam Rosenthal
- Yoshisato Yanagisawa
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nidhi Jaju (Gerrit)
Nidhi Jaju added 3 comments![]( "Open in Gerrit")
// TODO(guohu...@micrisoft.com): Consider consolidating the calculation.nit:
```suggestion
// TODO(guohu...@microsoft.com): Consider consolidating the calculation.
```
String initiator_url;I think we want to store an `AtomicString` here similar to `name`. This means that `PopulateAndResourceTimingInfo` would then just be setting the `AtomicString` on the mojo call, and `PerformanceResourceTiming::initiatorUrl()` wouldn't need to make the `AtomicString` every time it's called, it could just return the `AtomicString` directly.
resource->Options().initiator_info.initiator_url,Guohui DengIt seems like this is the only place where you pass in a non-empty value for initiator_url. Do we need to pass it in, or can we just call FillInitiatorInfo() or just a setter when we need to?
Yes, I will do that(using a setter) in the next patchset.
I will do the following:1) the url is stored in `KURL` before added to `ResourceTimingInfo`. It will be added using a setter.
2) I will not process the URL here. The sites can process it in a worker thread or on there server.(FYI in my next step I plan to add a couple of more locations where the `initiator_url` is added to `ResourceTimingInfo`. I will follow the same rules stated above.)
I will do the following:
> 1) the url is stored in KURL before added to ResourceTimingInfo. It will be added using a setter.
2) I will not process the URL here. The sites can process it in a worker thread or on there server.
- Guohui Deng
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa added 2 comments![]( "Open in Gerrit")
Guohui DengDo you have a CL to describe the final shape of the changes?
Or, is it possible to chain the CL to what happens next?
The newly added initiator_url fields usually looks to take "", which I suppose will be updated later.
Guohui DengThere are only two spots where the `initiator_url` is expected to be updated later. They are `resource_load_observer_for_worker.cc` and `html_frame_owner_element.cc`.
The reason is that I plan to only track the network downloaded resources, and I may not track all of them.
The `resource_load_observer_for_worker.cc` is similar to this one. `html_frame_owner_element.cc` change can be see here:
https://chromium-review.googlesource.com/c/chromium/src/+/6357989/17/third_party/blink/renderer/core/html/html_frame_owner_element.cc
(It's a draft but it shows what I plan to do)It would be much easier for me to review this one without making all the rest ready. I have a long way to go :) The next step is blocked by some work I need to do in `TaskAttribution`, which I am working on right now.
If you think we need all the rest of the CLs to be ready, I will do. Maybe I can make some CLs that are not complete?(with `TODO`s because we cannot decide on Javascript initiators)
Please let me know. Thanks!
Actually, I think, I can do the following:
1) Make a separate file, somewhere under `/render/core`, in the file I implement`FillInitiatorInfo` functions (two versions, for resource vs page)
2) Use these functions in `/render/core/loader/resource_load_observer_for_frame.cc`, `/renderer/core/loader/resource_load_observer_for_worder.cc` and `/renderer/core/html/html_frame_owner_element.cc`
What do you think?
Additionally, what do you think that I rename `ResourceLoadObserver` to `ResourceLoadClient`?
Sorry for the slow reply, it actually does not need to be a chain of CLs. However, I hope to see the design doc that explains the final picture.
I just followed the CLs you linked in the CL description and found https://docs.google.com/document/d/1ODMUQP9ua-0plxe0XhDds6aPCe_paZS6Cz1h1wdYiKU/edit?tab=t.0. It explains how you are going to change Chromium, but I feel details different from what you are doing with this CL and https://github.com/MicrosoftEdge/MSEdgeExplainers/blob/main/ResourceTimingInitiatorInfo/explainer.md.
Is it possible for you to update the design doc or create a new design doc to tell the full picture?
If you need to rename a class name, I hope to know the rationale. People who will read the code in the future, who can sometimes be ourselves, should want to understand why we did this.
I feel guilty on pushing back after making you wait long, but my LGTM brings certain size of approval in the code, and I want to do that with confidence.
// TODO(guohu...@micrisoft.com): Consider consolidating the calculation.nit:
```suggestion
// TODO(guohu...@microsoft.com): Consider consolidating the calculation.
```
I know old Chromium code still use TODO(somebody@name) style but currently TODO(crbug.com/xxx) is recommended because:
1. we can write more context in the crbug.
2. easy to delegate the task to somebody.
3. easy to discuss on the issue with the dedicated ID.
Will you file a new crbug and use it (or linked to the existing crbug)?
Related details
- Guohui Deng
- Ian Clelland
- Noam Rosenthal
Guohui Deng (Gerrit)
Guohui Deng added 5 comments![]( "Open in Gerrit")
@yyana...@chromium.org: No worries! I understand that reviewers can have too many incoming requests and other tasks at the same time. I appreciate your input and that we are moving this forward! :)
In my commit message I linked to Yosh's CLs but I was just giving credits to where it's due. Yosh did a lot of hard work that provided some very valuable information. However, after I took over this project, I decided to take a very different approach. So Yosh's design doc is not quite relevant now.
I updated my commit message in patchset(13) and you can see the link to the new design doc I just wrote. In "Implement plan" section I described what needs to be done. Most importantly, in the length A)2. section, I described why I would like to rename the class `ResourceLoadObserver`.
// TODO(guohu...@micrisoft.com): Consider consolidating the calculation.Yoshisato Yanagisawanit:
```suggestion
// TODO(guohu...@microsoft.com): Consider consolidating the calculation.
```
I know old Chromium code still use TODO(somebody@name) style but currently TODO(crbug.com/xxx) is recommended because:
1. we can write more context in the crbug.
2. easy to delegate the task to somebody.
3. easy to discuss on the issue with the dedicated ID.Will you file a new crbug and use it (or linked to the existing crbug)?
I created a new issue on crbug.com and used it instead of my account. That removed the misspell too. Thanks :)
Guohui DengCan we just store the KURL, similar to initial_url/name? Or even wait to remove the fragment identifier until the property is accessed, so that we don't need to scan the entire URL if not necessary?
Yes, that's a great idea. I Will update accordingly in the patchset after I receive more feedback from yyanagisawa. (continued in the next comment).
Done. I removed the "UrlWithoutFragment" and saved the string in an `AtomicString`.
I think we want to store an `AtomicString` here similar to `name`. This means that `PopulateAndResourceTimingInfo` would then just be setting the `AtomicString` on the mojo call, and `PerformanceResourceTiming::initiatorUrl()` wouldn't need to make the `AtomicString` every time it's called, it could just return the `AtomicString` directly.
100% and done.
There are more implications from this change. Please see the response from the other comment for details.
resource->Options().initiator_info.initiator_url,Guohui DengIt seems like this is the only place where you pass in a non-empty value for initiator_url. Do we need to pass it in, or can we just call FillInitiatorInfo() or just a setter when we need to?
Nidhi JajuYes, I will do that(using a setter) in the next patchset.
I will do the following:1) the url is stored in `KURL` before added to `ResourceTimingInfo`. It will be added using a setter.
2) I will not process the URL here. The sites can process it in a worker thread or on there server.(FYI in my next step I plan to add a couple of more locations where the `initiator_url` is added to `ResourceTimingInfo`. I will follow the same rules stated above.)
I will do the following:
> 1) the url is stored in KURL before added to ResourceTimingInfo. It will be added using a setter.
2) I will not process the URL here. The sites can process it in a worker thread or on there server.Do you plan to do this?
Now the latest patch(13) completed this improvement.
I need to correct myself, that the "ulr" was not stored in KURL, but it was implicitly converted to String when it was assigned to a String. I shouldn't have .GetString() in the early patch. However, now the `initiator_url` in `FetchInitiatorInfo` is no longer a String, but a `AtomicString`, so I will still need to call `GetString()` such that the `KURL` is converted to String then a `AtomisString`. And `UrlWithoutFragment` is completely removed.
Ah and I used a setter instead of adding a new parameter in `CreateResourceTimingInfo`
Related details
- Ian Clelland
- Nidhi Jaju
- Noam Rosenthal
- Yoshisato Yanagisawa
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nidhi Jaju (Gerrit)
Nidhi Jaju added 2 comments![]( "Open in Gerrit")
// TODO(guohu...@micrisoft.com): Consider consolidating the calculation.Yoshisato Yanagisawanit:
```suggestion
// TODO(guohu...@microsoft.com): Consider consolidating the calculation.
```
Guohui DengI know old Chromium code still use TODO(somebody@name) style but currently TODO(crbug.com/xxx) is recommended because:
1. we can write more context in the crbug.
2. easy to delegate the task to somebody.
3. easy to discuss on the issue with the dedicated ID.Will you file a new crbug and use it (or linked to the existing crbug)?
I created a new issue on crbug.com and used it instead of my account. That removed the misspell too. Thanks :)
Can you keep the rest of the comment for what this TODO is about? Also, please use the same TODO(crbug.com/xxxxxxxx) style in other places as well instead of your email.
```suggestion
// TODO(crbug.com/422626353): Consider consolidating the calculation.
```
resource->Options().initiator_info.initiator_url,Guohui DengIt seems like this is the only place where you pass in a non-empty value for initiator_url. Do we need to pass it in, or can we just call FillInitiatorInfo() or just a setter when we need to?
Nidhi JajuYes, I will do that(using a setter) in the next patchset.
I will do the following:1) the url is stored in `KURL` before added to `ResourceTimingInfo`. It will be added using a setter.
2) I will not process the URL here. The sites can process it in a worker thread or on there server.(FYI in my next step I plan to add a couple of more locations where the `initiator_url` is added to `ResourceTimingInfo`. I will follow the same rules stated above.)
Guohui DengI will do the following:
> 1) the url is stored in KURL before added to ResourceTimingInfo. It will be added using a setter.
2) I will not process the URL here. The sites can process it in a worker thread or on there server.Do you plan to do this?
Now the latest patch(13) completed this improvement.
I need to correct myself, that the "ulr" was not stored in KURL, but it was implicitly converted to String when it was assigned to a String. I shouldn't have .GetString() in the early patch. However, now the `initiator_url` in `FetchInitiatorInfo` is no longer a String, but a `AtomicString`, so I will still need to call `GetString()` such that the `KURL` is converted to String then a `AtomisString`. And `UrlWithoutFragment` is completely removed.
Ah and I used a setter instead of adding a new parameter in `CreateResourceTimingInfo`
- Guohui Deng
- Ian Clelland
- Noam Rosenthal
- Yoshisato Yanagisawa
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa added 1 comment![]( "Open in Gerrit")
Thanks. I left some comments there.
I still concerns that allowing ResourceLoadObserver to modify Resource itself is footgun. Do you have any ideas to avoid that? Or, is it so cumbersome to provide yet another channel to avoid modification?
Anyway, let's continue the discussion in the doc.
Related details
- Guohui Deng
- Ian Clelland
- Noam Rosenthal
Guohui Deng (Gerrit)
Guohui Deng added 2 comments![]( "Open in Gerrit")
Hi, yes there is an alternative and I am leaning towards to calling the class "client". I responded in the doc. Let's continue the discussion in the doc. :)
// TODO(guohu...@micrisoft.com): Consider consolidating the calculation.Yoshisato Yanagisawanit:
```suggestion
// TODO(guohu...@microsoft.com): Consider consolidating the calculation.
```
Guohui DengI know old Chromium code still use TODO(somebody@name) style but currently TODO(crbug.com/xxx) is recommended because:
1. we can write more context in the crbug.
2. easy to delegate the task to somebody.
3. easy to discuss on the issue with the dedicated ID.Will you file a new crbug and use it (or linked to the existing crbug)?
Nidhi JajuI created a new issue on crbug.com and used it instead of my account. That removed the misspell too. Thanks :)
Can you keep the rest of the comment for what this TODO is about? Also, please use the same TODO(crbug.com/xxxxxxxx) style in other places as well instead of your email.
```suggestion
// TODO(crbug.com/422626353): Consider consolidating the calculation.
```
Done. Sorry I missed these, I didn't realize I can use the (crbug.com/xxx) style with extra text. :)
Related details
- Ian Clelland
- Nidhi Jaju
- Noam Rosenthal
- Yoshisato Yanagisawa
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa added 2 comments![]( "Open in Gerrit")
Thanks kouhei@ for the suggestion.
I still have a concern on allowing ResourceLoadObserver to modify the Resource.
At the same time, it might not be good to block all your CL by that.
Is it possible to revise this CL without the ResourceLoadObserver modification, and put ResourceLoadObserver changes in the other CL? I mean keeps the changes in resource_fetcher as-is, but remove changes to ResourceObserver from this CL.
I hope to discuss the following part in the other CL.
> But, the logic that calculates the “initiator_url” depends on code in
third_party/blink/renderer/core/, which depends on “third_party/blink/renderer/platform/”
So we cannot calculate “initiator_url” in “third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc”
// Todo(guohuideng@microsoft): Refactor and consolidate FetchInitiatorInfo.Will you use TODO(crbug/<id>) instead of TODO(someboyd's email)?
Related details
- Guohui Deng
- Ian Clelland
- Nidhi Jaju
- Noam Rosenthal
Guohui Deng (Gerrit)
Guohui Deng added 2 comments![]( "Open in Gerrit")
Thanks kouhei@ for the suggestion.
I still have a concern on allowing ResourceLoadObserver to modify the Resource.
At the same time, it might not be good to block all your CL by that.Is it possible to revise this CL without the ResourceLoadObserver modification, and put ResourceLoadObserver changes in the other CL? I mean keeps the changes in resource_fetcher as-is, but remove changes to ResourceObserver from this CL.
I hope to discuss the following part in the other CL.
> But, the logic that calculates the “initiator_url” depends on code in
third_party/blink/renderer/core/, which depends on “third_party/blink/renderer/platform/”
So we cannot calculate “initiator_url” in “third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc”
It sounds like you plan to let this CL merge with the `ResourceLoadObserver` being still called an observer? Thanks for the flexibility and I will work with you on the follow-up work to get it properly fixed.
This CL doesn't change `ResourceLoadObserver` except for removing one `const` so that the method can modify the Resource.
I made a separate CL that renames `ResourceloadObserver`. It's here
https://chromium-review.googlesource.com/c/chromium/src/+/6639155
(It's chained behind this CL as well). And I am open to alternative too. There are pro and cons for each.
And please let me know what else I will need to do to move this CL forward.
Cheers,
Guohui
// Todo(guohuideng@microsoft): Refactor and consolidate FetchInitiatorInfo.Will you use TODO(crbug/<id>) instead of TODO(someboyd's email)?
- Ian Clelland
- Nidhi Jaju
- Noam Rosenthal
- Yoshisato Yanagisawa
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa added 1 comment![]( "Open in Gerrit")
Guohui DengThanks kouhei@ for the suggestion.
I still have a concern on allowing ResourceLoadObserver to modify the Resource.
At the same time, it might not be good to block all your CL by that.Is it possible to revise this CL without the ResourceLoadObserver modification, and put ResourceLoadObserver changes in the other CL? I mean keeps the changes in resource_fetcher as-is, but remove changes to ResourceObserver from this CL.
I hope to discuss the following part in the other CL.
> But, the logic that calculates the “initiator_url” depends on code in
third_party/blink/renderer/core/, which depends on “third_party/blink/renderer/platform/”
So we cannot calculate “initiator_url” in “third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc”
It sounds like you plan to let this CL merge with the `ResourceLoadObserver` being still called an observer? Thanks for the flexibility and I will work with you on the follow-up work to get it properly fixed.
This CL doesn't change `ResourceLoadObserver` except for removing one `const` so that the method can modify the Resource.
I made a separate CL that renames `ResourceloadObserver`. It's here
https://chromium-review.googlesource.com/c/chromium/src/+/6639155
(It's chained behind this CL as well). And I am open to alternative too. There are pro and cons for each.And please let me know what else I will need to do to move this CL forward.
Cheers,
Guohui
The point I (and maybe Noam) do not agree is making `ResourceLoadObserver` to modify the Resource structure. However, we do not have argument other than that.
I think it reasonable:
1. to make FetchInitiatorInfo to have initiator URL,
2. to make ResourceFetcher to set the initiator URL,
3. and to make ResourceTiming or PerformanceResourceTiming to have the initiator URL.
However, we do not agree to make `ResourceLoadObserver` to modify `Resource`. It is not a naming issue but a design issue.
I suggest removing the `ResourceLoadObserver` changes from this CL, while keeping what I think reasonable (explained above).
Related details
- Guohui Deng
- Ian Clelland
- Nidhi Jaju
- Noam Rosenthal
Nidhi Jaju (Gerrit)
Nidhi Jaju added 2 comments![]( "Open in Gerrit")
// TODO(crbug.com/422626353)Can you add the rest of the comment, of what this TODO is for?
if (RuntimeEnabledFeatures::ResourceTimingInitiatorEnabled()) {
FillInitiatorInfo(document_, resource->MutableOptions().initiator_info);
}I think what Yanagisawa-san was suggesting is removing this part of the change from this CL, and using a follow-up CL to pursue an alternate design i.e. using FrameFetchContext instead of ResourceLoadObserver to fill out the initiator info.
Related details
- Guohui Deng
- Ian Clelland
- Noam Rosenthal
Guohui Deng (Gerrit)
Guohui Deng added 1 comment![]( "Open in Gerrit")
if (RuntimeEnabledFeatures::ResourceTimingInitiatorEnabled()) {
FillInitiatorInfo(document_, resource->MutableOptions().initiator_info);
}I think what Yanagisawa-san was suggesting is removing this part of the change from this CL, and using a follow-up CL to pursue an alternate design i.e. using FrameFetchContext instead of ResourceLoadObserver to fill out the initiator info.
Thank you for pointing me to the right direction! `FetchContext` is a perfect place for me to put the `FillInitiatorUrl` and it nicely works for both `Frame` and `Worker`(which I will take care of soon). This is what I have been looking for and without it I cannot merge this CL. (I once saw `FetchContext` in the past, but I really missed it :))
I will upload a new patchset before the end of this week. Due to time zone difference, I think you guys are already on Friday. You guys have a great weekend and really appreciate the guidance!
Related details
- Ian Clelland
- Nidhi Jaju
- Noam Rosenthal
Guohui Deng (Gerrit)
Guohui Deng added 4 comments![]( "Open in Gerrit")
Following Nidhi's advice, I moved the `FillInitiatorInfo` function to `FetchContext` class. The `FillInitiatorInfo` function takes a parameter of `FetchInitiatorInfo`, and that's the only data open to write outside `platform/load`.
I think we solved the problem. Really appreciate the patience and guidance I receive from everyone here.
Now the "observer" problem is solved. This CL is ready for review again. Thanks!
Can you add the rest of the comment, of what this TODO is for?
I moved this to `third_party/blink/renderer/core/loader/frame_fetch_context.cc` and I fixed the comment there, including what this TODO is for.
if (RuntimeEnabledFeatures::ResourceTimingInitiatorEnabled()) {
FillInitiatorInfo(document_, resource->MutableOptions().initiator_info);
}Guohui DengI think what Yanagisawa-san was suggesting is removing this part of the change from this CL, and using a follow-up CL to pursue an alternate design i.e. using FrameFetchContext instead of ResourceLoadObserver to fill out the initiator info.
Thank you for pointing me to the right direction! `FetchContext` is a perfect place for me to put the `FillInitiatorUrl` and it nicely works for both `Frame` and `Worker`(which I will take care of soon). This is what I have been looking for and without it I cannot merge this CL. (I once saw `FetchContext` in the past, but I really missed it :))
I will upload a new patchset before the end of this week. Due to time zone difference, I think you guys are already on Friday. You guys have a great weekend and really appreciate the guidance!
Ah, the new patchset is ready now despite the gerrit outrage today :)
Related details
- Ian Clelland
- Nidhi Jaju
- Noam Rosenthal
- Yoshisato Yanagisawa
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa voted and added 2 comments![]( "Open in Gerrit")
Votes added by Yoshisato Yanagisawa
| Code-Review | +1 |
2 comments
if (RuntimeEnabledFeatures::ResourceTimingInitiatorEnabled()) {
FillInitiatorInfo(document_, resource->MutableOptions().initiator_info);
}Guohui DengI think what Yanagisawa-san was suggesting is removing this part of the change from this CL, and using a follow-up CL to pursue an alternate design i.e. using FrameFetchContext instead of ResourceLoadObserver to fill out the initiator info.
Guohui DengThank you for pointing me to the right direction! `FetchContext` is a perfect place for me to put the `FillInitiatorUrl` and it nicely works for both `Frame` and `Worker`(which I will take care of soon). This is what I have been looking for and without it I cannot merge this CL. (I once saw `FetchContext` in the past, but I really missed it :))
I will upload a new patchset before the end of this week. Due to time zone difference, I think you guys are already on Friday. You guys have a great weekend and really appreciate the guidance!
Ah, the new patchset is ready now despite the gerrit outrage today :)
- Guohui Deng
- Ian Clelland
- Nidhi Jaju
- Noam Rosenthal
Code-Coverage
Code-Owners
Noam Rosenthal (Gerrit)
Noam Rosenthal voted and added 1 comment![]( "Open in Gerrit")
Votes added by Noam Rosenthal
| Code-Review | +1 |
1 comment
Nice work, LGTM!
Related details
- Guohui Deng
- Ian Clelland
- Nidhi Jaju
Code-Coverage
Code-Review
Nidhi Jaju (Gerrit)
Nidhi Jaju voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nidhi Jaju
| Code-Review | +1 |
1 comment
lgtm, thanks
Related details
- Guohui Deng
- Ian Clelland
Code-Coverage
Code-Owners
Guohui Deng (Gerrit)
Guohui Deng added 2 comments![]( "Open in Gerrit")
I am closing this comment since we solved the observer problem.
Resource* resource) {Guohui DengHmm not sure about this. I don't know about the design of filling the initiator info in an "observer". It seems surprising because obsrver usually don't change what they observe.
Guohui DengIt's indeed not common.
The logic is here because it is not possible to implement it in platform/loader. The logic satisfies all the conditions listed here:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/loader/fetch/resource_load_observer.h;l=28;drc=c76e4f83a8c5786b463c3e55c070a21ac751b96bThere is actually a precedence in `ResourceLoadObserverForFrame` that the observer modifies the data it receives.
Notice the ` (... BlobDataHandle* blob) ` isn't "const" in `DidDownloadToBlob(...)`
(https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/resource_load_observer_for_frame.cc;l=286)
Eventually, the ownership of `blob` is transferred to a collection of inspector agents, see:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/inspector/inspector_network_agent.cc;l=1663;drc=c76e4f83a8c5786b463c3e55c070a21ac751b96b;bpv=1;bpt=1I think the most important reason for `ResourceLoadObserver` is that it's a place where we can add react logic to individual "load" that cannot happen in platform/loader folder. Modification to the observed data should be allowed here, although it's not common.
The main problem here is actually the name "observer", rather than the design that code in `platform/loader` calls into `core/loader` for something that cannot be in `platform/loader`. The alternative is to rename "observer" to something else. But then someone may object, reasoning that observer can modify things occasionally if it has to, and the name "observer" is suitable except for that we don't modify the data usually.
What do you think?
Guohui DengFor this CL, I think I can avoid this problem by making a separate callback/call_back_class that's explicitly named `InitiatorInfoCalculator`.(Which is parallel to this `ResourceLoadObserverForFrame` class). In future if we have more callbacks that modify the observed data, we can have a container class called `ResourceLoadCallbacksForFrame`.
But I don't quite like it because it's very verbose and it's duplicate to this `ResourceLoadObserverForFrame` class.
Alternatively, we can put all the callbacks in the same container(i.e., the current `ResourceLoadObserverForFrame`). Because some modifies the observed data, we don't call this container "observer". So we could rename this class `ResourceLoadObserverForFrame` to `ResourceLoadCallbacksForFrame`. This approach is simpler, but it's a big change. I don't know if someone would object.
Would appreciate any thoughts shared! Thanks!
Sorry I cannot edit my previous comment. Let me make some corrections and clarifications. Below are two options that I am considering:
Option 1. Rename `ResourceLoadObserver' class to `ResourceLoadClient`
Option 2. Use a partially bound `RepeatingCallback` named `fill_initiator_info_cb_`. This callback is set from `core/loader` and called in `platform/loader`1) has larger footprint, while 2) is a little hacky.
I am closing this comment since we solved the observer problem.
Related details
- Chromium IPC Reviews
- Ian Clelland
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: mea...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): mea...@chromium.org
Reviewer source(s):
mea...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Ian Clelland
- Mustafa Emre Acer
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mustafa Emre Acer (Gerrit)
Mustafa Emre Acer added 1 comment![]( "Open in Gerrit")
string initiator_url;Can this be a url.mojom.Url instead?
Related details
- Guohui Deng
- Ian Clelland
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Guohui Deng (Gerrit)
Guohui Deng added 1 comment![]( "Open in Gerrit")
string initiator_url;Can this be a url.mojom.Url instead?
Ah I see `url.mojom.Url` is commonly used in mojom. In this case, I still prefer using string.
From `FetchInitiatorInfo`, it's already calculated to be an `AtomicString`. The rest of the work is to transport it to `ResourceTiming` to report it. Using `url.mojom.Url` will convert the string to `KURL` and it involves some processing that's not necessary.
What do you think? Or should I rename this field to `initiator_url_string` to make it clear that the value is a string and it will never return as a URL?
Related details
- Ian Clelland
- Mustafa Emre Acer
Mustafa Emre Acer (Gerrit)
Mustafa Emre Acer added 1 comment![]( "Open in Gerrit")
string initiator_url;Guohui DengCan this be a url.mojom.Url instead?
Ah I see `url.mojom.Url` is commonly used in mojom. In this case, I still prefer using string.
From `FetchInitiatorInfo`, it's already calculated to be an `AtomicString`. The rest of the work is to transport it to `ResourceTiming` to report it. Using `url.mojom.Url` will convert the string to `KURL` and it involves some processing that's not necessary.
What do you think? Or should I rename this field to `initiator_url_string` to make it clear that the value is a string and it will never return as a URL?
FillInitiatorInfo seems to be setting it from document_->Url() which I assume is a KURL, right? That conversion should be automatic.
I understand this isn't security sensitive, but I'd still prefer the actual mojo type here (unless of course using the mojo type blows up the CL complexity)
Related details
- Guohui Deng
- Ian Clelland
Guohui Deng (Gerrit)
Guohui Deng added 1 comment![]( "Open in Gerrit")
string initiator_url;Guohui DengCan this be a url.mojom.Url instead?
Mustafa Emre AcerAh I see `url.mojom.Url` is commonly used in mojom. In this case, I still prefer using string.
From `FetchInitiatorInfo`, it's already calculated to be an `AtomicString`. The rest of the work is to transport it to `ResourceTiming` to report it. Using `url.mojom.Url` will convert the string to `KURL` and it involves some processing that's not necessary.
What do you think? Or should I rename this field to `initiator_url_string` to make it clear that the value is a string and it will never return as a URL?
FillInitiatorInfo seems to be setting it from document_->Url() which I assume is a KURL, right? That conversion should be automatic.
I understand this isn't security sensitive, but I'd still prefer the actual mojo type here (unless of course using the mojo type blows up the CL complexity)
`FillInitiatorInfo` stores an `AtomicString`. The data is converted from KURL -> String -> AtomicString. The result is an `AtomicString` (see third_party/blink/renderer/platform/loader/fetch/fetch_initiator_info.h)
It's a field of `PerformanceResourceTiming`, where these values are typically reported as strings and they default to empty strings if the information is not available (which is very common). So, it's much simpler we convert it to `String` early and process it like the other fields.
My understanding is that `url.mojom.Url` would result in a construction of KURL from a string which is time consuming.
Related details
- Ian Clelland
- Mustafa Emre Acer
Mustafa Emre Acer (Gerrit)
Mustafa Emre Acer added 1 comment![]( "Open in Gerrit")
string initiator_url;Guohui DengCan this be a url.mojom.Url instead?
Mustafa Emre AcerAh I see `url.mojom.Url` is commonly used in mojom. In this case, I still prefer using string.
From `FetchInitiatorInfo`, it's already calculated to be an `AtomicString`. The rest of the work is to transport it to `ResourceTiming` to report it. Using `url.mojom.Url` will convert the string to `KURL` and it involves some processing that's not necessary.
What do you think? Or should I rename this field to `initiator_url_string` to make it clear that the value is a string and it will never return as a URL?
Guohui DengFillInitiatorInfo seems to be setting it from document_->Url() which I assume is a KURL, right? That conversion should be automatic.
I understand this isn't security sensitive, but I'd still prefer the actual mojo type here (unless of course using the mojo type blows up the CL complexity)
`FillInitiatorInfo` stores an `AtomicString`. The data is converted from KURL -> String -> AtomicString. The result is an `AtomicString` (see third_party/blink/renderer/platform/loader/fetch/fetch_initiator_info.h)
It's a field of `PerformanceResourceTiming`, where these values are typically reported as strings and they default to empty strings if the information is not available (which is very common). So, it's much simpler we convert it to `String` early and process it like the other fields.
My understanding is that `url.mojom.Url` would result in a construction of KURL from a string which is time consuming.
Thanks for the explanation. In that case, changing this to initiator_url_string makes sense. Alternatively, you can add a short comment to the field saying that it's only for reporting and not used to make a security decision.
Related details
- Guohui Deng
- Ian Clelland
Nidhi Jaju (Gerrit)
Nidhi Jaju voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nidhi Jaju
| Code-Review | +0 |
1 comment
string initiator_url;Guohui DengCan this be a url.mojom.Url instead?
Mustafa Emre AcerAh I see `url.mojom.Url` is commonly used in mojom. In this case, I still prefer using string.
From `FetchInitiatorInfo`, it's already calculated to be an `AtomicString`. The rest of the work is to transport it to `ResourceTiming` to report it. Using `url.mojom.Url` will convert the string to `KURL` and it involves some processing that's not necessary.
What do you think? Or should I rename this field to `initiator_url_string` to make it clear that the value is a string and it will never return as a URL?
Guohui DengFillInitiatorInfo seems to be setting it from document_->Url() which I assume is a KURL, right? That conversion should be automatic.
I understand this isn't security sensitive, but I'd still prefer the actual mojo type here (unless of course using the mojo type blows up the CL complexity)
Mustafa Emre Acer`FillInitiatorInfo` stores an `AtomicString`. The data is converted from KURL -> String -> AtomicString. The result is an `AtomicString` (see third_party/blink/renderer/platform/loader/fetch/fetch_initiator_info.h)
It's a field of `PerformanceResourceTiming`, where these values are typically reported as strings and they default to empty strings if the information is not available (which is very common). So, it's much simpler we convert it to `String` early and process it like the other fields.
My understanding is that `url.mojom.Url` would result in a construction of KURL from a string which is time consuming.
Thanks for the explanation. In that case, changing this to initiator_url_string makes sense. Alternatively, you can add a short comment to the field saying that it's only for reporting and not used to make a security decision.
Taking another look, it seems that we only ever set `initiator_url` from a KURL and could use url.mojom.Url, and hence KURL, throughout to avoid processing and fetching the AtomicString from the KURL in the first place. We can use the empty KURL() for cases where there's no URL present.
Guohui Deng (Gerrit)
Guohui Deng added 1 comment![]( "Open in Gerrit")
Guohui DengCan this be a url.mojom.Url instead?
Mustafa Emre AcerAh I see `url.mojom.Url` is commonly used in mojom. In this case, I still prefer using string.
From `FetchInitiatorInfo`, it's already calculated to be an `AtomicString`. The rest of the work is to transport it to `ResourceTiming` to report it. Using `url.mojom.Url` will convert the string to `KURL` and it involves some processing that's not necessary.
What do you think? Or should I rename this field to `initiator_url_string` to make it clear that the value is a string and it will never return as a URL?
Guohui DengFillInitiatorInfo seems to be setting it from document_->Url() which I assume is a KURL, right? That conversion should be automatic.
I understand this isn't security sensitive, but I'd still prefer the actual mojo type here (unless of course using the mojo type blows up the CL complexity)
Mustafa Emre Acer`FillInitiatorInfo` stores an `AtomicString`. The data is converted from KURL -> String -> AtomicString. The result is an `AtomicString` (see third_party/blink/renderer/platform/loader/fetch/fetch_initiator_info.h)
It's a field of `PerformanceResourceTiming`, where these values are typically reported as strings and they default to empty strings if the information is not available (which is very common). So, it's much simpler we convert it to `String` early and process it like the other fields.
My understanding is that `url.mojom.Url` would result in a construction of KURL from a string which is time consuming.
Nidhi JajuThanks for the explanation. In that case, changing this to initiator_url_string makes sense. Alternatively, you can add a short comment to the field saying that it's only for reporting and not used to make a security decision.
Taking another look, it seems that we only ever set `initiator_url` from a KURL and could use url.mojom.Url, and hence KURL, throughout to avoid processing and fetching the AtomicString from the KURL in the first place. We can use the empty KURL() for cases where there's no URL present.
Done. I see why it's faster now. It doesn't really construct a KURL at all(I missed that). And when copying the KURL, `AtomicString`s are copied, it's faster than converting `AtomicString` to `String` then back to `AtomicString`.
Thanks a lot and I learnt more today. :)
Related details
- Ian Clelland
- Mustafa Emre Acer
- Nidhi Jaju
- Noam Rosenthal
- Yoshisato Yanagisawa
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guohui Deng (Gerrit)
Guohui Deng added 1 comment![]( "Open in Gerrit")
string initiator_url;Guohui DengCan this be a url.mojom.Url instead?
Mustafa Emre AcerAh I see `url.mojom.Url` is commonly used in mojom. In this case, I still prefer using string.
From `FetchInitiatorInfo`, it's already calculated to be an `AtomicString`. The rest of the work is to transport it to `ResourceTiming` to report it. Using `url.mojom.Url` will convert the string to `KURL` and it involves some processing that's not necessary.
What do you think? Or should I rename this field to `initiator_url_string` to make it clear that the value is a string and it will never return as a URL?
Guohui DengFillInitiatorInfo seems to be setting it from document_->Url() which I assume is a KURL, right? That conversion should be automatic.
I understand this isn't security sensitive, but I'd still prefer the actual mojo type here (unless of course using the mojo type blows up the CL complexity)
Mustafa Emre Acer`FillInitiatorInfo` stores an `AtomicString`. The data is converted from KURL -> String -> AtomicString. The result is an `AtomicString` (see third_party/blink/renderer/platform/loader/fetch/fetch_initiator_info.h)
It's a field of `PerformanceResourceTiming`, where these values are typically reported as strings and they default to empty strings if the information is not available (which is very common). So, it's much simpler we convert it to `String` early and process it like the other fields.
My understanding is that `url.mojom.Url` would result in a construction of KURL from a string which is time consuming.
Nidhi JajuThanks for the explanation. In that case, changing this to initiator_url_string makes sense. Alternatively, you can add a short comment to the field saying that it's only for reporting and not used to make a security decision.
Guohui DengTaking another look, it seems that we only ever set `initiator_url` from a KURL and could use url.mojom.Url, and hence KURL, throughout to avoid processing and fetching the AtomicString from the KURL in the first place. We can use the empty KURL() for cases where there's no URL present.
Done. I see why it's faster now. It doesn't really construct a KURL at all(I missed that). And when copying the KURL, `AtomicString`s are copied, it's faster than converting `AtomicString` to `String` then back to `AtomicString`.
Thanks a lot and I learnt more today. :)
I was wrong :( The new method copies a couple more fields. And that's minimal.
Mustafa Emre Acer (Gerrit)
Mustafa Emre Acer voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mustafa Emre Acer
| Code-Review | +1 |
- Guohui Deng
- Ian Clelland
Nidhi Jaju (Gerrit)
Nidhi Jaju voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nidhi Jaju
| Code-Review | +1 |
1 comment
- Guohui Deng
- Ian Clelland
- Noam Rosenthal
- Yoshisato Yanagisawa
Code-Coverage
Code-Review
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yoshisato Yanagisawa
| Code-Review | +1 |
1 comment
still lgtm
Related details
- Guohui Deng
- Ian Clelland
- Noam Rosenthal
Noam Rosenthal (Gerrit)
Noam Rosenthal voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Guohui Deng
- Ian Clelland
Guohui Deng (Gerrit)
Guohui Deng added 1 comment![]( "Open in Gerrit")
Thank you everyone for your time and guidance. I am submitting this CL now.
Related details
- Ian Clelland
Guohui Deng (Gerrit)
Guohui Deng voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Expose initiatorUrl in resourceTiming for html initiators
Unsolved: to find the initiator html for the subframe html.
Test cases are merged (with modifications) from:
https://chromium-review.googlesource.com/c/chromium/src/+/4812813
And For script initiators, TaskAttributionInfo is used
according to this draft CL:
https://chromium-review.googlesource.com/c/chromium/src/+/4931296/21
- M android_webview/test/data/web_tests/webexposed/global-interface-listing-expected.txt
- M third_party/blink/public/mojom/timing/resource_timing.mojom
- M third_party/blink/renderer/core/loader/cross_thread_resource_timing_info_copier.cc
- M third_party/blink/renderer/core/loader/frame_fetch_context.cc
- M third_party/blink/renderer/core/loader/frame_fetch_context.h
- M third_party/blink/renderer/core/timing/performance_resource_timing.cc
- M third_party/blink/renderer/core/timing/performance_resource_timing.h
- M third_party/blink/renderer/core/timing/performance_resource_timing.idl
- M third_party/blink/renderer/platform/loader/fetch/fetch_context.h
- M third_party/blink/renderer/platform/loader/fetch/fetch_initiator_info.h
- M third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- M third_party/blink/web_tests/TestExpectations
- M third_party/blink/web_tests/external/wpt/resource-timing/resources/test-initiator.js
- M third_party/blink/web_tests/external/wpt/resource-timing/tentative/document-initiated.html
- M third_party/blink/web_tests/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt
- D third_party/blink/web_tests/platform/mac/webexposed/global-interface-listing-platform-specific-expected.txt
- M third_party/blink/web_tests/webexposed/global-interface-listing-dedicated-worker-expected.txt
- M third_party/blink/web_tests/webexposed/global-interface-listing-expected.txt
- M third_party/blink/web_tests/webexposed/global-interface-listing-shared-worker-expected.txt
Code-Review: +1 by Mustafa Emre Acer, +1 by Noam Rosenthal, +1 by Yoshisato Yanagisawa, +1 by Nidhi Jaju
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/53250
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |