[webkit-reviews] review denied: [Bug 65297] Add iterator to CSSValueList : [Attachment 102209] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Luke Macpherson
<macpherson at chromium.org>'s request for review:
Bug 65297: Add iterator to CSSValueList
https://bugs.webkit.org/show_bug.cgi?id=65297

Attachment 102209: Patch
https://bugs.webkit.org/attachment.cgi?id=102209&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=102209&action=review


review- because I think you will make at least one change based on my comments.


> Source/WebCore/css/CSSPrimitiveValue.h:122
> +    bool isLength() { return isUnitTypeLength(m_type); }

This should be a const member function.

> Source/WebCore/css/CSSStyleSelector.cpp:194
> +    for (CSSValueListIterator i = CSSValueListIterator(value); i.hasValue();
i.next()) { \

This seems a little wordy. Since the constructor is not marked explicit you can
do without the type name:

    [...] CSSValueListIterator i = value [...]

Same at many other call sites.

> Source/WebCore/css/CSSStyleSelector.cpp:4454
> -		   m_style->setTextShadow(shadowData.release(), i /* add to the
list if this is not the firsty entry */);
> +		   m_style->setTextShadow(shadowData.release(), !i.index() /*
add to the list if this is not the firsty entry */);

I suggest fixing the "firsty" typo and changing to a "//" comment if you are
touching this line.

> Source/WebCore/css/CSSStyleSelector.cpp:5269
> -    Length width;
> -    Length height;
> +    Length width, height;

This is an anti-pattern in WebKit code and so should not be changed.

> Source/WebCore/css/CSSStyleSelector.cpp:5272
> +    CSSValueListIterator iterator = CSSValueListIterator(value);
> +    switch (iterator.length()) {

This use is not iteration at all. So already, right out of the gate, the
iterator class does two separate things. I think this should not be using
iterator. You could build the iterator out of a base class that can be used
just to wrap the list if you like.

> Source/WebCore/css/CSSValueList.cpp:94
>      for (size_t index = 0; index < m_values.size(); index++)
> -	   newList->append(item(index));
> +	   newList->append(itemWithoutBoundsCheck(index));

It seems strange to use m_values on one line and then call
itemWithoutBoundsCheck on the next. Can we just index into m_values instead?

> Source/WebCore/css/CSSValueList.h:51
> -    CSSValue* item(unsigned);
> +    CSSValue* item(unsigned index) { return index < m_values.size() ?
m_values[index].get() : 0; }
>      CSSValue* itemWithoutBoundsCheck(unsigned index) { return
m_values[index].get(); }

Seems these should take size_t instead of unsigned.

> Source/WebCore/css/CSSValueList.h:76
> +class CSSValueListIterator {

An iterator seems OK but not great to me; a “why” comment about the value of
the iterator would be welcome. It seems the main value offered is the integral
call to isValueList and the type check. Another is the use of
itemWithoutBoundsCheck function without having to be so wordy about it.

> Source/WebCore/css/CSSValueList.h:78
> +    CSSValueListIterator(CSSValue* value) : m_position(0) { m_list =
value->isValueList() ? static_cast<CSSValueList*>(value) : 0; }

We don’t need this to work with a 0 for CSSValue*?

> Source/WebCore/css/CSSValueList.h:79
> +    bool hasValue() { return m_list && m_position < m_list->length(); }

I’m not really all that pleased with hasValue as the name for this. I don’t
think it’s really natural to ask if an iterator "has a value". It makes more
sense to ask if an iterator “is at the end of the things it iterates”. I
understand that it’s nice to have the sense reversed, but a name like atEnd
seems better and more straightforward. Or maybe hasMoreValues is a good name?

This should be a const member function.

> Source/WebCore/css/CSSValueList.h:80
> +    CSSValue* value() { ASSERT(hasValue()); return
m_list->itemWithoutBoundsCheck(m_position); }

This should be a const member function.

> Source/WebCore/css/CSSValueList.h:82
> +    CSSValue* first() { ASSERT(length() > 0); return
m_list->itemWithoutBoundsCheck(0); }
> +    CSSValue* second() { ASSERT(length() > 1); return
m_list->itemWithoutBoundsCheck(1); }

These are strange. They have nothing to do with iteration, so don’t seem to
belong on an iterator. They would make more sense as CSSValueList member
functions. I see how you want to use the iterator also as just a type-checked
pointer, but that seems too “clever” to me and makes this a dual-purpose class.


> Source/WebCore/css/CSSValueList.h:83
> +    void next() { m_position++; }

A function like this should have a verb for its name. Perhaps “advance”?

It also should assert that we are not advancing past the end.

> Source/WebCore/css/CSSValueList.h:84
> +    size_t index() { return m_position; }

This should be a const member function.

> Source/WebCore/css/CSSValueList.h:85
> +    size_t length() { return m_list ? m_list->length() : 0; }

You could write the hasValue function more simply using this function.

    return m_position < length();

This should be a const member function.

> Source/WebCore/css/CSSValueList.h:87
> +    CSSValueList* m_list;

A CSSValueList is a reference-counted object, and this holds a non-RefPtr
pointer to it. This hides potential lifetime issues inside the class, which is
not a step in the right direction.