[Blink] Refactor preload logic to fix parser queueing [chromium/src : main]
Olivier Li (Gerrit)
Olivier Li added 1 comment![]( "Open in Gerrit")
static bool ShouldPreload(const Document* document,Olivier LiBlink Style Guide: Prefer enums or StrongAliases to bare bools for function parameters. To improve readability and type safety, consider using a `base::StrongAlias` for the `no_defer` parameter, for example: `using NoDefer = base::StrongAlias<class NoDeferTag, bool>;`.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reasonThis comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Acknowledged
Related details
- Alex Attar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Francois Pierre Doray (Gerrit)
Francois Pierre Doray added 1 comment![]( "Open in Gerrit")
return ShouldPreload(document_, preload->GetResourceType(),Should this return true when `preload->IsPreconnect()`, so that we don't filter out preconnect requests? Those are fulfilled by HTMLResourcePreloader::Preload even if AllowPreloadRequest() returns false.
Related details
- Alex Attar
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. |
Alex Attar (Gerrit)
Alex Attar voted and added 3 comments![]( "Open in Gerrit")
Votes added by Alex Attar
| Commit-Queue | +1 |
3 comments
static bool ShouldPreload(const Document* document,Blink Style Guide: Prefer enums or StrongAliases to bare bools for function parameters. To improve readability and type safety, consider using a `base::StrongAlias` for the `no_defer` parameter, for example: `using NoDefer = base::StrongAlias<class NoDeferTag, bool>;`.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reasonThis comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Done
return ShouldPreload(document_, preload->GetResourceType(),Should this return true when `preload->IsPreconnect()`, so that we don't filter out preconnect requests? Those are fulfilled by HTMLResourcePreloader::Preload even if AllowPreloadRequest() returns false.
oh that's a good point, thanks for catching this. I've added a `is_preconnect` check.
Related details
- Kouhei Ueno
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
2 comments
I'm not an expert in this code, but it looks ok to me. It'd be good to get one more +1.
default:This `switch` already seems to handle all of the enum values, doesn't it? (If not, which one did I miss?) If so, it's likely better to avoid adding a `default` so that future code finds this switch if new values are added.
Related details
- Alex Attar
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. |
Alex Attar (Gerrit)
Alex Attar added 1 comment![]( "Open in Gerrit")
This `switch` already seems to handle all of the enum values, doesn't it? (If not, which one did I miss?) If so, it's likely better to avoid adding a `default` so that future code finds this switch if new values are added.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Attar (Gerrit)
Alex Attar added 1 comment![]( "Open in Gerrit")
Please take a look. The Owner would like a second look,
Thanks
Related details
- Francois Pierre Doray
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Li (Gerrit)
Olivier Li voted and added 2 comments![]( "Open in Gerrit")
Votes added by Olivier Li
| Code-Review | +1 |
2 comments
This looks ok to me!
Please monitor the volumes for RequestResourceTime2.Preload and the timings for Blink.ScanAndPreloadTime.MainFrame.Initial on Android as this rolls out to Canary as it should have detectable impact on those numbers.
// like kLightweightNoStatePrefetch.Super-nit: Backticks around variable names.
Related details
- Alex Attar
- Francois Pierre Doray
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. |
Alex Attar (Gerrit)
Alex Attar added 1 comment![]( "Open in Gerrit")
Super-nit: Backticks around variable names.
- Francois Pierre Doray
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Attar (Gerrit)
Alex Attar voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
11 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/core/html/parser/html_resource_preloader.h
Insertions: 1, Deletions: 1.
@@ -53,7 +53,7 @@
// Centralized logic to determine if a preload request should be
// initiated. This is used by the parser to avoid queueing and by
// the preloader to filter requests, primarily for experiments
- // like kLightweightNoStatePrefetch.
+ // like `kLightweightNoStatePrefetch`.
static bool ShouldPreload(const Document* document,
ResourceType type,
bool is_preconnect,
```
Change information
[Blink] Refactor preload logic to fix parser queueing
This patch refactors the preload-filtering logic from
HTMLResourcePreloader::AllowPreloadRequest into new static functions
(ShouldPreload and IsTypePreloadable).
The new HTMLResourcePreloader::ShouldPreload function is now called by
HTMLDocumentParser before queueing a preload request. This fixes a bug
where the parser would queue requests (e.g., in the "html_only"
kLightweightNoStatePrefetch experiment) that would be immediately
discarded by the preloader, which was inefficient.
- M third_party/blink/renderer/core/html/parser/html_document_parser.cc
- M third_party/blink/renderer/core/html/parser/html_resource_preloader.cc
- M third_party/blink/renderer/core/html/parser/html_resource_preloader.h
Code-Review: +1 by Olivier Li, +1 by Mason Freed
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |