[webkit-reviews] review requested: [Bug 22365] Add a test to verify the results of DOM constructors : [Attachment 25317] Patch addressing Sam's comments

[bugzilla-daemon at webkit.org]

Pam Greene <pam at chromium.org> has asked  for review:
Bug 22365: Add a test to verify the results of DOM constructors
https://bugs.webkit.org/show_bug.cgi?id=22365

Attachment 25317: Patch addressing Sam's comments
https://bugs.webkit.org/attachment.cgi?id=25317&action=review

------- Additional Comments from Pam Greene <pam at chromium.org>
(In reply to comment #2)

> > +// These nodes have a working constructor.
> > +var nodes_constructor = [
> > +  'DOMParser',
> > +  'XMLHttpRequest', 'XMLSerializer',
> > +  'XPathEvaluator',
> > +  'XSLTProcessor'
> > +];
> 
> You are missing Audio, Option, Image, Worker, and MessageChannel here.
 
Audio, Option, and Image all have constructors, although with slightly
nonstandard object names; Worker throws an exception; and MessageChannel has no
constructor.  I've added them to the appropriate lists.

> > +  'UndetectableHTMLCollection',
> I don't think this is a real class in WebCore.  We do have class called
> HTMLAllCollection which is undetectable though.  Perhaps that is what you
were
> thinking of.

Looks like UndetectableHTMLCollection is a V8ism. It's not causing any problems
to test it in JSC too, but if you object I'll take it out. (I wouldn't propose
testing lots of V8isms in the layout tests, but as one of ~150 items in a list
it seems both reasonable and harmless.)

HTMLAllCollection is in the nodes_no_constructor list.

> Some of these should probably have constructors (of the non-constructible
> variety), but I guess it is good to test our current behavior.

Let me know which nodes are in the wrong lists, and I'll gladly move them.

> In general, we like to keep all code in the WebKit style guidelines, even
> tests, which mean 4 space indentation.

Done.