Bug 123794 – HTMLCollection should not be NodeList

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Antti Koivisto]

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.

[Antti Koivisto]

Created attachment 216016 [details]
patch

[Antti Koivisto]

and by HTMLContainer I mean HTMLCollection

[Andreas Kling]

Comment on attachment 216016 [details]
patch

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++)

Ditto.

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

LiveNodeList&

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

LiveNodeList&

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

HTMLCollection&
Also 'list'?

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

HTMLCollection&
Also 'list'?

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

nullptr

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

So

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

many

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

pointers

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

everywhere :|

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

nullptr

[EFL EWS Bot]

Comment on attachment 216016 [details]
patch

Attachment 216016 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/19638999

[Ryosuke Niwa]

Comment on attachment 216016 [details]
patch

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.

[Ryosuke Niwa]

Namely, I'm referring to https://bugs.webkit.org/show_bug.cgi?id=110611.

[Ryosuke Niwa]

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.

[Andreas Kling]

Comment on attachment 216016 [details]
patch

Sure go ahead.

[Csaba Osztrogonác]

http://trac.webkit.org/changeset/158663 broke the build everywhere ... :-/

Are you working on the fix or do you need any help?

[Antti Koivisto]

was fixed in https://trac.webkit.org/r158665

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

maybe http://trac.webkit.org/changeset/158667 ... let's see