[webkit-reviews] review granted: [Bug 88825] Radio node lists do not get updated when inserted back into a document : [Attachment 147404] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 88825: Radio node lists do not get updated when inserted back into a
document
https://bugs.webkit.org/show_bug.cgi?id=88825

Attachment 147404: Patch
https://bugs.webkit.org/attachment.cgi?id=147404&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=147404&action=review


> Source/WebCore/ChangeLog:13
> +	   Change the approach to the problem instead and detect when m_node is
dettached from the document.

Typo: dettached

> Source/WebCore/dom/Document.cpp:3867
> +    ASSERT(isDocumentNode());

We should not have this assertion here. Every Document function could assert
this!

> Source/WebCore/dom/Document.cpp:3873
> +    ASSERT(isDocumentNode());

Ditto.

> Source/WebCore/dom/DynamicNodeList.h:28
> +#include "Document.h"
> +#include "Node.h"

No need to include Node.h if you’re already including Document.h, since
Document is derived from Node and has to include it.

> Source/WebCore/dom/DynamicNodeList.h:40
> +    enum DynamicNodeListRootType {

I think this could just be called RootType since it’s a nested enum.

> Source/WebCore/dom/DynamicNodeList.h:42
> +	   NodeListIsRootedAtNode,
> +	   NodeListIsRootedAtDocument,

I don’t think the NodeListIs prefix is all that helpful. This enum is already
nested in the class, so it already has a lot of context.

> Source/WebCore/dom/DynamicNodeList.h:70
> +	   Caches(DynamicNodeListRootType rootType)

This should use the explicit keyword.

> Source/WebCore/dom/DynamicNodeList.h:85
> +	   unsigned lastItemOffset : 29; // Borrow 3-bits for bit fields

Why is it OK to have only 29 bits? What guarantees we don’t overflow?