Refactor srcset parser to align it with spec changes

Source/core/html/parser/HTMLSrcsetParser.cpp

Index: Source/core/html/parser/HTMLSrcsetParser.cpp
diff --git a/Source/core/html/parser/HTMLSrcsetParser.cpp b/Source/core/html/parser/HTMLSrcsetParser.cpp
index 35aa976503af65a7eba548215387d5f12e296021..48b092997d7b5a0c4e095b085c75137692b9449b 100644
--- a/Source/core/html/parser/HTMLSrcsetParser.cpp
+++ b/Source/core/html/parser/HTMLSrcsetParser.cpp
@@ -37,54 +37,152 @@
 
 namespace WebCore {
 
-static bool compareByScaleFactor(const ImageCandidate& first, const ImageCandidate& second)
+static bool compareByDensity(const ImageCandidate& first, const ImageCandidate& second)
 {
-    return first.scaleFactor() < second.scaleFactor();
+    return first.density() < second.density();
 }
 
+enum DescriptorTokenizerState {
+    Start,
+    InParenthesis,
+    AfterToken,
+};
+
+struct DescriptorToken {
+    unsigned start;
+    unsigned length;
+    DescriptorToken(unsigned start, unsigned length)
+        : start(start)
+        , length(length)
+    {
+    }
+};
+
+template<typename CharType>
+static void appendDescriptorAndReset(const CharType* attributeStart, const CharType*& descriptorStart, const CharType* position, Vector<DescriptorToken>& descriptors)
+{
+    if (position > descriptorStart) {

[eseidel, 2014/05/28 22:24:59]

I'm entertained that you use {} here and not below. :)  I believe officially
both are OK these days, but you might want to pick a style for your file here.
:)

[Yoav Weiss, 2014/05/29 05:17:40]

Leftovers from adding a printf at some point.
OTOH, I didn't know that the style guide now permits it.

+        descriptors.append(DescriptorToken(descriptorStart - attributeStart, position - descriptorStart));
+    }
+    descriptorStart = 0;

[eseidel, 2014/05/28 22:24:59]

It's a bit odd that this has a side-effect...

[Yoav Weiss, 2014/05/29 05:17:40]

This is why I added "AndReset" to the function name. Do you think it's not
enough?

+}
+
+// The following is called appendCharacter to match the spec's terminology.
 template<typename CharType>
-inline bool isComma(CharType character)
+static void appendCharacter(const CharType* descriptorStart, const CharType* position)
 {
-    return character == ',';
+    // Since we don't copy the tokens, this just set the point where the descriptor tokens start.
+    if (!descriptorStart)
+        descriptorStart = position;
 }
 
 template<typename CharType>
-static bool parseDescriptors(const CharType* descriptorsStart, const CharType* descriptorsEnd, DescriptorParsingResult& result)
+static bool isEOF(const CharType* position, const CharType* end)
 {
-    const CharType* position = descriptorsStart;
-    bool isValid = false;
-    bool isEmptyDescriptor = !(descriptorsEnd > descriptorsStart);
-    while (position < descriptorsEnd) {
-        // 13.1. Let descriptor list be the result of splitting unparsed descriptors on spaces.
-        skipWhile<CharType, isHTMLSpace<CharType> >(position, descriptorsEnd);
-        const CharType* currentDescriptorStart = position;
-        skipWhile<CharType, isNotHTMLSpace<CharType> >(position, descriptorsEnd);
-        const CharType* currentDescriptorEnd = position;
+    return position >= end;
+}
 
+template<typename CharType>
+static void tokenizeDescriptors(const CharType* attributeStart,
+    const CharType*& position,
+    const CharType* attributeEnd,
+    Vector<DescriptorToken>& descriptors)
+{
+    DescriptorTokenizerState state = Start;
+    const CharType* descriptorsStart = position;
+    const CharType* currentDescriptorStart = descriptorsStart;
+    while (true) {
+        switch (state) {
+        case Start:
+            if (isEOF(position, attributeEnd)) {
+                appendDescriptorAndReset(attributeStart, currentDescriptorStart, attributeEnd, descriptors);
+                return;
+            }
+            if (isComma(*position)) {
+                appendDescriptorAndReset(attributeStart, currentDescriptorStart, position, descriptors);
+                ++position;
+                return;
+            }
+            if (isHTMLSpace(*position)) {
+                appendDescriptorAndReset(attributeStart, currentDescriptorStart, position, descriptors);
+                currentDescriptorStart = position + 1;
+                state = AfterToken;
+            } else if (*position == '(') {
+                appendCharacter(currentDescriptorStart, position);
+                state = InParenthesis;
+            } else {
+                appendCharacter(currentDescriptorStart, position);
+            }
+            break;
+        case InParenthesis:
+            if (isEOF(position, attributeEnd)) {
+                appendDescriptorAndReset(attributeStart, currentDescriptorStart, attributeEnd, descriptors);
+                return;
+            }
+            if (*position == ')') {
+                appendCharacter(currentDescriptorStart, position);
+                state = Start;
+            } else {
+                appendCharacter(currentDescriptorStart, position);
+            }
+            break;
+        case AfterToken:
+            if (isEOF(position, attributeEnd))
+                return;
+            if (!isHTMLSpace(*position)) {
+                state = Start;
+                currentDescriptorStart = position;
+                --position;
+            }
+            break;
+        }
         ++position;
-        ASSERT(currentDescriptorEnd > currentDescriptorStart);
-        --currentDescriptorEnd;
-        unsigned descriptorLength = currentDescriptorEnd - currentDescriptorStart;
-        if (*currentDescriptorEnd == 'x') {
-            if (result.foundDescriptor())
+    }
+}
+
+template<typename CharType>
+static bool parseDescriptors(const CharType* attribute, Vector<DescriptorToken>& descriptors, DescriptorParsingResult& result)
+{
+    for (Vector<DescriptorToken>::iterator it = descriptors.begin(); it != descriptors.end(); ++it) {
+        if (it->length == 0)
+            continue;
+        CharType c = attribute[it->start + it->length - 1];
+        bool isValid = false;
+        if (RuntimeEnabledFeatures::pictureSizesEnabled() && c == 'w') {
+            if (result.foundDensity() || result.foundWidth())
                 return false;
-            result.scaleFactor = charactersToFloat(currentDescriptorStart, descriptorLength, &isValid);
-            if (!isValid || result.scaleFactor < 0)
+            result.resourceWidth = charactersToInt(attribute + it->start, it->length - 1, &isValid);
+            if (!isValid || result.resourceWidth <= 0)
                 return false;
-        } else if (RuntimeEnabledFeatures::pictureSizesEnabled() && *currentDescriptorEnd == 'w') {
-            if (result.foundDescriptor())
+        } else if (RuntimeEnabledFeatures::pictureSizesEnabled() && c == 'h') {
+            // This is here only for future compat purposes.
+            // The value of the 'h' descriptor is not used.
+            if (result.foundDensity() || result.foundHeight())
                 return false;
-            result.resourceWidth = charactersToInt(currentDescriptorStart, descriptorLength, &isValid);
-            if (!isValid || result.resourceWidth <= 0)
+            result.resourceHeight = charactersToInt(attribute + it->start, it->length - 1, &isValid);
+            if (!isValid || result.resourceHeight <= 0)
+                return false;
+        } else if (c == 'x') {
+            if (result.foundDensity() || result.foundHeight() || result.foundWidth())
+                return false;
+            result.density = charactersToFloat(attribute + it->start, it->length - 1, &isValid);
+            if (!isValid || result.density < 0)
                 return false;
         }
     }
-    if (isEmptyDescriptor)
-        result.scaleFactor = 1.0;
-    return result.foundDescriptor();
+    return true;
 }
 
-// http://www.whatwg.org/specs/web-apps/current-work/multipage/embedded-content-1.html#processing-the-image-candidates
+static bool parseDescriptors(const String& attribute, Vector<DescriptorToken>& descriptors, DescriptorParsingResult& result)
+{
+    // FIXME: See if StringView can't be extended to replace DescriptorToken here.
+    if (attribute.is8Bit()) {
+        return parseDescriptors(attribute.characters8(), descriptors, result);
+    }
+    return parseDescriptors(attribute.characters16(), descriptors, result);
+}
+
+// http://picture.responsiveimages.org/#parse-srcset-attr
 template<typename CharType>
 static void parseImageCandidatesFromSrcsetAttribute(const String& attribute, const CharType* attributeStart, unsigned length, Vector<ImageCandidate>& imageCandidates)
 {
@@ -92,33 +190,38 @@ static void parseImageCandidatesFromSrcsetAttribute(const String& attribute, con
     const CharType* attributeEnd = position + length;
 
     while (position < attributeEnd) {
-        DescriptorParsingResult result;
-        // 4. Splitting loop: Skip whitespace.
-        skipWhile<CharType, isHTMLSpace<CharType> >(position, attributeEnd);
-        if (position == attributeEnd)
+        // 4. Splitting loop: Collect a sequence of characters that are space characters or U+002C COMMA characters.
+        skipWhile<CharType, isHTMLSpaceOrComma<CharType> >(position, attributeEnd);
+        if (position == attributeEnd) {
+            // Contrary to spec language - descriptor parsing happens on each candidate, so when we reach the attributeEnd, we can exit.
             break;
-        const CharType* imageURLStart = position;
-
-        // If The current candidate is either totally empty or only contains space, skipping.
-        if (*position == ',') {
-            ++position;
-            continue;
         }
+        const CharType* imageURLStart = position;
+        // 6. Collect a sequence of characters that are not space characters, and let that be url.
 
-        // 5. Collect a sequence of characters that are not space characters, and let that be url.
         skipUntil<CharType, isHTMLSpace<CharType> >(position, attributeEnd);
         const CharType* imageURLEnd = position;
 
-        if (position != attributeEnd && *(position - 1) == ',') {
-            --imageURLEnd;
-            result.scaleFactor = 1.0;
+        DescriptorParsingResult result;
+
+        // 8. If url ends with a U+002C COMMA character (,)
+        if (isComma(*(position - 1))) {
+            // Remove all trailing U+002C COMMA characters from url.
+            imageURLEnd = position - 1;
+            reverseSkipWhile<CharType, isComma>(imageURLEnd, imageURLStart);
+            ++imageURLEnd;
+            // If url is empty, then jump to the step labeled splitting loop.
+            if (imageURLStart == imageURLEnd)
+                continue;
         } else {
-            // 7. Collect a sequence of characters that are not "," (U+002C) characters, and let that be descriptors.
-            skipWhile<CharType, isHTMLSpace<CharType> >(position, attributeEnd);
-            const CharType* descriptorsStart = position;
-            skipUntil<CharType, isComma<CharType> >(position, attributeEnd);
-            const CharType* descriptorsEnd = position;
-            if (!parseDescriptors(descriptorsStart, descriptorsEnd, result))
+            // Advancing position here (contrary to spec) to avoid an useless extra state machine step.
+            // Filed a spec bug: https://github.com/ResponsiveImagesCG/picture-element/issues/189
+            ++position;
+            Vector<DescriptorToken> descriptorTokens;
+            tokenizeDescriptors(attributeStart, position, attributeEnd, descriptorTokens);
+            // Contrary to spec language - descriptor parsing happens on each candidate.
+            // This is a black-box equivalent, to avoid storing descriptor lists for each candidate.
+            if (!parseDescriptors(attribute, descriptorTokens, result))
                 continue;
         }
 
@@ -151,16 +254,18 @@ static ImageCandidate pickBestImageCandidate(float deviceScaleFactor, unsigned s
     // http://picture.responsiveimages.org/#normalize-source-densities
     for (Vector<ImageCandidate>::iterator it = imageCandidates.begin(); it != imageCandidates.end(); ++it) {
         if (it->resourceWidth() > 0) {
-            it->setScaleFactor((float)it->resourceWidth() / (float)sourceSize);
+            it->setDensity((float)it->resourceWidth() / (float)sourceSize);
             ignoreSrc = true;
+        } else if (it->density() < 0) {
+            it->setDensity(1.0);
         }
     }
 
-    std::stable_sort(imageCandidates.begin(), imageCandidates.end(), compareByScaleFactor);
+    std::stable_sort(imageCandidates.begin(), imageCandidates.end(), compareByDensity);
 
     unsigned i;
     for (i = 0; i < imageCandidates.size() - 1; ++i) {
-        if ((imageCandidates[i].scaleFactor() >= deviceScaleFactor) && (!ignoreSrc || !imageCandidates[i].srcOrigin()))
+        if ((imageCandidates[i].density() >= deviceScaleFactor) && (!ignoreSrc || !imageCandidates[i].srcOrigin()))
             break;
     }
 
@@ -168,12 +273,12 @@ static ImageCandidate pickBestImageCandidate(float deviceScaleFactor, unsigned s
         ASSERT(i > 0);
         --i;
     }
-    float winningScaleFactor = imageCandidates[i].scaleFactor();
+    float winningDensity = imageCandidates[i].density();
 
     unsigned winner = i;
     // 16. If an entry b in candidates has the same associated ... pixel density as an earlier entry a in candidates,
     // then remove entry b
-    while ((i > 0) && (imageCandidates[--i].scaleFactor() == winningScaleFactor))
+    while ((i > 0) && (imageCandidates[--i].density() == winningDensity))
         winner = i;
 
     return imageCandidates[winner];
@@ -191,7 +296,7 @@ ImageCandidate bestFitSourceForSrcsetAttribute(float deviceScaleFactor, unsigned
 ImageCandidate bestFitSourceForImageAttributes(float deviceScaleFactor, unsigned sourceSize, const String& srcAttribute, const String& srcsetAttribute)
 {
     DescriptorParsingResult defaultResult;
-    defaultResult.scaleFactor = 1.0;
+    defaultResult.density = 1.0;
 
     if (srcsetAttribute.isNull()) {
         if (srcAttribute.isNull())
@@ -212,7 +317,7 @@ ImageCandidate bestFitSourceForImageAttributes(float deviceScaleFactor, unsigned
 String bestFitSourceForImageAttributes(float deviceScaleFactor, unsigned sourceSize, const String& srcAttribute, ImageCandidate& srcsetImageCandidate)
 {
     DescriptorParsingResult defaultResult;
-    defaultResult.scaleFactor = 1.0;
+    defaultResult.density = 1.0;
 
     if (srcsetImageCandidate.isEmpty())
         return srcAttribute;