Parse SVG with the HTML Parser [chromium/src : main]
Mason Freed (Gerrit)
Mason Freed voted Auto-Submit+1![]( "Open in Gerrit")
| Auto-Submit | +1 |
Related details
- David Baron
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers added 2 comments![]( "Open in Gerrit")
Parse SVG with the HTML ParserThere is compat risk because this is common, though a hopeful aspect is that external svg docs are fairly new. WDYT of de-risking with data using https://www.chromium.org/blink/platform-predictability/compat-tools/? This question is a little out of scope for this review; can you file a bug linked to this for that discussion?
- The new parser is only used in the special case of SVGs that areDoes this mean we would use the html parser for cases like:
<div style="filter: url(foo.svg)">
and
<use href="foo.svg">
But not:
<img src="foo.svg">
and
http://domain.com/foo.svg
?
Related details
- David Baron
- Mason Freed
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. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/14c0b917510000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/16d840c3510000
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
The pinpoints didn't show a win, but this area does show up on profiles, so I wouldn't write off the speedometer3 win. I've kicked off another run on the m1 pgo bot to get a second opinion.
Here's profile data showing how much (and where) we spend time in this area: https://pprof.corp.google.com/?id=78bbb00af1fc8facfb90bfc6100c4ad5&pivot=IsolatedSVGDocumentHost&tab=flame (profile from all subbenchmarks).
https://issues.chromium.org/u/1/issues/394015877 is an open bug to investigate this area.
AI Code Reviewer (Gerrit)
AI Code Reviewer added 2 comments![]( "Open in Gerrit")
bool is_srcdoc_document_;nit: Per the Blink Style Guide: Naming - "Precede boolean values with words like “is” and “did”", please consider renaming `parse_svg_with_html_parser_` to `should_parse_svg_with_html_parser_` for clarity and consistency with other boolean members in this file (e.g., `is_srcdoc_document_`).
_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:** reason
This 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)_
bool parse_svg_with_html_parser_ = false;nit: Per the Blink Style Guide: Naming - "Precede boolean values with words like “is” and “did”", please consider renaming `parse_svg_with_html_parser_` to `should_parse_svg_with_html_parser_`. This would make it consistent with the vast majority of other boolean member variables in this class.
_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:** reason
This 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)_
Related details
- David Baron
- Mason Freed
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 3 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Auto-Submit | +1 |
The pinpoints didn't show a win, but this area does show up on profiles, so I wouldn't write off the speedometer3 win. I've kicked off another run on the m1 pgo bot to get a second opinion.
Here's profile data showing how much (and where) we spend time in this area: https://pprof.corp.google.com/?id=78bbb00af1fc8facfb90bfc6100c4ad5&pivot=IsolatedSVGDocumentHost&tab=flame (profile from all subbenchmarks).
https://issues.chromium.org/u/1/issues/394015877 is an open bug to investigate this area.
Both pinpoints had errors in their setup though. One didn't add the flag at all, and the other added it without `--extra-browser-args=` before the `--enable-blink-features`.
I've kicked off a duplicate one here that should have both things fixed:
https://pinpoint-dot-chromeperf.appspot.com/job/154eb917510000
Parse SVG with the HTML ParserThere is compat risk because this is common, though a hopeful aspect is that external svg docs are fairly new. WDYT of de-risking with data using https://www.chromium.org/blink/platform-predictability/compat-tools/? This question is a little out of scope for this review; can you file a bug linked to this for that discussion?
Yeah, that's a good thought. It might be a bit difficult to do though, since the failure case will just be an incorrect (visual) rendering.
Filed: crbug.com/452392022
- The new parser is only used in the special case of SVGs that areDoes this mean we would use the html parser for cases like:
<div style="filter: url(foo.svg)">
and
<use href="foo.svg">But not:
<img src="foo.svg">
and
http://domain.com/foo.svg?
<div style="filter: url(foo.svg)">
I believe this would use this path, since foo.svg is loaded as an SVGImage, correct?
<use href="foo.svg">
This would use this path, assuming `foo.svg` is a valid SVG (with a root `<svg>` element). Else this will hit the fallback path and get re-parsed with XML.
> <img src="foo.svg">
This should also use this new path, it creates an SVGImage.
Correct this would not use this path. This SVG can include `<script>` elements which execute, and can define namespaces in a way that the HTML parser doesn't understand, so this path purposely does not support that.
Related details
- David Baron
- Philip Rogers
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/154eb917510000
Related details
- David Baron
- Mason Freed
- Philip Rogers
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf-pgo/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/16beb917510000
Mason Freed (Gerrit)
Mason Freed voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mason Freed
| Auto-Submit | +1 |
Mason FreedThe pinpoints didn't show a win, but this area does show up on profiles, so I wouldn't write off the speedometer3 win. I've kicked off another run on the m1 pgo bot to get a second opinion.
Here's profile data showing how much (and where) we spend time in this area: https://pprof.corp.google.com/?id=78bbb00af1fc8facfb90bfc6100c4ad5&pivot=IsolatedSVGDocumentHost&tab=flame (profile from all subbenchmarks).
https://issues.chromium.org/u/1/issues/394015877 is an open bug to investigate this area.
Both pinpoints had errors in their setup though. One didn't add the flag at all, and the other added it without `--extra-browser-args=` before the `--enable-blink-features`.
I've kicked off a duplicate one here that should have both things fixed:
https://pinpoint-dot-chromeperf.appspot.com/job/154eb917510000
https://pinpoint-dot-chromeperf.appspot.com/job/154eb917510000
No significant change. Boo.
I just uploaded a new patch that removes the XML fallback, just to see how much that accounts for on speedometer. That'll be here: https://pinpoint-dot-chromeperf.appspot.com/job/1686f710d10000
Related details
- David Baron
- Philip Rogers
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. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1686f710d10000
Related details
- David Baron
- Mason Freed
- Philip Rogers
David Baron (Gerrit)
David Baron added 1 comment![]( "Open in Gerrit")
So this seems like a pretty scary change that changes some fundamental things.
My understanding is that this is specifically *only* for SVG-as-image and not for SVG as a toplevel resource / iframe / etc. It would be good to make that clearer in the first line of the commit message.
My initial inclination is that this is a big enough change to an existing standardized behavior that we wouldn't want to do this without a good bit more external discussion and hopefully consensus (and I think I personally would probably still lean against the change, since I find the divergence between SVG-as-document and SVG-as-image and the break with existing precedent to be harfmul enough that I don't think it's worth it for performance alone, particularly when there are other things we could do to get the same performance gains and that we might end up doing anyway). But I also feel like I probably shouldn't be the only decision maker here.
Was there an intent-to-prototype sent? What was the feedback for that I2P? If not... maybe sending an I2P is a good way to get the appropriate sort of feedback about whether this is something we should move forward with?
Mason Freed (Gerrit)
Mason Freed voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mason Freed
| Auto-Submit | +0 |
| Commit-Queue | +1 |
1 comment
dbaron@, something's up with this patch. Somewhere along my long series of patchsets I introduced a bug. I'll work on it, but for now I'm moving you to CC. Please do feel free to review this patch in concept, but perhaps not yet in detail.
Related details
- David Baron
- Mason Freed
- Philip Rogers
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. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/15527c90d10000
Related details
- David Baron
- Mason Freed
- Philip Rogers
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. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/16d58fd0d10000
Related details
- Mason Freed
- Philip Rogers
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. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 2 comments![]( "Open in Gerrit")
- The new parser is only used in the special case of SVGs that areMason FreedDoes this mean we would use the html parser for cases like:
<div style="filter: url(foo.svg)">
and
<use href="foo.svg">But not:
<img src="foo.svg">
and
http://domain.com/foo.svg?
<div style="filter: url(foo.svg)">
I believe this would use this path, since foo.svg is loaded as an SVGImage, correct?
<use href="foo.svg">
This would use this path, assuming `foo.svg` is a valid SVG (with a root `<svg>` element). Else this will hit the fallback path and get re-parsed with XML.
> <img src="foo.svg">This should also use this new path, it creates an SVGImage.
Correct this would not use this path. This SVG can include `<script>` elements which execute, and can define namespaces in a way that the HTML parser doesn't understand, so this path purposely does not support that.
<div style="filter: url(foo.svg)">
I believe this would use this path, since foo.svg is loaded as an SVGImage, correct?
Yes it would, but it's not loaded as an SVGImage. If that is the assumption the request policy ought to be passed to `IsolatedSVGDocumentHost` from `SVGImage` rather than be handled completely by the former.
<use href="foo.svg">
This would use this path, assuming `foo.svg` is a valid SVG (with a root `<svg>` element). Else this will hit the fallback path and get re-parsed with XML.
Ditto here.
return HTMLDocumentParser::Append("<html><body>" + source);Isn't relying on the regular HTML parser insertion of "missing" elements enough for this? (This basically means making a copy of the entire input, so it seems a bit counterproductive.)
Stephen Chenney (Gerrit)
Stephen Chenney added 1 comment![]( "Open in Gerrit")
- The new parser is only used in the special case of SVGs that areMason FreedDoes this mean we would use the html parser for cases like:
<div style="filter: url(foo.svg)">
and
<use href="foo.svg">But not:
<img src="foo.svg">
and
http://domain.com/foo.svg?
<div style="filter: url(foo.svg)">
I believe this would use this path, since foo.svg is loaded as an SVGImage, correct?
<use href="foo.svg">
This would use this path, assuming `foo.svg` is a valid SVG (with a root `<svg>` element). Else this will hit the fallback path and get re-parsed with XML.
> <img src="foo.svg">This should also use this new path, it creates an SVGImage.
Correct this would not use this path. This SVG can include `<script>` elements which execute, and can define namespaces in a way that the HTML parser doesn't understand, so this path purposely does not support that.
<div style="filter: url(foo.svg)">
I believe this would use this path, since foo.svg is loaded as an SVGImage, correct?
I would have thought so but I just wrote a test that does this (an overlooked case of privacy preserving filters) and the constructor for SVGImage is never called. I'll try to figure out how the content is actually loaded and parsed.
Stephen Chenney (Gerrit)
fs@ answered that. It's interesting that this content will not taint a filter, but then it doesn't matter because the content must be same origin and can't load further resources.
Mason Freed (Gerrit)
Mason Freed abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
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 restored this change![]( "Open in Gerrit")
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. |
Mason Freed (Gerrit)
Mason Freed added 2 comments![]( "Open in Gerrit")
I'm going to abandon this patch. I don't have the bandwidth, and people seem concerned.
return HTMLDocumentParser::Append("<html><body>" + source);Isn't relying on the regular HTML parser insertion of "missing" elements enough for this? (This basically means making a copy of the entire input, so it seems a bit counterproductive.)
Yep, you're right. At some point while working on this patch, that seemed to be needed, but I guess that wasn't actually the case. Removed.
Related details
- Fredrik Söderquist
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 abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |