Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2024073002:
Add callbacks to ScriptCustomElementDefinition (Closed)
|
Created:
4 years, 6 months ago by kojii Modified:
4 years, 6 months ago CC:
blink-reviews, blink-reviews-bindings_chromium.org, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, dominicc+watchlist_chromium.org, eae+blinkwatch, rwlbuis, sof, webcomponents-bugzilla_chromium.org, yosin_UTC9 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd callbacks to ScriptCustomElementDefinition
This patch adds callbacks and observedAttributes to
ScriptCustomElementDefinition, as defined in the Element Definition
spec[1].
[1] https://html.spec.whatwg.org/multipage/scripting.html#element-definition
BUG=594918
TEST=imported/wpt/custom-elements/custom-elements-registry/define.html
Committed: https://crrev.com/d17a89371a6d36914685940a28756f275728d9a5
Cr-Commit-Position: refs/heads/master@{#398243}
Patch Set 1 #Patch Set 2 : Still WIP #Patch Set 3 : Fix when no callbacks, cleanup, and reset-result #
Total comments: 11
Patch Set 4 : yosin review #Patch Set 5 : Make ScriptCustomElementDefinitionBuilder friend to ScriptCustomElementDefinition #
Total comments: 20
Patch Set 6 : dominicc, yosin review #
Total comments: 6
Patch Set 7 : yosin review #Patch Set 8 : dominicc review #
Total comments: 11
Patch Set 9 : haraken review #
Total comments: 3
Patch Set 10 : haraken review #Patch Set 11 : v8CallOrCrash reverted to v8Call #Patch Set 12 : Add tryCatch #Messages
Total messages: 40 (11 generated)
Description was changed from ========== WIP: cacheProperties BUG= ========== to ========== Add callbacks to ScriptCustomElementDefinition This patch adds callbacks and observedAttributes to ScriptCustomElementDefinition, as defined in the Element Definition spec[1]. [1] https://html.spec.whatwg.org/multipage/scripting.html#element-definition BUG=594918 ==========
kojii@chromium.org changed reviewers: + dominicc@chromium.org
PTAL.
yosin@chromium.org changed reviewers: + yosin@chromium.org
Can we have a test? https://codereview.chromium.org/2024073002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2024073002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:121: v8::Local<v8::Array> array = v8::Array::New(scriptState->isolate(), 4); Could you introduce enum for array index? Or, use FOR_EACH_XXX? #define FOR_EACH_KEEP_ALIVE(V) \ V(0, prototype) \ V(1, disconnectedCallback) \ ... V(3, attributeChangeCallback) #define V(index, name) keepAliveIfNotEmpty(array, index, name) FOR_EACH_KEEP_ALIVE(V) #undef V https://codereview.chromium.org/2024073002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:153: if (!observedAttributes.isEmpty()) { Prefer early-return style https://codereview.chromium.org/2024073002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.h (right): https://codereview.chromium.org/2024073002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.h:36: const v8::Local<v8::Object>& constructor, Can we have struct ScriptCustomElementDefinitionInit or another name to as pass v8::Local by name? It is error prone to have many parameters with same type. struct Init { v8::Local<v8::Object> constructor; v8::Local<v8::Object> prototype; ... }; Init init; init.constructor = ... init.prototype = ... https://codereview.chromium.org/2024073002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.h:54: const v8::Local<v8::Object>& connectedCallback, FYI, since v8::Local<T> is just T*, pass by value may be faster. See comment in "base/strings/string_piece.h" https://codereview.chromium.org/2024073002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp (right): https://codereview.chromium.org/2024073002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:114: if (!getValue(m_constructor, "observedAttributes", observedAttributesValue)) Can we have const char* for these bar string literals to avoid typo?
Description was changed from ========== Add callbacks to ScriptCustomElementDefinition This patch adds callbacks and observedAttributes to ScriptCustomElementDefinition, as defined in the Element Definition spec[1]. [1] https://html.spec.whatwg.org/multipage/scripting.html#element-definition BUG=594918 ========== to ========== Add callbacks to ScriptCustomElementDefinition This patch adds callbacks and observedAttributes to ScriptCustomElementDefinition, as defined in the Element Definition spec[1]. [1] https://html.spec.whatwg.org/multipage/scripting.html#element-definition BUG=594918 TEST=imported/wpt/custom-elements/custom-elements-registry/define.html ==========
> Can we have a test? There's already upstreamed to wpt, you see -expected.txt turns to PASS in this CL. There were bugs in the test unfortunately X-| which I fixed last night at: https://github.com/w3c/web-platform-tests/pull/3099 and re-imported. https://codereview.chromium.org/2024073002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2024073002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:121: v8::Local<v8::Array> array = v8::Array::New(scriptState->isolate(), 4); On 2016/06/01 at 06:33:33, Yosi_UTC9 wrote: > Could you introduce enum for array index? > Or, use FOR_EACH_XXX? > > #define FOR_EACH_KEEP_ALIVE(V) \ > V(0, prototype) \ > V(1, disconnectedCallback) \ > ... > V(3, attributeChangeCallback) > > > #define V(index, name) keepAliveIfNotEmpty(array, index, name) > FOR_EACH_KEEP_ALIVE(V) > #undef V Done, not my favorite, but I can live with it. https://codereview.chromium.org/2024073002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:153: if (!observedAttributes.isEmpty()) { On 2016/06/01 at 06:33:33, Yosi_UTC9 wrote: > Prefer early-return style Done. https://codereview.chromium.org/2024073002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.h (right): https://codereview.chromium.org/2024073002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.h:36: const v8::Local<v8::Object>& constructor, On 2016/06/01 at 06:33:33, Yosi_UTC9 wrote: > Can we have struct ScriptCustomElementDefinitionInit or another name to as pass v8::Local by name? > It is error prone to have many parameters with same type. > > struct Init { > v8::Local<v8::Object> constructor; > v8::Local<v8::Object> prototype; > ... > }; > > > Init init; > init.constructor = ... > init.prototype = ... I think it's too much complex, when we have ScriptCustomElementDefinition and ScriptCustomElementDefinitionBuilder having almost the same members, and add yet another, 3rd almost similar struct. https://codereview.chromium.org/2024073002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.h:54: const v8::Local<v8::Object>& connectedCallback, On 2016/06/01 at 06:33:33, Yosi_UTC9 wrote: > FYI, since v8::Local<T> is just T*, pass by value may be faster. I can do that, but Set and many v8 functions receives Local, so I'll need to re-create them. Isn't it faster to pass reference than re-creating Locals? > See comment in "base/strings/string_piece.h" Which comment? I looked at the file but don't understand which comment in the file you're referring to. https://codereview.chromium.org/2024073002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp (right): https://codereview.chromium.org/2024073002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:114: if (!getValue(m_constructor, "observedAttributes", observedAttributesValue)) On 2016/06/01 at 06:33:33, Yosi_UTC9 wrote: > Can we have const char* for these bar string literals to avoid typo? Done.
https://codereview.chromium.org/2024073002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.h (right): https://codereview.chromium.org/2024073002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.h:36: const v8::Local<v8::Object>& constructor, On 2016/06/01 at 07:19:27, kojii wrote: > On 2016/06/01 at 06:33:33, Yosi_UTC9 wrote: > > Can we have struct ScriptCustomElementDefinitionInit or another name to as pass v8::Local by name? > > It is error prone to have many parameters with same type. > > > > struct Init { > > v8::Local<v8::Object> constructor; > > v8::Local<v8::Object> prototype; > > ... > > }; > > > > > > Init init; > > init.constructor = ... > > init.prototype = ... > > I think it's too much complex, when we have ScriptCustomElementDefinition and ScriptCustomElementDefinitionBuilder having almost the same members, and add yet another, 3rd almost similar struct. I made ScriptCustomElementDefinitionBuilder friend to ScriptCustomElementDefinition is PS5. Hope this resolves your concern.
Good work, some comments inline. https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:110: v8CallOrCrash(array->Set(scriptState->context(), 0, builder.m_prototype)); Why not use keepAliveIfNotEmpty for m_prototype too? It won't be empty but superfluous test is probably worth it to keep things consistent. https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:127: , m_constructor(builder.m_scriptState->isolate(), builder.m_constructor) I guess you could just use m_scriptState at this point but either way is OK. https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp (right): https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:128: return getCallable(connectedCallback, m_connectedCallback) Please add unit tests which detach the window in callbacks retrieving each of these properties; with a parsed upgrade candidate existing when define is called. See the fork() thing in the tests in custom-elements. https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.h (right): https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.h:41: friend class ScriptCustomElementDefinition; I don't really like the ScriptCustomElementDefinition being a friend of the builder. It puts the registry within reach of the definition, when really the registry is the master object and should act on the definition. If you're worried about the long, repetitive repetitive parameter list to create, I suggest you encapsulate the builder's state in a parameter object and pass that object to the ScriptCustomElementDefinition. A future unit test that wants to create a ScriptCustomElementDefinition can create one without having to go through a builder. https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.h:51: Vector<AtomicString> m_observedAttributes; Any reason not to just make this a HashSet and move or copy the HashSet to the definition? https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp (right): https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:88: // 8-9: observed attributes caching is done below, together with callbacks. Do these tests in order. Authors can observe this order. You can suggest domenic change the spec, and put a TODO that points to the spec issue. https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:96: // 8-9: observed attributes caching I think we can remove this comment; it might be useful to add a comment to cacheProperties indicating which steps it refers to.
FYI, changes here may be relevant: https://github.com/whatwg/html/pull/1333/files For example, can you use bindings marshaling for converting the value to sequence<DOMString> (I *think* that is what this change requires) and there's a note about exception handling.
https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:109: v8::Local<v8::Array> array = v8::Array::New(scriptState->isolate(), 4); I'll add "whenDefinedPromise" here, so JS side value holding and C++ cache should be described in one place. https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:111: #define V(index, name) \ We should have FOR_EACH_XXX() and it should be used in other places, e.g. member variable declaration and so on. Also, we should avoid macro expansion to generate code for one time usage. FOR_EACH_XXX() is a pattern utilized in v8, dart and Blink too. V8_HIDDEN_VALUES() https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So... In v8: https://code.google.com/p/chromium/codesearch#search/&q=FOR_EACH%20file:%5Esr... https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp (right): https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:89: bool ScriptCustomElementDefinitionBuilder::getCallable(const char* name, It is better to return Optional, or v8::Maybe<v8::Object>, rather than output variable with bool. https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So... Optional may be preferred way since it will be std::optional<T>. https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.h (right): https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.h:41: friend class ScriptCustomElementDefinition; On 2016/06/02 at 00:31:27, dominicc wrote: > I don't really like the ScriptCustomElementDefinition being a friend of the builder. It puts the registry within reach of the definition, when really the registry is the master object and should act on the definition. > > If you're worried about the long, repetitive repetitive parameter list to create, I suggest you encapsulate the builder's state in a parameter object and pass that object to the ScriptCustomElementDefinition. A future unit test that wants to create a ScriptCustomElementDefinition can create one without having to go through a builder. When const-ish isn't important in |ScriptCusomElementDefintion|, I suggest to make |SCEDBuilder| as friend of |SCED|. In this case, ctor of SCED is private default ctor. Then |SCEDBuilder| set member variables of SCED directly. Here is a simple example of my editor's ComputedStyle and ComputedStyle::Builder. https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.h:54: bool getValue(const v8::Local<v8::Object>&, const char*, Term "get" is so usual, we should use another name, e.g. |valueFor()|, |valueFor()|, ... https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.h:56: bool getCallable(const char*, v8::Local<v8::Object>&) const; In Blink, getter doesn't have "get", but in other projects, "get" is used for getter. Term "get" is so usual, we should use another name, e.g. |calalbleFor()|
PTAL. https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:110: v8CallOrCrash(array->Set(scriptState->context(), 0, builder.m_prototype)); On 2016/06/02 at 00:31:27, dominicc wrote: > Why not use keepAliveIfNotEmpty for m_prototype too? It won't be empty but superfluous test is probably worth it to keep things consistent. Done. https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:111: #define V(index, name) \ On 2016/06/02 at 01:33:38, Yosi_UTC9 wrote: > We should have FOR_EACH_XXX() and it should be used in other places, e.g. member variable declaration and so on. > Also, we should avoid macro expansion to generate code for one time usage. > FOR_EACH_XXX() is a pattern utilized in v8, dart and Blink too. > > V8_HIDDEN_VALUES() > https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So... > > In v8: > https://code.google.com/p/chromium/codesearch#search/&q=FOR_EACH%20file:%5Esr... I know, but I don't like it, esp. when the list is only 4. Let's worry about it when the list gets long enough to overcome the cost of readability. https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:127: , m_constructor(builder.m_scriptState->isolate(), builder.m_constructor) On 2016/06/02 at 00:31:27, dominicc wrote: > I guess you could just use m_scriptState at this point but either way is OK. This was reverted, see comments in ScriptCustomElementDefinitionBuilder.h. https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.h (right): https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.h:41: friend class ScriptCustomElementDefinition; On 2016/06/02 at 00:31:27, dominicc wrote: > I don't really like the ScriptCustomElementDefinition being a friend of the builder. It puts the registry within reach of the definition, when really the registry is the master object and should act on the definition. Reverted. https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.h:51: Vector<AtomicString> m_observedAttributes; On 2016/06/02 at 00:31:27, dominicc wrote: > Any reason not to just make this a HashSet and move or copy the HashSet to the definition? Done. Not really, had a micro-optimization in mind originally but not worth to worry. https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.h:56: bool getCallable(const char*, v8::Local<v8::Object>&) const; On 2016/06/02 at 01:33:38, Yosi_UTC9 wrote: > In Blink, getter doesn't have "get", but in other projects, "get" is used for getter. > Term "get" is so usual, we should use another name, e.g. |calalbleFor()| Done, thought "get" is chromium style but I don't have opinions here.
https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2024073002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:111: #define V(index, name) \ On 2016/06/02 at 03:18:08, kojii wrote: > On 2016/06/02 at 01:33:38, Yosi_UTC9 wrote: > > We should have FOR_EACH_XXX() and it should be used in other places, e.g. member variable declaration and so on. > > Also, we should avoid macro expansion to generate code for one time usage. > > FOR_EACH_XXX() is a pattern utilized in v8, dart and Blink too. > > > > V8_HIDDEN_VALUES() > > https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So... > > > > In v8: > > https://code.google.com/p/chromium/codesearch#search/&q=FOR_EACH%20file:%5Esr... > > I know, but I don't like it, esp. when the list is only 4. Let's worry about it when the list gets long enough to overcome the cost of readability. Since list is used for parameter of ctor, it might be 4 until spec is changed. But, size of Array soon to be 5, for |promiseWhenDefined|. Per isolation of CL's, I'm OK to rewrite this code when I add |promiseWhenDefined|. https://codereview.chromium.org/2024073002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2024073002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:124: #define V(index, name) \ No need to use macro, it is hard to read. Name |V()| comes from "visitor" for |FOR_EACH_XXX|. If you really want to use macro, it should be named SET_KEEP_ALIVE_IF_NO_EMPTY() or another descriptive name. We still have consistency issue between array construction and these setters. This is error prone, e.g. adding new line V(4, promiseWhenDefine) but forgot to update v8::Array::New(isoalte, 5); https://codereview.chromium.org/2024073002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp (right): https://codereview.chromium.org/2024073002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:113: const char* observedAttributes = "observedAttributes"; const char* const kObservedAttributes = "observedAttributes"; https://codereview.chromium.org/2024073002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:134: const char* connectedCallback = "connectedCallback"; const char* const kConnectedCallback= "connectedCallback"; Note: Blink doesn't have naming convention for constant name. So, it is OK to name without "k" prefix now. But, when we'll use Chromium-style, it should have prefix "k".
https://codereview.chromium.org/2024073002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2024073002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:124: #define V(index, name) \ On 2016/06/02 at 04:30:31, Yosi_UTC9 wrote: > No need to use macro, it is hard to read. > > Name |V()| comes from "visitor" for |FOR_EACH_XXX|. > If you really want to use macro, it should be named SET_KEEP_ALIVE_IF_NO_EMPTY() or another descriptive name. > > We still have consistency issue between array construction and these setters. > This is error prone, e.g. adding new line V(4, promiseWhenDefine) but forgot to update v8::Array::New(isoalte, 5); Done, reverted the macro as per your suggestion. https://codereview.chromium.org/2024073002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp (right): https://codereview.chromium.org/2024073002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:113: const char* observedAttributes = "observedAttributes"; On 2016/06/02 at 04:30:31, Yosi_UTC9 wrote: > const char* const kObservedAttributes = "observedAttributes"; Done. https://codereview.chromium.org/2024073002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:134: const char* connectedCallback = "connectedCallback"; On 2016/06/02 at 04:30:31, Yosi_UTC9 wrote: > const char* const kConnectedCallback= "connectedCallback"; > > Note: Blink doesn't have naming convention for constant name. So, it is OK to name without "k" prefix now. > But, when we'll use Chromium-style, it should have prefix "k". Done.
Any thoughts on the ordering?
On 2016/06/02 at 05:19:20, dominicc wrote: > Any thoughts on the ordering? Ordering of...keepAlive()?
On 2016/06/02 at 05:30:11, kojii wrote: > On 2016/06/02 at 05:19:20, dominicc wrote: > > Any thoughts on the ordering? > > Ordering of...keepAlive()? Retrieving the prototype etc. at the right time. Putting the steps together the way you have done makes sense. Just file an issue for domenic and put a TODO comment on the code you have saying, hey, check that this issue is closed and conform with what the editors decide.
On 2016/06/02 at 07:46:48, dominicc wrote: > On 2016/06/02 at 05:30:11, kojii wrote: > > On 2016/06/02 at 05:19:20, dominicc wrote: > > > Any thoughts on the ordering? > > > > Ordering of...keepAlive()? > > Retrieving the prototype etc. at the right time. > > Putting the steps together the way you have done makes sense. Just file an issue for domenic and put a TODO comment on the code you have saying, hey, check that this issue is closed and conform with what the editors decide. Ah, make sense, filed https://github.com/whatwg/html/issues/1373 and put the comment in PS8.
ping?
dominicc@chromium.org changed reviewers: + blink-reviews-bindings@chromium.org
LGTM, sorry for the slow reply +blink-reviews-bindings for bindings
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2024073002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2024073002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:88: persistent.setPhantom(); Why do you need to create a phantom handle? I think that just setting the handle on the array would be enough to keep the V8 object alive. https://codereview.chromium.org/2024073002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp (right): https://codereview.chromium.org/2024073002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:62: I'd change 'char*' in this file with 'String'. https://codereview.chromium.org/2024073002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:96: return true; Just to confirm: You want to return true for undefined but return false for null, right? https://codereview.chromium.org/2024073002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:131: bool ScriptCustomElementDefinitionBuilder::cacheProperties() I'm a bit confused with the name "cacheProperties". This method is just looking up existing atributes on m_constructor and m_prototype. How is it "caching" somethings? https://codereview.chromium.org/2024073002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right): https://codereview.chromium.org/2024073002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8Binding.h:779: static inline String nativeValue(v8::Isolate* isolate, v8::Local<v8::Value> value, ExceptionState& exceptionState) Why do you want to return a String for NativeValueTraits<AtomicString>? Shouldn't this return an AtomicString?
Patchset #9 (id:160001) has been deleted
Patchset #9 (id:180001) has been deleted
PTAL. https://codereview.chromium.org/2024073002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2024073002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:88: persistent.setPhantom(); On 2016/06/06 at 01:39:15, haraken wrote: > Why do you need to create a phantom handle? I think that just setting the handle on the array would be enough to keep the V8 object alive. This is for perf to retrieve the value without going through v8 map, while v8 object is to keep it alive. Should I just keep a pointer, not ScopedPersistent in this case? https://codereview.chromium.org/2024073002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp (right): https://codereview.chromium.org/2024073002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:62: On 2016/06/06 at 01:39:15, haraken wrote: > I'd change 'char*' in this file with 'String'. Done. https://codereview.chromium.org/2024073002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:96: return true; On 2016/06/06 at 01:39:15, haraken wrote: > Just to confirm: You want to return true for undefined but return false for null, right? Yes: Undefined or IsCallable -> true (success) Otherwise -> false (throw error) Added the comment to clarify that. https://codereview.chromium.org/2024073002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:131: bool ScriptCustomElementDefinitionBuilder::cacheProperties() On 2016/06/06 at 01:39:15, haraken wrote: > I'm a bit confused with the name "cacheProperties". This method is just looking up existing atributes on m_constructor and m_prototype. How is it "caching" somethings? This method retrieves values and keep it in member field, so that Blink uses the value at this point even if JS changes them afterwards; i.e. class C { connectedCallback() {} } customElements.define('a-a', C); C.prototype.connectedCallback = function () { doSomethingDifferent(); } The spec says we must call the function before "define()". That said, maybe "retrieve" works better? https://codereview.chromium.org/2024073002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right): https://codereview.chromium.org/2024073002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8Binding.h:779: static inline String nativeValue(v8::Isolate* isolate, v8::Local<v8::Value> value, ExceptionState& exceptionState) On 2016/06/06 at 01:39:15, haraken wrote: > Why do you want to return a String for NativeValueTraits<AtomicString>? Shouldn't this return an AtomicString? Done, that was typo, thank you for catching.
LGTM https://codereview.chromium.org/2024073002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp (right): https://codereview.chromium.org/2024073002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:131: bool ScriptCustomElementDefinitionBuilder::cacheProperties() On 2016/06/06 11:12:35, kojii wrote: > On 2016/06/06 at 01:39:15, haraken wrote: > > I'm a bit confused with the name "cacheProperties". This method is just > looking up existing atributes on m_constructor and m_prototype. How is it > "caching" somethings? > > This method retrieves values and keep it in member field, so that Blink uses the > value at this point even if JS changes them afterwards; i.e. > class C { connectedCallback() {} } > customElements.define('a-a', C); > C.prototype.connectedCallback = function () { doSomethingDifferent(); } > The spec says we must call the function before "define()". > > That said, maybe "retrieve" works better? rememberOriginalProperties ? Also add a comment about the behavior. https://codereview.chromium.org/2024073002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp (right): https://codereview.chromium.org/2024073002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:91: return v8Call(object->Get(context, nameString), value); Use v8CallOrCrash.
On 2016/06/06 at 11:45:52, haraken wrote: > > That said, maybe "retrieve" works better? > > rememberOriginalProperties ? Done. > Also add a comment about the behavior. Done. https://codereview.chromium.org/2024073002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp (right): https://codereview.chromium.org/2024073002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:91: return v8Call(object->Get(context, nameString), value); On 2016/06/06 at 11:45:52, haraken wrote: > Use v8CallOrCrash. Done.
LGTM
https://codereview.chromium.org/2024073002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp (right): https://codereview.chromium.org/2024073002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:91: return v8Call(object->Get(context, nameString), value); On 2016/06/06 at 13:10:40, kojii wrote: > On 2016/06/06 at 11:45:52, haraken wrote: > > Use v8CallOrCrash. > > Done. Oh, no, I had to revert this in PS11. The spec defines "rethrow any exceptions" during the Get(): https://html.spec.whatwg.org/multipage/scripting.html#element-definition and the test has a case where getter throws an exception: imported/wpt/custom-elements/custom-elements-registry/define.html so changing this to v8CallOrCrash() crashed the test. We do check the return value, and the exception is stored in m_exceptionState to rethrow. I assume v8call() is fine as long as we handle the exceptions appropriately.
On 2016/06/06 at 14:43:33, kojii wrote: > ...and the exception is stored in m_exceptionState to rethrow. Ah, we don't store in m_exceptionState explicitly, but tests indicate we're rethrowing correctly. Probably the error is kept in context? > I assume v8call() is fine as long as we handle the exceptions appropriately. So, minor correction, but this still stands.
On 2016/06/06 14:43:33, kojii wrote: > https://codereview.chromium.org/2024073002/diff/200001/third_party/WebKit/Sou... > File > third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp > (right): > > https://codereview.chromium.org/2024073002/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:91: > return v8Call(object->Get(context, nameString), value); > On 2016/06/06 at 13:10:40, kojii wrote: > > On 2016/06/06 at 11:45:52, haraken wrote: > > > Use v8CallOrCrash. > > > > Done. > > Oh, no, I had to revert this in PS11. The spec defines "rethrow any exceptions" > during the Get(): > https://html.spec.whatwg.org/multipage/scripting.html#element-definition > and the test has a case where getter throws an exception: > imported/wpt/custom-elements/custom-elements-registry/define.html > so changing this to v8CallOrCrash() crashed the test. > > We do check the return value, and the exception is stored in m_exceptionState to > rethrow. I assume v8call() is fine as long as we handle the exceptions > appropriately. Then the current code will work, but it would be better to make the behavior more explicit by using v8::TryCatch. v8::TryCatch tryCatch; object->Get(); if (tryCatch.hadException()) { exceptionState.rethrowV8Exception(); return; }
On 2016/06/07 at 00:27:44, haraken wrote:
> > We do check the return value, and the exception is stored in
m_exceptionState to
> > rethrow. I assume v8call() is fine as long as we handle the exceptions
> > appropriately.
>
> Then the current code will work, but it would be better to make the behavior
more explicit by using v8::TryCatch.
>
> v8::TryCatch tryCatch;
> object->Get();
> if (tryCatch.hadException()) {
> exceptionState.rethrowV8Exception();
> return;
> }
Done, thank you for the suggestion.
LGTM
The CQ bit was checked by kojii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominicc@chromium.org Link to the patchset: https://codereview.chromium.org/2024073002/#ps260001 (title: "Add tryCatch")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2024073002/260001
Message was sent while issue was closed.
Committed patchset #12 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Add callbacks to ScriptCustomElementDefinition This patch adds callbacks and observedAttributes to ScriptCustomElementDefinition, as defined in the Element Definition spec[1]. [1] https://html.spec.whatwg.org/multipage/scripting.html#element-definition BUG=594918 TEST=imported/wpt/custom-elements/custom-elements-registry/define.html ========== to ========== Add callbacks to ScriptCustomElementDefinition This patch adds callbacks and observedAttributes to ScriptCustomElementDefinition, as defined in the Element Definition spec[1]. [1] https://html.spec.whatwg.org/multipage/scripting.html#element-definition BUG=594918 TEST=imported/wpt/custom-elements/custom-elements-registry/define.html Committed: https://crrev.com/d17a89371a6d36914685940a28756f275728d9a5 Cr-Commit-Position: refs/heads/master@{#398243} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/d17a89371a6d36914685940a28756f275728d9a5 Cr-Commit-Position: refs/heads/master@{#398243} |
