Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
536133003:
MergeNetworkList can't group address under the same interface (Closed)
|
Created:
6 years, 3 months ago by guoweis_webrtc Modified:
6 years, 3 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, juberti2, Sergey Ulanov, jln (very slow on Chromium), pthatcher1, jiayl, guoweis_left_chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionIpcNetworkManager in renderer/p2p didn't specify the right network prefix,
which prevent grouping mechanism in MergeNetworkList from working.
The end result is that each Network after MergeNetworkList contains only 1 ip address. With this change, the grouping will happen correctly. WebRTC has logic to only retrieve single IP from each network for connectivity check already.
Patch Set 1 #
Total comments: 3
Patch Set 2 : add a new unittest for the change #Patch Set 3 : add more comment in test case. #Patch Set 4 : add virtual dtor to the interface #Patch Set 5 : Polish the unit test #Patch Set 6 : clarify a comment #Patch Set 7 : Disable a unit test until webrtc fix is merged into chrome #
Total comments: 20
Patch Set 8 : address comments by SergeyU #
Total comments: 6
Patch Set 9 : address further comments #Patch Set 10 : fix one more comment #
Total comments: 16
Patch Set 11 : Change prefix from size_t to unsigned int per @wfh's feedback comment #
Total comments: 3
Patch Set 12 : Change the prefix length type from unsigned int to uint32 #
Messages
Total messages: 50 (18 generated)
guoweis@webrtc.org changed reviewers: + juberti@chromium.org, sergeyu@chromium.org
https://codereview.chromium.org/536133003/diff/1/content/renderer/p2p/ipc_net... File content/renderer/p2p/ipc_network_manager.cc (right): https://codereview.chromium.org/536133003/diff/1/content/renderer/p2p/ipc_net... content/renderer/p2p/ipc_network_manager.cc:87: rtc::IPAddress prefix = rtc::TruncateIP(rtc::IPAddress(address), Do you really need to truncate the prefix? Shouldn't rtc::Network() just ignore all the bits in the address after the first it->network_prefix bits?
https://codereview.chromium.org/536133003/diff/1/content/renderer/p2p/ipc_net... File content/renderer/p2p/ipc_network_manager.cc (right): https://codereview.chromium.org/536133003/diff/1/content/renderer/p2p/ipc_net... content/renderer/p2p/ipc_network_manager.cc:87: rtc::IPAddress prefix = rtc::TruncateIP(rtc::IPAddress(address), On 2014/09/03 22:55:22, Sergey Ulanov wrote: > Do you really need to truncate the prefix? Shouldn't rtc::Network() just ignore > all the bits in the address after the first it->network_prefix bits? Yes, I do want to truncate the address. Network is modeled against an interface and passing one of multiple addresses into its ctor doesn't seem right to me. The Network class ctor is correctly defined as passing prefix which is the right thing to do here.
Can we write up a unit test for this? https://codereview.chromium.org/536133003/diff/1/content/renderer/p2p/ipc_net... File content/renderer/p2p/ipc_network_manager.cc (right): https://codereview.chromium.org/536133003/diff/1/content/renderer/p2p/ipc_net... content/renderer/p2p/ipc_network_manager.cc:87: rtc::IPAddress prefix = rtc::TruncateIP(rtc::IPAddress(address), On 2014/09/03 22:58:14, guoweis wrote: > On 2014/09/03 22:55:22, Sergey Ulanov wrote: > > Do you really need to truncate the prefix? Shouldn't rtc::Network() just > ignore > > all the bits in the address after the first it->network_prefix bits? > > Yes, I do want to truncate the address. Network is modeled against an interface > and passing one of multiple addresses into its ctor doesn't seem right to me. > The Network class ctor is correctly defined as passing prefix which is the right > thing to do here. It does seem unnecessary to pass both |prefix| and |network_prefix| into Network. I would think just |prefix| would be sufficient.
ah, understand now. Sergey, apologize if I misunderstood your point. I'll have a test case for this in next round.
btw, maybe I reply too fast. If you don't pass both prefix and prefix length, then what if the prefix ends with 0? for example, 172.0.0.0/24? The standard way to specify subnet is always prefix and length, correct?
On 2014/09/03 23:21:13, guoweis wrote: > btw, maybe I reply too fast. If you don't pass both prefix and prefix length, > then what if the prefix ends with 0? > > for example, 172.0.0.0/24? The standard way to specify subnet is always prefix > and length, correct? Yes, you are correct. It's not always possible to compute length from the truncated address.
On 2014/09/03 23:25:34, juberti2 wrote: > On 2014/09/03 23:21:13, guoweis wrote: > > btw, maybe I reply too fast. If you don't pass both prefix and prefix length, > > then what if the prefix ends with 0? > > > > for example, 172.0.0.0/24? The standard way to specify subnet is always prefix > > and length, correct? > > Yes, you are correct. It's not always possible to compute length from the > truncated address. Good that you suggested a test case. I found another bug inside MergeNetworkList. However, I'm not sure how I could 1) upload them into the same issue 2) I probably have to check in the change into webrtc instead chromium. Need to figure the sequence of it. The url for the webrtc change is https://webrtc-codereview.appspot.com/19249005
guoweis@webrtc.org changed reviewers: + jschuh@chromium.org
+content owner for p2p message change
guoweis@webrtc.org changed reviewers: + dcheng@chromium.org
+another p2pmessage owner as jschuh is OOO
guoweis@webrtc.org changed reviewers: + jln@chromium.org - dcheng@chromium.org, jschuh@chromium.org
ok, try one more time with the owner
guoweis@webrtc.org changed reviewers: + pthatcher@webrtc.org
Hi, This one has been outstanding for couple days. Wonder if anyone could take a look and provide feedback? Thanks, Guowei
guoweis@webrtc.org changed reviewers: + jiayl@chromium.org
+Jiayl
sorry about delay. Feel free to ping me over IM if you are waiting for my response for too long. https://codereview.chromium.org/536133003/diff/120001/content/renderer/p2p/ip... File content/renderer/p2p/ipc_network_manager_unittest.cc (right): https://codereview.chromium.org/536133003/diff/120001/content/renderer/p2p/ip... content/renderer/p2p/ipc_network_manager_unittest.cc:11: namespace { nit: tests don't need to be in the anonymous namespace https://codereview.chromium.org/536133003/diff/120001/content/renderer/p2p/ip... content/renderer/p2p/ipc_network_manager_unittest.cc:31: IpcNetworkManager* mgr_; protected:? https://codereview.chromium.org/536133003/diff/120001/content/renderer/p2p/ip... content/renderer/p2p/ipc_network_manager_unittest.cc:51: net::NetworkInterface("em1", "em1", 0, nit: 4 space indentation relative to the previous line. https://codereview.chromium.org/536133003/diff/120001/content/renderer/p2p/mo... File content/renderer/p2p/mock_socket_dispatcher.h (right): https://codereview.chromium.org/536133003/diff/120001/content/renderer/p2p/mo... content/renderer/p2p/mock_socket_dispatcher.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. year remove (c) https://codereview.chromium.org/536133003/diff/120001/content/renderer/p2p/mo... content/renderer/p2p/mock_socket_dispatcher.h:13: class MockP2PSocketDispatcher : public P2PSocketDispatcherInterface { Unless this class is shared between multiple tests it's not worth putting it into a separate file. MOve it to ipc_network_manager_unittest.cc? https://codereview.chromium.org/536133003/diff/120001/content/renderer/p2p/so... File content/renderer/p2p/socket_dispatcher.h (right): https://codereview.chromium.org/536133003/diff/120001/content/renderer/p2p/so... content/renderer/p2p/socket_dispatcher.h:51: class P2PSocketDispatcherInterface { We don't normally use Interface suffix for interfaces. Usage of interface instead of implementation should not be penalized. Instead you can use Impl suffix for the implementation. But in this case it's probably better to give this interface a different name. I suggest NetworkListManager https://codereview.chromium.org/536133003/diff/120001/content/renderer/p2p/so... content/renderer/p2p/socket_dispatcher.h:68: : public IPC::MessageFilter, public P2PSocketDispatcherInterface { indentation. https://codereview.chromium.org/536133003/diff/120001/content/renderer/p2p/so... content/renderer/p2p/socket_dispatcher.h:72: // Each observer is called immidiately after it is registered and This comment should be in the interface definition. In the implementation you just need one like like this: // P2PSocketDispatcher interface:
PTAL https://codereview.chromium.org/536133003/diff/120001/content/renderer/p2p/ip... File content/renderer/p2p/ipc_network_manager_unittest.cc (right): https://codereview.chromium.org/536133003/diff/120001/content/renderer/p2p/ip... content/renderer/p2p/ipc_network_manager_unittest.cc:11: namespace { On 2014/09/10 18:05:32, Sergey Ulanov wrote: > nit: tests don't need to be in the anonymous namespace Done. https://codereview.chromium.org/536133003/diff/120001/content/renderer/p2p/ip... content/renderer/p2p/ipc_network_manager_unittest.cc:31: IpcNetworkManager* mgr_; On 2014/09/10 18:05:32, Sergey Ulanov wrote: > protected:? Done. https://codereview.chromium.org/536133003/diff/120001/content/renderer/p2p/ip... content/renderer/p2p/ipc_network_manager_unittest.cc:51: net::NetworkInterface("em1", "em1", 0, On 2014/09/10 18:05:32, Sergey Ulanov wrote: > nit: 4 space indentation relative to the previous line. Done. https://codereview.chromium.org/536133003/diff/120001/content/renderer/p2p/mo... File content/renderer/p2p/mock_socket_dispatcher.h (right): https://codereview.chromium.org/536133003/diff/120001/content/renderer/p2p/mo... content/renderer/p2p/mock_socket_dispatcher.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/09/10 18:05:32, Sergey Ulanov wrote: > year > remove (c) Done. https://codereview.chromium.org/536133003/diff/120001/content/renderer/p2p/mo... content/renderer/p2p/mock_socket_dispatcher.h:13: class MockP2PSocketDispatcher : public P2PSocketDispatcherInterface { On 2014/09/10 18:05:32, Sergey Ulanov wrote: > Unless this class is shared between multiple tests it's not worth putting it > into a separate file. MOve it to ipc_network_manager_unittest.cc? Done. https://codereview.chromium.org/536133003/diff/120001/content/renderer/p2p/so... File content/renderer/p2p/socket_dispatcher.h (right): https://codereview.chromium.org/536133003/diff/120001/content/renderer/p2p/so... content/renderer/p2p/socket_dispatcher.h:51: class P2PSocketDispatcherInterface { On 2014/09/10 18:05:32, Sergey Ulanov wrote: > We don't normally use Interface suffix for interfaces. Usage of interface > instead of implementation should not be penalized. Instead you can use Impl > suffix for the implementation. > > But in this case it's probably better to give this interface a different name. I > suggest NetworkListManager Since it's the manager of NetworkListObserver, I name it NetworkListObserverManager, hope it's ok. https://codereview.chromium.org/536133003/diff/120001/content/renderer/p2p/so... content/renderer/p2p/socket_dispatcher.h:68: : public IPC::MessageFilter, public P2PSocketDispatcherInterface { On 2014/09/10 18:05:32, Sergey Ulanov wrote: > indentation. Done. https://codereview.chromium.org/536133003/diff/120001/content/renderer/p2p/so... content/renderer/p2p/socket_dispatcher.h:72: // Each observer is called immidiately after it is registered and On 2014/09/10 18:05:32, Sergey Ulanov wrote: > This comment should be in the interface definition. In the implementation you > just need one like like this: > // P2PSocketDispatcher interface: I feel how it should be called is really an implementation detail such that I put it here. For example, the comment doesn't really apply with the Mock one that I created
Patchset #8 (id:140001) has been deleted
https://codereview.chromium.org/536133003/diff/120001/content/renderer/p2p/so... File content/renderer/p2p/socket_dispatcher.h (right): https://codereview.chromium.org/536133003/diff/120001/content/renderer/p2p/so... content/renderer/p2p/socket_dispatcher.h:51: class P2PSocketDispatcherInterface { On 2014/09/10 19:56:33, guoweis wrote: > On 2014/09/10 18:05:32, Sergey Ulanov wrote: > > We don't normally use Interface suffix for interfaces. Usage of interface > > instead of implementation should not be penalized. Instead you can use Impl > > suffix for the implementation. > > > > But in this case it's probably better to give this interface a different name. > I > > suggest NetworkListManager > > Since it's the manager of NetworkListObserver, I name it > NetworkListObserverManager, hope it's ok. This doesn't look like a good name. It doesn't manage NetworkListObserver. It's an object that keeps current list of network interfaces and allows other object to get notified when the list changes. Storing list of observers is something a correct implementation has to do, but it's not the core responsibility of objects that implement this insterface. https://codereview.chromium.org/536133003/diff/120001/content/renderer/p2p/so... content/renderer/p2p/socket_dispatcher.h:72: // Each observer is called immidiately after it is registered and On 2014/09/10 19:56:33, guoweis wrote: > On 2014/09/10 18:05:32, Sergey Ulanov wrote: > > This comment should be in the interface definition. In the implementation you > > just need one like like this: > > // P2PSocketDispatcher interface: > > I feel how it should be called is really an implementation detail such that I > put it here. For example, the comment doesn't really apply with the Mock one > that I created These are not implementation details - they are part of the interface contract that should be documented in the interface. Not knowing these details about interface makes it impossible to use it correctly. Even though your Mock doesn't really abide by that contract the test overall still does - it still calls the observer after AddNetworkListObserver() . https://codereview.chromium.org/536133003/diff/160001/content/renderer/p2p/ip... File content/renderer/p2p/ipc_network_manager_unittest.cc (right): https://codereview.chromium.org/536133003/diff/160001/content/renderer/p2p/ip... content/renderer/p2p/ipc_network_manager_unittest.cc:40: delete mgr_; please use scoped_ptr<> to store owned objects. Then you woundn't need to delete these objects explicitly. https://codereview.chromium.org/536133003/diff/160001/content/renderer/p2p/ip... content/renderer/p2p/ipc_network_manager_unittest.cc:45: IpcNetworkManager* mgr_; this is not a good name. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Namin... https://codereview.chromium.org/536133003/diff/160001/content/renderer/p2p/so... File content/renderer/p2p/socket_dispatcher.h (right): https://codereview.chromium.org/536133003/diff/160001/content/renderer/p2p/so... content/renderer/p2p/socket_dispatcher.h:51: class NetworkListObserverManager { Please move this to a separate file and add comments to explain what it's used for.
PTAL https://codereview.chromium.org/536133003/diff/120001/content/renderer/p2p/so... File content/renderer/p2p/socket_dispatcher.h (right): https://codereview.chromium.org/536133003/diff/120001/content/renderer/p2p/so... content/renderer/p2p/socket_dispatcher.h:51: class P2PSocketDispatcherInterface { On 2014/09/10 22:28:25, Sergey Ulanov wrote: > On 2014/09/10 19:56:33, guoweis wrote: > > On 2014/09/10 18:05:32, Sergey Ulanov wrote: > > > We don't normally use Interface suffix for interfaces. Usage of interface > > > instead of implementation should not be penalized. Instead you can use Impl > > > suffix for the implementation. > > > > > > But in this case it's probably better to give this interface a different > name. > > I > > > suggest NetworkListManager > > > > Since it's the manager of NetworkListObserver, I name it > > NetworkListObserverManager, hope it's ok. > > This doesn't look like a good name. It doesn't manage NetworkListObserver. It's > an object that keeps current list of network interfaces and allows other object > to get notified when the list changes. Storing list of observers is something a > correct implementation has to do, but it's not the core responsibility of > objects that implement this insterface. Changed to NetworkListManager https://codereview.chromium.org/536133003/diff/120001/content/renderer/p2p/so... content/renderer/p2p/socket_dispatcher.h:72: // Each observer is called immidiately after it is registered and On 2014/09/10 22:28:25, Sergey Ulanov wrote: > On 2014/09/10 19:56:33, guoweis wrote: > > On 2014/09/10 18:05:32, Sergey Ulanov wrote: > > > This comment should be in the interface definition. In the implementation > you > > > just need one like like this: > > > // P2PSocketDispatcher interface: > > > > I feel how it should be called is really an implementation detail such that I > > put it here. For example, the comment doesn't really apply with the Mock one > > that I created > > These are not implementation details - they are part of the interface contract > that should be documented in the interface. Not knowing these details about > interface makes it impossible to use it correctly. Even though your Mock doesn't > really abide by that contract the test overall still does - it still calls the > observer after AddNetworkListObserver() . Done. https://codereview.chromium.org/536133003/diff/160001/content/renderer/p2p/ip... File content/renderer/p2p/ipc_network_manager_unittest.cc (right): https://codereview.chromium.org/536133003/diff/160001/content/renderer/p2p/ip... content/renderer/p2p/ipc_network_manager_unittest.cc:40: delete mgr_; On 2014/09/10 22:28:25, Sergey Ulanov wrote: > please use scoped_ptr<> to store owned objects. Then you woundn't need to delete > these objects explicitly. Done. https://codereview.chromium.org/536133003/diff/160001/content/renderer/p2p/ip... content/renderer/p2p/ipc_network_manager_unittest.cc:45: IpcNetworkManager* mgr_; On 2014/09/10 22:28:25, Sergey Ulanov wrote: > this is not a good name. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Namin... Done. https://codereview.chromium.org/536133003/diff/160001/content/renderer/p2p/so... File content/renderer/p2p/socket_dispatcher.h (right): https://codereview.chromium.org/536133003/diff/160001/content/renderer/p2p/so... content/renderer/p2p/socket_dispatcher.h:51: class NetworkListObserverManager { On 2014/09/10 22:28:25, Sergey Ulanov wrote: > Please move this to a separate file and add comments to explain what it's used > for. Done.
Looks mostly good, but I have some more style nits. https://codereview.chromium.org/536133003/diff/200001/content/renderer/p2p/ip... File content/renderer/p2p/ipc_network_manager.h (right): https://codereview.chromium.org/536133003/diff/200001/content/renderer/p2p/ip... content/renderer/p2p/ipc_network_manager.h:26: NetworkListManager* socket_dispatcher); please rename the variable too https://codereview.chromium.org/536133003/diff/200001/content/renderer/p2p/ip... File content/renderer/p2p/ipc_network_manager_unittest.cc (right): https://codereview.chromium.org/536133003/diff/200001/content/renderer/p2p/ip... content/renderer/p2p/ipc_network_manager_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2014, remove (c) https://codereview.chromium.org/536133003/diff/200001/content/renderer/p2p/ip... content/renderer/p2p/ipc_network_manager_unittest.cc:18: NetworkListObserver*) OVERRIDE {} keep argument name? https://codereview.chromium.org/536133003/diff/200001/content/renderer/p2p/ip... content/renderer/p2p/ipc_network_manager_unittest.cc:29: static const std::string kIPv6PublicAddrString1 = static std::string objects are not allowed (see http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Static_and_Gl...) and trybots would reject this change. Please use char[] for it. https://codereview.chromium.org/536133003/diff/200001/content/renderer/p2p/ip... content/renderer/p2p/ipc_network_manager_unittest.cc:37: socket_dispatcher_.reset(new MockP2PSocketDispatcher()); Use initializers, e.g.: IpcNetworkManagerTest() : socket_dispatcher_(new MockP2PSocketDispatcher()), network_manager_(new IpcNetworkManager(socket_dispatcher_.get())) {} https://codereview.chromium.org/536133003/diff/200001/content/renderer/p2p/ip... content/renderer/p2p/ipc_network_manager_unittest.cc:44: remove this empty line https://codereview.chromium.org/536133003/diff/200001/content/renderer/p2p/so... File content/renderer/p2p/socket_dispatcher.h (right): https://codereview.chromium.org/536133003/diff/200001/content/renderer/p2p/so... content/renderer/p2p/socket_dispatcher.h:53: public IPC::MessageFilter, public NetworkListManager { This should be formatted either like this: class CONTENT_EXPORT P2PSocketDispatcher : public IPC::MessageFilter, public NetworkListManager { or this: class CONTENT_EXPORT P2PSocketDispatcher : public IPC::MessageFilter, public NetworkListManager { Also please try using clang-format (https://code.google.com/p/chromium/wiki/ClangFormat) https://codereview.chromium.org/536133003/diff/200001/content/renderer/p2p/so... content/renderer/p2p/socket_dispatcher.h:60: nit: remove this empty line
PTAL https://codereview.chromium.org/536133003/diff/200001/content/renderer/p2p/ip... File content/renderer/p2p/ipc_network_manager.h (right): https://codereview.chromium.org/536133003/diff/200001/content/renderer/p2p/ip... content/renderer/p2p/ipc_network_manager.h:26: NetworkListManager* socket_dispatcher); On 2014/09/10 23:37:15, Sergey Ulanov wrote: > please rename the variable too Done. https://codereview.chromium.org/536133003/diff/200001/content/renderer/p2p/ip... File content/renderer/p2p/ipc_network_manager_unittest.cc (right): https://codereview.chromium.org/536133003/diff/200001/content/renderer/p2p/ip... content/renderer/p2p/ipc_network_manager_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/09/10 23:37:16, Sergey Ulanov wrote: > 2014, remove (c) Done. https://codereview.chromium.org/536133003/diff/200001/content/renderer/p2p/ip... content/renderer/p2p/ipc_network_manager_unittest.cc:18: NetworkListObserver*) OVERRIDE {} On 2014/09/10 23:37:15, Sergey Ulanov wrote: > keep argument name? Done. https://codereview.chromium.org/536133003/diff/200001/content/renderer/p2p/ip... content/renderer/p2p/ipc_network_manager_unittest.cc:29: static const std::string kIPv6PublicAddrString1 = On 2014/09/10 23:37:16, Sergey Ulanov wrote: > static std::string objects are not allowed (see > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Static_and_Gl...) > and trybots would reject this change. > > Please use char[] for it. Done. https://codereview.chromium.org/536133003/diff/200001/content/renderer/p2p/ip... content/renderer/p2p/ipc_network_manager_unittest.cc:37: socket_dispatcher_.reset(new MockP2PSocketDispatcher()); On 2014/09/10 23:37:16, Sergey Ulanov wrote: > Use initializers, e.g.: > IpcNetworkManagerTest() > : socket_dispatcher_(new MockP2PSocketDispatcher()), > network_manager_(new IpcNetworkManager(socket_dispatcher_.get())) {} Done. https://codereview.chromium.org/536133003/diff/200001/content/renderer/p2p/ip... content/renderer/p2p/ipc_network_manager_unittest.cc:44: On 2014/09/10 23:37:16, Sergey Ulanov wrote: > remove this empty line Done. https://codereview.chromium.org/536133003/diff/200001/content/renderer/p2p/so... File content/renderer/p2p/socket_dispatcher.h (right): https://codereview.chromium.org/536133003/diff/200001/content/renderer/p2p/so... content/renderer/p2p/socket_dispatcher.h:53: public IPC::MessageFilter, public NetworkListManager { On 2014/09/10 23:37:16, Sergey Ulanov wrote: > This should be formatted either like this: > > class CONTENT_EXPORT P2PSocketDispatcher : public IPC::MessageFilter, > public NetworkListManager { > > > or this: > > class CONTENT_EXPORT P2PSocketDispatcher > : public IPC::MessageFilter, > public NetworkListManager { > > > Also please try using clang-format > (https://code.google.com/p/chromium/wiki/ClangFormat) Done. https://codereview.chromium.org/536133003/diff/200001/content/renderer/p2p/so... content/renderer/p2p/socket_dispatcher.h:60: On 2014/09/10 23:37:16, Sergey Ulanov wrote: > nit: remove this empty line Done.
lgtm
The CQ bit was checked by guoweis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/536133003/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
guoweis@webrtc.org changed reviewers: + inferno@chromium.org
+inferno for just content/common/p2p_message.h.
wfh: please OWNERS review content/common/p2p_message.h.
https://codereview.chromium.org/536133003/diff/260001/content/common/p2p_mess... File content/common/p2p_messages.h (right): https://codereview.chromium.org/536133003/diff/260001/content/common/p2p_mess... content/common/p2p_messages.h:31: IPC_STRUCT_TRAITS_MEMBER(network_prefix) network_prefix is size_t, and we prefer not to have size_t length variables sent across IPC- see http://www.chromium.org/Home/chromium-security/education/security-tips-for-ip.... can you change this to a fixed length type? Thanks!
guoweis@google.com changed reviewers: + rsleevi@chromium.org
@wfh: could you OWNERS review the change in p2p_message.h? @rsleevi: Could you OWNERS review the change in net_util.h and net_util.cc? wfh from security asked me to change prefix_length from size_t to unsigned int such that it's the same size across platforms.
IPC messages LGTM https://codereview.chromium.org/536133003/diff/280001/net/base/net_util.h File net/base/net_util.h (right): https://codereview.chromium.org/536133003/diff/280001/net/base/net_util.h#new... net/base/net_util.h:458: unsigned int network_prefix; nit: could use uint32 like interface_index
Patchset #11 (id:220001) has been deleted
Patchset #11 (id:240001) has been deleted
Patchset #11 (id:260001) has been deleted
guoweis@chromium.org changed reviewers: + davidben@chromium.org, guoweis@chromium.org - rsleevi@chromium.org
@davidben: Could you OWNERS review the change in net_util.h and net_util.cc? wfh from security asked me to change prefix_length from size_t to unsigned int such that it's the same size across platforms.
lgtm with comment, though I'm slightly puzzled why the IPC layer particularly cares about size_t vs. explicitly sized (we support different sizes of that across IPC boundaries?), and it's a little unfortunate that the needs of //ipc dictate how random //net structs look like. But *shrug*. https://codereview.chromium.org/536133003/diff/280001/net/base/net_util.h File net/base/net_util.h (right): https://codereview.chromium.org/536133003/diff/280001/net/base/net_util.h#new... net/base/net_util.h:458: unsigned int network_prefix; On 2014/09/13 00:09:10, Will Harris wrote: > nit: could use uint32 like interface_index +1. If the goal is to avoid implicitly sized types, uint32 is probably better than unsigned int (though I think they're the same on our all platforms?). The guideline explicitly lists the (u)int* types.
Thanks for the review. Changed to uint32. https://codereview.chromium.org/536133003/diff/280001/net/base/net_util.h File net/base/net_util.h (right): https://codereview.chromium.org/536133003/diff/280001/net/base/net_util.h#new... net/base/net_util.h:458: unsigned int network_prefix; On 2014/09/15 17:27:43, David Benjamin wrote: > On 2014/09/13 00:09:10, Will Harris wrote: > > nit: could use uint32 like interface_index > > +1. If the goal is to avoid implicitly sized types, uint32 is probably better > than unsigned int (though I think they're the same on our all platforms?). The > guideline explicitly lists the (u)int* types. Done.
The CQ bit was checked by guoweis@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/536133003/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
