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

Issue 2915253002: Delete SignalThread class. (Closed)

Created:
3 years, 6 months ago by nisse-webrtc
Modified:
3 years, 5 months ago
CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, kwiberg-webrtc
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

Delete SignalThread class. Rewrite AsyncResolver to use PlatformThread directly, not SignalThread, and update includes of peerconnection client to not depend on signalthread.h. BUG=webrtc:6424, webrtc:7723 Review-Url: https://codereview.webrtc.org/2915253002 Cr-Commit-Position: refs/heads/master@{#18833} Committed: https://chromium.googlesource.com/external/webrtc/+/bc8feda1db02b2a9b501e4aa43926ca7e861b638

Patch Set 1 #

Patch Set 2 : Remove thread check on AsyncResolver::GetResolvedAddress and AsyncResolver::GetError. #

Patch Set 3 : Invoke SignalDone on the main thread. #

Total comments: 31

Patch Set 4 : Address Taylor's comments, and delete lock. #

Patch Set 5 : Address nits. #

Patch Set 6 : Write the member variables for the result only on the |construction_thread_|. #

Total comments: 16

Patch Set 7 : Address comments on const and std::move. #

Patch Set 8 : Fix resolver memory leak. #

Patch Set 9 : Use a task queue. #

Total comments: 5

Patch Set 10 : Address nits. #

Patch Set 11 : Implement Destroy logic, using new method MessageQueue::PostFunctor. #

Total comments: 12

Patch Set 12 : Fix state update. #

Patch Set 13 : Address Karl's comments. #

Patch Set 14 : Fix template magic. Naming and formatting fixes. #

Total comments: 12

Patch Set 15 : Fix issue with post on destruction, adding one level of indirection. #

Patch Set 16 : Rename Call --> Run and Callable --> Runnable. #

Patch Set 17 : Fix for the case of SignalDone calling Destroy. #

Patch Set 18 : Rebase, a bit tricky due to move base --> rtc_base. #

Patch Set 19 : Added TODO comment on posting of QueuedTask. #

Total comments: 2

Patch Set 20 : Another TODO on moving to QueuedTask. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+169 lines, -568 lines) Patch
D webrtc/base/signalthread.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +0 lines, -19 lines 0 comments Download
M webrtc/examples/peerconnection/client/peer_connection_client.h View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M webrtc/examples/peerconnection/client/peer_connection_client.cc View 1 2 3 4 1 chunk +2 lines, -1 line 0 comments Download
M webrtc/rtc_base/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 3 chunks +1 line, -3 lines 0 comments Download
M webrtc/rtc_base/messagequeue.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 3 chunks +33 lines, -0 lines 0 comments Download
M webrtc/rtc_base/messagequeue.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +18 lines, -0 lines 0 comments Download
M webrtc/rtc_base/nethelpers.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +35 lines, -11 lines 0 comments Download
M webrtc/rtc_base/nethelpers.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 3 chunks +79 lines, -13 lines 0 comments Download
D webrtc/rtc_base/signalthread.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +0 lines, -161 lines 0 comments Download
D webrtc/rtc_base/signalthread.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +0 lines, -154 lines 0 comments Download
D webrtc/rtc_base/signalthread_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +0 lines, -205 lines 0 comments Download

Messages

Total messages: 45 (14 generated)
nisse-webrtc
PTAL, and see my own code comments. I'm not really familiar with PlatformThread, or what ...
3 years, 6 months ago (2017-06-02 08:41:04 UTC) #2
Taylor Brandstetter
https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc#newcode140 webrtc/base/nethelpers.cc:140: main_->Invoke<void>(RTC_FROM_HERE, On 2017/06/02 08:41:04, nisse-webrtc wrote: > The sequence ...
3 years, 6 months ago (2017-06-05 22:33:25 UTC) #4
nisse-webrtc
https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc#newcode144 webrtc/base/nethelpers.cc:144: void AsyncResolver::OnWorkDone() { On 2017/06/05 22:33:25, Taylor Brandstetter wrote: ...
3 years, 6 months ago (2017-06-07 07:17:31 UTC) #5
nisse-webrtc
Ping? Tommi, I think you said you wanted to provide some comments?
3 years, 6 months ago (2017-06-08 07:12:43 UTC) #6
tommi
sorry, I thought I had submitted the below comments. better late than never :-/ Let ...
3 years, 6 months ago (2017-06-08 08:02:11 UTC) #7
nisse-webrtc
https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.cc#newcode91 webrtc/base/nethelpers.cc:91: worker_(ThreadEntry, this, "AsyncResolver"), On 2017/06/08 08:02:05, tommi (wëbrtc) wrote: ...
3 years, 6 months ago (2017-06-08 09:52:33 UTC) #8
Taylor Brandstetter
lgtm https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.h File webrtc/base/nethelpers.h (right): https://codereview.webrtc.org/2915253002/diff/40001/webrtc/base/nethelpers.h#newcode33 webrtc/base/nethelpers.h:33: // the SignalDone from AsyncResolverInterface when the operation ...
3 years, 6 months ago (2017-06-09 02:50:13 UTC) #9
nisse-webrtc
Tommi, any comments on the updated cl? https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.cc#newcode137 webrtc/base/nethelpers.cc:137: construction_thread_->Invoke<void>( Actually, ...
3 years, 6 months ago (2017-06-09 09:44:52 UTC) #10
Taylor Brandstetter
https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.cc#newcode137 webrtc/base/nethelpers.cc:137: construction_thread_->Invoke<void>( On 2017/06/09 09:44:52, nisse-webrtc wrote: > Actually, I ...
3 years, 6 months ago (2017-06-09 18:02:58 UTC) #11
tommi
https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.cc#newcode102 webrtc/base/nethelpers.cc:102: worker_.Start(); implementing a custom QueuedTask that handles the PostTaskAndReply ...
3 years, 6 months ago (2017-06-15 10:20:32 UTC) #12
nisse-webrtc
https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.cc#newcode102 webrtc/base/nethelpers.cc:102: worker_.Start(); On 2017/06/15 10:20:32, tommi (wëbrtc) wrote: > implementing ...
3 years, 6 months ago (2017-06-15 11:33:48 UTC) #13
nisse-webrtc
On 2017/06/15 11:33:48, nisse-webrtc wrote: > https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.cc > File webrtc/base/nethelpers.cc (right): > > https://codereview.webrtc.org/2915253002/diff/100001/webrtc/base/nethelpers.cc#newcode102 > ...
3 years, 6 months ago (2017-06-15 12:18:24 UTC) #14
tommi
lgtm https://codereview.webrtc.org/2915253002/diff/160001/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2915253002/diff/160001/webrtc/base/nethelpers.cc#newcode92 webrtc/base/nethelpers.cc:92: : construction_thread_(Thread::Current()) {} add RTC_DCHECK that construction_thread_ is ...
3 years, 6 months ago (2017-06-19 13:01:47 UTC) #15
nisse-webrtc
https://codereview.webrtc.org/2915253002/diff/160001/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2915253002/diff/160001/webrtc/base/nethelpers.cc#newcode92 webrtc/base/nethelpers.cc:92: : construction_thread_(Thread::Current()) {} On 2017/06/19 13:01:47, tommi (wëbrtc) wrote: ...
3 years, 6 months ago (2017-06-19 14:07:21 UTC) #18
nisse-webrtc
On 2017/06/19 14:07:21, nisse-webrtc wrote: > https://codereview.webrtc.org/2915253002/diff/160001/webrtc/base/nethelpers.cc > File webrtc/base/nethelpers.cc (right): > > https://codereview.webrtc.org/2915253002/diff/160001/webrtc/base/nethelpers.cc#newcode92 > ...
3 years, 6 months ago (2017-06-19 15:17:47 UTC) #21
nisse-webrtc
On 2017/06/19 15:17:47, nisse-webrtc wrote: > I'll try to implement that, and undo the change ...
3 years, 5 months ago (2017-06-26 12:40:56 UTC) #22
nisse-webrtc
Karl, can you have a look at the new MessageQueue::PostFunctor method?
3 years, 5 months ago (2017-06-26 12:41:33 UTC) #24
kwiberg-webrtc
The MessageQueue::PostFunctor looks good, but I have some comments. https://codereview.webrtc.org/2915253002/diff/200001/webrtc/base/messagequeue.cc File webrtc/base/messagequeue.cc (right): https://codereview.webrtc.org/2915253002/diff/200001/webrtc/base/messagequeue.cc#newcode57 webrtc/base/messagequeue.cc:57: ...
3 years, 5 months ago (2017-06-26 13:07:48 UTC) #25
nisse-webrtc
I've addressed Karl's comment. I see one remaining problem: The message posted to the construction_thread_ ...
3 years, 5 months ago (2017-06-27 08:27:17 UTC) #26
kwiberg-webrtc
lgtm for the new MessageQueue::PostFunctor method https://codereview.webrtc.org/2915253002/diff/200001/webrtc/base/messagequeue.cc File webrtc/base/messagequeue.cc (right): https://codereview.webrtc.org/2915253002/diff/200001/webrtc/base/messagequeue.cc#newcode548 webrtc/base/messagequeue.cc:548: RTC_DEFINE_STATIC_LOCAL(FunctorPostMessageHandler, handler, ()); ...
3 years, 5 months ago (2017-06-27 09:16:28 UTC) #27
tommi
got one crazy-ish question/proposal that I'd like to hear what you think about (karl as ...
3 years, 5 months ago (2017-06-27 12:01:14 UTC) #28
kwiberg-webrtc
https://codereview.webrtc.org/2915253002/diff/260001/webrtc/base/messagequeue.h File webrtc/base/messagequeue.h (right): https://codereview.webrtc.org/2915253002/diff/260001/webrtc/base/messagequeue.h#newcode152 webrtc/base/messagequeue.h:152: virtual void Call(void) = 0; On 2017/06/27 12:01:14, tommi ...
3 years, 5 months ago (2017-06-27 13:14:56 UTC) #29
nisse-webrtc
https://codereview.webrtc.org/2915253002/diff/200001/webrtc/base/messagequeue.cc File webrtc/base/messagequeue.cc (right): https://codereview.webrtc.org/2915253002/diff/200001/webrtc/base/messagequeue.cc#newcode546 webrtc/base/messagequeue.cc:546: { On 2017/06/27 12:01:14, tommi (wëbrtc) wrote: > can ...
3 years, 5 months ago (2017-06-28 11:47:54 UTC) #30
tommi
https://codereview.webrtc.org/2915253002/diff/260001/webrtc/base/messagequeue.h File webrtc/base/messagequeue.h (right): https://codereview.webrtc.org/2915253002/diff/260001/webrtc/base/messagequeue.h#newcode152 webrtc/base/messagequeue.h:152: virtual void Call(void) = 0; On 2017/06/28 11:47:54, nisse-webrtc ...
3 years, 5 months ago (2017-06-28 13:17:33 UTC) #31
nisse-webrtc
https://codereview.webrtc.org/2915253002/diff/260001/webrtc/base/messagequeue.h File webrtc/base/messagequeue.h (right): https://codereview.webrtc.org/2915253002/diff/260001/webrtc/base/messagequeue.h#newcode152 webrtc/base/messagequeue.h:152: virtual void Call(void) = 0; On 2017/06/28 11:47:54, nisse-webrtc ...
3 years, 5 months ago (2017-06-29 10:40:32 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2915253002/360001
3 years, 5 months ago (2017-06-29 12:21:35 UTC) #35
tommi
lgtm as is then with TODOs to do the remaining work https://codereview.webrtc.org/2915253002/diff/360001/webrtc/rtc_base/messagequeue.h File webrtc/rtc_base/messagequeue.h (right): ...
3 years, 5 months ago (2017-06-29 12:27:52 UTC) #36
nisse-webrtc
https://codereview.webrtc.org/2915253002/diff/360001/webrtc/rtc_base/messagequeue.h File webrtc/rtc_base/messagequeue.h (right): https://codereview.webrtc.org/2915253002/diff/360001/webrtc/rtc_base/messagequeue.h#newcode150 webrtc/rtc_base/messagequeue.h:150: class RunnableData : public MessageData { On 2017/06/29 12:27:52, ...
3 years, 5 months ago (2017-06-29 12:38:38 UTC) #38
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2915253002/380001
3 years, 5 months ago (2017-06-29 12:39:05 UTC) #41
commit-bot: I haz the power
Committed patchset #20 (id:380001) as https://chromium.googlesource.com/external/webrtc/+/bc8feda1db02b2a9b501e4aa43926ca7e861b638
3 years, 5 months ago (2017-06-29 13:21:32 UTC) #44
Taylor Brandstetter
3 years, 5 months ago (2017-07-11 18:01:47 UTC) #45
Message was sent while issue was closed.
A revert of this CL (patchset #20 id:380001) has been created in
https://codereview.webrtc.org/2979733002/ by deadbeef@webrtc.org.

The reason for reverting is: Seems to be causing new crashes, possibly because
of changes to the "Destroy(false)" behavior. Will re-land after investigating
these crashes more and addressing the root cause..

Powered by Google App Engine
This is Rietveld 408576698