Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1501393003:
Deprecate fetching stylesheets with unsupported type (Closed)
|
Created:
5 years ago by suzyh_UTC10 (ex-contributor) Modified:
4 years, 11 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, gavinp+prerender_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDeprecate fetching stylesheets with unsupported type
The spec http://www.w3.org/TR/html5/document-metadata.html#the-link-element
specifies that "If the UA does not support the given MIME type for the given
link relationship, then the UA should not obtain the resource".
The linked bug contains a testcase that illustrates that Blink is not currently
doing this, and is obtaining the resource even if the link type is e.g.
"application/javascript".
This patch translates the testcase from the linked bug into Blink layout tests
and implements a deprecation warning ahead of implementing the suggested fix.
For more background and discussion, see
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/nH1O6WszMgo/discussion
BUG=286682
Committed: https://crrev.com/db6584f5ba2d6cda49fa42775903399cd4d427d6
Cr-Commit-Position: refs/heads/master@{#370531}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Updated tests, converted from fix to deprecation warning #
Total comments: 2
Patch Set 3 : Fix MIME type comparison, update deprecation message #Patch Set 4 : Make deprecation warning dependent on isStyleSheet check #Patch Set 5 : Rebase #Messages
Total messages: 51 (19 generated)
suzyh@chromium.org changed reviewers: + yoav@yoav.ws
Hi, I was unsure who would be a good person to review this patch. If you are not best placed to review, I'd appreciate a suggestion for who to send it to instead! Thanks, Suzy
On 2015/12/08 23:24:27, suzyh wrote: > Hi, > > I was unsure who would be a good person to review this patch. If you are not > best placed to review, I'd appreciate a suggestion for who to send it to > instead! > > Thanks, > Suzy Hi Suzy, Yeah I can review the CL, no problem :) The code looks good in and of itself. I would add more non-dataURI based tests that make sure that a resource is not downloaded. I believe that in order for that to happen you would also need to do some HTMLPreloadScanner changes as well. Otherwise, I think that landing this CL would require an "Intent to Implement and Ship" email to blink-dev, to announce this highly-exposed change and make sure it we are confident it will not break content.
On 2015/12/09 07:13:12, Yoav Weiss wrote: > On 2015/12/08 23:24:27, suzyh wrote: > > Hi, > > > > I was unsure who would be a good person to review this patch. If you are not > > best placed to review, I'd appreciate a suggestion for who to send it to > > instead! > > > > Thanks, > > Suzy > > Hi Suzy, > > Yeah I can review the CL, no problem :) > > The code looks good in and of itself. I would add more non-dataURI based tests > that make sure that a resource is not downloaded. > I believe that in order for that to happen you would also need to do some > HTMLPreloadScanner changes as well. Specifically, you probably want to take a look at TokenPreloadScanner::processLinkAttribute > > Otherwise, I think that landing this CL would require an "Intent to Implement > and Ship" email to blink-dev, to announce this highly-exposed change and make > sure it we are confident it will not break content.
On 2015/12/09 07:13:12, Yoav Weiss wrote: > On 2015/12/08 23:24:27, suzyh wrote: > > Hi, > > > > I was unsure who would be a good person to review this patch. If you are not > > best placed to review, I'd appreciate a suggestion for who to send it to > > instead! > > > > Thanks, > > Suzy > > Hi Suzy, > > Yeah I can review the CL, no problem :) > > The code looks good in and of itself. I would add more non-dataURI based tests > that make sure that a resource is not downloaded. > I believe that in order for that to happen you would also need to do some > HTMLPreloadScanner changes as well. > > Otherwise, I think that landing this CL would require an "Intent to Implement > and Ship" email to blink-dev, to announce this highly-exposed change and make > sure it we are confident it will not break content. There's obviously some compat risk here so we need to be toughtful of that. Can you characterize it at all? Eg. it sounds like Mozilla already has this behavior, what about Edge? How might this behavior change be likely to impact a page - eg. maybe "<link rel=stylesheet type=text/webkit-only-css href=webkit.css>"? Or is it more the side-effects of resource loading that we're concerned with (eg. imagine an Android WebView or Chrome-specific app exploiting this instead of a 1px image as a form of beacon). Big picture as long as we have reason to believe the risk is low (and we're prepared to revert if we see breakage that suggests otherwise), I'd personally be fine considering this a "trivial" change (almost every bug-fix we do has the potential to be a breaking web exposed change). A "web-facing PSA" to blink-dev wouldn't hurt though, might flesh out reasons to be more concerned about the compat risk.
On 2015/12/09 21:26:11, Rick Byers wrote: > On 2015/12/09 07:13:12, Yoav Weiss wrote: > > On 2015/12/08 23:24:27, suzyh wrote: > > > Hi, > > > > > > I was unsure who would be a good person to review this patch. If you are not > > > best placed to review, I'd appreciate a suggestion for who to send it to > > > instead! > > > > > > Thanks, > > > Suzy > > > > Hi Suzy, > > > > Yeah I can review the CL, no problem :) > > > > The code looks good in and of itself. I would add more non-dataURI based tests > > that make sure that a resource is not downloaded. > > I believe that in order for that to happen you would also need to do some > > HTMLPreloadScanner changes as well. > > > > Otherwise, I think that landing this CL would require an "Intent to Implement > > and Ship" email to blink-dev, to announce this highly-exposed change and make > > sure it we are confident it will not break content. > > There's obviously some compat risk here so we need to be toughtful of that. Can > you characterize it at all? Eg. it sounds like Mozilla already has this > behavior, what about Edge? How might this behavior change be likely to impact a > page - eg. maybe "<link rel=stylesheet type=text/webkit-only-css > href=webkit.css>"? Or is it more the side-effects of resource loading that > we're concerned with (eg. imagine an Android WebView or Chrome-specific app > exploiting this instead of a 1px image as a form of beacon). > > Big picture as long as we have reason to believe the risk is low (and we're > prepared to revert if we see breakage that suggests otherwise), I'd personally > be fine considering this a "trivial" change (almost every bug-fix we do has the > potential to be a breaking web exposed change). A "web-facing PSA" to blink-dev > wouldn't hurt though, might flesh out reasons to be more concerned about the > compat risk. Would also be helpful to try and gather data regarding usage through, HTTPArchive, GitHub search or use counters, to increase our confidence that this won't end up blowing in our faces :)
Thank you both for the input, and apologies for the delay in responding. > > > The code looks good in and of itself. I would add more non-dataURI based tests > > > that make sure that a resource is not downloaded. > > > I believe that in order for that to happen you would also need to do some > > > HTMLPreloadScanner changes as well. Will do. I'm still new to Blink; can you point me towards some tests that I can model mine on? > > > Otherwise, I think that landing this CL would require an "Intent to Implement > > > and Ship" email to blink-dev, to announce this highly-exposed change and make > > > sure it we are confident it will not break content. > > > > There's obviously some compat risk here so we need to be toughtful of that. Can > > you characterize it at all? Eg. it sounds like Mozilla already has this > > behavior, what about Edge? How might this behavior change be likely to impact a > > page - eg. maybe "<link rel=stylesheet type=text/webkit-only-css > > href=webkit.css>"? Or is it more the side-effects of resource loading that > > we're concerned with (eg. imagine an Android WebView or Chrome-specific app > > exploiting this instead of a 1px image as a form of beacon). > > > > Big picture as long as we have reason to believe the risk is low (and we're > > prepared to revert if we see breakage that suggests otherwise), I'd personally > > be fine considering this a "trivial" change (almost every bug-fix we do has the > > potential to be a breaking web exposed change). A "web-facing PSA" to blink-dev > > wouldn't hurt though, might flesh out reasons to be more concerned about the > > compat risk. I've been able to establish that Firefox, IE and Edge pass the test (do not fetch the resource), while Chrome and Safari fail (fetch the resource). Web-facing PSA sent to blink-dev: https://groups.google.com/a/chromium.org/d/topic/blink-dev/nH1O6WszMgo/discus... > Would also be helpful to try and gather data regarding usage through, HTTPArchive, GitHub search or use counters, to increase our confidence that this won't end up blowing in our faces :) I couldn't figure out how to collect this data from HTTPArchive, GitHub or code.google.com; there doesn't seem to be a mechanism for regex code search there. Since it seems that Blink/WebKit are the odd ones out for this behaviour, do you think it's worth the effort to add a use counter to get numbers for this behaviour, or are you happy for us to just make this change and be prepared to revert if the impact is worse than expected?
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
https://codereview.chromium.org/1501393003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLLinkElement.cpp (right): https://codereview.chromium.org/1501393003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLLinkElement.cpp:594: bool LinkStyle::styleSheetTypeIsSupported(const String& type) const This is static it doesn't need to be a method, move it to be static in the cpp file.
Description was changed from ========== Fetch external resource only if type is supported The spec http://www.w3.org/TR/html5/document-metadata.html#the-link-element specifies that "If the UA does not support the given MIME type for the given link relationship, then the UA should not obtain the resource". The linked bug contains a testcase that illustrates that Blink is not currently doing this, and is obtaining the resource even if the link type is e.g. "application/javascript". This patch translates the testcase from the linked bug into a Blink layout test and implements the suggested fix so that the test passes. BUG=286682 ========== to ========== Fetch external resource only if type is supported The spec http://www.w3.org/TR/html5/document-metadata.html#the-link-element specifies that "If the UA does not support the given MIME type for the given link relationship, then the UA should not obtain the resource". The linked bug contains a testcase that illustrates that Blink is not currently doing this, and is obtaining the resource even if the link type is e.g. "application/javascript". This patch translates the testcase from the linked bug into a Blink layout test and implements the suggested fix so that the test passes. BUG=286682 ==========
On 2015/12/17 03:48:10, suzyh wrote: > Thank you both for the input, and apologies for the delay in responding. > > > > > The code looks good in and of itself. I would add more non-dataURI based > tests > > > > that make sure that a resource is not downloaded. > > > > I believe that in order for that to happen you would also need to do some > > > > HTMLPreloadScanner changes as well. > > Will do. I'm still new to Blink; can you point me towards some tests that I can > model mine on? > > > > > Otherwise, I think that landing this CL would require an "Intent to > Implement > > > > and Ship" email to blink-dev, to announce this highly-exposed change and > make > > > > sure it we are confident it will not break content. > > > > > > There's obviously some compat risk here so we need to be toughtful of that. > Can > > > you characterize it at all? Eg. it sounds like Mozilla already has this > > > behavior, what about Edge? How might this behavior change be likely to > impact a > > > page - eg. maybe "<link rel=stylesheet type=text/webkit-only-css > > > href=webkit.css>"? Or is it more the side-effects of resource loading that > > > we're concerned with (eg. imagine an Android WebView or Chrome-specific app > > > exploiting this instead of a 1px image as a form of beacon). > > > > > > Big picture as long as we have reason to believe the risk is low (and we're > > > prepared to revert if we see breakage that suggests otherwise), I'd > personally > > > be fine considering this a "trivial" change (almost every bug-fix we do has > the > > > potential to be a breaking web exposed change). A "web-facing PSA" to > blink-dev > > > wouldn't hurt though, might flesh out reasons to be more concerned about the > > > compat risk. > > I've been able to establish that Firefox, IE and Edge pass the test (do not > fetch the resource), while Chrome and Safari fail (fetch the resource). > > Web-facing PSA sent to blink-dev: > https://groups.google.com/a/chromium.org/d/topic/blink-dev/nH1O6WszMgo/discus... > > > Would also be helpful to try and gather data regarding usage through, > HTTPArchive, GitHub search or use counters, to increase our confidence that this > won't end up blowing in our faces :) > > I couldn't figure out how to collect this data from HTTPArchive, GitHub or > http://code.google.com; there doesn't seem to be a mechanism for regex code search > there. There's a BigQuery interface that includes HTTPArchive's response bodies. igrigorik (CCed) could probably tell you more about it's current state, but here's a tutorial he wrote in 2013: https://www.igvita.com/2013/06/20/http-archive-bigquery-web-performance-answers/ > > Since it seems that Blink/WebKit are the odd ones out for this behaviour, do you > think it's worth the effort to add a use counter to get numbers for this > behaviour Like Rick said, it wouldn't be surprising to see this used in the wild as a "Mobile only stylesheets". > , or are you happy for us to just make this change and be prepared to > revert if the impact is worse than expected? That's a question for the API owners. Rick?
On 2015/12/17 03:48:10, suzyh wrote: > Thank you both for the input, and apologies for the delay in responding. > > > > > The code looks good in and of itself. I would add more non-dataURI based > tests > > > > that make sure that a resource is not downloaded. > > > > I believe that in order for that to happen you would also need to do some > > > > HTMLPreloadScanner changes as well. > > Will do. I'm still new to Blink; can you point me towards some tests that I can > model mine on? http/tests/preload/link_header_preload.php is one I recently wrote that you can build on. Basically, the test looks into ResourceTiming data to see if a resource was downloaded or not. Also, feel free to ping me if you need help with the HTMLPreloadScanner changes.
On 2015/12/17 07:59:57, Yoav Weiss wrote: > On 2015/12/17 03:48:10, suzyh wrote: > > Thank you both for the input, and apologies for the delay in responding. > > > > > > > The code looks good in and of itself. I would add more non-dataURI based > > tests > > > > > that make sure that a resource is not downloaded. > > > > > I believe that in order for that to happen you would also need to do > some > > > > > HTMLPreloadScanner changes as well. > > > > Will do. I'm still new to Blink; can you point me towards some tests that I > can > > model mine on? > > > > > > > Otherwise, I think that landing this CL would require an "Intent to > > Implement > > > > > and Ship" email to blink-dev, to announce this highly-exposed change and > > make > > > > > sure it we are confident it will not break content. > > > > > > > > There's obviously some compat risk here so we need to be toughtful of > that. > > Can > > > > you characterize it at all? Eg. it sounds like Mozilla already has this > > > > behavior, what about Edge? How might this behavior change be likely to > > impact a > > > > page - eg. maybe "<link rel=stylesheet type=text/webkit-only-css > > > > href=webkit.css>"? Or is it more the side-effects of resource loading > that > > > > we're concerned with (eg. imagine an Android WebView or Chrome-specific > app > > > > exploiting this instead of a 1px image as a form of beacon). > > > > > > > > Big picture as long as we have reason to believe the risk is low (and > we're > > > > prepared to revert if we see breakage that suggests otherwise), I'd > > personally > > > > be fine considering this a "trivial" change (almost every bug-fix we do > has > > the > > > > potential to be a breaking web exposed change). A "web-facing PSA" to > > blink-dev > > > > wouldn't hurt though, might flesh out reasons to be more concerned about > the > > > > compat risk. > > > > I've been able to establish that Firefox, IE and Edge pass the test (do not > > fetch the resource), while Chrome and Safari fail (fetch the resource). > > > > Web-facing PSA sent to blink-dev: > > > https://groups.google.com/a/chromium.org/d/topic/blink-dev/nH1O6WszMgo/discus... > > > > > Would also be helpful to try and gather data regarding usage through, > > HTTPArchive, GitHub search or use counters, to increase our confidence that > this > > won't end up blowing in our faces :) > > > > I couldn't figure out how to collect this data from HTTPArchive, GitHub or > > http://code.google.com; there doesn't seem to be a mechanism for regex code > search > > there. > > There's a BigQuery interface that includes HTTPArchive's response bodies. > igrigorik (CCed) could probably tell you more about it's current state, but > here's a tutorial he wrote in 2013: > https://www.igvita.com/2013/06/20/http-archive-bigquery-web-performance-answers/ There is Oct + Nov data in new "har" dataset that you can search. Here's a quick example: SELECT * FROM ( SELECT JSON_EXTRACT(payload, '$.request.url') AS url, JSON_EXTRACT(payload, '$._contentType') AS contentType, JSON_EXTRACT(payload, '$.response.content.text') AS content, INTEGER(JSON_EXTRACT(payload, '$.response.content.size')) as contentSize, JSON_EXTRACT(payload, '$._body') AS body, FROM [httparchive:har.desktop_oct_1_2015_requests] ) WHERE body = 'true' AND body CONTAINS 'some text'
> > Since it seems that Blink/WebKit are the odd ones out for this behaviour, do you > > think it's worth the effort to add a use counter to get numbers for this > > behaviour > > Like Rick said, it wouldn't be surprising to see this used in the wild as a "Mobile only stylesheets". > > > , or are you happy for us to just make this change and be prepared to > > revert if the impact is worse than expected? > > That's a question for the API owners. Rick? Actually making sure rbyers is on the cc list for this patch this time... > > There's a BigQuery interface that includes HTTPArchive's response bodies. > > igrigorik (CCed) could probably tell you more about it's current state, but > > here's a tutorial he wrote in 2013: > > https://www.igvita.com/2013/06/20/http-archive-bigquery-web-performance-answers/ > > There is Oct + Nov data in new "har" dataset that you can search. Here's a quick example: Thanks, Ilya. Unfortunately I get a permissions error when I try to run your example: """ Error: Access Denied: Table httparchive:har.desktop_oct_1_2015_requests: The user does not have permission to query a table in dataset httparchive:har """ Is that dataset public?
On 2015/12/18 02:30:35, suzyh wrote: > > > Since it seems that Blink/WebKit are the odd ones out for this behaviour, do > you > > > think it's worth the effort to add a use counter to get numbers for this > > > behaviour > > > > Like Rick said, it wouldn't be surprising to see this used in the wild as a > "Mobile only stylesheets". > > > > > , or are you happy for us to just make this change and be prepared to > > > revert if the impact is worse than expected? > > > > That's a question for the API owners. Rick? > > Actually making sure rbyers is on the cc list for this patch this time... Ah, sorry - I should have added myself. If we added a use counter and it showed non-trivial usage of non-css stylesheet types, what would that tell us? Wouldn't it be likely that much of that usage wouldn't actually impact page behavior (stuff left in by accident, bugs, etc)? Anyway this feels like just a bugfix to me. The fact that Edge behaves this way makes it unlikely to be a serious compat issue. I personally think you should just land it and keep an eye out for reports of issues (it helps knowing that Cr-Blink-CSS gets great triage coverage!). Nit: would you mind updating the CL summary to clarify somehow that this is limited just to <link rel=stylesheet>? Eg: "Don't fetch stylesheets with an unsupported type" > > > > > There's a BigQuery interface that includes HTTPArchive's response bodies. > > > igrigorik (CCed) could probably tell you more about it's current state, but > > > here's a tutorial he wrote in 2013: > > > > https://www.igvita.com/2013/06/20/http-archive-bigquery-web-performance-answers/ > > > > There is Oct + Nov data in new "har" dataset that you can search. Here's a > quick example: > > Thanks, Ilya. Unfortunately I get a permissions error when I try to run your > example: > """ > Error: Access Denied: Table httparchive:har.desktop_oct_1_2015_requests: The > user does not have permission to query a table in dataset httparchive:har > """ > Is that dataset public?
rbyers@chromium.org changed reviewers: + rbyers@chromium.org
https://codereview.chromium.org/1501393003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/loader/link-load-only-supported-stylesheet-types.html (right): https://codereview.chromium.org/1501393003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/loader/link-load-only-supported-stylesheet-types.html:4: <link rel="stylesheet" type="application/javascript" href="data:text/css,body { color: red !important; }"> nit: for completeness can you also add test cases for type="text/css" and no type attribute specified (eg. maybe make each stylesheet change the color of a different div).
Description was changed from ========== Fetch external resource only if type is supported The spec http://www.w3.org/TR/html5/document-metadata.html#the-link-element specifies that "If the UA does not support the given MIME type for the given link relationship, then the UA should not obtain the resource". The linked bug contains a testcase that illustrates that Blink is not currently doing this, and is obtaining the resource even if the link type is e.g. "application/javascript". This patch translates the testcase from the linked bug into a Blink layout test and implements the suggested fix so that the test passes. BUG=286682 ========== to ========== Deprecate fetching stylesheets with unsupported type The spec http://www.w3.org/TR/html5/document-metadata.html#the-link-element specifies that "If the UA does not support the given MIME type for the given link relationship, then the UA should not obtain the resource". The linked bug contains a testcase that illustrates that Blink is not currently doing this, and is obtaining the resource even if the link type is e.g. "application/javascript". This patch translates the testcase from the linked bug into Blink layout tests and implements a deprecation warning ahead of implementing the suggested fix. BUG=286682 ==========
Following from the discussion on the associated PSA thread, I've converted this patch from a fix into a deprecation warning. I'm still missing the chromestatus.com feature number for the deprecation warning; I'll get that sorted out in the next day or so, but thought I would send this off for your review in the meantime. I've added additional tests as requested, but with the expectation as a text file instead, to capture the console warning. I think it would be good to have the HTML expectation as well to show the colour of the text, but I haven't yet worked out how to have both. Yoav, I ended up writing non-dataURI based tests along the same lines as the test I already had, rather than the link_header_preload.php test that you suggested. Is this sufficient, or do you think I need to revisit this? https://codereview.chromium.org/1501393003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/loader/link-load-only-supported-stylesheet-types.html (right): https://codereview.chromium.org/1501393003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/loader/link-load-only-supported-stylesheet-types.html:4: <link rel="stylesheet" type="application/javascript" href="data:text/css,body { color: red !important; }"> On 2015/12/21 at 19:58:41, Rick Byers wrote: > nit: for completeness can you also add test cases for type="text/css" and no type attribute specified (eg. maybe make each stylesheet change the color of a different div). Done. https://codereview.chromium.org/1501393003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLLinkElement.cpp (right): https://codereview.chromium.org/1501393003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLLinkElement.cpp:594: bool LinkStyle::styleSheetTypeIsSupported(const String& type) const On 2015/12/17 at 07:07:07, esprehn wrote: > This is static it doesn't need to be a method, move it to be static in the cpp file. Done.
The tests look fine for a deprecation warning. Once we'd actually remove support, we'd need to make sure that the styles are not applied, at well that the resources are not being downloaded. LGTM % mimetype comparison comments https://codereview.chromium.org/1501393003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLLinkElement.cpp (right): https://codereview.chromium.org/1501393003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLLinkElement.cpp:131: return type.isEmpty() || type.contains("text/css"); This should be a case agnostic comparison, and should use startsWith rather than contains. Otherwise, it'd be nice to plumb this through platform/MimeTypeRegistry and simple_webmimregistry_impl and eventually components/mime_util/mime_util.c I don't think it's critical to do that in current CL, and we want to push the deprecation for 49, but maybe add a TODO and plumb everything for the next step?
LGTM (modulo Yoav's nit)
On 2016/01/18 18:53:56, Rick Byers wrote: > LGTM (modulo Yoav's nit) Oh please also add a link to the blink-dev discussion in the CL description/
Description was changed from ========== Deprecate fetching stylesheets with unsupported type The spec http://www.w3.org/TR/html5/document-metadata.html#the-link-element specifies that "If the UA does not support the given MIME type for the given link relationship, then the UA should not obtain the resource". The linked bug contains a testcase that illustrates that Blink is not currently doing this, and is obtaining the resource even if the link type is e.g. "application/javascript". This patch translates the testcase from the linked bug into Blink layout tests and implements a deprecation warning ahead of implementing the suggested fix. BUG=286682 ========== to ========== Deprecate fetching stylesheets with unsupported type The spec http://www.w3.org/TR/html5/document-metadata.html#the-link-element specifies that "If the UA does not support the given MIME type for the given link relationship, then the UA should not obtain the resource". The linked bug contains a testcase that illustrates that Blink is not currently doing this, and is obtaining the resource even if the link type is e.g. "application/javascript". This patch translates the testcase from the linked bug into Blink layout tests and implements a deprecation warning ahead of implementing the suggested fix. For more background and discussion, see https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/nH1O6WszMgo/... BUG=286682 ==========
Description was changed from ========== Deprecate fetching stylesheets with unsupported type The spec http://www.w3.org/TR/html5/document-metadata.html#the-link-element specifies that "If the UA does not support the given MIME type for the given link relationship, then the UA should not obtain the resource". The linked bug contains a testcase that illustrates that Blink is not currently doing this, and is obtaining the resource even if the link type is e.g. "application/javascript". This patch translates the testcase from the linked bug into Blink layout tests and implements a deprecation warning ahead of implementing the suggested fix. For more background and discussion, see https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/nH1O6WszMgo/... BUG=286682 ========== to ========== Deprecate fetching stylesheets with unsupported type The spec http://www.w3.org/TR/html5/document-metadata.html#the-link-element specifies that "If the UA does not support the given MIME type for the given link relationship, then the UA should not obtain the resource". The linked bug contains a testcase that illustrates that Blink is not currently doing this, and is obtaining the resource even if the link type is e.g. "application/javascript". This patch translates the testcase from the linked bug into Blink layout tests and implements a deprecation warning ahead of implementing the suggested fix. For more background and discussion, see https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/nH1O6WszMgo/... BUG=286682 ==========
Thanks! PTAL at the MIME type comparison. Shane advised me that I don't need to add a feature to chromestatus for this, so I've changed the deprecation warning to not include that part. On 2016/01/18 at 18:54:17, rbyers wrote: > On 2016/01/18 18:53:56, Rick Byers wrote: > > LGTM (modulo Yoav's nit) > > Oh please also add a link to the blink-dev discussion in the CL description/ Done. https://codereview.chromium.org/1501393003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLLinkElement.cpp (right): https://codereview.chromium.org/1501393003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLLinkElement.cpp:131: return type.isEmpty() || type.contains("text/css"); On 2016/01/18 at 08:21:18, Yoav Weiss wrote: > This should be a case agnostic comparison, and should use startsWith rather than contains. > > Otherwise, it'd be nice to plumb this through platform/MimeTypeRegistry and simple_webmimregistry_impl and eventually components/mime_util/mime_util.c > > I don't think it's critical to do that in current CL, and we want to push the deprecation for 49, but maybe add a TODO and plumb everything for the next step? I've plumbed it through platform/MimeTypeRegistry and used platform/ContentType, as done in e.g. core/html/HTMLImageElement.cpp:supportedImageType. Using platform/ContentType I understand this becomes an equality check rather than startsWith.
On 2016/01/19 01:58:13, suzyh wrote: > Thanks! PTAL at the MIME type comparison. > > Shane advised me that I don't need to add a feature to chromestatus for this, so > I've changed the deprecation warning to not include that part. > > > On 2016/01/18 at 18:54:17, rbyers wrote: > > On 2016/01/18 18:53:56, Rick Byers wrote: > > > LGTM (modulo Yoav's nit) > > > > Oh please also add a link to the blink-dev discussion in the CL description/ > > Done. > > https://codereview.chromium.org/1501393003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/HTMLLinkElement.cpp (right): > > https://codereview.chromium.org/1501393003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLLinkElement.cpp:131: return > type.isEmpty() || type.contains("text/css"); > On 2016/01/18 at 08:21:18, Yoav Weiss wrote: > > This should be a case agnostic comparison, and should use startsWith rather > than contains. > > > > Otherwise, it'd be nice to plumb this through platform/MimeTypeRegistry and > simple_webmimregistry_impl and eventually components/mime_util/mime_util.c > > > > I don't think it's critical to do that in current CL, and we want to push the > deprecation for 49, but maybe add a TODO and plumb everything for the next step? > > I've plumbed it through platform/MimeTypeRegistry and used platform/ContentType, > as done in e.g. core/html/HTMLImageElement.cpp:supportedImageType. Using > platform/ContentType I understand this becomes an equality check rather than > startsWith. LGTM! Looks significantly better :)
The CQ bit was checked by suzyh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/1501393003/#ps40001 (title: "Fix MIME type comparison, update deprecation message")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1501393003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1501393003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by suzyh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1501393003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1501393003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Attempting to commit the patch uncovered a bug: my code displayed a deprecation warning for links that weren't stylesheets. I've moved the check/deprecation warning inside the following if statement, and I think this fixes it, although until I get a clean CQ dry run I'm not 100% sure. I'll run it again tomorrow but I'm sending this back to you now in case I need another lgtm before submitting.
lgtm
The CQ bit was checked by suzyh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1501393003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1501393003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by suzyh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1501393003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1501393003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by suzyh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yoav@yoav.ws, rbyers@chromium.org, esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/1501393003/#ps80001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1501393003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1501393003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Deprecate fetching stylesheets with unsupported type The spec http://www.w3.org/TR/html5/document-metadata.html#the-link-element specifies that "If the UA does not support the given MIME type for the given link relationship, then the UA should not obtain the resource". The linked bug contains a testcase that illustrates that Blink is not currently doing this, and is obtaining the resource even if the link type is e.g. "application/javascript". This patch translates the testcase from the linked bug into Blink layout tests and implements a deprecation warning ahead of implementing the suggested fix. For more background and discussion, see https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/nH1O6WszMgo/... BUG=286682 ========== to ========== Deprecate fetching stylesheets with unsupported type The spec http://www.w3.org/TR/html5/document-metadata.html#the-link-element specifies that "If the UA does not support the given MIME type for the given link relationship, then the UA should not obtain the resource". The linked bug contains a testcase that illustrates that Blink is not currently doing this, and is obtaining the resource even if the link type is e.g. "application/javascript". This patch translates the testcase from the linked bug into Blink layout tests and implements a deprecation warning ahead of implementing the suggested fix. For more background and discussion, see https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/nH1O6WszMgo/... BUG=286682 Committed: https://crrev.com/db6584f5ba2d6cda49fa42775903399cd4d427d6 Cr-Commit-Position: refs/heads/master@{#370531} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/db6584f5ba2d6cda49fa42775903399cd4d427d6 Cr-Commit-Position: refs/heads/master@{#370531} |
