[webkit-reviews] review denied: [Bug 33696] [dom] let's cache nodelists instead of dynamicnodelist::cache : [Attachment 46836] Added a layout test

[bugzilla-daemon at webkit.org]

Maciej Stachowiak <mjs at apple.com> has denied anton muhin
<antonm at chromium.org>'s request for review:
Bug 33696: [dom] let's cache nodelists instead of dynamicnodelist::cache
https://bugs.webkit.org/show_bug.cgi?id=33696

Attachment 46836: Added a layout test
https://bugs.webkit.org/attachment.cgi?id=46836&action=review

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
I like the performance boost from this patch, but I am pretty sure this patch
violates the DOM Level 3 specification. Here is the spec for
getElementsByTagName:
http://www.w3.org/TR/DOM-Level-3-Core/core.html#ID-A6C9094

Notice that it says the return value is "A new NodeList object containing all
the matched Elements". So it's required that every call needs to return a new
object.

getElementsByTagNameNS has the same requirement:
http://www.w3.org/TR/DOM-Level-3-Core/core.html#ID-getElBTNNS

getElementByName and getElementsByClassName are specified by HTML5, and it does
not explicitly say one way or the other whether every call to these methods
must return a new object:
http://dev.w3.org/html5/spec/Overview.html#dom-document-getelementsbyname
http://dev.w3.org/html5/spec/Overview.html#dom-document-getelementsbyclassname

But I am not sure if the difference from the DOM Level 3 Core methods is
intentional; it seems possible this was just an oversight and HTML5 is not yet
final.


I think the benefit of caching is greater than the compatibility risk, but I
would much prefer that we get the specs to explicitly allow caching. The DOM
Core spec is owned by the W3C Web Apps Working Group and HTML5 is being jointly
developed by the W#C HTML Working Group and the WHATWG. I suggest contacting
those organizations first.

Since at least part of this is explicitly disallowed by the most recent
relevant spec, r-. However, I would love to see this change go in if we can get
the standards aligned. I would not want to land the change if it violates
standards, though, because then we will face accusations of "cheating" by
violating standards to win a benchmark.