Bug 30772

Summary: [Qt] Review the API of QWebElementCollection
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: WebKit QtAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, kenneth, vestbo
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 30767    
Bug Blocks: 29843    
Attachments:
Description Flags
Add a non-const iterator to QWebElementCollection
none
Add a non-const iterator to QWebElementCollection
none
Add a non-const iterator to QWebElementCollection kenneth: review+

Benjamin Poulain
Reported 2009-10-26 06:16:38 PDT
QWebElementCollection has been added back in https://bugs.webkit.org/show_bug.cgi?id=30767. The current API is the original, unreviewed, one. Here is my suggestions: 1) Give a coherent API using iterator (related to (1)): -QWebElement QWebElementCollection::first() const +iterator QWebElementCollection::begin() -QWebElement QWebElementCollection::last() const +iterator QWebElementCollection::end() To be coherent with QList: +const_iterator QWebElementCollection::constBegin() const +const_iterator QWebElementCollection::constEnd() const 2) The API could be modified to enable lazy population in the future. The following changes would be required: -changes of (1) -int QWebElementCollection::count() const; -QWebElement QWebElementCollection::at(int i) const; -QWebElement QWebElementCollection::operator[](int i) const If the changes of (2) are made, it could make sense to remove QWebElement::findFirst() because it would be equivalent to QWebElement::findAll().begin().
Attachments
Add a non-const iterator to QWebElementCollection (6.45 KB, patch)
2009-11-09 08:53 PST, Benjamin Poulain 		no flags
DetailsFormatted DiffDiff
Add a non-const iterator to QWebElementCollection (8.94 KB, patch)
2009-11-09 09:08 PST, Benjamin Poulain 		no flags
DetailsFormatted DiffDiff
Add a non-const iterator to QWebElementCollection (8.94 KB, patch)
2009-11-09 09:13 PST, Benjamin Poulain 		kenneth: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Kenneth Rohde Christiansen
Comment 1 2009-10-26 10:58:00 PDT
(In reply to comment #0) > QWebElementCollection has been added back in > https://bugs.webkit.org/show_bug.cgi?id=30767. The current API is the original, > unreviewed, one. > > Here is my suggestions: > > 1) Give a coherent API using iterator (related to (1)): 1? > -QWebElement QWebElementCollection::first() const > +iterator QWebElementCollection::begin() > -QWebElement QWebElementCollection::last() const > +iterator QWebElementCollection::end() True, but QVector/QList actually have first() methods. > 2) The API could be modified to enable lazy population in the future. > The following changes would be required: > -changes of (1) > -int QWebElementCollection::count() const; > -QWebElement QWebElementCollection::at(int i) const; > -QWebElement QWebElementCollection::operator[](int i) const > > > If the changes of (2) are made, it could make sense to remove > QWebElement::findFirst() because it would be equivalent to > QWebElement::findAll().begin(). True, but it might be a bit harder to find and make the resulting code a bit more obscure.
Benjamin Poulain
Comment 2 2009-10-26 11:57:23 PDT
(In reply to comment #1) > (In reply to comment #0) > > 1) Give a coherent API using iterator (related to (1)): > > 1? My bad, I had splitted that in 3 before. > > -QWebElement QWebElementCollection::first() const > > +iterator QWebElementCollection::begin() > > -QWebElement QWebElementCollection::last() const > > +iterator QWebElementCollection::end() > > True, but QVector/QList actually have first() methods. I do not have anything against first(), I wanted to avoid last(). :) If we find the QWebElements on the fly when they are needed, first() is ok, but last() is not. > > 2) The API could be modified to enable lazy population in the future. > > The following changes would be required: > > -changes of (1) > > -int QWebElementCollection::count() const; > > -QWebElement QWebElementCollection::at(int i) const; > > -QWebElement QWebElementCollection::operator[](int i) const > > > > > > If the changes of (2) are made, it could make sense to remove > > QWebElement::findFirst() because it would be equivalent to > > QWebElement::findAll().begin(). > > True, but it might be a bit harder to find and make the resulting code a bit > more obscure. The justification I have heard for QWebElement::findFirst() is that findAll() is slow. So my idea was to get the element one by one and avoid the problem altogether. With QWebElement::findFirst(), I expect something different from QWebElement::nextSibling(). I would expect to be able to write something like this: QWebElement element = root.findFirst("p"); while (!element.isNull()) { // do something element = element.nextSibling("p"); // or element = element.nextSibling(); ? } But maybe I am biased because I have used jQuery.
Kenneth Rohde Christiansen
Comment 3 2009-10-26 12:12:05 PDT
> I do not have anything against first(), I wanted to avoid last(). :) > If we find the QWebElements on the fly when they are needed, first() is ok, but > last() is not. This is because last actually returns an element? so you mean that you need to load everything. Well, isnt that the same if you use end() and then get the element for that? If so, it could be noted in the documentation that is might be slow, and a similar comment could be added to the info about that class: "The collection supports lazy loading, thus any access to an element, will force loading all elements before it." > The justification I have heard for QWebElement::findFirst() is that findAll() > is slow. So my idea was to get the element one by one and avoid the problem > altogether. Yes, I agree that lazy loading is cool :-) Have you though about unloading as well? > With QWebElement::findFirst(), I expect something different from > QWebElement::nextSibling(). I would expect to be able to write something like > this: > > QWebElement element = root.findFirst("p"); > while (!element.isNull()) { > // do something > element = element.nextSibling("p"); > // or element = element.nextSibling(); ? > } nextSibling should give the real next sibling as the QWebElement represents a tree of the DOM. Thus, I do not like the element.nextSibling("p") API. It is confusing. I guess such functionality should only be added to the collection itself. > > But maybe I am biased because I have used jQuery.
Tor Arne Vestbø
Comment 4 2009-10-27 07:00:52 PDT
Feel free to add patch with non-const iterators, the others i think we should keep and document that they will be slow (when we add lazy init).
Benjamin Poulain
Comment 5 2009-11-09 08:53:42 PST
Created attachment 42758 [details] Add a non-const iterator to QWebElementCollection
Benjamin Poulain
Comment 6 2009-11-09 09:08:17 PST
Created attachment 42759 [details] Add a non-const iterator to QWebElementCollection
Benjamin Poulain
Comment 7 2009-11-09 09:13:19 PST
Created attachment 42760 [details] Add a non-const iterator to QWebElementCollection
Kenneth Rohde Christiansen
Comment 8 2009-11-09 09:23:01 PST
Landed in 50661
Note You need to log in before you can comment on or make changes to this bug.