[webkit-reviews] review denied: [Bug 124533] Ability to iterate over API::Array : [Attachment 217645] implement using FilterIterator

[bugzilla-daemon at webkit.org]

Anders Carlsson <andersca at apple.com> has denied Martin Hock <mhock at apple.com>'s
request for review:
Bug 124533: Ability to iterate over API::Array
https://bugs.webkit.org/show_bug.cgi?id=124533

Attachment 217645: implement using FilterIterator
https://bugs.webkit.org/attachment.cgi?id=217645&action=review

------- Additional Comments from Anders Carlsson <andersca at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=217645&action=review


> Source/WebKit2/Shared/APIArray.h:47
> +    inline static bool isType(const RefPtr<Object> &object) { return
object->type() == T::APIType; }

I think this can be private. We also put static before inline.

> Source/WebKit2/Shared/APIArray.h:69
> +    typedef Vector<RefPtr<Object>>::iterator iterator;
> +    typedef Vector<RefPtr<Object>>::const_iterator const_iterator;
> +    iterator begin() { return m_elements.begin(); }
> +    iterator end() { return m_elements.end(); }
> +    const_iterator begin() const { return m_elements.begin(); }
> +    const_iterator end() const { return m_elements.end(); }

I don’t think these are all needed, can just use m_elements.begin() and
m_elements.end().

> Source/WebKit2/Shared/APIArray.h:73
> +    {

it() is not a good member function name. How about calling it elementsOfType? 

(It should also not return a FilterIterator but an object that has begin and
end members - see my comment in FilterIterator.h

> Source/WebKit2/Shared/FilterIterator.h:36
> +template<typename Predicate, typename Iterator, typename T>

There shouldn’t be a need for a third type parameter here. See my comment in
iterator*.

> Source/WebKit2/Shared/FilterIterator.h:40
> +	   : m_pred(pred)

You should std::move here.

> Source/WebKit2/Shared/FilterIterator.h:58
> +    const T& operator*() const

Instead of returning a const T& here, you should return a const
std::iterator_traits<typename Iterator>::reference.

> Source/WebKit2/Shared/FilterIterator.h:62
> +	   return reinterpret_cast<const T&>(*m_iter);

As you suspected, this part is wrong - operator* should not do any reinterpret
casts.

> Source/WebKit2/Shared/FilterIterator.h:69
> +    FilterIterator& begin() { return *this; }
> +    FilterIterator end() { return FilterIterator(m_pred, m_end, m_end); }

I don’t think the iterator itself should have these. Instead, there should be
an object that you can create from a pair of iterators. That object should have
begin  and end members.

>
Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageEditorClient.cpp:157

> +	   for (auto type : typesArray->it<WebString>())

This should be a const auto& to avoid reefing and dereffing over and over.

>
Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageEditorClient.cpp:160

> +	   for (auto item : dataArray->it<WebData>()) {

This should be a const auto& to avoid ref churn.