Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2316263003:
Add toImplSequence() (Closed)
DescriptionAdd toImplSequence()
Description to be written.
BUG=643049
Committed: https://crrev.com/363251af3e48f6fc7143d122cb480a98e110d6b2
Cr-Commit-Position: refs/heads/master@{#417886}
Patch Set 1 #Patch Set 2 : style #
Total comments: 22
Patch Set 3 : comments #
Total comments: 3
Messages
Total messages: 26 (14 generated)
The CQ bit was checked by bashi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add toImplSequence() BUG=643049 ========== to ========== Add toImplSequence() Description to be written. BUG=643049 ==========
bashi@chromium.org changed reviewers: + davaajav@google.com, dominicc@chromium.org
dominicc@, davaajav@ Does this work for observedAttributes?
yukishiino@chromium.org changed reviewers: + yukishiino@chromium.org
https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp (right): https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp:908: v8::MaybeLocal<v8::Object> getIterator(v8::Local<v8::Object> object, v8::Isolate* isolate, ExceptionState& exceptionState) Is it worth doing to make this a general utility as "get an accessor property"? Then, we can say getAccessorProperty(obj, v8::Symbol::GetIterator(isolate)).
https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp (right): https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp:908: v8::MaybeLocal<v8::Object> getIterator(v8::Local<v8::Object> object, v8::Isolate* isolate, ExceptionState& exceptionState) On 2016/09/08 06:24:21, Yuki wrote: > Is it worth doing to make this a general utility as "get an accessor property"? > > Then, we can say getAccessorProperty(obj, v8::Symbol::GetIterator(isolate)). That's a good idea but the point of this CL is to provide getIterator defined in https://tc39.github.io/ecma262/#sec-getiterator We can generalize it later.
On 2016/09/08 at 06:15:48, bashi wrote:
> dominicc@, davaajav@
>
> Does this work for observedAttributes?
I think it works for observedAttributes. Running the following code throws
TypeError as it should.
var constructor = function () {}
constructor.prototype.attributeChangedCallback = function () { };
constructor.observedAttributes = {[Symbol.iterator]: 1};
customElements.define('a-a', constructor);
LGTM. https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp (right): https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp:908: v8::MaybeLocal<v8::Object> getIterator(v8::Local<v8::Object> object, v8::Isolate* isolate, ExceptionState& exceptionState) On 2016/09/08 06:30:49, bashi1 wrote: > On 2016/09/08 06:24:21, Yuki wrote: > > Is it worth doing to make this a general utility as "get an accessor > property"? > > > > Then, we can say getAccessorProperty(obj, v8::Symbol::GetIterator(isolate)). > > That's a good idea but the point of this CL is to provide getIterator defined in > https://tc39.github.io/ecma262/#sec-getiterator > > We can generalize it later. Oops, I thought this is an accessor property, but it's not. I understand. By the way, I'm a little uneasy with the name |blink::getIterator|. Does it sound a bit too general, especially considering that C++ also supports iterators? I don't have a good idea, though. Off the top of my head, getEsIterator? esGetIterator? where es = ECMAScript. https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp:908: v8::MaybeLocal<v8::Object> getIterator(v8::Local<v8::Object> object, v8::Isolate* isolate, ExceptionState& exceptionState) Please make v8::Isolate* as the first argument. https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right): https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Binding.h:737: VectorType toImplSequence(v8::Local<v8::Value> value, v8::Isolate* isolate, ExceptionState& exceptionState) Could you make v8::Isolate* the first argument? https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Binding.h:739: typedef typename VectorType::ValueType ValueType; I guess using VectorType::ValueType; does the trick. https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Binding.h:760: exceptionState.throwTypeError("The next property should be callable."); I think "Iterator.next should be callable." is easier to understand, like below. https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Binding.h:767: exceptionState.throwTypeError("Iterator.next() does not return an object."); s/does/did/
haraken@chromium.org changed reviewers: + haraken@chromium.org
Let's add unit tests to V8BindingTest.cpp and layout tests for custom elements. https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp (right): https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp:913: return v8::MaybeLocal<v8::Object>(); Don't we need to throw an exception here? https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp:923: return v8::MaybeLocal<v8::Object>(); Ditto. https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right): https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Binding.h:732: v8::MaybeLocal<v8::Object> getIterator(v8::Local<v8::Object>, v8::Isolate*, ExceptionState&); I don't have any strong opinion but might prefer returning v8::Local<v8::Object>. i.e., return an empty handle when the iterator is not available. https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Binding.h:765: return VectorType(); Don't we need to throw an exception? https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Binding.h:775: return VectorType(); Ditto. https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Binding.h:778: return VectorType(); Ditto.
The CQ bit was checked by bashi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
Added unit test. I guess that there are some layout tests already (or davaajav@ is adding them?). https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp (right): https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp:908: v8::MaybeLocal<v8::Object> getIterator(v8::Local<v8::Object> object, v8::Isolate* isolate, ExceptionState& exceptionState) On 2016/09/08 08:32:21, Yuki wrote: > Please make v8::Isolate* as the first argument. Done. https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp:913: return v8::MaybeLocal<v8::Object>(); On 2016/09/08 09:32:08, haraken wrote: > > Don't we need to throw an exception here? No. IIRC, !ToLocal() means that v8 already thrown an exception. https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right): https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Binding.h:732: v8::MaybeLocal<v8::Object> getIterator(v8::Local<v8::Object>, v8::Isolate*, ExceptionState&); On 2016/09/08 09:32:08, haraken wrote: > > I don't have any strong opinion but might prefer returning > v8::Local<v8::Object>. i.e., return an empty handle when the iterator is not > available. > I think using MaybeLocal has two advantages: 1. Caller must check empty handles 2. - !MaybeLocal.ToLocal() -> We are sure that there is an exception. - Local.IsEmpty() -> No information. MaybeLocal is useful in some cases. We shouldn't avoid using it too much. https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Binding.h:737: VectorType toImplSequence(v8::Local<v8::Value> value, v8::Isolate* isolate, ExceptionState& exceptionState) On 2016/09/08 08:32:21, Yuki wrote: > Could you make v8::Isolate* the first argument? Done. Having said that it's inconsistent with other array or sequence conversion functions like toImplArray(). I'm not sure putting isolate first makes sense at this point. (I don't want to change other signatures in this CL) https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Binding.h:739: typedef typename VectorType::ValueType ValueType; On 2016/09/08 08:32:21, Yuki wrote: > I guess > using VectorType::ValueType; > does the trick. Done. https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Binding.h:760: exceptionState.throwTypeError("The next property should be callable."); On 2016/09/08 08:32:21, Yuki wrote: > I think > "Iterator.next should be callable." > is easier to understand, like below. Done. https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Binding.h:765: return VectorType(); On 2016/09/08 09:32:08, haraken wrote: > > Don't we need to throw an exception? No, as described in another reply. https://codereview.chromium.org/2316263003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Binding.h:767: exceptionState.throwTypeError("Iterator.next() does not return an object."); On 2016/09/08 08:32:21, Yuki wrote: > s/does/did/ Done. https://codereview.chromium.org/2316263003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right): https://codereview.chromium.org/2316263003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Binding.h:889: VectorType toImplSequence(v8::Isolate* isolate, v8::Local<v8::Value> value, ExceptionState& exceptionState) Moved below toExecutionContext(). We can't call callAsFunction() directly and need to use V8ScriptRunner::callFunction() to avoid an assertion failure in V8. https://codereview.chromium.org/2316263003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Binding.h:932: if (doneBoolean->Value()) Don't append when |done| is true, as |value| is `undefined` in that case.
LGTM https://codereview.chromium.org/2316263003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right): https://codereview.chromium.org/2316263003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Binding.h:907: while (true) { Maybe we want to share this code with DictionaryIterator::next (in a follow-up CL).
LGTM too; FYI Davaa's tests are here: https://codereview.chromium.org/2321903003
The CQ bit was checked by davaajav@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from yukishiino@chromium.org Link to the patchset: https://codereview.chromium.org/2316263003/#ps40001 (title: "comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add toImplSequence() Description to be written. BUG=643049 ========== to ========== Add toImplSequence() Description to be written. BUG=643049 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add toImplSequence() Description to be written. BUG=643049 ========== to ========== Add toImplSequence() Description to be written. BUG=643049 Committed: https://crrev.com/363251af3e48f6fc7143d122cb480a98e110d6b2 Cr-Commit-Position: refs/heads/master@{#417886} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/363251af3e48f6fc7143d122cb480a98e110d6b2 Cr-Commit-Position: refs/heads/master@{#417886} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
