Bug 123794 - HTMLCollection should not be NodeList
Summary: HTMLCollection should not be NodeList
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2013-11-05 01:56 PST by Antti Koivisto
Modified: 2014-02-06 08:30 PST (History)
8 users (show)

See Also:

patch (81.41 KB, patch)
2013-11-05 02:12 PST, Antti Koivisto
kling: review+
eflews.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2013-11-05 01:56:08 PST
HTMLContainer and NodeList are unrelated types in DOM yet our HTMLContainer inherits NodeList for code sharing reasons. While some code does get shared the types are sufficiently different that this results in lots of unnecessary branches, complexity and general awkwardness. Code sharing can be better achieved by other means than inheritance.
Comment 1 Antti Koivisto 2013-11-05 02:12:02 PST
Created attachment 216016 [details]
Comment 2 Antti Koivisto 2013-11-05 02:16:11 PST
and by HTMLContainer I mean HTMLCollection
Comment 3 Andreas Kling 2013-11-05 02:19:02 PST
Comment on attachment 216016 [details]

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

> Source/WebCore/dom/Document.cpp:502
> +    for (unsigned i = 0; i < WTF_ARRAY_LENGTH(m_nodeListAndCollectionCounts); i++)

++i LOL

> Source/WebCore/dom/Document.cpp:592
> +    for (unsigned i = 0; i < WTF_ARRAY_LENGTH(m_nodeListAndCollectionCounts); i++)


> Source/WebCore/dom/Document.cpp:3361
> +void Document::registerNodeList(LiveNodeList* list)


> Source/WebCore/dom/Document.cpp:3368
> +void Document::unregisterNodeList(LiveNodeList* list)


> Source/WebCore/dom/Document.cpp:3377
> +void Document::registerCollection(HTMLCollection* list)

Also 'list'?

> Source/WebCore/dom/Document.cpp:3386
> +{

Also 'list'?

> Source/WebCore/dom/Document.h:704
> +    bool shouldInvalidateNodeListAndCollectionCaches(const QualifiedName* attrName = 0) const;


> Source/WebCore/dom/LiveNodeList.cpp:43
> +inline bool isMatchingElement(const NodeListType*, Element*);


> Source/WebCore/dom/LiveNodeList.cpp:45
> +template <> inline bool isMatchingElement(const LiveNodeList* nodeList, Element* element)


> Source/WebCore/dom/LiveNodeList.cpp:50
> +template <> inline bool isMatchingElement(const HTMLTagNodeList* nodeList, Element* element)


> Source/WebCore/dom/LiveNodeList.cpp:55
> +template <> inline bool isMatchingElement(const ClassNodeList* nodeList, Element* element)

everywhere :|

> Source/WebCore/dom/LiveNodeList.cpp:67
> +    return 0;

Comment 4 EFL EWS Bot 2013-11-05 02:25:44 PST
Comment on attachment 216016 [details]

Attachment 216016 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/19638999
Comment 5 Ryosuke Niwa 2013-11-05 02:28:36 PST
Comment on attachment 216016 [details]

We shouldn't do this at least until we move getElementsByTagName, etc… off of NodeList.  That was the whole point in creating this mess in the first place.  Sorry for leaving the mess but let me finish the transition first.
Comment 6 Ryosuke Niwa 2013-11-05 02:32:35 PST
Namely, I'm referring to https://bugs.webkit.org/show_bug.cgi?id=110611.
Comment 7 Ryosuke Niwa 2013-11-05 04:09:59 PST
Antti and I had some discussions on IRC.  Antti intends to land this patch and then re-share code for the caches and I'll implement the transition once he finished that work.
Comment 8 Andreas Kling 2013-11-05 04:41:51 PST
Comment on attachment 216016 [details]

Sure go ahead.
Comment 9 Csaba Osztrogonác 2013-11-05 07:00:53 PST
http://trac.webkit.org/changeset/158663 broke the build everywhere ... :-/

Are you working on the fix or do you need any help?
Comment 10 Antti Koivisto 2013-11-05 07:21:17 PST
was fixed in https://trac.webkit.org/r158665
Comment 11 Csaba Osztrogonác 2013-11-05 07:22:55 PST
(In reply to comment #10)
> was fixed in https://trac.webkit.org/r158665

No, it wasn't. Windows, EFL and GTK build is still broken.
Comment 12 Csaba Osztrogonác 2013-11-05 07:24:08 PST
maybe http://trac.webkit.org/changeset/158667 ... let's see