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 34 matching lines @@
 #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"
 
 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)
     , 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