Diff
Modified: trunk/LayoutTests/ChangeLog (153626 => 153627)
--- trunk/LayoutTests/ChangeLog 2013-08-02 00:02:14 UTC (rev 153626)
+++ trunk/LayoutTests/ChangeLog 2013-08-02 00:02:25 UTC (rev 153627)
@@ -1,5 +1,32 @@
2013-08-01 Dean Jackson <[email protected]>
+ srcset algorithm breaks base64 src attributes
+ https://bugs.webkit.org/show_bug.cgi?id=119413
+
+ Reviewed by Darin Adler.
+
+ Four new tests that exercise candidate matching. In particular:
+ - base64 encoded src attributes
+ - base64 encoded srcset attributes that are escaped
+ - attributes without scale modifiers
+ - attributes that have modifiers other than scale
+
+ * fast/hidpi/image-srcset-change-dynamically-from-js.html: Minor change to add scale modifier.
+ * fast/hidpi/image-srcset-data-src.html: Added.
+ * fast/hidpi/image-srcset-data-srcset.html: Added.
+ * fast/hidpi/image-srcset-nomodifier.html: Added.
+ * fast/hidpi/image-srcset-viewport-modifiers.html: Added.
+ * platform/mac/fast/hidpi/image-srcset-data-src-expected.png: Added.
+ * platform/mac/fast/hidpi/image-srcset-data-src-expected.txt: Added.
+ * platform/mac/fast/hidpi/image-srcset-data-srcset-expected.png: Added.
+ * platform/mac/fast/hidpi/image-srcset-data-srcset-expected.txt: Added.
+ * platform/mac/fast/hidpi/image-srcset-nomodifier-expected.png: Added.
+ * platform/mac/fast/hidpi/image-srcset-nomodifier-expected.txt: Added.
+ * platform/mac/fast/hidpi/image-srcset-viewport-modifiers-expected.png: Added.
+ * platform/mac/fast/hidpi/image-srcset-viewport-modifiers-expected.txt: Added.
+
+2013-08-01 Dean Jackson <[email protected]>
+
Minor cleanup for srcset tests
https://bugs.webkit.org/show_bug.cgi?id=119407
Modified: trunk/LayoutTests/fast/hidpi/image-srcset-change-dynamically-from-js.html (153626 => 153627)
--- trunk/LayoutTests/fast/hidpi/image-srcset-change-dynamically-from-js.html 2013-08-02 00:02:14 UTC (rev 153626)
+++ trunk/LayoutTests/fast/hidpi/image-srcset-change-dynamically-from-js.html 2013-08-02 00:02:25 UTC (rev 153627)
@@ -4,7 +4,7 @@
function updateSrc() {
var img = document.getElementById("foo");
img.src = ""
- img.srcset = "resources/green-200-px-square.png";
+ img.srcset = "resources/green-200-px-square.png 1x";
}
function runTest() {
Added: trunk/LayoutTests/fast/hidpi/image-srcset-data-src.html (0 => 153627)
--- trunk/LayoutTests/fast/hidpi/image-srcset-data-src.html (rev 0)
+++ trunk/LayoutTests/fast/hidpi/image-srcset-data-src.html 2013-08-02 00:02:25 UTC (rev 153627)
@@ -0,0 +1,10 @@
+<html>
+<head>
+<script src=""
+</head>
+
+<body id="body">
+ <div>This test passes if the image below looks like a 2x2 grid (yellow, gray, light green, dark green).</div>
+ <img height="100" width="100" src=""
+</body>
+</html>
Property changes on: trunk/LayoutTests/fast/hidpi/image-srcset-data-src.html
___________________________________________________________________
Added: svn:mime-type
Added: svn:keywords
Added: svn:eol-style
Added: trunk/LayoutTests/fast/hidpi/image-srcset-data-srcset.html (0 => 153627)
--- trunk/LayoutTests/fast/hidpi/image-srcset-data-srcset.html (rev 0)
+++ trunk/LayoutTests/fast/hidpi/image-srcset-data-srcset.html 2013-08-02 00:02:25 UTC (rev 153627)
@@ -0,0 +1,10 @@
+<html>
+<head>
+<script src=""
+</head>
+
+<body id="body">
+ <div>This test passes if the image below looks like a 2x2 grid (yellow, gray, light green, dark green). In this case the srcset attribute has a base64 url, with the comma escaped.</div>
+ <img height="100" width="100" src="" srcset="data:image/png;base64%2CiVBORw0KGgoAAAANSUhEUgAAAJYAAAA8CAIAAAAL5NQ9AAAACXBIWXMAAAsTAAALEwEAmpwYAAAAB3RJTUUH2woaBQc4oLEFpAAAABl0RVh0Q29tbWVudABDcmVhdGVkIHdpdGggR0lNUFeBDhcAAACMSURBVHja7dNBEYAgFEVRPhHMYgAzUIsmVnFvB/fsoQb+ObfBmzMvxneW1D1vzz2wFiEUQiFEKIRCKIQIhVAIhRChEAqhECIUQiEUQoRCKIRCiFAIhVAIEep3xTWTLzzu5oVCKIRCiFAIhVAIEQqhEAohQiEUQiFEKIRCKIQIhVAIhRChEAqhECLUZi3VEwcBMGr1NgAAAABJRU5ErkJggg== 2x, data:image/png;base64%2CiVBORw0KGgoAAAANSUhEUgAAAJYAAAA8CAIAAAAL5NQ9AAAACXBIWXMAAAsTAAALEwEAmpwYAAAAB3RJTUUH2woaBQc4oLEFpAAAABl0RVh0Q29tbWVudABDcmVhdGVkIHdpdGggR0lNUFeBDhcAAACMSURBVHja7dNBEYAgFEVRPhHMYgAzUIsmVnFvB/fsoQb+ObfBmzMvxneW1D1vzz2wFiEUQiFEKIRCKIQIhVAIhRChEAqhECIUQiEUQoRCKIRCiFAIhVAIEep3xTWTLzzu5oVCKIRCiFAIhVAIEQqhEAohQiEUQiFEKIRCKIQIhVAIhRChEAqhECLUZi3VEwcBMGr1NgAAAABJRU5ErkJggg== 1x">
+</body>
+</html>
Property changes on: trunk/LayoutTests/fast/hidpi/image-srcset-data-srcset.html
___________________________________________________________________
Added: svn:mime-type
Added: svn:keywords
Added: svn:eol-style
Added: trunk/LayoutTests/fast/hidpi/image-srcset-nomodifier.html (0 => 153627)
--- trunk/LayoutTests/fast/hidpi/image-srcset-nomodifier.html (rev 0)
+++ trunk/LayoutTests/fast/hidpi/image-srcset-nomodifier.html 2013-08-02 00:02:25 UTC (rev 153627)
@@ -0,0 +1,10 @@
+<html>
+<head>
+<script src=""
+</head>
+
+<body id="body">
+ <div>This test passes if the image below is empty, since the srcset is invalid (does not have a scale modifier anywhere and the src attribute is empty).</div>
+ <img height="100" width="100" src="" srcset="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAJYAAAA8CAIAAAAL5NQ9AAAACXBIWXMAAAsTAAALEwEAmpwYAAAAB3RJTUUH2woaBQc4oLEFpAAAABl0RVh0Q29tbWVudABDcmVhdGVkIHdpdGggR0lNUFeBDhcAAACMSURBVHja7dNBEYAgFEVRPhHMYgAzUIsmVnFvB/fsoQb+ObfBmzMvxneW1D1vzz2wFiEUQiFEKIRCKIQIhVAIhRChEAqhECIUQiEUQoRCKIRCiFAIhVAIEep3xTWTLzzu5oVCKIRCiFAIhVAIEQqhEAohQiEUQiFEKIRCKIQIhVAIhRChEAqhECLUZi3VEwcBMGr1NgAAAABJRU5ErkJggg==">
+</body>
+</html>
Property changes on: trunk/LayoutTests/fast/hidpi/image-srcset-nomodifier.html
___________________________________________________________________
Added: svn:mime-type
Added: svn:keywords
Added: svn:eol-style
Added: trunk/LayoutTests/fast/hidpi/image-srcset-viewport-modifiers.html (0 => 153627)
--- trunk/LayoutTests/fast/hidpi/image-srcset-viewport-modifiers.html (rev 0)
+++ trunk/LayoutTests/fast/hidpi/image-srcset-viewport-modifiers.html 2013-08-02 00:02:25 UTC (rev 153627)
@@ -0,0 +1,10 @@
+<html>
+<head>
+<script src=""
+</head>
+
+<body id="body">
+ <div>This test passes if the image below is a blue square, since the srcset is unhandled (uses a viewport modifier that we will skip)</div>
+ <img height="100" width="100" src="" srcset="resources/green-200-px-square.png 100w 2x">
+</body>
+</html>
Property changes on: trunk/LayoutTests/fast/hidpi/image-srcset-viewport-modifiers.html
___________________________________________________________________
Added: svn:mime-type
Added: svn:keywords
Added: svn:eol-style
Added: trunk/LayoutTests/platform/mac/fast/hidpi/image-srcset-data-src-expected.png
(Binary files differ)
Added: svn:mime-type
Added: trunk/LayoutTests/platform/mac/fast/hidpi/image-srcset-data-src-expected.txt (0 => 153627)
--- trunk/LayoutTests/platform/mac/fast/hidpi/image-srcset-data-src-expected.txt (rev 0)
+++ trunk/LayoutTests/platform/mac/fast/hidpi/image-srcset-data-src-expected.txt 2013-08-02 00:02:25 UTC (rev 153627)
@@ -0,0 +1,11 @@
+layer at (0,0) size 800x600
+ RenderView at (0,0) size 800x600
+layer at (0,0) size 800x600
+ RenderBlock {HTML} at (0,0) size 800x600
+ RenderBody {BODY} at (8,8) size 784x584
+ RenderBlock {DIV} at (0,0) size 784x18
+ RenderText {#text} at (0,0) size 598x18
+ text run at (0,0) width 598: "This test passes if the image below looks like a 2x2 grid (yellow, gray, light green, dark green)."
+ RenderBlock (anonymous) at (0,18) size 784x100
+ RenderImage {IMG} at (0,0) size 100x100
+ RenderText {#text} at (0,0) size 0x0
Property changes on: trunk/LayoutTests/platform/mac/fast/hidpi/image-srcset-data-src-expected.txt
___________________________________________________________________
Added: svn:mime-type
Added: svn:keywords
Added: svn:eol-style
Added: trunk/LayoutTests/platform/mac/fast/hidpi/image-srcset-data-srcset-expected.png
(Binary files differ)
Added: svn:mime-type
Added: trunk/LayoutTests/platform/mac/fast/hidpi/image-srcset-data-srcset-expected.txt (0 => 153627)
--- trunk/LayoutTests/platform/mac/fast/hidpi/image-srcset-data-srcset-expected.txt (rev 0)
+++ trunk/LayoutTests/platform/mac/fast/hidpi/image-srcset-data-srcset-expected.txt 2013-08-02 00:02:25 UTC (rev 153627)
@@ -0,0 +1,12 @@
+layer at (0,0) size 800x600
+ RenderView at (0,0) size 800x600
+layer at (0,0) size 800x600
+ RenderBlock {HTML} at (0,0) size 800x600
+ RenderBody {BODY} at (8,8) size 784x584
+ RenderBlock {DIV} at (0,0) size 784x36
+ RenderText {#text} at (0,0) size 734x36
+ text run at (0,0) width 734: "This test passes if the image below looks like a 2x2 grid (yellow, gray, light green, dark green). In this case the srcset"
+ text run at (0,18) width 323: "attribute has a base64 url, with the comma escaped."
+ RenderBlock (anonymous) at (0,36) size 784x100
+ RenderImage {IMG} at (0,0) size 100x100
+ RenderText {#text} at (0,0) size 0x0
Property changes on: trunk/LayoutTests/platform/mac/fast/hidpi/image-srcset-data-srcset-expected.txt
___________________________________________________________________
Added: svn:mime-type
Added: svn:keywords
Added: svn:eol-style
Added: trunk/LayoutTests/platform/mac/fast/hidpi/image-srcset-nomodifier-expected.png
(Binary files differ)
Added: svn:mime-type
Added: trunk/LayoutTests/platform/mac/fast/hidpi/image-srcset-nomodifier-expected.txt (0 => 153627)
--- trunk/LayoutTests/platform/mac/fast/hidpi/image-srcset-nomodifier-expected.txt (rev 0)
+++ trunk/LayoutTests/platform/mac/fast/hidpi/image-srcset-nomodifier-expected.txt 2013-08-02 00:02:25 UTC (rev 153627)
@@ -0,0 +1,12 @@
+layer at (0,0) size 800x600
+ RenderView at (0,0) size 800x600
+layer at (0,0) size 800x600
+ RenderBlock {HTML} at (0,0) size 800x600
+ RenderBody {BODY} at (8,8) size 784x584
+ RenderBlock {DIV} at (0,0) size 784x36
+ RenderText {#text} at (0,0) size 775x36
+ text run at (0,0) width 775: "This test passes if the image below is empty, since the srcset is invalid (does not have a scale modifier anywhere and the src"
+ text run at (0,18) width 117: "attribute is empty)."
+ RenderBlock (anonymous) at (0,36) size 784x100
+ RenderImage {IMG} at (0,0) size 100x100
+ RenderText {#text} at (0,0) size 0x0
Property changes on: trunk/LayoutTests/platform/mac/fast/hidpi/image-srcset-nomodifier-expected.txt
___________________________________________________________________
Added: svn:mime-type
Added: svn:keywords
Added: svn:eol-style
Added: trunk/LayoutTests/platform/mac/fast/hidpi/image-srcset-viewport-modifiers-expected.png
(Binary files differ)
Added: svn:mime-type
Added: trunk/LayoutTests/platform/mac/fast/hidpi/image-srcset-viewport-modifiers-expected.txt (0 => 153627)
--- trunk/LayoutTests/platform/mac/fast/hidpi/image-srcset-viewport-modifiers-expected.txt (rev 0)
+++ trunk/LayoutTests/platform/mac/fast/hidpi/image-srcset-viewport-modifiers-expected.txt 2013-08-02 00:02:25 UTC (rev 153627)
@@ -0,0 +1,11 @@
+layer at (0,0) size 800x600
+ RenderView at (0,0) size 800x600
+layer at (0,0) size 800x600
+ RenderBlock {HTML} at (0,0) size 800x600
+ RenderBody {BODY} at (8,8) size 784x584
+ RenderBlock {DIV} at (0,0) size 784x18
+ RenderText {#text} at (0,0) size 782x18
+ text run at (0,0) width 782: "This test passes if the image below is a blue square, since the srcset is unhandled (uses a viewport modifier that we will skip)"
+ RenderBlock (anonymous) at (0,18) size 784x100
+ RenderImage {IMG} at (0,0) size 100x100
+ RenderText {#text} at (0,0) size 0x0
Property changes on: trunk/LayoutTests/platform/mac/fast/hidpi/image-srcset-viewport-modifiers-expected.txt
___________________________________________________________________
Added: svn:mime-type
Added: svn:keywords
Added: svn:eol-style
Modified: trunk/Source/WebCore/ChangeLog (153626 => 153627)
--- trunk/Source/WebCore/ChangeLog 2013-08-02 00:02:14 UTC (rev 153626)
+++ trunk/Source/WebCore/ChangeLog 2013-08-02 00:02:25 UTC (rev 153627)
@@ -1,3 +1,38 @@
+2013-08-01 Dean Jackson <[email protected]>
+
+ srcset algorithm breaks base64 src attributes
+ https://bugs.webkit.org/show_bug.cgi?id=119413
+
+ Reviewed by Darin Adler.
+
+ Base64 encoded src attributes typically have a COMMA
+ character which was breaking in the candidate matching
+ algorithm. Make sure to handle that case, and to unescape
+ any incoming URLs.
+
+ Slight cleanup of the srcset matching algorithm. The
+ candidates are now gathered from a single update
+ method. I've renamed the methods in the process.
+ This means we now reparse the srcset attribute if
+ only the src changes, but I think the code is
+ cleaner this way.
+
+ Tests: fast/hidpi/image-srcset-data-src.html
+ fast/hidpi/image-srcset-data-srcset.html
+ fast/hidpi/image-srcset-nomodifier.html
+ fast/hidpi/image-srcset-viewport-modifiers.html
+
+ * html/HTMLImageElement.cpp:
+ (WebCore::HTMLImageElement::HTMLImageElement): No need to initialise m_bestFitImageURL.
+ (WebCore::HTMLImageElement::imageSourceURL): Use isEmpty() rather than checking for nullAtom.
+ (WebCore::HTMLImageElement::determineBestImageForScaleFactor): New renamed method that selects the best
+ candidate for the image source.
+ (WebCore::HTMLImageElement::collectImageCandidatesFromSrcSet): Gather the srcset images. Changes include
+ simplifying the whitespace and skipping candidates that we don't yet support.
+ (WebCore::HTMLImageElement::collectImageCandidateFromSrc): Add the src attribute to the list of candidates.
+ (WebCore::HTMLImageElement::parseAttribute): Now both attributes call determineBestImageForScaleFactor.
+ * html/HTMLImageElement.h: No need for m_srcImageIndex any more.
+
2013-08-01 Romain Perier <[email protected]>
Implement img element's srcset attribute
Modified: trunk/Source/WebCore/html/HTMLImageElement.cpp (153626 => 153627)
--- trunk/Source/WebCore/html/HTMLImageElement.cpp 2013-08-02 00:02:14 UTC (rev 153626)
+++ trunk/Source/WebCore/html/HTMLImageElement.cpp 2013-08-02 00:02:25 UTC (rev 153627)
@@ -49,8 +49,6 @@
, m_imageLoader(this)
, m_form(form)
, m_compositeOperator(CompositeSourceOver)
- , m_bestFitImageURL(nullAtom)
- , m_srcImageIndex(notFound)
{
ASSERT(hasTagName(imgTag));
if (form)
@@ -114,16 +112,29 @@
const AtomicString& HTMLImageElement::imageSourceURL() const
{
- return m_bestFitImageURL == nullAtom ? getAttribute(srcAttr) : m_bestFitImageURL;
+ return m_bestFitImageURL.isEmpty() ? fastGetAttribute(srcAttr) : m_bestFitImageURL;
}
-void HTMLImageElement::updateBestImageForScaleFactor()
+void HTMLImageElement::determineBestImageForScaleFactor()
{
+ m_imagesWithScale.clear();
+ m_bestFitImageURL = nullAtom;
+
+ collectImageCandidatesFromSrcSet();
+ collectImageCandidateFromSrc();
+ stable_sort(m_imagesWithScale.begin(), m_imagesWithScale.end(), compareByScaleFactor);
+
+ for (size_t i = 1; i < m_imagesWithScale.size(); ++i) {
+ if (m_imagesWithScale[i-1].scaleFactor == m_imagesWithScale[i].scaleFactor) {
+ m_imagesWithScale.remove(i);
+ i--;
+ }
+ }
+
float pageScaleFactor = 1.0;
-
- m_bestFitImageURL = nullAtom;
if (Page* page = document()->page())
pageScaleFactor = page->deviceScaleFactor();
+
for (size_t i = 0; i < m_imagesWithScale.size(); ++i) {
if (m_imagesWithScale[i].scaleFactor >= pageScaleFactor) {
m_bestFitImageURL = m_imagesWithScale[i].imageURL;
@@ -132,47 +143,48 @@
}
}
-void HTMLImageElement::updateImagesFromSrcSet(const AtomicString& srcset)
+void HTMLImageElement::collectImageCandidatesFromSrcSet()
{
- const String& string = static_cast<const String&>(srcset);
- Vector<String> srcSet;
+ const String& srcSetAttributeValue = fastGetAttribute(srcsetAttr).string().simplifyWhiteSpace(isHTMLSpace);
+ Vector<String> srcSetTokens;
- string.split(',', srcSet);
- for (size_t i = 0; i < srcSet.size(); ++i) {
+ srcSetAttributeValue.split(',', srcSetTokens);
+ for (size_t i = 0; i < srcSetTokens.size(); ++i) {
Vector<String> data;
float imgScaleFactor = 1.0;
bool validScaleFactor = false;
- srcSet[i].stripWhiteSpace().split(' ', data);
- if (data.size() > 0 && data.last().endsWith('x')) {
- imgScaleFactor = data.last().substring(0, data.last().length() - 1).toFloat(&validScaleFactor);
- if (!validScaleFactor)
- imgScaleFactor = 1.0;
- }
- if (!data.size() || (data.size() == 1 && validScaleFactor))
+ srcSetTokens[i].stripWhiteSpace().split(' ', data);
+ // There must be at least one candidate descriptor, and the last one must
+ // be a scale factor. Since we don't support descriptors other than scale,
+ // it's better to discard any rule with such descriptors rather than accept
+ // only the scale data.
+ if (data.size() != 2)
continue;
+ if (!data.last().endsWith('x'))
+ continue;
+
+ imgScaleFactor = data.last().substring(0, data.last().length() - 1).toFloat(&validScaleFactor);
+ if (!validScaleFactor)
+ continue;
+
ImageWithScale image;
- image.imageURL = data[0];
+ image.imageURL = decodeURLEscapeSequences(data[0]);
image.scaleFactor = imgScaleFactor;
+
m_imagesWithScale.append(image);
}
- const AtomicString& src = ""
+}
+
+void HTMLImageElement::collectImageCandidateFromSrc()
+{
+ const AtomicString& src = ""
ImageWithScale image;
if (!src.isEmpty()) {
- image.imageURL = getAttribute(srcAttr);
+ image.imageURL = decodeURLEscapeSequences(src);
image.scaleFactor = 1.0;
m_imagesWithScale.append(image);
}
- stable_sort(m_imagesWithScale.begin(), m_imagesWithScale.end(), compareByScaleFactor);
-
- for (size_t i = 1; i < m_imagesWithScale.size(); ++i) {
- if (m_imagesWithScale[i-1].scaleFactor == m_imagesWithScale[i].scaleFactor) {
- m_imagesWithScale.remove(i);
- i--;
- }
- }
- if (!src.isEmpty())
- m_srcImageIndex = m_imagesWithScale.find(image);
}
void HTMLImageElement::parseAttribute(const QualifiedName& name, const AtomicString& value)
@@ -180,17 +192,9 @@
if (name == altAttr) {
if (renderer() && renderer()->isImage())
toRenderImage(renderer())->updateAltText();
- } else if (name == srcAttr) {
- if (m_srcImageIndex != notFound)
- m_imagesWithScale.remove(m_srcImageIndex);
- updateImagesFromSrcSet(value);
- updateBestImageForScaleFactor();
+ } else if (name == srcAttr || name == srcsetAttr) {
+ determineBestImageForScaleFactor();
m_imageLoader.updateFromElementIgnoringPreviousError();
- } else if (name == srcsetAttr) {
- m_imagesWithScale.clear();
- updateImagesFromSrcSet(value);
- updateBestImageForScaleFactor();
- m_imageLoader.updateFromElementIgnoringPreviousError();
}
else if (name == usemapAttr)
setIsLink(!value.isNull());
Modified: trunk/Source/WebCore/html/HTMLImageElement.h (153626 => 153627)
--- trunk/Source/WebCore/html/HTMLImageElement.h 2013-08-02 00:02:14 UTC (rev 153626)
+++ trunk/Source/WebCore/html/HTMLImageElement.h 2013-08-02 00:02:25 UTC (rev 153627)
@@ -109,10 +109,10 @@
virtual void setItemValueText(const String&, ExceptionCode&) OVERRIDE;
#endif
- void updateImagesFromSrcSet(const AtomicString& srcset);
+ void collectImageCandidateFromSrc();
+ void collectImageCandidatesFromSrcSet();
+ void determineBestImageForScaleFactor();
- void updateBestImageForScaleFactor();
-
struct ImageWithScale {
String imageURL;
float scaleFactor;
@@ -131,7 +131,6 @@
HTMLFormElement* m_form;
CompositeOperator m_compositeOperator;
AtomicString m_bestFitImageURL;
- size_t m_srcImageIndex;
Vector<ImageWithScale> m_imagesWithScale;
};
