Enable srcset support in HTMLImageElement

Source/core/html/parser/HTMLSrcsetParser.cpp

+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.

[abarth-chromium, 2013/09/19 18:09:52]

Same problem with the license block.

+
+#include "config.h"
+#include "core/html/parser/HTMLSrcsetParser.h"
+
+#include "core/html/parser/HTMLParserIdioms.h"
+#include "core/platform/ParsingUtilities.h"
+
+namespace WebCore {
+
+static inline bool compareByScaleFactor(const ImageCandidate& first, const ImageCandidate& second)

[abarth-chromium, 2013/09/19 18:09:52]

The |static| and |inline| keywords are redundant here.  I'd recommend just using
|static|.

+{
+    return first.scaleFactor() < second.scaleFactor();
+}
+
+// http://www.whatwg.org/specs/web-apps/current-work/multipage/embedded-content-1.html#processing-the-image-candidates
+template<typename CharType>
+static void parseImageCandidatesFromSrcsetAttribute(const String& attribute, const CharType* attributeStart, unsigned length, Vector<ImageCandidate>& imageCandidates)
+{
+    const CharType* position = attributeStart;
+    const CharType* attributeEnd = position + length;
+
+    while (position < attributeEnd) {
+        float imgScaleFactor = 1.0;
+        // 4. Splitting loop: Skip whitespace.
+        skipWhile<CharType, isHTMLSpace<CharType> >(position, attributeEnd);
+        if (position == attributeEnd)
+            break;
+        const CharType* imageURLStart = position;
+
+        // If The current candidate is either totally empty or only contains space, skipping.
+        if (*position == ',') {
+            ++position;
+            continue;
+        }
+        // 5. Collect a sequence of characters that are not space characters, and let that be url.
+        ++position;
+        skipWhile<CharType, isNotHTMLSpace<CharType> >(position, attributeEnd);

[abarth-chromium, 2013/09/19 18:09:52]

skipUntil<CharType, isHTMLSpaceOrComma<CharType> > ?

[Yoav Weiss, 2013/09/20 07:31:36]

skipUntil<CharType, isHTMLSpace<CharType> >

The algorithm shouldn't split on commas here, or data URIs will break.

+        const CharType* imageURLEnd = position;
+
+        if (position != attributeEnd && *(position - 1) == ',') {
+            --imageURLEnd;
+        } 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* qualifierStart = position;
+            if (position != attributeEnd && *position != ',') {
+                // This part differs from the spec as the current implementation only supports pixel density descriptors for now.
+                skipUntil<CharType, isHTMLSpaceOrComma<CharType> >(position, attributeEnd);
+                const CharType* qualifierEnd = position;
+                // Make sure there are no other descriptors
+                skipWhile<CharType, isHTMLSpace<CharType> >(position, attributeEnd);
+                // If the first non-html-space character after the scale modifier is not a comma,
+                // the current candidate is an invalid input.
+                if (position != attributeEnd && *position != ',') {
+                    skipUntil<CharType>(position, attributeEnd, ',');
+                    ++position;
+                    continue;
+                }
+                // If the current qualifier is not an 'x', the resource is ignored
+                if (*(qualifierEnd - 1) != 'x')

[abarth-chromium, 2013/09/19 18:09:52]

How do we know qualifierEnd - 1 is a valid address?  Maybe add an ASSERT at the
point where qualifierEnd becomes strictly greater than qualifierStart?

[Yoav Weiss, 2013/09/20 07:31:36]

When going into the skipUntil at line 51, we know that *position is not a white
space nor a comma.
Therefore, skipUntil must have advances, and qualifierEnd must be greater than
qualifierStart.

I'll add an ASSERT for that at line 53.
Should I also add a comment that explains the above?

[abarth-chromium, 2013/09/20 16:08:07]

I think an ASSERT is probably enough.

+                    continue;
+
+                bool validScaleFactor = false;
+                unsigned scaleFactorLengthWithoutUnit = qualifierEnd - qualifierStart - 1;
+                imgScaleFactor = charactersToFloat(qualifierStart, scaleFactorLengthWithoutUnit, &validScaleFactor);
+
+                if (!validScaleFactor)
+                    continue;
+            }
+        }
+
+        imageCandidates.append(ImageCandidate(attribute, imageURLStart - attributeStart, imageURLEnd - imageURLStart, imgScaleFactor));
+        // 11. Return to the step labeled splitting loop.
+    }
+}
+
+static void parseImageCandidatesFromSrcsetAttribute(const String& attribute, Vector<ImageCandidate>& imageCandidates)
+{
+    if (attribute.is8Bit())

[abarth-chromium, 2013/09/19 18:09:52]

What if attribute.isNull() ?  Calling is8Bit() on a null string will crash.  We
probably don't call these functions will null strings, but we might as well
handle that case and just return.

+        parseImageCandidatesFromSrcsetAttribute<LChar>(attribute, attribute.characters8(), attribute.length(), imageCandidates);
+    else
+        parseImageCandidatesFromSrcsetAttribute<UChar>(attribute, attribute.characters16(), attribute.length(), imageCandidates);
+}
+
+static ImageCandidate pickBestImageCandidate(float deviceScaleFactor, Vector<ImageCandidate>& imageCandidates)
+{
+    std::stable_sort(imageCandidates.begin(), imageCandidates.end(), compareByScaleFactor);
+
+    unsigned i;
+    for (i = 0; i < imageCandidates.size() - 1; ++i) {
+        if (imageCandidates[i].scaleFactor() >= deviceScaleFactor)
+            break;
+    }
+    return imageCandidates[i];

[abarth-chromium, 2013/09/19 18:09:52]

This will crash if imageCandidates is empty.

Rather than having the callers check imageCandidates.isEmpty(), we should handle
it in this function.

+}
+
+ImageCandidate bestFitSourceForSrcsetAttribute(float deviceScaleFactor, const String& srcsetAttribute)
+{
+    Vector<ImageCandidate> imageCandidates;
+
+    parseImageCandidatesFromSrcsetAttribute(srcsetAttribute, imageCandidates);
+    if (imageCandidates.isEmpty())
+        return ImageCandidate();
+    return pickBestImageCandidate(deviceScaleFactor, imageCandidates);
+}
+
+String bestFitSourceForImageAttributes(float deviceScaleFactor, const String& srcAttribute, const String& srcsetAttribute)
+{
+    if (srcsetAttribute.isNull())
+        return srcAttribute;

[abarth-chromium, 2013/09/19 18:09:52]

Or just:

return String()

[cbiesinger, 2013/09/19 23:01:44]

But that's not equivalent, right? If srcset is empty, we want to use src.

[abarth-chromium, 2013/09/20 16:08:07]

In this case, you're testing for isNull().  There's only one null string, and
it's the same one you get from calling String().

[abarth-chromium, 2013/09/20 16:08:58]

Oh, you're totally right.  Please ignore me.

+
+    Vector<ImageCandidate> imageCandidates;
+
+    parseImageCandidatesFromSrcsetAttribute(srcsetAttribute, imageCandidates);
+
+    if (!srcAttribute.isEmpty())
+        imageCandidates.append(ImageCandidate(srcAttribute, 0, srcAttribute.length(), 1.0));
+
+    if (imageCandidates.isEmpty())
+        return String();
+
+    return pickBestImageCandidate(deviceScaleFactor, imageCandidates).toString();
+}
+
+String bestFitSourceForImageAttributes(float deviceScaleFactor, const String& srcAttribute, ImageCandidate& srcsetImageCandidate)
+{
+    if (srcsetImageCandidate.isEmpty())
+        return srcAttribute;
+
+    Vector<ImageCandidate> imageCandidates;
+    imageCandidates.append(srcsetImageCandidate);
+
+    if (!srcAttribute.isEmpty())
+        imageCandidates.append(ImageCandidate(srcAttribute, 0, srcAttribute.length(), 1.0));
+
+    return pickBestImageCandidate(deviceScaleFactor, imageCandidates).toString();
+}
+
+}