- Revision
- 164929
- Author
- [email protected]
- Date
- 2014-03-01 15:28:40 -0800 (Sat, 01 Mar 2014)
Log Message
Fix srcset related bugs
https://bugs.webkit.org/show_bug.cgi?id=129539
Source/WebCore:
Fixed a bug reported against Blink's srcset implementation at https://code.google.com/p/chromium/issues/detail?id=347998
When both src and srcset had only 1x descriptors and the DPR was higher than 1, the src's resource was picked.
Also fixed the invalid descriptor handling, which wasn't aligned with the spec, and therefore was not as forward compatible as it should be.
Invalid descriptors caused the entire resource to be ignored, rather than just the descriptors themselves.
Patch by Yoav Weiss <[email protected]> on 2014-03-01
Reviewed by Andreas Kling.
Tests: fast/hidpi/image-srcset-invalid-descriptor.html
fast/hidpi/image-srcset-src-selection-1x-both.html
* html/parser/HTMLParserIdioms.cpp:
(WebCore::parseDescriptors):
(WebCore::parseImagesWithScaleFromSrcsetAttribute):
(WebCore::bestFitSourceForImageAttributes):
LayoutTests:
These tests make sure that:
1. Invalid srcset descriptors are handled according to the spec. The invalid descriptors are ignored, but the resource is not.
2. When both src and srcset have only 1x descriptors and the DPR is higher than 1, the srcset's resource is picked.
I've also fixed the invalid inputs test, which was faulty.
Patch by Yoav Weiss <[email protected]> on 2014-03-01
Reviewed by Andreas Kling.
* fast/hidpi/image-srcset-invalid-descriptor-expected.txt: Added.
* fast/hidpi/image-srcset-invalid-descriptor.html: Added.
* fast/hidpi/image-srcset-invalid-inputs-correct-src.html:
* fast/hidpi/image-srcset-src-selection-1x-both-expected.txt: Added.
* fast/hidpi/image-srcset-src-selection-1x-both.html: Added.
* fast/hidpi/resources/srcset-helper.js:
(runTest):
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (164928 => 164929)
--- trunk/LayoutTests/ChangeLog 2014-03-01 23:22:56 UTC (rev 164928)
+++ trunk/LayoutTests/ChangeLog 2014-03-01 23:28:40 UTC (rev 164929)
@@ -1,3 +1,24 @@
+2014-03-01 Yoav Weiss <[email protected]>
+
+ Fix srcset related bugs
+ https://bugs.webkit.org/show_bug.cgi?id=129539
+
+ These tests make sure that:
+ 1. Invalid srcset descriptors are handled according to the spec. The invalid descriptors are ignored, but the resource is not.
+ 2. When both src and srcset have only 1x descriptors and the DPR is higher than 1, the srcset's resource is picked.
+
+ I've also fixed the invalid inputs test, which was faulty.
+
+ Reviewed by Andreas Kling.
+
+ * fast/hidpi/image-srcset-invalid-descriptor-expected.txt: Added.
+ * fast/hidpi/image-srcset-invalid-descriptor.html: Added.
+ * fast/hidpi/image-srcset-invalid-inputs-correct-src.html:
+ * fast/hidpi/image-srcset-src-selection-1x-both-expected.txt: Added.
+ * fast/hidpi/image-srcset-src-selection-1x-both.html: Added.
+ * fast/hidpi/resources/srcset-helper.js:
+ (runTest):
+
2014-03-01 Adenilson Cavalcanti <[email protected]>
Set css3/filter/huge-region as skipped
Added: trunk/LayoutTests/fast/hidpi/image-srcset-invalid-descriptor-expected.txt (0 => 164929)
--- trunk/LayoutTests/fast/hidpi/image-srcset-invalid-descriptor-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/hidpi/image-srcset-invalid-descriptor-expected.txt 2014-03-01 23:28:40 UTC (rev 164929)
@@ -0,0 +1,8 @@
+PASS successfullyParsed is true
+
+TEST COMPLETE
+PASS document.getElementById("foo").clientWidth==100 is true
+PASS document.getElementById("foo2").clientWidth==200 is true
+This test passes if the image below is a 100px blue square when the deviceScaleFactor is 1.
+It tests that even though the 1x resource contains many invalid descriptors, only the invalid descriptors are ignored (according to the spec).
+
Added: trunk/LayoutTests/fast/hidpi/image-srcset-invalid-descriptor.html (0 => 164929)
--- trunk/LayoutTests/fast/hidpi/image-srcset-invalid-descriptor.html (rev 0)
+++ trunk/LayoutTests/fast/hidpi/image-srcset-invalid-descriptor.html 2014-03-01 23:28:40 UTC (rev 164929)
@@ -0,0 +1,23 @@
+<html>
+<head>
+<script>
+ window.targetScaleFactor = 1;
+</script>
+<script src=""
+<script src=""
+<script>
+ addEventListener("load", function() {
+ shouldBeTrue('document.getElementById("foo").clientWidth==100');
+ shouldBeTrue('document.getElementById("foo2").clientWidth==200');
+ }, false);
+</script>
+</head>
+
+<body id="body">
+ <div>This test passes if the image below is a 100px blue square when the deviceScaleFactor is 1.</div>
+ <div> It tests that even though the 1x resource contains many invalid descriptors,
+ only the invalid descriptors are ignored (according to the spec).</div>
+ <img id="foo" src="" srcset="resources/blue-100-px-square.png 43q 1x dfs 3dd, resources/green-400-px-square.png 2x">
+ <img id="foo2" src="" srcset="resources/blue-100-px-square.png 300q, resources/green-400-px-square.png 2x">
+</body>
+</html>
Modified: trunk/LayoutTests/fast/hidpi/image-srcset-invalid-inputs-correct-src.html (164928 => 164929)
--- trunk/LayoutTests/fast/hidpi/image-srcset-invalid-inputs-correct-src.html 2014-03-01 23:22:56 UTC (rev 164928)
+++ trunk/LayoutTests/fast/hidpi/image-srcset-invalid-inputs-correct-src.html 2014-03-01 23:28:40 UTC (rev 164929)
@@ -16,6 +16,6 @@
<body id="body">
<div>This test passes if this img tag below is a green square regardless of the scale factor. It ensures that invalid inputs
from srcset are ignored, and src is selected (since it's the only candidate left)</div>
- <img id="foo" src="" srcset="1x,, , , 2x ,,">
+ <img id="foo" src="" srcset="1x 5w,, , , 2x 600h,,">
</body>
</html>
Added: trunk/LayoutTests/fast/hidpi/image-srcset-src-selection-1x-both-expected.txt (0 => 164929)
--- trunk/LayoutTests/fast/hidpi/image-srcset-src-selection-1x-both-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/hidpi/image-srcset-src-selection-1x-both-expected.txt 2014-03-01 23:28:40 UTC (rev 164929)
@@ -0,0 +1,6 @@
+PASS successfullyParsed is true
+
+TEST COMPLETE
+PASS document.getElementById("foo").clientWidth==400 is true
+This test passes if the div below is a blue 100px square when the deviceScaleFactor is 1. It simply ensures that when both src and srcset are specified for the same DPR, srcset wins.
+
Added: trunk/LayoutTests/fast/hidpi/image-srcset-src-selection-1x-both.html (0 => 164929)
--- trunk/LayoutTests/fast/hidpi/image-srcset-src-selection-1x-both.html (rev 0)
+++ trunk/LayoutTests/fast/hidpi/image-srcset-src-selection-1x-both.html 2014-03-01 23:28:40 UTC (rev 164929)
@@ -0,0 +1,24 @@
+<html>
+<head>
+<script>
+ window.targetScaleFactor = 2;
+</script>
+<script src=""
+<script src=""
+<script>
+ if (window.testRunner) {
+ testRunner.dumpAsText();
+ }
+
+ addEventListener("load", function() {
+ shouldBeTrue('document.getElementById("foo").clientWidth==400');
+ }, false);
+</script>
+</head>
+
+<body id="body">
+ <div>This test passes if the div below is a blue 100px square when the deviceScaleFactor is 1.
+ It simply ensures that when both src and srcset are specified for the same DPR, srcset wins.</div>
+ <img id="foo" src="" srcset="resources/green-400-px-square.png 1x">
+</body>
+</html>
Modified: trunk/LayoutTests/fast/hidpi/resources/srcset-helper.js (164928 => 164929)
--- trunk/LayoutTests/fast/hidpi/resources/srcset-helper.js 2014-03-01 23:22:56 UTC (rev 164928)
+++ trunk/LayoutTests/fast/hidpi/resources/srcset-helper.js 2014-03-01 23:28:40 UTC (rev 164929)
@@ -5,6 +5,9 @@
if (!window.targetScaleFactor)
window.targetScaleFactor = 2;
+ if(!sessionStorage.pageReloaded && window.targetScaleFactor == window.deviceScaleFactor)
+ return;
+
if (!sessionStorage.scaleFactorIsSet) {
testRunner.waitUntilDone();
testRunner.setBackingScaleFactor(targetScaleFactor, scaleFactorIsSet);
Modified: trunk/Source/WebCore/ChangeLog (164928 => 164929)
--- trunk/Source/WebCore/ChangeLog 2014-03-01 23:22:56 UTC (rev 164928)
+++ trunk/Source/WebCore/ChangeLog 2014-03-01 23:28:40 UTC (rev 164929)
@@ -1,3 +1,24 @@
+2014-03-01 Yoav Weiss <[email protected]>
+
+ Fix srcset related bugs
+ https://bugs.webkit.org/show_bug.cgi?id=129539
+
+ Fixed a bug reported against Blink's srcset implementation at https://code.google.com/p/chromium/issues/detail?id=347998
+ When both src and srcset had only 1x descriptors and the DPR was higher than 1, the src's resource was picked.
+
+ Also fixed the invalid descriptor handling, which wasn't aligned with the spec, and therefore was not as forward compatible as it should be.
+ Invalid descriptors caused the entire resource to be ignored, rather than just the descriptors themselves.
+
+ Reviewed by Andreas Kling.
+
+ Tests: fast/hidpi/image-srcset-invalid-descriptor.html
+ fast/hidpi/image-srcset-src-selection-1x-both.html
+
+ * html/parser/HTMLParserIdioms.cpp:
+ (WebCore::parseDescriptors):
+ (WebCore::parseImagesWithScaleFromSrcsetAttribute):
+ (WebCore::bestFitSourceForImageAttributes):
+
2014-03-01 Darin Adler <[email protected]>
Improve "bad parent" and "bad child list" assertions in line boxes
Modified: trunk/Source/WebCore/html/parser/HTMLParserIdioms.cpp (164928 => 164929)
--- trunk/Source/WebCore/html/parser/HTMLParserIdioms.cpp 2014-03-01 23:22:56 UTC (rev 164928)
+++ trunk/Source/WebCore/html/parser/HTMLParserIdioms.cpp 2014-03-01 23:28:40 UTC (rev 164929)
@@ -302,6 +302,40 @@
return isHTMLSpace(character) || character == ',';
}
+static bool parseDescriptors(const String& attribute, size_t start, size_t end, float& imageScaleFactor)
+{
+ size_t descriptorStart;
+ size_t descriptorEnd;
+ size_t position = start;
+ bool isFoundScaleFactor = false;
+ bool isEmptyDescriptor = !(end > start);
+ bool isValid = false;
+
+ while (position < end) {
+ while (isHTMLSpace(attribute[position]) && position < end)
+ ++position;
+ descriptorStart = position;
+ while (isNotHTMLSpace(attribute[position]) && position < end)
+ ++position;
+ descriptorEnd = position;
+
+ ASSERT(descriptorEnd > descriptorStart);
+ --descriptorEnd;
+ // This part differs from the spec as the current implementation only supports pixel density descriptors.
+ if (attribute[descriptorEnd] != 'x')
+ continue;
+
+ if (isFoundScaleFactor)
+ return false;
+
+ imageScaleFactor = charactersToFloat(attribute.deprecatedCharacters() + descriptorStart, descriptorEnd - descriptorStart, &isValid);
+ isFoundScaleFactor = true;
+
+ }
+
+ return isEmptyDescriptor || isValid;
+}
+
// See the specifications for more details about the algorithm to follow.
// http://www.w3.org/TR/2013/WD-html-srcset-20130228/#processing-the-image-candidates.
static void parseImagesWithScaleFromSrcsetAttribute(const String& srcsetAttribute, ImageCandidates& imageCandidates)
@@ -335,42 +369,15 @@
} else {
// 7. Collect a sequence of characters that are not "," (U+002C) characters, and let that be descriptors.
size_t imageScaleStart = srcsetAttribute.find(isNotHTMLSpace, imageURLEnd + 1);
- if (imageScaleStart == notFound)
- separator = srcsetAttributeLength;
- else if (srcsetAttribute[imageScaleStart] == ',')
- separator = imageScaleStart;
- else {
- // This part differs from the spec as the current implementation only supports pixel density descriptors for now.
- size_t imageScaleEnd = srcsetAttribute.find(isHTMLSpaceOrComma, imageScaleStart + 1);
- imageScaleEnd = (imageScaleEnd == notFound) ? srcsetAttributeLength : imageScaleEnd;
- size_t commaPosition = imageScaleEnd;
- // Make sure there are no other descriptors.
- while ((commaPosition < srcsetAttributeLength - 1) && isHTMLSpace(srcsetAttribute[commaPosition]))
- ++commaPosition;
- // If the first not html space character after the scale modifier is not a comma,
- // the current candidate is an invalid input.
- if ((commaPosition < srcsetAttributeLength - 1) && srcsetAttribute[commaPosition] != ',') {
- // Find the nearest comma and skip the input.
- commaPosition = srcsetAttribute.find(',', commaPosition + 1);
- if (commaPosition == notFound)
- break;
- imageCandidateStart = commaPosition + 1;
- continue;
- }
- separator = commaPosition;
- if (srcsetAttribute[imageScaleEnd - 1] != 'x') {
- imageCandidateStart = separator + 1;
- continue;
- }
- bool validScaleFactor = false;
- size_t scaleFactorLengthWithoutUnit = imageScaleEnd - imageScaleStart - 1;
- imageScaleFactor = charactersToFloat(srcsetAttribute.deprecatedCharacters() + imageScaleStart, scaleFactorLengthWithoutUnit, &validScaleFactor);
+ imageScaleStart = (imageScaleStart == notFound) ? srcsetAttributeLength : imageScaleStart;
+ size_t imageScaleEnd = srcsetAttribute.find(',' , imageScaleStart + 1);
+ imageScaleEnd = (imageScaleEnd == notFound) ? srcsetAttributeLength : imageScaleEnd;
- if (!validScaleFactor) {
- imageCandidateStart = separator + 1;
- continue;
- }
+ if (!parseDescriptors(srcsetAttribute, imageScaleStart, imageScaleEnd, imageScaleFactor)) {
+ imageCandidateStart = imageScaleEnd + 1;
+ continue;
}
+ separator = imageScaleEnd;
}
ImageWithScale image(imageURLStart, imageURLEnd - imageURLStart, imageScaleFactor);
imageCandidates.append(image);
@@ -399,8 +406,16 @@
if (imageCandidates[i].scaleFactor() >= deviceScaleFactor)
return imageCandidates[i];
}
- const ImageWithScale& lastCandidate = imageCandidates.last();
- return lastCandidate;
+
+ // 16. If an entry b in candidates has the same associated ... pixel density as an earlier entry a in candidates,
+ // then remove entry b
+ size_t winner = imageCandidates.size() - 1;
+ size_t previousCandidate = winner;
+ float winningScaleFactor = imageCandidates.last().scaleFactor();
+ while ((previousCandidate > 0) && (imageCandidates[--previousCandidate].scaleFactor() == winningScaleFactor))
+ winner = previousCandidate;
+
+ return imageCandidates[winner];
}
}
