Implement script-side of callback reactions for Custom Elements V1

Implement script-side of callback reactions for Custom Elements V1

This patch implements script-side of callback reactions. Abstract
classes were implemented in a separate CL[1].

Also contains following refactoring:
1. Callbacks are stored as v8::Function to make calling them easier.
2. m_observedAttribute is moved to the super class
   (CustomElementDefinition) to allow iteratiion.
3. The order to Get("observedAttributes") was changed as per the spec
   change[2].

The wpt test failure is a test issue, PR[3] is sent.

[1] https://codereview.chromium.org/2058823002
[2] https://github.com/whatwg/html/issues/1373
[3] https://github.com/w3c/web-platform-tests/pull/3175

BUG=594918
Committed: https://crrev.com/4f581ae17af347201ede92b01dc47068ee0cf2d5
Cr-Commit-Position: refs/heads/master@{#399859}

[kojii, 2016-06-14 04:34:07]

Description was changed from

==========
WIP: script-callback

BUG=
==========

to

==========
Implement script-side of callback reactions for Custom Elements V1

This patch implements script-side of callback reactions. Abstract
classes were implemented in a separate CL[1].

Also contains following refactoring:
1. Callbacks are stored as v8::Function to make calling them easier.
2. m_observedAttribute is moved to the super class
   (CustomElementDefinition) to allow iteratiion.
3. The order to Get("observedAttributes") was changed as per the spec
   change[2].

[1] https://codereview.chromium.org/2058823002
[2] https://github.com/whatwg/html/issues/1373

BUG=594918
==========

[kojii, 2016-06-14 05:34:33]

Description was changed from

==========
Implement script-side of callback reactions for Custom Elements V1

This patch implements script-side of callback reactions. Abstract
classes were implemented in a separate CL[1].

Also contains following refactoring:
1. Callbacks are stored as v8::Function to make calling them easier.
2. m_observedAttribute is moved to the super class
   (CustomElementDefinition) to allow iteratiion.
3. The order to Get("observedAttributes") was changed as per the spec
   change[2].

[1] https://codereview.chromium.org/2058823002
[2] https://github.com/whatwg/html/issues/1373

BUG=594918
==========

to

==========
Implement script-side of callback reactions for Custom Elements V1

This patch implements script-side of callback reactions. Abstract
classes were implemented in a separate CL[1].

Also contains following refactoring:
1. Callbacks are stored as v8::Function to make calling them easier.
2. m_observedAttribute is moved to the super class
   (CustomElementDefinition) to allow iteratiion.
3. The order to Get("observedAttributes") was changed as per the spec
   change[2].

The test failure is a test issue, PR[3] is sent.

[1] https://codereview.chromium.org/2058823002
[2] https://github.com/whatwg/html/issues/1373
[3] https://github.com/w3c/web-platform-tests/pull/3175

BUG=594918
==========

[kojii, 2016-06-14 05:35:54]

Description was changed from

==========
Implement script-side of callback reactions for Custom Elements V1

This patch implements script-side of callback reactions. Abstract
classes were implemented in a separate CL[1].

Also contains following refactoring:
1. Callbacks are stored as v8::Function to make calling them easier.
2. m_observedAttribute is moved to the super class
   (CustomElementDefinition) to allow iteratiion.
3. The order to Get("observedAttributes") was changed as per the spec
   change[2].

The test failure is a test issue, PR[3] is sent.

[1] https://codereview.chromium.org/2058823002
[2] https://github.com/whatwg/html/issues/1373
[3] https://github.com/w3c/web-platform-tests/pull/3175

BUG=594918
==========

to

==========
Implement script-side of callback reactions for Custom Elements V1

This patch implements script-side of callback reactions. Abstract
classes were implemented in a separate CL[1].

Also contains following refactoring:
1. Callbacks are stored as v8::Function to make calling them easier.
2. m_observedAttribute is moved to the super class
   (CustomElementDefinition) to allow iteratiion.
3. The order to Get("observedAttributes") was changed as per the spec
   change[2].

The wpt test failure is a test issue, PR[3] is sent.

[1] https://codereview.chromium.org/2058823002
[2] https://github.com/whatwg/html/issues/1373
[3] https://github.com/w3c/web-platform-tests/pull/3175

BUG=594918
==========

[kojii, 2016-06-14 05:37:44]

kojii@chromium.org changed reviewers:
+ dominicc@chromium.org, haraken@chromium.org

[kojii, 2016-06-14 05:37:45]

PTAL.

[dominicc (has gone to gerrit), 2016-06-14 09:19:45]

Some comments inline.

https://codereview.chromium.org/2060753002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
(right):

https://codereview.chromium.org/2060753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:154:
m_observedAttributes = observedAttributes;
We should make the CustomElementDefinition take observed attributes in its
constructor; no need to make the state mutable by the derived class.

https://codereview.chromium.org/2060753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:259:
static v8::Local<v8::Value> v8StringOrNull(v8::Isolate* isolate,
This is a nice little helper; maybe this should live in V8Binding.h, I guess?

https://codereview.chromium.org/2060753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:271:
v8::Isolate* isolate = m_scriptState->isolate();
This needs to set up the ScriptState::Scope just like runCallback does.

https://codereview.chromium.org/2060753002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.h
(left):

https://codereview.chromium.org/2060753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.h:13:
#include "wtf/HashSet.h"
Thanks for doing all this clean-up.

[kojii, 2016-06-14 11:18:54]

All done, thank you for your prompt review.

haraken@, PTAL for bindings; thank you for your time helping us in advance.

[haraken, 2016-06-14 11:39:24]

Mostly looks good.

https://codereview.chromium.org/2060753002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
(right):

https://codereview.chromium.org/2060753002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:239:
ScriptController::callFunction(

Nit: Use V8ScriptRunner::callFunction. ScriptController's one is just an alias
for something (which we should remove)...

https://codereview.chromium.org/2060753002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.h
(right):

https://codereview.chromium.org/2060753002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.h:69:
void runCallback(ScopedPersistent<v8::Function>&, Element*,

Nit: Change the parameter type to v8::Local<v8::Function>. ScopedPersistent is a
type used to hold some handle. We prefer using v8::Local handles on stack.

https://codereview.chromium.org/2060753002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/2060753002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8Binding.h:410: inline
v8::Local<v8::Value> v8StringOrNull(v8::Isolate* isolate, const AtomicString&
string)

Do you really need this behavior? "string or null" is not a speced behavior, so
we've tried hard to deprecate this method. And now you're introducing it again
:)

[kojii, 2016-06-14 13:59:43]

Comment on V8StringOrNull below, others are done in PS5.

https://codereview.chromium.org/2060753002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/2060753002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8Binding.h:410: inline
v8::Local<v8::Value> v8StringOrNull(v8::Isolate* isolate, const AtomicString&
string)
On 2016/06/14 at 11:39:24, haraken wrote:
> Do you really need this behavior? "string or null" is not a speced behavior,
so we've tried hard to deprecate this method. And now you're introducing it
again :)

The attributeChangedCallback spec says "null" for append/remove case:
https://dom.spec.whatwg.org/#concept-element-attributes-change-ext
and upgrade case:
https://html.spec.whatwg.org/multipage/scripting.html#concept-upgrade-an-element

The append/remove spec above also defines the use of "null" for
MutationObserver. I couldn't figure out how we convert this to v8::Null, but we
seem to work as spec'ed:
http://jsbin.com/tajesihuga

Is this so special and different from other parts of the spec?

[haraken, 2016-06-14 15:22:51]

haraken@chromium.org changed reviewers:
+ foolip@chromium.org, jl@opera.com

[haraken, 2016-06-14 15:22:51]

On 2016/06/14 13:59:43, kojii wrote:
> Comment on V8StringOrNull below, others are done in PS5.
> 
>
https://codereview.chromium.org/2060753002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right):
> 
>
https://codereview.chromium.org/2060753002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/V8Binding.h:410: inline
> v8::Local<v8::Value> v8StringOrNull(v8::Isolate* isolate, const AtomicString&
> string)
> On 2016/06/14 at 11:39:24, haraken wrote:
> > Do you really need this behavior? "string or null" is not a speced behavior,
> so we've tried hard to deprecate this method. And now you're introducing it
> again :)
> 
> The attributeChangedCallback spec says "null" for append/remove case:
> https://dom.spec.whatwg.org/#concept-element-attributes-change-ext
> and upgrade case:
>
https://html.spec.whatwg.org/multipage/scripting.html#concept-upgrade-an-element
> 
> The append/remove spec above also defines the use of "null" for
> MutationObserver. I couldn't figure out how we convert this to v8::Null, but
we
> seem to work as spec'ed:
> http://jsbin.com/tajesihuga

Good point. MutationObsever is doing the conversion here:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...

> 
> Is this so special and different from other parts of the spec?

Yeah, previously we had [TreatReturnedNullStringAsNull] IDL extended attribute
to indicate that a returned null string should be converted to null instead of
an empty string. foolip@ and jl@ tried hard to remove the IDL extended
attribute.

+foolip +jl

[kojii, 2016-06-14 15:47:41]

On 2016/06/14 at 15:22:51, haraken wrote:
> On 2016/06/14 13:59:43, kojii wrote:
> > Comment on V8StringOrNull below, others are done in PS5.
> > 
> >
https://codereview.chromium.org/2060753002/diff/60001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right):
> > 
> >
https://codereview.chromium.org/2060753002/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/bindings/core/v8/V8Binding.h:410: inline
> > v8::Local<v8::Value> v8StringOrNull(v8::Isolate* isolate, const
AtomicString&
> > string)
> > On 2016/06/14 at 11:39:24, haraken wrote:
> > > Do you really need this behavior? "string or null" is not a speced
behavior,
> > so we've tried hard to deprecate this method. And now you're introducing it
> > again :)
> > 
> > The attributeChangedCallback spec says "null" for append/remove case:
> > https://dom.spec.whatwg.org/#concept-element-attributes-change-ext
> > and upgrade case:
> >
https://html.spec.whatwg.org/multipage/scripting.html#concept-upgrade-an-element
> > 
> > The append/remove spec above also defines the use of "null" for
> > MutationObserver. I couldn't figure out how we convert this to v8::Null, but
we
> > seem to work as spec'ed:
> > http://jsbin.com/tajesihuga
> 
> Good point. MutationObsever is doing the conversion here:
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...
> 
> > 
> > Is this so special and different from other parts of the spec?
> 
> Yeah, previously we had [TreatReturnedNullStringAsNull] IDL extended attribute
to indicate that a returned null string should be converted to null instead of
an empty string. foolip@ and jl@ tried hard to remove the IDL extended
attribute.
> 
> +foolip +jl

Another case is Element.getAttribute(), where null indicates missing while empty
string is a bool attribute. When IDL is "DOMString?", this is string-or-null,
no?

Test here: http://output.jsbin.com/baqaci

[haraken, 2016-06-14 23:51:55]

On 2016/06/14 15:47:41, kojii wrote:
> On 2016/06/14 at 15:22:51, haraken wrote:
> > On 2016/06/14 13:59:43, kojii wrote:
> > > Comment on V8StringOrNull below, others are done in PS5.
> > > 
> > >
>
https://codereview.chromium.org/2060753002/diff/60001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right):
> > > 
> > >
>
https://codereview.chromium.org/2060753002/diff/60001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/bindings/core/v8/V8Binding.h:410: inline
> > > v8::Local<v8::Value> v8StringOrNull(v8::Isolate* isolate, const
> AtomicString&
> > > string)
> > > On 2016/06/14 at 11:39:24, haraken wrote:
> > > > Do you really need this behavior? "string or null" is not a speced
> behavior,
> > > so we've tried hard to deprecate this method. And now you're introducing
it
> > > again :)
> > > 
> > > The attributeChangedCallback spec says "null" for append/remove case:
> > > https://dom.spec.whatwg.org/#concept-element-attributes-change-ext
> > > and upgrade case:
> > >
>
https://html.spec.whatwg.org/multipage/scripting.html#concept-upgrade-an-element
> > > 
> > > The append/remove spec above also defines the use of "null" for
> > > MutationObserver. I couldn't figure out how we convert this to v8::Null,
but
> we
> > > seem to work as spec'ed:
> > > http://jsbin.com/tajesihuga
> > 
> > Good point. MutationObsever is doing the conversion here:
> > 
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...
> > 
> > > 
> > > Is this so special and different from other parts of the spec?
> > 
> > Yeah, previously we had [TreatReturnedNullStringAsNull] IDL extended
attribute
> to indicate that a returned null string should be converted to null instead of
> an empty string. foolip@ and jl@ tried hard to remove the IDL extended
> attribute.
> > 
> > +foolip +jl
> 
> Another case is Element.getAttribute(), where null indicates missing while
empty
> string is a bool attribute. When IDL is "DOMString?", this is string-or-null,
> no?

ah, thanks for pointing it out! You're right. I remembered what we've done is to
replace [TreatReturnedNullStringAsNull] with DOMString?, let the IDL compiler
handle the conversion, and thus removed v8StringOrNull.

So the behavior here makes sense. LGTM. (Sorry about the noise!)

[kojii, 2016-06-15 04:20:30]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2016-06-15 04:20:50]

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

[commit-bot: I haz the power, 2016-06-15 04:33:51]

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

[commit-bot: I haz the power, 2016-06-15 04:33:53]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[kojii, 2016-06-15 04:55:51]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2016-06-15 04:56:09]

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

[commit-bot: I haz the power, 2016-06-15 05:03:51]

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

[commit-bot: I haz the power, 2016-06-15 05:03:52]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[kojii, 2016-06-15 05:29:47]

The CQ bit was checked by kojii@chromium.org

[kojii, 2016-06-15 05:29:47]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2060753002/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2016-06-15 05:29:57]

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

[commit-bot: I haz the power, 2016-06-15 06:33:14]

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

[commit-bot: I haz the power, 2016-06-15 06:33:15]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[kojii, 2016-06-15 06:39:13]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2016-06-15 06:39:22]

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

[commit-bot: I haz the power, 2016-06-15 08:32:19]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-15 08:35:29]

Description was changed from

==========
Implement script-side of callback reactions for Custom Elements V1

This patch implements script-side of callback reactions. Abstract
classes were implemented in a separate CL[1].

Also contains following refactoring:
1. Callbacks are stored as v8::Function to make calling them easier.
2. m_observedAttribute is moved to the super class
   (CustomElementDefinition) to allow iteratiion.
3. The order to Get("observedAttributes") was changed as per the spec
   change[2].

The wpt test failure is a test issue, PR[3] is sent.

[1] https://codereview.chromium.org/2058823002
[2] https://github.com/whatwg/html/issues/1373
[3] https://github.com/w3c/web-platform-tests/pull/3175

BUG=594918
==========

to

==========
Implement script-side of callback reactions for Custom Elements V1

This patch implements script-side of callback reactions. Abstract
classes were implemented in a separate CL[1].

Also contains following refactoring:
1. Callbacks are stored as v8::Function to make calling them easier.
2. m_observedAttribute is moved to the super class
   (CustomElementDefinition) to allow iteratiion.
3. The order to Get("observedAttributes") was changed as per the spec
   change[2].

The wpt test failure is a test issue, PR[3] is sent.

[1] https://codereview.chromium.org/2058823002
[2] https://github.com/whatwg/html/issues/1373
[3] https://github.com/w3c/web-platform-tests/pull/3175

BUG=594918

Committed: https://crrev.com/0dee3ec4784b2fb73d3aefaab1e00e92767ac767
Cr-Commit-Position: refs/heads/master@{#399858}
==========

[commit-bot: I haz the power, 2016-06-15 08:35:30]

Patchset 6 (id:??) landed as
https://crrev.com/0dee3ec4784b2fb73d3aefaab1e00e92767ac767
Cr-Commit-Position: refs/heads/master@{#399858}

[commit-bot: I haz the power, 2016-06-15 08:38:14]

Description was changed from

==========
Implement script-side of callback reactions for Custom Elements V1

This patch implements script-side of callback reactions. Abstract
classes were implemented in a separate CL[1].

Also contains following refactoring:
1. Callbacks are stored as v8::Function to make calling them easier.
2. m_observedAttribute is moved to the super class
   (CustomElementDefinition) to allow iteratiion.
3. The order to Get("observedAttributes") was changed as per the spec
   change[2].

The wpt test failure is a test issue, PR[3] is sent.

[1] https://codereview.chromium.org/2058823002
[2] https://github.com/whatwg/html/issues/1373
[3] https://github.com/w3c/web-platform-tests/pull/3175

BUG=594918

Committed: https://crrev.com/0dee3ec4784b2fb73d3aefaab1e00e92767ac767
Cr-Commit-Position: refs/heads/master@{#399858}
==========

to

==========
Implement script-side of callback reactions for Custom Elements V1

This patch implements script-side of callback reactions. Abstract
classes were implemented in a separate CL[1].

Also contains following refactoring:
1. Callbacks are stored as v8::Function to make calling them easier.
2. m_observedAttribute is moved to the super class
   (CustomElementDefinition) to allow iteratiion.
3. The order to Get("observedAttributes") was changed as per the spec
   change[2].

The wpt test failure is a test issue, PR[3] is sent.

[1] https://codereview.chromium.org/2058823002
[2] https://github.com/whatwg/html/issues/1373
[3] https://github.com/w3c/web-platform-tests/pull/3175

BUG=594918

Committed: https://crrev.com/4f581ae17af347201ede92b01dc47068ee0cf2d5
Cr-Commit-Position: refs/heads/master@{#399859}
==========

[commit-bot: I haz the power, 2016-06-15 08:38:16]

Patchset 6 (id:??) landed as
https://crrev.com/4f581ae17af347201ede92b01dc47068ee0cf2d5
Cr-Commit-Position: refs/heads/master@{#399859}

[picksi1, 2016-06-15 09:21:12]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/2066813003/ by picksi@chromium.org.

The reason for reverting is: Seems to have broken the Linux builder:
https://build.chromium.org/p/chromium.perf/builders/Linux%20Builder/builds/44040
.

[picksi1, 2016-06-15 09:29:25]

The revert failed (missing an LGTM)... but that may be fortuitous as there is
another CL in flight (https://codereview.chromium.org/2066793003/) that may fix
it.

I will revert this patch again if the new patch fails to fix the build
(https://build.chromium.org/p/chromium.perf/builders/Linux%20Builder/builds/44047)

[picksi1, 2016-06-15 09:51:49]

The new patch fixed it :)

[kojii, 2016-06-15 09:54:28]

Thank you picksi@!

[Michael Achenbach, 2016-06-15 12:54:03]

machenbach@chromium.org changed reviewers:
+ machenbach@chromium.org

[Michael Achenbach, 2016-06-15 12:54:03]

API stability nag (no action required):

This breaks the api stability checker:
https://build.chromium.org/p/chromium.fyi/builders/Linux%20V8%20API%20Stabili...

Please leave enough time between v8-side changes and following chromium changes
(min. 2-3 days) so that v8 is always revertible cleanly to the last canary
version.

[kojii, 2016-06-15 14:10:30]

On 2016/06/15 at 12:54:03, machenbach wrote:
> API stability nag (no action required):
> 
> This breaks the api stability checker:
>
https://build.chromium.org/p/chromium.fyi/builders/Linux%20V8%20API%20Stabili...
> 
> Please leave enough time between v8-side changes and following chromium
changes (min. 2-3 days) so that v8 is always revertible cleanly to the last
canary version.

machenbach@, please see comment 35. Somehow this CL landed twice, and that broke
out build too. Take https://codereview.chromium.org/2066793003/ and your build
should be fixed too.