Do FilteringNetworkManager::CheckPermission from new Initialize method.

Do FilteringNetworkManager::CheckPermission from new Initialize method.

This is guaranteed to be called from PeerConnection on the correct
thread when the PeerConnection is initializing. This means it's no
longer PeerConnectionDependencyFactory's responsibility to initialize
it, and it can be initialized in a thread safe way.

BUG=617648
Committed: https://crrev.com/c6dcde40c5dc3ad78ee039f6a7e9e71437256512
Cr-Commit-Position: refs/heads/master@{#404395}

[Taylor_Brandstetter, 2016-06-29 23:34:21]

deadbeef@chromium.org changed reviewers:
+ sergeyu@chromium.org, tommi@chromium.org

[Taylor_Brandstetter, 2016-06-29 23:34:23]

tommi@chromium.org: Please review changes in
peer_connection_dependency_factory.cc.

sergeyu@chromium.org: Please review changes /p2p/.

This is the solution I proposed on this CL:
https://codereview.chromium.org/2061903003/

https://codereview.chromium.org/2113523003/diff/1/content/renderer/p2p/filter...
File content/renderer/p2p/filtering_network_manager.cc (right):

https://codereview.chromium.org/2113523003/diff/1/content/renderer/p2p/filter...
content/renderer/p2p/filtering_network_manager.cc:94: void
FilteringNetworkManager::CheckPermission() {
Just moved this to match where it is in the header file; nothing has changed.

[tommi (sloooow) - chröme, 2016-06-30 10:48:54]

lgtm

[Taylor_Brandstetter, 2016-07-07 01:09:33]

Pinging Sergey. We should try to get this into M53.

[Sergey Ulanov, 2016-07-07 18:09:09]

LGTM after my comments are addressed. Sorry for the delay - didn't know you were
waiting for my review

https://codereview.chromium.org/2113523003/diff/1/content/renderer/p2p/filter...
File content/renderer/p2p/filtering_network_manager.cc (right):

https://codereview.chromium.org/2113523003/diff/1/content/renderer/p2p/filter...
content/renderer/p2p/filtering_network_manager.cc:49: if (media_permission_) {
nit: remove {} for consistency with other single-line if statements in this
file.

https://codereview.chromium.org/2113523003/diff/1/content/renderer/p2p/filter...
content/renderer/p2p/filtering_network_manager.cc:49: if (media_permission_) {
Call NetworkManager::Initialize() to initialize the parent class.

https://codereview.chromium.org/2113523003/diff/1/content/renderer/p2p/port_a...
File content/renderer/p2p/port_allocator.cc (right):

https://codereview.chromium.org/2113523003/diff/1/content/renderer/p2p/port_a...
content/renderer/p2p/port_allocator.cc:55: if (network_manager_) {
nit: Do you really need this condition? AFAICT network_manager_ should always be
set. If the constructor can take nullptr for network_manager_, can you please
add comments about this in the header?

https://codereview.chromium.org/2113523003/diff/1/content/renderer/p2p/port_a...
content/renderer/p2p/port_allocator.cc:55: if (network_manager_) {
call cricket::BasicPortAllocator::Initialize() here to initialize the parent

https://codereview.chromium.org/2113523003/diff/1/content/renderer/p2p/port_a...
File content/renderer/p2p/port_allocator.h (right):

https://codereview.chromium.org/2113523003/diff/1/content/renderer/p2p/port_a...
content/renderer/p2p/port_allocator.h:49: void Initialize() override;
Who is supposed to call this method? I don't see it being called anywhere right
now.

[Taylor_Brandstetter, 2016-07-07 21:30:28]

https://codereview.chromium.org/2113523003/diff/1/content/renderer/p2p/filter...
File content/renderer/p2p/filtering_network_manager.cc (right):

https://codereview.chromium.org/2113523003/diff/1/content/renderer/p2p/filter...
content/renderer/p2p/filtering_network_manager.cc:49: if (media_permission_) {
On 2016/07/07 18:09:08, Sergey Ulanov wrote:
> nit: remove {} for consistency with other single-line if statements in this
> file.

Done.

https://codereview.chromium.org/2113523003/diff/1/content/renderer/p2p/filter...
content/renderer/p2p/filtering_network_manager.cc:49: if (media_permission_) {
On 2016/07/07 18:09:08, Sergey Ulanov wrote:
> Call NetworkManager::Initialize() to initialize the parent class.

I assume you meant NetworkManagerBase; if so, done.

https://codereview.chromium.org/2113523003/diff/1/content/renderer/p2p/port_a...
File content/renderer/p2p/port_allocator.cc (right):

https://codereview.chromium.org/2113523003/diff/1/content/renderer/p2p/port_a...
content/renderer/p2p/port_allocator.cc:55: if (network_manager_) {
On 2016/07/07 18:09:08, Sergey Ulanov wrote:
> call cricket::BasicPortAllocator::Initialize() here to initialize the parent

Done.

https://codereview.chromium.org/2113523003/diff/1/content/renderer/p2p/port_a...
content/renderer/p2p/port_allocator.cc:55: if (network_manager_) {
On 2016/07/07 18:09:08, Sergey Ulanov wrote:
> nit: Do you really need this condition? AFAICT network_manager_ should always
be
> set. If the constructor can take nullptr for network_manager_, can you please
> add comments about this in the header?

I didn't see any "RTC_DCHECK(network_manager_)" so I assumed it was possible for
it to be null. I'll add that DCHECK.

https://codereview.chromium.org/2113523003/diff/1/content/renderer/p2p/port_a...
File content/renderer/p2p/port_allocator.h (right):

https://codereview.chromium.org/2113523003/diff/1/content/renderer/p2p/port_a...
content/renderer/p2p/port_allocator.h:49: void Initialize() override;
On 2016/07/07 18:09:08, Sergey Ulanov wrote:
> Who is supposed to call this method? I don't see it being called anywhere
right
> now.

It's called by PeerConnection:
https://cs.chromium.org/chromium/src/third_party/webrtc/api/peerconnection.cc...

This is so that PeerConnection can handle the thread-specific initialization,
meaning PeerConnectionDependencyFactory doesn't need to.

[Taylor_Brandstetter, 2016-07-07 21:30:54]

The CQ bit was checked by deadbeef@chromium.org

[Taylor_Brandstetter, 2016-07-07 21:30:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyu@chromium.org, tommi@chromium.org
Link to the patchset: https://codereview.chromium.org/2113523003/#ps20001
(title: "Responding to comments.")

[commit-bot: I haz the power, 2016-07-07 21:32:24]

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

[Sergey Ulanov, 2016-07-07 21:38:23]

https://codereview.chromium.org/2113523003/diff/20001/content/renderer/p2p/po...
File content/renderer/p2p/port_allocator.cc (right):

https://codereview.chromium.org/2113523003/diff/20001/content/renderer/p2p/po...
content/renderer/p2p/port_allocator.cc:30: RTC_DCHECK(network_manager);
This code is not in webrtc, so this should be DCHECK, not RTC_DCHECK.
Also add a DCHECK for socket_dispatcher_ and socket_factory

[Taylor_Brandstetter, 2016-07-07 21:48:47]

The CQ bit was unchecked by deadbeef@chromium.org

[Taylor_Brandstetter, 2016-07-07 21:49:06]

https://codereview.chromium.org/2113523003/diff/20001/content/renderer/p2p/po...
File content/renderer/p2p/port_allocator.cc (right):

https://codereview.chromium.org/2113523003/diff/20001/content/renderer/p2p/po...
content/renderer/p2p/port_allocator.cc:30: RTC_DCHECK(network_manager);
On 2016/07/07 21:38:23, Sergey Ulanov wrote:
> This code is not in webrtc, so this should be DCHECK, not RTC_DCHECK.
> Also add a DCHECK for socket_dispatcher_ and socket_factory

Thanks for catching that. Force of habit.

[Taylor_Brandstetter, 2016-07-07 22:50:21]

The CQ bit was checked by deadbeef@chromium.org

[Taylor_Brandstetter, 2016-07-07 22:50:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyu@chromium.org, tommi@chromium.org
Link to the patchset: https://codereview.chromium.org/2113523003/#ps60001
(title: "Fixing the network_manager DCHECK. It's std::moved so must check member
variable.")

[commit-bot: I haz the power, 2016-07-07 22:51:04]

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

[commit-bot: I haz the power, 2016-07-08 00:52:26]

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

[commit-bot: I haz the power, 2016-07-08 00:52:28]

Try jobs failed on following builders:
  mac_chromium_gyp_rel on master.tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)

[Taylor_Brandstetter, 2016-07-08 15:35:53]

The CQ bit was checked by deadbeef@chromium.org

[commit-bot: I haz the power, 2016-07-08 15:36:23]

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

[commit-bot: I haz the power, 2016-07-08 15:42:07]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-08 15:42:09]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-08 15:45:21]

Description was changed from

==========
Do FilteringNetworkManager::CheckPermission from new Initialize method.

This is guaranteed to be called from PeerConnection on the correct
thread when the PeerConnection is initializing. This means it's no
longer PeerConnectionDependencyFactory's responsibility to initialize
it, and it can be initialized in a thread safe way.

BUG=617648
==========

to

==========
Do FilteringNetworkManager::CheckPermission from new Initialize method.

This is guaranteed to be called from PeerConnection on the correct
thread when the PeerConnection is initializing. This means it's no
longer PeerConnectionDependencyFactory's responsibility to initialize
it, and it can be initialized in a thread safe way.

BUG=617648

Committed: https://crrev.com/c6dcde40c5dc3ad78ee039f6a7e9e71437256512
Cr-Commit-Position: refs/heads/master@{#404395}
==========

[commit-bot: I haz the power, 2016-07-08 15:45:22]

Patchset 4 (id:??) landed as
https://crrev.com/c6dcde40c5dc3ad78ee039f6a7e9e71437256512
Cr-Commit-Position: refs/heads/master@{#404395}