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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 209893002: Custom wrap constructor overloads. (Closed)

Created:
6 years, 9 months ago by sof
Modified:
6 years, 9 months ago
CC:
blink-reviews, kojih, arv+blink, jsbell+bindings_chromium.org, kouhei+bindings_chromium.org, abarth-chromium, marja+watch_chromium.org, adamk+blink_chromium.org, Nate Chapin, Inactive
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Custom wrap constructor overloads. Follow up on crrev.com/206793006 and also call wrap() in the code generated for a particular constructor overloading when [Custom=Wrap] is in effect for one or more constructors. R=nbarth@chromium.org,haraken@chromium.org BUG=338804 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169839

Patch Set 1 #

Total comments: 2

Patch Set 2 : Code tweak #

Unified diffs Side-by-side diffs Delta from patch set Stats (+15 lines, -12 lines) Patch
M Source/bindings/templates/methods.cpp View 1 1 chunk +5 lines, -1 line 0 comments Download
M Source/bindings/tests/results/V8TestInterface.cpp View 1 chunk +1 line, -1 line 0 comments Download
M Source/bindings/tests/results/V8TestInterfaceConstructor.cpp View 2 chunks +2 lines, -2 lines 0 comments Download
M Source/bindings/tests/results/V8TestInterfaceConstructor2.cpp View 2 chunks +2 lines, -2 lines 0 comments Download
M Source/bindings/tests/results/V8TestInterfaceConstructor3.cpp View 1 chunk +1 line, -1 line 0 comments Download
M Source/bindings/tests/results/V8TestInterfacePython2.cpp View 1 1 chunk +1 line, -2 lines 0 comments Download
M Source/bindings/tests/results/V8TestInterfaceWillBeGarbageCollected.cpp View 1 chunk +1 line, -1 line 0 comments Download
M Source/bindings/tests/results/V8TestNode.cpp View 1 chunk +1 line, -1 line 0 comments Download
M Source/bindings/tests/results/V8TestTypedefs.cpp View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 27 (0 generated)
sof
6 years, 9 months ago (2014-03-24 08:30:53 UTC) #1
haraken
LGTM, thanks for the fix!
6 years, 9 months ago (2014-03-24 08:32:32 UTC) #2
sof
The CQ bit was checked by sigbjornf@opera.com
6 years, 9 months ago (2014-03-24 08:34:16 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/209893002/1
6 years, 9 months ago (2014-03-24 08:34:19 UTC) #4
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 9 months ago (2014-03-24 08:34:29 UTC) #5
commit-bot: I haz the power
Try jobs failed on following builders: tryserver.blink on mac_blink_compile_dbg
6 years, 9 months ago (2014-03-24 08:34:30 UTC) #6
Nils Barth (inactive)
(confused) https://codereview.chromium.org/209893002/diff/1/Source/bindings/templates/methods.cpp File Source/bindings/templates/methods.cpp (right): https://codereview.chromium.org/209893002/diff/1/Source/bindings/templates/methods.cpp#newcode399 Source/bindings/templates/methods.cpp:399: v8SetReturnValue(info, wrapper); I don't follow how this differs ...
6 years, 9 months ago (2014-03-24 08:50:25 UTC) #7
Nils Barth (inactive)
On 2014/03/24 08:34:30, I haz the power (commit-bot) wrote: > Try jobs failed on following ...
6 years, 9 months ago (2014-03-24 08:53:39 UTC) #8
Nils Barth (inactive)
Refactoring suggestion/request. https://codereview.chromium.org/209893002/diff/1/Source/bindings/templates/methods.cpp File Source/bindings/templates/methods.cpp (right): https://codereview.chromium.org/209893002/diff/1/Source/bindings/templates/methods.cpp#newcode399 Source/bindings/templates/methods.cpp:399: v8SetReturnValue(info, wrapper); On 2014/03/24 08:50:26, Nils Barth ...
6 years, 9 months ago (2014-03-24 08:54:59 UTC) #9
sof
The CQ bit was checked by sigbjornf@opera.com
6 years, 9 months ago (2014-03-24 08:55:38 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/209893002/1
6 years, 9 months ago (2014-03-24 08:55:42 UTC) #11
Nils Barth (inactive)
The CQ bit was unchecked by nbarth@chromium.org
6 years, 9 months ago (2014-03-24 08:56:00 UTC) #12
Nils Barth (inactive)
On 2014/03/24 08:55:42, I haz the power (commit-bot) wrote: > CQ is trying da patch. ...
6 years, 9 months ago (2014-03-24 08:56:27 UTC) #13
sof
On 2014/03/24 08:54:59, Nils Barth wrote: > Refactoring suggestion/request. > > https://codereview.chromium.org/209893002/diff/1/Source/bindings/templates/methods.cpp > File Source/bindings/templates/methods.cpp ...
6 years, 9 months ago (2014-03-24 08:57:17 UTC) #14
sof
The CQ bit was checked by sigbjornf@opera.com
6 years, 9 months ago (2014-03-24 09:01:31 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/209893002/20001
6 years, 9 months ago (2014-03-24 09:01:33 UTC) #16
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 9 months ago (2014-03-24 09:42:06 UTC) #17
commit-bot: I haz the power
Try jobs failed on following builders: tryserver.blink on win_blink_rel
6 years, 9 months ago (2014-03-24 09:42:06 UTC) #18
sof
The CQ bit was checked by sigbjornf@opera.com
6 years, 9 months ago (2014-03-24 09:42:54 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/209893002/20001
6 years, 9 months ago (2014-03-24 09:43:06 UTC) #20
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 9 months ago (2014-03-24 10:24:01 UTC) #21
commit-bot: I haz the power
Try jobs failed on following builders: tryserver.blink on win_blink_rel
6 years, 9 months ago (2014-03-24 10:24:01 UTC) #22
sof
The CQ bit was checked by sigbjornf@opera.com
6 years, 9 months ago (2014-03-24 10:24:51 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/209893002/20001
6 years, 9 months ago (2014-03-24 10:25:01 UTC) #24
sof
The CQ bit was checked by sigbjornf@opera.com
6 years, 9 months ago (2014-03-24 10:27:01 UTC) #25
commit-bot: I haz the power
Change committed as 169839
6 years, 9 months ago (2014-03-24 11:00:59 UTC) #26
Nils Barth (inactive)
6 years, 9 months ago (2014-03-25 01:28:38 UTC) #27
Message was sent while issue was closed. 
On 2014/03/24 08:57:17, sof wrote:
> > Could you factor out the wrapper as I did?
> > v8::Handle<v8::Object> wrapper = wrap(impl.get(), info.Holder(),
> > info.GetIsolate());
> > ...and then the v8SetReturnValue *after* the if?
> 
> Aw, why that formulation? ok then.

I think it's a bit clearer that way.
Firstly, we're already using an auxiliary |wrapper| variable
in one code path, so this minimizes how much code is in the conditional
and clarifies exactly how the two paths differ.
Also, it keeps the definition close to use.
Does this make sense?

I've documented this suggestion at:
http://www.chromium.org/developers/design-documents/idl-compiler#TOC-Back-end...
in "Templated code style".

Anyway, thanks for being accommodating!

Powered by Google App Engine
This is Rietveld 408576698