[webkit-reviews] review denied: [Bug 119061] Setting up OrderIterator shouldn't require an extra Vector : [Attachment 230107] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Manuel Rego Casasnovas
<rego at igalia.com>'s request for review:
Bug 119061: Setting up OrderIterator shouldn't require an extra Vector
https://bugs.webkit.org/show_bug.cgi?id=119061

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

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


Looks good. review- because of the use of an iterator rather than an index for
m_childrenIterator.

> Source/WebCore/rendering/OrderIterator.cpp:59
> +static bool sortByOrderValueAndIndex(ChildIndex childIndex1, const
ChildIndex childIndex2)

The const here is neither needed nor helpful.

I don’t think the name “sort” is good for a less-than function. Maybe
“compareBy” or “isLessThanBy”?

> Source/WebCore/rendering/OrderIterator.cpp:74
> +    m_iterator.m_children.append(std::make_pair(&child, m_childIndex++));

No need for make_pair any more, can just use braces in C++11:

    m_iterator.m_children.append({ &child, m_childIndex++ });

> Source/WebCore/rendering/OrderIterator.h:42
> +typedef std::pair<RenderBox*, int> ChildIndex;

Seems a little overkill to typedef this. Also the name is too broad. ChildIndex
doesn’t give a rendering context, or even an OrderIterator context.

> Source/WebCore/rendering/OrderIterator.h:44
>  class OrderIterator {

This name also seems to broad. I think OrderedRendererChildIterator would be
better, or some other name like this.

> Source/WebCore/rendering/OrderIterator.h:55
> +    typedef Vector<ChildIndex> ChildrenVector;

No need for a typedef here.

> Source/WebCore/rendering/OrderIterator.h:57
> +    ChildrenVector::const_iterator m_childrenIterator;

Should use an index, not an iterator, to track a position within a vector. It’s
especially dangerous to have this if there’s any chance m_children might be
modified while the iterator is around, since the vector memory can be
reallocated.