Support NavigationHandleUserData attachment for Omnibox [chromium/src : main]
Huanpo Lin (Gerrit)
Huanpo Lin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Huanpo Lin
| Commit-Queue | +1 |
Related details
- Hiroki Nakagawa
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima voted and added 1 comment![]( "Open in Gerrit")
Votes added by Takashi Toyoshima
| Code-Review | +1 |
1 comment
void NavigationHandleUserData::AttachOmniboxTypedNavigationHandleUserData(Just in case, if I remember your old change correctly, PLMO's Start callback is invoked before the LoadURLWithParams returns. So, if you want to check the user data in the Start event, this timing might be late.
Related details
- Hiroki Nakagawa
- Huanpo Lin
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. |
Huanpo Lin (Gerrit)
Huanpo Lin added 2 comments![]( "Open in Gerrit")
include tluk@ for c/b/u/browser_commands.cc and browser_navigator related files.
void NavigationHandleUserData::AttachOmniboxTypedNavigationHandleUserData(Just in case, if I remember your old change correctly, PLMO's Start callback is invoked before the LoadURLWithParams returns. So, if you want to check the user data in the Start event, this timing might be late.
Acked, thanks!
I'll be aware of the timing when introducing Omnibox logic in the corresponding PLMO.
Related details
- Hiroki Nakagawa
- Thomas Lukaszewicz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Lukaszewicz (Gerrit)
Thomas Lukaszewicz added 1 comment![]( "Open in Gerrit")
base::RepeatingCallback<void(content::NavigationHandle&)>Modifying the bloated `NavigateParams` with a metrics-specific callback is not ideal and likely not necessary.
`Navigate()` returns a `base::WeakPtr<content::NavigationHandle>` immediately once the navigation has been initiated. Could we instead attach the UserData on the handle after the navigation is requested in `chrome/browser/ui/browser_commands.cc`? i.e. something like
```
auto result = Navigate(¶ms);
if (result) {
if (ui::PageTransitionCoreTypeIs(
params.transition, ui::PAGE_TRANSITION_TYPED)) {
page_load_metrics::NavigationHandleUserData::
AttachOmniboxTypedNavigationHandleUserData(*result);
} else if (ui::PageTransitionCoreTypeIs(
params.transition, ui::PAGE_TRANSITION_GENERATED)) {
page_load_metrics::NavigationHandleUserData::
AttachOmniboxSuggestedNavigationHandleUserData(*result);
}
}
...
```
Related details
- Hiroki Nakagawa
- Huanpo Lin
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. |
Huanpo Lin (Gerrit)
Huanpo Lin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Huanpo Lin
| Commit-Queue | +1 |
1 comment
base::RepeatingCallback<void(content::NavigationHandle&)>Modifying the bloated `NavigateParams` with a metrics-specific callback is not ideal and likely not necessary.
`Navigate()` returns a `base::WeakPtr<content::NavigationHandle>` immediately once the navigation has been initiated. Could we instead attach the UserData on the handle after the navigation is requested in `chrome/browser/ui/browser_commands.cc`? i.e. something like
```
auto result = Navigate(¶ms);
if (result) {
if (ui::PageTransitionCoreTypeIs(
params.transition, ui::PAGE_TRANSITION_TYPED)) {
page_load_metrics::NavigationHandleUserData::
AttachOmniboxTypedNavigationHandleUserData(*result);
} else if (ui::PageTransitionCoreTypeIs(
params.transition, ui::PAGE_TRANSITION_GENERATED)) {
page_load_metrics::NavigationHandleUserData::
AttachOmniboxSuggestedNavigationHandleUserData(*result);
}
}
...
```
Updated, PTAL.
I was thinking of using the callback pattern so it could be used by other trigger types if some triggers happen to hit the same code path.
Since it is not ideal, I replace it by the page transition checks.
Related details
- Hiroki Nakagawa
- Takashi Toyoshima
- Thomas Lukaszewicz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroki Nakagawa (Gerrit)
Hiroki Nakagawa added 1 comment![]( "Open in Gerrit")
}Is it possible to distinguish DUI from DSE by page transition? What about other navigation initiators on Omnibox like AI mode?
Related details
- Huanpo Lin
- Takashi Toyoshima
- Thomas Lukaszewicz
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. |
Thomas Lukaszewicz (Gerrit)
Thomas Lukaszewicz added 1 comment![]( "Open in Gerrit")
Thanks this looks much better - will circle back and +1 after others have approved
Related details
- Huanpo Lin
- Takashi Toyoshima
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. |
Huanpo Lin (Gerrit)
Huanpo Lin added 1 comment![]( "Open in Gerrit")
Is it possible to distinguish DUI from DSE by page transition? What about other navigation initiators on Omnibox like AI mode?
Yes, DUI can be distinguished from DSE by ui::PAGE_TRANSITION_FROM_ADDRESS_BAR x (ui::PAGE_TRANSITION_TYPED, ui::PAGE_TRANSITION_GENERATED)
https://source.chromium.org/chromium/chromium/src/+/0d2c1d791ba7f07d5b76e2fa1a0af3509ac19d96:chrome/browser/preloading/prerender/prerender_manager.cc;l=230
https://source.chromium.org/chromium/chromium/src/+/0d2c1d791ba7f07d5b76e2fa1a0af3509ac19d96:chrome/browser/preloading/prerender/prerender_manager.cc;l=392, and the qualifier `ui::PAGE_TRANSITION_FROM_ADDRESS_BAR` is appended by `OmniboxEditModel::OpenMatch`, so it won't be there for AI mode, which is generated by `OmniboxEditModel::NavigateToAiModeWithoutContextualizer` calling `ChromeOmniboxClient::OpenUrl()` directly.
https://source.chromium.org/chromium/chromium/src/+/5392c43500788023e99230171f83de10299924f3:chrome/browser/ui/omnibox/omnibox_edit_model.cc;l=2983;bpv=1;bpt=0
The combination ui::PAGE_TRANSITION_FROM_ADDRESS_BAR x (ui::PAGE_TRANSITION_TYPED, ui::PAGE_TRANSITION_GENERATED) should be only related to Omnibox DSE/DUI navigations/preloading.
Related details
- Hiroki Nakagawa
- Takashi Toyoshima
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. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 1 comment![]( "Open in Gerrit")
revisit after Hiroki's comment is resolved (please set my attention bit again)
Related details
- Hiroki Nakagawa
- Huanpo Lin
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. |
Hiroki Nakagawa (Gerrit)
Hiroki Nakagawa added 1 comment![]( "Open in Gerrit")
base::RepeatingCallback<void(content::NavigationHandle&)>Huanpo LinModifying the bloated `NavigateParams` with a metrics-specific callback is not ideal and likely not necessary.
`Navigate()` returns a `base::WeakPtr<content::NavigationHandle>` immediately once the navigation has been initiated. Could we instead attach the UserData on the handle after the navigation is requested in `chrome/browser/ui/browser_commands.cc`? i.e. something like
```
auto result = Navigate(¶ms);
if (result) {
if (ui::PageTransitionCoreTypeIs(
params.transition, ui::PAGE_TRANSITION_TYPED)) {
page_load_metrics::NavigationHandleUserData::
AttachOmniboxTypedNavigationHandleUserData(*result);
} else if (ui::PageTransitionCoreTypeIs(
params.transition, ui::PAGE_TRANSITION_GENERATED)) {
page_load_metrics::NavigationHandleUserData::
AttachOmniboxSuggestedNavigationHandleUserData(*result);
}
}
...
```
Updated, PTAL.
I was thinking of using the callback pattern so it could be used by other trigger types if some triggers happen to hit the same code path.
Since it is not ideal, I replace it by the page transition checks.
I agree with Robert's initial thought. I think it makes sense to add a field to `NavigateParams` and attach it to the `NavigationHandle` in `Navigate()`, so that we can use the same code path for BookmarkBar, NTP, and other navigation triggers that eventually call `Navigate()`.
However, passing a callback might be overkill. Instead, how about directly passing `InitiatorLocation`?
Related details
- Huanpo Lin
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. |
Huanpo Lin (Gerrit)
Huanpo Lin added 1 comment![]( "Open in Gerrit")
More information about the NavigationHandleUserData attachment design in PS6.
The original problem was both NTP and BookmarkBar use AUTO_BOOKMARK, so they cannot be distinguished by page transition only, and information of the triggers cannot be hardcoded in content/ due to layer violations.
The attachment callback function pattern for both NTP and BookmarkBar for metrics recording was created due to this. (http://crrev.com/c/4918014 and http://crrev.com/c/4842248) https://docs.google.com/document/d/1-eZdVXImWMv3o7UA3rut_DKKkE89iqfjiNTHwIAvXAU/edit?usp=sharing
Using the same pattern (NavigationHandleUserData) to make the logic consistent is favored, so PS6 was used.
NTP and BookmarkBar are using `OpenURLFromTab` and `OpenAllHelper`, respectively.
```auto navigation = chrome::OpenCurrentURL(browser_);``` is likely to be used by omnibox only, so it won't be re-used by other triggers at the moment.
Related details
- Thomas Lukaszewicz
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. |
Hiroki Nakagawa (Gerrit)
Hiroki Nakagawa added 1 comment![]( "Open in Gerrit")
and information of the triggers cannot be hardcoded in content/ due to layer violations.
rakina@ suggests another pattern to make the initiator location injectable from both content/ and chrome/. Can we use this approach to resolve the layering issue?
https://docs.google.com/document/d/1urfbqZd8Y6iCBenTWEpJFkLO4rm7GlUvnOjFRjQ-cII/edit?disco=AAAB9DbI8U0
NTP and BookmarkBar are using OpenURLFromTab and OpenAllHelper, respectively.
auto navigation = chrome::OpenCurrentURL(browser_); is likely to be used by omnibox only, so it won't be re-used by other triggers at the moment.
To clarify, I mean the reusability of `Navigate()`, not the `Open*()` functions. `OpenCurrentURL()` may not be reused, but it internally calls `Navigate()`, which is already called from various initiator locations. With this approach, those locations can also naturally be supported eventually.
Related details
- Huanpo Lin
- Thomas Lukaszewicz
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. |