[webkit-reviews] review denied: [Bug 34231] Nodes in XPath result snapshots should keep JS wrappers alive : [Attachment 87964] Re-Rebased proposed fix: Mark the node in the XPathResult nodeset for snapshot types

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 34231: Nodes in XPath result snapshots should keep JS wrappers alive
https://bugs.webkit.org/show_bug.cgi?id=34231

Attachment 87964: Re-Rebased proposed fix: Mark the node in the XPathResult
nodeset for snapshot types
https://bugs.webkit.org/attachment.cgi?id=87964&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=87964&action=review

R- because this breaks the build, and also I don't think that it's worth making
the fix for snapshot results only. But looks mostly good!

> LayoutTests/ChangeLog:12
> +	   Test that an event listener registered on an XPATH result snapshots'
node does
> +	   not crash. This case was already fixed prior to this change. However
it is better
> +	   to keep it as a regression test.

Geoff's test crashes with an assertion in debug builds of ToT. I think that
this hasn't changed since it was attached.

> Source/WebCore/ChangeLog:15
> +	   * Android.jscbindings.mk:
> +	   Added the new file to our build systems.

ChangeLog doesn't mention most changed project files.

> Source/WebCore/WebCore.vcproj/WebCore.vcproj:63100
> +						ExcludedFromBuild="true"

Since this file is excluded from build, it should be added to
JSBindingsAllInOne.cpp. This is why the build failed.

> Source/WebCore/bindings/js/JSXPathResultCustom.cpp:2
> + * Copyright (C) 2010 Julien Chaffraix <jchaffraix at webkit.org>

2011

> Source/WebCore/bindings/js/JSXPathResultCustom.cpp:3
> + * All right reserved.

Why did you split the copyright line in two?

> Source/WebCore/bindings/js/JSXPathResultCustom.cpp:43
> +    // Only mark the snapshots' node.

I don't understand what this comment is saying.

> Source/WebCore/bindings/js/JSXPathResultCustom.cpp:44
> +    if (impl()->resultType() == XPathResult::UNORDERED_NODE_SNAPSHOT_TYPE ||
impl()->resultType() == XPathResult::ORDERED_NODE_SNAPSHOT_TYPE) {

What about other types based on NodeSet? There are two iterator types, and two
single-node types, and it seems that they are affected, too.

> Source/WebCore/bindings/js/JSXPathResultCustom.cpp:47
> +	   ExceptionCode ignoredException = 0;
> +	   unsigned long snapshotLength =
impl()->snapshotLength(ignoredException);
> +	   ASSERT_UNUSED(ignoredException, !ignoredException);

I would suggest adding necessary functions to XPathResult to avoid the runtime
cost of doing the checks repeatedly, and also to avoid the ugly code that
ignores exceptions.

> Source/WebCore/bindings/js/JSXPathResultCustom.cpp:54
> +    // FIXME: We need to mark the result of ANY_UNORDERED_NODE_TYPE and
FIRST_ORDERED_NODE_TYPE.

Ah yes. Why not do that right now?