Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2588303002:
bindings: Don't wrap nullable callback functions in Nullable<T> (Closed)
Descriptionbindings: Don't wrap nullable callback functions in Nullable<T>
The code generator wrongly uses `Nullable<Callback*>` when nullable
callback functions are used like below:
callback C = void();
interface I { void f(C? callback); }
Since C++ impl of callback functions are garbage collected types,
we can just use `Callback*`.
BUG=675859
Committed: https://crrev.com/ab4bb3dbcec83cc7220b011b5e313eebc260a7dc
Cr-Commit-Position: refs/heads/master@{#439816}
Patch Set 1 #
Total comments: 2
Patch Set 2 : comments #Patch Set 3 : rebase #
Total comments: 2
Messages
Total messages: 24 (12 generated)
The CQ bit was checked by bashi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bashi@chromium.org changed reviewers: + haraken@chromium.org, peria@chromium.org, yukishiino@chromium.org
PTAL
lgtm
LGTM. https://codereview.chromium.org/2588303002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/scripts/v8_types.py (right): https://codereview.chromium.org/2588303002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/v8_types.py:972: or idl_type.is_custom_callback_function or idl_type.is_callback_interface) nit: You may want to sort things alphabetically, or group things like is_callback_function is_custom_callback_function is_callback_interface ?
https://codereview.chromium.org/2588303002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/scripts/v8_types.py (right): https://codereview.chromium.org/2588303002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/v8_types.py:972: or idl_type.is_custom_callback_function or idl_type.is_callback_interface) On 2016/12/20 07:03:30, Yuki wrote: > nit: You may want to sort things alphabetically, or group things like > is_callback_function > is_custom_callback_function > is_callback_interface > ? Done (grouped)
The CQ bit was checked by bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peria@chromium.org, yukishiino@chromium.org Link to the patchset: https://codereview.chromium.org/2588303002/#ps20001 (title: "comments")
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
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
LGTM
The CQ bit was checked by bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, peria@chromium.org, yukishiino@chromium.org Link to the patchset: https://codereview.chromium.org/2588303002/#ps40001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482242321812890,
"parent_rev": "27545f19fdcaae60e886dea47147d446ff60594c", "commit_rev":
"800c2397aea1e533d70092bfea6f499d1f12e3ce"}
Message was sent while issue was closed.
Description was changed from
==========
bindings: Don't wrap nullable callback functions in Nullable<T>
The code generator wrongly uses `Nullable<Callback*>` when nullable
callback functions are used like below:
callback C = void();
interface I { void f(C? callback); }
Since C++ impl of callback functions are garbage collected types,
we can just use `Callback*`.
BUG=675859
==========
to
==========
bindings: Don't wrap nullable callback functions in Nullable<T>
The code generator wrongly uses `Nullable<Callback*>` when nullable
callback functions are used like below:
callback C = void();
interface I { void f(C? callback); }
Since C++ impl of callback functions are garbage collected types,
we can just use `Callback*`.
BUG=675859
Review-Url: https://codereview.chromium.org/2588303002
==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from
==========
bindings: Don't wrap nullable callback functions in Nullable<T>
The code generator wrongly uses `Nullable<Callback*>` when nullable
callback functions are used like below:
callback C = void();
interface I { void f(C? callback); }
Since C++ impl of callback functions are garbage collected types,
we can just use `Callback*`.
BUG=675859
Review-Url: https://codereview.chromium.org/2588303002
==========
to
==========
bindings: Don't wrap nullable callback functions in Nullable<T>
The code generator wrongly uses `Nullable<Callback*>` when nullable
callback functions are used like below:
callback C = void();
interface I { void f(C? callback); }
Since C++ impl of callback functions are garbage collected types,
we can just use `Callback*`.
BUG=675859
Committed: https://crrev.com/ab4bb3dbcec83cc7220b011b5e313eebc260a7dc
Cr-Commit-Position: refs/heads/master@{#439816}
==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/ab4bb3dbcec83cc7220b011b5e313eebc260a7dc Cr-Commit-Position: refs/heads/master@{#439816}
Message was sent while issue was closed.
zqzhang@chromium.org changed reviewers: + zqzhang@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2588303002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/tests/results/core/V8TestCallbackFunctions.cpp (right): https://codereview.chromium.org/2588303002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/tests/results/core/V8TestCallbackFunctions.cpp:258: voidCallbackFunctionArg = VoidCallbackFunction::create(ScriptState::current(info.GetIsolate()), v8::Local<v8::Function>::Cast(info[0])); bashi@, I think here we need a null-check for info[0] before creating VoidCallbackFunction?
Message was sent while issue was closed.
https://codereview.chromium.org/2588303002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/tests/results/core/V8TestCallbackFunctions.cpp (right): https://codereview.chromium.org/2588303002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/tests/results/core/V8TestCallbackFunctions.cpp:258: voidCallbackFunctionArg = VoidCallbackFunction::create(ScriptState::current(info.GetIsolate()), v8::Local<v8::Function>::Cast(info[0])); On 2016/12/22 11:34:17, Zhiqiang Zhang wrote: > bashi@, I think here we need a null-check for info[0] before creating > VoidCallbackFunction? Sorry I missed this. Yes, we should add check. Uploaded a fix. https://codereview.chromium.org/2607433002/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
