Bug 30772 - [Qt] Review the API of QWebElementCollection
Summary: [Qt] Review the API of QWebElementCollection
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt
Depends on: 30767
Blocks: 29843
  Show dependency treegraph
 
Reported: 2009-10-26 06:16 PDT by Benjamin Poulain
Modified: 2009-11-09 09:23 PST (History)
3 users (show)

See Also:


Attachments
Add a non-const iterator to QWebElementCollection (6.45 KB, patch)
2009-11-09 08:53 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Add a non-const iterator to QWebElementCollection (8.94 KB, patch)
2009-11-09 09:08 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Add a non-const iterator to QWebElementCollection (8.94 KB, patch)
2009-11-09 09:13 PST, Benjamin Poulain
kenneth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 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().
Comment 1 Kenneth Rohde Christiansen 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.
Comment 2 Benjamin Poulain 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.
Comment 3 Kenneth Rohde Christiansen 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.
Comment 4 Tor Arne Vestbø 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).
Comment 5 Benjamin Poulain 2009-11-09 08:53:42 PST
Created attachment 42758 [details]
Add a non-const iterator to QWebElementCollection
Comment 6 Benjamin Poulain 2009-11-09 09:08:17 PST
Created attachment 42759 [details]
Add a non-const iterator to QWebElementCollection
Comment 7 Benjamin Poulain 2009-11-09 09:13:19 PST
Created attachment 42760 [details]
Add a non-const iterator to QWebElementCollection
Comment 8 Kenneth Rohde Christiansen 2009-11-09 09:23:01 PST
Landed in 50661