Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2367543004:
Extended implementation to use interface as arguments (Closed)
DescriptionExtended implementation to use interface as arguments
This CL is connected to Issue 2312093003, which generates bindings for
IDL callback functions. This CL exteneded implementation to be able to
use not only string or long but also interface as arguments of callback
functions.
BUG=569301
Committed: https://crrev.com/f2210407f37d733416f6bc11f955cd0ac29ba43c
Cr-Commit-Position: refs/heads/master@{#420598}
Patch Set 1 #
Total comments: 37
Patch Set 2 : Addressed comments #Patch Set 3 : Addressed comments #
Total comments: 6
Patch Set 4 : Addressed comments #Patch Set 5 : Use generated code for callback function #
Messages
Total messages: 27 (12 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...
https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/dom/idl-callback-function-unittest.html (right): https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/idl-callback-function-unittest.html:20: var div = [document.createElement('div'), document.createElement('div')]; nit: You don't have to use an array. Maybe just following is enough: var div1 = document.createElement('div'); var div2 = document.createElement('div'); https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/idl-callback-function-unittest.html:22: d[0].innerHTML = 'hello'; nit: 4-space indent https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/scripts/v8_callback_function.py (right): https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:34: if argument_type.is_interface_type: why simply do following won't work? if argument.idl_type.is_interface_type: ... if argument.idl_type could be None, you will need to check it first. https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:36: cpp_includes.update(argument.idl_type.includes_for_type(callback_function.extended_attributes)) Could you use idl_type.add_includes_for_type instead of using cpp_includes? for argument in callback_function.arguments: argument.idl_type.add_includes_for_type(callback_function.extended_attributes) https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:39: 'cpp_includes': sorted(cpp_includes), cpp_includes -> includes https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp (right): https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp:30: void CallbackFunctionTest::testInterfaceCallback(ScriptState* scriptState, V8TestInterfaceCallback* callback, HeapVector<Member<HTMLDivElement>>& div, ExceptionState& exceptionState) div -> divElements ? https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/testing/CallbackFunctionTest.idl (right): https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/testing/CallbackFunctionTest.idl:6: [ExperimentalCallbackFunction] callback TestInterfaceCallback = void (HTMLDivElement[] div); div -> divElements? https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/testing/CallbackFunctionTest.idl:10: [CallWith=ScriptState, RaisesException] void testInterfaceCallback(TestInterfaceCallback callback, HTMLDivElement[] div); ditto.
PTAL https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/dom/idl-callback-function-unittest.html (right): https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/idl-callback-function-unittest.html:20: var div = [document.createElement('div'), document.createElement('div')]; On 2016/09/23 02:16:05, bashi1 (slow til Sep 26) wrote: > nit: You don't have to use an array. Maybe just following is enough: > var div1 = document.createElement('div'); > var div2 = document.createElement('div'); I defined testInterfaceCallback using array of interface to be able to address any cases as much as possible, so I need to use array at this point. https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/idl-callback-function-unittest.html:22: d[0].innerHTML = 'hello'; On 2016/09/23 02:16:05, bashi1 (slow til Sep 26) wrote: > nit: 4-space indent Done. https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/scripts/v8_callback_function.py (right): https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:34: if argument_type.is_interface_type: On 2016/09/23 02:16:05, bashi1 (slow til Sep 26) wrote: > why simply do following won't work? > > if argument.idl_type.is_interface_type: > ... > > if argument.idl_type could be None, you will need to check it first. As I defined callback function using array of interface, argument type is IdlArrayType, not IdlType. Therefore, it needs these writings. Do you have any other ideas to address this point? https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:36: cpp_includes.update(argument.idl_type.includes_for_type(callback_function.extended_attributes)) On 2016/09/23 02:16:05, bashi1 (slow til Sep 26) wrote: > Could you use idl_type.add_includes_for_type instead of using cpp_includes? > > for argument in callback_function.arguments: > argument.idl_type.add_includes_for_type(callback_function.extended_attributes) Done. https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:39: 'cpp_includes': sorted(cpp_includes), On 2016/09/23 02:16:05, bashi1 (slow til Sep 26) wrote: > cpp_includes -> includes Done. https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp (right): https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp:30: void CallbackFunctionTest::testInterfaceCallback(ScriptState* scriptState, V8TestInterfaceCallback* callback, HeapVector<Member<HTMLDivElement>>& div, ExceptionState& exceptionState) On 2016/09/23 02:16:05, bashi1 (slow til Sep 26) wrote: > div -> divElements ? Done. https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/testing/CallbackFunctionTest.idl (right): https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/testing/CallbackFunctionTest.idl:6: [ExperimentalCallbackFunction] callback TestInterfaceCallback = void (HTMLDivElement[] div); On 2016/09/23 02:16:05, bashi1 (slow til Sep 26) wrote: > div -> divElements? Done. https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/testing/CallbackFunctionTest.idl:10: [CallWith=ScriptState, RaisesException] void testInterfaceCallback(TestInterfaceCallback callback, HTMLDivElement[] div); On 2016/09/23 02:16:05, bashi1 (slow til Sep 26) wrote: > ditto. Done.
I'm sorry to put comments on an old PS. https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/dom/idl-callback-function-unittest.html (right): https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/idl-callback-function-unittest.html:20: var div = [document.createElement('div'), document.createElement('div')]; On 2016/09/23 02:16:05, bashi1 (slow til Sep 26) wrote: > nit: You don't have to use an array. Maybe just following is enough: > var div1 = document.createElement('div'); > var div2 = document.createElement('div'); +1. Container is an interface, but its elements are also interface. If this test fails, it maybe difficult to find which interface is the root cause or not. The simpler, the better. https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/idl-callback-function-unittest.html:20: var div = [document.createElement('div'), document.createElement('div')]; On 2016/09/23 02:43:41, lkawai wrote: > On 2016/09/23 02:16:05, bashi1 (slow til Sep 26) wrote: > > nit: You don't have to use an array. Maybe just following is enough: > > var div1 = document.createElement('div'); > > var div2 = document.createElement('div'); > > I defined testInterfaceCallback using array of interface to be able to address > any cases as much as possible, so I need to use array at this point. Then I recommend to make two separate tests. One is to take one div element as an argument, and the other takes an array of div elements. https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/idl-callback-function-unittest.html:21: var getDivElement = function(d) { rename this function. it does not meet its behavior. https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/scripts/v8_callback_function.py (right): https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:30: cpp_includes = set(CALLBACK_FUNCTION_CPP_INCLUDES) you can use |includes| instead of this? https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:34: if argument_type.is_interface_type: On 2016/09/23 02:43:42, lkawai wrote: > On 2016/09/23 02:16:05, bashi1 (slow til Sep 26) wrote: > > why simply do following won't work? > > > > if argument.idl_type.is_interface_type: > > ... > > > > if argument.idl_type could be None, you will need to check it first. > > As I defined callback function using array of interface, argument type is > IdlArrayType, not IdlType. Therefore, it needs these writings. Do you have any > other ideas to address this point? I recommend to make a utility function which takes an idl_type, and returns a set of includes. Then you can check if the idl_type is a container and call it recursively to work for nested containers. https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:35: forward_declarations.append('%s' % argument_type.name) do we need "'%s' %"? https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:36: cpp_includes.update(argument.idl_type.includes_for_type(callback_function.extended_attributes)) On 2016/09/23 02:16:05, bashi1 (slow til Sep 26) wrote: > Could you use idl_type.add_includes_for_type instead of using cpp_includes? > > for argument in callback_function.arguments: > argument.idl_type.add_includes_for_type(callback_function.extended_attributes) Not related to this CL, but I feel the name idl_type.add_includes_for_type() is confusing, and it effects on v8_global.includes. I think we need to fix them. https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp (right): https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp:32: ScriptWrappable* scriptWrappable; initialize |scriptWrappable|
https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/dom/idl-callback-function-unittest.html (right): https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/idl-callback-function-unittest.html:20: var div = [document.createElement('div'), document.createElement('div')]; On 2016/09/23 02:43:41, lkawai wrote: > On 2016/09/23 02:16:05, bashi1 (slow til Sep 26) wrote: > > nit: You don't have to use an array. Maybe just following is enough: > > var div1 = document.createElement('div'); > > var div2 = document.createElement('div'); > > I defined testInterfaceCallback using array of interface to be able to address > any cases as much as possible, so I need to use array at this point. This lacks simple use though. You should add following idl callbacks then: void callback(HTMLDivElement div); https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/scripts/v8_callback_function.py (right): https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:34: if argument_type.is_interface_type: On 2016/09/23 02:43:42, lkawai wrote: > On 2016/09/23 02:16:05, bashi1 (slow til Sep 26) wrote: > > why simply do following won't work? > > > > if argument.idl_type.is_interface_type: > > ... > > > > if argument.idl_type could be None, you will need to check it first. > > As I defined callback function using array of interface, argument type is > IdlArrayType, not IdlType. Therefore, it needs these writings. Do you have any > other ideas to address this point? Hmm, I'm not sure why you need to use IdlArrayType here. My understanding is: - callback_function -> an object of IdlCallbackFunction - callback_function.arguments -> a list of IdlArgument - IdlArgument has |idl_type| If above is correct, you can just do: for argument in callback_function.arguments: if argument.idl_type.is_interface: forward_declarations.append(...)
https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/scripts/v8_callback_function.py (right): https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:36: cpp_includes.update(argument.idl_type.includes_for_type(callback_function.extended_attributes)) On 2016/09/23 02:50:05, peria wrote: > On 2016/09/23 02:16:05, bashi1 (slow til Sep 26) wrote: > > Could you use idl_type.add_includes_for_type instead of using cpp_includes? > > > > for argument in callback_function.arguments: > > > argument.idl_type.add_includes_for_type(callback_function.extended_attributes) > > Not related to this CL, but I feel the name idl_type.add_includes_for_type() is > confusing, and it effects on v8_global.includes. I think we need to fix them. Yeah, I totally agree we should avoid using global variables. v8_globals.py exists for historical reason (it was enough at the beginning of the code generator and we haven't been encouraged to keep the code generator "healthy").
The CQ bit was checked by lkawai@google.com to run a CQ dry run
PTAL https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/dom/idl-callback-function-unittest.html (right): https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/idl-callback-function-unittest.html:20: var div = [document.createElement('div'), document.createElement('div')]; On 2016/09/23 02:50:05, peria wrote: > On 2016/09/23 02:43:41, lkawai wrote: > > On 2016/09/23 02:16:05, bashi1 (slow til Sep 26) wrote: > > > nit: You don't have to use an array. Maybe just following is enough: > > > var div1 = document.createElement('div'); > > > var div2 = document.createElement('div'); > > > > I defined testInterfaceCallback using array of interface to be able to address > > any cases as much as possible, so I need to use array at this point. > > Then I recommend to make two separate tests. > One is to take one div element as an argument, and the other takes an array of > div elements. Done. https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/idl-callback-function-unittest.html:21: var getDivElement = function(d) { On 2016/09/23 02:50:05, peria wrote: > rename this function. it does not meet its behavior. Done. https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/scripts/v8_callback_function.py (right): https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:30: cpp_includes = set(CALLBACK_FUNCTION_CPP_INCLUDES) On 2016/09/23 02:50:05, peria wrote: > you can use |includes| instead of this? Done. https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:34: if argument_type.is_interface_type: On 2016/09/23 02:16:05, bashi1 (slow til Sep 26) wrote: > why simply do following won't work? > > if argument.idl_type.is_interface_type: > ... > > if argument.idl_type could be None, you will need to check it first. Done. https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:34: if argument_type.is_interface_type: On 2016/09/23 02:57:56, bashi1 (slow til Sep 26) wrote: > On 2016/09/23 02:43:42, lkawai wrote: > > On 2016/09/23 02:16:05, bashi1 (slow til Sep 26) wrote: > > > why simply do following won't work? > > > > > > if argument.idl_type.is_interface_type: > > > ... > > > > > > if argument.idl_type could be None, you will need to check it first. > > > > As I defined callback function using array of interface, argument type is > > IdlArrayType, not IdlType. Therefore, it needs these writings. Do you have any > > other ideas to address this point? > > Hmm, I'm not sure why you need to use IdlArrayType here. My understanding is: > - callback_function -> an object of IdlCallbackFunction > - callback_function.arguments -> a list of IdlArgument > - IdlArgument has |idl_type| > > If above is correct, you can just do: > > for argument in callback_function.arguments: > if argument.idl_type.is_interface: > forward_declarations.append(...) Done. https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:34: if argument_type.is_interface_type: On 2016/09/23 02:50:05, peria wrote: > On 2016/09/23 02:43:42, lkawai wrote: > > On 2016/09/23 02:16:05, bashi1 (slow til Sep 26) wrote: > > > why simply do following won't work? > > > > > > if argument.idl_type.is_interface_type: > > > ... > > > > > > if argument.idl_type could be None, you will need to check it first. > > > > As I defined callback function using array of interface, argument type is > > IdlArrayType, not IdlType. Therefore, it needs these writings. Do you have any > > other ideas to address this point? > > I recommend to make a utility function which takes an idl_type, and returns a > set of includes. > Then you can check if the idl_type is a container and call it recursively to > work for nested containers. Acknowledged. https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:35: forward_declarations.append('%s' % argument_type.name) On 2016/09/23 02:50:05, peria wrote: > do we need "'%s' %"? Done. https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:36: cpp_includes.update(argument.idl_type.includes_for_type(callback_function.extended_attributes)) On 2016/09/23 02:50:05, peria wrote: > On 2016/09/23 02:16:05, bashi1 (slow til Sep 26) wrote: > > Could you use idl_type.add_includes_for_type instead of using cpp_includes? > > > > for argument in callback_function.arguments: > > > argument.idl_type.add_includes_for_type(callback_function.extended_attributes) > > Not related to this CL, but I feel the name idl_type.add_includes_for_type() is > confusing, and it effects on v8_global.includes. I think we need to fix them. Acknowledged. https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp (right): https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp:32: ScriptWrappable* scriptWrappable; On 2016/09/23 02:50:05, peria wrote: > initialize |scriptWrappable| In the past CL, I got a comment that initialization should be written in the following function. Which is better?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm on my side https://codereview.chromium.org/2367543004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/idl-callback-function-unittest.html (right): https://codereview.chromium.org/2367543004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/idl-callback-function-unittest.html:20: var divElements = document.createElement('div'); divElements -> divElement https://codereview.chromium.org/2367543004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/scripts/v8_callback_function.py (right): https://codereview.chromium.org/2367543004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:38: 'forward_declarations': forward_declarations, sorted(forward_declarations) https://codereview.chromium.org/2367543004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl (right): https://codereview.chromium.org/2367543004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl:37: [ExperimentalCallbackFunction] callback VoidCallbackFunctionInterfaceArg = void (HTMLDivElement divElements); divElements -> divElement
PTAL https://codereview.chromium.org/2367543004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/idl-callback-function-unittest.html (right): https://codereview.chromium.org/2367543004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/idl-callback-function-unittest.html:20: var divElements = document.createElement('div'); On 2016/09/23 05:45:18, bashi1 (slow til Sep 26) wrote: > divElements -> divElement Done. https://codereview.chromium.org/2367543004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/scripts/v8_callback_function.py (right): https://codereview.chromium.org/2367543004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/v8_callback_function.py:38: 'forward_declarations': forward_declarations, On 2016/09/23 05:45:18, bashi1 (slow til Sep 26) wrote: > sorted(forward_declarations) Done. https://codereview.chromium.org/2367543004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl (right): https://codereview.chromium.org/2367543004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl:37: [ExperimentalCallbackFunction] callback VoidCallbackFunctionInterfaceArg = void (HTMLDivElement divElements); On 2016/09/23 05:45:18, bashi1 (slow til Sep 26) wrote: > divElements -> divElement 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...
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 Link to the patchset: https://codereview.chromium.org/2367543004/#ps60001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM.
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Extended implementation to use interface as arguments This CL is connected to Issue 2312093003, which generates bindings for IDL callback functions. This CL exteneded implementation to be able to use not only string or long but also interface as arguments of callback functions. BUG=569301 ========== to ========== Extended implementation to use interface as arguments This CL is connected to Issue 2312093003, which generates bindings for IDL callback functions. This CL exteneded implementation to be able to use not only string or long but also interface as arguments of callback functions. BUG=569301 Committed: https://crrev.com/f2210407f37d733416f6bc11f955cd0ac29ba43c Cr-Commit-Position: refs/heads/master@{#420598} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/f2210407f37d733416f6bc11f955cd0ac29ba43c Cr-Commit-Position: refs/heads/master@{#420598}
Message was sent while issue was closed.
lgtm https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp (right): https://codereview.chromium.org/2367543004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp:32: ScriptWrappable* scriptWrappable; On 2016/09/23 05:35:45, lkawai wrote: > On 2016/09/23 02:50:05, peria wrote: > > initialize |scriptWrappable| > > In the past CL, I got a comment that initialization should be written in the > following function. Which is better? All variables must be initialized at their declarations. Otherwise, when someone updates the code later, they may use uninitialized values. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
