Parse the link element's size attribute

Source/core/html/HTMLLinkElement.cpp

Index: Source/core/html/HTMLLinkElement.cpp
diff --git a/Source/core/html/HTMLLinkElement.cpp b/Source/core/html/HTMLLinkElement.cpp
index 9025639b7fae91cba4e8635db5337f4b3d32c78f..b3f41f5c972cff99e127fa733d8f353c8e9f8afa 100644
--- a/Source/core/html/HTMLLinkElement.cpp
+++ b/Source/core/html/HTMLLinkElement.cpp
@@ -47,6 +47,7 @@
 #include "core/loader/FrameLoader.h"
 #include "core/loader/FrameLoaderClient.h"
 #include "wtf/StdLibExtras.h"
+#include "wtf/text/StringBuffer.h"
 
 namespace WebCore {
 
@@ -58,6 +59,72 @@ static LinkEventSender& linkLoadEventSender()
     return sharedLoadEventSender;
 }
 
+void HTMLLinkElement::parseSizesAttribute(const AtomicString& value, Vector<IntSize>* iconSizes)
+{
+    ASSERT(!iconSizes->size());

[Inactive, 2014/03/12 23:27:13]

ASSERT(iconSizes.isEmpty()); would be more readable.

[michaelbai, 2014/03/13 05:50:11]

Done.

+    if (value.isNull() || value.isEmpty())

[Inactive, 2014/03/12 23:27:13]

if (value.isEmpty())

The isNull() check is superfluous as it is already included in your isEmpty()
check.

[michaelbai, 2014/03/13 05:50:11]

Done.

+        return;
+
+    enum State {
+        ParseStart,
+        ParseWidth,
+        ParseHeight
+    };
+    int width = 0;
+    StringBuffer<UChar> buff(value.length());

[Inactive, 2014/03/12 23:27:13]

We prefer not to use abbreviations. Why do we need a StringBuffer at all. Can't
we just track the start and the end with pointers in the original String and
then use something like WTF::charactersToInt(start, end - start)?

Adam probably has a good proposal here. :) Right now, having a StringBuffer +
constructing temporary Strings to convert to integer seems very inefficient to
me.

[michaelbai, 2014/03/13 05:50:11]

Can we assume the passed in |value| is always 8bit? otherwise,
WTF::charactersToInt probably can't use it here because we don't know
AtomicString::characters8() or AtomicString::characters16() should used.


On 2014/03/12 23:27:13, Chris Dumez wrote:

[Inactive, 2014/03/14 18:24:47]

I don't think you can make this assumption although I am not 100% sure. I
believe you need to handle both cases 8bit and 16bit. You can use CSSMarkup.cpp
as reference (it does something very similar to what you're doing and avoids
duplication between 8/16 bits cases using templates).

+    unsigned buffIndex = 0;

[Inactive, 2014/03/12 23:27:13]

Ditto

+    int i = 0;
+    UChar c = value[i++];
+    State state = ParseStart;
+    bool invalid = false;
+    while (c) {
+        if (state == ParseWidth) {
+            if (c >= '0' && c <= '9') {
+                buff[buffIndex++] = c;
+            } else if (c == 'x' || c == 'X') {
+                buff[buffIndex] = 0;
+                width = String(buff.characters()).toInt();
+                buffIndex = 0;
+                state = ParseHeight;
+            } else {
+                invalid = true;
+                break;
+            }
+        } else if (state == ParseHeight) {
+            if (c >= '0' && c <= '9') {
+                buff[buffIndex++] = c;
+            } else if (c == ' ') {
+                buff[buffIndex] = 0;
+                int height = String(buff.characters()).toInt();
+                iconSizes->append(IntSize(width, height));
+                buffIndex = 0;
+                state = ParseStart;
+            } else {
+                invalid = true;
+                break;
+            }
+        } else if (state == ParseStart) {
+            if (c >= '0' && c <= '9') {
+                buff[buffIndex++] = c;
+                state = ParseWidth;
+            } else if (c != ' ') {
+                invalid = true;
+                break;
+            }
+        }
+        c = value[i++];
+    }
+    if (invalid || state == ParseWidth || (state == ParseHeight && !buffIndex)) {
+        iconSizes->clear();
+        return;
+    }
+    if (state == ParseHeight && buffIndex > 0) {
+        buff[buffIndex] = 0;
+        int height = String(buff.characters()).toInt();
+        iconSizes->append(IntSize(width, height));
+    }
+}
+
 inline HTMLLinkElement::HTMLLinkElement(Document& document, bool createdByParser)
     : HTMLElement(linkTag, document)
     , m_linkLoader(this)
@@ -96,6 +163,7 @@ void HTMLLinkElement::parseAttribute(const QualifiedName& name, const AtomicStri
         process();
     } else if (name == sizesAttr) {
         m_sizes->setValue(value);
+        parseSizesAttribute(value, &m_iconSizes);
         process();
     } else if (name == mediaAttr) {
         m_media = value.string().lower();
@@ -347,9 +415,9 @@ IconType HTMLLinkElement::iconType() const
     return m_relAttribute.iconType();
 }
 
-const AtomicString& HTMLLinkElement::iconSizes() const
+const Vector<IntSize>& HTMLLinkElement::iconSizes() const
 {
-    return m_sizes->toString();
+    return m_iconSizes;
 }
 
 DOMSettableTokenList* HTMLLinkElement::sizes() const