[Webkit-unassigned] [Bug 22379] Make CSSOM use less memory

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=22379


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #25768|review?                     |review+
               Flag|                            |




------- Comment #15 from darin at apple.com  2008-12-05 06:03 PDT -------
(From update of attachment 25768)
> +    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


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.