Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(112)

Issue 954723002: [bindings] Support is_explicit_nullable for method arguments. (Closed)

Created:
5 years, 10 months ago by vivekg_samsung
Modified:
5 years, 9 months ago
Reviewers:
vivekg, Jens Widell
CC:
arv+blink, blink-reviews, blink-reviews-bindings_chromium.org, vivekg_samsung
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

[bindings] Support is_explicit_nullable for method arguments. R=jl@opera.com Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191100

Patch Set 1 #

Total comments: 2

Patch Set 2 : Review comments addressed! #

Total comments: 4

Patch Set 3 : Modified patch #

Total comments: 1

Patch Set 4 : Fixed patch #

Total comments: 1

Patch Set 5 : Patch for landing! #

Unified diffs Side-by-side diffs Delta from patch set Stats (+72 lines, -4 lines) Patch
M Source/bindings/scripts/v8_methods.py View 1 2 3 2 chunks +8 lines, -3 lines 0 comments Download
M Source/bindings/scripts/v8_types.py View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M Source/bindings/templates/methods.cpp View 1 2 3 4 1 chunk +4 lines, -0 lines 0 comments Download
M Source/bindings/tests/idls/core/TestObject.idl View 1 2 2 chunks +2 lines, -0 lines 0 comments Download
M Source/bindings/tests/results/core/V8TestObject.cpp View 1 2 3 4 4 chunks +56 lines, -0 lines 0 comments Download

Messages

Total messages: 15 (3 generated)
vivekg
Hi Jens, Can you please take look? Its not ready yet but just wanted to ...
5 years, 10 months ago (2015-02-24 13:58:11 UTC) #2
Jens Widell
To solve the Document.idl/NodeIterator case (redundant check, and actually incorrect code in Oilpan builds) we ...
5 years, 10 months ago (2015-02-24 14:29:03 UTC) #3
Jens Widell
https://codereview.chromium.org/954723002/diff/1/Source/bindings/templates/methods.cpp File Source/bindings/templates/methods.cpp (right): https://codereview.chromium.org/954723002/diff/1/Source/bindings/templates/methods.cpp#newcode175 Source/bindings/templates/methods.cpp:175: {{argument.v8_value_to_local_cpp_value}}; On 2015/02/24 14:29:03, Jens Widell wrote: > This ...
5 years, 10 months ago (2015-02-24 14:30:29 UTC) #4
vivekg
jl@, sorry I was on a medical leave. I have uploaded a new patch, PTAL. ...
5 years, 9 months ago (2015-03-02 07:34:22 UTC) #5
Jens Widell
https://codereview.chromium.org/954723002/diff/20001/Source/bindings/scripts/v8_methods.py File Source/bindings/scripts/v8_methods.py (right): https://codereview.chromium.org/954723002/diff/20001/Source/bindings/scripts/v8_methods.py#newcode218 Source/bindings/scripts/v8_methods.py:218: else idl_type.cpp_type_args( I think it would be better to ...
5 years, 9 months ago (2015-03-02 07:53:13 UTC) #6
vivekg
Thank you Jens, I have uploaded a new patch with all the comments addressed. PTAL.
5 years, 9 months ago (2015-03-02 08:36:47 UTC) #7
Jens Widell
https://codereview.chromium.org/954723002/diff/40001/Source/bindings/scripts/v8_methods.py File Source/bindings/scripts/v8_methods.py (right): https://codereview.chromium.org/954723002/diff/40001/Source/bindings/scripts/v8_methods.py#newcode215 Source/bindings/scripts/v8_methods.py:215: if idl_type.is_explicit_nullable and not argument.is_variadic: This is not quite ...
5 years, 9 months ago (2015-03-02 08:42:17 UTC) #8
vivekg
On 2015/03/02 at 08:42:17, jl wrote: > https://codereview.chromium.org/954723002/diff/40001/Source/bindings/scripts/v8_methods.py > File Source/bindings/scripts/v8_methods.py (right): > > https://codereview.chromium.org/954723002/diff/40001/Source/bindings/scripts/v8_methods.py#newcode215 ...
5 years, 9 months ago (2015-03-02 10:45:20 UTC) #9
Jens Widell
LGTM after one more fix. https://codereview.chromium.org/954723002/diff/60001/Source/bindings/templates/methods.cpp File Source/bindings/templates/methods.cpp (right): https://codereview.chromium.org/954723002/diff/60001/Source/bindings/templates/methods.cpp#newcode182 Source/bindings/templates/methods.cpp:182: if (!info[{{argument.index}}]->IsNull()) { The ...
5 years, 9 months ago (2015-03-02 11:04:35 UTC) #10
vivekg
On 2015/03/02 at 11:04:35, jl wrote: > LGTM after one more fix. > > https://codereview.chromium.org/954723002/diff/60001/Source/bindings/templates/methods.cpp ...
5 years, 9 months ago (2015-03-02 11:08:07 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/954723002/80001
5 years, 9 months ago (2015-03-02 11:51:03 UTC) #14
commit-bot: I haz the power
5 years, 9 months ago (2015-03-02 13:10:59 UTC) #15
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191100

Powered by Google App Engine
This is Rietveld 408576698