[VL] Let callers specify 404 policy in QueryHistory() [chromium/src : main]
Svend Larsen (Gerrit)
Svend Larsen voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Marc Treib
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marc Treib (Gerrit)
Marc Treib added 4 comments![]( "Open in Gerrit")
Generally LG, only I'm not fully convinced yet that we really can't add this to `QueryParams` - see comment below.
to allow their callers to specify other QueryOptions.Ugh, that's unfortunate... which ones are those (and how many)?
The only query method that takes a QueryOptions but not 404policy which I could immediately find is `GetAnnotatedVisits`, and it seems like that one might as well support the 404policy too.
std::u16string(), options, history::VisitQuery404sPolicy::kInclude404s,The corresponding prod code excludes 404s - is the difference intentional?
VisitQuery404sPolicy::kExclude404s, &visits);`policy_for_404s`, and remove the TODO above
Related details
- Svend Larsen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Svend Larsen (Gerrit)
Svend Larsen added 1 comment![]( "Open in Gerrit")
to allow their callers to specify other QueryOptions.Ugh, that's unfortunate... which ones are those (and how many)?
The only query method that takes a QueryOptions but not 404policy which I could immediately find is `GetAnnotatedVisits`, and it seems like that one might as well support the 404policy too.
(Sorry for the delay, I was out for a good chunk of yesterday.) `GetAnnotatedVisits()` could indeed support a 404 policy. The other methods that I'm aware of are `BrowsingHistoryService::QueryHistory()` (definitely) and `WebHistoryService::QueryHistory()` (possibly, I don't have enough context to tell for sure). But maybe it's worthwhile to create a separate `QueryOptions`-like type for those methods that's missing `policy_for_404_visits`? Something like this:
```
// Has all the fields that `QueryOptions` currently has.
QueryOptionsBase
|
| // No additional fields.
+-- UIQueryOptions : QueryOptionsBase
|
| // Adds `policy_for_404_visits` and maybe a ctor that takes a
| // `UIQueryOptions`.
+-- QueryOptions : QueryOptionsBase
```
WDYT?
Related details
- Marc Treib
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marc Treib (Gerrit)
Marc Treib added 1 comment![]( "Open in Gerrit")
to allow their callers to specify other QueryOptions.Svend LarsenUgh, that's unfortunate... which ones are those (and how many)?
The only query method that takes a QueryOptions but not 404policy which I could immediately find is `GetAnnotatedVisits`, and it seems like that one might as well support the 404policy too.
(Sorry for the delay, I was out for a good chunk of yesterday.) `GetAnnotatedVisits()` could indeed support a 404 policy. The other methods that I'm aware of are `BrowsingHistoryService::QueryHistory()` (definitely) and `WebHistoryService::QueryHistory()` (possibly, I don't have enough context to tell for sure). But maybe it's worthwhile to create a separate `QueryOptions`-like type for those methods that's missing `policy_for_404_visits`? Something like this:
```
// Has all the fields that `QueryOptions` currently has.
QueryOptionsBase
|
| // No additional fields.
+-- UIQueryOptions : QueryOptionsBase
|
| // Adds `policy_for_404_visits` and maybe a ctor that takes a
| // `UIQueryOptions`.
+-- QueryOptions : QueryOptionsBase
```WDYT?
So, `BrowsingHistoryService` is what actually populates chrome://history. It combines results from `HistoryService` (local history) and `WebHistoryService` (history downloaded from a Google server). `WebHistoryService` is mainly needed because it has 1 year's worth of history data, whereas the local history DB has only 3 months.
`BrowsingHistoryService` itself doesn't really do anything, so it'd just need to pass through the param.
`WebHistoryService` unfortunately can't really be updated to support a 404 policy, since it uses a legacy API that nobody wants to touch anymore. So it'll just return whatever history sync has uploaded.
I don't really know what the conclusion is TBH. Maybe just put the new bool into the existing struct, and accept that `WebHistoryService` doesn't support it? Since there's not much we can do about it anyway - IIUC ideally we'd want it to always exclude 404s, but that's just not on the table unfortunately.
Related details
- Svend Larsen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Svend Larsen (Gerrit)
Svend Larsen added 1 comment![]( "Open in Gerrit")
to allow their callers to specify other QueryOptions.Svend LarsenUgh, that's unfortunate... which ones are those (and how many)?
The only query method that takes a QueryOptions but not 404policy which I could immediately find is `GetAnnotatedVisits`, and it seems like that one might as well support the 404policy too.
Marc Treib(Sorry for the delay, I was out for a good chunk of yesterday.) `GetAnnotatedVisits()` could indeed support a 404 policy. The other methods that I'm aware of are `BrowsingHistoryService::QueryHistory()` (definitely) and `WebHistoryService::QueryHistory()` (possibly, I don't have enough context to tell for sure). But maybe it's worthwhile to create a separate `QueryOptions`-like type for those methods that's missing `policy_for_404_visits`? Something like this:
```
// Has all the fields that `QueryOptions` currently has.
QueryOptionsBase
|
| // No additional fields.
+-- UIQueryOptions : QueryOptionsBase
|
| // Adds `policy_for_404_visits` and maybe a ctor that takes a
| // `UIQueryOptions`.
+-- QueryOptions : QueryOptionsBase
```WDYT?
So, `BrowsingHistoryService` is what actually populates chrome://history. It combines results from `HistoryService` (local history) and `WebHistoryService` (history downloaded from a Google server). `WebHistoryService` is mainly needed because it has 1 year's worth of history data, whereas the local history DB has only 3 months.
`BrowsingHistoryService` itself doesn't really do anything, so it'd just need to pass through the param.
`WebHistoryService` unfortunately can't really be updated to support a 404 policy, since it uses a legacy API that nobody wants to touch anymore. So it'll just return whatever history sync has uploaded.I don't really know what the conclusion is TBH. Maybe just put the new bool into the existing struct, and accept that `WebHistoryService` doesn't support it? Since there's not much we can do about it anyway - IIUC ideally we'd want it to always exclude 404s, but that's just not on the table unfortunately.
Okay, that SGTM. I'll put up a new patchset later today that adds `policy_for_404_visits` to `QueryOptions`, and documents that `WebHistoryService::QueryHistory` doesn't support `QueryOptions::policy_for_404_visits`. I'll also simplify the existing methods that take a separate 404s policy param and a `QueryOptions` param.
Related details
- Marc Treib
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Svend Larsen (Gerrit)
Svend Larsen added 4 comments![]( "Open in Gerrit")
to allow their callers to specify other QueryOptions.Svend LarsenUgh, that's unfortunate... which ones are those (and how many)?
The only query method that takes a QueryOptions but not 404policy which I could immediately find is `GetAnnotatedVisits`, and it seems like that one might as well support the 404policy too.
Marc Treib(Sorry for the delay, I was out for a good chunk of yesterday.) `GetAnnotatedVisits()` could indeed support a 404 policy. The other methods that I'm aware of are `BrowsingHistoryService::QueryHistory()` (definitely) and `WebHistoryService::QueryHistory()` (possibly, I don't have enough context to tell for sure). But maybe it's worthwhile to create a separate `QueryOptions`-like type for those methods that's missing `policy_for_404_visits`? Something like this:
```
// Has all the fields that `QueryOptions` currently has.
QueryOptionsBase
|
| // No additional fields.
+-- UIQueryOptions : QueryOptionsBase
|
| // Adds `policy_for_404_visits` and maybe a ctor that takes a
| // `UIQueryOptions`.
+-- QueryOptions : QueryOptionsBase
```WDYT?
Svend LarsenSo, `BrowsingHistoryService` is what actually populates chrome://history. It combines results from `HistoryService` (local history) and `WebHistoryService` (history downloaded from a Google server). `WebHistoryService` is mainly needed because it has 1 year's worth of history data, whereas the local history DB has only 3 months.
`BrowsingHistoryService` itself doesn't really do anything, so it'd just need to pass through the param.
`WebHistoryService` unfortunately can't really be updated to support a 404 policy, since it uses a legacy API that nobody wants to touch anymore. So it'll just return whatever history sync has uploaded.I don't really know what the conclusion is TBH. Maybe just put the new bool into the existing struct, and accept that `WebHistoryService` doesn't support it? Since there's not much we can do about it anyway - IIUC ideally we'd want it to always exclude 404s, but that's just not on the table unfortunately.
Okay, that SGTM. I'll put up a new patchset later today that adds `policy_for_404_visits` to `QueryOptions`, and documents that `WebHistoryService::QueryHistory` doesn't support `QueryOptions::policy_for_404_visits`. I'll also simplify the existing methods that take a separate 404s policy param and a `QueryOptions` param.
Done, PTAL! (This approach also had the side benefit of reducing the number of test files this CL has to touch.)
std::u16string(), options, history::VisitQuery404sPolicy::kInclude404s,The corresponding prod code excludes 404s - is the difference intentional?
It wasn't! Fixed.
`policy_for_404s`, and remove the TODO above
Done
// Temporarily defaulted to `kExclude404s`; this may change in the future.
// Callers are strongly encouraged to explicitly set a value, unless they are
// certain that the handling of 404 visits is irrelevant for their use case.
VisitQuery404sPolicy policy_for_404_visits =
VisitQuery404sPolicy::kExclude404s;I considered three options here:
1. default to `kExclude404s`;
2. default to `kInclude404s`;
2. don't default the member and let accesses error;
3. don't default the member and add `CHECK`s inside methods that take a `QueryOptions` param.
Option 3 produced errors that were less obvious than would be helpful, and option 4 would've been easy to miss if a new method were to be added, and would then have become option 3. Option 2 felt dangerous, in case I missed a caller that expected to exclude 404s.
Related details
- Marc Treib
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marc Treib (Gerrit)
Marc Treib voted and added 6 comments![]( "Open in Gerrit")
Votes added by Marc Treib
| Code-Review | +1 |
6 comments
to allow their callers to specify other QueryOptions.Svend LarsenUgh, that's unfortunate... which ones are those (and how many)?
The only query method that takes a QueryOptions but not 404policy which I could immediately find is `GetAnnotatedVisits`, and it seems like that one might as well support the 404policy too.
Marc Treib(Sorry for the delay, I was out for a good chunk of yesterday.) `GetAnnotatedVisits()` could indeed support a 404 policy. The other methods that I'm aware of are `BrowsingHistoryService::QueryHistory()` (definitely) and `WebHistoryService::QueryHistory()` (possibly, I don't have enough context to tell for sure). But maybe it's worthwhile to create a separate `QueryOptions`-like type for those methods that's missing `policy_for_404_visits`? Something like this:
```
// Has all the fields that `QueryOptions` currently has.
QueryOptionsBase
|
| // No additional fields.
+-- UIQueryOptions : QueryOptionsBase
|
| // Adds `policy_for_404_visits` and maybe a ctor that takes a
| // `UIQueryOptions`.
+-- QueryOptions : QueryOptionsBase
```WDYT?
Svend LarsenSo, `BrowsingHistoryService` is what actually populates chrome://history. It combines results from `HistoryService` (local history) and `WebHistoryService` (history downloaded from a Google server). `WebHistoryService` is mainly needed because it has 1 year's worth of history data, whereas the local history DB has only 3 months.
`BrowsingHistoryService` itself doesn't really do anything, so it'd just need to pass through the param.
`WebHistoryService` unfortunately can't really be updated to support a 404 policy, since it uses a legacy API that nobody wants to touch anymore. So it'll just return whatever history sync has uploaded.I don't really know what the conclusion is TBH. Maybe just put the new bool into the existing struct, and accept that `WebHistoryService` doesn't support it? Since there's not much we can do about it anyway - IIUC ideally we'd want it to always exclude 404s, but that's just not on the table unfortunately.
Svend LarsenOkay, that SGTM. I'll put up a new patchset later today that adds `policy_for_404_visits` to `QueryOptions`, and documents that `WebHistoryService::QueryHistory` doesn't support `QueryOptions::policy_for_404_visits`. I'll also simplify the existing methods that take a separate 404s policy param and a `QueryOptions` param.
Done, PTAL! (This approach also had the side benefit of reducing the number of test files this CL has to touch.)
Acknowledged
/*max_count=*/150, history::VisitQuery404sPolicy::kExclude404s,nit: `/*policy_for_404_visits=*/` (for consistency with all the other params)
// Temporarily defaulted to `kExclude404s`; this may change in the future.
// Callers are strongly encouraged to explicitly set a value, unless they are
// certain that the handling of 404 visits is irrelevant for their use case.
VisitQuery404sPolicy policy_for_404_visits =
VisitQuery404sPolicy::kExclude404s;I considered three options here:
1. default to `kExclude404s`;
2. default to `kInclude404s`;
2. don't default the member and let accesses error;
3. don't default the member and add `CHECK`s inside methods that take a `QueryOptions` param.Option 3 produced errors that were less obvious than would be helpful, and option 4 would've been easy to miss if a new method were to be added, and would then have become option 3. Option 2 felt dangerous, in case I missed a caller that expected to exclude 404s.
All the other fields here are defaulted, so I wouldn't much like options 3 or 4.
Between 1 and 2, I agree that 1 is safer for now, but maybe 2 would be the better long-term option? Anyway, I can live with either of them.
options = QueryOptions(); // Reset options to default.From here on, the policy will be "exclude" (the default), where previously it was "include". Intentional? I guess it doesn't really matter for the remaining checks.
options = QueryOptions(); // Reset options to default.Same here - left to default on purpose?
Related details
- Svend Larsen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Svend Larsen (Gerrit)
Svend Larsen added 4 comments![]( "Open in Gerrit")
/*max_count=*/150, history::VisitQuery404sPolicy::kExclude404s,nit: `/*policy_for_404_visits=*/` (for consistency with all the other params)
Done
// Temporarily defaulted to `kExclude404s`; this may change in the future.
// Callers are strongly encouraged to explicitly set a value, unless they are
// certain that the handling of 404 visits is irrelevant for their use case.
VisitQuery404sPolicy policy_for_404_visits =
VisitQuery404sPolicy::kExclude404s;Marc TreibI considered three options here:
1. default to `kExclude404s`;
2. default to `kInclude404s`;
2. don't default the member and let accesses error;
3. don't default the member and add `CHECK`s inside methods that take a `QueryOptions` param.Option 3 produced errors that were less obvious than would be helpful, and option 4 would've been easy to miss if a new method were to be added, and would then have become option 3. Option 2 felt dangerous, in case I missed a caller that expected to exclude 404s.
All the other fields here are defaulted, so I wouldn't much like options 3 or 4.
Between 1 and 2, I agree that 1 is safer for now, but maybe 2 would be the better long-term option? Anyway, I can live with either of them.
Yeah, I think 2 is a better long-term option too — maybe that's a follow-up bug post-rollout. I added a TODO.
options = QueryOptions(); // Reset options to default.From here on, the policy will be "exclude" (the default), where previously it was "include". Intentional? I guess it doesn't really matter for the remaining checks.
Yeah, my thought was that it didn't affect the rest of the test, so it was fine to leave defaulted.
options = QueryOptions(); // Reset options to default.Same here - left to default on purpose?
I didn't think it affected the remaining tests, but on second pass, it does — I added an extra query for a `max_count` including 404s.
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Svend Larsen (Gerrit)
Svend Larsen added 1 comment![]( "Open in Gerrit")
shend@: PTAL for ash/quick_insert/.
Related details
- Darren Shen
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Svend Larsen (Gerrit)
Svend Larsen added 1 comment![]( "Open in Gerrit")
yaoxia@: PTAL for components/browsing_topics/, in particular to confirm that BrowsingTopicsCalculator doesn't want to receive 404.
Related details
- Darren Shen
- Yao Xiao
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Svend Larsen (Gerrit)
Svend Larsen added 1 comment![]( "Open in Gerrit")
- Darren Shen
- Yao Xiao
- manuk hovanesian
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Svend Larsen (Gerrit)
Svend Larsen added 1 comment![]( "Open in Gerrit")
manukh@: PTAL for components/history_clusters/.
And components/omnibox/browser/local_history_zero_suggest_provider.cc, if you feel comfortable.
Svend Larsen (Gerrit)
Svend Larsen added 1 comment![]( "Open in Gerrit")
romanarora@: PTAL for components/visited_url_ranking/ (this is the change I chatted you about the other day).
Related details
- Darren Shen
- Roman Arora
- Yao Xiao
- manuk hovanesian
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Svend Larsen (Gerrit)
Svend Larsen added 1 comment![]( "Open in Gerrit")
sczs@: PTAL for ios/chrome/browser/history/ui_bundled/base_history_view_controller.mm.
Related details
- Darren Shen
- Roman Arora
- Sergio Collazos
- Yao Xiao
- manuk hovanesian
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Svend Larsen (Gerrit)
Svend Larsen added 1 comment![]( "Open in Gerrit")
sczs@: PTAL for ios/chrome/browser/history/ui_bundled/base_history_view_controller.mm.
And ios/chrome/browser/tabs_search/model/tabs_search_service.mm, if you feel comfortable doing so.
Yao Xiao (Gerrit)
Yao Xiao voted Code-Review+1![]( "Open in Gerrit")
- Darren Shen
- Roman Arora
- Sergio Collazos
- Svend Larsen
- manuk hovanesian
Roman Arora (Gerrit)
Roman Arora voted and added 1 comment![]( "Open in Gerrit")
Votes added by Roman Arora
| Code-Review | +1 |
1 comment
LGTM For components/visited_url_ranking/internal/history_url_visit_data_fetcher.cc
Related details
- Darren Shen
manuk hovanesian (Gerrit)
manuk hovanesian voted and added 1 comment![]( "Open in Gerrit")
Votes added by manuk hovanesian
| Code-Review | +1 |
1 comment
lgtm
Related details
- Darren Shen
- Sergio Collazos
- Svend Larsen