Introduce NavigationInitiatorLocation in content/public [chromium/src : main]
Huanpo Lin (Gerrit)
Huanpo Lin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Huanpo Lin
| Commit-Queue | +1 |
1 comment
I'm creating this CL for https://docs.google.com/document/d/1urfbqZd8Y6iCBenTWEpJFkLO4rm7GlUvnOjFRjQ-cII/edit?disco=AAAB9DbI8U0
PTAL
Related details
- Ken Okada
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ken Okada (Gerrit)
Ken Okada added 5 comments![]( "Open in Gerrit")
enum class InitiatorLocation {Related to the comment at NavigationInitiatorLocationType.
I guess that it's not enough. InitiatorLocation looks to be a concept of enum open in //content and closed in //components/page_load_metrics, but
1. //components/page_load_metrics can't contain embedder's knowledge. For chrome specific knowledge, we must place them into //chrome/browser/page_load_metrics/, like NTP_PLMO is placed in it. https://source.chromium.org/chromium/chromium/src/+/main:components/page_load_metrics/README.md
2. And hence, it's not closed enum in //conponenets/page_load_metrics.
typedef uint16_t NavigationInitiatorLocationType;This smells. I guess that using uint16_t to //content/public is for realizing an open enum, but how "order" open them and why it's legitimate?
In the current code, the conversion is done in components/page_load_metrics/browser/initiator_type_enum.h:
```
inline void AttachNewTabPageNavigationHandleUserData(
content::NavigationHandle& navigation_handle) {
content::InitiatorTypeNavigationHandleUserData::CreateForNavigationHandle(
navigation_handle,
content::NavigationInitiatorLocation{
content::NavigationSource::kEmbedder,
static_cast<content::NavigationInitiatorLocationType>(
InitiatorLocation::kNewTabPage)});
}
```
Who ensures that all callers use this correnpondence of plm::InitiatorLocation and content::NavigationInitiatorLocationType?
Why this is not enough? https://docs.google.com/document/d/1urfbqZd8Y6iCBenTWEpJFkLO4rm7GlUvnOjFRjQ-cII/edit?disco=AAAB9DbI8U0
enum class NavigationSource {Could you briefly explain what do the concepts NavigationSource, NavigationInitiatorLocationType, InitiatorType mean? I kind of understand the diff of NavigationSource and NavInitLocType, but cant't understand the diff of NavInitLocType and InitiatorType. (I guess that the latter would be just a holder, and feel that it's better to use more systematic naming, like PreloadServingMetrics (a container of metrics) and PreloadServingMetricsHolder (NavigationHandleUserData to hold it).)
// Copyright 2026 The Chromium AuthorsI think it's better to split CLs for just rename and modification.
Related details
- Huanpo Lin
- Rakina Zata Amni
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 4 comments![]( "Open in Gerrit")
Votes added by Huanpo Lin
| Commit-Queue | +1 |
4 comments
enum class InitiatorLocation {Related to the comment at NavigationInitiatorLocationType.
I guess that it's not enough. InitiatorLocation looks to be a concept of enum open in //content and closed in //components/page_load_metrics, but
1. //components/page_load_metrics can't contain embedder's knowledge. For chrome specific knowledge, we must place them into //chrome/browser/page_load_metrics/, like NTP_PLMO is placed in it. https://source.chromium.org/chromium/chromium/src/+/main:components/page_load_metrics/README.md
2. And hence, it's not closed enum in //conponenets/page_load_metrics.
Sorry, I'm a bit confused, I think that only `components/page_load_metrics/renderer/` cannot have embedder knowledge? `components/page_load_metrics/browser/navigation_handle_user_data.h` was originally located in `components/page_load_metrics/browser/` as well.
typedef uint16_t NavigationInitiatorLocationType;This smells. I guess that using uint16_t to //content/public is for realizing an open enum, but how "order" open them and why it's legitimate?
In the current code, the conversion is done in components/page_load_metrics/browser/initiator_type_enum.h:
```
inline void AttachNewTabPageNavigationHandleUserData(
content::NavigationHandle& navigation_handle) {
content::InitiatorTypeNavigationHandleUserData::CreateForNavigationHandle(
navigation_handle,
content::NavigationInitiatorLocation{
content::NavigationSource::kEmbedder,
static_cast<content::NavigationInitiatorLocationType>(
InitiatorLocation::kNewTabPage)});
}
```Who ensures that all callers use this correnpondence of plm::InitiatorLocation and content::NavigationInitiatorLocationType?
Why this is not enough? https://docs.google.com/document/d/1urfbqZd8Y6iCBenTWEpJFkLO4rm7GlUvnOjFRjQ-cII/edit?disco=AAAB9DbI8U0
The pattern is trying to follow what `DisabledReason` in `content/public/browser/back_forward_cache.h`, which also uses `uint16_t`, does.
And the number will be numbered by (NavigationSource, NavigationInitiatorLocation), the first tuple will be multiplied by a number greater than 2^16 guaranteeing the uniqueness of the two-part key. But that's based on the assumption that the users should be aware of the enum should come from the correct headers. If needed I can add more comments about it in the corresponding headers.
enum class NavigationSource {Could you briefly explain what do the concepts NavigationSource, NavigationInitiatorLocationType, InitiatorType mean? I kind of understand the diff of NavigationSource and NavInitLocType, but cant't understand the diff of NavInitLocType and InitiatorType. (I guess that the latter would be just a holder, and feel that it's better to use more systematic naming, like PreloadServingMetrics (a container of metrics) and PreloadServingMetricsHolder (NavigationHandleUserData to hold it).)
I've added more comments in this file. PTAL
InitiatorTypeNavigationHandleUserData holds the tuple (NavigationSource, NavigationInitiatorLocation) representing the (content/embedder, trigger source id). The structure is trying to do what the document mentions (splitting content/embedder as some of the sources are in content/ like link clicks)
// Copyright 2026 The Chromium AuthorsI think it's better to split CLs for just rename and modification.
To double check, the only 1-to-1 mapping in this CL is `page_load_metrics::NavigationHandleUserData::InitiatorLocation` to in `page_load_metrics::InitiatorLocation` in `page_load_metrics`components/page_load_metrics/browser/initiator_type_enum.h`, but nothing to do with this file.
Do you mean to create ``page_load_metrics`components/page_load_metrics/browser/initiator_type_enum.h`` in another CL first or (?
Related details
- Ken Okada
- Rakina Zata Amni
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. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 3 comments![]( "Open in Gerrit")
types.Now this needs usage both outside and within `//content` (you need to do this, otherwise it doesn't belong as a `//content` public API and should be fully be in the embedder). Can you mention what your plan is for the "within `//content`" part?
class CONTENT_EXPORT InitiatorTypeNavigationHandleUserDataDo we actually need a UserData now that we have `NavigationInitiatorLocation` in `//content`? I was thinking about putting it in `NavigationHandle` / `OpenURLParams` `NavigationController::LoadURLParams` directly. Then you can also avoid the many constructor functions for different sources?
typedef uint16_t NavigationInitiatorLocationType;Huanpo LinThis smells. I guess that using uint16_t to //content/public is for realizing an open enum, but how "order" open them and why it's legitimate?
In the current code, the conversion is done in components/page_load_metrics/browser/initiator_type_enum.h:
```
inline void AttachNewTabPageNavigationHandleUserData(
content::NavigationHandle& navigation_handle) {
content::InitiatorTypeNavigationHandleUserData::CreateForNavigationHandle(
navigation_handle,
content::NavigationInitiatorLocation{
content::NavigationSource::kEmbedder,
static_cast<content::NavigationInitiatorLocationType>(
InitiatorLocation::kNewTabPage)});
}
```Who ensures that all callers use this correnpondence of plm::InitiatorLocation and content::NavigationInitiatorLocationType?
Why this is not enough? https://docs.google.com/document/d/1urfbqZd8Y6iCBenTWEpJFkLO4rm7GlUvnOjFRjQ-cII/edit?disco=AAAB9DbI8U0
The pattern is trying to follow what `DisabledReason` in `content/public/browser/back_forward_cache.h`, which also uses `uint16_t`, does.
And the number will be numbered by (NavigationSource, NavigationInitiatorLocation), the first tuple will be multiplied by a number greater than 2^16 guaranteeing the uniqueness of the two-part key. But that's based on the assumption that the users should be aware of the enum should come from the correct headers. If needed I can add more comments about it in the corresponding headers.
Yeah following DisabledReason is good but I think there are some bugs here. The observers in chrome/ (e.g. `BookmarkBarMetricsObserver::OnCommit`) are blindly casting id to page_load_metrics::InitiatorLocation without checking if source == content::NavigationSource::kEmbedder.
If content/ ever sets a NavigationInitiatorLocation with source = kContent and id = 1, BookmarkBarPageLoadMetricsObserver will incorrectly interpret it as kBookmarkBar (which is also 1) because it didn't verify the source.
If you want to follow the BackForwardCache::DisabledReason pattern, you should never cast .id. Instead, compare the entire struct using its safe operator==:
```
if (!navigation_userdata ||
navigation_userdata->initiator_location() !=
content::NavigationInitiatorLocation{
content::NavigationSource::kEmbedder,
static_cast<content::NavigationInitiatorLocationType>(
page_load_metrics::InitiatorLocation::kBookmarkBar)}) {
return STOP_OBSERVING;
}
```
(Consider adding a small helper factory in page_load_metrics to make constructing these embedder locations less verbose for observers.)Related details
- Ken Okada
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 3 comments![]( "Open in Gerrit")
Votes added by Huanpo Lin
| Commit-Queue | +1 |
3 comments
types.Now this needs usage both outside and within `//content` (you need to do this, otherwise it doesn't belong as a `//content` public API and should be fully be in the embedder). Can you mention what your plan is for the "within `//content`" part?
Updated the comments, PTAL.
The plan of adding kContent related NavigationInitiatorLocation is mentioned.
class CONTENT_EXPORT InitiatorTypeNavigationHandleUserDataDo we actually need a UserData now that we have `NavigationInitiatorLocation` in `//content`? I was thinking about putting it in `NavigationHandle` / `OpenURLParams` `NavigationController::LoadURLParams` directly. Then you can also avoid the many constructor functions for different sources?
Thanks, I've just removed UserData to reduce the code complexity.
The field is added to `NavigationHandle` directly.
I see, I've created helper functions and limit the usage of static_cast to those functions only. PTAL.
Related details
- Ken Okada
- Rakina Zata Amni
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")
include tluk for desktop/bookmark/omnibox review.
Related details
- Ken Okada
- Rakina Zata Amni
- 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. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 3 comments![]( "Open in Gerrit")
Thanks! This CL is rather large, I think it's probably nicer to split at this point.
inline void AttachNewTabPageNavigationHandleUserData(This name is now stale, and do we really need different functions for each instead of 1 generic function?
// Represents the initiator location of a navigation, uniquely identified by aThis needs more explanation of what it means, and how it differs from other similar things already e.g. PageTransition. When should someone rely on this instead of the others, and when should someone add new values here?
Related details
- Huanpo Lin
- Ken Okada
- 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. |