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

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Pam Greene <pam at chromium.org>'s request
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 Sam Weinig <sam at webkit.org>
> +
> +// These nodes throw an exception.
> +var nodes_exception = [

Many of the objects tested here are not Nodes, they are just objects exposed by
the DOM. 

> +    'Attr', 'CharacterData', 'CDATASection', 'Comment', 'Document',
> +    'DocumentFragment', 'DocumentType', 'Element', 'Entity',
> +    'EntityReference', 'HTMLDocument', 'Node', 'Notation',
> +    'ProcessingInstruction', 'Text', 'HTMLAnchorElement',
> +    'HTMLAppletElement', 'HTMLAreaElement', 'HTMLBaseElement',
> +    'HTMLBaseFontElement', 'HTMLBlockquoteElement', 'HTMLBodyElement',
> +    'HTMLBRElement', 'HTMLButtonElement', 'HTMLCanvasElement',
> +    'HTMLDirectoryElement', 'HTMLDivElement', 'HTMLDListElement',
> +    'HTMLEmbedElement', 'HTMLFieldSetElement', 'HTMLFontElement',
> +    'HTMLFormElement', 'HTMLFrameElement', 'HTMLFrameSetElement',
> +    'HTMLHeadingElement', 'HTMLHeadElement', 'HTMLHRElement',
> +    'HTMLHtmlElement', 'HTMLIFrameElement', 'HTMLImageElement',
> +    'HTMLInputElement', 'HTMLIsIndexElement', 'HTMLLabelElement',
> +    'HTMLLegendElement', 'HTMLLIElement', 'HTMLLinkElement',
> +    'HTMLMapElement', 'HTMLMarqueeElement', 'HTMLMenuElement',
> +    'HTMLMetaElement', 'HTMLModElement', 'HTMLObjectElement',
> +    'HTMLOListElement', 'HTMLOptGroupElement', 'HTMLOptionElement',
> +    'HTMLParagraphElement', 'HTMLParamElement', 'HTMLPreElement',
> +    'HTMLQuoteElement', 'HTMLScriptElement', 'HTMLSelectElement',
> +    'HTMLStyleElement', 'HTMLTableCaptionElement',
> +    'HTMLTableColElement', 'HTMLTableElement',
> +    'HTMLTableSectionElement', 'HTMLTableCellElement',
> +    'HTMLTableRowElement', 'HTMLTextAreaElement', 'HTMLTitleElement',
> +    'HTMLUListElement', 'HTMLElement', 'CanvasRenderingContext2D',
> +    'Clipboard', 'Counter', 'CSSCharsetRule', 'CSSFontFaceRule',
> +    'CSSImportRule', 'CSSMediaRule', 'CSSPageRule',
> +    'CSSPrimitiveValue', 'CSSRule', 'CSSRuleList',
> +    'CSSStyleDeclaration', 'CSSStyleRule', 'CSSStyleSheet',
> +    'CSSValue', 'CSSValueList', 'DOMImplementation', 'Event',
> +    'HTMLCollection', 'KeyboardEvent', 'MediaList', 'MimeType',
> +    'MimeTypeArray', 'MouseEvent', 'MutationEvent', 'NamedNodeMap',
> +    'NodeFilter', 'NodeList', 'OverflowEvent', 'Plugin',
> +    'PluginArray', 'Range', 'Rect', 'StyleSheet', 'StyleSheetList',
> +    'TextEvent', 'UIEvent', 'WheelEvent', 'XPathResult', 'Worker'

Worker should not throw an exception, it should be in the list of types with
working constructors, even if it is not currently.  How did you make this list?
 Is it derived from the list in DOMWindow.idl?

> +];
> +
> +// These nodes have a working constructor.
> +var nodes_constructor = [
> +    'DOMParser', 'XMLHttpRequest', 'XMLSerializer', 'XPathEvaluator',
> +    'XSLTProcessor'
> +];
> +
> +// These nodes have no constructor.
> +var nodes_no_constructor = [
> +    'EventTargetNode', 'BarInfo', 'CanvasGradient', 'CanvasPattern',
> +    'Console', 'DOMSelection', 'DOMWindow', 'History',
> +    'UndetectableHTMLCollection', 'HTMLOptionsCollection',
> +    'InspectorController', 'Location', 'Navigator', 'NodeIterator',
> +    'RGBColor', 'Screen', 'TreeWalker', 'XPathExpression',
> +    'XPathNSResolver', 'EventTarget', 'EventListener', 'NPObject',
> +    'HTMLAllCollection', 'MessageChannel'
> +];

BarInfo, CanvasGradient, CanvasPattern, Console, DOMSelection, DOMWindowm,
History, HTMLOptionsCollection, Location, Navigator, NodeIterator, RGBColor,
Screen, TreeWalker and XPathExpression should all have non-constructible
constructors, and therefore should go in the first list.

MessageChannel should have a working constructor, so should go in the second
list.

> +
> +// These nodes have a working constructor, but their object names differ.
> +// This is therefore a map from node to constructor.
> +var nodes_different_constructor = {
> +    'Audio': 'HTMLAudioElement',
> +    'Option': 'HTMLOptionElement',
> +    'Image': 'HTMLImageElement'
> +}
> +
> +function TryAllocate(node) {
> +    var Cons = this[node];
> +    if (!Cons) return 'no constructor';
> +    try { return new Cons(); }
> +    catch (e) { return 'exception'; }
> +}
> +
> +function check(name, expected) {
> +    actual = TryAllocate(node);
> +    if (actual == expected) {
> +	 document.write("PASS: " + name + " '" + expected + "'<br>");
> +    } else {
> +	 document.write("FAIL: " + name + " wanted '" + expected + "', got '" +

> +	     actual + "'<br>");
> +    }
> +}
> +
> +
> +for (var i = 0; i < nodes_exception.length; i++) {
> +    var node = nodes_exception[i];
> +    check(node, 'exception');
> +}
> +
> +for (var i = 0; i < nodes_no_constructor.length; i++) {
> +    var node = nodes_no_constructor[i];
> +    check(node, 'no constructor');
> +}
> +
> +for (var i = 0; i < nodes_constructor.length; i++) {
> +    var node = nodes_constructor[i];
> +    check(node, '[object ' + node + ']');
> +}
> +
> +for (var node in nodes_different_constructor) {
> +    check(node, '[object ' + nodes_different_constructor[node] + ']');
> +}

It would be nice if this test used the shouldBe() style tests. 

r-.