Bug 103364 - HTMLCollection on Document should be stored on NodeListsNodeData like other HTMLCollections and LiveNodeLists
Summary: HTMLCollection on Document should be stored on NodeListsNodeData like other H...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 78909
  Show dependency treegraph
 
Reported: 2012-11-26 22:08 PST by Ryosuke Niwa
Modified: 2012-11-27 12:17 PST (History)
9 users (show)

See Also:


Attachments
Cleanup (36.32 KB, patch)
2012-11-27 01:31 PST, Ryosuke Niwa
andersca: review+
rniwa: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2012-11-26 22:08:30 PST
HTMLCollection on Document should use NodeRareData like other HTMLCollection and LiveNodeList
Comment 1 Ryosuke Niwa 2012-11-27 01:31:41 PST
Created attachment 176196 [details]
Cleanup
Comment 2 Ryosuke Niwa 2012-11-27 11:27:21 PST
Comment on attachment 176196 [details]
Cleanup

Oh, oops. This changelog contains the old bug title :(
Comment 3 Ryosuke Niwa 2012-11-27 11:59:24 PST
Committed r135893: <http://trac.webkit.org/changeset/135893>
Comment 4 Darin Adler 2012-11-27 12:14:56 PST
Comment on attachment 176196 [details]
Cleanup

View in context: https://bugs.webkit.org/attachment.cgi?id=176196&action=review

> Source/WebCore/dom/NodeRareData.h:79
> +    PassRefPtr<T> addCacheWithAtomicName(Node* node, CollectionType collectionType)

Could this have been ContainerNode* across the board instead of Node*?
Comment 5 Ryosuke Niwa 2012-11-27 12:17:36 PST
(In reply to comment #4)
> (From update of attachment 176196 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176196&action=review
> 
> > Source/WebCore/dom/NodeRareData.h:79
> > +    PassRefPtr<T> addCacheWithAtomicName(Node* node, CollectionType collectionType)
> 
> Could this have been ContainerNode* across the board instead of Node*?

No. Some addCacheWithAtomicName callers are member functions of Node. It is true that all owner nodes are ContainerNode since they’re either Document, Element, or ShadowRoot but we probably need to move all those member functions from Node to ContainerNode in order to change the type. Otherwise, we’ll be doing a lot of const_casts, which isn’t particularly better than the current code IMO.