[Qt] Review the API of QWebElementCollection

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Benjamin Poulain]

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().

[Kenneth Rohde Christiansen]

(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]

(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]

> 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ø]

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]

Created attachment 42758 [details]
Add a non-const iterator to QWebElementCollection

[Benjamin Poulain]

Created attachment 42759 [details]
Add a non-const iterator to QWebElementCollection

[Benjamin Poulain]

Created attachment 42760 [details]
Add a non-const iterator to QWebElementCollection

[Kenneth Rohde Christiansen]

Landed in 50661