Bug 41081 - Add a NodeList-derivated wrapper class for a ListHashSet
Summary: Add a NodeList-derivated wrapper class for a ListHashSet
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Antonio Gomes
URL:
Keywords:
Depends on:
Blocks: 40197
  Show dependency treegraph
 
Reported: 2010-06-23 10:58 PDT by Antonio Gomes
Modified: 2010-07-29 08:13 PDT (History)
10 users (show)

See Also:


Attachments
patch v1 (14.60 KB, patch)
2010-06-23 11:13 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
patch v1.1 - fixed style issues (14.52 KB, patch)
2010-06-23 11:23 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
(commit r61818, r=dhyatt) patch v1.2 (14.43 KB, patch)
2010-06-23 20:43 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antonio Gomes 2010-06-23 10:58:26 PDT
This is a preparation work for bug 40197, which is about extending the current hit testing system to also support rectangular hit area, instead of only points. As discussed by hyatt in https://bugs.webkit.org/show_bug.cgi?id=40197#c19 , a ListHashSet structure is to be used to store the hit-tested nodes. In order to expose this rect-based hit test functionality to the DOM, a nodesFromRect method will be added to Document class, and it will return a NodeList wrapping this ListHashSet.

This bug intends to provide this wrapper class.
Comment 1 Antonio Gomes 2010-06-23 11:13:53 PDT
Created attachment 59533 [details]
patch v1

I have not added LayoutTests for this new NodeList at the moment, because it is only going to be used by patch to be submitted in bug 40197 - RefPrt<NodeList> Document::nodesFromRect const will use it/expose it.
Comment 2 WebKit Review Bot 2010-06-23 11:16:30 PDT
Attachment 59533 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/dom/StaticHashSetNodeList.h:40:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/dom/StaticHashSetNodeList.cpp:46:  Semicolon defining empty statement for this loop. Use { } instead.  [whitespace/semicolon] [5]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Antonio Gomes 2010-06-23 11:23:46 PDT
Created attachment 59539 [details]
patch v1.1 - fixed style issues
Comment 4 Antonio Gomes 2010-06-23 20:43:16 PDT
Created attachment 59608 [details]
(commit r61818, r=dhyatt) patch v1.2

Removed an unnecessery FIXME in the code (due to accidental copy & paste) pointed out by dhyatt on IRC
Comment 5 Dave Hyatt 2010-06-24 12:46:20 PDT
Comment on attachment 59608 [details]
(commit r61818, r=dhyatt) patch v1.2

r=me
Comment 6 Antonio Gomes 2010-06-24 21:42:22 PDT
Comment on attachment 59608 [details]
(commit r61818, r=dhyatt) patch v1.2

Clearing flags on attachment: 59608

Committed r61818: <http://trac.webkit.org/changeset/61818>
Comment 7 Darin Adler 2010-06-25 12:39:11 PDT
Why a ListHashSet and not a Vector?
Comment 8 Darin Adler 2010-06-25 12:39:31 PDT
Once the hit test is done, it would be best to copy the contents of the ListHashSet into a Vector.
Comment 9 Antonio Gomes 2010-06-25 13:13:31 PDT
(In reply to comment #8)
> Once the hit test is done, it would be best to copy the contents of the ListHashSet into a Vector.

We were using Vector in previous versions of the rect-based HitTest patch, but it was suggested to use ListHashSet. see:

https://bugs.webkit.org/show_bug.cgi?id=40197#c11
https://bugs.webkit.org/show_bug.cgi?id=40197#c15
https://bugs.webkit.org/show_bug.cgi?id=40197#c21

I understand your concern, though. It is probably because every NodeList::item(int n) operation has to iterate over the ListHashSet, which is O(n). With a Vector it'd be O(1) , since we can just do Vector::at(int n). Is that the case, Darin?
Comment 10 Darin Adler 2010-06-25 13:29:11 PDT
It’s fine to use ListHashSet to build up the list; a good efficient way to check for duplicates and membership while creating it. Once the list is created, though, moving to a Vector makes sense to me.
Comment 11 Darin Adler 2010-06-25 13:29:39 PDT
(In reply to comment #9)
> I understand your concern, though. It is probably because every NodeList::item(int n) operation has to iterate over the ListHashSet, which is O(n). With a Vector it'd be O(1) , since we can just do Vector::at(int n). Is that the case, Darin?

That’s one of my concerns. The other is that a ListHashSet uses a lot of extra memory.
Comment 12 Antonio Gomes 2010-07-05 12:01:36 PDT
(In reply to comment #11)
> (In reply to comment #9)
> > I understand your concern, though. It is probably because every NodeList::item(int n) operation has to iterate over the ListHashSet, which is O(n). With a Vector it'd be O(1) , since we can just do Vector::at(int n). Is that the case, Darin?
> 
> That’s one of my concerns. The other is that a ListHashSet uses a lot of extra memory.

Darin, I am really open to make it as you suggested: copy the listhashset over a vector after hittest is ready, and expose it as a StaticNodeList instead of as the added StaticHashSetNodeList. In this case, we can also discuss and decide if we need this StaticHashSetListNode at all, or just remove it.

If we decide to keep it, I will templatize StaticListNode to either work with a Vector or a ListHashSet, as talked to Dyatt on irc, merging StaticHashSetListNode into StaticListNode anyways.

I will try to get the core patch (in bug 40197) in a better shape first, then get back to this.
Comment 13 Simon Hausmann 2010-07-29 08:13:11 PDT
Revision r61818 cherry-picked into qtwebkit-2.1 with commit 8a8a28d007539792bf63f36817cfc3abdf994555