User-Agent: Move ShouldOverrideUserAgent() to NavigationControllerImpl [chromium/src : main]
Takashi Nakayama (Gerrit)
Takashi Nakayama voted and added 2 comments![]( "Open in Gerrit")
Votes added by Takashi Nakayama
| Commit-Queue | +1 |
2 comments
Charlie Reis@cr...@chromium.org Could you please take a look at this CL because the Tokyo-based reviewer is OOO this week? Thanks.
Takashi NakayamaThanks, but we don't allow new content/public APIs until there's a use for them outside content/ in the tree:
https://chromium.googlesource.com/chromium/src/+/HEAD/content/public/README.mdThis is partly so that we can review how the new API will be used, to determine if it's the right API to expose. Can you include the use case in this CL, or a dependent CL that we can get approved as well?
Takashi NakayamaI will create dependent CL(s) next week, then ask for your review again. Thanks!
Charlie ReisCreated dependents CL(s). Could you please take a look again?
Takashi NakayamaThanks! All of the changes in https://chromium-review.googlesource.com/c/chromium/src/+/6905125/5 and https://chromium-review.googlesource.com/c/chromium/src/+/6906464/1 are within content/, which means this shouldn't need to be exposed in the content/public API. Can we put it on NavigationControllerImpl instead?
Thank you for the detailed and careful suggestions! I changed the function to a method of `NavigationControllerImpl` and changed the name (and updated CL description accordingly). Could you please take a look?
// last committed entry. `last_committed_entry` can be a nullptr.Charlie ReisNeeding to pass (specifically) the last committed entry here feels error prone, and something that's probably better handled within NavigationController itself. Maybe the use case for the API will make it clearer whether this is the right approach.
Takashi NakayamaThe use case in `PrefetchContainer::ShouldApplyUserAgentOverride` [here](https://chromium-review.googlesource.com/c/chromium/src/+/6905125/5/content/browser/preloading/prefetch/prefetch_container.cc#1526) seems to have access to the NavigationController and calls GetLastCommittedEntry() on it to pass into this function.
As noted, that seems error prone, where someone could pass in the wrong entry (including allowing time to pass so that an earlier acquired last committed entry is no longer the current one).
Can we make this an instance method on NavigationControllerImpl instead, so that it can look up GetLastCommittedEntry() internally and callers don't have to worry about it?
Let me merge this to the other thread crrev.com/c/6894097/comments/36a08211_9088dbca
Related details
- Charlie Reis
- Hiroshige Hayashizaki
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Nakayama (Gerrit)
Takashi Nakayama added 1 comment![]( "Open in Gerrit")
// last committed entry. `last_committed_entry` can be a nullptr.Charlie ReisNeeding to pass (specifically) the last committed entry here feels error prone, and something that's probably better handled within NavigationController itself. Maybe the use case for the API will make it clearer whether this is the right approach.
Takashi NakayamaThe use case in `PrefetchContainer::ShouldApplyUserAgentOverride` [here](https://chromium-review.googlesource.com/c/chromium/src/+/6905125/5/content/browser/preloading/prefetch/prefetch_container.cc#1526) seems to have access to the NavigationController and calls GetLastCommittedEntry() on it to pass into this function.
As noted, that seems error prone, where someone could pass in the wrong entry (including allowing time to pass so that an earlier acquired last committed entry is no longer the current one).
Can we make this an instance method on NavigationControllerImpl instead, so that it can look up GetLastCommittedEntry() internally and callers don't have to worry about it?
Let me merge this to the other thread crrev.com/c/6894097/comments/36a08211_9088dbca
Marked as resolved.
Charlie Reis (Gerrit)
Charlie Reis voted and added 1 comment![]( "Open in Gerrit")
Votes added by Charlie Reis
| Code-Review | +1 |
Charlie Reis@cr...@chromium.org Could you please take a look at this CL because the Tokyo-based reviewer is OOO this week? Thanks.
Takashi NakayamaThanks, but we don't allow new content/public APIs until there's a use for them outside content/ in the tree:
https://chromium.googlesource.com/chromium/src/+/HEAD/content/public/README.mdThis is partly so that we can review how the new API will be used, to determine if it's the right API to expose. Can you include the use case in this CL, or a dependent CL that we can get approved as well?
Takashi NakayamaI will create dependent CL(s) next week, then ask for your review again. Thanks!
Charlie ReisCreated dependents CL(s). Could you please take a look again?
Takashi NakayamaThanks! All of the changes in https://chromium-review.googlesource.com/c/chromium/src/+/6905125/5 and https://chromium-review.googlesource.com/c/chromium/src/+/6906464/1 are within content/, which means this shouldn't need to be exposed in the content/public API. Can we put it on NavigationControllerImpl instead?
Thank you for the detailed and careful suggestions! I changed the function to a method of `NavigationControllerImpl` and changed the name (and updated CL description accordingly). Could you please take a look?
- Hiroshige Hayashizaki
- Takashi Nakayama
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Nakayama (Gerrit)
Takashi Nakayama voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Hiroshige Hayashizaki
Code-Coverage
Code-Owners
Code-Review
| 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")
Change information
User-Agent: Move ShouldOverrideUserAgent() to NavigationControllerImpl
The function `ShouldOverrideUserAgent()` is responsible for converting
`NavigationController::UserAgentOverrideOption` to final bool decision.
This CL changes this function into a method of NavigationControllerImpl
so that other code that handles UA overrides (e.g. //content prefetch)
can use this feature in future. This CL also renames the function name
to `ShouldOverrideUserAgentInNextNavigation()` for clarity.
Bug: 441612842
- M content/browser/renderer_host/navigation_controller_impl.cc
- M content/browser/renderer_host/navigation_controller_impl.h
Code-Review: +1 by Charlie Reis
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |