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

Issue 1403683004: RefCountInterface: Make AddRef() and Release() const (Closed)

Created:
5 years, 2 months ago by magjed_webrtc
Modified:
5 years, 2 months ago
Reviewers:
tommi, perkj_webrtc
CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, rwolff_gocast.it, yujie_mao (webrtc), Andrew MacDonald, henrika_webrtc, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, mflodman
Base URL:
https://chromium.googlesource.com/external/webrtc.git@master
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

RefCountInterface: Make AddRef() and Release() const This CL makes AddRef() and Release() const member methods and the refcount integer mutable. This is reasonable, because they only manage the lifetime of the object, and this is also how it's done in Chromium. The purpose is to be able to capture a const pointer in a scoped_refptr, which is currenty impossible. The practial problem this CL solves is this: void Foo::Bar() const {} rtc::Callback0<void> Foo::MakeClosure() const { return rtc::Bind(&Foo::Bar, this); } We currently capture |this| as const Foo*. With this CL, |this| will be captured as scoped_refptr<const Foo>. A test is also added in bind_unittest to check this behaviour. BUG=webrtc:5065 R=perkj@webrtc.org, tommi@webrtc.org Committed: https://crrev.com/1b40a9a8afe0d7b2244ad8dea19e8222fec3c207 Cr-Commit-Position: refs/heads/master@{#10253}

Patch Set 1 #

Patch Set 2 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+3 lines, -4 lines) Patch
M talk/app/webrtc/dtmfsender_unittest.cc View 1 2 chunks +1 line, -2 lines 0 comments Download
M webrtc/base/refcount.h View 1 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 11 (3 generated)
magjed_webrtc
Please take a look.
5 years, 2 months ago (2015-10-12 11:27:55 UTC) #2
tommi
lgtm
5 years, 2 months ago (2015-10-12 11:50:15 UTC) #3
perkj_webrtc
On 2015/10/12 11:50:15, tommi (webrtc) wrote: > lgtm lgtm
5 years, 2 months ago (2015-10-12 11:51:16 UTC) #4
perkj_webrtc
On 2015/10/12 11:50:15, tommi (webrtc) wrote: > lgtm lgtm
5 years, 2 months ago (2015-10-12 11:51:19 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1403683004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1403683004/1
5 years, 2 months ago (2015-10-12 11:53:34 UTC) #7
commit-bot: I haz the power
Try jobs failed on following builders: mac_x64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
5 years, 2 months ago (2015-10-12 13:07:52 UTC) #9
magjed_webrtc
Committed patchset #1 (id:1) manually as 1b40a9a8afe0d7b2244ad8dea19e8222fec3c207 (presubmit successful).
5 years, 2 months ago (2015-10-12 13:50:57 UTC) #10
commit-bot: I haz the power
5 years, 2 months ago (2015-10-12 13:51:05 UTC) #11
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/1b40a9a8afe0d7b2244ad8dea19e8222fec3c207
Cr-Commit-Position: refs/heads/master@{#10253}

Powered by Google App Engine
This is Rietveld 408576698