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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 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 232071: Patch
https://bugs.webkit.org/attachment.cgi?id=232071&action=review

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


> Source/WebCore/rendering/OrderIterator.cpp:94
> +    OrderIterator::OrderValues& orderValues = m_iterator.m_orderValues;

I suggest auto& here.

> Source/WebCore/rendering/OrderIterator.h:46
> +    OrderIterator(RenderBox&);

Should mark this explicit.

> Source/WebCore/rendering/OrderIterator.h:58
> +    typedef Vector<int, 1> OrderValues;

I don’t think we need a typedef for this. The only places that would use this
type should just use auto instead.

> Source/WebCore/rendering/OrderIterator.h:73
> +    OrderIteratorPopulator(OrderIterator& iterator)
> +	   : m_iterator(iterator)
> +    {
> +	   // Note that we don't release the memory here, we only invalidate
the size
> +	   // This avoids unneeded reallocation if the size ends up not
changing.
> +	   m_iterator.m_orderValues.shrink(0);
> +    }
> +
> +    ~OrderIteratorPopulator();

I’m not sure I understand why the constructor is inlined and included here in
the header but none of the other functions are. It would be nice to be
consistent. Maybe no inlining is the right way to start.

> Source/WebCore/rendering/OrderIterator.h:78
> +

Please omit this blank line.