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..f333d09bacc101f43a4beac12bd706fd3473eae6 100644
--- a/Source/core/html/HTMLLinkElement.cpp
+++ b/Source/core/html/HTMLLinkElement.cpp
@@ -52,12 +52,46 @@ namespace WebCore {
 
 using namespace HTMLNames;
 
+const String sizeSeparator = "x";

[Inactive, 2014/03/12 20:09:52]

This could be a UChar.

+
 static LinkEventSender& linkLoadEventSender()
 {
     DEFINE_STATIC_LOCAL(LinkEventSender, sharedLoadEventSender, (EventTypeNames::load));
     return sharedLoadEventSender;
 }
 
+void HTMLLinkElement::parseSizesAttribute(const AtomicString& value, Vector<IntSize>* iconSizes)

[Inactive, 2014/03/12 20:09:52]

The Vector could be passed by reference instead of pointer.

[michaelbai, 2014/03/12 23:08:41]

iconSizes will change and should be pass as pointer, right?

On 2014/03/12 20:09:52, Chris Dumez wrote:

[Inactive, 2014/03/12 23:14:38]

I said a by reference, not by *const* reference. 

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

I has been told to use pointer in this case, I searched blink-dev, it seemed we
decide to use reference here. 

On 2014/03/12 23:14:38, Chris Dumez wrote:

+{

[Inactive, 2014/03/12 20:09:52]

Should we add an assertion to make sure the Vector is initially empty?

[michaelbai, 2014/03/12 23:08:41]

Done.

+    RefPtr<DOMSettableTokenList> tokenList = DOMSettableTokenList::create();

[abarth-chromium, 2014/03/12 19:42:34]

Why are we using a DOMSettableTokenList here?

+    tokenList->setValue(value.lower());
+    const SpaceSplitString& tokens = tokenList->tokens();
+    bool successful = true;
+    for (unsigned i = 0; i < tokens.size(); i++) {

[abarth-chromium, 2014/03/12 19:42:34]

++i

+        const String& sizeString = (String&)tokens[i];

[abarth-chromium, 2014/03/12 19:42:34]

Why is (String&) needed here?  We generally don't use C-style casts, and I'm not
sure why we'd need a cast at all.

+        Vector<String> result;
+        sizeString.split(sizeSeparator, result);
+        if (result.size() != 2) {
+            successful = false;
+            break;
+        }
+        bool ok = false;
+        int width = result[0].toUInt(&ok);
+        if (!ok) {
+            successful = false;
+            break;
+        }
+        ok = false;
+        int height = result[1].toUInt(&ok);
+        if (!ok) {
+            successful = false;
+            break;
+        }
+        iconSizes->append(IntSize(width, height));
+    }
+    if (!successful)
+        iconSizes->clear();
+}

[abarth-chromium, 2014/03/12 19:42:34]

This function is very slow.  Why not write a version that just walks the string
and parses it?

[michaelbai, 2014/03/12 23:08:41]

I were thinking I should use existing utility, changed to parse it.

On 2014/03/12 19:42:34, abarth wrote:

+
 inline HTMLLinkElement::HTMLLinkElement(Document& document, bool createdByParser)
     : HTMLElement(linkTag, document)
     , m_linkLoader(this)
@@ -96,6 +130,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 +382,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