[searchprefetch] Set the tracking logic for keep alive beacon requests [chromium/src : main]
Lingqi Chi (Gerrit)
Lingqi Chi added 1 comment![]( "Open in Gerrit")
Related details
- Hiroki Nakagawa
- Ming-Ying Chung
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ming-Ying Chung (Gerrit)
Ming-Ying Chung voted and added 3 comments![]( "Open in Gerrit")
Votes added by Ming-Ying Chung
| Code-Review | +1 |
3 comments
lgtm
GURL GetDefaultSearchEngineUrl(content::BrowserContext* browser_context) {can this be null?
std::string_view query_piece = url.query();
url::Component query(0, query_piece.length());
url::Component key;
url::Component value;
while (url::ExtractQueryKeyValue(query_piece, &query, &key, &value)) {
std::string_view key_view = query_piece.substr(key.begin, key.len);
std::string_view value_view = query_piece.substr(value.begin, value.len);
if (key_view != "pf") {
continue;
}
if (value_view == kNavigationPrefetchParam.Get() ||
value_view == kSuggestPrefetchParam.Get()) {
return true;
}
}not sure about the intention here but can we use existing helper like `net::GetValueForKeyInQuery()` instead?
Related details
- Hiroki Nakagawa
- Lingqi Chi
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 voted and added 6 comments![]( "Open in Gerrit")
Votes added by Hiroki Nakagawa
| Code-Review | +1 |
6 comments
LGTM, thank you!
if (url.ReplaceComponents(replacements) != host) {`url.GetHost()` should work?
// It should not happen in our case?Yeah, this happens only when the API is the FetchLater:
https://source.chromium.org/chromium/chromium/src/+/main:content/public/browser/keep_alive_request_tracker.h;l=74-76;drc=5be55f3378e1097bdd863956de2a4baac12b3f03
EXPECT_NE(nullptr, search_prefetch_service);`ASSERT` may be more appropriate? (this is a pre-condition of the test)
fetch("$1", { method: "POST",keepalive: true})nit: `{ method: "POST", keepalive: true }` (adding spaces before "keepalive" and after "true" for consistency)
"/ack?pf=" + kSuggestPrefetchParam.Get());Can we add other random query parameters to check if the implementation correctly extract the pf?
Related details
- Lingqi Chi
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. |
Lingqi Chi (Gerrit)
Lingqi Chi added 7 comments![]( "Open in Gerrit")
GURL GetDefaultSearchEngineUrl(content::BrowserContext* browser_context) {can this be null?
in most cases no..
It will be null only if the corresponding storage partition is being destroyed. And I'm not sure if we will hit this case or not, so I use pointer here.
Should I add a DumpWithoutCrash to ensure it will never be null when reaching here?
if (url.ReplaceComponents(replacements) != host) {Lingqi Chi`url.GetHost()` should work?
Done
std::string_view query_piece = url.query();
url::Component query(0, query_piece.length());
url::Component key;
url::Component value;
while (url::ExtractQueryKeyValue(query_piece, &query, &key, &value)) {
std::string_view key_view = query_piece.substr(key.begin, key.len);
std::string_view value_view = query_piece.substr(value.begin, value.len);
if (key_view != "pf") {
continue;
}
if (value_view == kNavigationPrefetchParam.Get() ||
value_view == kSuggestPrefetchParam.Get()) {
return true;
}
}not sure about the intention here but can we use existing helper like `net::GetValueForKeyInQuery()` instead?
TIL!!
Yeah, this happens only when the API is the FetchLater:
https://source.chromium.org/chromium/chromium/src/+/main:content/public/browser/keep_alive_request_tracker.h;l=74-76;drc=5be55f3378e1097bdd863956de2a4baac12b3f03
Thanks!!
`ASSERT` may be more appropriate? (this is a pre-condition of the test)
Done
nit: `{ method: "POST", keepalive: true }` (adding spaces before "keepalive" and after "true" for consistency)
Done
"/ack?pf=" + kSuggestPrefetchParam.Get());Can we add other random query parameters to check if the implementation correctly extract the pf?
I updated the unit tests a little bit, to enrich the test cases.
Ideally the unit tests can catch a bug if the implementation cannot correctly extract the pf
Related details
- Hiroki Nakagawa
- Ming-Ying Chung
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. |
Lingqi Chi (Gerrit)
Lingqi Chi added 1 comment![]( "Open in Gerrit")
oops, I forgot to commit the patch that removing the debugging code.. sorry.
Related details
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. |
Lingqi Chi (Gerrit)
Lingqi Chi added 1 comment![]( "Open in Gerrit")
- Hiroki Nakagawa
- Ming-Ying Chung
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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Hiroki Nakagawa
| Code-Review | +1 |
1 comment
"/ack?pf=" + kSuggestPrefetchParam.Get());Lingqi ChiCan we add other random query parameters to check if the implementation correctly extract the pf?
I updated the unit tests a little bit, to enrich the test cases.
Ideally the unit tests can catch a bug if the implementation cannot correctly extract the pf
- Lingqi Chi
- Ming-Ying Chung
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. |