Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1349823004:
Check media permissions (mic/camera) before exposing local addresses to WebRTC. (Closed)
|
Created:
5 years, 3 months ago by guoweis_left_chromium Modified:
5 years, 2 months ago 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, asvitkine+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCheck media permissions (mic/camera) before exposing local addresses to WebRTC.
This change implements FilteringNetworkManager which wraps IpcNetworkManager from PeerConnectionDependencyFactory and exposes rtc::NetworkManager to WebRTC. P2PNetworkManager will check mic/camera permissions and only if at least one of them is granted, the full network list from IpcNetworkManager is provided to WebRTC calls. Otherwise, WebRTC can only bind to the "any" address.
Currently, this is disabled by default and can be enabled with a finch experiment "WebRTC-LocalIPPermissionCheck". UMAs are added to track how many calls are impacted (either denied due to lack of permissions, or call set up time delayed) before fully turned on.
Test cases added for FilteringNetworkManager to test its handling of api StartUpdating() and asynchronous signals such as permission status update and IpcNetworkManager's SignalNetworksChanged in different order.
When multiple_routes option is not requested, EmptyNetworkManager is used instead which simply just returns BLOCKED as permission status which will cause WebRTC to use the "any" address networks.
This depends on the ongoing CL for MediaPermissionDispatcher https://codereview.chromium.org/1351443005/
In my test with my dev machine, I don't see the setup time has been increased by this call change and we have UMA to check that too.
BUG=520101
Committed: https://crrev.com/8c1b85686e9e30179087c6767b6e2bc4684cf8e4
Cr-Commit-Position: refs/heads/master@{#351187}
Patch Set 1 : #
Total comments: 26
Patch Set 2 : #
Total comments: 40
Patch Set 3 : address comments. #
Total comments: 15
Patch Set 4 : #
Total comments: 3
Patch Set 5 : #Patch Set 6 : #
Total comments: 3
Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #
Total comments: 1
Patch Set 10 : #Patch Set 11 : rebase, fix test issue, fix windows build issue. #Messages
Total messages: 60 (30 generated)
Patchset #1 (id:1) has been deleted
guoweis@chromium.org changed reviewers: + sergeyu@chromium.org
PTAL.
https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/em... File content/renderer/p2p/empty_network_manager.cc (right): https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/em... content/renderer/p2p/empty_network_manager.cc:16: const scoped_refptr<base::SingleThreadTaskRunner>& task_runner) You use |task_runner| just to check that this class is used on the right thread. Use base::ThreadChecker for that. You just need to call DetachFromThread() in the constructor. Then it will be attached automatically to the next thread this class is called on. https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/em... content/renderer/p2p/empty_network_manager.cc:31: start_updating_time_ = base::TimeTicks::Now(); I don't think you actually need to store this value. Instead just store a boolean flag to remember that the event has been fired. https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/em... content/renderer/p2p/empty_network_manager.cc:42: if (!networks) Do we expect |networks| to be nullptr? https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/em... File content/renderer/p2p/empty_network_manager.h (right): https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/em... content/renderer/p2p/empty_network_manager.h:27: public base::SupportsWeakPtr<EmptyNetworkManager>, It's preferrable to use WeakPtrFactory instead of inheriting from SupportsWeakPtr. https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/em... content/renderer/p2p/empty_network_manager.h:28: public sigslot::has_slots<> { Don't need has_slots<> https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/em... content/renderer/p2p/empty_network_manager.h:32: CONTENT_EXPORT EmptyNetworkManager( Mark the whole class as CONTENT_EXPORT? https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/fi... File content/renderer/p2p/filtering_network_manager.cc (right): https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/fi... content/renderer/p2p/filtering_network_manager.cc:11: #include "content/renderer/media/media_permission_dispatcher_proxy.h" I don't see this being used anywhere https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/fi... content/renderer/p2p/filtering_network_manager.cc:40: task_runner_->PostTask(FROM_HERE, Is it possible to call CheckPermissions() the first time StartUpdating() is called? This is the only place where |task_runner_| is used to post tasks. So if you call CheckPermissions() the first time StartUpdating() is called then you wouldn't need task_runner (base::ThreadChecker can be used to verify that all methods are called on the right thread) https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/fi... content/renderer/p2p/filtering_network_manager.cc:90: if (!networks) I don't think you need this check. This method is not allowed to be called with nullptr https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/fi... File content/renderer/p2p/filtering_network_manager.h (right): https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/fi... content/renderer/p2p/filtering_network_manager.h:41: public base::SupportsWeakPtr<FilteringNetworkManager>, Please use WeakPtrFactory instead of inheriting from SupportsWeakPtr https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/fi... content/renderer/p2p/filtering_network_manager.h:45: struct CONTENT_EXPORT Params { Mark FilteringNetworkManager class as CONTENT_EXPORT, then you wouldn't need to add CONTENT_EXPORT for Params and constructor/destructor https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/fi... content/renderer/p2p/filtering_network_manager.h:45: struct CONTENT_EXPORT Params { Do you really need this struct? It doesn't really make anything better, especially if you remove task_runner as I suggested in my other comments. https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/po... File content/renderer/p2p/port_allocator.h (right): https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/po... content/renderer/p2p/port_allocator.h:48: struct Params { Do you really need to add this? Does it need to be in this CL?
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
PTAL. https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/em... File content/renderer/p2p/empty_network_manager.cc (right): https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/em... content/renderer/p2p/empty_network_manager.cc:16: const scoped_refptr<base::SingleThreadTaskRunner>& task_runner) On 2015/09/21 22:12:58, Sergey Ulanov wrote: > You use |task_runner| just to check that this class is used on the right thread. > Use base::ThreadChecker for that. You just need to call DetachFromThread() in > the constructor. Then it will be attached automatically to the next thread this > class is called on. Done. https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/em... content/renderer/p2p/empty_network_manager.cc:31: start_updating_time_ = base::TimeTicks::Now(); On 2015/09/21 22:12:58, Sergey Ulanov wrote: > I don't think you actually need to store this value. Instead just store a > boolean flag to remember that the event has been fired. Done. https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/em... content/renderer/p2p/empty_network_manager.cc:42: if (!networks) On 2015/09/21 22:12:58, Sergey Ulanov wrote: > Do we expect |networks| to be nullptr? Done. https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/em... File content/renderer/p2p/empty_network_manager.h (right): https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/em... content/renderer/p2p/empty_network_manager.h:27: public base::SupportsWeakPtr<EmptyNetworkManager>, On 2015/09/21 22:12:58, Sergey Ulanov wrote: > It's preferrable to use WeakPtrFactory instead of inheriting from > SupportsWeakPtr. Done. https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/em... content/renderer/p2p/empty_network_manager.h:28: public sigslot::has_slots<> { On 2015/09/21 22:12:58, Sergey Ulanov wrote: > Don't need has_slots<> Done. https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/em... content/renderer/p2p/empty_network_manager.h:32: CONTENT_EXPORT EmptyNetworkManager( On 2015/09/21 22:12:58, Sergey Ulanov wrote: > Mark the whole class as CONTENT_EXPORT? Done. https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/fi... File content/renderer/p2p/filtering_network_manager.cc (right): https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/fi... content/renderer/p2p/filtering_network_manager.cc:11: #include "content/renderer/media/media_permission_dispatcher_proxy.h" On 2015/09/21 22:12:58, Sergey Ulanov wrote: > I don't see this being used anywhere Done. https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/fi... content/renderer/p2p/filtering_network_manager.cc:40: task_runner_->PostTask(FROM_HERE, On 2015/09/21 22:12:58, Sergey Ulanov wrote: > Is it possible to call CheckPermissions() the first time StartUpdating() is > called? > This is the only place where |task_runner_| is used to post tasks. So if you > call CheckPermissions() the first time StartUpdating() is called then you > wouldn't need task_runner (base::ThreadChecker can be used to verify that all > methods are called on the right thread) Problem with that approach is it serializes the permission check after StartUpdating(). In my test, it increases 10 to 15 ms call set up time on my dev machine and would expect to be even longer on lower end machines. https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/fi... content/renderer/p2p/filtering_network_manager.cc:90: if (!networks) On 2015/09/21 22:12:58, Sergey Ulanov wrote: > I don't think you need this check. This method is not allowed to be called with > nullptr Done. https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/fi... File content/renderer/p2p/filtering_network_manager.h (right): https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/fi... content/renderer/p2p/filtering_network_manager.h:41: public base::SupportsWeakPtr<FilteringNetworkManager>, On 2015/09/21 22:12:59, Sergey Ulanov wrote: > Please use WeakPtrFactory instead of inheriting from SupportsWeakPtr Done. https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/fi... content/renderer/p2p/filtering_network_manager.h:45: struct CONTENT_EXPORT Params { On 2015/09/21 22:12:58, Sergey Ulanov wrote: > Do you really need this struct? It doesn't really make anything better, > especially if you remove task_runner as I suggested in my other comments. Done. https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/fi... content/renderer/p2p/filtering_network_manager.h:45: struct CONTENT_EXPORT Params { On 2015/09/21 22:12:59, Sergey Ulanov wrote: > Mark FilteringNetworkManager class as CONTENT_EXPORT, then you wouldn't need to > add CONTENT_EXPORT for Params and constructor/destructor Done. https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/po... File content/renderer/p2p/port_allocator.h (right): https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/po... content/renderer/p2p/port_allocator.h:48: struct Params { On 2015/09/21 22:12:59, Sergey Ulanov wrote: > Do you really need to add this? Does it need to be in this CL? Done.
On 2015/09/22 17:56:56, guoweis_chromium wrote: > PTAL. > > https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/em... > File content/renderer/p2p/empty_network_manager.cc (right): > > https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/em... > content/renderer/p2p/empty_network_manager.cc:16: const > scoped_refptr<base::SingleThreadTaskRunner>& task_runner) > On 2015/09/21 22:12:58, Sergey Ulanov wrote: > > You use |task_runner| just to check that this class is used on the right > thread. > > Use base::ThreadChecker for that. You just need to call DetachFromThread() in > > the constructor. Then it will be attached automatically to the next thread > this > > class is called on. > > Done. > > https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/em... > content/renderer/p2p/empty_network_manager.cc:31: start_updating_time_ = > base::TimeTicks::Now(); > On 2015/09/21 22:12:58, Sergey Ulanov wrote: > > I don't think you actually need to store this value. Instead just store a > > boolean flag to remember that the event has been fired. > > Done. > > https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/em... > content/renderer/p2p/empty_network_manager.cc:42: if (!networks) > On 2015/09/21 22:12:58, Sergey Ulanov wrote: > > Do we expect |networks| to be nullptr? > > Done. > > https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/em... > File content/renderer/p2p/empty_network_manager.h (right): > > https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/em... > content/renderer/p2p/empty_network_manager.h:27: public > base::SupportsWeakPtr<EmptyNetworkManager>, > On 2015/09/21 22:12:58, Sergey Ulanov wrote: > > It's preferrable to use WeakPtrFactory instead of inheriting from > > SupportsWeakPtr. > > Done. > > https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/em... > content/renderer/p2p/empty_network_manager.h:28: public sigslot::has_slots<> { > On 2015/09/21 22:12:58, Sergey Ulanov wrote: > > Don't need has_slots<> > > Done. > > https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/em... > content/renderer/p2p/empty_network_manager.h:32: CONTENT_EXPORT > EmptyNetworkManager( > On 2015/09/21 22:12:58, Sergey Ulanov wrote: > > Mark the whole class as CONTENT_EXPORT? > > Done. > > https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/fi... > File content/renderer/p2p/filtering_network_manager.cc (right): > > https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/fi... > content/renderer/p2p/filtering_network_manager.cc:11: #include > "content/renderer/media/media_permission_dispatcher_proxy.h" > On 2015/09/21 22:12:58, Sergey Ulanov wrote: > > I don't see this being used anywhere > > Done. > > https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/fi... > content/renderer/p2p/filtering_network_manager.cc:40: > task_runner_->PostTask(FROM_HERE, > On 2015/09/21 22:12:58, Sergey Ulanov wrote: > > Is it possible to call CheckPermissions() the first time StartUpdating() is > > called? > > This is the only place where |task_runner_| is used to post tasks. So if you > > call CheckPermissions() the first time StartUpdating() is called then you > > wouldn't need task_runner (base::ThreadChecker can be used to verify that all > > methods are called on the right thread) > > Problem with that approach is it serializes the permission check after > StartUpdating(). In my test, it increases 10 to 15 ms call set up time on my dev > machine and would expect to be even longer on lower end machines. > > https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/fi... > content/renderer/p2p/filtering_network_manager.cc:90: if (!networks) > On 2015/09/21 22:12:58, Sergey Ulanov wrote: > > I don't think you need this check. This method is not allowed to be called > with > > nullptr > > Done. > > https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/fi... > File content/renderer/p2p/filtering_network_manager.h (right): > > https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/fi... > content/renderer/p2p/filtering_network_manager.h:41: public > base::SupportsWeakPtr<FilteringNetworkManager>, > On 2015/09/21 22:12:59, Sergey Ulanov wrote: > > Please use WeakPtrFactory instead of inheriting from SupportsWeakPtr > > Done. > > https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/fi... > content/renderer/p2p/filtering_network_manager.h:45: struct CONTENT_EXPORT > Params { > On 2015/09/21 22:12:58, Sergey Ulanov wrote: > > Do you really need this struct? It doesn't really make anything better, > > especially if you remove task_runner as I suggested in my other comments. > > Done. > > https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/fi... > content/renderer/p2p/filtering_network_manager.h:45: struct CONTENT_EXPORT > Params { > On 2015/09/21 22:12:59, Sergey Ulanov wrote: > > Mark FilteringNetworkManager class as CONTENT_EXPORT, then you wouldn't need > to > > add CONTENT_EXPORT for Params and constructor/destructor > > Done. > > https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/po... > File content/renderer/p2p/port_allocator.h (right): > > https://codereview.chromium.org/1349823004/diff/20001/content/renderer/p2p/po... > content/renderer/p2p/port_allocator.h:48: struct Params { > On 2015/09/21 22:12:59, Sergey Ulanov wrote: > > Do you really need to add this? Does it need to be in this CL? > > Done. Ping.
Mostly style nits, except for the issue in port_allocator.cc https://codereview.chromium.org/1349823004/diff/100001/content/renderer/media... File content/renderer/media/webrtc/peer_connection_dependency_factory.cc (right): https://codereview.chromium.org/1349823004/diff/100001/content/renderer/media... content/renderer/media/webrtc/peer_connection_dependency_factory.cc:117: scoped_ptr<media::MediaPermission>& media_permission, this shouldn't be a reference. scoped_ptr<> is usually passed by value. https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/e... File content/renderer/p2p/empty_network_manager.h (right): https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/e... content/renderer/p2p/empty_network_manager.h:5: // A NetworkManager implementation which handles the case where local address nit: It's better to put this comment just above the class definition https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/e... content/renderer/p2p/empty_network_manager.h:14: #include "base/time/time.h" don't need this https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/e... content/renderer/p2p/empty_network_manager.h:17: #include "third_party/webrtc/base/sigslot.h" don't need this https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/e... content/renderer/p2p/empty_network_manager.h:18: #include "url/gurl.h" don't need this https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/e... content/renderer/p2p/empty_network_manager.h:21: class NetworkManager; don't need this - network.h is included above. https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... File content/renderer/p2p/filtering_network_manager.cc (right): https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... content/renderer/p2p/filtering_network_manager.cc:33: pending_permission_checks_ = 2; best to move this to CheckPermissions() where the permission requests are made. https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... content/renderer/p2p/filtering_network_manager.cc:35: base::Bind(&FilteringNetworkManager::CheckPermissions, Maybe rename CheckPermission() to Initialize() and then move this PostTask() call to P2PPortAllocatorFactory() constructor, where FilteringNetworkManager is created? That way you can avoid having task_runner parameter. NetworkManager implementations should be single threaded and shouldn't need to jump from one thread to another. Also you can move media_permission parameter to the Initialize() method then you wouldn't need to store it in a member. https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... content/renderer/p2p/filtering_network_manager.cc:124: if (pending_permission_checks_ == 0 && nit: !pending_permission_checks_ https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... File content/renderer/p2p/filtering_network_manager.h (right): https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... content/renderer/p2p/filtering_network_manager.h:5: // FilteringNetworkManager exposes rtc::NetworkManager to nit: Move this just above class definition https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... content/renderer/p2p/filtering_network_manager.h:22: #include "base/single_thread_task_runner.h" nit: move this to the .cc file. SingleThreadTaskRunner can be forward-declared. https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... content/renderer/p2p/filtering_network_manager.h:26: #include "media/base/media_permission.h" nit: move this to the .cc file. MediaPermission can be forward-declared. https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... content/renderer/p2p/filtering_network_manager.h:32: class NetworkManager; don't need this - network.h is included above. https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... content/renderer/p2p/filtering_network_manager.h:37: class MediaPermissionDispatcherProxy; don't need this. https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... File content/renderer/p2p/filtering_network_manager_unittest.cc (right): https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... content/renderer/p2p/filtering_network_manager_unittest.cc:139: network_manager_.reset(new EmptyNetworkManager()); This is a test for FilteringNetworkManager. I don't think you need to test EmptyNetworkManager here. https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... content/renderer/p2p/filtering_network_manager_unittest.cc:174: while (task_runner_->HasPendingTask()) task_runner_->RunUntilIdle() https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/n... File content/renderer/p2p/network_manager_uma.h (right): https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/n... content/renderer/p2p/network_manager_uma.h:15: enum IPPermissionStatus { I'm not sure it's worth adding an extra file just for this enum and the two functions below. Maybe put it all in filtering_network_manager.h? https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/n... content/renderer/p2p/network_manager_uma.h:24: void ReportTimeToUpdateNetwork(const base::TimeDelta& ticks); maybe calle it ReportTimeToUpdateNetworkList() https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/p... File content/renderer/p2p/port_allocator.cc (right): https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/p... content/renderer/p2p/port_allocator.cc:58: network_manager_task_runner_->DeleteSoon(FROM_HERE, If you need this it means P2PPortAllocator is deleted on the wrong thread. PortAllocator is single-threaded interface and so this class should be deleted on the worker thread on which it's being used. Is this something that can be fixed easily? If not then please add open a bug and add TODO to fix it later.
Patchset #4 (id:140001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:160001) has been deleted
Patchset #3 (id:180001) has been deleted
PTAL. https://codereview.chromium.org/1349823004/diff/100001/content/renderer/media... File content/renderer/media/webrtc/peer_connection_dependency_factory.cc (right): https://codereview.chromium.org/1349823004/diff/100001/content/renderer/media... content/renderer/media/webrtc/peer_connection_dependency_factory.cc:117: scoped_ptr<media::MediaPermission>& media_permission, On 2015/09/24 19:37:28, Sergey Ulanov wrote: > this shouldn't be a reference. scoped_ptr<> is usually passed by value. Added comment to explain why it is this way. https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/e... File content/renderer/p2p/empty_network_manager.h (right): https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/e... content/renderer/p2p/empty_network_manager.h:5: // A NetworkManager implementation which handles the case where local address On 2015/09/24 19:37:28, Sergey Ulanov wrote: > nit: It's better to put this comment just above the class definition Done. https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/e... content/renderer/p2p/empty_network_manager.h:14: #include "base/time/time.h" On 2015/09/24 19:37:28, Sergey Ulanov wrote: > don't need this Done. https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/e... content/renderer/p2p/empty_network_manager.h:17: #include "third_party/webrtc/base/sigslot.h" On 2015/09/24 19:37:28, Sergey Ulanov wrote: > don't need this Done. https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/e... content/renderer/p2p/empty_network_manager.h:18: #include "url/gurl.h" On 2015/09/24 19:37:28, Sergey Ulanov wrote: > don't need this Done. https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/e... content/renderer/p2p/empty_network_manager.h:21: class NetworkManager; On 2015/09/24 19:37:28, Sergey Ulanov wrote: > don't need this - network.h is included above. Done. https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... File content/renderer/p2p/filtering_network_manager.cc (right): https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... content/renderer/p2p/filtering_network_manager.cc:33: pending_permission_checks_ = 2; On 2015/09/24 19:37:28, Sergey Ulanov wrote: > best to move this to CheckPermissions() where the permission requests are made. This line above (setting the pending checks to 2) can't happen inside the CheckPermission as StartUpdating() might be called earlier than that and treat as non-pending check situation. https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... content/renderer/p2p/filtering_network_manager.cc:35: base::Bind(&FilteringNetworkManager::CheckPermissions, On 2015/09/24 19:37:28, Sergey Ulanov wrote: > Maybe rename CheckPermission() to Initialize() and then move this PostTask() > call to P2PPortAllocatorFactory() constructor, where FilteringNetworkManager is > created? That way you can avoid having task_runner parameter. NetworkManager > implementations should be single threaded and shouldn't need to jump from one > thread to another. > > Also you can move media_permission parameter to the Initialize() method then you > wouldn't need to store it in a member. I kept the name. Let me know if you feel strong about it. https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... content/renderer/p2p/filtering_network_manager.cc:124: if (pending_permission_checks_ == 0 && On 2015/09/24 19:37:28, Sergey Ulanov wrote: > nit: !pending_permission_checks_ Done. https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... File content/renderer/p2p/filtering_network_manager.h (right): https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... content/renderer/p2p/filtering_network_manager.h:5: // FilteringNetworkManager exposes rtc::NetworkManager to On 2015/09/24 19:37:29, Sergey Ulanov wrote: > nit: Move this just above class definition Done. https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... content/renderer/p2p/filtering_network_manager.h:22: #include "base/single_thread_task_runner.h" On 2015/09/24 19:37:29, Sergey Ulanov wrote: > nit: move this to the .cc file. SingleThreadTaskRunner can be forward-declared. Done. https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... content/renderer/p2p/filtering_network_manager.h:26: #include "media/base/media_permission.h" On 2015/09/24 19:37:29, Sergey Ulanov wrote: > nit: move this to the .cc file. MediaPermission can be forward-declared. Done. https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... content/renderer/p2p/filtering_network_manager.h:32: class NetworkManager; On 2015/09/24 19:37:29, Sergey Ulanov wrote: > don't need this - network.h is included above. Done. https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... content/renderer/p2p/filtering_network_manager.h:37: class MediaPermissionDispatcherProxy; On 2015/09/24 19:37:29, Sergey Ulanov wrote: > don't need this. Done. https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... File content/renderer/p2p/filtering_network_manager_unittest.cc (right): https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... content/renderer/p2p/filtering_network_manager_unittest.cc:139: network_manager_.reset(new EmptyNetworkManager()); On 2015/09/24 19:37:29, Sergey Ulanov wrote: > This is a test for FilteringNetworkManager. I don't think you need to test > EmptyNetworkManager here. I do have a test case which just tests for this. Since they share the same mocks, I didn't separate them into different files. Hope it's ok. https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... content/renderer/p2p/filtering_network_manager_unittest.cc:174: while (task_runner_->HasPendingTask()) On 2015/09/24 19:37:29, Sergey Ulanov wrote: > task_runner_->RunUntilIdle() Done. https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/n... File content/renderer/p2p/network_manager_uma.h (right): https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/n... content/renderer/p2p/network_manager_uma.h:15: enum IPPermissionStatus { On 2015/09/24 19:37:29, Sergey Ulanov wrote: > I'm not sure it's worth adding an extra file just for this enum and the two > functions below. Maybe put it all in filtering_network_manager.h? I need this in both EmptyNetworkManager as well as FilteringNetworkManager https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/n... content/renderer/p2p/network_manager_uma.h:24: void ReportTimeToUpdateNetwork(const base::TimeDelta& ticks); On 2015/09/24 19:37:29, Sergey Ulanov wrote: > maybe calle it ReportTimeToUpdateNetworkList() Done. https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/p... File content/renderer/p2p/port_allocator.cc (right): https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/p... content/renderer/p2p/port_allocator.cc:58: network_manager_task_runner_->DeleteSoon(FROM_HERE, On 2015/09/24 19:37:29, Sergey Ulanov wrote: > If you need this it means P2PPortAllocator is deleted on the wrong thread. > PortAllocator is single-threaded interface and so this class should be deleted > on the worker thread on which it's being used. > > Is this something that can be fixed easily? If not then please add open a bug > and add TODO to fix it later. Done.
On 2015/09/24 23:06:01, guoweis_chromium wrote: > PTAL. > > https://codereview.chromium.org/1349823004/diff/100001/content/renderer/media... > File content/renderer/media/webrtc/peer_connection_dependency_factory.cc > (right): > > https://codereview.chromium.org/1349823004/diff/100001/content/renderer/media... > content/renderer/media/webrtc/peer_connection_dependency_factory.cc:117: > scoped_ptr<media::MediaPermission>& media_permission, > On 2015/09/24 19:37:28, Sergey Ulanov wrote: > > this shouldn't be a reference. scoped_ptr<> is usually passed by value. > > Added comment to explain why it is this way. > > https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/e... > File content/renderer/p2p/empty_network_manager.h (right): > > https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/e... > content/renderer/p2p/empty_network_manager.h:5: // A NetworkManager > implementation which handles the case where local address > On 2015/09/24 19:37:28, Sergey Ulanov wrote: > > nit: It's better to put this comment just above the class definition > > Done. > > https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/e... > content/renderer/p2p/empty_network_manager.h:14: #include "base/time/time.h" > On 2015/09/24 19:37:28, Sergey Ulanov wrote: > > don't need this > > Done. > > https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/e... > content/renderer/p2p/empty_network_manager.h:17: #include > "third_party/webrtc/base/sigslot.h" > On 2015/09/24 19:37:28, Sergey Ulanov wrote: > > don't need this > > Done. > > https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/e... > content/renderer/p2p/empty_network_manager.h:18: #include "url/gurl.h" > On 2015/09/24 19:37:28, Sergey Ulanov wrote: > > don't need this > > Done. > > https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/e... > content/renderer/p2p/empty_network_manager.h:21: class NetworkManager; > On 2015/09/24 19:37:28, Sergey Ulanov wrote: > > don't need this - network.h is included above. > > Done. > > https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... > File content/renderer/p2p/filtering_network_manager.cc (right): > > https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... > content/renderer/p2p/filtering_network_manager.cc:33: pending_permission_checks_ > = 2; > On 2015/09/24 19:37:28, Sergey Ulanov wrote: > > best to move this to CheckPermissions() where the permission requests are > made. > > This line above (setting the pending checks to 2) can't happen inside the > CheckPermission as StartUpdating() might be called earlier than that and treat > as non-pending check situation. Please ignore the comment above. That was incorrect. > > https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... > content/renderer/p2p/filtering_network_manager.cc:35: > base::Bind(&FilteringNetworkManager::CheckPermissions, > On 2015/09/24 19:37:28, Sergey Ulanov wrote: > > Maybe rename CheckPermission() to Initialize() and then move this PostTask() > > call to P2PPortAllocatorFactory() constructor, where FilteringNetworkManager > is > > created? That way you can avoid having task_runner parameter. NetworkManager > > implementations should be single threaded and shouldn't need to jump from one > > thread to another. > > > > Also you can move media_permission parameter to the Initialize() method then > you > > wouldn't need to store it in a member. > > I kept the name. Let me know if you feel strong about it. > > https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... > content/renderer/p2p/filtering_network_manager.cc:124: if > (pending_permission_checks_ == 0 && > On 2015/09/24 19:37:28, Sergey Ulanov wrote: > > nit: !pending_permission_checks_ > > Done. > > https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... > File content/renderer/p2p/filtering_network_manager.h (right): > > https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... > content/renderer/p2p/filtering_network_manager.h:5: // FilteringNetworkManager > exposes rtc::NetworkManager to > On 2015/09/24 19:37:29, Sergey Ulanov wrote: > > nit: Move this just above class definition > > Done. > > https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... > content/renderer/p2p/filtering_network_manager.h:22: #include > "base/single_thread_task_runner.h" > On 2015/09/24 19:37:29, Sergey Ulanov wrote: > > nit: move this to the .cc file. SingleThreadTaskRunner can be > forward-declared. > > Done. > > https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... > content/renderer/p2p/filtering_network_manager.h:26: #include > "media/base/media_permission.h" > On 2015/09/24 19:37:29, Sergey Ulanov wrote: > > nit: move this to the .cc file. MediaPermission can be forward-declared. > > Done. > > https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... > content/renderer/p2p/filtering_network_manager.h:32: class NetworkManager; > On 2015/09/24 19:37:29, Sergey Ulanov wrote: > > don't need this - network.h is included above. > > Done. > > https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... > content/renderer/p2p/filtering_network_manager.h:37: class > MediaPermissionDispatcherProxy; > On 2015/09/24 19:37:29, Sergey Ulanov wrote: > > don't need this. > > Done. > > https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... > File content/renderer/p2p/filtering_network_manager_unittest.cc (right): > > https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... > content/renderer/p2p/filtering_network_manager_unittest.cc:139: > network_manager_.reset(new EmptyNetworkManager()); > On 2015/09/24 19:37:29, Sergey Ulanov wrote: > > This is a test for FilteringNetworkManager. I don't think you need to test > > EmptyNetworkManager here. > > I do have a test case which just tests for this. Since they share the same > mocks, I didn't separate them into different files. Hope it's ok. > > https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... > content/renderer/p2p/filtering_network_manager_unittest.cc:174: while > (task_runner_->HasPendingTask()) > On 2015/09/24 19:37:29, Sergey Ulanov wrote: > > task_runner_->RunUntilIdle() > > Done. > > https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/n... > File content/renderer/p2p/network_manager_uma.h (right): > > https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/n... > content/renderer/p2p/network_manager_uma.h:15: enum IPPermissionStatus { > On 2015/09/24 19:37:29, Sergey Ulanov wrote: > > I'm not sure it's worth adding an extra file just for this enum and the two > > functions below. Maybe put it all in filtering_network_manager.h? > > I need this in both EmptyNetworkManager as well as FilteringNetworkManager > > https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/n... > content/renderer/p2p/network_manager_uma.h:24: void > ReportTimeToUpdateNetwork(const base::TimeDelta& ticks); > On 2015/09/24 19:37:29, Sergey Ulanov wrote: > > maybe calle it ReportTimeToUpdateNetworkList() > > Done. > > https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/p... > File content/renderer/p2p/port_allocator.cc (right): > > https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/p... > content/renderer/p2p/port_allocator.cc:58: > network_manager_task_runner_->DeleteSoon(FROM_HERE, > On 2015/09/24 19:37:29, Sergey Ulanov wrote: > > If you need this it means P2PPortAllocator is deleted on the wrong thread. > > PortAllocator is single-threaded interface and so this class should be deleted > > on the worker thread on which it's being used. > > > > Is this something that can be fixed easily? If not then please add open a bug > > and add TODO to fix it later. > > Done.
rkaplow@: please review histograms.xml
guoweis@chromium.org changed reviewers: + rkaplow@chromium.org
+rkaplow for histograms.xml
https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... File content/renderer/p2p/filtering_network_manager.cc (right): https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/f... content/renderer/p2p/filtering_network_manager.cc:33: pending_permission_checks_ = 2; On 2015/09/24 23:06:00, guoweis_chromium wrote: > On 2015/09/24 19:37:28, Sergey Ulanov wrote: > > best to move this to CheckPermissions() where the permission requests are > made. > > This line above (setting the pending checks to 2) can't happen inside the > CheckPermission as StartUpdating() might be called earlier than that and treat > as non-pending check situation. StartUpdating() must be called on the same thread as CheckPermissions(), so it cannot be called before CheckPermission(), if it is then something went wrong. That's why I suggested to rename CheckPermission() to Initialize() - because then it would be reasonable to require that Initialize() is called before anything else. https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/n... File content/renderer/p2p/network_manager_uma.h (right): https://codereview.chromium.org/1349823004/diff/100001/content/renderer/p2p/n... content/renderer/p2p/network_manager_uma.h:15: enum IPPermissionStatus { On 2015/09/24 23:06:01, guoweis_chromium wrote: > On 2015/09/24 19:37:29, Sergey Ulanov wrote: > > I'm not sure it's worth adding an extra file just for this enum and the two > > functions below. Maybe put it all in filtering_network_manager.h? > > I need this in both EmptyNetworkManager as well as FilteringNetworkManager Yes, my suggestion was to include filtering_network_manager from empty_network_manager. But feel free to keep it as is. https://codereview.chromium.org/1349823004/diff/200001/content/renderer/media... File content/renderer/media/webrtc/peer_connection_dependency_factory.cc (right): https://codereview.chromium.org/1349823004/diff/200001/content/renderer/media... content/renderer/media/webrtc/peer_connection_dependency_factory.cc:117: // constructor inside rtc::RefCountedObject. Not sure what's the problem here. Passing scoped_ptr<> by value doesn't require copying the object that's pointed by scoped_ptr<>, so how does it cause copy constructor being invoked? https://codereview.chromium.org/1349823004/diff/200001/content/renderer/media... content/renderer/media/webrtc/peer_connection_dependency_factory.cc:163: static_cast<FilteringNetworkManager*>( Please don't use static_cast to downcast pointers unless it's necessary. You can avoid it here by storing the origin FNM pointer: FilteringNetworkManager* nm = new FilteringNetworkManager(); network_manager.reset(nm); PostTask(base::Bind(..., base::Unretained(nm))); https://codereview.chromium.org/1349823004/diff/200001/content/renderer/media... content/renderer/media/webrtc/peer_connection_dependency_factory.cc:165: ->GetWeakPtr())); You don't need weak_ptr here. The network manager will be destroyed on the same thread, so CheckPermission() will always be called before destruction if you call it here. I.e. you can use base::Unretained. https://codereview.chromium.org/1349823004/diff/200001/content/renderer/p2p/f... File content/renderer/p2p/filtering_network_manager.cc (right): https://codereview.chromium.org/1349823004/diff/200001/content/renderer/p2p/f... content/renderer/p2p/filtering_network_manager.cc:33: void FilteringNetworkManager::CheckPermissions() { This method must always be called before StartUpdating(). Maybe add a flag for that and a DCHECK in StartUpdating()? https://codereview.chromium.org/1349823004/diff/200001/content/renderer/p2p/f... File content/renderer/p2p/filtering_network_manager.h (right): https://codereview.chromium.org/1349823004/diff/200001/content/renderer/p2p/f... content/renderer/p2p/filtering_network_manager.h:46: base::WeakPtr<FilteringNetworkManager> GetWeakPtr(); Don't need this - CheckPermissions() task can be posted using base::Unretained() https://codereview.chromium.org/1349823004/diff/200001/content/renderer/p2p/p... File content/renderer/p2p/port_allocator.cc (right): https://codereview.chromium.org/1349823004/diff/200001/content/renderer/p2p/p... content/renderer/p2p/port_allocator.cc:59: // only used on worker thread. The destructor is invoked on signaling thread Please Also mention that DeleteSoon() below can be removed once this bug is fixed.
PTAL. https://codereview.chromium.org/1349823004/diff/200001/content/renderer/media... File content/renderer/media/webrtc/peer_connection_dependency_factory.cc (right): https://codereview.chromium.org/1349823004/diff/200001/content/renderer/media... content/renderer/media/webrtc/peer_connection_dependency_factory.cc:117: // constructor inside rtc::RefCountedObject. rtc::RefCountedObject is a template as RefCountedObject(P1 p1, P2 p2, P3 p3, P4 p4, P5 p5, P6 p6, P7 p7, P8 p8) : T(p1, p2, p3, p4, p5, p6, p7, p8), ref_count_(0) { } So scoped_ptr<> will be passed to both T (i.e. P2PPortAllocatorFactory here) and RefCountedObject. I guess that's where a copy ctro is invoked. https://codereview.chromium.org/1349823004/diff/200001/content/renderer/media... content/renderer/media/webrtc/peer_connection_dependency_factory.cc:163: static_cast<FilteringNetworkManager*>( On 2015/09/25 00:45:30, Sergey Ulanov wrote: > Please don't use static_cast to downcast pointers unless it's necessary. You can > avoid it here by storing the origin FNM pointer: > > FilteringNetworkManager* nm = new FilteringNetworkManager(); > network_manager.reset(nm); > PostTask(base::Bind(..., base::Unretained(nm))); Done. https://codereview.chromium.org/1349823004/diff/200001/content/renderer/media... content/renderer/media/webrtc/peer_connection_dependency_factory.cc:165: ->GetWeakPtr())); On 2015/09/25 00:45:30, Sergey Ulanov wrote: > You don't need weak_ptr here. The network manager will be destroyed on the same > thread, so CheckPermission() will always be called before destruction if you > call it here. I.e. you can use base::Unretained. Done. https://codereview.chromium.org/1349823004/diff/200001/content/renderer/p2p/f... File content/renderer/p2p/filtering_network_manager.cc (right): https://codereview.chromium.org/1349823004/diff/200001/content/renderer/p2p/f... content/renderer/p2p/filtering_network_manager.cc:33: void FilteringNetworkManager::CheckPermissions() { On 2015/09/25 00:45:31, Sergey Ulanov wrote: > This method must always be called before StartUpdating(). Maybe add a flag for > that and a DCHECK in StartUpdating()? Done. https://codereview.chromium.org/1349823004/diff/200001/content/renderer/p2p/f... File content/renderer/p2p/filtering_network_manager.h (right): https://codereview.chromium.org/1349823004/diff/200001/content/renderer/p2p/f... content/renderer/p2p/filtering_network_manager.h:46: base::WeakPtr<FilteringNetworkManager> GetWeakPtr(); On 2015/09/25 00:45:31, Sergey Ulanov wrote: > Don't need this - CheckPermissions() task can be posted using base::Unretained() Done. https://codereview.chromium.org/1349823004/diff/200001/content/renderer/p2p/p... File content/renderer/p2p/port_allocator.cc (right): https://codereview.chromium.org/1349823004/diff/200001/content/renderer/p2p/p... content/renderer/p2p/port_allocator.cc:59: // only used on worker thread. The destructor is invoked on signaling thread On 2015/09/25 00:45:31, Sergey Ulanov wrote: > Please Also mention that DeleteSoon() below can be removed once this bug is > fixed. Done.
Patchset #4 (id:220001) has been deleted
https://codereview.chromium.org/1349823004/diff/190014/content/renderer/media... File content/renderer/media/webrtc/peer_connection_dependency_factory.cc (right): https://codereview.chromium.org/1349823004/diff/190014/content/renderer/media... content/renderer/media/webrtc/peer_connection_dependency_factory.cc:460: "Enabled" && instead of == "Enabled", best practice is to use StartsWith. This allows more flexibility https://codereview.chromium.org/1349823004/diff/190014/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1349823004/diff/190014/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:51911: + requested and/or granted. This is collected in the first time when networks remove 'in'
rkaplow@, PTAL. https://codereview.chromium.org/1349823004/diff/190014/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1349823004/diff/190014/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:51911: + requested and/or granted. This is collected in the first time when networks On 2015/09/25 14:46:03, rkaplow wrote: > remove 'in' Done.
https://codereview.chromium.org/1349823004/diff/200001/content/renderer/media... File content/renderer/media/webrtc/peer_connection_dependency_factory.cc (right): https://codereview.chromium.org/1349823004/diff/200001/content/renderer/media... content/renderer/media/webrtc/peer_connection_dependency_factory.cc:114: class P2PPortAllocatorFactory : public webrtc::PortAllocatorFactoryInterface { I thinks you can just change this to 'public rtc::RefCountedObject<webrtc::PortAllocatorFactoryInterface>'. This will allow to avoid the problem with RefCountedObject constructor. https://codereview.chromium.org/1349823004/diff/200001/content/renderer/media... content/renderer/media/webrtc/peer_connection_dependency_factory.cc:117: // constructor inside rtc::RefCountedObject. On 2015/09/25 03:42:20, guoweis_chromium wrote: > rtc::RefCountedObject is a template as > RefCountedObject(P1 p1, P2 p2, P3 p3, P4 p4, P5 p5, P6 p6, P7 p7, P8 p8) > : T(p1, p2, p3, p4, p5, p6, p7, p8), ref_count_(0) { > } > > So scoped_ptr<> will be passed to both T (i.e. P2PPortAllocatorFactory here) and > RefCountedObject. I guess that's where a copy ctro is invoked. I see. So the problem is that rtc::RefCountedOject is not compatible with non-copyable constructor parameters. In either case it's not right to pass scoped_ptr<> by reference like this. Style guide explicitly prohibits non-const reference parameters: https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Ar... See my comment above for suggestion to how this problem can be avoided. But if that doesn't work pass media_permission here as raw pointer. https://codereview.chromium.org/1349823004/diff/200001/content/renderer/media... content/renderer/media/webrtc/peer_connection_dependency_factory.cc:476: new rtc::RefCountedObject<P2PPortAllocatorFactory>( this can be 'new P2PPortAllocatorFactory' if P2PPortAllocatorFactory inherited from RefCountedObject.
On 2015/09/25 18:19:20, Sergey Ulanov wrote: > https://codereview.chromium.org/1349823004/diff/200001/content/renderer/media... > File content/renderer/media/webrtc/peer_connection_dependency_factory.cc > (right): > > https://codereview.chromium.org/1349823004/diff/200001/content/renderer/media... > content/renderer/media/webrtc/peer_connection_dependency_factory.cc:114: class > P2PPortAllocatorFactory : public webrtc::PortAllocatorFactoryInterface { > I thinks you can just change this to 'public > rtc::RefCountedObject<webrtc::PortAllocatorFactoryInterface>'. > This will allow to avoid the problem with RefCountedObject constructor. > > https://codereview.chromium.org/1349823004/diff/200001/content/renderer/media... > content/renderer/media/webrtc/peer_connection_dependency_factory.cc:117: // > constructor inside rtc::RefCountedObject. > On 2015/09/25 03:42:20, guoweis_chromium wrote: > > rtc::RefCountedObject is a template as > > RefCountedObject(P1 p1, P2 p2, P3 p3, P4 p4, P5 p5, P6 p6, P7 p7, P8 p8) > > : T(p1, p2, p3, p4, p5, p6, p7, p8), ref_count_(0) { > > } > > > > So scoped_ptr<> will be passed to both T (i.e. P2PPortAllocatorFactory here) > and > > RefCountedObject. I guess that's where a copy ctro is invoked. > > I see. So the problem is that rtc::RefCountedOject is not compatible with > non-copyable constructor parameters. > In either case it's not right to pass scoped_ptr<> by reference like this. Style > guide explicitly prohibits non-const reference parameters: > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Ar... > > See my comment above for suggestion to how this problem can be avoided. But if > that doesn't work pass media_permission here as raw pointer. > > https://codereview.chromium.org/1349823004/diff/200001/content/renderer/media... > content/renderer/media/webrtc/peer_connection_dependency_factory.cc:476: new > rtc::RefCountedObject<P2PPortAllocatorFactory>( > this can be 'new P2PPortAllocatorFactory' if P2PPortAllocatorFactory inherited > from RefCountedObject. Cool. that fixed the problem. PTAL.
lgtm histogram lgtm
https://codereview.chromium.org/1349823004/diff/270001/content/renderer/media... File content/renderer/media/webrtc/peer_connection_dependency_factory.cc (right): https://codereview.chromium.org/1349823004/diff/270001/content/renderer/media... content/renderer/media/webrtc/peer_connection_dependency_factory.cc:120: const scoped_refptr<P2PSocketDispatcher> socket_dispatcher, it's fine to pass scoped_refptr<> as const-ref (the way it was before - it actually helps to avoid extra ref-count increment/decrement when calling the function), but if you are not passing it as reference, then you don't need const here. https://codereview.chromium.org/1349823004/diff/270001/content/renderer/media... content/renderer/media/webrtc/peer_connection_dependency_factory.cc:184: const scoped_refptr<P2PSocketDispatcher> socket_dispatcher_; nit: don't need const https://codereview.chromium.org/1349823004/diff/270001/content/renderer/media... content/renderer/media/webrtc/peer_connection_dependency_factory.cc:187: const P2PPortAllocator::Config& config_; This shouldn't be a reference. Never store references as class members. Same for origin_ below.
On 2015/09/25 19:11:44, Sergey Ulanov wrote: > https://codereview.chromium.org/1349823004/diff/270001/content/renderer/media... > File content/renderer/media/webrtc/peer_connection_dependency_factory.cc > (right): > > https://codereview.chromium.org/1349823004/diff/270001/content/renderer/media... > content/renderer/media/webrtc/peer_connection_dependency_factory.cc:120: const > scoped_refptr<P2PSocketDispatcher> socket_dispatcher, > it's fine to pass scoped_refptr<> as const-ref (the way it was before - it > actually helps to avoid extra ref-count increment/decrement when calling the > function), but if you are not passing it as reference, then you don't need const > here. > > https://codereview.chromium.org/1349823004/diff/270001/content/renderer/media... > content/renderer/media/webrtc/peer_connection_dependency_factory.cc:184: const > scoped_refptr<P2PSocketDispatcher> socket_dispatcher_; > nit: don't need const > > https://codereview.chromium.org/1349823004/diff/270001/content/renderer/media... > content/renderer/media/webrtc/peer_connection_dependency_factory.cc:187: const > P2PPortAllocator::Config& config_; > This shouldn't be a reference. Never store references as class members. Same for > origin_ below. Done.
lgtm https://codereview.chromium.org/1349823004/diff/330001/content/renderer/media... File content/renderer/media/webrtc/peer_connection_dependency_factory.cc (right): https://codereview.chromium.org/1349823004/diff/330001/content/renderer/media... content/renderer/media/webrtc/peer_connection_dependency_factory.cc:187: const P2PPortAllocator::Config config_; nit: don't need const here and for members below.
On 2015/09/25 22:11:23, Sergey Ulanov wrote: > lgtm > > https://codereview.chromium.org/1349823004/diff/330001/content/renderer/media... > File content/renderer/media/webrtc/peer_connection_dependency_factory.cc > (right): > > https://codereview.chromium.org/1349823004/diff/330001/content/renderer/media... > content/renderer/media/webrtc/peer_connection_dependency_factory.cc:187: const > P2PPortAllocator::Config config_; > nit: don't need const here and for members below. Thanks. Landing this now.
The CQ bit was checked by guoweis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/1349823004/#ps350001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349823004/350001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349823004/350001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by guoweis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/1349823004/#ps370001 (title: "rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349823004/370001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349823004/370001
The CQ bit was unchecked by guoweis@chromium.org
The CQ bit was checked by guoweis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/1349823004/#ps390001 (title: "fix test issue.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349823004/390001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349823004/390001
The CQ bit was unchecked by guoweis@chromium.org
Patchset #11 (id:370001) has been deleted
Patchset #11 (id:390001) has been deleted
The CQ bit was checked by guoweis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/1349823004/#ps410001 (title: "rebase, fix test issue, fix windows build issue.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349823004/410001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349823004/410001
The CQ bit was unchecked by guoweis@chromium.org
The CQ bit was checked by guoweis@chromium.org
The CQ bit was unchecked by guoweis@chromium.org
The CQ bit was checked by guoweis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349823004/410001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349823004/410001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by guoweis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349823004/410001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349823004/410001
Message was sent while issue was closed.
Committed patchset #11 (id:410001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/8c1b85686e9e30179087c6767b6e2bc4684cf8e4 Cr-Commit-Position: refs/heads/master@{#351187} |
