Make CongestionController::remote_bitrate_estimator_ a non-pointer.

Make CongestionController::remote_bitrate_estimator_ a non-pointer.

It was only assigned at construction, and this improves consistency
with remote_estimator_proxy_.

The declaration of the private WrappingBitrateEstimator had to be
moved to the header file, and it was also converted from
system_wrappers' CriticalSectionWrapper to rtc::CriticalSection.

BUG=webrtc:6847
Review-Url: https://codereview.webrtc.org/2642363003
Cr-Commit-Position: refs/heads/master@{#16236}
Committed: https://chromium.googlesource.com/external/webrtc/+/365aebdade5ac602e901e4a3c7674e8407ab5d26

[nisse-webrtc, 2017-01-20 15:24:58]

nisse@webrtc.org changed reviewers:
+ stefan@webrtc.org

[stefan-webrtc, 2017-01-23 10:38:12]

lgtm % nits

https://codereview.webrtc.org/2642363003/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/include/congestion_controller.h
(right):

https://codereview.webrtc.org/2642363003/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/include/congestion_controller.h:19:
#include "webrtc/base/deprecation.h"
Looks like this can be removed, could you do that too? Thanks

https://codereview.webrtc.org/2642363003/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/include/congestion_controller.h:171:
WrappingBitrateEstimator remote_bitrate_estimator_;
Move this down to RemoteEstimatorProxy just to group const ptrs together?

[nisse-webrtc, 2017-01-23 10:47:03]

I have one question on construction order, see below.

https://codereview.webrtc.org/2642363003/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/include/congestion_controller.h
(right):

https://codereview.webrtc.org/2642363003/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/include/congestion_controller.h:19:
#include "webrtc/base/deprecation.h"
On 2017/01/23 10:38:12, stefan-webrtc wrote:
> Looks like this can be removed, could you do that too? Thanks

Done.

https://codereview.webrtc.org/2642363003/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/include/congestion_controller.h:171:
WrappingBitrateEstimator remote_bitrate_estimator_;
On 2017/01/23 10:38:12, stefan-webrtc wrote:
> Move this down to RemoteEstimatorProxy just to group const ptrs together?

Done.

https://codereview.webrtc.org/2642363003/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/congestion_controller.cc (right):

https://codereview.webrtc.org/2642363003/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/congestion_controller.cc:165: //
Constructed last as this object calls the provided callback on
This comment looks a bit scary, since bitrate_controller_ is far from last. If
the comment is still valid, can we clarify what other members
bitrate_controller_ depends on? Or should it just be deleted?

[nisse-webrtc, 2017-01-23 13:05:15]

https://codereview.webrtc.org/2642363003/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/congestion_controller.cc (right):

https://codereview.webrtc.org/2642363003/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/congestion_controller.cc:165: //
Constructed last as this object calls the provided callback on
On 2017/01/23 10:47:02, nisse-webrtc wrote:
> This comment looks a bit scary, since bitrate_controller_ is far from last. If
> the comment is still valid, can we clarify what other members
> bitrate_controller_ depends on? Or should it just be deleted?

I'm deleting this comment, BitrateController has an optional and deprecated
BitrateObserver which is invoked from the constructor if set, but in our case,
there's no such observer.

[nisse-webrtc, 2017-01-23 13:05:36]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 13:05:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-23 13:06:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 13:06:36]

Dry run: Try jobs failed on following builders:
  win_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/17900)
  win_clang_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_dbg/builds/10176)
  win_clang_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_rel/builds/10084)
  win_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/15444)
  win_rel on master.tryserver.webrtc (JOB_FAILED, no build URL)
  win_x64_clang_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...)
  win_x64_clang_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_rel/build...)
  win_x64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/5505)
  win_x64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/21907)

[nisse-webrtc, 2017-01-24 10:18:50]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-01-24 10:18:50]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2642363003/#ps40001 (title:
"Delete obsolete comment.")

[commit-bot: I haz the power, 2017-01-24 10:18:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-24 10:53:39]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485253130208290,
"parent_rev": "d2b092f38abad228b4c38697e0c56a189044d9c5", "commit_rev":
"365aebdade5ac602e901e4a3c7674e8407ab5d26"}

[commit-bot: I haz the power, 2017-01-24 10:53:41]

Description was changed from

==========
Make CongestionController::remote_bitrate_estimator_ a non-pointer.

It was only assigned at construction, and this improves consistency
with remote_estimator_proxy_.

The declaration of the private WrappingBitrateEstimator had to be
moved to the header file, and it was also converted from
system_wrappers' CriticalSectionWrapper to rtc::CriticalSection.

BUG=webrtc:6847
==========

to

==========
Make CongestionController::remote_bitrate_estimator_ a non-pointer.

It was only assigned at construction, and this improves consistency
with remote_estimator_proxy_.

The declaration of the private WrappingBitrateEstimator had to be
moved to the header file, and it was also converted from
system_wrappers' CriticalSectionWrapper to rtc::CriticalSection.

BUG=webrtc:6847

Review-Url: https://codereview.webrtc.org/2642363003
Cr-Commit-Position: refs/heads/master@{#16236}
Committed:
https://chromium.googlesource.com/external/webrtc/+/365aebdade5ac602e901e4a3c...
==========

[commit-bot: I haz the power, 2017-01-24 10:53:42]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/365aebdade5ac602e901e4a3c...