Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1995993002:
Only initialize usrsctp when it's used and uninitialize when it's not being used. (Closed)
|
Created:
4 years, 7 months ago by tommi Modified:
4 years, 7 months ago Reviewers:
Taylor Brandstetter CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionOnly initialize usrsctp when it's used and uninitialize when it's not being used.
BUG=chromium:612366, webrtc:5909
R=deadbeef@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/7d01331eca13c620b3e6fa118c022438568f64b1
Patch Set 1 : Rearrange code: code that needs to be public, stays, static functions are now anonymous #Patch Set 2 : Make globals anonymous and codecs_ const #Patch Set 3 : Change debug_name_ to be const char*, remove unused method and rename the other to for_testing #
Total comments: 4
Patch Set 4 : Remove thread checker and add {}'s. #
Messages
Total messages: 32 (14 generated)
tommi@webrtc.org changed reviewers: + deadbeef@webrtc.org
git cl format
The CQ bit was checked by tommi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1995993002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995993002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Description was changed from ========== Only initialize usrsctp when it's used and uninitialize when it's not being used. BUG=chromium:612366 ========== to ========== Only initialize usrsctp when it's used and uninitialize when it's not being used. BUG=chromium:612366, webrtc:5909 ==========
Dry run: Try jobs failed on following builders: linux_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/15243)
Replace dcheck with comment.
The CQ bit was checked by tommi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1995993002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995993002/40001
Rearrange code so that code that needs to be public, is public and static functions are now in an anonymous namespace
Make globals anonymous and codecs_ const
Change debug_name_ to be const char*, remove unused method and rename the other to for_testing
The CQ bit was checked by tommi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1995993002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995993002/100001
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nits. I was just about to upload effectively the same CL, but yours has a lot of extra refactoring so let's go with yours. :) https://codereview.webrtc.org/1995993002/diff/100001/webrtc/media/sctp/sctpda... File webrtc/media/sctp/sctpdataengine.cc (right): https://codereview.webrtc.org/1995993002/diff/100001/webrtc/media/sctp/sctpda... webrtc/media/sctp/sctpdataengine.cc:308: if (!g_usrsctp_usage_count) nit: We use {}s even for 1-line if statements. https://codereview.webrtc.org/1995993002/diff/100001/webrtc/media/sctp/sctpda... webrtc/media/sctp/sctpdataengine.cc:342: RTC_DCHECK(thread_checker_.CalledOnValidThread()); Is this the only place thread_checker_ is used? Could you add a comment explaining its purpose?
Remove thread checker and add {}'s.
https://codereview.webrtc.org/1995993002/diff/100001/webrtc/media/sctp/sctpda... File webrtc/media/sctp/sctpdataengine.cc (right): https://codereview.webrtc.org/1995993002/diff/100001/webrtc/media/sctp/sctpda... webrtc/media/sctp/sctpdataengine.cc:308: if (!g_usrsctp_usage_count) On 2016/05/19 15:53:13, Taylor Brandstetter wrote: > nit: We use {}s even for 1-line if statements. Done. (here and in a couple of other places) https://codereview.webrtc.org/1995993002/diff/100001/webrtc/media/sctp/sctpda... webrtc/media/sctp/sctpdataengine.cc:342: RTC_DCHECK(thread_checker_.CalledOnValidThread()); On 2016/05/19 15:53:13, Taylor Brandstetter wrote: > Is this the only place thread_checker_ is used? Could you add a comment > explaining its purpose? I've removed the thread checker now and moved the implementation back to the header. The reason I put the thread checker there initially was to make sure that codecs_ was always accessed on the same thread, since there was no lock and it was not const. I initially also wanted to use the checker for CreateChannel but found out it's called on the worker thread. Now that I've made codecs_ const though, there's nothing to worry about as far as this method goes and the checker isn't necessary.
The CQ bit was checked by tommi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org Link to the patchset: https://codereview.webrtc.org/1995993002/#ps120001 (title: "Remove thread checker and add {}'s.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1995993002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995993002/120001
Message was sent while issue was closed.
Description was changed from ========== Only initialize usrsctp when it's used and uninitialize when it's not being used. BUG=chromium:612366, webrtc:5909 ========== to ========== Only initialize usrsctp when it's used and uninitialize when it's not being used. BUG=chromium:612366, webrtc:5909 R=deadbeef@webrtc.org Committed: https://crrev.com/7d01331eca13c620b3e6fa118c022438568f64b1 Cr-Commit-Position: refs/heads/master@{#12816} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7d01331eca13c620b3e6fa118c022438568f64b1 Cr-Commit-Position: refs/heads/master@{#12816}
Message was sent while issue was closed.
Description was changed from ========== Only initialize usrsctp when it's used and uninitialize when it's not being used. BUG=chromium:612366, webrtc:5909 R=deadbeef@webrtc.org Committed: https://crrev.com/7d01331eca13c620b3e6fa118c022438568f64b1 Cr-Commit-Position: refs/heads/master@{#12816} ========== to ========== Only initialize usrsctp when it's used and uninitialize when it's not being used. BUG=chromium:612366, webrtc:5909 R=deadbeef@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/7d01331eca13c620b3e6fa118... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001) manually as 7d01331eca13c620b3e6fa118c022438568f64b1 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
