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..059855c3f9205c55fe1f8faea6022e7b9aa35471 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,7 @@ 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.
+

[dominicc (has gone to gerrit), 2016/06/23 04:13:10]

Delete this blank line; put just one blank line between tests.

 test_with_window((w) => {
   class Y extends w.HTMLElement {}
   let X = (function () {}).bind({});
@@ -79,13 +77,9 @@ 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_true(w.customElements.get('a-a') === X, 'asserting that the first definition worked');

[dominicc (has gone to gerrit), 2016/06/23 04:13:10]

Don't put "asserting" in the message because that's evident from
assert_true/assert_false that this is an assertion. Try to incorporate the word
"should".

Use assert_equals instead of === in cases like this because it will generate a
more useful message if the assertion fails (what the result actually was, rather
than just "false.") assert_equals does === IIRC; it is exactly what you want.

+  assert_false(w.customElements.get('a-a') === Y, 'asserting that the first definition worked');

[dominicc (has gone to gerrit), 2016/06/23 04:13:10]

You can probably omit this assertion; it is not very informative given the
assertion above.

 }, 'Duplicate name defined recursively');
 
 test_with_window((w) => {
@@ -97,9 +91,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,13 +102,9 @@ 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_true(w.customElements.get('first-name') === X, 'asserting that the first definition worked');
+  assert_false(w.customElements.get('second-name') === X, 'asserting that the first definition worked');
 }, 'Reused constructor recursively');
 
 test_with_window((w) => {
@@ -192,6 +179,7 @@ test_with_window((w) => {
                       'document element');
 }, 'Upgrade: sets prototype of existing elements');
 
+
 test_with_window((w) => {
   let doc = w.document;
   var shadow = doc.body.attachShadow({mode: 'open'});
@@ -209,4 +197,96 @@ test_with_window((w) => {
                       'the constructor should have been invoked once for the ' +
                       'elements in the shadow tree');
 }, 'Upgrade: shadow tree');
+
+test_with_window((w) => {
+  let invocations = [];

[dominicc (has gone to gerrit), 2016/06/23 04:13:10]

There's only one thing going into invocations, so maybe you can simplify the
test by eliminating it and using assert_throws. I believe assert_throws has a
mode where it does === so you can create and save the object you will throw and
then check exactly that was thrown--read the source/docs of assert_throws to be
sure.

+  class Y extends w.HTMLElement {}
+  let X = (function () {}).bind({});
+  Object.defineProperty(X, 'prototype', {
+    get() { throw 42; }
+  });
+  try {
+    w.customElements.define('a-a', X);
+  } catch(e) {
+    invocations.push(e);
+  }
+  assert_equals(invocations[0], 42, 'rethrows Get(constructor, "prototype") exception');
+  w.customElements.define('a-a', Y);
+  assert_true(w.customElements.get('a-a') === Y, 'algorithm terminates when the first set of steps threw an exception');

[dominicc (has gone to gerrit), 2016/06/23 04:13:10]

"the first set of steps" is a bit vague. This description doesn't really match
what's being tested here; it seems more high-level, that the same name can be
registered successfully after failure.

+}, 'If an exception is thrown, rethrow that exception and terminate the algorithm');
+
+test_with_window((w) => {

[dominicc (has gone to gerrit), 2016/06/23 04:13:10]

Tests from here vvv

+  function F() {}
+  F.prototype = new Object();
+  Object.defineProperty(F.prototype, 'connectedCallback', {
+    get() { return new Object(); }
+  });
+  assert_throws(TypeError.prototype, () => {
+    w.customElements.define('a-a', F);
+  }, 'defining an element with a constructor with a connectedCallback that is ' +
+     'not undefined and not callable should throw a TypeError'); 
+}, 'Retrieved connectedCallback is not undefined and not callable');
+
+test_with_window((w) => {
+  function F() {}
+  F.prototype = new Object();
+  Object.defineProperty(F.prototype, 'disconnectedCallback', {
+    get() { return new Object(); }
+  });
+  assert_throws(TypeError.prototype, () => {
+    w.customElements.define('a-a', F);
+  }, 'defining an element with a constructor with a disconnectedCallback that is ' +
+     'not undefined and not callable should throw a TypeError'); 
+}, 'Retrieved disconnectedCallback is not undefined and not callable');
+
+test_with_window((w) => {
+  function F() {}
+  F.prototype = new Object();
+  Object.defineProperty(F.prototype, 'attributeChangedCallback', {
+    get() {return new Object(); }
+  });
+  assert_throws(TypeError.prototype, () => {
+    w.customElements.define('a-a', F);
+  }, 'defining an element with a constructor with a attribiteChangedCallback that is ' +
+     'not undefined and not callable should throw a TypeError'); 
+}, 'Retrieved attrbuteChangedCallback is not undefined and not callable');

[dominicc (has gone to gerrit), 2016/06/23 04:13:10]

... to here ^^^

are perfect. Good work.

+
+// should be TypeError
+test_with_window((w) => {
+  let invalid_name = 'annotation-xml';
+  assert_throws('SYNTAX_ERR', () => {

[dominicc (has gone to gerrit), 2016/06/23 04:13:10]

The implementation has a bug you found--awesome. We have a few choices about
what to do in this case.

You've written the test so that it passes and matches the (incorrect)
implementation. If we upstreamed this test and other browsers wrote code to pass
the test, they'd implement the same bug, which is bad. It's like our bad test
RNA damaging their DNA.

One thing we could do is put a big TODO here saying there's a bug and someone
should catch it before we upstream.

However the Blink test system also has a way to handle this situation. This is
outside of testharness. So you just write the test the way it *should* be (check
for TypeError) and the test will fail.

Then you can generate this thing called an expectation file by running just this
test using run-webkit-tests and passing the --reset-results flag. It will put a
file in your source tree like define-element-expected.txt. Add and commit that
and upload it as part of this test.

The -expected.txt file lets us treat a failing test as an "expected" failure.
Then we'll work on making sure we have no -expected.txt files before we ship.

You can read more about this here:

https://www.chromium.org/developers/testing/webkit-layout-tests#TOC-Success-a...
https://www.chromium.org/developers/testing/webkit-layout-tests#TOC-Script-Tests

+    w.customElements.define(invalid_name, HTMLElement);
+    }, 'defining a constructor that is an interface object whose interface is HTMLElement' + 
+       'should throw TypeError not SyntaxError');

[dominicc (has gone to gerrit), 2016/06/23 04:13:09]

Let's put a comment mentioning the exact step.

The technique you're using here, making the first possible successive step throw
an error, is really good. But it is subtle to the reader. It's worth a brief
comment.

+}, 'Invalid constructor');
+
+// should be TypeError
+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');
+
+
+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');
+}, 'Invalid constructor');
+
+
+test_with_window((w) => {
+  let element = w.document.createElement('a-a');
+  w.document.body.appendChild(element);
+  w.customElements.define('a-a', Set);
+  assert_true(w.customElements.get('a-a') === Set, 'definition is added to CustomElementRegistry');
+  assert_equals(element.matches(':defined'), false, 'fails during upgrade');
+  assert_equals(element.matches(':not(:defined)'), true, 'fails during upgrade');
+}, 'Invalid constructor');
+
+
 </script>