Make |extends| option in custom element behave correctly

Make |extends| option in custom element behave correctly

When null is given to |extends| while instantiating the
custom element, it should be the same case when |extends|
is not specified at all.

BUG=323882
TEST=fast/dom/custom/type-extension-is-null.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180811

[deepak.sa, 2014-08-14 14:42:59]

PTAL?
Thanks!

[esprehn, 2014-08-14 14:45:52]

Why do !(...) instead of just using != ?

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[deepak.sa, 2014-08-14 14:49:30]

On 2014/08/14 14:45:52, esprehn wrote:
> Why do !(...) instead of just using != ?

Initially, i was calling a function when i did this : !(...) .
It should be != . I'll change it right away. Thanks!

[dominicc (has gone to gerrit), 2014-08-15 07:00:19]

Can we update the upstream W3C tests for Custom Elements as well?

https://codereview.chromium.org/476713002/diff/20001/LayoutTests/fast/dom/cus...
File LayoutTests/fast/dom/custom/type-extension-is-null.html (right):

https://codereview.chromium.org/476713002/diff/20001/LayoutTests/fast/dom/cus...
LayoutTests/fast/dom/custom/type-extension-is-null.html:9: 'type extensions
should have an "is" attribute');
The assertion arguments seem to contradict the assertion message.

[deepak.sa, 2014-08-18 12:50:27]

Updated TestExpectations for w3c tests for custom elements.

https://codereview.chromium.org/476713002/diff/20001/LayoutTests/fast/dom/cus...
File LayoutTests/fast/dom/custom/type-extension-is-null.html (right):

https://codereview.chromium.org/476713002/diff/20001/LayoutTests/fast/dom/cus...
LayoutTests/fast/dom/custom/type-extension-is-null.html:9: 'type extensions
should have an "is" attribute');
On 2014/08/15 07:00:19, dominicc wrote:
> The assertion arguments seem to contradict the assertion message.

Assertion message updated.

[dominicc (has gone to gerrit), 2014-08-19 01:03:06]

More comments inline. Nearly there, it just takes some care to get the tests
right.

https://codereview.chromium.org/476713002/diff/40001/LayoutTests/fast/dom/cus...
File LayoutTests/fast/dom/custom/type-extension-is-null.html (right):

https://codereview.chromium.org/476713002/diff/40001/LayoutTests/fast/dom/cus...
LayoutTests/fast/dom/custom/type-extension-is-null.html:6: var A =
document.registerElement('x-foo', {extends: null, prototype: null});
Move this back inside the test since it is specific to it.

https://codereview.chromium.org/476713002/diff/40001/LayoutTests/fast/dom/cus...
LayoutTests/fast/dom/custom/type-extension-is-null.html:11: }, 'localName should
not be "null" and type extension should not have "is" attribute');
I think the test name and the assertion message should be reversed. The current
test name is much more specific than the assertion message.

https://codereview.chromium.org/476713002/diff/40001/LayoutTests/fast/dom/cus...
LayoutTests/fast/dom/custom/type-extension-is-null.html:14: assert_equals(new
A().localName, 'x-foo',
I think this should be two assertions in one test and not a separate test. The
nature of the test is the same.

[deepak.sa, 2014-08-19 12:34:25]

PTAL?
Thanks!

https://codereview.chromium.org/476713002/diff/40001/LayoutTests/fast/dom/cus...
File LayoutTests/fast/dom/custom/type-extension-is-null.html (right):

https://codereview.chromium.org/476713002/diff/40001/LayoutTests/fast/dom/cus...
LayoutTests/fast/dom/custom/type-extension-is-null.html:6: var A =
document.registerElement('x-foo', {extends: null, prototype: null});
On 2014/08/19 01:03:06, dominicc wrote:
> Move this back inside the test since it is specific to it.

Done.

https://codereview.chromium.org/476713002/diff/40001/LayoutTests/fast/dom/cus...
LayoutTests/fast/dom/custom/type-extension-is-null.html:11: }, 'localName should
not be "null" and type extension should not have "is" attribute');
On 2014/08/19 01:03:06, dominicc wrote:
> I think the test name and the assertion message should be reversed. The
current
> test name is much more specific than the assertion message.

Done.

https://codereview.chromium.org/476713002/diff/40001/LayoutTests/fast/dom/cus...
LayoutTests/fast/dom/custom/type-extension-is-null.html:14: assert_equals(new
A().localName, 'x-foo',
On 2014/08/19 01:03:06, dominicc wrote:
> I think this should be two assertions in one test and not a separate test. The
> nature of the test is the same.

Done.

[deepak.sa, 2014-08-21 06:37:57]

PING!

[dominicc (has gone to gerrit), 2014-08-21 07:15:22]

The CQ bit was checked by dominicc@chromium.org

[dominicc (has gone to gerrit), 2014-08-21 07:15:23]

lgtm

LGTM.

Nit: It might be better without the parentheses around the != "null" test.

[commit-bot: I haz the power, 2014-08-21 07:15:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/deepak.sa@samsung.com/476713002/60001

[deepak.sa, 2014-08-21 07:36:34]

On 2014/08/21 07:15:23, dominicc wrote:

> Nit: It might be better without the parentheses around the != "null" test.

Thanks for the review!!

I am sorry, but i am not following you properly. You are saying that the
parenthesis around the != "null" are not required. Right? But, what about this
"test", you are asking?

[dominicc (has gone to gerrit), 2014-08-21 08:09:54]

On 2014/08/21 07:36:34, deepak.sa wrote:
> On 2014/08/21 07:15:23, dominicc wrote:
> 
> > Nit: It might be better without the parentheses around the != "null" test.
> 
> Thanks for the review!!
> 
> I am sorry, but i am not following you properly. You are saying that the
> parenthesis around the != "null" are not required. Right? But, what about this
> "test", you are asking?

Oh, I meant in the expression; by "test" I mean't the thing the if statement is
doing. You can just write

&& extends != "null";

[dominicc (has gone to gerrit), 2014-08-21 08:10:27]

+haraken for bindings.

Still LGTM with nit.

[deepak.sa, 2014-08-21 08:50:05]

The CQ bit was checked by deepak.sa@samsung.com

[commit-bot: I haz the power, 2014-08-21 08:50:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/deepak.sa@samsung.com/476713002/80001

[commit-bot: I haz the power, 2014-08-21 19:26:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[commit-bot: I haz the power, 2014-08-21 21:11:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-21 21:11:59]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[deepak.sa, 2014-08-23 09:20:19]

Ping! bindings/  required!

[haraken, 2014-08-23 11:47:03]

bindings/ LGTM.

[deepak.sa, 2014-08-23 12:16:05]

The CQ bit was checked by deepak.sa@samsung.com

[commit-bot: I haz the power, 2014-08-23 12:19:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/deepak.sa@samsung.com/476713002/80001

[commit-bot: I haz the power, 2014-08-24 04:45:55]

Committed patchset #5 (80001) as 180811