TaskAttribution: Instrument NavigateEvent.intercept() handlers [chromium/src : main]
Michal Mocny (Gerrit)
Michal Mocny voted and added 4 comments![]( "Open in Gerrit")
Votes added by Michal Mocny
| Code-Review | +1 |
4 comments
I know its still WIP, but endorsing this approach :D
synchronous events, we never want to overwrite the current staet, butstate
// Always propagate `task_state`, even if it's null, since the current state
// may not be null.it took me a bit to parse, but I think this is saying "this might be used to clear current task state", right?
// task state, regardless of whether JavaScript is running or not. Typically
// nested dispatch (e.g. events) uses the current state, but in rare cases
// this needs to be overridden, such as with the navigation API.Patch LGTM but this detail I need to understand better some (future) day.
Related details
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
void SetTaskState(scheduler::TaskAttributionInfo* task,Blink Style Guide: Prefer enums or StrongAliases to bare bools for function parameters. The boolean parameter 'should_propagate_ignoring_top_level' is not obvious from the function name 'SetTaskState'. Consider using an enum or base::StrongAlias to improve readability at call sites.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: 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
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny voted and added 4 comments![]( "Open in Gerrit")
Votes added by Michal Mocny
| Code-Review | +1 |
4 comments
[SupportsTaskAttribution]Oh nice, I didn't know we have this.
Q: Would it be possible to make one of these (decorators?) for the PaintTimingMixin?
using PassKeyType = base::PassKey<NavigateEvent>;This is public-- is this a temporary and eventually will be private + friend list? Or is this sorta just a way to mark "be extra careful"?
// An RAII scope that forces Task Attribution to ingore the top-levelPlease fix this WARNING reported by Spellchecker: "ingore" is a possible misspelling of "ignore".
To bypass Spellchecker, add a f...
"ingore" is a possible misspelling of "ignore".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
// Always propagate `task_state`, even if it's null, since the current state
// may not be null.it took me a bit to parse, but I think this is saying "this might be used to clear current task state", right?
Done
Related details
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michal Mocny
| Code-Review | +1 |
1 comment
// An RAII scope that forces Task Attribution to ingore the top-levelPlease fix this WARNING reported by Spellchecker: "ingore" is a possible misspelling of "ignore".
To bypass Spellchecker, add a f...
"ingore" is a possible misspelling of "ignore".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
Done
Related details
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley voted and added 7 comments![]( "Open in Gerrit")
Votes added by Scott Haseley
| Commit-Queue | +1 |
I know its still WIP, but endorsing this approach :D
Thanks! It's better-ish now. Unfortunate this isn't straightforward, but I can live with this.
synchronous events, we never want to overwrite the current staet, butScott Haseleystate
Done
[SupportsTaskAttribution]Oh nice, I didn't know we have this.
Q: Would it be possible to make one of these (decorators?) for the PaintTimingMixin?
It's possible to add more, yes! But it depends on what for. This one isn't specced, it's just an internal convenience (an extended attribute), although I suspect we'll do something like this for AsyncContext. If you need it to be web-exposed (i.e. put in the spec), that might be different.
---
Aside: I'd like to use this more, but the InContext check doesn't work well with how low-level this is.
using PassKeyType = base::PassKey<NavigateEvent>;This is public-- is this a temporary and eventually will be private + friend list? Or is this sorta just a way to mark "be extra careful"?
Oh I just did that so I could use the type in NavigateEvent. Would using PassKey<> directly be better (I really don't know)?
---
I also don't want this to expand beyond this. We need a better v8 API, but that will take time.
// Always propagate `task_state`, even if it's null, since the current state
// may not be null.it took me a bit to parse, but I think this is saying "this might be used to clear current task state", right?
Obsolete
void SetTaskState(scheduler::TaskAttributionInfo* task,Blink Style Guide: Prefer enums or StrongAliases to bare bools for function parameters. The boolean parameter 'should_propagate_ignoring_top_level' is not obvious from the function name 'SetTaskState'. Consider using an enum or base::StrongAlias to improve readability at call sites.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: 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)_
Obsolete.
// task state, regardless of whether JavaScript is running or not. Typically
// nested dispatch (e.g. events) uses the current state, but in rare cases
// this needs to be overridden, such as with the navigation API.Patch LGTM but this detail I need to understand better some (future) day.
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michal Mocny
| Code-Review | +1 |
1 comment
using PassKeyType = base::PassKey<NavigateEvent>;Scott HaseleyThis is public-- is this a temporary and eventually will be private + friend list? Or is this sorta just a way to mark "be extra careful"?
Oh I just did that so I could use the type in NavigateEvent. Would using PassKey<> directly be better (I really don't know)?
---
I also don't want this to expand beyond this. We need a better v8 API, but that will take time.
Ah sorry, I misread what this did. This still only specifically allows the one class to use it. (I first read this as a public object/reference that anyone could just pass to the constructor)
Related details
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley voted and added 1 comment![]( "Open in Gerrit")
Votes added by Scott Haseley
| Commit-Queue | +1 |
1 comment
Changed to ignore the known JS-is-running case, using UserNavigationInvolvement!
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michal Mocny
| Code-Review | +1 |
1 comment
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley 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
TaskAttribution: Instrument NavigateEvent.intercept() handlers
Propagate task state from NavigationEvent.intercept() to the `handler`
and `precommitHandler`. This is needed for crrev.com/c/7638455 which
changes how the SoftNavigationContext task state variable is set,
wrapping individual events (including navigate) instead of creating a
TaskScope associated with the same-document navigation IPC task.
This is a bit different from other propagation in that we have to
ignore the is-JavaScript-running "top level" because of how deeply
nested the handler dispatch is under a ScriptState::Scope, which breaks
out top-level check. To work around this, this CL introduced a top-level
override scope -- pass-keyed to NavigateEvent -- to ignore the top-
level check when propagating state. crbug.com/490536691 tracks improving
this. We only use this with UserNavigationInvolvement::kNone, since
the current task state should continue to be used if JS is executing.
- M third_party/blink/renderer/core/navigation_api/navigate_event.cc
- M third_party/blink/renderer/core/navigation_api/navigation_intercept_options.idl
- M third_party/blink/renderer/core/scheduler/build.gni
- A third_party/blink/renderer/core/scheduler/task_attribution_top_level_override_scope.cc
- A third_party/blink/renderer/core/scheduler/task_attribution_top_level_override_scope.h
- M third_party/blink/renderer/core/scheduler/task_attribution_tracker_impl.cc
- M third_party/blink/renderer/core/scheduler/task_attribution_tracker_impl.h
- A third_party/blink/web_tests/wpt_internal/task-tracking/track-navigate-intercept.html
Code-Review: +1 by Michal Mocny
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |