Implement Custom Element's class side inheritance

Implement Custom Element's class side inheritance

Custom element should set __proto__ internal property of the generated
constructor. __proto__ should be set to base class object.
As class inheritance is supported in DOM elements, it should also be
supported in Custom elements.

BUG=271466, 443369
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193847

[deepak.s, 2015-04-09 06:01:28]

deepak.s@samsung.com changed reviewers:
+ dominicc@chromium.org

[deepak.s, 2015-04-09 06:01:29]

PTAL?
Thanks!

[dominicc (has gone to gerrit), 2015-04-09 06:37:57]

Some comments inline. Looks OK but from the test output I'm not 100% sure this
is correct.

https://codereview.chromium.org/1069953002/diff/1/LayoutTests/fast/dom/custom...
File LayoutTests/fast/dom/custom/inherited-prototype.html (right):

https://codereview.chromium.org/1069953002/diff/1/LayoutTests/fast/dom/custom...
LayoutTests/fast/dom/custom/inherited-prototype.html:11: 
Less vertical whitespace here.

https://codereview.chromium.org/1069953002/diff/1/LayoutTests/fast/dom/custom...
File
LayoutTests/fast/dom/custom/registration-context-delete-during-register-base-constructor-retrieval-expected.txt
(right):

https://codereview.chromium.org/1069953002/diff/1/LayoutTests/fast/dom/custom...
LayoutTests/fast/dom/custom/registration-context-delete-during-register-base-constructor-retrieval-expected.txt:6:
PASS caught expected exception "NotSupportedError: Failed to execute
'registerElement' on 'Document': Registration failed for type 'x-a'. Prototype
constructor property is not configurable."
It's a concern that "PASS destroyed context" isn't showing up in the test
output, and I'm not sure the error message that is showing up means this test is
running as we want it to. That second error message originates from
prototypeIsValid.

https://codereview.chromium.org/1069953002/diff/1/Source/bindings/core/v8/Cus...
File Source/bindings/core/v8/CustomElementConstructorBuilder.cpp (right):

https://codereview.chromium.org/1069953002/diff/1/Source/bindings/core/v8/Cus...
Source/bindings/core/v8/CustomElementConstructorBuilder.cpp:189:
v8::Local<v8::String> constructorKey = v8String(isolate, "constructor");
Move this down near to where it is used.

https://codereview.chromium.org/1069953002/diff/1/Source/bindings/core/v8/Cus...
Source/bindings/core/v8/CustomElementConstructorBuilder.cpp:204: if
(!v8CallBoolean(m_constructor->SetPrototype(context,
m_prototype->Get(constructorKey))))
FYI It looks like the bool SetPrototype is deprecated, but this is fine this way
because it's consistent with the surrounding stuff. We can update it later in a
clean-up pass.

https://code.google.com/p/chromium/codesearch#chromium/src/v8/include/v8.h&q=...

[deepak.s, 2015-04-09 09:50:00]

https://codereview.chromium.org/1069953002/diff/1/LayoutTests/fast/dom/custom...
File
LayoutTests/fast/dom/custom/registration-context-delete-during-register-base-constructor-retrieval-expected.txt
(right):

https://codereview.chromium.org/1069953002/diff/1/LayoutTests/fast/dom/custom...
LayoutTests/fast/dom/custom/registration-context-delete-during-register-base-constructor-retrieval-expected.txt:6:
PASS caught expected exception "NotSupportedError: Failed to execute
'registerElement' on 'Document': Registration failed for type 'x-a'. Prototype
constructor property is not configurable."
On 2015/04/09 06:37:57, dominicc wrote:
> It's a concern that "PASS destroyed context" isn't showing up in the test
> output,

This is happening because it is failing to configure Prototype constructor
property,
which is valid as per this test also:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

That is why "PASS destroyed context" is not showing up.

> and I'm not sure the error message that is showing up means this test is
> running as we want it to. That second error message originates from
> prototypeIsValid.

Yeah, test is not running as per our expectations.
Either we have to write 'configurable : true' in the constructor property,
we have to test it some other way.

[dominicc (has gone to gerrit), 2015-04-10 03:25:15]

Nit: The title of this issue has a typo, make sure the description is
correct.

On Thu, Apr 9, 2015 at 6:50 PM, <deepak.s@samsung.com> wrote:

>
> https://codereview.chromium.org/1069953002/diff/1/
> LayoutTests/fast/dom/custom/registration-context-delete-
> during-register-base-constructor-retrieval-expected.txt
> File
> LayoutTests/fast/dom/custom/registration-context-delete-
> during-register-base-constructor-retrieval-expected.txt
> (right):
>
> https://codereview.chromium.org/1069953002/diff/1/
> LayoutTests/fast/dom/custom/registration-context-delete-
> during-register-base-constructor-retrieval-expected.txt#newcode6
> LayoutTests/fast/dom/custom/registration-context-delete-
> during-register-base-constructor-retrieval-expected.txt:6:
> PASS caught expected exception "NotSupportedError: Failed to execute
> 'registerElement' on 'Document': Registration failed for type 'x-a'.
> Prototype constructor property is not configurable."
> On 2015/04/09 06:37:57, dominicc wrote:
>
>> It's a concern that "PASS destroyed context" isn't showing up in the
>>
> test
>
>> output,
>>
>
> This is happening because it is failing to configure Prototype
> constructor property,
> which is valid as per this test also:
> https://code.google.com/p/chromium/codesearch#chromium/
> src/third_party/WebKit/LayoutTests/fast/dom/custom/
> document-register-basic.html&q=constructor\:%20file:%28
>
<https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...>
> ^src/third_party/WebKit/LayoutTests/fast/dom/custom/%
> 29%20package:^chromium$&type=cs&l=34
>
> That is why "PASS destroyed context" is not showing up.
>
>  and I'm not sure the error message that is showing up means this test
>>
> is
>
>> running as we want it to. That second error message originates from
>> prototypeIsValid.
>>
>
> Yeah, test is not running as per our expectations.
> Either we have to write 'configurable : true' in the constructor
> property,
> we have to test it some other way.
>
> https://codereview.chromium.org/1069953002/
>

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

[deepak.s, 2015-04-10 04:05:04]

On 2015/04/10 03:25:15, dominicc wrote:
> Nit: The title of this issue has a typo, make sure the description is
> correct.
> 
> On Thu, Apr 9, 2015 at 6:50 PM, <mailto:deepak.s@samsung.com> wrote:
> 
> >
> > https://codereview.chromium.org/1069953002/diff/1/
> > LayoutTests/fast/dom/custom/registration-context-delete-
> > during-register-base-constructor-retrieval-expected.txt
> > File
> > LayoutTests/fast/dom/custom/registration-context-delete-
> > during-register-base-constructor-retrieval-expected.txt
> > (right):
> >
> > https://codereview.chromium.org/1069953002/diff/1/
> > LayoutTests/fast/dom/custom/registration-context-delete-
> > during-register-base-constructor-retrieval-expected.txt#newcode6
> > LayoutTests/fast/dom/custom/registration-context-delete-
> > during-register-base-constructor-retrieval-expected.txt:6:
> > PASS caught expected exception "NotSupportedError: Failed to execute
> > 'registerElement' on 'Document': Registration failed for type 'x-a'.
> > Prototype constructor property is not configurable."
> > On 2015/04/09 06:37:57, dominicc wrote:
> >
> >> It's a concern that "PASS destroyed context" isn't showing up in the
> >>
> > test
> >
> >> output,
> >>
> >
> > This is happening because it is failing to configure Prototype
> > constructor property,
> > which is valid as per this test also:
> > https://code.google.com/p/chromium/codesearch#chromium/
> > src/third_party/WebKit/LayoutTests/fast/dom/custom/
> > document-register-basic.html&q=constructor\:%20file:%28
> >
>
<https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...>
> > ^src/third_party/WebKit/LayoutTests/fast/dom/custom/%
> > 29%20package:^chromium$&type=cs&l=34
> >
> > That is why "PASS destroyed context" is not showing up.
> >
> >  and I'm not sure the error message that is showing up means this test
> >>
> > is
> >
> >> running as we want it to. That second error message originates from
> >> prototypeIsValid.
> >>
> >
> > Yeah, test is not running as per our expectations.
> > Either we have to write 'configurable : true' in the constructor
> > property,
> > we have to test it some other way.
> >
> > https://codereview.chromium.org/1069953002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

Thanks for the review!
I'll update the description.
What about the "context delete" test case? Should I upload new patch without it?

[dominicc (has gone to gerrit), 2015-04-13 13:17:43]

On 2015/04/10 at 04:05:04, deepak.s wrote:
> Thanks for the review!
> I'll update the description.
> What about the "context delete" test case? Should I upload new patch without
it?

I think we need that test. Are you saying context deletion is impossible in this
case?

[deepak.s, 2015-04-15 06:44:53]

PTAL?

https://codereview.chromium.org/1069953002/diff/1/LayoutTests/fast/dom/custom...
File LayoutTests/fast/dom/custom/inherited-prototype.html (right):

https://codereview.chromium.org/1069953002/diff/1/LayoutTests/fast/dom/custom...
LayoutTests/fast/dom/custom/inherited-prototype.html:11: 
On 2015/04/09 06:37:57, dominicc wrote:
> Less vertical whitespace here.

Done.

https://codereview.chromium.org/1069953002/diff/1/Source/bindings/core/v8/Cus...
File Source/bindings/core/v8/CustomElementConstructorBuilder.cpp (right):

https://codereview.chromium.org/1069953002/diff/1/Source/bindings/core/v8/Cus...
Source/bindings/core/v8/CustomElementConstructorBuilder.cpp:189:
v8::Local<v8::String> constructorKey = v8String(isolate, "constructor");
On 2015/04/09 06:37:57, dominicc wrote:
> Move this down near to where it is used.

Done.

[dominicc (has gone to gerrit), 2015-04-16 05:26:46]

The CQ bit was checked by dominicc@chromium.org

[dominicc (has gone to gerrit), 2015-04-16 05:26:47]

lgtm

[commit-bot: I haz the power, 2015-04-16 05:26:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1069953002/20001

[deepak.s, 2015-04-16 05:44:54]

deepak.s@samsung.com changed reviewers:
+ esprehn@chromium.org, haraken@chromium.org

[deepak.s, 2015-04-16 05:44:55]

Need Owner's review.
Thanks!

[commit-bot: I haz the power, 2015-04-16 05:44:59]

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

[commit-bot: I haz the power, 2015-04-16 05:45:00]

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

[esprehn, 2015-04-16 06:20:37]

lgtm

[deepak.s, 2015-04-16 06:36:37]

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

[commit-bot: I haz the power, 2015-04-16 06:36:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1069953002/20001

[commit-bot: I haz the power, 2015-04-16 06:40:34]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193847

[haraken, 2015-04-16 07:38:58]

LGTM

[dominicc (has gone to gerrit), 2015-04-21 03:56:19]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1055173006/ by dominicc@chromium.org.

The reason for reverting is: This caused Issue 478329..

[deepak.s, 2015-04-21 08:50:58]

On 2015/04/21 03:56:19, dominicc wrote:
> A revert of this CL (patchset #2 id:20001) has been created in
> https://codereview.chromium.org/1055173006/ by mailto:dominicc@chromium.org.
> 
> The reason for reverting is: This caused Issue 478329..

Reland of this patch : https://codereview.chromium.org/1097243002/