Added test for step 2, 14 in define element.

third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html

Index: third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html
diff --git a/third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html b/third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html
index b3fed436a10a506f3eed44e807c7e5ae27e0f68d..3b8d1686ff01d362d30398920083f9a2a96efee8 100644
--- a/third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html
+++ b/third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html
@@ -64,9 +64,6 @@ test_with_window((w) => {
      'a NotSupportedError');
 }, 'Duplicate name');
 
-// TODO(dominicc): Update this (perhaps by removing this comment) when
-// https://github.com/whatwg/html/pull/1333 lands/issue
-// https://github.com/whatwg/html/issues/1329 is closed.
 test_with_window((w) => {
   class Y extends w.HTMLElement {}
   let X = (function () {}).bind({});
@@ -79,13 +76,8 @@ test_with_window((w) => {
       return new Object();
     }
   });
-  // TODO(dominicc): When callback retrieval is implemented, change this
-  // to pass a valid constructor and recursively call define when retrieving
-  // callbacks instead; then it is possible to assert the first definition
-  // worked:
-  //  let element = Reflect.construct(HTMLElement, [], X);
-  //  assert_equals(element.localName, 'a-a');
   w.customElements.define('a-a', X);
+  assert_equals(w.customElements.get('a-a'), X, 'the first definition should have worked');
 }, 'Duplicate name defined recursively');
 
 test_with_window((w) => {
@@ -97,9 +89,6 @@ test_with_window((w) => {
      'registry should throw a NotSupportedError');
 }, 'Reused constructor');
 
-// TODO(dominicc): Update this (perhaps by removing this comment) when
-// https://github.com/whatwg/html/pull/1333 lands/issue
-// https://github.com/whatwg/html/issues/1329 is closed.
 test_with_window((w) => {
   let X = (function () {}).bind({});
   Object.defineProperty(X, 'prototype', {
@@ -111,25 +100,11 @@ test_with_window((w) => {
       return new Object();
     }
   });
-  // TODO(dominicc): When callback retrieval is implemented, change this
-  // to pass a valid constructor and recursively call define when retrieving
-  // callbacks instead; then it is possible to assert the first definition
-  // worked:
-  //  let element = Reflect.construct(HTMLElement, [], X);
-  //  assert_equals(element.localName, 'a-a');
   w.customElements.define('first-name', X);
+  assert_equals(w.customElements.get('first-name'), X, 'asserting that the first definition worked');

[dominicc (has gone to gerrit), 2016/06/28 03:54:57]

Be consistent; consider using the same description as line 80.

 }, 'Reused constructor recursively');
 
 test_with_window((w) => {
-  function F() {}
-  F.prototype = 42;
-  assert_throws(TypeError.prototype, () => {
-    w.customElements.define('a-a', F);
-  }, 'defining an element with a constructor with a prototype that is not an ' +
-     'object should throw a TypeError');
-}, 'Retrieved prototype is a non-object');
-
-test_with_window((w) => {
   assert_throws(TypeError.prototype, () => {
     let not_a_constructor = () => {};
     let invalid_name = 'annotation-xml';
@@ -209,4 +184,143 @@ test_with_window((w) => {
                       'the constructor should have been invoked once for the ' +
                       'elements in the shadow tree');
 }, 'Upgrade: shadow tree');
+
+// last statement in 14

[dominicc (has gone to gerrit), 2016/06/28 03:54:57]

Could you add a step number, and maybe some text, here? The spec could change;
adding some more clues will make it easier to work out if it has changed.

+test_with_window((w) => {
+  class Y extends w.HTMLElement {}
+  let X = (function () {}).bind({});
+  Object.defineProperty(X, 'prototype', {
+    get() { throw { name: 42 }; }
+  });
+  assert_throws({ name: 42 }, () => {
+    w.customElements.define('a-a', X);
+  }, 'should rethrow constructor exception');
+  w.customElements.define('a-a', Y);
+  assert_equals(w.customElements.get('a-a'), Y, 'the same name can be registered after failure');
+}, 'If an exception is thrown, rethrow that exception and terminate the algorithm');
+
+// 14.1
+test_with_window((w) => {
+  let X = (function () {}).bind({});
+  Object.defineProperty(X, 'prototype', {
+    get() { throw { name: 'prototype throws' }; }
+  });
+  assert_throws({ name: 'prototype throws' }, () => {
+    w.customElements.define('a-a', X);
+  }, 'Get(constructor, prototype) should throw');
+}, 'Rethrow any exceptions thrown while getting prototype');
+
+// 14.2
+test_with_window((w) => {
+  function F() {}
+  F.prototype = 42;
+  assert_throws(TypeError.prototype, () => {
+    w.customElements.define('a-a', F);
+  }, 'defining an element with a constructor with a prototype that is not an ' +
+     'object should throw a TypeError');
+}, 'Retrieved prototype is a non-object');
+
+// 14.3, 14.5, 14.7
+test_with_window((w) => {
+  let callbacks = ['connectedCallback', 'disconnectedCallback', 'attributeChangedCallback'];
+  function F() {}
+  F.prototype = new Object();

[dominicc (has gone to gerrit), 2016/06/28 03:54:58]

I don't think you need this line; functions get a prototype property with a
fresh Object automatically. Below too.

+  for(var i=0; i < callbacks.length; i++ ){
+    Object.defineProperty(F.prototype, callbacks[i], {
+      get() { throw { name: callbacks[i] }; }
+    });
+    assert_throws({ name: callbacks[i] }, () => {
+      w.customElements.define('a-a', F);

[dominicc (has gone to gerrit), 2016/06/28 03:54:58]

Wow, this is clever. I guess callbacks are in the *reverse* order they're
retrieved. Could you put a brief comment on the callbacks = line explaining
that? If we add callbacks in future, the person editing this file will need to
be aware of that.

+    }, 'Get(prototype, callback) should throw');

[dominicc (has gone to gerrit), 2016/06/28 03:54:58]

If this fails, it will not be clear which one is buggy. Maybe include
callbacks[i] in the message?

+  }
+}, 'Rethrow any exceptions thrown while getting a callback');
+
+// 14.4, 14.6, 14.9.1
+test_with_window((w) => {
+  let callbacks = ['connectedCallback', 'disconnectedCallback', 'attributeChangedCallback'];

[dominicc (has gone to gerrit), 2016/06/28 03:54:57]

Consider moving this outside the test and reusing it.

+  function F() {}
+  F.prototype = new Object();
+  for(var i=0; i < callbacks.length; i++){
+    Object.defineProperty(F.prototype, callbacks[i], {
+      get() {return new Object(); }
+    });
+    assert_throws(TypeError.prototype, () => {
+      w.customElements.define('a-a', F);
+    }, 'defining an element with a constructor with a callback that is ' +
+       'not undefined and not callable should throw a TypeError'); 
+  }
+}, 'Retrieved callback is not undefined and not callable');
+
+// 14.9.2
+test_with_window((w) => {

[dominicc (has gone to gerrit), 2016/06/28 03:54:58]

As with my comment with the other file, maybe using class syntax is more
illustrative and succinct here?

+  let X = (function () {}).bind({});
+  X.prototype = new Object();
+  Object.defineProperty(X.prototype, 'attributeChangedCallback', {
+    get() { return function(){}; }
+  });
+  Object.defineProperty(X, 'observedAttributes', {
+    get() { throw { name: 'observedAttributes throws' }; }
+  });
+  assert_throws({ name: 'observedAttributes throws' }, () => {
+    w.customElements.define('a-a', X);
+  }, 'Get(constructor, observedAttributes) should throw');
+}, 'Rethrow any exceptions thrown while getting observedAttributes');
+
+// 14.9.2 test Get(constructor, observedAttributes) does not throw if
+// attributeChangedCallback is undefined.
+test_with_window((w) => {
+  let observedAttributes_invoked = false;
+  let X = (function () {}).bind({});
+  X.prototype = new Object();
+  Object.defineProperty(X, 'observedAttributes', {
+    get() { observedAttributes_invoked = true; }
+  });
+  assert_false( observedAttributes_invoked, 'Get(constructor, observedAttributes) should not be invoked');
+}, 'Get(constructor, observedAttributes) should not execute if ' +
+         'attributeChangedCallback is undefined');

[dominicc (has gone to gerrit), 2016/06/28 03:54:57]

Be consistent with indentation; compare this to line 293. I think the style you
have on line 293 where the 's line up is more readable, but at least be
consistent.

+
+// step 2
+// 2. If constructor is an interface object whose corresponding interface either is 
+//    HTMLElement or has HTMLElement in its set of inherited interfaces, throw 
+//    a TypeError and abort these steps.
+// 3. If name is not a valid custom element name, then throw a "SyntaxError" DOMException 
+//    and abort these steps.
+test_with_window((w) => {
+  let invalid_name = 'annotation-xml';
+  assert_throws('SYNTAX_ERR', () => {
+    w.customElements.define(invalid_name, HTMLElement);

[dominicc (has gone to gerrit), 2016/06/28 03:54:58]

Indenting JavaScript is surprisingly hard, with arrow functions, promise chains,
multiline string literals, etc. In general your style is good.

I think in this case, this needs to be indented a couple of spaces to set the
body of the arrow function off from the closing brace. Below too.

+    }, 'defining a constructor that is an interface object whose interface is HTMLElement' + 
+       'should throw TypeError not SyntaxError');
+}, 'Invalid constructor');
+
+// step 2
+test_with_window((w) => {
+  let invalid_name = 'annotation-xml';
+  assert_throws('SYNTAX_ERR', () => {
+    w.customElements.define(invalid_name, HTMLButtonElement);
+    }, 'defining a constructor that is an interface object who has HTMLElement in its set of inhertied interfaces' + 
+       'should throw TypeError not SyntaxError');
+}, 'Invalid constructor');
+
+// 2
+test_with_window((w) => {
+  let invalid_name = 'annotation-xml';
+  assert_throws('SYNTAX_ERR', () => {
+    w.customElements.define(invalid_name, class extends HTMLElement {});
+    }, 'defining author-defined custom element constructor' + 
+       'should pass this step without throwing TypeError');

[dominicc (has gone to gerrit), 2016/06/28 03:54:58]

Here's another opportunity to be consistent. You probably want to make a string
literal as long as possible before wrapping it, and then choose a good place to
wrap it (I try to put the space between words at the *end* of the earlier line.)

In this case there's a bit of a visual difference between 301 and 310; 301 is
much longer.

Go for making this as long is will fit within 80 columns, then wrapping. That
way everything will be a consistent length.

+}, 'Invalid constructor');
+
+// this test throws Invalid Constructor Error 
+test_with_window((w) => {
+  let element = w.document.createElement('a-a');
+  w.document.body.appendChild(element);
+  class Foo extends Set{
+    constructor(){
+      return Reflect.construct(HTMLElement, [], Foo);
+    }
+  }
+  w.customElements.define('a-a', Foo);
+}, 'Invalid constructor');
 </script>
+</body>

[dominicc (has gone to gerrit), 2016/06/28 03:54:58]

Thanks for adding this; makes sense to add it since we wrote the open tag.