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

Issue 79203004: [Gin] Add a mechanism for wrapping C++ object (Closed)

Created:
7 years, 1 month ago by abarth-chromium
Modified:
7 years, 1 month ago
CC:
chromium-reviews
Visibility:
Public.

Description

[Gin] Add a mechanism for wrapping C++ object This CL adds a mechanism for wrapping C++ objects to Gin. The approach in this CL is similar to Blink's ScriptWrappable class, with a couple of differences: 1) gin::Wrappable has a vtable whereas Blink's ScriptWrappable class does not. Having a vtable in this base class lets us simplify a large number of concerns. We've talked about adding a vtable to ScriptWrappable but have avoided it because Blink creates many thousands of wrapped objects. When we refactor Blink to use Gin, we can still support the non-vtable approach, but most clients of Gin will want the simpler approach. 2) In Gin, we've bound together the notion of being reference counted with the notion of being wrappable from JavaScript. In Blink, those concepts are separate because we don't want to introduce a virtual destructor for ScriptWrappable. However, because gin::Wrappable already has a vtable, adding a virtual destructor is relatively cheap. Actually wrapping a C++ object still takes too much typing, but we can improve that in future CLs. R=jochen@chromium.org BUG=317398 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236555

Patch Set 1 #

Total comments: 2

Patch Set 2 : Remove extra friend #

Total comments: 1

Patch Set 3 : Disallow copy and assign #

Unified diffs Side-by-side diffs Delta from patch set Stats (+276 lines, -12 lines) Patch
M gin/array_buffer.cc View 1 chunk +1 line, -1 line 0 comments Download
M gin/dictionary.cc View 1 chunk +2 lines, -1 line 0 comments Download
M gin/gin.gyp View 3 chunks +13 lines, -10 lines 0 comments Download
A gin/wrappable.h View 1 2 1 chunk +60 lines, -0 lines 0 comments Download
A gin/wrappable.cc View 1 chunk +63 lines, -0 lines 0 comments Download
A gin/wrappable_unittest.cc View 1 1 chunk +137 lines, -0 lines 0 comments Download

Messages

Total messages: 9 (0 generated)
abarth-chromium
7 years, 1 month ago (2013-11-21 01:40:59 UTC) #1
abarth-chromium
https://codereview.chromium.org/79203004/diff/1/gin/array_buffer.cc File gin/array_buffer.cc (right): https://codereview.chromium.org/79203004/diff/1/gin/array_buffer.cc#newcode110 gin/array_buffer.cc:110: parameter->self_reference_ = NULL; I can make this change in ...
7 years, 1 month ago (2013-11-21 02:53:43 UTC) #2
jochen (gone - plz use gerrit)
https://codereview.chromium.org/79203004/diff/40001/gin/wrappable.h File gin/wrappable.h (right): https://codereview.chromium.org/79203004/diff/40001/gin/wrappable.h#newcode30 gin/wrappable.h:30: v8::Persistent<v8::Object> wrapper_; // Weak if you end up copying ...
7 years, 1 month ago (2013-11-21 13:06:27 UTC) #3
abarth-chromium
On 2013/11/21 13:06:27, jochen wrote: > https://codereview.chromium.org/79203004/diff/40001/gin/wrappable.h > File gin/wrappable.h (right): > > https://codereview.chromium.org/79203004/diff/40001/gin/wrappable.h#newcode30 > ...
7 years, 1 month ago (2013-11-21 15:25:01 UTC) #4
jochen (gone - plz use gerrit)
On 2013/11/21 15:25:01, abarth wrote: > On 2013/11/21 13:06:27, jochen wrote: > > https://codereview.chromium.org/79203004/diff/40001/gin/wrappable.h > ...
7 years, 1 month ago (2013-11-21 15:39:55 UTC) #5
abarth-chromium
On 2013/11/21 15:39:55, jochen wrote: > Anyway, adding DISALLOW_COPY_AND_ASSIGN would also work. Done. PTAL. Thanks!
7 years, 1 month ago (2013-11-21 16:06:51 UTC) #6
jochen (gone - plz use gerrit)
lgtm, thanks
7 years, 1 month ago (2013-11-21 16:13:34 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/abarth@chromium.org/79203004/130001
7 years, 1 month ago (2013-11-21 16:19:44 UTC) #8
commit-bot: I haz the power
7 years, 1 month ago (2013-11-21 18:38:53 UTC) #9
Message was sent while issue was closed.
Change committed as 236555

Powered by Google App Engine
This is Rietveld 408576698