[Connection-Allowlist] [Preload] Fix parsing enforcement [chromium/src : main]
Sarah Murphy (Gerrit)
Sarah Murphy voted![]( "Open in Gerrit")
| Auto-Submit | +1 |
| Commit-Queue | +1 |
Related details
- Mike West
- Shivani Sharma
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shivani Sharma (Gerrit)
Shivani Sharma added 1 comment![]( "Open in Gerrit")
Moving myself to cc as Mike is a blink owner.
It would be good to rebase this as a child branch of https://chromium-review.googlesource.com/c/chromium/src/+/7708601 so it only shows the diff from the parent branch.
Related details
- Brandon Maslen
- Mike West
- Sarah Murphy
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. |
Sarah Murphy (Gerrit)
Sarah Murphy voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sarah Murphy
| Auto-Submit | +1 |
| Commit-Queue | +1 |
Moving myself to cc as Mike is a blink owner.
It would be good to rebase this as a child branch of https://chromium-review.googlesource.com/c/chromium/src/+/7708601 so it only shows the diff from the parent branch.
- Brandon Maslen
- Mike West
- Shivani Sharma
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike West (Gerrit)
Mike West added 1 comment![]( "Open in Gerrit")
rel_copy.Replace('\f', ' ');Hrm.
I'd like for us to find a way to reuse `RelList` (`DOMTokenList`) rather than re-parsing this, as we already encode the ASCII whitespace characters in too many disparate places. Making `LinkRelAttribute` a subclass of `RelList` with member functions that call `this->contains()` seems like a simpler design overall. That said, that's a larger change given usage in the codebase. Perhaps add a TODO? It'd be good cleanup.
For the moment, though, let's not scan the string 4 times. Consider using [`SplitOnASCIIWhitespace(...)`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/parser/html_parser_idioms.h;drc=d4203fb18d457eb93924af6bb2d24c954402978e;l=45) instead which already has the trimming behavior we want here and will only walk through the string once.
(We could also lowercase the string here to avoid it when matching all the `EqualIgnoringAsciiCase` below. Might be a simple optimization.)
Related details
- Brandon Maslen
- Sarah Murphy
- Shivani Sharma
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. |
Sarah Murphy (Gerrit)
Sarah Murphy voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sarah Murphy
| Auto-Submit | +1 |
| Commit-Queue | +2 |
1 comment
Hrm.
I'd like for us to find a way to reuse `RelList` (`DOMTokenList`) rather than re-parsing this, as we already encode the ASCII whitespace characters in too many disparate places. Making `LinkRelAttribute` a subclass of `RelList` with member functions that call `this->contains()` seems like a simpler design overall. That said, that's a larger change given usage in the codebase. Perhaps add a TODO? It'd be good cleanup.
For the moment, though, let's not scan the string 4 times. Consider using [`SplitOnASCIIWhitespace(...)`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/parser/html_parser_idioms.h;drc=d4203fb18d457eb93924af6bb2d24c954402978e;l=45) instead which already has the trimming behavior we want here and will only walk through the string once.
(We could also lowercase the string here to avoid it when matching all the `EqualIgnoringAsciiCase` below. Might be a simple optimization.)
This is good feedback! I've created crbug.com/505286910 for the clean-up work you called out, and lowercased the string as suggested. Thanks!
Related details
- Brandon Maslen
- Mike West
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike West (Gerrit)
Mike West voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mike West
| Code-Review | +1 |
| Commit-Queue | +2 |
1 comment
Related details
- Brandon Maslen
- Sarah Murphy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brandon Maslen (Gerrit)
Brandon Maslen voted![]( "Open in Gerrit")
- Mike West
- Sarah Murphy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| 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
[Connection-Allowlist] [Preload] Fix parsing enforcement
Enable previously-disabled Connection-Allowlist test:
1. WhitespaceInRelAttributeNormalized: verifies that <link rel>
attributes containing tabs, carriage returns, or form feeds are
correctly parsed for preconnect/dns-prefetch enforcement.
Supporting production fixes:
- link_rel_attribute.cc: normalize all ASCII whitespace (tab, CR, FF)
to spaces before splitting rel tokens, per the HTML spec.
- M third_party/blink/renderer/core/html/link_rel_attribute.cc
- M third_party/blink/renderer/core/loader/connection_allowlist_preload_test.cc
- D third_party/blink/web_tests/external/wpt/html/semantics/document-metadata/the-link-element/link-rel-attribute-tokenization-expected.txt
Code-Review: +1 by Brandon Maslen, +1 by Mike West
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |