Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2312093003:
Generated bindings for IDL callback functions (Closed)
DescriptionGenerated bindings for IDL callback functions
This CL adds bindings generation for callback functions by adding
templates and implementing code generator. It also adds tests for callback
functions whose arguments and return value are string type.
BUG=569301
Committed: https://crrev.com/692903ec763bdfab525f6a104138508557bb9037
Cr-Commit-Position: refs/heads/master@{#419708}
Patch Set 1 #Patch Set 2 : Changed BUILD.gn #
Total comments: 13
Patch Set 3 : Edited template #
Total comments: 8
Patch Set 4 : Addressed comments #Patch Set 5 : Did rebase-update #
Total comments: 12
Patch Set 6 : Addressed comments #
Total comments: 72
Patch Set 7 : Addressed comments #
Total comments: 37
Patch Set 8 : Addressed comments #Patch Set 9 : Changed handling of void #Patch Set 10 : Addressed win failure #
Total comments: 21
Patch Set 11 : Addressed comments #Messages
Total messages: 82 (50 generated)
lkawai@google.com changed reviewers: + bashi@chromium.org, haraken@chromium.org, peria@chromium.org, yukishiino@chromium.org
PTAL
The CQ bit was checked by lkawai@google.com 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: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
PTAL
The CQ bit was checked by lkawai@google.com 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: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by lkawai@google.com 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...
PTAL
https://codereview.chromium.org/2312093003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h (right): https://codereview.chromium.org/2312093003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:37: X(Test, Callback) \ Are you really using this private property? If not, please remove this line. https://codereview.chromium.org/2312093003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/scripts/v8_types.py (right): https://codereview.chromium.org/2312093003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/v8_types.py:387: return set(['bindings/core/v8/V8TestCallback.h']) You'd like to do something similar to line 372. Avoid hard-coding. https://codereview.chromium.org/2312093003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp (right): https://codereview.chromium.org/2312093003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp:5596: static void voidMethodVoidCallbackFunction2ArgMethod(const v8::FunctionCallbackInfo<v8::Value>& info) Add new expectation files. You must already have gotten V8VoidCallbackFunction2.h and V8VoidCallbackFunction2.cpp. Please add them to this CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
https://codereview.chromium.org/2312093003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/scripts/v8_callback_function.py (right): https://codereview.chromium.org/2312093003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:28: 'bindings/core/v8/ScopedPersistent.h', I think it's fine to start with copy&pasting, but let's start with the minimum things. I think we don't need ActiveDOMCallback.h and DOMWrapperWorld.h for the time being. https://codereview.chromium.org/2312093003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:34: 'wtf/Assertions.h', Ditto. I think we don't need ScriptController.h and V8PrivateProperty.h. Instead we may need V8ScriptRunner.h . https://codereview.chromium.org/2312093003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:53: IdlTypeBase.callback_cpp_type = property(cpp_type) v8_callback_interface.py already updated this property. We shouldn't override it. Please remove this line. Also, we don't need to define cpp_type() in this file. https://codereview.chromium.org/2312093003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/templates/callback_function.h (right): https://codereview.chromium.org/2312093003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/templates/callback_function.h:13: class {{exported}}{{v8_class}} final : public GarbageCollectedFinalized<{{v8_class}}>, public ActiveDOMCallback { Let's start with a simplest version. We don't need to inherit from ActiveDOMCallback. https://codereview.chromium.org/2312093003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/templates/callback_function.h:14: USING_GARBAGE_COLLECTED_MIXIN({{v8_class}}); Let's remove this line. https://codereview.chromium.org/2312093003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/templates/callback_function.h:17: static {{v8_class}}* create(v8::Local<v8::Function> callback, v8::Local<v8::Object> owner, ScriptState* scriptState) We don't need |owner|. https://codereview.chromium.org/2312093003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/templates/callback_function.h:28: ExecutionContext* getExecutionContext() const { return ContextLifecycleObserver::getExecutionContext(); } Let's remove getExecutionContext(). If you need an ExecutionContext* in call() function, then call() function should take an ExecutionContext as an argument. Another option is toExecutionContext(m_scriptState->context()). Anyway, we don't need getExecutionContext() here.
PTAL https://codereview.chromium.org/2312093003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h (right): https://codereview.chromium.org/2312093003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:37: X(Test, Callback) \ On 2016/09/07 08:27:12, Yuki wrote: > Are you really using this private property? > > If not, please remove this line. Acknowledged. https://codereview.chromium.org/2312093003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/scripts/v8_callback_function.py (right): https://codereview.chromium.org/2312093003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:28: 'bindings/core/v8/ScopedPersistent.h', On 2016/09/07 10:04:02, Yuki wrote: > I think it's fine to start with copy&pasting, but let's start with the minimum > things. > > I think we don't need ActiveDOMCallback.h and DOMWrapperWorld.h for the time > being. Done. https://codereview.chromium.org/2312093003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:34: 'wtf/Assertions.h', On 2016/09/07 10:04:02, Yuki wrote: > Ditto. > > I think we don't need ScriptController.h and V8PrivateProperty.h. > > Instead we may need V8ScriptRunner.h . Done. https://codereview.chromium.org/2312093003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:53: IdlTypeBase.callback_cpp_type = property(cpp_type) On 2016/09/07 10:04:02, Yuki wrote: > v8_callback_interface.py already updated this property. > We shouldn't override it. Please remove this line. > > Also, we don't need to define cpp_type() in this file. Done. https://codereview.chromium.org/2312093003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/scripts/v8_types.py (right): https://codereview.chromium.org/2312093003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/v8_types.py:387: return set(['bindings/core/v8/V8TestCallback.h']) On 2016/09/07 08:27:12, Yuki wrote: > You'd like to do something similar to line 372. > Avoid hard-coding. Done. https://codereview.chromium.org/2312093003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp (right): https://codereview.chromium.org/2312093003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp:5596: static void voidMethodVoidCallbackFunction2ArgMethod(const v8::FunctionCallbackInfo<v8::Value>& info) On 2016/09/07 08:27:12, Yuki wrote: > Add new expectation files. You must already have gotten > V8VoidCallbackFunction2.h and V8VoidCallbackFunction2.cpp. Please add them to > this CL. Done. https://codereview.chromium.org/2312093003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/templates/callback_function.h (right): https://codereview.chromium.org/2312093003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/templates/callback_function.h:13: class {{exported}}{{v8_class}} final : public GarbageCollectedFinalized<{{v8_class}}>, public ActiveDOMCallback { On 2016/09/07 10:04:02, Yuki wrote: > Let's start with a simplest version. > We don't need to inherit from ActiveDOMCallback. Done. https://codereview.chromium.org/2312093003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/templates/callback_function.h:14: USING_GARBAGE_COLLECTED_MIXIN({{v8_class}}); On 2016/09/07 10:04:02, Yuki wrote: > Let's remove this line. Done. https://codereview.chromium.org/2312093003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/templates/callback_function.h:17: static {{v8_class}}* create(v8::Local<v8::Function> callback, v8::Local<v8::Object> owner, ScriptState* scriptState) On 2016/09/07 10:04:02, Yuki wrote: > We don't need |owner|. Done. https://codereview.chromium.org/2312093003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/templates/callback_function.h:28: ExecutionContext* getExecutionContext() const { return ContextLifecycleObserver::getExecutionContext(); } On 2016/09/07 10:04:02, Yuki wrote: > Let's remove getExecutionContext(). > > If you need an ExecutionContext* in call() function, then call() function should > take an ExecutionContext as an argument. Another option is > toExecutionContext(m_scriptState->context()). > > Anyway, we don't need getExecutionContext() here. Done.
The CQ bit was checked by lkawai@google.com 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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lkawai@google.com 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...
PTAL https://codereview.chromium.org/2312093003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h (right): https://codereview.chromium.org/2312093003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:37: X(Test, Callback) \ On 2016/09/08 09:11:31, lkawai wrote: > On 2016/09/07 08:27:12, Yuki wrote: > > Are you really using this private property? > > > > If not, please remove this line. > > Acknowledged. Done.
https://codereview.chromium.org/2312093003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/scripts/utilities.py (right): https://codereview.chromium.org/2312093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/utilities.py:185: def callback_functions(self): s/TODO/TODO(lkawai)/ https://codereview.chromium.org/2312093003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/scripts/v8_callback_function.py (right): https://codereview.chromium.org/2312093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:11: # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. The file header comment (of a new file) must be: // Copyright 2016 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. https://codereview.chromium.org/2312093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:15: Extends IdlTypeBase with property |callback_cpp_type|. This line no longer applies. https://codereview.chromium.org/2312093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:26: 'bindings/core/v8/ScriptValue.h', See a below comment. Let's stop using ScriptValue. https://codereview.chromium.org/2312093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:71: argument_declarations = ['ScriptValue thisValue'] I think we'd like to use ScriptWrappable* as |this| object when we calls back the function. Let's use 'ScriptWrappable* scriptWrappable' as the first argument. https://codereview.chromium.org/2312093003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/scripts/v8_types.py (right): https://codereview.chromium.org/2312093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/v8_types.py:387: return set(['bindings/core/v8/V8%s.h' % base_idl_type]) Could you avoid hard-coding of "core"? See line 372 for example.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
Your V8CallbakFunction does not need to take a ScriptState. You should use the
function's context.
v8::Local<v8::Function> function = ...;
ScriptState* scriptState = ScriptState::from(function->CreationContext());
Also I'd recommend not to keep the return value. You'd better return the return
value immediately. You can make call() function take another argument to return
a return value.
bool call(ScriptWrappable* thisObject, Arg1Type arg1, Arg2Type arg2,
ReturnType& returnValue);
On 2016/09/08 12:10:01, Yuki wrote: > Your V8CallbakFunction does not need to take a ScriptState. You should use the > function's context. > v8::Local<v8::Function> function = ...; > ScriptState* scriptState = ScriptState::from(function->CreationContext()); I was wrong. V8CallbackFunction::call() should take a ScriptState. V8CallbackFunction::V8CallbackFunction does not need to take a ScriptState.
The CQ bit was checked by lkawai@google.com 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...
PTAL https://codereview.chromium.org/2312093003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/scripts/utilities.py (right): https://codereview.chromium.org/2312093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/utilities.py:185: def callback_functions(self): On 2016/09/08 10:45:25, Yuki wrote: > s/TODO/TODO(lkawai)/ Done. https://codereview.chromium.org/2312093003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/scripts/v8_callback_function.py (right): https://codereview.chromium.org/2312093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:11: # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. On 2016/09/08 10:45:25, Yuki wrote: > The file header comment (of a new file) must be: > > // Copyright 2016 The Chromium Authors. All rights reserved. > // Use of this source code is governed by a BSD-style license that can be > // found in the LICENSE file. Done. https://codereview.chromium.org/2312093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:15: Extends IdlTypeBase with property |callback_cpp_type|. On 2016/09/08 10:45:25, Yuki wrote: > This line no longer applies. Done. https://codereview.chromium.org/2312093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:26: 'bindings/core/v8/ScriptValue.h', On 2016/09/08 10:45:25, Yuki wrote: > See a below comment. Let's stop using ScriptValue. Done. https://codereview.chromium.org/2312093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:71: argument_declarations = ['ScriptValue thisValue'] On 2016/09/08 10:45:25, Yuki wrote: > I think we'd like to use ScriptWrappable* as |this| object when we calls back > the function. > > Let's use 'ScriptWrappable* scriptWrappable' as the first argument. Done. https://codereview.chromium.org/2312093003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/scripts/v8_types.py (right): https://codereview.chromium.org/2312093003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/v8_types.py:387: return set(['bindings/core/v8/V8%s.h' % base_idl_type]) On 2016/09/08 10:45:25, Yuki wrote: > Could you avoid hard-coding of "core"? > > See line 372 for example. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/dom/idl-callback-function-unittest.html (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/dom/idl-callback-function-unittest.html:6: var test = internals.callbackFunctionTest(); test -> callbackFunctionTest https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/code_generator_v8.py (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/code_generator_v8.py:422: class CodeGeneratorCallbackFunction(object): This class is similar to CodeGeneratorUnionType. It would be better to have a base class for both to reduce duplicated code. That said you don't need to address this comment in this CL. You can do that in a follow-up CL. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/compute_interfaces_info_individual.py (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/compute_interfaces_info_individual.py:221: if 'ExperimentalCallbackFunction' in callback_function.extended_attributes: Please add a comment to describe what 'component_dir' is and why we need to store it. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/v8_callback_function.py (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:32: def member_cpp_type(): Why does this need to be a function? https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:44: 'member_cpp_type': member_cpp_type(), It's not clear what 'member_cpp_type' means here. There is no 'member' in the spec description. http://heycam.github.io/webidl/#dfn-callback-function https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:45: 'v8_value_to_local_cpp_value_dict': idl_type.v8_value_to_local_cpp_value( Also, please consider better naming for |v8_value_to_local_cpp_value_dict| https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:55: 'handle': '%sHandle' % argument.name, What does 'handle' mean here? https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/callback_function.cpp (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/callback_function.cpp:25: {% set return_default = 'return false' %} Remove this and simply use 'return false' where {{return_default}} is used. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/callback_function.cpp:47: {% if rvalue_cpp_type != 'void' %} Did you try generating bindings for a callback of which return type is 'void' ? https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/callback_function.cpp:53: else { return false; } Remove else clause and simply return false. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/tests/results/core/V8VoidCallbackFunction2.cpp (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/results/core/V8VoidCallbackFunction2.cpp:18: m_callback.setPhantom(); It seems we need to check isEmpty() before calling setPhantom() https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/results/core/V8VoidCallbackFunction2.cpp:40: returnValue = cppValue; Do we need this? https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/tests/results/core/V8VoidCallbackFunction2.h (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/results/core/V8VoidCallbackFunction2.h:31: bool call(ScriptState* scriptState, ScriptWrappable* scriptWrappable, Nullable<void*>& returnValue); "Nullable<void*>& returnValue" doesn't look correct to me... https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp:1: // Copyright 2014 The Chromium Authors. All rights reserved. s/2014/2016/ https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp:25: return String("SUCCESS: ") + returnValue; } Place '{' and '}' in a new line. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp:26: else { return String("Error!"); } Place '{' and '}' in a new line. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/testing/CallbackFunctionTest.h (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. s/2014/2016/ https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/testing/CallbackFunctionTest.idl (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.idl:1: // Copyright 2014 The Chromium Authors. All rights reserved. s/2014/2016/ https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/bindings/bindings_tests.py (left): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/bindings/bindings_tests.py:285: Don't remove this empty line
https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/dom/idl-callback-function-unittest.html (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/dom/idl-callback-function-unittest.html:10: assert_equals(test.testDOMString(callback1), 'SUCCESS: hello, world'); I'd recommend to make testDOMString take string arguments so that you don't need to hard-code "hello" or "world". Also I'd recommend to rename this function because you're not testing DOMString, you're testing a callback function. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl:35: [ExperimentalCallbackFunction] callback VoidCallbackFunction2 = void (); I'd recommend to have two types of callback functions. One callback function takes no argument and returns nothing (= void). The other takes some arguments and returns something. So that we can see generated code that handles arguments and return value. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/tests/results/core/V8VoidCallbackFunction2.cpp (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/results/core/V8VoidCallbackFunction2.cpp:18: m_callback.setPhantom(); On 2016/09/13 00:35:06, bashi1 wrote: > It seems we need to check isEmpty() before calling setPhantom() Should have been checked at call sites, but it's a good practice to put a DCHECK(). https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/results/core/V8VoidCallbackFunction2.cpp:29: I thought that you had ScriptState::Scope scope(scriptState); Did you remove that? We'd need it.
please update the description to say what you did and not how you did. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/utilities.py (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/utilities.py:186: # TODO(lkawai): Currrently modules can not use callback functions defined in core. This comment shows an issue which should be fixed. Please write what you need to do in TODO comments. e.g. make callback functions defined in core/ be usable in modules/. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/callback_function.cpp (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/callback_function.cpp:13: {{v8_class}}::{{v8_class}}(v8::Local<v8::Function> callback, ScriptState* scriptState) [style] redundant space after ',' https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/callback_function.cpp:39: v8::Local<v8::Value> *argv = 0; [style] s/0/nullptr/ https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/callback_function.h (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/callback_function.h:18: static {{v8_class}}* create(v8::Local<v8::Function> callback, ScriptState* scriptState) if you don't need scriptState itself, please use "v8::Isolate* isolate" instead. [style] Plus, move scriptState or isolate in the first argument. http://crbug.com/424446 https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/callback_function.h:23: {{v8_class}}(v8::Local<v8::Function>, ScriptState*); If you have a create() method, I recommend to keep the constructor in private. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl:35: [ExperimentalCallbackFunction] callback VoidCallbackFunction2 = void (); could you rename this type more descriptive? e.g. VoidExperimentalCallbackFunction, ExperimentalClosure https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl:327: void voidMethodVoidCallbackFunction2Arg(VoidCallbackFunction2 voidCallbackFunction2Arg); ditto. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/tests/results/core/V8VoidCallbackFunction2.h (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/results/core/V8VoidCallbackFunction2.h:31: bool call(ScriptState* scriptState, ScriptWrappable* scriptWrappable, Nullable<void*>& returnValue); On 2016/09/13 00:35:06, bashi1 wrote: > "Nullable<void*>& returnValue" doesn't look correct to me... JFYI: this code is a generated one, so if it is wrong, you need to update the template file. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp:25: return String("SUCCESS: ") + returnValue; } On 2016/09/13 00:35:07, bashi1 wrote: > Place '{' and '}' in a new line. [style] Not correct. Keep '{' as-is. if (...) { ....; } else { ....; } https://www.chromium.org/blink/coding-style https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp:26: else { return String("Error!"); } [style] In this case, you can remove 'else {' and '}' if (...) { return ...; } return ...; https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/testing/CallbackFunctionTest.idl (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.idl:5: [ExperimentalCallbackFunction]callback TestCallback = DOMString (DOMString str1, DOMString str2); I'm not familiar with style guide of IDLs, but I think you need a space between options '[...]' and the keyword 'callback'. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.idl:8: [CallWith=ScriptState, RaisesException] DOMString testDOMString(TestCallback callback); please rename this method to show what is tested in. (see Shiino-san's comment in idl-callback-function-unittest.html)
The CQ bit was checked by lkawai@google.com 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...
PTAL https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/dom/idl-callback-function-unittest.html (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/dom/idl-callback-function-unittest.html:6: var test = internals.callbackFunctionTest(); On 2016/09/13 00:35:05, bashi1 wrote: > test -> callbackFunctionTest Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/dom/idl-callback-function-unittest.html:10: assert_equals(test.testDOMString(callback1), 'SUCCESS: hello, world'); On 2016/09/14 11:57:22, Yuki wrote: > I'd recommend to make testDOMString take string arguments so that you don't need > to hard-code "hello" or "world". > > Also I'd recommend to rename this function because you're not testing DOMString, > you're testing a callback function. Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/code_generator_v8.py (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/code_generator_v8.py:422: class CodeGeneratorCallbackFunction(object): On 2016/09/13 00:35:05, bashi1 wrote: > This class is similar to CodeGeneratorUnionType. It would be better to have a > base class for both to reduce duplicated code. > > That said you don't need to address this comment in this CL. You can do that in > a follow-up CL. Acknowledged. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/compute_interfaces_info_individual.py (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/compute_interfaces_info_individual.py:221: if 'ExperimentalCallbackFunction' in callback_function.extended_attributes: On 2016/09/13 00:35:05, bashi1 wrote: > Please add a comment to describe what 'component_dir' is and why we need to > store it. Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/utilities.py (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/utilities.py:186: # TODO(lkawai): Currrently modules can not use callback functions defined in core. On 2016/09/15 01:14:32, peria wrote: > This comment shows an issue which should be fixed. > Please write what you need to do in TODO comments. > e.g. make callback functions defined in core/ be usable in modules/. Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/v8_callback_function.py (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:32: def member_cpp_type(): On 2016/09/13 00:35:06, bashi1 wrote: > Why does this need to be a function? Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:44: 'member_cpp_type': member_cpp_type(), On 2016/09/13 00:35:06, bashi1 wrote: > It's not clear what 'member_cpp_type' means here. There is no 'member' in the > spec description. > > http://heycam.github.io/webidl/#dfn-callback-function Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:45: 'v8_value_to_local_cpp_value_dict': idl_type.v8_value_to_local_cpp_value( On 2016/09/13 00:35:06, bashi1 wrote: > Also, please consider better naming for |v8_value_to_local_cpp_value_dict| Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:55: 'handle': '%sHandle' % argument.name, On 2016/09/13 00:35:06, bashi1 wrote: > What does 'handle' mean here? Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/callback_function.cpp (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/callback_function.cpp:13: {{v8_class}}::{{v8_class}}(v8::Local<v8::Function> callback, ScriptState* scriptState) On 2016/09/15 01:14:32, peria wrote: > [style] redundant space after ',' Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/callback_function.cpp:25: {% set return_default = 'return false' %} On 2016/09/13 00:35:06, bashi1 wrote: > Remove this and simply use 'return false' where {{return_default}} is used. Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/callback_function.cpp:39: v8::Local<v8::Value> *argv = 0; On 2016/09/15 01:14:32, peria wrote: > [style] s/0/nullptr/ Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/callback_function.cpp:47: {% if rvalue_cpp_type != 'void' %} On 2016/09/13 00:35:06, bashi1 wrote: > Did you try generating bindings for a callback of which return type is 'void' ? Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/callback_function.cpp:53: else { return false; } On 2016/09/13 00:35:06, bashi1 wrote: > Remove else clause and simply return false. Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/callback_function.h (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/callback_function.h:18: static {{v8_class}}* create(v8::Local<v8::Function> callback, ScriptState* scriptState) On 2016/09/15 01:14:32, peria wrote: > if you don't need scriptState itself, please use "v8::Isolate* isolate" instead. > [style] Plus, move scriptState or isolate in the first argument. > http://crbug.com/424446 Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/callback_function.h:23: {{v8_class}}(v8::Local<v8::Function>, ScriptState*); On 2016/09/15 01:14:32, peria wrote: > If you have a create() method, I recommend to keep the constructor in private. Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl:35: [ExperimentalCallbackFunction] callback VoidCallbackFunction2 = void (); On 2016/09/15 01:14:32, peria wrote: > could you rename this type more descriptive? > e.g. VoidExperimentalCallbackFunction, ExperimentalClosure Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl:35: [ExperimentalCallbackFunction] callback VoidCallbackFunction2 = void (); On 2016/09/15 01:14:32, peria wrote: > could you rename this type more descriptive? > e.g. VoidExperimentalCallbackFunction, ExperimentalClosure Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl:35: [ExperimentalCallbackFunction] callback VoidCallbackFunction2 = void (); On 2016/09/14 11:57:22, Yuki wrote: > I'd recommend to have two types of callback functions. > One callback function takes no argument and returns nothing (= void). > The other takes some arguments and returns something. > So that we can see generated code that handles arguments and return value. Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl:327: void voidMethodVoidCallbackFunction2Arg(VoidCallbackFunction2 voidCallbackFunction2Arg); On 2016/09/15 01:14:32, peria wrote: > ditto. Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/tests/results/core/V8VoidCallbackFunction2.cpp (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/results/core/V8VoidCallbackFunction2.cpp:18: m_callback.setPhantom(); On 2016/09/13 00:35:06, bashi1 wrote: > It seems we need to check isEmpty() before calling setPhantom() Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/results/core/V8VoidCallbackFunction2.cpp:18: m_callback.setPhantom(); On 2016/09/14 11:57:23, Yuki wrote: > On 2016/09/13 00:35:06, bashi1 wrote: > > It seems we need to check isEmpty() before calling setPhantom() > > Should have been checked at call sites, but it's a good practice to put a > DCHECK(). Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/results/core/V8VoidCallbackFunction2.cpp:29: On 2016/09/14 11:57:23, Yuki wrote: > I thought that you had > ScriptState::Scope scope(scriptState); > Did you remove that? We'd need it. Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/results/core/V8VoidCallbackFunction2.cpp:40: returnValue = cppValue; On 2016/09/13 00:35:06, bashi1 wrote: > Do we need this? Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/tests/results/core/V8VoidCallbackFunction2.h (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/results/core/V8VoidCallbackFunction2.h:31: bool call(ScriptState* scriptState, ScriptWrappable* scriptWrappable, Nullable<void*>& returnValue); On 2016/09/15 01:14:32, peria wrote: > On 2016/09/13 00:35:06, bashi1 wrote: > > "Nullable<void*>& returnValue" doesn't look correct to me... > > JFYI: this code is a generated one, so if it is wrong, you need to update the > template file. Acknowledged. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/results/core/V8VoidCallbackFunction2.h:31: bool call(ScriptState* scriptState, ScriptWrappable* scriptWrappable, Nullable<void*>& returnValue); On 2016/09/13 00:35:06, bashi1 wrote: > "Nullable<void*>& returnValue" doesn't look correct to me... Acknowledged. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/results/core/V8VoidCallbackFunction2.h:31: bool call(ScriptState* scriptState, ScriptWrappable* scriptWrappable, Nullable<void*>& returnValue); On 2016/09/13 00:35:06, bashi1 wrote: > "Nullable<void*>& returnValue" doesn't look correct to me... Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/09/13 00:35:06, bashi1 wrote: > s/2014/2016/ Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp:25: return String("SUCCESS: ") + returnValue; } On 2016/09/13 00:35:07, bashi1 wrote: > Place '{' and '}' in a new line. Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp:25: return String("SUCCESS: ") + returnValue; } On 2016/09/15 01:14:33, peria wrote: > On 2016/09/13 00:35:07, bashi1 wrote: > > Place '{' and '}' in a new line. > > [style] > Not correct. Keep '{' as-is. > > if (...) { > ....; > } else { > ....; > } > > https://www.chromium.org/blink/coding-style Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp:26: else { return String("Error!"); } On 2016/09/13 00:35:06, bashi1 wrote: > Place '{' and '}' in a new line. Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp:26: else { return String("Error!"); } On 2016/09/15 01:14:33, peria wrote: > [style] In this case, you can remove 'else {' and '}' > > if (...) { > return ...; > } > return ...; Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/testing/CallbackFunctionTest.h (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/09/13 00:35:07, bashi1 wrote: > s/2014/2016/ Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/testing/CallbackFunctionTest.idl (right): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.idl:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/09/13 00:35:07, bashi1 wrote: > s/2014/2016/ Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.idl:5: [ExperimentalCallbackFunction]callback TestCallback = DOMString (DOMString str1, DOMString str2); On 2016/09/15 01:14:33, peria wrote: > I'm not familiar with style guide of IDLs, but I think you need a space > between options '[...]' and the keyword 'callback'. Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.idl:8: [CallWith=ScriptState, RaisesException] DOMString testDOMString(TestCallback callback); On 2016/09/15 01:14:33, peria wrote: > please rename this method to show what is tested in. > (see Shiino-san's comment in idl-callback-function-unittest.html) Done. https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/bindings/bindings_tests.py (left): https://codereview.chromium.org/2312093003/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/bindings/bindings_tests.py:285: On 2016/09/13 00:35:07, bashi1 wrote: > Don't remove this empty line Done.
looks much better than past PSs. https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/BUILD.gn (right): https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BUILD.gn:206: # bindings_core_generated_aggregate_files = Drop these comment-out lines https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/utilities.py (right): https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/utilities.py:186: # TODO(lkawai): Make callback functions defined in core/ Be usable in modules/. s/Be/be/ https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/v8_callback_function.py (right): https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:37: 'return_cpp_value': idl_type.v8_value_to_local_cpp_value( This entry has not a value but a function, so I prefer to name the key more descriptive like 'return_value_converter'. https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:57: if return_cpp_type != 'void': |return_cpp_type| has a suffix '&' as you defined on line #35, so this condition must be always true. You can find 'returnValue' in the argument of V8VoidExperimentalCallbackFunction. https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/callback_function.h (right): https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/callback_function.h:15: class {{exported}}{{v8_class}} final : public GarbageCollectedFinalized<{{v8_class}}>{ [style] put a space before '{' https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/callback_function.h:20: return new {{v8_class}}(isolate, callback); [style] 4 space indent https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/callback_function.h:25: DECLARE_VIRTUAL_TRACE(); DECLARE_TRACE(); JFYI: This class is a subclass of GCF<> and not expected to be inherited. In this case, we don't have to make trace() methods virtual. https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/testing/CallbackFunctionTest.h (right): https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.h:26: String testCallback(ScriptState*, V8TestCallback*, String, String, ExceptionState&); Use constant reference type 'const String&' for input strings in arguments.
https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/BUILD.gn (right): https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BUILD.gn:205: #if (is_win && is_official_build) { Please remove this. https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/v8_callback_function.py (right): https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:32: 'v8_class': v8_utilities.v8_class_name(callback_function), nit: alphabetical order https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:47: 'handle_name': '%sHandle' % argument.name, nit: alphabetical order https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/tests/results/core/V8LongExperimentalCallbackFunction.cpp (right): https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/results/core/V8LongExperimentalCallbackFunction.cpp:9: #include "wtf/Assertions.h" alphabetical order. (You will need to sort() includes in .py) https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/results/core/V8LongExperimentalCallbackFunction.cpp:19: m_callback.setPhantom(); As chatted offline, just calling setPhantom() wouldn't work. Please do one of followings: 1. Remove setPhantom() call. Add TODO comments which described the current implementation could cause leaks 2. Make m_callback be reachable via WrapperVisitor ? (shiino-san?) 3. Pass the owner (v8 object) of this callback and store m_callback in the owner as a private property https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/results/core/V8LongExperimentalCallbackFunction.cpp:37: v8::Local<v8::Value> argv[] = { num1Handle, num2Handle }; Handle -> Argument https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/results/core/V8LongExperimentalCallbackFunction.cpp:45: if (exceptionState.hadException()) wrong indent https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/tests/results/core/V8LongExperimentalCallbackFunction.h (right): https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/results/core/V8LongExperimentalCallbackFunction.h:37: } // namespace blink nit: Add one empty line (in the template) https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/tests/results/core/V8VoidExperimentalCallbackFunction.h (right): https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/results/core/V8VoidExperimentalCallbackFunction.h:30: bool call(ScriptState* scriptState, ScriptWrappable* scriptWrappable, void*& returnValue); "void*& returnValue" doesn't look correct to me. https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/testing/CallbackFunctionTest.idl (right): https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.idl:8: [CallWith=ScriptState, RaisesException] DOMString testCallback(TestCallback callback, DOMString msg1, DOMString msg2); 4-space indent https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/bindings/bindings_tests.py (right): https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/bindings/bindings_tests.py:304: Remove this empty line
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by lkawai@google.com 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...
PTAL https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/BUILD.gn (right): https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BUILD.gn:205: #if (is_win && is_official_build) { On 2016/09/16 05:51:18, bashi1 wrote: > Please remove this. Done. https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BUILD.gn:206: # bindings_core_generated_aggregate_files = On 2016/09/16 05:48:18, peria wrote: > Drop these comment-out lines Done. https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/utilities.py (right): https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/utilities.py:186: # TODO(lkawai): Make callback functions defined in core/ Be usable in modules/. On 2016/09/16 05:48:18, peria wrote: > s/Be/be/ Done. https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/v8_callback_function.py (right): https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:32: 'v8_class': v8_utilities.v8_class_name(callback_function), On 2016/09/16 05:51:19, bashi1 wrote: > nit: alphabetical order Done. https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:37: 'return_cpp_value': idl_type.v8_value_to_local_cpp_value( On 2016/09/16 05:48:18, peria wrote: > This entry has not a value but a function, so I prefer to name the key more > descriptive like 'return_value_converter'. Done. https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:47: 'handle_name': '%sHandle' % argument.name, On 2016/09/16 05:51:19, bashi1 wrote: > nit: alphabetical order Done. https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:57: if return_cpp_type != 'void': On 2016/09/16 05:48:18, peria wrote: > |return_cpp_type| has a suffix '&' as you defined on line #35, > so this condition must be always true. > You can find 'returnValue' in the argument of > V8VoidExperimentalCallbackFunction. Done. https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/callback_function.h (right): https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/callback_function.h:15: class {{exported}}{{v8_class}} final : public GarbageCollectedFinalized<{{v8_class}}>{ On 2016/09/16 05:48:18, peria wrote: > [style] put a space before '{' Done. https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/callback_function.h:20: return new {{v8_class}}(isolate, callback); On 2016/09/16 05:48:18, peria wrote: > [style] 4 space indent Done. https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/callback_function.h:25: DECLARE_VIRTUAL_TRACE(); On 2016/09/16 05:48:18, peria wrote: > DECLARE_TRACE(); > > JFYI: > This class is a subclass of GCF<> and not expected to be inherited. > In this case, we don't have to make trace() methods virtual. Done. https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/tests/results/core/V8LongExperimentalCallbackFunction.cpp (right): https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/results/core/V8LongExperimentalCallbackFunction.cpp:9: #include "wtf/Assertions.h" On 2016/09/16 05:51:19, bashi1 wrote: > alphabetical order. > > (You will need to sort() includes in .py) Done. https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/results/core/V8LongExperimentalCallbackFunction.cpp:37: v8::Local<v8::Value> argv[] = { num1Handle, num2Handle }; On 2016/09/16 05:51:19, bashi1 wrote: > Handle -> Argument Done. https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/results/core/V8LongExperimentalCallbackFunction.cpp:45: if (exceptionState.hadException()) On 2016/09/16 05:51:19, bashi1 wrote: > wrong indent Done. https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/tests/results/core/V8LongExperimentalCallbackFunction.h (right): https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/results/core/V8LongExperimentalCallbackFunction.h:37: } // namespace blink On 2016/09/16 05:51:19, bashi1 wrote: > nit: Add one empty line (in the template) Done. https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/tests/results/core/V8VoidExperimentalCallbackFunction.h (right): https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/results/core/V8VoidExperimentalCallbackFunction.h:30: bool call(ScriptState* scriptState, ScriptWrappable* scriptWrappable, void*& returnValue); On 2016/09/16 05:51:19, bashi1 wrote: > "void*& returnValue" doesn't look correct to me. Done. https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/testing/CallbackFunctionTest.h (right): https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.h:26: String testCallback(ScriptState*, V8TestCallback*, String, String, ExceptionState&); On 2016/09/16 05:48:18, peria wrote: > Use constant reference type 'const String&' for input strings in arguments. Done. https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/testing/CallbackFunctionTest.idl (right): https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.idl:8: [CallWith=ScriptState, RaisesException] DOMString testCallback(TestCallback callback, DOMString msg1, DOMString msg2); On 2016/09/16 05:51:19, bashi1 wrote: > 4-space indent Done. https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/bindings/bindings_tests.py (right): https://codereview.chromium.org/2312093003/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/bindings/bindings_tests.py:304: On 2016/09/16 05:51:19, bashi1 wrote: > Remove this empty line Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
PTAL
The CQ bit was checked by lkawai@google.com 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...
LGTM! But we need to somehow fix or work around the build breakage on Windows. ;)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
LGTM! Thank you for being patient for reviews. Great work! Let's address windows build failures next week.
PTAL
The CQ bit was checked by lkawai@google.com 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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/idl_compiler.py (right): https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/idl_compiler.py:41: from code_generator_v8 import CodeGeneratorDictionaryImpl, CodeGeneratorV8, CodeGeneratorUnionType, CodeGeneratorCallbackFunction # pylint: disable=W0403 Pylint warns this line because it was edited in this CL. It means other lines (#42 and #43) can be also warned. So I recommend to do either of followings. 1) Put "pylint: disable=W0403" to all lines in this file. 2) Disable the pylint for this file. c.f. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/scrip... 3) ignore as you did with --bypass-hooks
On 2016/09/20 02:47:07, peria wrote: > https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... > File third_party/WebKit/Source/bindings/scripts/idl_compiler.py (right): > > https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/scripts/idl_compiler.py:41: from > code_generator_v8 import CodeGeneratorDictionaryImpl, CodeGeneratorV8, > CodeGeneratorUnionType, CodeGeneratorCallbackFunction # pylint: disable=W0403 > Pylint warns this line because it was edited in this CL. It means other lines > (#42 and #43) can be also warned. > So I recommend to do either of followings. > > 1) Put "pylint: disable=W0403" to all lines in this file. > 2) Disable the pylint for this file. c.f. > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/scrip... > 3) ignore as you did with --bypass-hooks Hmm, I'd prefer not to do 1 and 3 because: - we should avoid using bypass-hooks - This CL is not for fixing import problems. I don't want to add more changed lines which are not essentially related. Rather than disabling warnings I think we should stop using relative imports, but that's a separate issue.
On 2016/09/20 03:08:50, bashi1 (slow til Sep 26) wrote: > On 2016/09/20 02:47:07, peria wrote: > > > https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/bindings/scripts/idl_compiler.py (right): > > > > > https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... > > third_party/WebKit/Source/bindings/scripts/idl_compiler.py:41: from > > code_generator_v8 import CodeGeneratorDictionaryImpl, CodeGeneratorV8, > > CodeGeneratorUnionType, CodeGeneratorCallbackFunction # pylint: disable=W0403 > > Pylint warns this line because it was edited in this CL. It means other lines > > (#42 and #43) can be also warned. > > So I recommend to do either of followings. > > > > 1) Put "pylint: disable=W0403" to all lines in this file. > > 2) Disable the pylint for this file. c.f. > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/scrip... > > 3) ignore as you did with --bypass-hooks > > Hmm, I'd prefer not to do 1 and 3 because: > - we should avoid using bypass-hooks > - This CL is not for fixing import problems. I don't want to add more changed > lines which are not essentially related. > > Rather than disabling warnings I think we should stop using relative imports, > but that's a separate issue. Yes, Bashi-san is correct. The most preferable direction is not to use "pylint: disable" or "--bypass-hooks". Please go ahead and fix it in another CL.
Sorry about the review delay. LGTM with nits. https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/utilities.py (right): https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/utilities.py:186: # TODO(lkawai): Make callback functions defined in core/ be usable in modules/. I'm just curious but can't we fix the TODO just by returning self._component_info_modules['callback_functions'] + self._component_info['callback_functions'] ? https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/tests/results/core/V8VoidExperimentalCallbackFunction.cpp (right): https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/results/core/V8VoidExperimentalCallbackFunction.cpp:32: if (m_callback.isEmpty()) I'd move this check to above line 31. If m_callback is empty, we don't need to enter the ScriptState::Scope. https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/BUILD.gn (right): https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/BUILD.gn:212: #maybe not correct Remove this. https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp (right): https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp:17: String CallbackFunctionTest::testCallback(ScriptState* scriptState, V8TestCallback* callback, const String& msg1, const String& msg2, ExceptionState& exceptionState) msg => message Blink prefers a fully qualified name. https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp:19: ScriptWrappable* scriptWrappable = nullptr; This variable is redundant. You can directly pass in nullptr to callback->call. https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/testing/CallbackFunctionTest.h (right): https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.h:18: class CallbackFunctionTest final : public GarbageCollectedFinalized<CallbackFunctionTest>, public ScriptWrappable { You can use GarbageCollected<CallbackFunctionTest>. Then you can remove the destructor. https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.h:24: virtual ~CallbackFunctionTest() = default; You can remove this. https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/testing/CallbackFunctionTest.idl (right): https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.idl:5: [ExperimentalCallbackFunction] callback TestCallback = DOMString (DOMString msg1, DOMString msg2); msg => message https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.idl:8: [CallWith=ScriptState, RaisesException] DOMString testCallback(TestCallback callback, DOMString msg1, DOMString msg2); msg => message
https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/utilities.py (right): https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/utilities.py:186: # TODO(lkawai): Make callback functions defined in core/ be usable in modules/. On 2016/09/20 07:08:21, haraken wrote: > > I'm just curious but can't we fix the TODO just by returning > self._component_info_modules['callback_functions'] + > self._component_info['callback_functions'] ? Sorry for lack of explanations. IIUC if we do that, we will create .cpp/.h under both bindings/core/v8 and bindings/modules/v8 when a callback is defined in core/. Lisa-san, correct me if I'm wrong (I don't remember the exact reason).
The CQ bit was checked by lkawai@google.com 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...
PTAL https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/idl_compiler.py (right): https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/idl_compiler.py:41: from code_generator_v8 import CodeGeneratorDictionaryImpl, CodeGeneratorV8, CodeGeneratorUnionType, CodeGeneratorCallbackFunction # pylint: disable=W0403 On 2016/09/20 02:47:06, peria wrote: > Pylint warns this line because it was edited in this CL. It means other lines > (#42 and #43) can be also warned. > So I recommend to do either of followings. > > 1) Put "pylint: disable=W0403" to all lines in this file. > 2) Disable the pylint for this file. c.f. > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/scrip... > 3) ignore as you did with --bypass-hooks Acknowledged. https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/utilities.py (right): https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/utilities.py:186: # TODO(lkawai): Make callback functions defined in core/ be usable in modules/. On 2016/09/20 07:08:21, haraken wrote: > > I'm just curious but can't we fix the TODO just by returning > self._component_info_modules['callback_functions'] + > self._component_info['callback_functions'] ? Acknowledged. https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/tests/results/core/V8VoidExperimentalCallbackFunction.cpp (right): https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/results/core/V8VoidExperimentalCallbackFunction.cpp:32: if (m_callback.isEmpty()) On 2016/09/20 07:08:22, haraken wrote: > > I'd move this check to above line 31. If m_callback is empty, we don't need to > enter the ScriptState::Scope. Done. https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/BUILD.gn (right): https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/BUILD.gn:212: #maybe not correct On 2016/09/20 07:08:22, haraken wrote: > > Remove this. Done. https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp (right): https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp:17: String CallbackFunctionTest::testCallback(ScriptState* scriptState, V8TestCallback* callback, const String& msg1, const String& msg2, ExceptionState& exceptionState) On 2016/09/20 07:08:22, haraken wrote: > > msg => message > > Blink prefers a fully qualified name. Done. https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp:19: ScriptWrappable* scriptWrappable = nullptr; On 2016/09/20 07:08:22, haraken wrote: > > This variable is redundant. You can directly pass in nullptr to callback->call. > Done. https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/testing/CallbackFunctionTest.h (right): https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.h:18: class CallbackFunctionTest final : public GarbageCollectedFinalized<CallbackFunctionTest>, public ScriptWrappable { On 2016/09/20 07:08:22, haraken wrote: > > You can use GarbageCollected<CallbackFunctionTest>. Then you can remove the > destructor. Done. https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.h:24: virtual ~CallbackFunctionTest() = default; On 2016/09/20 07:08:22, haraken wrote: > > You can remove this. Done. https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/testing/CallbackFunctionTest.idl (right): https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.idl:5: [ExperimentalCallbackFunction] callback TestCallback = DOMString (DOMString msg1, DOMString msg2); On 2016/09/20 07:08:22, haraken wrote: > > msg => message Done. https://codereview.chromium.org/2312093003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/CallbackFunctionTest.idl:8: [CallWith=ScriptState, RaisesException] DOMString testCallback(TestCallback callback, DOMString msg1, DOMString msg2); On 2016/09/20 07:08:22, haraken wrote: > > msg => message Done.
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lkawai@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bashi@chromium.org, yukishiino@chromium.org Link to the patchset: https://codereview.chromium.org/2312093003/#ps200001 (title: "Addressed 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.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== generated bindings for IDL callback functions This CL adds bindings generation for callback functions by adding templates and implementing code generator. It also adds tests for callback functions whose arguments and return value are string type. BUG=569301 ========== to ========== generated bindings for IDL callback functions This CL adds bindings generation for callback functions by adding templates and implementing code generator. It also adds tests for callback functions whose arguments and return value are string type. BUG=569301 Committed: https://crrev.com/692903ec763bdfab525f6a104138508557bb9037 Cr-Commit-Position: refs/heads/master@{#419708} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/692903ec763bdfab525f6a104138508557bb9037 Cr-Commit-Position: refs/heads/master@{#419708}
Message was sent while issue was closed.
Description was changed from ========== generated bindings for IDL callback functions This CL adds bindings generation for callback functions by adding templates and implementing code generator. It also adds tests for callback functions whose arguments and return value are string type. BUG=569301 Committed: https://crrev.com/692903ec763bdfab525f6a104138508557bb9037 Cr-Commit-Position: refs/heads/master@{#419708} ========== to ========== generated bindings for IDL callback functions This CL adds bindings generation for callback functions by adding templates and implementing code generator. It also adds tests for callback functions whose arguments and return value are string type. BUG=569301 Committed: https://crrev.com/692903ec763bdfab525f6a104138508557bb9037 Cr-Commit-Position: refs/heads/master@{#419708} ==========
Message was sent while issue was closed.
Description was changed from ========== generated bindings for IDL callback functions This CL adds bindings generation for callback functions by adding templates and implementing code generator. It also adds tests for callback functions whose arguments and return value are string type. BUG=569301 Committed: https://crrev.com/692903ec763bdfab525f6a104138508557bb9037 Cr-Commit-Position: refs/heads/master@{#419708} ========== to ========== Generated bindings for IDL callback functions This CL adds bindings generation for callback functions by adding templates and implementing code generator. It also adds tests for callback functions whose arguments and return value are string type. BUG=569301 Committed: https://crrev.com/692903ec763bdfab525f6a104138508557bb9037 Cr-Commit-Position: refs/heads/master@{#419708} ========== |
