Bug 124533 – Ability to iterate over API::Array

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Martin Hock]

Created attachment 217221 [details]
patch

Currently, we always use the at<T>() method to get members out of an API::Array.  But this means we have to write a boilerplate index-based for loop and manually check if each call returns null.  Instead, we can instantiate an iterator for the type we are interested in, and that iterator is responsible for returning the members of its type.  We can also take advantage of the range based for loop syntax since API::Array is WebKit2 specific.

[WebKit Commit Bot]

Attachment 217221 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/Shared/APIArray.h', u'Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageEditorClient.cpp']" exit_code: 1
Source/WebKit2/Shared/APIArray.h:62:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebKit2/Shared/APIArray.h:97:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebKit2/Shared/APIArray.h:100:  Missing space inside { }.  [whitespace/braces] [5]
Total errors found: 3 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Hock]

Created attachment 217222 [details]
style + changelog

[EFL EWS Bot]

Comment on attachment 217222 [details]
style + changelog

Attachment 217222 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/27728075

[Tim Horton]

Comment on attachment 217222 [details]
style + changelog

View in context: https://bugs.webkit.org/attachment.cgi?id=217222&action=review

> Source/WebKit2/Shared/APIArray.h:67
> +        : it(it)
> +        , end(end)

these ought to be indented

[Martin Hock]

Created attachment 217244 [details]
fixes

[Martin Hock]

Created attachment 217265 [details]
rebase

[Simon Fraser (smfr)]

Comment on attachment 217265 [details]
rebase

View in context: https://bugs.webkit.org/attachment.cgi?id=217265&action=review

> Source/WebKit2/Shared/APIArray.h:82
> +        const T* operator*()

const function?

> Source/WebKit2/Shared/APIArray.h:89
> +        inline bool operator==(const_iterator<T> &other) { return it == other.it; }
> +        inline bool operator!=(const_iterator<T> &other) { return it != other.it; }

Should be const

> Source/WebKit2/Shared/APIArray.h:97
> +        const_iterator<T> begin()

const?

> Source/WebKit2/Shared/APIArray.h:102
> +        const_iterator<T> end()

const?

[Martin Hock]

Created attachment 217329 [details]
constification

Thanks, Simon!

[Daniel Bates]

Comment on attachment 217329 [details]
constification

View in context: https://bugs.webkit.org/attachment.cgi?id=217329&action=review

Can we write a TestWebKitAPI test for this? The tests are in <http://trac.webkit.org/browser/trunk/Tools/TestWebKitAPI/Tests/>. Specifically, the WebKit2 tests are in <http://trac.webkit.org/browser/trunk/Tools/TestWebKitAPI/Tests/WebKit2>. You may find it beneficial to look at an existing test as an example. You can run the tests by executing the script Tools/Scripts/run-test-webkit-api.

> Source/WebKit2/Shared/APIArray.h:62
> +        Vector<RefPtr<Object>>::const_iterator it;

Private instance variables should be prefixed with "m_" per (3) of section Names of the WebKit Code Style Guidelines, <http://www.webkit.org/coding/coding-style.html#names-data-members>.

Although it isn't formerly written in our code style guidelines and members of a class are private by default, I suggest explicitly adding a private label to demarcate that these instance variables are private.

> Source/WebKit2/Shared/APIArray.h:63
> +        Vector<RefPtr<Object>>::const_iterator end;

Private instance variables should be prefixed with "m_" per (3) of section Names of the WebKit Code Style Guidelines, <http://www.webkit.org/coding/coding-style.html#names-data-members>.

> Source/WebKit2/Shared/APIArray.h:73
> +        void operator++()

This is OK as-is when this iterator is used in a C++11 range-based for-loop. For your consideration, I suggest having this function return the iterator to conform to the expected behavior of the prefix operator.

> Source/WebKit2/Shared/APIArray.h:88
> +        inline bool operator==(const_iterator<T> &other) const { return it == other.it; }

Nit: & should be on the left per (2) of section Pointer and References of the WebKit Code Style Guidelines, <http://www.webkit.org/coding/coding-style.html#pointers-cpp>.

> Source/WebKit2/Shared/APIArray.h:89
> +        inline bool operator!=(const_iterator<T> &other) const { return it != other.it; }

Ditto.

> Source/WebKit2/Shared/APIArray.h:94
> +    const Vector<RefPtr<Object>>& m_elements;

Nit: This line should be indented. (*)

Although it isn't formerly written in our code style guidelines and members of a class are private by default, I suggest explicitly adding a private label to demarcate that these instance variables are private.

(*) We don't seem to explicitly document this in our code style guidelines. Although, it's implied in various examples and is consistent with the style of existing code in our repository. We should look to make this explicit in the WebKit Code Style Guidelines as well as standardize whether to include or omit the private label for class members.

> Source/WebKit2/Shared/APIArray.h:96
> +        iterator_factory(Vector<RefPtr<Object>> &elements) : m_elements(elements) { }

Nit: & should be on the left per (2) of section Pointer and References of the WebKit Code Style Guidelines, <http://www.webkit.org/coding/coding-style.html#pointers-cpp>.

[Brady Eidson]

(In reply to comment #9)
> (From update of attachment 217329 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217329&action=review
> 
> Can we write a TestWebKitAPI test for this? The tests are in <http://trac.webkit.org/browser/trunk/Tools/TestWebKitAPI/Tests/>. Specifically, the WebKit2 tests are in <http://trac.webkit.org/browser/trunk/Tools/TestWebKitAPI/Tests/WebKit2>. You may find it beneficial to look at an existing test as an example. You can run the tests by executing the script Tools/Scripts/run-test-webkit-api.

I disagree that a TestWebKitAPI test is necessary here, especially as this isn't WebKit API - It's internal to WebKit (even though it's related to an API implementor, there's no exposed API).

Also, knowing what larger task of Martin's this is part of, I'm confident it will be sufficiently tested by other code when the task is complete.

I don't think r-'ing based on this request is reasonable.

The style nits should definitely all be fixed.

[Brady Eidson]

As for the C++ iterator mechanics involved here, I think many folks on IRC were urging Martin to get Anders to take a look at this.

[Anders Carlsson]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 217329 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=217329&action=review
> > 
> > Can we write a TestWebKitAPI test for this? The tests are in <http://trac.webkit.org/browser/trunk/Tools/TestWebKitAPI/Tests/>. Specifically, the WebKit2 tests are in <http://trac.webkit.org/browser/trunk/Tools/TestWebKitAPI/Tests/WebKit2>. You may find it beneficial to look at an existing test as an example. You can run the tests by executing the script Tools/Scripts/run-test-webkit-api.
> 
> I disagree that a TestWebKitAPI test is necessary here, especially as this isn't WebKit API - It's internal to WebKit (even though it's related to an API implementor, there's no exposed API).

We have lots of tests for things that aren’t API in TestWebKitAPI. I think this is one of the features that it’d be great to have an API test for.

[Brady Eidson]

(In reply to comment #12)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (From update of attachment 217329 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=217329&action=review
> > > 
> > > Can we write a TestWebKitAPI test for this? The tests are in <http://trac.webkit.org/browser/trunk/Tools/TestWebKitAPI/Tests/>. Specifically, the WebKit2 tests are in <http://trac.webkit.org/browser/trunk/Tools/TestWebKitAPI/Tests/WebKit2>. You may find it beneficial to look at an existing test as an example. You can run the tests by executing the script Tools/Scripts/run-test-webkit-api.
> > 
> > I disagree that a TestWebKitAPI test is necessary here, especially as this isn't WebKit API - It's internal to WebKit (even though it's related to an API implementor, there's no exposed API).
> 
> We have lots of tests for things that aren’t API in TestWebKitAPI. I think this is one of the features that it’d be great to have an API test for.

This is something that needs a unit test, not an API test.  Do we unit test the rest of our collection classes?

AFAIK, we do not.  We rely on the fact that they are used in code in the rest of the project.

[Brady Eidson]

How would this *explicitly* be tested in TestWebKitAPI without exposing API for it?

And, being familiar with the larger task Martin is working on, do you not think that the API tests he'll be adding for the actual API he's working on won't also cover this?

[Anders Carlsson]

(In reply to comment #13)
> 
> This is something that needs a unit test, not an API test.  Do we unit test the rest of our collection classes?

Yes.

> 
> AFAIK, we do not.  We rely on the fact that they are used in code in the rest of the project.

This is incorrect. TestWebKitAPI has:

./Tests/WTF
./Tests/WTF/AtomicString.cpp
./Tests/WTF/cf
./Tests/WTF/cf/RetainPtr.cpp
./Tests/WTF/cf/RetainPtrHashing.cpp
./Tests/WTF/CheckedArithmeticOperations.cpp
./Tests/WTF/CString.cpp
./Tests/WTF/Deque.cpp
./Tests/WTF/Functional.cpp
./Tests/WTF/HashMap.cpp
./Tests/WTF/HashSet.cpp
./Tests/WTF/IntegerToStringConversion.cpp
./Tests/WTF/ListHashSet.cpp
./Tests/WTF/MathExtras.cpp
./Tests/WTF/MD5.cpp
./Tests/WTF/MediaTime.cpp
./Tests/WTF/MetaAllocator.cpp
./Tests/WTF/MoveOnly.h
./Tests/WTF/ns
./Tests/WTF/ns/RetainPtr.mm
./Tests/WTF/RedBlackTree.cpp
./Tests/WTF/Ref.cpp
./Tests/WTF/RefLogger.h
./Tests/WTF/RefPtr.cpp
./Tests/WTF/SaturatedArithmeticOperations.cpp
./Tests/WTF/SHA1.cpp
./Tests/WTF/StringBuilder.cpp
./Tests/WTF/StringHasher.cpp
./Tests/WTF/StringImpl.cpp
./Tests/WTF/StringOperators.cpp
./Tests/WTF/TemporaryChange.cpp
./Tests/WTF/Vector.cpp
./Tests/WTF/WTFString.cpp

[Brady Eidson]

(In reply to comment #15)
> (In reply to comment #13)
> > 
> > This is something that needs a unit test, not an API test.  Do we unit test the rest of our collection classes?
> 
> Yes.

I was definitely wrong.
 
> > 
> > AFAIK, we do not.  We rely on the fact that they are used in code in the rest of the project.
> 
> This is incorrect. TestWebKitAPI has:
> 
> ./Tests/WTF
> ./Tests/WTF/AtomicString.cpp
> ./Tests/WTF/cf
> ./Tests/WTF/cf/RetainPtr.cpp
> ./Tests/WTF/cf/RetainPtrHashing.cpp
> ./Tests/WTF/CheckedArithmeticOperations.cpp
> ./Tests/WTF/CString.cpp
> ./Tests/WTF/Deque.cpp
> ./Tests/WTF/Functional.cpp
> ./Tests/WTF/HashMap.cpp
> ./Tests/WTF/HashSet.cpp
> ./Tests/WTF/IntegerToStringConversion.cpp
> ./Tests/WTF/ListHashSet.cpp
> ./Tests/WTF/MathExtras.cpp
> ./Tests/WTF/MD5.cpp
> ./Tests/WTF/MediaTime.cpp
> ./Tests/WTF/MetaAllocator.cpp
> ./Tests/WTF/MoveOnly.h
> ./Tests/WTF/ns
> ./Tests/WTF/ns/RetainPtr.mm
> ./Tests/WTF/RedBlackTree.cpp
> ./Tests/WTF/Ref.cpp
> ./Tests/WTF/RefLogger.h
> ./Tests/WTF/RefPtr.cpp
> ./Tests/WTF/SaturatedArithmeticOperations.cpp
> ./Tests/WTF/SHA1.cpp
> ./Tests/WTF/StringBuilder.cpp
> ./Tests/WTF/StringHasher.cpp
> ./Tests/WTF/StringImpl.cpp
> ./Tests/WTF/StringOperators.cpp
> ./Tests/WTF/TemporaryChange.cpp
> ./Tests/WTF/Vector.cpp
> ./Tests/WTF/WTFString.cpp

Key distinction - Those all seem to be tests of WTF API.

To reiterate, what this patch add's is not in and of itself API.

[Brady Eidson]

Digging further, I see that we directly test non-API WebCore internals directly.

Had no idea we did this.

Nevermind, carry on.

[Alexey Proskuryakov]

Testing WebKit2 internals would be new territory though - WebCore internals setup is pretty complicated, and doesn't allow for WebKit layer testing.

Does anyone have an idea for how to add unit testing without a major effort? Otherwise, maybe that shouldn't block landing, provided that this code will be well tested functionally soon.

[Martin Hock]

Created attachment 217645 [details]
implement using FilterIterator

After discussing with Anders, I wrote this in terms of a FilterIterator variant.  We also decided that it would be difficult to add a unit test for WebKit2 internals to TestWebKitAPI, and the code is indirectly tested through PasteboardNotifications.  I did create a unit test which I'll upload as another attachment, which I could get to compile through some bad and incorrect C preprocessor trickery, but it would not link.

[Martin Hock]

Created attachment 217646 [details]
testcase (for exposition only, do not use!)

[Brady Eidson]

(In reply to comment #19)
> Created an attachment (id=217645) [details]
> 
> ...We also decided that it would be difficult to add a unit test for WebKit2 internals to TestWebKitAPI...

This is what I thought!

[Anders Carlsson]

Comment on attachment 217645 [details]
implement using FilterIterator

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.

[Martin Hock]

Created attachment 217968 [details]
refactor per Anders' review

[Martin Hock]

Comment on attachment 217645 [details]
implement using FilterIterator

View in context: https://bugs.webkit.org/attachment.cgi?id=217645&action=review

>> Source/WebKit2/Shared/FilterIterator.h:40
>> +        : m_pred(pred)
> 
> You should std::move here.

I didn't do this as I wasn't sure the benefit of std::move applied to a function pointer.  How is it more efficient than simply copying the address?

For future reference, could you point me to some information about good uses of std::move?

[Brady Eidson]

(In reply to comment #24)
> (From update of attachment 217645 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217645&action=review
> 
> >> Source/WebKit2/Shared/FilterIterator.h:40
> >> +        : m_pred(pred)
> > 
> > You should std::move here.
> 
> I didn't do this as I wasn't sure the benefit of std::move applied to a function pointer.  How is it more efficient than simply copying the address?

Because of the templatization, Predicate is not always just a function pointer.  It could be any class that has a function call operator.  In the case of a more complex class, std::move'ing would be beneficial.

Since it's not a *detriment* to the function pointer case, may as well do it!

> For future reference, could you point me to some information about good uses of std::move?

Anders recently shared this with me - It should be required reading for all of us C++ 11'ers!  (Warning: Covers way more than just std::move)
http://thbecker.net/articles/rvalue_references/section_01.html

[Martin Hock]

Created attachment 218179 [details]
use std::move for pass-by-value arguments

Thanks for the reference, Brady.  It seems like as a general rule of thumb, one should always std::move constructor arguments passed by value into an initializer list (assuming said arguments are not used elsewhere in the constructor).  This avoids an extra copy (does the compiler usually optimize away this kind of copy?)

[Anders Carlsson]

Comment on attachment 218179 [details]
use std::move for pass-by-value arguments

View in context: https://bugs.webkit.org/attachment.cgi?id=218179&action=review

> Source/WebKit2/Shared/FilterIterator.h:75
> +    class Iterable {

I think you can call this IteratorPair instead. Also, why not make it a class template allowing an arbitrary iterator type.

> Source/WebKit2/Shared/FilterIterator.h:77
> +    private:
> +        FilterIterator m_iter, m_end;

Private should go after public.

> Source/WebKit2/Shared/FilterIterator.h:82
> +        { }

braces should go on separate lines. Missing newline after the constructor.

> Source/WebKit2/Shared/FilterIterator.h:84
> +        FilterIterator& begin() { return m_iter; }
> +        FilterIterator& end() { return m_end; }

I don’t think this should return references.

> Source/WebKit2/Shared/FilterIterator.h:86
> +

Extra newline.

[Martin Hock]

Created attachment 218200 [details]
split out IteratorPair

[Sam Weinig]

Comment on attachment 218200 [details]
split out IteratorPair

View in context: https://bugs.webkit.org/attachment.cgi?id=218200&action=review

One more pass!

> Source/WebKit2/Shared/APIArray.h:41
> +    static inline const T *getObject(const RefPtr<Object>& object) { return static_cast<const T *>(object.get()); }

*s are on the wrong side.

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

& on the wrong side.

> Source/WebKit2/Shared/FilterIterator.h:43
> +private:
> +    const Predicate m_pred;
> +    const Cast m_cast;
> +    Iterator m_iter;
> +    Iterator m_end;
> +

This should go at the bottom, under the public part.

> Source/WebKit2/Shared/IteratorPair.h:36
> +    Iterator end() { return m_end; }
> +private:

Please put a newline between these two.

[Martin Hock]

Created attachment 218257 [details]
address Sam's comments

Turns out there was a way to avoid using auto return type via std::declval (this was the reason for putting the private members first).  Thanks to Anders for the hint.  I tried using declval before, and previously understand how to apply it in this case, but the expression I used for the auto return type could be adapted into an expression involving declvals.

[WebKit Commit Bot]

Comment on attachment 218257 [details]
address Sam's comments

Clearing flags on attachment: 218257

Committed r159992: <http://trac.webkit.org/changeset/159992>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.