Use streaming sanitizer also for fragment parsing (behind a flag) [chromium/src : main]
Noam Rosenthal (Gerrit)
Noam Rosenthal voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Daniel Vogelheim
- Noam Rosenthal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Vogelheim (Gerrit)
Daniel Vogelheim added 3 comments![]( "Open in Gerrit")
StreamingSanitizer* streaming_sanitizer = nullptr;I wonder if this would benefit from a single top-level if-statement for the feature flag. E.g.,
if (RuntimeEnabledFeatures::StreamingSanitizerEnabled()) {
... new code ..
} else {
... old code ...
}I find it a bit difficult to follow & compare what we'd do with or without the flag.
} else {style nitpick: Above, you always return from the `if` and return to the top-level block, while here you use `else`. Dropping the `else` would be more consistent.
static bool CheckRootElement(Sanitizer::Mode mode,The name is technically correct, but IMHO not very descriptive. Maybe `IsContextElementScripty(..)` ? (Which is what the rule is about: If the context element we're inserting into is `<script>` (HTML or SVG), then we just shouldn't set anything.)
Related details
- Mason Freed
- Noam Rosenthal
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. |
Mason Freed (Gerrit)
Mason Freed voted and added 8 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
8 comments
StreamingSanitizer* streaming_sanitizer = nullptr;I wonder if this would benefit from a single top-level if-statement for the feature flag. E.g.,
if (RuntimeEnabledFeatures::StreamingSanitizerEnabled()) {
... new code ..
} else {
... old code ...
}I find it a bit difficult to follow & compare what we'd do with or without the flag.
Hmm, I'm not sure I agree (depending on how large the if {} else {} blocks get), but I guess ok either way.
StreamingSanitizer::TextNodeMergeMode::kKeepSeparate, options,side note, I'm unclear on how this is used, so I'll leave it to vogelheim@ to evaluate these settings.
DCHECK(temp_text_node_separators_.empty());maybe `CHECK`?
// This is a workaround to keep the fragment-based sanitizer behavior of
// keeping text nodes separate when the streaming sanitizer would otherwise
// merge them. See https://github.com/WICG/sanitizer-api/issues/390.Ahh. Ok, thanks, you can ignore my comment above.
void ReplaceWithChildren(Node* node) {Maybe `Node& node` since you assume it's non-nullptr.
} else {style nitpick: Above, you always return from the `if` and return to the top-level block, while here you use `else`. Dropping the `else` would be more consistent.
Unresolving because I agree
name: "StreamingSanitizer",Please add a quick comment about what this is, and maybe a crbug link.
Related details
- Noam Rosenthal
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. |