[SVG] Support `ping`, `hreflang`, `type` and `referrerPolicy` for SVGAElement [chromium/src : main]
Divyansh Mangal (Gerrit)
Message from Divyansh Mangal![]( "Open in Gerrit")
Set Ready For Review
Related details
- Divyansh Mangal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt added 10 comments![]( "Open in Gerrit")
Looks great overall, mostly just questions.
// SVGAElement also has a `type` attribute for which lookup should be
// applied.Does `name` also need to be checked here?
if (auto* html_anchor = DynamicTo<HTMLAnchorElement>(anchor)) {
AnchorElementUtils::SendPings(
destination_url, html_anchor->GetDocument(),
html_anchor->FastGetAttribute(html_names::kPingAttr));
}Does this need another case for `SVGAnchorElement`?
// Pings should not be sent if MHTML page is loaded.
if (document.Fetcher()->Archive()) {
return;
}Are there any tests for this?
for (unsigned i = 0; i < ping_urls.size(); i++) {Not your code, but this should be `++i` per coding guidelines and `i` should be a `wtf_size_t`. Even better, you can avoid both with an iterator:
```
for (const auto& url : ping_urls) {
PingLoader::SendLinkAuditPing(document.GetFrame(),
document.CompleteURL(url),
destination_url);
}
}
```
if (RuntimeEnabledFeatures::SvgAnchorElementAttributesEnabled()) {You can put both of these under the same check for `SvgAnchorElementAttributesEnabled` so it's not being checked twice
if (window.location.origin != get_host_info().HTTPS_ORIGIN) {
window.location = get_host_info().HTTPS_ORIGIN + window.location.pathname;
} else {What is this doing, and when does it get hit?
var anchorElement = document.getElementById('click-me');Prefer `const` or `let` for these test variables so you get more intuitive scoping
if (hash.length > 0)
hash = hash.substring(1);This could use a comment. Is it trimming the `#`?
if (hash === "origin")
expected_referrer = "http://127.0.0.1:8000/";
else if (hash === "unsafe-url")
expected_referrer = "http://127.0.0.1:8000/security/referrer-policy-attribute-svg-anchor-unsafe-url.html";
else if (hash === "no-policy")
expected_referrer = "http://127.0.0.1:8000/";
else if (hash === "origin-when-crossorigin")
expected_referrer = "http://127.0.0.1:8000/";
else if (hash === "no-referrer" ||
hash === "no-referrer-when-downgrade")
expected_referrer = "";
else
throw new Error("Unexpected hash value!");This should be a `switch`
Related details
- Divyansh Mangal
- Vinay Singh
- Virali Purbey
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. |
Divyansh Mangal (Gerrit)
Divyansh Mangal added 9 comments![]( "Open in Gerrit")
// SVGAElement also has a `type` attribute for which lookup should be
// applied.Does `name` also need to be checked here?
Personally, I believe for each attribute of `SVGAElement` we should be able to get the attributes using this lookup, but I will keep this comment open and wait for owner's opinion as well.
if (auto* html_anchor = DynamicTo<HTMLAnchorElement>(anchor)) {
AnchorElementUtils::SendPings(
destination_url, html_anchor->GetDocument(),
html_anchor->FastGetAttribute(html_names::kPingAttr));
}Does this need another case for `SVGAnchorElement`?
Probably yes based on `EnclosingLinkEventParentOrSelf()`, but once again will wait for owner's opinion to confirm once.
// Pings should not be sent if MHTML page is loaded.
if (document.Fetcher()->Archive()) {
return;
}Are there any tests for this?
Yes, this particular code was added in `HTMLAnchorElement` because of a bug
CL:https://codereview.chromium.org/2848953003 fixes that and had tests as well
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/testing/data/frameserialization/remove_attributes.html;l=1;drc=9d6f972e353f541f4f3261fa971d20fa61d839bd?q=remove_attributes.html&sq=&ss=chromium%2Fchromium%2Fsrc
I mainly migrated the code and kept it as is.
for (unsigned i = 0; i < ping_urls.size(); i++) {Not your code, but this should be `++i` per coding guidelines and `i` should be a `wtf_size_t`. Even better, you can avoid both with an iterator:
```
for (const auto& url : ping_urls) {
PingLoader::SendLinkAuditPing(document.GetFrame(),
document.CompleteURL(url),
destination_url);
}
}
```
Done
if (RuntimeEnabledFeatures::SvgAnchorElementAttributesEnabled()) {You can put both of these under the same check for `SvgAnchorElementAttributesEnabled` so it's not being checked twice
Done
if (window.location.origin != get_host_info().HTTPS_ORIGIN) {
window.location = get_host_info().HTTPS_ORIGIN + window.location.pathname;
} else {What is this doing, and when does it get hit?
Hi @ksc...@microsoft.com, I mainly followed the same pattern for html anchor element downgrade referrer policy third_party/blink/web_tests/http/tests/security/referrer-policy-attribute-anchor-no-referrer-when-downgrade.html
That being said based on my findings this piece of code gets triggered when the test is initially loaded over HTTP (e.g., http://127.0.0.1:8000) instead of HTTPS.
The test needs to run on HTTPS because:
1) The test validates "no-referrer-when-downgrade" policy - which specifically tests the behavior when navigating from HTTPS → HTTP
2) The SVG anchor targets an HTTP URL (http://127.0.0.1:8000/security/resources/referrer-attr-svg-anchor-target.html)
3) For the referrer policy to properly test "downgrade" behavior, the initial page must be on HTTPS so that the navigation to HTTP constitutes a downgrade
Once again, this is the same pattern for html anchor element, iframe, img etc (elements which supports referrerPolicy attribute) and I believe we should follow the same for SVGAelement as well.
Let me know if you want to discuss this further.
var anchorElement = document.getElementById('click-me');Prefer `const` or `let` for these test variables so you get more intuitive scoping
Done
This could use a comment. Is it trimming the `#`?
Yes you are right, added the comment as you suggested.
Just mentioning though, I mainly followed the pattern in existing tests like
third_party/blink/web_tests/http/tests/security/resources/referrer-attr-anchor-target.html for html anchor element.
if (hash === "origin")
expected_referrer = "http://127.0.0.1:8000/";
else if (hash === "unsafe-url")
expected_referrer = "http://127.0.0.1:8000/security/referrer-policy-attribute-svg-anchor-unsafe-url.html";
else if (hash === "no-policy")
expected_referrer = "http://127.0.0.1:8000/";
else if (hash === "origin-when-crossorigin")
expected_referrer = "http://127.0.0.1:8000/";
else if (hash === "no-referrer" ||
hash === "no-referrer-when-downgrade")
expected_referrer = "";
else
throw new Error("Unexpected hash value!");This should be a `switch`
- Kurt Catti-Schmidt
- Vinay Singh
- Virali Purbey
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. |
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt added 4 comments![]( "Open in Gerrit")
Overall looks good but there are still a few open questions. Adding @f...@opera.com, but heads up that he's out until tomorrow.
// SVGAElement also has a `type` attribute for which lookup should be
// applied.Divyansh MangalDoes `name` also need to be checked here?
Personally, I believe for each attribute of `SVGAElement` we should be able to get the attributes using this lookup, but I will keep this comment open and wait for owner's opinion as well.
Yeah that's fine, let's see what Fredrick's take on this is because I'm not familiar with fast attribute lookup. I wonder if there are more opportunities to expand this for more SVG attributes.
if (window.location.origin != get_host_info().HTTPS_ORIGIN) {
window.location = get_host_info().HTTPS_ORIGIN + window.location.pathname;
} else {Divyansh MangalWhat is this doing, and when does it get hit?
Hi @ksc...@microsoft.com, I mainly followed the same pattern for html anchor element downgrade referrer policy third_party/blink/web_tests/http/tests/security/referrer-policy-attribute-anchor-no-referrer-when-downgrade.html
That being said based on my findings this piece of code gets triggered when the test is initially loaded over HTTP (e.g., http://127.0.0.1:8000) instead of HTTPS.
The test needs to run on HTTPS because:1) The test validates "no-referrer-when-downgrade" policy - which specifically tests the behavior when navigating from HTTPS → HTTP
2) The SVG anchor targets an HTTP URL (http://127.0.0.1:8000/security/resources/referrer-attr-svg-anchor-target.html)
3) For the referrer policy to properly test "downgrade" behavior, the initial page must be on HTTPS so that the navigation to HTTP constitutes a downgrade
Once again, this is the same pattern for html anchor element, iframe, img etc (elements which supports referrerPolicy attribute) and I believe we should follow the same for SVGAelement as well.
Let me know if you want to discuss this further.
That makes sense, I appreciate the thorough investigation. So does that trigger a navigate that will rerun the test under https and hit the `else` block? Because if not, this looks like dead code that's not testing anything, hence my original question.
Can you add a comment so this is clear? Something like:
```
// Test downgrade behavior for non-https navigations. This will initiate a navigation that will run the `else` block below.
```
if (hash.length > 0)
hash = hash.substring(1);Divyansh MangalThis could use a comment. Is it trimming the `#`?
Yes you are right, added the comment as you suggested.
Just mentioning though, I mainly followed the pattern in existing tests like
third_party/blink/web_tests/http/tests/security/resources/referrer-attr-anchor-target.html for html anchor element.
I appreciate all the cleanups. It's frustrating when existing code that you move doesn't follow conventions but changes like this are a good opportunity for improvement.
Related details
- Divyansh Mangal
- Fredrik Söderquist
- Vinay Singh
- Virali Purbey
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. |
Divyansh Mangal (Gerrit)
Divyansh Mangal added 1 comment![]( "Open in Gerrit")
if (window.location.origin != get_host_info().HTTPS_ORIGIN) {
window.location = get_host_info().HTTPS_ORIGIN + window.location.pathname;
} else {Divyansh MangalWhat is this doing, and when does it get hit?
Kurt Catti-SchmidtHi @ksc...@microsoft.com, I mainly followed the same pattern for html anchor element downgrade referrer policy third_party/blink/web_tests/http/tests/security/referrer-policy-attribute-anchor-no-referrer-when-downgrade.html
That being said based on my findings this piece of code gets triggered when the test is initially loaded over HTTP (e.g., http://127.0.0.1:8000) instead of HTTPS.
The test needs to run on HTTPS because:1) The test validates "no-referrer-when-downgrade" policy - which specifically tests the behavior when navigating from HTTPS → HTTP
2) The SVG anchor targets an HTTP URL (http://127.0.0.1:8000/security/resources/referrer-attr-svg-anchor-target.html)
3) For the referrer policy to properly test "downgrade" behavior, the initial page must be on HTTPS so that the navigation to HTTP constitutes a downgrade
Once again, this is the same pattern for html anchor element, iframe, img etc (elements which supports referrerPolicy attribute) and I believe we should follow the same for SVGAelement as well.
Let me know if you want to discuss this further.
That makes sense, I appreciate the thorough investigation. So does that trigger a navigate that will rerun the test under https and hit the `else` block? Because if not, this looks like dead code that's not testing anything, hence my original question.
Can you add a comment so this is clear? Something like:
```
// Test downgrade behavior for non-https navigations. This will initiate a navigation that will run the `else` block below.
```
Yes your understanding is correct. The else block will run the second time.
Incorporated the comment as per suggestion, Thanks!
Related details
- Fredrik Söderquist
- Kurt Catti-Schmidt
- Vinay Singh
- Virali Purbey
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 6 comments![]( "Open in Gerrit")
// SVGAElement also has a `type` attribute for which lookup should be
// applied.Divyansh MangalDoes `name` also need to be checked here?
Kurt Catti-SchmidtPersonally, I believe for each attribute of `SVGAElement` we should be able to get the attributes using this lookup, but I will keep this comment open and wait for owner's opinion as well.
Yeah that's fine, let's see what Fredrick's take on this is because I'm not familiar with fast attribute lookup. I wonder if there are more opportunities to expand this for more SVG attributes.
This ought to be handled by overriding `IsAnimatableAttribute()` and checking that there. See `SVGScriptElement`.
if (auto* html_anchor = DynamicTo<HTMLAnchorElement>(anchor)) {
AnchorElementUtils::SendPings(
destination_url, html_anchor->GetDocument(),
html_anchor->FastGetAttribute(html_names::kPingAttr));
}Divyansh MangalDoes this need another case for `SVGAnchorElement`?
Fredrik SöderquistProbably yes based on `EnclosingLinkEventParentOrSelf()`, but once again will wait for owner's opinion to confirm once.
Yes, I would say so.
!HasRel(link_relations, kRelationNoReferrer)) {Could move this before the parsing.
}, "SVG anchor ping attribute should be settable via DOM property");...ping content attribute should be settable via ping IDL attribute
(or some such)
We should make these (==this and the following tests) into WPTs (maybe there are existing WPTs for `<html:a>`?
Related details
- Divyansh Mangal
Divyansh Mangal (Gerrit)
Divyansh Mangal added 6 comments![]( "Open in Gerrit")
// SVGAElement also has a `type` attribute for which lookup should be
// applied.Divyansh MangalDoes `name` also need to be checked here?
Kurt Catti-SchmidtPersonally, I believe for each attribute of `SVGAElement` we should be able to get the attributes using this lookup, but I will keep this comment open and wait for owner's opinion as well.
Fredrik SöderquistYeah that's fine, let's see what Fredrick's take on this is because I'm not familiar with fast attribute lookup. I wonder if there are more opportunities to expand this for more SVG attributes.
This ought to be handled by overriding `IsAnimatableAttribute()` and checking that there. See `SVGScriptElement`.
Done
if (auto* html_anchor = DynamicTo<HTMLAnchorElement>(anchor)) {
AnchorElementUtils::SendPings(
destination_url, html_anchor->GetDocument(),
html_anchor->FastGetAttribute(html_names::kPingAttr));
}Divyansh MangalDoes this need another case for `SVGAnchorElement`?
Fredrik SöderquistProbably yes based on `EnclosingLinkEventParentOrSelf()`, but once again will wait for owner's opinion to confirm once.
Yes, I would say so.
Done
unsigned link_relations,Divyansh Mangal`uint32_t`
Done
Could move this before the parsing.
Done
}, "SVG anchor ping attribute should be settable via DOM property");...ping content attribute should be settable via ping IDL attribute
(or some such)
Done
We should make these (==this and the following tests) into WPTs (maybe there are existing WPTs for `<html:a>`?
The <html:a> element for referrerpolicy primarily had web_tests, which were introduced in the patchset:
https://codereview.chromium.org/1253413003
That being said, I tried my best to convert them to WPTs for SVGAElement. I do have to work around for host name due to linting errors, especially with the downgrade test but I was able to convert them to working wpts.
Let me know what you think of them, should I remove the web_tests one as they seem to be redundant now for SVGAElement?
Related details
- Fredrik Söderquist
- Kurt Catti-Schmidt
- Vinay Singh
- Virali Purbey
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. |