Add a tuple of name, local name for hashing custom element definitions.

Add a tuple of name, local name for hashing custom element definitions.

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

[dominicc (has gone to gerrit), 2016-05-20 01:16:31]

dominicc@chromium.org changed reviewers:
+ yosin@chromium.org

[dominicc (has gone to gerrit), 2016-05-20 01:16:39]

PTAL

[yosin_UTC9, 2016-05-20 01:49:19]

I don't want to have this tuple.

I would like to have complete "customer element definition" which has:
 - localName
 - prototype (C++ constructor)
 - constructor (for get)
 - connectedCallback
 - disconnectCallback
 - etc
There values are captured at CRE.define() and store in to JS Map<DefintionId,
CustomerElementDefintioin>.

CRE also have another map "when-defined-promise" map = Map<tagName,
Promise<void>>.

WDYT?

[kojii, 2016-05-20 04:41:37]

Looks reasonable to me.

On 2016/05/20 at 01:49:19, yosin wrote:
> I don't want to have this tuple.
> 
> I would like to have complete "customer element definition" which has:

We already have a class for it.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

and it should have fields as defined in the spec:
https://html.spec.whatwg.org/multipage/scripting.html#custom-element-definition

but I also see several parts of the spec use "name and local name", having a
separate class from CustomElementDefinition looks reasonable to me.

Maybe CustomElementDefinition should have this class as a member, or subclass
from this?

[dominicc (has gone to gerrit), 2016-05-20 05:15:24]

dominicc@chromium.org changed reviewers:
+ yukishiino@chromium.org

[dominicc (has gone to gerrit), 2016-05-20 05:15:45]

PTAL

+yukishiino for bindings OWNERs

[Yuki, 2016-05-20 05:57:00]

lgtm

https://codereview.chromium.org/1996213002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElementDescriptorHash.h
(right):

https://codereview.chromium.org/1996213002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementDescriptorHash.h:41: 
nit: no need an empty line?

[yosin_UTC9, 2016-05-20 06:17:49]

On 2016/05/20 at 05:15:45, dominicc wrote:
> PTAL
> 
> +yukishiino for bindings OWNERs

OK, finally I understand. |CustomElementDescriptor| is key for looking up
customer element definition.
It contains local name and "is" attribute, and namespace (in future?).
Can we rename |m_name| to |m_isAttrValue| or something?


The internal createElementNS steps, given document, namespace, qualifiedName,
and options, are as follows:

1. Let namespace, prefix, and localName be the result of passing namespace and
qualifiedName to validate and extract. Rethrow any exceptions.
2. Let is be the value of is member of options, or null if no such member
exists.
3. Let definition be the result of looking up a custom element definition, given
the context object, namespace, localName, and is.
4. If is is non-null and definition is null, then throw a NotFoundError.
5. Let element be the result of creating an element given document, localName,
namespace, prefix, is, and with the synchronous custom elements flag set.
Rethrow any exceptions.
6. If is is non-null, then set an attribute value for element using "is" and is.
7. Return element.

[dominicc (has gone to gerrit), 2016-05-20 06:36:40]

Thank you yukishiino.

yosin--comments inline to summarize our discussion.

On 2016/05/20 at 06:17:49, yosin wrote:
> On 2016/05/20 at 05:15:45, dominicc wrote:
> > PTAL
> > 
> > +yukishiino for bindings OWNERs
> 
> OK, finally I understand. |CustomElementDescriptor| is key for looking up
customer element definition.
> It contains local name and "is" attribute, and namespace (in future?).

The namespace for a match is always the HTML namespace; I was thinking we would
not carry that around. Instead we check the namespace is HTML when creating the
descriptor.

> Can we rename |m_name| to |m_isAttrValue| or something?

The name is not always the is attribute; only when m_name != m_localName.

> The internal createElementNS steps, given document, namespace, qualifiedName,
and options, are as follows:
> 
> 1. Let namespace, prefix, and localName be the result of passing namespace and
qualifiedName to validate and extract. Rethrow any exceptions.
> 2. Let is be the value of is member of options, or null if no such member
exists.
> 3. Let definition be the result of looking up a custom element definition,
given the context object, namespace, localName, and is.
> 4. If is is non-null and definition is null, then throw a NotFoundError.
> 5. Let element be the result of creating an element given document, localName,
namespace, prefix, is, and with the synchronous custom elements flag set.
Rethrow any exceptions.
> 6. If is is non-null, then set an attribute value for element using "is" and
is.
> 7. Return element.

Step 3 passes namespace to check == HTML namespace.

[yosin_UTC9, 2016-05-20 06:53:06]

On 2016/05/20 at 06:36:40, dominicc wrote:
> Thank you yukishiino.
> 
> yosin--comments inline to summarize our discussion.
> 
> On 2016/05/20 at 06:17:49, yosin wrote:
> > On 2016/05/20 at 05:15:45, dominicc wrote:
> > > PTAL
> > > 
> > > +yukishiino for bindings OWNERs
> > 
> > OK, finally I understand. |CustomElementDescriptor| is key for looking up
customer element definition.
> > It contains local name and "is" attribute, and namespace (in future?).
> 
> The namespace for a match is always the HTML namespace; I was thinking we
would not carry that around. Instead we check the namespace is HTML when
creating the descriptor.
> 
> > Can we rename |m_name| to |m_isAttrValue| or something?
> 
> The name is not always the is attribute; only when m_name != m_localName.
> 
> > The internal createElementNS steps, given document, namespace,
qualifiedName, and options, are as follows:
> > 
> > 1. Let namespace, prefix, and localName be the result of passing namespace
and qualifiedName to validate and extract. Rethrow any exceptions.
> > 2. Let is be the value of is member of options, or null if no such member
exists.
> > 3. Let definition be the result of looking up a custom element definition,
given the context object, namespace, localName, and is.
> > 4. If is is non-null and definition is null, then throw a NotFoundError.
> > 5. Let element be the result of creating an element given document,
localName, namespace, prefix, is, and with the synchronous custom elements flag
set. Rethrow any exceptions.
> > 6. If is is non-null, then set an attribute value for element using "is" and
is.
> > 7. Return element.
> 
> Step 3 passes namespace to check == HTML namespace.

Thanks for explanation. Finally I understand.
LGTM w/ small nit

[dominicc (has gone to gerrit), 2016-05-20 08:31:41]

Could you publish the nit? Did I miss something?
On May 20, 2016 3:53 PM, "yosin@chromium.org via codereview.chromium.org" <
reply@chromiumcodereview-hr.appspotmail.com> wrote:

> On 2016/05/20 at 06:36:40, dominicc wrote:
> > Thank you yukishiino.
> >
> > yosin--comments inline to summarize our discussion.
> >
> > On 2016/05/20 at 06:17:49, yosin wrote:
> > > On 2016/05/20 at 05:15:45, dominicc wrote:
> > > > PTAL
> > > >
> > > > +yukishiino for bindings OWNERs
> > >
> > > OK, finally I understand. |CustomElementDescriptor| is key for looking
> up
> customer element definition.
> > > It contains local name and "is" attribute, and namespace (in future?).
> >
> > The namespace for a match is always the HTML namespace; I was thinking we
> would not carry that around. Instead we check the namespace is HTML when
> creating the descriptor.
> >
> > > Can we rename |m_name| to |m_isAttrValue| or something?
> >
> > The name is not always the is attribute; only when m_name != m_localName.
> >
> > > The internal createElementNS steps, given document, namespace,
> qualifiedName, and options, are as follows:
> > >
> > > 1. Let namespace, prefix, and localName be the result of passing
> namespace
> and qualifiedName to validate and extract. Rethrow any exceptions.
> > > 2. Let is be the value of is member of options, or null if no such
> member
> exists.
> > > 3. Let definition be the result of looking up a custom element
> definition,
> given the context object, namespace, localName, and is.
> > > 4. If is is non-null and definition is null, then throw a
> NotFoundError.
> > > 5. Let element be the result of creating an element given document,
> localName, namespace, prefix, is, and with the synchronous custom elements
> flag
> set. Rethrow any exceptions.
> > > 6. If is is non-null, then set an attribute value for element using
> "is" and
> is.
> > > 7. Return element.
> >
> > Step 3 passes namespace to check == HTML namespace.
>
> Thanks for explanation. Finally I understand.
> LGTM w/ small nit
>
> https://codereview.chromium.org/1996213002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[dominicc (has gone to gerrit), 2016-05-20 08:31:42]

Could you publish the nit? Did I miss something?
On May 20, 2016 3:53 PM, "yosin@chromium.org via codereview.chromium.org" <
reply@chromiumcodereview-hr.appspotmail.com> wrote:

> On 2016/05/20 at 06:36:40, dominicc wrote:
> > Thank you yukishiino.
> >
> > yosin--comments inline to summarize our discussion.
> >
> > On 2016/05/20 at 06:17:49, yosin wrote:
> > > On 2016/05/20 at 05:15:45, dominicc wrote:
> > > > PTAL
> > > >
> > > > +yukishiino for bindings OWNERs
> > >
> > > OK, finally I understand. |CustomElementDescriptor| is key for looking
> up
> customer element definition.
> > > It contains local name and "is" attribute, and namespace (in future?).
> >
> > The namespace for a match is always the HTML namespace; I was thinking we
> would not carry that around. Instead we check the namespace is HTML when
> creating the descriptor.
> >
> > > Can we rename |m_name| to |m_isAttrValue| or something?
> >
> > The name is not always the is attribute; only when m_name != m_localName.
> >
> > > The internal createElementNS steps, given document, namespace,
> qualifiedName, and options, are as follows:
> > >
> > > 1. Let namespace, prefix, and localName be the result of passing
> namespace
> and qualifiedName to validate and extract. Rethrow any exceptions.
> > > 2. Let is be the value of is member of options, or null if no such
> member
> exists.
> > > 3. Let definition be the result of looking up a custom element
> definition,
> given the context object, namespace, localName, and is.
> > > 4. If is is non-null and definition is null, then throw a
> NotFoundError.
> > > 5. Let element be the result of creating an element given document,
> localName, namespace, prefix, is, and with the synchronous custom elements
> flag
> set. Rethrow any exceptions.
> > > 6. If is is non-null, then set an attribute value for element using
> "is" and
> is.
> > > 7. Return element.
> >
> > Step 3 passes namespace to check == HTML namespace.
>
> Thanks for explanation. Finally I understand.
> LGTM w/ small nit
>
> https://codereview.chromium.org/1996213002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dominicc (has gone to gerrit), 2016-05-20 08:32:33]

Ah, yukishiino's note about the blank line. I'll take a look.
On May 20, 2016 3:53 PM, "yosin@chromium.org via codereview.chromium.org" <
reply@chromiumcodereview-hr.appspotmail.com> wrote:

> On 2016/05/20 at 06:36:40, dominicc wrote:
> > Thank you yukishiino.
> >
> > yosin--comments inline to summarize our discussion.
> >
> > On 2016/05/20 at 06:17:49, yosin wrote:
> > > On 2016/05/20 at 05:15:45, dominicc wrote:
> > > > PTAL
> > > >
> > > > +yukishiino for bindings OWNERs
> > >
> > > OK, finally I understand. |CustomElementDescriptor| is key for looking
> up
> customer element definition.
> > > It contains local name and "is" attribute, and namespace (in future?).
> >
> > The namespace for a match is always the HTML namespace; I was thinking we
> would not carry that around. Instead we check the namespace is HTML when
> creating the descriptor.
> >
> > > Can we rename |m_name| to |m_isAttrValue| or something?
> >
> > The name is not always the is attribute; only when m_name != m_localName.
> >
> > > The internal createElementNS steps, given document, namespace,
> qualifiedName, and options, are as follows:
> > >
> > > 1. Let namespace, prefix, and localName be the result of passing
> namespace
> and qualifiedName to validate and extract. Rethrow any exceptions.
> > > 2. Let is be the value of is member of options, or null if no such
> member
> exists.
> > > 3. Let definition be the result of looking up a custom element
> definition,
> given the context object, namespace, localName, and is.
> > > 4. If is is non-null and definition is null, then throw a
> NotFoundError.
> > > 5. Let element be the result of creating an element given document,
> localName, namespace, prefix, is, and with the synchronous custom elements
> flag
> set. Rethrow any exceptions.
> > > 6. If is is non-null, then set an attribute value for element using
> "is" and
> is.
> > > 7. Return element.
> >
> > Step 3 passes namespace to check == HTML namespace.
>
> Thanks for explanation. Finally I understand.
> LGTM w/ small nit
>
> https://codereview.chromium.org/1996213002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[dominicc (has gone to gerrit), 2016-05-20 08:32:33]

Ah, yukishiino's note about the blank line. I'll take a look.
On May 20, 2016 3:53 PM, "yosin@chromium.org via codereview.chromium.org" <
reply@chromiumcodereview-hr.appspotmail.com> wrote:

> On 2016/05/20 at 06:36:40, dominicc wrote:
> > Thank you yukishiino.
> >
> > yosin--comments inline to summarize our discussion.
> >
> > On 2016/05/20 at 06:17:49, yosin wrote:
> > > On 2016/05/20 at 05:15:45, dominicc wrote:
> > > > PTAL
> > > >
> > > > +yukishiino for bindings OWNERs
> > >
> > > OK, finally I understand. |CustomElementDescriptor| is key for looking
> up
> customer element definition.
> > > It contains local name and "is" attribute, and namespace (in future?).
> >
> > The namespace for a match is always the HTML namespace; I was thinking we
> would not carry that around. Instead we check the namespace is HTML when
> creating the descriptor.
> >
> > > Can we rename |m_name| to |m_isAttrValue| or something?
> >
> > The name is not always the is attribute; only when m_name != m_localName.
> >
> > > The internal createElementNS steps, given document, namespace,
> qualifiedName, and options, are as follows:
> > >
> > > 1. Let namespace, prefix, and localName be the result of passing
> namespace
> and qualifiedName to validate and extract. Rethrow any exceptions.
> > > 2. Let is be the value of is member of options, or null if no such
> member
> exists.
> > > 3. Let definition be the result of looking up a custom element
> definition,
> given the context object, namespace, localName, and is.
> > > 4. If is is non-null and definition is null, then throw a
> NotFoundError.
> > > 5. Let element be the result of creating an element given document,
> localName, namespace, prefix, is, and with the synchronous custom elements
> flag
> set. Rethrow any exceptions.
> > > 6. If is is non-null, then set an attribute value for element using
> "is" and
> is.
> > > 7. Return element.
> >
> > Step 3 passes namespace to check == HTML namespace.
>
> Thanks for explanation. Finally I understand.
> LGTM w/ small nit
>
> https://codereview.chromium.org/1996213002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[yosin_UTC9, 2016-05-20 09:01:16]

you're right. I forgot to publish.

https://codereview.chromium.org/1996213002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElementDescriptorHash.h
(right):

https://codereview.chromium.org/1996213002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementDescriptorHash.h:49:
typedef blink::CustomElementDescriptorHash Hash;
nit: Let's be C++11'ish ;-)

using Hash = blink::CustomElementDescriptorHash;

[dominicc (has gone to gerrit), 2016-05-21 00:38:27]

The CQ bit was checked by dominicc@chromium.org

[dominicc (has gone to gerrit), 2016-05-21 00:38:28]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org,
yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/1996213002/#ps80001
(title: "Address nits.")

[commit-bot: I haz the power, 2016-05-21 00:38:50]

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

[commit-bot: I haz the power, 2016-05-21 00:41:20]

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

[commit-bot: I haz the power, 2016-05-21 00:41:21]

Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dominicc (has gone to gerrit), 2016-05-21 05:18:20]

The CQ bit was checked by dominicc@chromium.org

[dominicc (has gone to gerrit), 2016-05-21 05:18:20]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org,
yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/1996213002/#ps100001
(title: "Bring patch to head.")

[commit-bot: I haz the power, 2016-05-21 05:18:30]

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

[commit-bot: I haz the power, 2016-05-21 07:41:11]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-21 07:42:40]

Description was changed from

==========
Add a tuple of name, local name for hashing custom element definitions.

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

to

==========
Add a tuple of name, local name for hashing custom element definitions.

BUG=594918

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

[commit-bot: I haz the power, 2016-05-21 07:42:41]

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