Refactor Geolocation connection and update logic [chromium/src : main]
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
bool EnsureGeolocationConnection(GeoNotifier*);For clarity, please add the parameter name to the declaration of `EnsureGeolocationConnection`. The role of the `GeoNotifier*` parameter is not obvious from the function name and type alone. (Blink Style Guide: Naming - May leave obvious parameter names out of function declarations)
_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
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alvin Ji (Gerrit)
Alvin Ji added 1 comment![]( "Open in Gerrit")
For clarity, please add the parameter name to the declaration of `EnsureGeolocationConnection`. The role of the `GeoNotifier*` parameter is not obvious from the function name and type alone. (Blink Style Guide: Naming - May leave obvious parameter names out of function declarations)
_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
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alvin Ji (Gerrit)
Alvin Ji added 1 comment![]( "Open in Gerrit")
Related details
- Matt Reynolds
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matt Reynolds (Gerrit)
Matt Reynolds added 1 comment![]( "Open in Gerrit")
!updating_) {Previously, UpdateGeolocationConnection would close the mojo pipe if it was no longer needed. Now I think the pipe is left open even when we stop calling QueryNextPosition. Is that intended?
Related details
- Alvin Ji
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alvin Ji (Gerrit)
Alvin Ji added 1 comment![]( "Open in Gerrit")
!updating_) {Previously, UpdateGeolocationConnection would close the mojo pipe if it was no longer needed. Now I think the pipe is left open even when we stop calling QueryNextPosition. Is that intended?
With this change, the pipe disconnecting path now is:
`OnPositionUpdated` -> `PositionChanged` -> `MakeSuccessCallbacks` -> if not `HasListeners` -> `StopUpdating` -> `ResetGeolocationConnection` and pipe is reset here.
I think this is probably not the best way doing it. We can also move the `HasListeners` check in the end of `OnPositionUpdated` like:
```
void Geolocation::OnPositionUpdated(){
...
if (HasLiseners()) {
// The in-flight query is now complete.
updating_ = false;
QueryNextPosition();
} else {
StopUpdating();
}
}
```
What do you think?
Related details
- Matt Reynolds
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matt Reynolds (Gerrit)
Matt Reynolds added 3 comments![]( "Open in Gerrit")
!updating_) {Alvin JiPreviously, UpdateGeolocationConnection would close the mojo pipe if it was no longer needed. Now I think the pipe is left open even when we stop calling QueryNextPosition. Is that intended?
With this change, the pipe disconnecting path now is:
`OnPositionUpdated` -> `PositionChanged` -> `MakeSuccessCallbacks` -> if not `HasListeners` -> `StopUpdating` -> `ResetGeolocationConnection` and pipe is reset here.I think this is probably not the best way doing it. We can also move the `HasListeners` check in the end of `OnPositionUpdated` like:
```
void Geolocation::OnPositionUpdated(){
...
if (HasLiseners()) {
// The in-flight query is now complete.
updating_ = false;
QueryNextPosition();
} else {
StopUpdating();
}
}
```
What do you think?
I'm having trouble following the chain of logic to convince myself that we're correctly resetting the pipe in all the same cases where it was previously reset. I think moving the HasListeners check to inside OnPositionUpdated (and anywhere else where HasListeners might be false) should make this clearer.
if (GetPage() && GetPage()->IsPageVisible()) {Should this also check HasListeners?
StopUpdating();nit: Let's make this an early exit to reduce nesting.
Related details
- Alvin Ji
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alvin Ji (Gerrit)
Alvin Ji added 4 comments![]( "Open in Gerrit")
Hi Matt,
PTAL at latest patchset, thanks!
Alvin
!updating_) {Alvin JiPreviously, UpdateGeolocationConnection would close the mojo pipe if it was no longer needed. Now I think the pipe is left open even when we stop calling QueryNextPosition. Is that intended?
Matt ReynoldsWith this change, the pipe disconnecting path now is:
`OnPositionUpdated` -> `PositionChanged` -> `MakeSuccessCallbacks` -> if not `HasListeners` -> `StopUpdating` -> `ResetGeolocationConnection` and pipe is reset here.I think this is probably not the best way doing it. We can also move the `HasListeners` check in the end of `OnPositionUpdated` like:
```
void Geolocation::OnPositionUpdated(){
...
if (HasLiseners()) {
// The in-flight query is now complete.
updating_ = false;
QueryNextPosition();
} else {
StopUpdating();
}
}
```
What do you think?
I'm having trouble following the chain of logic to convince myself that we're correctly resetting the pipe in all the same cases where it was previously reset. I think moving the HasListeners check to inside OnPositionUpdated (and anywhere else where HasListeners might be false) should make this clearer.
Thanks for review this, I have the geolocation.cc refactored further, now we don't need to pass notifier for `StartUpdating` (it is also renamed `UpdateGeolocationState`), please review latest patchset see if it make more sense now. Thanks!
Should this also check HasListeners?
Done, thanks for catching this.
nit: Let's make this an early exit to reduce nesting.
I have this part refactored again, so we don't need to iterate notifiers for starting them.
Related details
- Matt Reynolds
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matt Reynolds (Gerrit)
Matt Reynolds added 2 comments![]( "Open in Gerrit")
if (!GetExecutionContext()) {It's not clear to me why we're checking for null instead of keeping the DCHECK (and maybe upgrading to a CHECK). Can you explain why this is needed? Is it now expected that OnPositionUpdated will be called when the execution context is lost?
bool new_enable_high_accuracy = false;
for (auto& notifier : *one_shots_) {
if (notifier->Options()->enableHighAccuracy()) {
new_enable_high_accuracy = true;
break;
}
}
if (!new_enable_high_accuracy) {
for (auto& notifier : watchers_->Notifiers()) {
if (notifier->Options()->enableHighAccuracy()) {
new_enable_high_accuracy = true;
break;
}
}
}Let's use `std::ranges::any_of`
```
const bool has_high_accuracy_notifier =
std::ranges::any_of(*one_shots_,
[](const auto& notifier) {
return notifier->Options()->enableHighAccuracy();
}) ||
std::ranges::any_of(watchers_->Notifiers(),
[](const auto& notifier) {
return notifier->Options()->enableHighAccuracy();
});
```
Related details
- Alvin Ji
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alvin Ji (Gerrit)
Alvin Ji added 3 comments![]( "Open in Gerrit")
Thanks for the review, I have the CL updated and PATL again. Thanks!
It's not clear to me why we're checking for null instead of keeping the DCHECK (and maybe upgrading to a CHECK). Can you explain why this is needed? Is it now expected that OnPositionUpdated will be called when the execution context is lost?
Thanks, as we discussed offline, I think CHECK / DCHECK execution context in asynchronous callback is probably not needed. Similar for `OnGeolocationPermissionStatusUpdated`, I also added the context check and return if it is already destroyed.
bool new_enable_high_accuracy = false;
for (auto& notifier : *one_shots_) {
if (notifier->Options()->enableHighAccuracy()) {
new_enable_high_accuracy = true;
break;
}
}
if (!new_enable_high_accuracy) {
for (auto& notifier : watchers_->Notifiers()) {
if (notifier->Options()->enableHighAccuracy()) {
new_enable_high_accuracy = true;
break;
}
}
}Let's use `std::ranges::any_of`
```
const bool has_high_accuracy_notifier =
std::ranges::any_of(*one_shots_,
[](const auto& notifier) {
return notifier->Options()->enableHighAccuracy();
}) ||
std::ranges::any_of(watchers_->Notifiers(),
[](const auto& notifier) {
return notifier->Options()->enableHighAccuracy();
});
```
- Matt Reynolds
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alvin Ji (Gerrit)
Alvin Ji added 2 comments![]( "Open in Gerrit")
Sorry, please review patchset #15, I left a comment to explain what we missed in patchset#14
if (has_high_accuracy_notifier && !enable_high_accuracy_) {
enable_high_accuracy_ = true;If we do this we won't be able to trigger the transition from high to low accuracy but only the other way around.
Related details
- Matt Reynolds
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |