[webkit-reviews] review granted: [Bug 22379] Make CSSOM use less memory : [Attachment 25768] switch CSSMutableStyleDeclaration out from the DeprecatedValueList.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 22379: Make CSSOM use less memory
https://bugs.webkit.org/show_bug.cgi?id=22379

Attachment 25768: switch CSSMutableStyleDeclaration out from the
DeprecatedValueList.
https://bugs.webkit.org/attachment.cgi?id=25768&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    Vector<CSSProperty>::const_iterator it = findPropertyWithId(propertyID);


Something I always tell people who are working with Vector is that iterators
are just pointers and thus I think it's unnecessary to use the typedef or use
end() to indicate "not found". So I would have done:

    const CSSProperty* property = findPropertyWithId(propertyID);

And I would have make it return 0 when not found rather than end().

> +    String value = returnText ? (*it).value()->cssText() : String();

I think it->value() reads better than (*it).value.

> +	       (*it) = property;

No parentheses needed here.

>  void CSSMutableStyleDeclaration::removePropertiesInSet(const int* set,
unsigned length, bool notifyChanged)
>  {
> -    bool changed = false;
> -    for (unsigned i = 0; i < length; i++) {
> -	   RefPtr<CSSValue> value = getPropertyCSSValue(set[i]);
> -	   if (value) {
> -	       m_values.remove(CSSProperty(set[i], value.release(), false));
> -	       changed = true;
> +    ASSERT(!m_iteratorCount);
> +    
> +    if (m_properties.isEmpty())
> +	   return;
> +    
> +    // FIXME: This is often used with static sets and in that case
constructing the hash repeatedly is pretty pointless.
> +    HashSet<unsigned> toRemove;
> +    for (unsigned i = 0; i < length; i++)
> +	   toRemove.add(set[i]);

I think we should change this function to take a HashSet and make a helper
function to use with it. I think the function is *always* used with static
sets.

I don't understand why you chose HashSet<unsigned> rather than HashSet<int> or
HashSet<CSSPropertyID>.

> Index: WebCore/css/CSSProperty.h
> ===================================================================
> --- WebCore/css/CSSProperty.h (revision 39020)
> +++ WebCore/css/CSSProperty.h (working copy)
> @@ -45,6 +45,7 @@ public:
>	   m_id = other.m_id;
>	   m_shorthandID = other.m_shorthandID;
>	   m_important = other.m_important;
> +	   m_implicit = other.m_implicit;
>	   m_value = other.m_value;
>	   return *this;
>      }


Maybe you could land this change sepaarately?

> +namespace WTF {
> +    template<> struct VectorTraits<WebCore::CSSProperty> :
SimpleClassVectorTraits { };
> +}

I think this needs a comment to explain what it means and why we're doing it.

> -    Vector<int> properties;
> -    DeprecatedValueListConstIterator<CSSProperty> end;
> -    for (DeprecatedValueListConstIterator<CSSProperty>
it(style->valuesIterator()); it != end; ++it) {
> -	   const CSSProperty& property = *it;
> -	   RefPtr<CSSValue> value = getPropertyCSSValue(property.id());
> -	   if (value && (value->cssText() == property.value()->cssText()))
> -	       properties.append(property.id());
> +    Vector<int> propertiesToRemove;
> +    {
> +	   CSSMutableStyleDeclaration::const_iterator end = style->end();
> +	   for (CSSMutableStyleDeclaration::const_iterator it = style->begin();
it != end; ++it) {
> +	       const CSSProperty& property = *it;
> +	       RefPtr<CSSValue> value = getPropertyCSSValue(property.id());
> +	       if (value && (value->cssText() == property.value()->cssText()))
> +		   propertiesToRemove.append(property.id());
> +	   }
>      }
>  
> -    for (unsigned i = 0; i < properties.size(); i++)
> -	   style->removeProperty(properties[i]);
> +    // FIXME: This should use mass removal.
> +    for (unsigned i = 0; i < propertiesToRemove.size(); i++)
> +	   style->removeProperty(propertiesToRemove[i]);
>  }

Why not change this to use removePropertiesFromSet now? If you're going to add
a comment, please mention the name of the function specifically.

How did you test the impact on speed?

r=me