Parse the link element's size attribute

Source/core/html/HTMLLinkElement.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  * Copyright (C) 2003, 2006, 2007, 2008, 2009, 2010 Apple Inc. All rights reserved.
  * Copyright (C) 2009 Rob Buis (rwlbuis@gmail.com)
  * Copyright (C) 2011 Google Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ skipping 29 matching lines @@
 #include "core/fetch/CSSStyleSheetResource.h"
 #include "core/fetch/FetchRequest.h"
 #include "core/fetch/ResourceFetcher.h"
 #include "core/frame/FrameView.h"
 #include "core/frame/LocalFrame.h"
 #include "core/frame/csp/ContentSecurityPolicy.h"
 #include "core/html/imports/LinkImport.h"
 #include "core/loader/FrameLoader.h"
 #include "core/loader/FrameLoaderClient.h"
 #include "wtf/StdLibExtras.h"
+#include "wtf/text/StringBuffer.h"
 
 namespace WebCore {
 
 using namespace HTMLNames;
 
 static LinkEventSender& linkLoadEventSender()
 {
     DEFINE_STATIC_LOCAL(LinkEventSender, sharedLoadEventSender, (EventTypeNames::load));
     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)
     , m_sizes(DOMSettableTokenList::create())
     , m_createdByParser(createdByParser)
     , m_isInShadowTree(false)
     , m_beforeLoadRecurseCount(0)
 {
     ScriptWrappable::init(this);
 }
@@ skipping 18 matching lines @@
     if (name == relAttr) {
         m_relAttribute = LinkRelAttribute(value);
         process();
     } else if (name == hrefAttr) {
         process();
     } else if (name == typeAttr) {
         m_type = value;
         process();
     } else if (name == sizesAttr) {
         m_sizes->setValue(value);
+        parseSizesAttribute(value, &m_iconSizes);
         process();
     } else if (name == mediaAttr) {
         m_media = value.string().lower();
         process();
     } else if (name == disabledAttr) {
         if (LinkStyle* link = linkStyle())
             link->setDisabledState(!value.isNull());
     } else if (name == onbeforeloadAttr)
         setAttributeEventListener(EventTypeNames::beforeload, createAttributeEventListener(this, name, value));
     else {
@@ skipping 231 matching lines @@
 const AtomicString& HTMLLinkElement::type() const
 {
     return getAttribute(typeAttr);
 }
 
 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
 {
     return m_sizes.get();
 }
 
 PassOwnPtr<LinkStyle> LinkStyle::create(HTMLLinkElement* owner)
 {
     return adoptPtr(new LinkStyle(owner));
@@ skipping 263 matching lines @@
 void LinkStyle::ownerRemoved()
 {
     if (m_sheet)
         clearSheet();
 
     if (styleSheetIsLoading())
         removePendingSheet(RemovePendingSheetNotifyLater);
 }
 
 } // namespace WebCore