We just use toImplArray to convert the property to a sequence.

Here's the spec for define:
https://html.spec.whatwg.org/#dom-customelementregistry-define

Step 10.6.2 says:

"If observedAttributesIterable is not undefined, then set observedAttributes to the result of converting observedAttributesIterable to a sequence<DOMString>. Rethrow any exceptions from the conversion."

Here's the definition of how to do that conversion:

https://heycam.github.io/webidl/#es-sequence

We should probably be using toV8Sequence:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/v8/V8Binding.h?sq=package:chromium&rcl=1473247219&l=733

However it looks like it doesn't exactly match the spec? There's nothing in the spec about dates; the implementation doesn't retrieve symbol.iterator; etc.

Yeah, toV8Sequence is a bit out-of-date. "Date" was removed from the spec.

https://www.w3.org/Bugs/Public/show_bug.cgi?id=22824

In general we should make it up-to-date but we should also be careful not to break existing behavior. I'll take a closer look later.

Patch up at https://codereview.chromium.org/2316263003 by bashi-san

Layout tests at https://codereview.chromium.org/2321903003

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/363251af3e48f6fc7143d122cb480a98e110d6b2

commit 363251af3e48f6fc7143d122cb480a98e110d6b2
Author: bashi <bashi@chromium.org>
Date: Mon Sep 12 07:38:45 2016

Add toImplSequence()

Description to be written.

BUG= 643049 

Review-Url: https://codereview.chromium.org/2316263003
Cr-Commit-Position: refs/heads/master@{#417886}

[modify] https://crrev.com/363251af3e48f6fc7143d122cb480a98e110d6b2/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp
[modify] https://crrev.com/363251af3e48f6fc7143d122cb480a98e110d6b2/third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp
[modify] https://crrev.com/363251af3e48f6fc7143d122cb480a98e110d6b2/third_party/WebKit/Source/bindings/core/v8/V8Binding.h
[modify] https://crrev.com/363251af3e48f6fc7143d122cb480a98e110d6b2/third_party/WebKit/Source/bindings/core/v8/V8BindingTest.cpp

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/75613110cd274c6fee47675dfe78df3ba20205f4

commit 75613110cd274c6fee47675dfe78df3ba20205f4
Author: davaajav <davaajav@google.com>
Date: Thu Sep 15 02:26:57 2016

Custom Elements: layout tests for observedAttributes

1. attributeChangedCallback should be fired only from
the elements in observedAttributes
2. Checks for cases when converting observedAttributes
to sequence<DOMString> should throw TypeError
https://heycam.github.io/webidl/#create-sequence-from-iterable

BUG= 643049 

Review-Url: https://codereview.chromium.org/2321903003
Cr-Commit-Position: refs/heads/master@{#418761}

[modify] https://crrev.com/75613110cd274c6fee47675dfe78df3ba20205f4/third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html

It looks like sequence conversion should rethrow exceptions. In this algorithm:

https://heycam.github.io/webidl/#create-sequence-from-iterable

Step 2 works, Davaa's test shows this.
Step 4.2, 4.5 seems to swallow exceptions? We didn't add a test for this and rniwa says a WebKit custom elements test indicates we don't transmit that exception correctly.

Hmm, we don't use v8::TryCatch in toImplSequence so I think if there is an exception it should be thrown. Do you have a repro case?

See also:  issue 393866  - it'd be nice to get toImplSequence used for handling sequence<> correctly elsewhere too.

I looked for the repro case but couldn't find it. I will reopen if I find it.

OK, I have created a repro for you:

http://codepen.io/anon/pen/WGmObZ

This should throw 'bar' from define but it does not. The problem is precisely because you're not using TryCatch. toImplSequence/getEsIterator should be using TryCatch to transmit the exception to the ExceptionState parameter that those methods take. It doesn't.

Or should the caller use ExceptionState *and* TryCatch? (Why can ExceptionState transmit V8 exceptions then?)

'use strict';

let attrs = {};
attrs[Symbol.iterator] = function*() {
  throw 'bar';
}

class X extends HTMLElement {
  static get observedAttributes() {
    console.log('retrieving attrs');
    return attrs;
  }
  attributeChangedCallback(name) {
    console.log(`${name} changed`)
  }
}

customElements.define('x-x', X);

Thanks for the repro. I confirmed "bar" isn't thrown.

I'm still wondering why an exception wasn't thrown. IIUC when we don't use v8::TryCatch, an exception is already thrown if ToLocal() fails.

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/v8/V8Binding.h?rcl=0&l=1045

jochen@, haraken@: Am I missing something? Should we use v8::TryCatch in toImplSequence() to transmit an exception to ExceptionState?

An exception should have been thrown when ToLocal() fails.

Is it possible that we have a v8::TryCatch somewhere above the stack and it is suppressing the exception?

The problem is getEsIterator doesn't use TryCatch to populate the ExceptionState with the exception.

An API where you have to use TryCatch *and* ExceptionState doesn't make much sense to me--we may as well just use TryCatch.

An API where you have to use ExceptionState but *can't* have TryCatch somewhere up the stack doesn't make much sense to me either.

Some APIs are called with JS callers on the stack, others without, that is part of the reason ExceptionState exists.

Hence you should catch the exceptions emanating from @@iterator and propagate them to the ExceptionState your API uses.

Thanks for the clarification. I understand your point and it makes sense to me. I'll create CL to use TryCatch.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/19f37fd06b24d3af5e84f019e561d18c125a29e9

commit 19f37fd06b24d3af5e84f019e561d18c125a29e9
Author: bashi <bashi@chromium.org>
Date: Wed Oct 26 07:11:16 2016

Use v8::TryCatch in toImplSequence

To transmit exceptions to ExceptionState.

BUG= 643049 

Review-Url: https://codereview.chromium.org/2455483002
Cr-Commit-Position: refs/heads/master@{#427621}

[modify] https://crrev.com/19f37fd06b24d3af5e84f019e561d18c125a29e9/third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html
[modify] https://crrev.com/19f37fd06b24d3af5e84f019e561d18c125a29e9/third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp
[modify] https://crrev.com/19f37fd06b24d3af5e84f019e561d18c125a29e9/third_party/WebKit/Source/bindings/core/v8/V8Binding.h