Remove special text node behavior from streaming sanitizer [chromium/src : main]
Noam Rosenthal (Gerrit)
Noam Rosenthal added 1 comment![]( "Open in Gerrit")
Related details
- Daniel Vogelheim
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 voted and added 5 comments![]( "Open in Gerrit")
Votes added by Daniel Vogelheim
| Code-Review | +1 |
5 comments
Looks good.
I'm a little annoyed by the formatting changes; although I have to concede you're technically in the right there... :)
doc->setSanitizer(sanitizer);
doc->SetContent(html);
doc->SetMimeType(keywords::kTextHtml);
if (sanitizer) {
sanitizer->DidParseDocument(doc);
}[maybe] I find this construction with setting a Document member odd... I wonder if it'd be easier to instead have a second parameter to SetContent (i.e., `doc->SetContent(html, sanitizer)`).
Is Document::sanitizer_ used for other purposes I may be overlooking right now? Is it still useful after this method has finished, or should it be cleared?
if (++current_ == end_) {It seems nearly all changes in this file, other than the constructor changes in lines 372..383 are trivial formating changes that have nothing to do with the remainder of this CL and make it cumbersome to review. Could we revert those?
(There's a few more instances in other files.)
(I think technically, the style guide is on your side here as the style guide now wants any conditional to have braces. On the other hand, I don't think we usually like this type of churn.)
StreamingSanitizer(Sanitizer* sanitizer, Sanitizer::Mode mode)I like this simplification.
I do think at this point the StreamingSanitizer is just a wrapper around Sanitizer.
void StreamingSanitizer::DidParseDocument(Document* document) {I wonder whether this can replace the special treatment in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/sanitizer/sanitizer_api.cc;l=51-57
(Although... I think the current impl is very close to the current spec, which is useful on its own.)
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. |
Noam Rosenthal (Gerrit)
Noam Rosenthal added 2 comments![]( "Open in Gerrit")
doc->setSanitizer(sanitizer);
doc->SetContent(html);
doc->SetMimeType(keywords::kTextHtml);
if (sanitizer) {
sanitizer->DidParseDocument(doc);
}[maybe] I find this construction with setting a Document member odd... I wonder if it'd be easier to instead have a second parameter to SetContent (i.e., `doc->SetContent(html, sanitizer)`).
Is Document::sanitizer_ used for other purposes I may be overlooking right now? Is it still useful after this method has finished, or should it be cleared?
Done
It seems nearly all changes in this file, other than the constructor changes in lines 372..383 are trivial formating changes that have nothing to do with the remainder of this CL and make it cumbersome to review. Could we revert those?
(There's a few more instances in other files.)
(I think technically, the style guide is on your side here as the style guide now wants any conditional to have braces. On the other hand, I don't think we usually like this type of churn.)
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Jägenstedt (Gerrit)
Philip Jägenstedt voted and added 1 comment![]( "Open in Gerrit")
Votes added by Philip Jägenstedt
| Code-Review | +1 |
1 comment
SanitizerAPI::SanitizeInternal(Should this path be reachable even after the runtime feature is enabled? Depends on whether `CreateStreamingSanitizer()` can return nullptr, which isn't obvious here at the call site. If not, then add an assert here?
Same below.
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. |
Noam Rosenthal (Gerrit)
Noam Rosenthal added 1 comment![]( "Open in Gerrit")
Should this path be reachable even after the runtime feature is enabled? Depends on whether `CreateStreamingSanitizer()` can return nullptr, which isn't obvious here at the call site. If not, then add an assert here?
Same below.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/59541.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Noam Rosenthal (Gerrit)
Noam Rosenthal voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |