[VL] Inline and remove history::kVisitedLinksOn404 [chromium/src : main]
Svend L (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Kyra Seevers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kyra Seevers (Gerrit)
Kyra Seevers voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kyra Seevers
| Code-Review | +1 |
1 comment
Thanks for the cleanup - can't believe its been this long! LGTM
Related details
- Svend L
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
manuk hovanesian (Gerrit)
manuk hovanesian added 5 comments![]( "Open in Gerrit")
status_code_qualifies_for_ntp_most_visited &&before:
feature was enabled,
so status_code_qualifies_for_ntp_most_visited was false,
so should_consider_for_ntp_most_visited was always false.
shouldn't we remove lines 326-331 to preserve current launch behavior?
// visits were excluded to avoid making a feature change to `VisitTracker`
// at the same time as the change to make 404s eligible for History (before
// that change, 404 visits were skipped upstream of this code).optional nit, indent subsequent lines of todo comments
```
// chains.
// TODO...
// visits were
// at the same
// that change
// TODO
// navigation
// them
```
const bool browser_should_update_history = !browser_url_is_unreachable;can we dedupe these variables?
params->should_update_history = !document_loader->HasUnreachableURL();optional, likewise, should we dedupe these variables
const bool status_code_qualifies_for_ntp_most_visited =this was false before, so shouldn't consider_for_ntp_most_visited also be false now to preserve launched behavior ?
Related details
- Svend L
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. |
Svend L (Gerrit)
Svend L added 6 comments![]( "Open in Gerrit")
status_code_qualifies_for_ntp_most_visited &&before:
feature was enabled,
so status_code_qualifies_for_ntp_most_visited was false,
so should_consider_for_ntp_most_visited was always false.shouldn't we remove lines 326-331 to preserve current launch behavior?
Before, the condition was this (whitespace changed for extra legibility):
```cpp
!(
base::FeatureList::IsEnabled(history::kVisitedLinksOn404) &&
http_response_code == 404
)
```
The feature is now always true, so the condition is now equivalent to:
```cpp
!(
http_response_code == 404
)
```
so I think the current inlining is correct.
// visits were excluded to avoid making a feature change to `VisitTracker`
// at the same time as the change to make 404s eligible for History (before
// that change, 404 visits were skipped upstream of this code).optional nit, indent subsequent lines of todo comments
```
// chains.
// TODO...
// visits were
// at the same
// that change
// TODO
// navigation
// them
```
Done
const bool browser_should_update_history = !browser_url_is_unreachable;can we dedupe these variables?
We could, but this variable is used with a bunch of other variables that are the browser-calculated version of renderer-provided values. They're then all used together several times in blocks, like this one on L18494:
```c++
const bool everything_except_origin_matches =
((!ShouldVerify("method") || browser_method == params.method) &&
(!ShouldVerify("url_is_unreachable") ||
browser_url_is_unreachable == params.url_is_unreachable) &&
(!ShouldVerify("post_id") || browser_post_id == params.post_id) &&
(!ShouldVerify("is_overriding_user_agent") ||
browser_is_overriding_user_agent == params.is_overriding_user_agent) &&
(!ShouldVerify("http_status_code") ||
browser_http_status_code == params.http_status_code) &&
(!ShouldVerify("should_update_history") ||
browser_should_update_history == params.should_update_history) &&
// ...
```
Other `browser_`-prefixed variables could also be easily inlined but aren't, like this one:
```c++
const bool browser_url_is_unreachable = is_error_document;
```
I suspect they're preserved as variables for consistency and legibility during comparisons to the renderer-provided values. I think I should keep that pattern here.
params->should_update_history = !document_loader->HasUnreachableURL();optional, likewise, should we dedupe these variables
`should_update_history` is a member of `mojom::DidCommitProvisionalLoadParams`, so I don't think we can dedupe here.
const bool status_code_qualifies_for_ntp_most_visited =this was false before, so shouldn't consider_for_ntp_most_visited also be false now to preserve launched behavior ?
Same as the thread on `chrome/browser/history/history_tab_helper.cc`, the conditional with `base::FeatureList::IsEnabled(history::kVisitedLinksOn404)` now always `true` is equivalent to:
```cpp
const bool status_code_qualifies_for_ntp_most_visited = !(http_response_code == 404);
```
So I believe this is inlined correctly.
Related details
- manuk hovanesian
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
manuk hovanesian (Gerrit)
manuk hovanesian voted and added 1 comment![]( "Open in Gerrit")
Votes added by manuk hovanesian
| Code-Review | +1 |
1 comment
Related details
- Svend L
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |