Commit 27e30b2c authored by Alex Rudenko's avatar Alex Rudenko Committed by Commit Bot

Fix image preloading for <picture> elements with explicitly empty image types

HTMLPreloadScanner and HTMLImageElement were using slightly different
logic to determine if an image type is supported. In particular,
HTMLImageElement considered an empty image type as supported and
HTMLPreloadScanner did not. In the following case, it'd result
in HTMLPreloadScanner trying to preload a resource (.png) that will not
be used by HTMLImageElement (it will use .avif):

```
<picture>
  <source srcset="images/test.avif" type="">
  <img src="images/test.png">
</picture>
```

This CL adds a static method HTMLImageElement::SupportedImageType
that is used by HTMLPreloadScanner to check if an image type is
supported. Also, redundant checks for type emptiness are removed
from HTMLImageElement.

Fixed: 1141776
Change-Id: I89ed95c18bbbc86613f6b362ba935f428e1d05b2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2494720
Commit-Queue: Alex Rudenko <alexrudenko@chromium.org>
Reviewed-by: default avatarFredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#820206}
parent da531d49
...@@ -335,7 +335,7 @@ String HTMLImageElement::AltText() const { ...@@ -335,7 +335,7 @@ String HTMLImageElement::AltText() const {
return FastGetAttribute(html_names::kTitleAttr); return FastGetAttribute(html_names::kTitleAttr);
} }
static bool SupportedImageType(const String& type) { bool HTMLImageElement::SupportedImageType(const String& type) {
String trimmed_type = ContentType(type).GetType(); String trimmed_type = ContentType(type).GetType();
// An empty type attribute is implicitly supported. // An empty type attribute is implicitly supported.
if (trimmed_type.IsEmpty()) if (trimmed_type.IsEmpty())
...@@ -367,7 +367,7 @@ ImageCandidate HTMLImageElement::FindBestFitImageFromPictureParent() { ...@@ -367,7 +367,7 @@ ImageCandidate HTMLImageElement::FindBestFitImageFromPictureParent() {
if (srcset.IsEmpty()) if (srcset.IsEmpty())
continue; continue;
String type = source->FastGetAttribute(html_names::kTypeAttr); String type = source->FastGetAttribute(html_names::kTypeAttr);
if (!type.IsEmpty() && !SupportedImageType(type)) if (!SupportedImageType(type))
continue; continue;
if (!source->MediaQueryMatches()) if (!source->MediaQueryMatches())
......
...@@ -181,6 +181,8 @@ class CORE_EXPORT HTMLImageElement final ...@@ -181,6 +181,8 @@ class CORE_EXPORT HTMLImageElement final
void SetIsAdRelated() { is_ad_related_ = true; } void SetIsAdRelated() { is_ad_related_ = true; }
bool IsAdRelated() const override { return is_ad_related_; } bool IsAdRelated() const override { return is_ad_related_; }
static bool SupportedImageType(const String& type);
protected: protected:
// Controls how an image element appears in the layout. See: // Controls how an image element appears in the layout. See:
// https://html.spec.whatwg.org/C/#image-request // https://html.spec.whatwg.org/C/#image-request
......
...@@ -499,8 +499,7 @@ class TokenPreloadScanner::StartTagScanner { ...@@ -499,8 +499,7 @@ class TokenPreloadScanner::StartTagScanner {
// FIXME - Don't match media multiple times. // FIXME - Don't match media multiple times.
matched_ &= MediaAttributeMatches(*media_values_, attribute_value); matched_ &= MediaAttributeMatches(*media_values_, attribute_value);
} else if (Match(attribute_name, html_names::kTypeAttr)) { } else if (Match(attribute_name, html_names::kTypeAttr)) {
matched_ &= MIMETypeRegistry::IsSupportedImagePrefixedMIMEType( matched_ &= HTMLImageElement::SupportedImageType(attribute_value);
ContentType(attribute_value).GetType());
} }
} }
...@@ -633,14 +632,13 @@ class TokenPreloadScanner::StartTagScanner { ...@@ -633,14 +632,13 @@ class TokenPreloadScanner::StartTagScanner {
MIMETypeRegistry::IsSupportedStyleSheetMIMEType( MIMETypeRegistry::IsSupportedStyleSheetMIMEType(
ContentType(type_attribute_value_).GetType()); ContentType(type_attribute_value_).GetType());
} else if (link_is_preload_) { } else if (link_is_preload_) {
if (type == ResourceType::kImage)
return HTMLImageElement::SupportedImageType(type_attribute_value_);
if (type_attribute_value_.IsEmpty()) if (type_attribute_value_.IsEmpty())
return true; return true;
String type_from_attribute = ContentType(type_attribute_value_).GetType(); String type_from_attribute = ContentType(type_attribute_value_).GetType();
if ((type == ResourceType::kFont && if ((type == ResourceType::kFont &&
!MIMETypeRegistry::IsSupportedFontMIMEType(type_from_attribute)) || !MIMETypeRegistry::IsSupportedFontMIMEType(type_from_attribute)) ||
(type == ResourceType::kImage &&
!MIMETypeRegistry::IsSupportedImagePrefixedMIMEType(
type_from_attribute)) ||
(type == ResourceType::kCSSStyleSheet && (type == ResourceType::kCSSStyleSheet &&
!MIMETypeRegistry::IsSupportedStyleSheetMIMEType( !MIMETypeRegistry::IsSupportedStyleSheetMIMEType(
type_from_attribute))) { type_from_attribute))) {
......
...@@ -761,6 +761,10 @@ TEST_F(HTMLPreloadScannerTest, testPicture) { ...@@ -761,6 +761,10 @@ TEST_F(HTMLPreloadScannerTest, testPicture) {
{"http://example.test", {"http://example.test",
"<picture><source srcset='srcset_bla.gif'><img src='bla.gif'></picture>", "<picture><source srcset='srcset_bla.gif'><img src='bla.gif'></picture>",
"srcset_bla.gif", "http://example.test/", ResourceType::kImage, 0}, "srcset_bla.gif", "http://example.test/", ResourceType::kImage, 0},
{"http://example.test",
"<picture><source srcset='srcset_bla.gif' type=''><img "
"src='bla.gif'></picture>",
"srcset_bla.gif", "http://example.test/", ResourceType::kImage, 0},
{"http://example.test", {"http://example.test",
"<picture><source sizes='50vw' srcset='srcset_bla.gif'><img " "<picture><source sizes='50vw' srcset='srcset_bla.gif'><img "
"src='bla.gif'></picture>", "src='bla.gif'></picture>",
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment