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.
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.
Created attachment 217222 [details] style + changelog
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
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
Created attachment 217244 [details] fixes
Created attachment 217265 [details] rebase
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?
Created attachment 217329 [details] constification Thanks, Simon!
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>.
(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.
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.
(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.
(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.
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?
(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
(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.
Digging further, I see that we directly test non-API WebCore internals directly. Had no idea we did this. Nevermind, carry on.
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.
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.
Created attachment 217646 [details] testcase (for exposition only, do not use!)
(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!
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.
Created attachment 217968 [details] refactor per Anders' review
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?
(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
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?)
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.
Created attachment 218200 [details] split out IteratorPair
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.
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.
Comment on attachment 218257 [details] address Sam's comments Clearing flags on attachment: 218257 Committed r159992: <http://trac.webkit.org/changeset/159992>
All reviewed patches have been landed. Closing bug.