[EventTiming] Add targetIdentifier string in case Node is disconnected [chromium/src : main]
Michal Mocny (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Noam Rosenthal
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley added 4 comments![]( "Open in Gerrit")
[EventTiming] Add targetIdentifier string in case Node is disconnectedIs there an I2P, chromestatus entry, or explainer for this? I think it would be good to have something before landing code, since this will require blink-dev intents.
#include "third_party/blink/renderer/core/timing/timing_shared.h"nit/optional: timing_utils.h perhaps?
Code search for precedent in blink/renderer:
98 instances of "_utils.h": https://source.chromium.org/search?q=f:%22_utils.h%22&sq=&ss=chromium%2Fchromium%2Fsrc:third_party%2Fblink%2Frenderer%2F
0 of "_shared.h": https://source.chromium.org/search?q=f:%22_shared.h%22&sq=&ss=chromium%2Fchromium%2Fsrc:third_party%2Fblink%2Frenderer%2F
EventTarget* target,Is there ever a case where the EventTarget is not a Node? I would think that trusted input events would always target a node? Just wondering if this part needs to change, given Node IsA(n) EventTarget.
Related details
- Michal Mocny
- Noam Rosenthal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny added 3 comments![]( "Open in Gerrit")
[EventTiming] Add targetIdentifier string in case Node is disconnectedIs there an I2P, chromestatus entry, or explainer for this? I think it would be good to have something before landing code, since this will require blink-dev intents.
Not yet.
- Spec Feature request: https://github.com/w3c/event-timing/issues/126
- Spec wording PR (recent): https://github.com/w3c/event-timing/pull/160
After that, I will update to create a chromestatus entry.
#include "third_party/blink/renderer/core/timing/timing_shared.h"nit/optional: timing_utils.h perhaps?
Code search for precedent in blink/renderer:
98 instances of "_utils.h": https://source.chromium.org/search?q=f:%22_utils.h%22&sq=&ss=chromium%2Fchromium%2Fsrc:third_party%2Fblink%2Frenderer%2F
0 of "_shared.h": https://source.chromium.org/search?q=f:%22_shared.h%22&sq=&ss=chromium%2Fchromium%2Fsrc:third_party%2Fblink%2Frenderer%2F
Hash, yeah actually that was the initial name I used and it got renamed as part of some iteration I will move it.
Another option is to just put it into some existing shared header...
EventTarget* target,Is there ever a case where the EventTarget is not a Node? I would think that trusted input events would always target a node? Just wondering if this part needs to change, given Node IsA(n) EventTarget.
I don't actually know.
We are and continue to enforce that only EventTargets that are nodes are exposed as the target property, but since the loaf script invoker attribution works for targets that are not nodes, I figured I might as well offer the same support here.
Even if it doesn't ever do anything, i still think it cleans things up to not have the caller have to do the conversion. I think.
Related details
- Noam Rosenthal
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AI Code Reviewer (Gerrit)
AI Code Reviewer added 2 comments![]( "Open in Gerrit")
EventTimingReportingInfo repoerting_info,nit: There appears to be a typo in the parameter name `repoerting_info`; it should likely be `reporting_info`. (Blink Style Guide: Naming - Be Descriptive)
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
EventTimingReportingInfo reporting_info,Blink Style Guide: May leave obvious parameter names out of function declarations. Please consider omitting the names for parameters `reporting_info`, `target`, and `source` in this declaration, as their types are self-descriptive. Parameter names should be included in the implementation file.
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Related details
- Noam Rosenthal
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny added 2 comments![]( "Open in Gerrit")
nit: There appears to be a typo in the parameter name `repoerting_info`; it should likely be `reporting_info`. (Blink Style Guide: Naming - Be Descriptive)
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reasonThis comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Done
Blink Style Guide: May leave obvious parameter names out of function declarations. Please consider omitting the names for parameters `reporting_info`, `target`, and `source` in this declaration, as their types are self-descriptive. Parameter names should be included in the implementation file.
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reasonThis comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Done
Related details
- Noam Rosenthal
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny added 4 comments![]( "Open in Gerrit")
[EventTiming] Add targetIdentifier string in case Node is disconnectedMichal MocnyIs there an I2P, chromestatus entry, or explainer for this? I think it would be good to have something before landing code, since this will require blink-dev intents.
Not yet.
- Spec Feature request: https://github.com/w3c/event-timing/issues/126
- Spec wording PR (recent): https://github.com/w3c/event-timing/pull/160
After that, I will update to create a chromestatus entry.
Done, and updated the COMMIT message with links.
#include "third_party/blink/renderer/core/timing/timing_shared.h"Michal Mocnynit/optional: timing_utils.h perhaps?
Code search for precedent in blink/renderer:
98 instances of "_utils.h": https://source.chromium.org/search?q=f:%22_utils.h%22&sq=&ss=chromium%2Fchromium%2Fsrc:third_party%2Fblink%2Frenderer%2F
0 of "_shared.h": https://source.chromium.org/search?q=f:%22_shared.h%22&sq=&ss=chromium%2Fchromium%2Fsrc:third_party%2Fblink%2Frenderer%2F
Hash, yeah actually that was the initial name I used and it got renamed as part of some iteration I will move it.
Another option is to just put it into some existing shared header...
Done
Michal MocnyIs there ever a case where the EventTarget is not a Node? I would think that trusted input events would always target a node? Just wondering if this part needs to change, given Node IsA(n) EventTarget.
I don't actually know.
We are and continue to enforce that only EventTargets that are nodes are exposed as the target property, but since the loaf script invoker attribution works for targets that are not nodes, I figured I might as well offer the same support here.
Even if it doesn't ever do anything, i still think it cleans things up to not have the caller have to do the conversion. I think.
SetTarget will remain nullptr if target is not a Node, but will now get an identifier string (InterfaceName).
I think you are right that for Events which are dispatched against HitTest targets that this will always have a Node.
However, we do have a proposal to add more event types to Event Timing, for example interactions with browser UI that fire e.g. navigate/popstate events or otherwise. These would not have a Node target.
I think we can leave this to be future proof in this way?
Related details
- Noam Rosenthal
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley voted and added 4 comments![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
| Commit-Queue | +1 |
4 comments
LGTM, but I think you'll need to update webexposed/ (failures in earlier patchset). I'll kick off a dry run.
#include "third_party/blink/renderer/core/timing/timing_shared.h"Michal Mocnynit/optional: timing_utils.h perhaps?
Code search for precedent in blink/renderer:
98 instances of "_utils.h": https://source.chromium.org/search?q=f:%22_utils.h%22&sq=&ss=chromium%2Fchromium%2Fsrc:third_party%2Fblink%2Frenderer%2F
0 of "_shared.h": https://source.chromium.org/search?q=f:%22_shared.h%22&sq=&ss=chromium%2Fchromium%2Fsrc:third_party%2Fblink%2Frenderer%2F
Michal MocnyHash, yeah actually that was the initial name I used and it got renamed as part of some iteration I will move it.
Another option is to just put it into some existing shared header...
Done
Another option is to just put it into some existing shared header...
Possibly a static method on WindowPerformance, if it's tied to that and makes sense spec-wise? I'm fine either way.
EventTarget* target,Michal MocnyIs there ever a case where the EventTarget is not a Node? I would think that trusted input events would always target a node? Just wondering if this part needs to change, given Node IsA(n) EventTarget.
Michal MocnyI don't actually know.
We are and continue to enforce that only EventTargets that are nodes are exposed as the target property, but since the loaf script invoker attribution works for targets that are not nodes, I figured I might as well offer the same support here.
Even if it doesn't ever do anything, i still think it cleans things up to not have the caller have to do the conversion. I think.
SetTarget will remain nullptr if target is not a Node, but will now get an identifier string (InterfaceName).
I think you are right that for Events which are dispatched against HitTest targets that this will always have a Node.
However, we do have a proposal to add more event types to Event Timing, for example interactions with browser UI that fire e.g. navigate/popstate events or otherwise. These would not have a Node target.
I think we can leave this to be future proof in this way?
SGTM.
entry->SetTarget(event.RawTarget());This is outside the scope of this CL, but is it okay to use `RawTarget()` here? The comments for that say it isn't web-exposed, but the entry is exposing it. Looks like `event.target()` has additional checks for pseudo elements, which are missing in PerformanceEventTiming.
Related details
- Michal Mocny
- Noam Rosenthal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal added 1 comment![]( "Open in Gerrit")
builder.AddString("targetIdentifier", targetIdentifier());You probably want to flag-protect this
Related details
- Michal Mocny
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |