Issue 843943002: Refactor VideoCapturerDelegate to use WeakPtr instead of refcount.

Issue 843943002: Refactor VideoCapturerDelegate to use WeakPtr instead of refcount. (Closed)

Created:
5 years, 11 months ago by perkj_chrome

Modified:
5 years, 11 months ago

Reviewers:
magjed_chromium

CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, wjia+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Refactor VideoCapturerDelegate to use WeakPtr instead of refcount. The reason is to fix a problem where the delegate could be deleted on the IO-thread instead of the main render thread. BUG=444177 Committed: https://crrev.com/60cd512c159e730fe2f2998a828bfa6727dd1785 Cr-Commit-Position: refs/heads/master@{#310776}

Patch Set 1 #

Patch Set 2 : Cleaned up tests a bit. #

Patch Set 3 : And make it right. #

Total comments: 4

Patch Set 4 : Addressed comments. #

Patch Set 5 : Fix source_formats_callback_.reset #

Created: 5 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+35 lines, -26 lines)			Patch
M	content/renderer/media/media_stream_video_capture_source_unittest.cc	View	1	7 chunks	+14 lines, -10 lines	0 comments	Download
M	content/renderer/media/media_stream_video_capturer_source.h	View	1 2	6 chunks	+7 lines, -6 lines	0 comments	Download
M	content/renderer/media/media_stream_video_capturer_source.cc	View	1 2 3 4	7 chunks	+13 lines, -9 lines	0 comments	Download
M	content/renderer/media/user_media_client_impl.cc	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download

Messages

Total messages: 13 (5 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

magjed_chromium

lgtm % nits https://codereview.chromium.org/843943002/diff/60001/content/renderer/media/media_stream_video_capturer_source.cc File content/renderer/media/media_stream_video_capturer_source.cc (right): https://codereview.chromium.org/843943002/diff/60001/content/renderer/media/media_stream_video_capturer_source.cc#newcode97 content/renderer/media/media_stream_video_capturer_source.cc:97: Is this new empty line on ...

5 years, 11 months ago (2015-01-09 11:15:53 UTC) #4

perkj_chrome

https://codereview.chromium.org/843943002/diff/60001/content/renderer/media/media_stream_video_capturer_source.cc File content/renderer/media/media_stream_video_capturer_source.cc (right): https://codereview.chromium.org/843943002/diff/60001/content/renderer/media/media_stream_video_capturer_source.cc#newcode97 content/renderer/media/media_stream_video_capturer_source.cc:97: On 2015/01/09 11:15:53, magjed wrote: > Is this new ...

5 years, 11 months ago (2015-01-09 11:43:59 UTC) #5

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/843943002/80001

5 years, 11 months ago (2015-01-09 11:44:58 UTC) #7

commit-bot: I haz the power

Try jobs failed on following builders: linux_chromium_asan_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel/builds/23825)

5 years, 11 months ago (2015-01-09 13:18:14 UTC) #9

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/843943002/100001

5 years, 11 months ago (2015-01-09 15:37:06 UTC) #11

commit-bot: I haz the power

5 years, 11 months ago (2015-01-09 16:29:32 UTC) #13

Message was sent while issue was closed.

Patchset 5 (id:??) landed as
https://crrev.com/60cd512c159e730fe2f2998a828bfa6727dd1785
Cr-Commit-Position: refs/heads/master@{#310776}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages