Move logic from EventTiming::TryCreate into constructor. [chromium/src : main]
Johannes Henkel (Gerrit)
Johannes Henkel voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Michal Mocny
- 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 5 comments![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
5 comments
LGTM % nits and suggestions
I don't have a strong preference as to whether or not this is better than the existing pattern. I like that the std::optional<> makes it clear that the Scope is conditional, which this removes, although maybe that is a detail that call sites don't need to care about. Also, preventing unexpected moves is nice too. I think we tend to use both patterns in Chrome (not sure on stats), so I think this is fine either way.
Wait for Michal in case he has a stronger opinion :).
EventTiming eventTiming(frame && window ? window : nullptr, *event_,```suggestion
EventTiming event_timing(frame && window ? window : nullptr, *event_,
```
Might as well fix this while you're in here.
frame_ && frame_->DomWindow() ? frame_->DomWindow() : nullptr, *event,Could the c'tor take a frame instead to simplify all of the construction sites?
EventTiming(EventTiming&& other) = delete;
EventTiming& operator=(EventTiming&& other) = delete;nit: not needed ("For a non-copyable/movable type, delete the copy operations (the move operations will be implicitly deleted)" https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++-dos-and-donts.md#explicitly-declare-class-copyability_movability)
// * In the constructor, notifies the InteractiveDetector if the event needs
// to be logged into input delay histograms.
// * In the constructor, invokes WindowPerformance::EventTimingProcessingStart
// * In the destructor, invokes WindowPerformance::EventTimingProcessingEndA lot of this (% that we log histograms) is implementation details, and I don't think that should be documented here? I think it's good to document what this does at a high level, but the class comments mostly does this?
WDYT about moving the comment about histograms (minus the implementation detail) to up to the class comment and nixing this part? Or even moving all of the c'tor comments into one coherent class comment?
Related details
- Johannes Henkel
- 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
I appreciate this change, and I actually really like the outcome (~ scott's comments).
Buuuut this yak shave is going to require a lot of work to integrate into my stack of growing CLs. Not so bad for the CL that is currently under review and which will concflict, but also all the patches to expose new events, etc.
I would like to see this land asap so I can integrate, but if its alright with you, maybe we could try not step on toes for the next couple days?
Related details
- Johannes Henkel
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. |
Johannes Henkel (Gerrit)
Johannes Henkel added 6 comments![]( "Open in Gerrit")
LGTM % nits and suggestions
I don't have a strong preference as to whether or not this is better than the existing pattern. I like that the std::optional<> makes it clear that the Scope is conditional, which this removes, although maybe that is a detail that call sites don't need to care about. Also, preventing unexpected moves is nice too. I think we tend to use both patterns in Chrome (not sure on stats), so I think this is fine either way.
Wait for Michal in case he has a stronger opinion :).
Cool, thank you!
Addressed the feedback - thanks a lot. I also removed a check statement. This one ...
```
entry_ = performance->EventTimingProcessingStart(event, processing_start,
hit_test_target);
CHECK(entry_);
```
It surprised me that this one would trip, because EventTimingProcessingStart only has this way to return a nullptr as far as I can tell (and I would hope that we have a window and a frame given the other checks in the constructor).
```
if (!DomWindow() || !DomWindow()->GetFrame()) {
return nullptr;
}
```
But OK?
EventTiming eventTiming(frame && window ? window : nullptr, *event_,```suggestion
EventTiming event_timing(frame && window ? window : nullptr, *event_,
```Might as well fix this while you're in here.
Done
frame_ && frame_->DomWindow() ? frame_->DomWindow() : nullptr, *event,Could the c'tor take a frame instead to simplify all of the construction sites?
yeah that's way nicer - thanks!
EventTiming(EventTiming&& other) = delete;
EventTiming& operator=(EventTiming&& other) = delete;nit: not needed ("For a non-copyable/movable type, delete the copy operations (the move operations will be implicitly deleted)" https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++-dos-and-donts.md#explicitly-declare-class-copyability_movability)
Done
// * In the constructor, notifies the InteractiveDetector if the event needs
// to be logged into input delay histograms.
// * In the constructor, invokes WindowPerformance::EventTimingProcessingStart
// * In the destructor, invokes WindowPerformance::EventTimingProcessingEndA lot of this (% that we log histograms) is implementation details, and I don't think that should be documented here? I think it's good to document what this does at a high level, but the class comments mostly does this?
WDYT about moving the comment about histograms (minus the implementation detail) to up to the class comment and nixing this part? Or even moving all of the c'tor comments into one coherent class comment?
Added the important stuff to the class level comment no mention of the histograms so it's shorter now.
Related details
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 1 comment![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
1 comment
Addressed the feedback - thanks a lot. I also removed a check statement. This one ...
```
entry_ = performance->EventTimingProcessingStart(event, processing_start,
hit_test_target);
CHECK(entry_);
```It surprised me that this one would trip, because EventTimingProcessingStart only has this way to return a nullptr as far as I can tell (and I would hope that we have a window and a frame given the other checks in the constructor).
```
if (!DomWindow() || !DomWindow()->GetFrame()) {
return nullptr;
}
```But OK?
That's weird! You're checking for detach right before, so not sure. Ideally, with your change, that `return nullptr` could be a check. It might have to do with `DomWindow()` checking `IsContextDestroyed()` under the hood, but not sure. Probably worth understanding?
Related details
- Johannes Henkel
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. |
Johannes Henkel (Gerrit)
Johannes Henkel removed Mirko Bonadei from this change![]( "Open in Gerrit")
- Michal Mocny
- 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. |
Johannes Henkel (Gerrit)
Johannes Henkel removed Jerome Jiang from this change![]( "Open in Gerrit")
- Jerome Jiang
Johannes Henkel (Gerrit)
Johannes Henkel removed cblume...@chromium.org from this change![]( "Open in Gerrit")
Johannes Henkel (Gerrit)
Johannes Henkel removed chrome-intell...@chromium.org from this change![]( "Open in Gerrit")
Johannes Henkel (Gerrit)
Johannes Henkel removed chrome-intelligence-te...@google.com from this change![]( "Open in Gerrit")
Johannes Henkel (Gerrit)
Johannes Henkel removed feature-me...@chromium.org from this change![]( "Open in Gerrit")
Johannes Henkel (Gerrit)
Johannes Henkel removed penghuan...@chromium.org from this change![]( "Open in Gerrit")
Johannes Henkel (Gerrit)
Johannes Henkel removed jz...@chromium.org from this change![]( "Open in Gerrit")
Johannes Henkel (Gerrit)
Johannes Henkel removed fgal...@chromium.org from this change![]( "Open in Gerrit")
Johannes Henkel (Gerrit)
Johannes Henkel removed mar...@chromium.org from this change![]( "Open in Gerrit")
Johannes Henkel (Gerrit)
Johannes Henkel added 4 comments![]( "Open in Gerrit")
Scott HaseleyAddressed the feedback - thanks a lot. I also removed a check statement. This one ...
```
entry_ = performance->EventTimingProcessingStart(event, processing_start,
hit_test_target);
CHECK(entry_);
```It surprised me that this one would trip, because EventTimingProcessingStart only has this way to return a nullptr as far as I can tell (and I would hope that we have a window and a frame given the other checks in the constructor).
```
if (!DomWindow() || !DomWindow()->GetFrame()) {
return nullptr;
}
```But OK?
That's weird! You're checking for detach right before, so not sure. Ideally, with your change, that `return nullptr` could be a check. It might have to do with `DomWindow()` checking `IsContextDestroyed()` under the hood, but not sure. Probably worth understanding?
Thanks a lot - I think your theory is correct!
// performance->EventTimingProcessingStart - which we call below - requires
// execution context internally.
if (performance->GetExecutionContext() == nullptr) {
return;
}I added this one to confirm Scott's theory. And with that, the dry run passes.
CHECK(performance->GetExecutionContext() != nullptr);This was a bit paranoid, I'll back it out, I think the if statement above is sufficient.
CHECK(entry_);This check used to fail, before I added the if statement in l. 97.
Related details
- Michal Mocny
- 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. |
Johannes Henkel (Gerrit)
Johannes Henkel added 1 comment![]( "Open in Gerrit")
CHECK(performance->GetExecutionContext() != nullptr);This was a bit paranoid, I'll back it out, I think the if statement above is sufficient.
Done
Related details
- Michal Mocny
- 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 added 1 comment![]( "Open in Gerrit")
// performance->EventTimingProcessingStart - which we call below - requires
// execution context internally.
if (performance->GetExecutionContext() == nullptr) {
return;
}I added this one to confirm Scott's theory. And with that, the dry run passes.
Huzzah! Glad that was it.
I don't love that this leaks details of `EventTimingProcessingStart`. Another option, which I think is maybe a bit clearer and more idiomatic, is to check for detach directly via `window`, which _is_ the `ExecutionContext` here. IOW, to you could change the `window` test above to `if (!window || window->IsContextDestroyed())`.
Also, I think it would be better if EventTimingProcessingStart() can't return null, i.e. change the existing detach branch to a CHECK and document in the header that the associated window must not be detached. Because otherwise we have a branch that can't be hit (i.e. we're handling this in two places, and IMO we should only handle it in one, and the onus should be either on the caller or method for handling this).
WDYT?
Related details
- Johannes Henkel
- 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. |
Johannes Henkel (Gerrit)
Johannes Henkel voted and added 2 comments![]( "Open in Gerrit")
Votes added by Johannes Henkel
| Commit-Queue | +1 |
2 comments
// performance->EventTimingProcessingStart - which we call below - requires
// execution context internally.
if (performance->GetExecutionContext() == nullptr) {
return;
}Scott HaseleyI added this one to confirm Scott's theory. And with that, the dry run passes.
Huzzah! Glad that was it.
I don't love that this leaks details of `EventTimingProcessingStart`. Another option, which I think is maybe a bit clearer and more idiomatic, is to check for detach directly via `window`, which _is_ the `ExecutionContext` here. IOW, to you could change the `window` test above to `if (!window || window->IsContextDestroyed())`.
Also, I think it would be better if EventTimingProcessingStart() can't return null, i.e. change the existing detach branch to a CHECK and document in the header that the associated window must not be detached. Because otherwise we have a branch that can't be hit (i.e. we're handling this in two places, and IMO we should only handle it in one, and the onus should be either on the caller or method for handling this).
WDYT?
Yes, thank you! I added checks to EventTimingProcessing{Start,End} and documented these. Yes, I do think it's better to make the requirements there a bit stiffer and handle the context not existing in the caller.
CHECK(entry_);Johannes HenkelThis check used to fail, before I added the if statement in l. 97.
Acknowledged
Related details
- Michal Mocny
- 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 1 comment![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
1 comment
LGTM. I like how much cleaner the places where we create an EventTiming are!
Related details
- Johannes Henkel
- 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
I love it!
One warning is there is another patch that touch nearby files, may want to wait for that to land or confirm merge is clean.
Related details
- Johannes Henkel
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. |
Johannes Henkel (Gerrit)
Johannes Henkel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Johannes Henkel
| Commit-Queue | +2 |
1 comment
Thanks a lot!
Especially for the wonderful suggestions to make this cl better.
Michal I think your cl went in, so I rebased this one and will try to submit as well.
Related details
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. |
Johannes Henkel (Gerrit)
Johannes Henkel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Johannes Henkel
| Commit-Queue | +2 |
Scott HaseleyAddressed the feedback - thanks a lot. I also removed a check statement. This one ...
```
entry_ = performance->EventTimingProcessingStart(event, processing_start,
hit_test_target);
CHECK(entry_);
```It surprised me that this one would trip, because EventTimingProcessingStart only has this way to return a nullptr as far as I can tell (and I would hope that we have a window and a frame given the other checks in the constructor).
```
if (!DomWindow() || !DomWindow()->GetFrame()) {
return nullptr;
}
```But OK?
Johannes HenkelThat's weird! You're checking for detach right before, so not sure. Ideally, with your change, that `return nullptr` could be a check. It might have to do with `DomWindow()` checking `IsContextDestroyed()` under the hood, but not sure. Probably worth understanding?
Thanks a lot - I think your theory is correct!
Acknowledged
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Unreviewed changes
11 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
Move logic from EventTiming::TryCreate into constructor.
This is an attempt at yak-shaving away the TryCreate method, which
returns an std::optional instance of EventTiming, to be used with the
classic RAII pattern.
This slight variation moves the logic into the constructor instead,
which means no move semantics or std::optional are required. Instead,
when client code decides last second that it only wants a dummy
EventTiming, it passes a nullptr as the first argument.
- M third_party/blink/renderer/core/dom/events/event_dispatcher.cc
- M third_party/blink/renderer/core/input/mouse_event_manager.cc
- M third_party/blink/renderer/core/input/pointer_event_manager.cc
- M third_party/blink/renderer/core/timing/event_timing.cc
- M third_party/blink/renderer/core/timing/event_timing.h
- M third_party/blink/renderer/core/timing/window_performance.cc
- M third_party/blink/renderer/core/timing/window_performance.h
Code-Review: +1 by Michal Mocny, +1 by Scott Haseley
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |