[omnibox-next] Fix crash due to unbound page remote. [chromium/src : main]
Khalid Peer (Gerrit)
Khalid Peer voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Moe Ahmadi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Khalid Peer (Gerrit)
Khalid Peer voted Auto-Submit+1![]( "Open in Gerrit")
| Auto-Submit | +1 |
Related details
- Moe Ahmadi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Moe Ahmadi (Gerrit)
Moe Ahmadi voted and added 2 comments![]( "Open in Gerrit")
Votes added by Moe Ahmadi
| Code-Review | +1 |
2 comments
const AutocompleteProviderClient* client =
autocomplete_controller()->autocomplete_provider_client();
page_->UpdateLensSearchEligibility(
ContextualSearchProvider::LensEntrypointEligible(input, client) &&
input.IsZeroSuggest());your CL fixes the crash but I'm curious why we update the eligibility on AutocompleteController Start observation. wouldn't that cause the chip visibility to change? or is this called early enough (before the popup shows)? I don't think this is the ideal event to listen for to update the chip visibility. do you know how the smart tab chip visibility is determined? we should try to be consistent wrt the two.
Related details
- Khalid Peer
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. |
Khalid Peer (Gerrit)
Khalid Peer added 1 comment![]( "Open in Gerrit")
const AutocompleteProviderClient* client =
autocomplete_controller()->autocomplete_provider_client();
page_->UpdateLensSearchEligibility(
ContextualSearchProvider::LensEntrypointEligible(input, client) &&
input.IsZeroSuggest());your CL fixes the crash but I'm curious why we update the eligibility on AutocompleteController Start observation. wouldn't that cause the chip visibility to change? or is this called early enough (before the popup shows)? I don't think this is the ideal event to listen for to update the chip visibility. do you know how the smart tab chip visibility is determined? we should try to be consistent wrt the two.
For reference, `AutocompleteController::OnStart()` is invoked at the very beginning of [the `AutocompleteController::Start()` method](https://source.chromium.org/chromium/chromium/src/+/main:components/omnibox/browser/autocomplete_controller.cc;l=651-655;drc=9d1a22037ffe551a091c6c76d0f3d2765517b80a).
Therefore, it's essentially invoked **before** the Omnibox popup shows up (since it depends on the AutocompleteResult that's constructed by `AutocompleteController::Start()`) and will also be re-invoked every time the user types another character into the Omnibox.
Related details
- Moe Ahmadi
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. |
Khalid Peer (Gerrit)
Khalid Peer added 1 comment![]( "Open in Gerrit")
const AutocompleteProviderClient* client =
autocomplete_controller()->autocomplete_provider_client();
page_->UpdateLensSearchEligibility(
ContextualSearchProvider::LensEntrypointEligible(input, client) &&
input.IsZeroSuggest());Khalid Peeryour CL fixes the crash but I'm curious why we update the eligibility on AutocompleteController Start observation. wouldn't that cause the chip visibility to change? or is this called early enough (before the popup shows)? I don't think this is the ideal event to listen for to update the chip visibility. do you know how the smart tab chip visibility is determined? we should try to be consistent wrt the two.
For reference, `AutocompleteController::OnStart()` is invoked at the very beginning of [the `AutocompleteController::Start()` method](https://source.chromium.org/chromium/chromium/src/+/main:components/omnibox/browser/autocomplete_controller.cc;l=651-655;drc=9d1a22037ffe551a091c6c76d0f3d2765517b80a).
Therefore, it's essentially invoked **before** the Omnibox popup shows up (since it depends on the AutocompleteResult that's constructed by `AutocompleteController::Start()`) and will also be re-invoked every time the user types another character into the Omnibox.
The "recent tab chip" visibility criteria is based [on a tab strip model observer method](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/webui/cr_components/searchbox/contextual_searchbox_handler.cc;l=192-204;drc=d81d1221a1ddff2931b35ad2ec6729ee5114fc9d), which is something we can't use here (since the input to the Omnibox can change while the user is still on the same tab).
Related details
- Moe Ahmadi
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. |
Moe Ahmadi (Gerrit)
Moe Ahmadi added 1 comment![]( "Open in Gerrit")
const AutocompleteProviderClient* client =
autocomplete_controller()->autocomplete_provider_client();
page_->UpdateLensSearchEligibility(
ContextualSearchProvider::LensEntrypointEligible(input, client) &&
input.IsZeroSuggest());Khalid Peeryour CL fixes the crash but I'm curious why we update the eligibility on AutocompleteController Start observation. wouldn't that cause the chip visibility to change? or is this called early enough (before the popup shows)? I don't think this is the ideal event to listen for to update the chip visibility. do you know how the smart tab chip visibility is determined? we should try to be consistent wrt the two.
For reference, `AutocompleteController::OnStart()` is invoked at the very beginning of [the `AutocompleteController::Start()` method](https://source.chromium.org/chromium/chromium/src/+/main:components/omnibox/browser/autocomplete_controller.cc;l=651-655;drc=9d1a22037ffe551a091c6c76d0f3d2765517b80a).
Therefore, it's essentially invoked **before** the Omnibox popup shows up (since it depends on the AutocompleteResult that's constructed by `AutocompleteController::Start()`) and will also be re-invoked every time the user types another character into the Omnibox.
that makes sense. `re-invoked every time the user types another character into the Omnibox` this part seems excessive.
Related details
- Khalid Peer
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. |
Khalid Peer (Gerrit)
Khalid Peer voted and added 1 comment![]( "Open in Gerrit")
Votes added by Khalid Peer
| Commit-Queue | +1 |
1 comment
const AutocompleteProviderClient* client =
autocomplete_controller()->autocomplete_provider_client();
page_->UpdateLensSearchEligibility(
ContextualSearchProvider::LensEntrypointEligible(input, client) &&
input.IsZeroSuggest());Khalid Peeryour CL fixes the crash but I'm curious why we update the eligibility on AutocompleteController Start observation. wouldn't that cause the chip visibility to change? or is this called early enough (before the popup shows)? I don't think this is the ideal event to listen for to update the chip visibility. do you know how the smart tab chip visibility is determined? we should try to be consistent wrt the two.
Moe AhmadiFor reference, `AutocompleteController::OnStart()` is invoked at the very beginning of [the `AutocompleteController::Start()` method](https://source.chromium.org/chromium/chromium/src/+/main:components/omnibox/browser/autocomplete_controller.cc;l=651-655;drc=9d1a22037ffe551a091c6c76d0f3d2765517b80a).
Therefore, it's essentially invoked **before** the Omnibox popup shows up (since it depends on the AutocompleteResult that's constructed by `AutocompleteController::Start()`) and will also be re-invoked every time the user types another character into the Omnibox.
that makes sense. `re-invoked every time the user types another character into the Omnibox` this part seems excessive.
We could implement a more targeted observer method for this, but for [this CL](https://chromium-review.googlesource.com/c/chromium/src/+/7242923) I opted for this approach to deliver a working solution that fit within the M144 timeline.
That being said, I'm happy to revisit this approach if it turns out to be too expensive from a browser performance POV.
Related details
- Moe Ahmadi
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. |
Moe Ahmadi (Gerrit)
Moe Ahmadi added 1 comment![]( "Open in Gerrit")
const AutocompleteProviderClient* client =
autocomplete_controller()->autocomplete_provider_client();
page_->UpdateLensSearchEligibility(
ContextualSearchProvider::LensEntrypointEligible(input, client) &&
input.IsZeroSuggest());Khalid Peeryour CL fixes the crash but I'm curious why we update the eligibility on AutocompleteController Start observation. wouldn't that cause the chip visibility to change? or is this called early enough (before the popup shows)? I don't think this is the ideal event to listen for to update the chip visibility. do you know how the smart tab chip visibility is determined? we should try to be consistent wrt the two.
Moe AhmadiFor reference, `AutocompleteController::OnStart()` is invoked at the very beginning of [the `AutocompleteController::Start()` method](https://source.chromium.org/chromium/chromium/src/+/main:components/omnibox/browser/autocomplete_controller.cc;l=651-655;drc=9d1a22037ffe551a091c6c76d0f3d2765517b80a).
Therefore, it's essentially invoked **before** the Omnibox popup shows up (since it depends on the AutocompleteResult that's constructed by `AutocompleteController::Start()`) and will also be re-invoked every time the user types another character into the Omnibox.
Khalid Peerthat makes sense. `re-invoked every time the user types another character into the Omnibox` this part seems excessive.
We could implement a more targeted observer method for this, but for [this CL](https://chromium-review.googlesource.com/c/chromium/src/+/7242923) I opted for this approach to deliver a working solution that fit within the M144 timeline.
That being said, I'm happy to revisit this approach if it turns out to be too expensive from a browser performance POV.
I don't think it'll be too expensive to have mojom calls for every keystroke. as long as it works well in the short term and you can optimize it / make it consistent later, this is good for now. Just wanted to point this out and see whether we're thinking about a longer term solution. please file a bug and leave a TODO for post-MVP. thanks!
Related details
- Khalid Peer
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. |
Khalid Peer (Gerrit)
Khalid Peer added 1 comment![]( "Open in Gerrit")
const AutocompleteProviderClient* client =
autocomplete_controller()->autocomplete_provider_client();
page_->UpdateLensSearchEligibility(
ContextualSearchProvider::LensEntrypointEligible(input, client) &&
input.IsZeroSuggest());Khalid Peeryour CL fixes the crash but I'm curious why we update the eligibility on AutocompleteController Start observation. wouldn't that cause the chip visibility to change? or is this called early enough (before the popup shows)? I don't think this is the ideal event to listen for to update the chip visibility. do you know how the smart tab chip visibility is determined? we should try to be consistent wrt the two.
Moe AhmadiFor reference, `AutocompleteController::OnStart()` is invoked at the very beginning of [the `AutocompleteController::Start()` method](https://source.chromium.org/chromium/chromium/src/+/main:components/omnibox/browser/autocomplete_controller.cc;l=651-655;drc=9d1a22037ffe551a091c6c76d0f3d2765517b80a).
Therefore, it's essentially invoked **before** the Omnibox popup shows up (since it depends on the AutocompleteResult that's constructed by `AutocompleteController::Start()`) and will also be re-invoked every time the user types another character into the Omnibox.
Khalid Peerthat makes sense. `re-invoked every time the user types another character into the Omnibox` this part seems excessive.
Moe AhmadiWe could implement a more targeted observer method for this, but for [this CL](https://chromium-review.googlesource.com/c/chromium/src/+/7242923) I opted for this approach to deliver a working solution that fit within the M144 timeline.
That being said, I'm happy to revisit this approach if it turns out to be too expensive from a browser performance POV.
I don't think it'll be too expensive to have mojom calls for every keystroke. as long as it works well in the short term and you can optimize it / make it consistent later, this is good for now. Just wanted to point this out and see whether we're thinking about a longer term solution. please file a bug and leave a TODO for post-MVP. thanks!
Sounds good to me! I've gone ahead and filed crbug.com/469098088 to track any potential optimization work as suggested here (and will update this code with a TODO comment).
Related details
- Moe Ahmadi
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. |
Khalid Peer (Gerrit)
Khalid Peer added 1 comment![]( "Open in Gerrit")
const AutocompleteProviderClient* client =
autocomplete_controller()->autocomplete_provider_client();
page_->UpdateLensSearchEligibility(
ContextualSearchProvider::LensEntrypointEligible(input, client) &&
input.IsZeroSuggest());Khalid Peeryour CL fixes the crash but I'm curious why we update the eligibility on AutocompleteController Start observation. wouldn't that cause the chip visibility to change? or is this called early enough (before the popup shows)? I don't think this is the ideal event to listen for to update the chip visibility. do you know how the smart tab chip visibility is determined? we should try to be consistent wrt the two.
Moe AhmadiFor reference, `AutocompleteController::OnStart()` is invoked at the very beginning of [the `AutocompleteController::Start()` method](https://source.chromium.org/chromium/chromium/src/+/main:components/omnibox/browser/autocomplete_controller.cc;l=651-655;drc=9d1a22037ffe551a091c6c76d0f3d2765517b80a).
Therefore, it's essentially invoked **before** the Omnibox popup shows up (since it depends on the AutocompleteResult that's constructed by `AutocompleteController::Start()`) and will also be re-invoked every time the user types another character into the Omnibox.
Khalid Peerthat makes sense. `re-invoked every time the user types another character into the Omnibox` this part seems excessive.
Moe AhmadiWe could implement a more targeted observer method for this, but for [this CL](https://chromium-review.googlesource.com/c/chromium/src/+/7242923) I opted for this approach to deliver a working solution that fit within the M144 timeline.
That being said, I'm happy to revisit this approach if it turns out to be too expensive from a browser performance POV.
Khalid PeerI don't think it'll be too expensive to have mojom calls for every keystroke. as long as it works well in the short term and you can optimize it / make it consistent later, this is good for now. Just wanted to point this out and see whether we're thinking about a longer term solution. please file a bug and leave a TODO for post-MVP. thanks!
Sounds good to me! I've gone ahead and filed crbug.com/469098088 to track any potential optimization work as suggested here (and will update this code with a TODO comment).
Done
Related details
- Moe Ahmadi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Khalid Peer (Gerrit)
Khalid Peer voted Commit-Queue+2![]( "Open in Gerrit")
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 with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/ui/webui/searchbox/webui_omnibox_handler.cc
Insertions: 4, Deletions: 0.
@@ -99,6 +99,10 @@
WebuiOmniboxHandler::~WebuiOmniboxHandler() = default;
+// TODO(crbug.com/469098088): Use something other than
+// `AutocompleteController::Observer::OnStart()` to reduce the IPC overhead
+// due to the fact that `AutocompleteController::Start()` gets invoked on
+// *every* keystroke in the Omnibox.
void WebuiOmniboxHandler::OnStart(AutocompleteController* controller,
const AutocompleteInput& input) {
// Ignore the call until the page remote is bound and ready to receive calls.
```
Change information
[omnibox-next] Fix crash due to unbound page remote.
This CL essentially adds a conditional `IsRemoteBound()` check to guard
the `UpdateLensSearchEligibility()` call, which aligns with the approach
taken in other parts of `webui_omnibox_handler.cc`, to resolve a crash.
- M chrome/browser/ui/webui/searchbox/webui_omnibox_handler.cc
Code-Review: +1 by Moe Ahmadi
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |