Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9416083:
Make PPB_NetworkList_Private immutable. (Closed)
|
Created:
8 years, 10 months ago by Sergey Ulanov Modified:
8 years, 10 months ago CC:
chromium-reviews, piman+watch_chromium.org, darin-cc_chromium.org, yzshen+watch_chromium.org, ihf+watch_chromium.org, Wez, yzshen1 Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMake PPB_NetworkList_Private immutable.
- NetworkList objects are now immutable.
- Added PPB_NetworkMonitor_Private interface.
- NetworkList resource is passed to PPB_NetworkMonitor_Callback
- Replaced GetIPAddress() with GetIPAddresses() to handle the case when there is more than one or two addresses assigned to an interface.
BUG=114808
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=123548
Patch Set 1 : - #
Total comments: 16
Patch Set 2 : - #
Total comments: 13
Patch Set 3 : - #Patch Set 4 : - #
Total comments: 6
Messages
Total messages: 14 (0 generated)
http://codereview.chromium.org/9416083/diff/3002/ppapi/api/private/ppb_networ... File ppapi/api/private/ppb_network_list_private.idl (right): http://codereview.chromium.org/9416083/diff/3002/ppapi/api/private/ppb_networ... ppapi/api/private/ppb_network_list_private.idl:106: * network interface. Can you say what the return value is on error? http://codereview.chromium.org/9416083/diff/3002/ppapi/api/private/ppb_networ... File ppapi/api/private/ppb_network_monitor_private.idl (right): http://codereview.chromium.org/9416083/diff/3002/ppapi/api/private/ppb_networ... ppapi/api/private/ppb_network_monitor_private.idl:33: * immidiately after this method is called and then later every time immediately is spelled wrong, but let's say "once (to supply the initial network configuration)" instead. http://codereview.chromium.org/9416083/diff/3002/ppapi/api/private/ppb_networ... ppapi/api/private/ppb_network_monitor_private.idl:46: int32_t StartMonitoring([in] PPB_NetworkMonitor_Callback callback, I don't really feel like we need this ID. I think it's OK for this notification to be global so every call to StartMonitoring would overwrite the old callback.
http://codereview.chromium.org/9416083/diff/3002/ppapi/api/private/ppb_networ... File ppapi/api/private/ppb_network_list_private.idl (right): http://codereview.chromium.org/9416083/diff/3002/ppapi/api/private/ppb_networ... ppapi/api/private/ppb_network_list_private.idl:54: * the list is immutable. Current networks configuration can be nit: The current network configuration ... http://codereview.chromium.org/9416083/diff/3002/ppapi/api/private/ppb_networ... ppapi/api/private/ppb_network_list_private.idl:103: * addresses are filled in. How does the caller detect that condition? Is the return-value -<N> in that case, for example? http://codereview.chromium.org/9416083/diff/3002/ppapi/api/private/ppb_networ... File ppapi/api/private/ppb_network_monitor_private.idl (right): http://codereview.chromium.org/9416083/diff/3002/ppapi/api/private/ppb_networ... ppapi/api/private/ppb_network_monitor_private.idl:46: int32_t StartMonitoring([in] PPB_NetworkMonitor_Callback callback, Why not return a PP_Resource identifying the monitor instance, and get rid of StopMonitoring()?
http://codereview.chromium.org/9416083/diff/3002/ppapi/api/private/ppb_networ... File ppapi/api/private/ppb_network_list_private.idl (right): http://codereview.chromium.org/9416083/diff/3002/ppapi/api/private/ppb_networ... ppapi/api/private/ppb_network_list_private.idl:18: enum PP_NetworkListType { How about PP_NetworkListType_Private / PP_NetworkListState_Private? http://codereview.chromium.org/9416083/diff/3002/ppapi/api/private/ppb_networ... ppapi/api/private/ppb_network_list_private.idl:112: [out] int32_t count); This is an in/out parameter, right?
http://codereview.chromium.org/9416083/diff/3002/ppapi/api/private/ppb_networ... File ppapi/api/private/ppb_network_list_private.idl (right): http://codereview.chromium.org/9416083/diff/3002/ppapi/api/private/ppb_networ... ppapi/api/private/ppb_network_list_private.idl:18: enum PP_NetworkListType { On 2012/02/22 18:59:39, yzshen1 wrote: > How about PP_NetworkListType_Private / PP_NetworkListState_Private? Done. http://codereview.chromium.org/9416083/diff/3002/ppapi/api/private/ppb_networ... ppapi/api/private/ppb_network_list_private.idl:54: * the list is immutable. Current networks configuration can be On 2012/02/22 17:46:43, Wez wrote: > nit: The current network configuration ... Done. http://codereview.chromium.org/9416083/diff/3002/ppapi/api/private/ppb_networ... ppapi/api/private/ppb_network_list_private.idl:103: * addresses are filled in. On 2012/02/22 17:46:43, Wez wrote: > How does the caller detect that condition? Is the return-value -<N> in that > case, for example? Caller can detect this by verifying that count is greater or equals the returned value. http://codereview.chromium.org/9416083/diff/3002/ppapi/api/private/ppb_networ... ppapi/api/private/ppb_network_list_private.idl:106: * network interface. On 2012/02/22 17:26:39, brettw wrote: > Can you say what the return value is on error? Done. http://codereview.chromium.org/9416083/diff/3002/ppapi/api/private/ppb_networ... ppapi/api/private/ppb_network_list_private.idl:112: [out] int32_t count); On 2012/02/22 18:59:39, yzshen1 wrote: > This is an in/out parameter, right? Actually this is supposed to be [in] - size of the buffer that the caller has allocated. http://codereview.chromium.org/9416083/diff/3002/ppapi/api/private/ppb_networ... File ppapi/api/private/ppb_network_monitor_private.idl (right): http://codereview.chromium.org/9416083/diff/3002/ppapi/api/private/ppb_networ... ppapi/api/private/ppb_network_monitor_private.idl:33: * immidiately after this method is called and then later every time On 2012/02/22 17:26:39, brettw wrote: > immediately is spelled wrong, but let's say "once (to supply the initial network > configuration)" instead. Done. http://codereview.chromium.org/9416083/diff/3002/ppapi/api/private/ppb_networ... ppapi/api/private/ppb_network_monitor_private.idl:46: int32_t StartMonitoring([in] PPB_NetworkMonitor_Callback callback, On 2012/02/22 17:26:39, brettw wrote: > I don't really feel like we need this ID. I think it's OK for this notification > to be global so every call to StartMonitoring would overwrite the old callback. Done. Also removed StopMonitoring() and renamed this method to SetCallback(). Now the plugin should stop notifications by calling this method with callback = NULL. http://codereview.chromium.org/9416083/diff/3002/ppapi/api/private/ppb_networ... ppapi/api/private/ppb_network_monitor_private.idl:46: int32_t StartMonitoring([in] PPB_NetworkMonitor_Callback callback, On 2012/02/22 17:46:43, Wez wrote: > Why not return a PP_Resource identifying the monitor instance, and get rid of > StopMonitoring()? Just changed it so that there is always only one monitoring callback. http://codereview.chromium.org/9416083/diff/7001/ppapi/api/private/ppb_networ... File ppapi/api/private/ppb_network_monitor_private.idl (right): http://codereview.chromium.org/9416083/diff/7001/ppapi/api/private/ppb_networ... ppapi/api/private/ppb_network_monitor_private.idl:47: int32_t SetCallback([in] PP_Instance instance, Also added this |instance| parameters, but not sure if we really need it here.
I just looked briefly, I'll leave this in dmichael's hands for the rest of the review. http://codereview.chromium.org/9416083/diff/7001/ppapi/api/private/ppb_networ... File ppapi/api/private/ppb_network_list_private.idl (right): http://codereview.chromium.org/9416083/diff/7001/ppapi/api/private/ppb_networ... ppapi/api/private/ppb_network_list_private.idl:18: enum PP_NetworkListType_Priave { Spelling :) http://codereview.chromium.org/9416083/diff/7001/ppapi/api/private/ppb_networ... ppapi/api/private/ppb_network_list_private.idl:39: enum PP_NetworkListState_Priave { ditto
http://codereview.chromium.org/9416083/diff/7001/ppapi/api/private/ppb_networ... File ppapi/api/private/ppb_network_list_private.idl (right): http://codereview.chromium.org/9416083/diff/7001/ppapi/api/private/ppb_networ... ppapi/api/private/ppb_network_list_private.idl:102: * space to store all addresses, then only thej first thej->the http://codereview.chromium.org/9416083/diff/7001/ppapi/api/private/ppb_networ... ppapi/api/private/ppb_network_list_private.idl:113: nit: trailing whitespace http://codereview.chromium.org/9416083/diff/7001/ppapi/api/private/ppb_networ... File ppapi/api/private/ppb_network_monitor_private.idl (right): http://codereview.chromium.org/9416083/diff/7001/ppapi/api/private/ppb_networ... ppapi/api/private/ppb_network_monitor_private.idl:36: * with <code>callback</code> set to NULL. Hmm, I think I prefer wez's suggestion to return a PP_Resource. A plugin would cancel the callbacks by releasing the resource. It might make it easier to scope the lifetime of the backing object when you're implementing. That also makes it possible to have multiple callback handlers, if that's something you want.
http://codereview.chromium.org/9416083/diff/7001/ppapi/api/private/ppb_networ... File ppapi/api/private/ppb_network_list_private.idl (right): http://codereview.chromium.org/9416083/diff/7001/ppapi/api/private/ppb_networ... ppapi/api/private/ppb_network_list_private.idl:18: enum PP_NetworkListType_Priave { On 2012/02/24 04:45:31, brettw wrote: > Spelling :) Ouch. Fixed. http://codereview.chromium.org/9416083/diff/7001/ppapi/api/private/ppb_networ... ppapi/api/private/ppb_network_list_private.idl:39: enum PP_NetworkListState_Priave { On 2012/02/24 04:45:31, brettw wrote: > ditto Done. http://codereview.chromium.org/9416083/diff/7001/ppapi/api/private/ppb_networ... ppapi/api/private/ppb_network_list_private.idl:102: * space to store all addresses, then only thej first On 2012/02/24 18:04:11, dmichael wrote: > thej->the Done. http://codereview.chromium.org/9416083/diff/7001/ppapi/api/private/ppb_networ... ppapi/api/private/ppb_network_list_private.idl:113: On 2012/02/24 18:04:11, dmichael wrote: > nit: trailing whitespace Done. http://codereview.chromium.org/9416083/diff/7001/ppapi/api/private/ppb_networ... File ppapi/api/private/ppb_network_monitor_private.idl (right): http://codereview.chromium.org/9416083/diff/7001/ppapi/api/private/ppb_networ... ppapi/api/private/ppb_network_monitor_private.idl:36: * with <code>callback</code> set to NULL. On 2012/02/24 18:04:11, dmichael wrote: > Hmm, I think I prefer wez's suggestion to return a PP_Resource. A plugin would > cancel the callbacks by releasing the resource. It might make it easier to scope > the lifetime of the backing object when you're implementing. That also makes it > possible to have multiple callback handlers, if that's something you want. Done and also renamed it to Create(). In that case do we also need IsNetworkMonitor() method?
http://codereview.chromium.org/9416083/diff/7001/ppapi/api/private/ppb_networ... File ppapi/api/private/ppb_network_monitor_private.idl (right): http://codereview.chromium.org/9416083/diff/7001/ppapi/api/private/ppb_networ... ppapi/api/private/ppb_network_monitor_private.idl:36: * with <code>callback</code> set to NULL. On 2012/02/24 18:25:11, sergeyu wrote: > On 2012/02/24 18:04:11, dmichael wrote: > > Hmm, I think I prefer wez's suggestion to return a PP_Resource. A plugin would > > cancel the callbacks by releasing the resource. It might make it easier to > scope > > the lifetime of the backing object when you're implementing. That also makes > it > > possible to have multiple callback handlers, if that's something you want. > > Done and also renamed it to Create(). In that case do we also need > IsNetworkMonitor() method? Yes. I guess that's the downside; more boilerplate. If this direction seems to annoying when you're implementing, feel free to circle back and do it more like the other approach you had. I'm hoping this way is a bit easier to implement (and maybe to use too, once you put a C++ wrapper on it).
http://codereview.chromium.org/9416083/diff/7001/ppapi/api/private/ppb_networ... File ppapi/api/private/ppb_network_monitor_private.idl (right): http://codereview.chromium.org/9416083/diff/7001/ppapi/api/private/ppb_networ... ppapi/api/private/ppb_network_monitor_private.idl:36: * with <code>callback</code> set to NULL. On 2012/02/24 18:29:11, dmichael wrote: > On 2012/02/24 18:25:11, sergeyu wrote: > > On 2012/02/24 18:04:11, dmichael wrote: > > > Hmm, I think I prefer wez's suggestion to return a PP_Resource. A plugin > would > > > cancel the callbacks by releasing the resource. It might make it easier to > > scope > > > the lifetime of the backing object when you're implementing. That also makes > > it > > > possible to have multiple callback handlers, if that's something you want. > > > > Done and also renamed it to Create(). In that case do we also need > > IsNetworkMonitor() method? > Yes. I guess that's the downside; more boilerplate. Done. > > If this direction seems to annoying when you're implementing, feel free to > circle back and do it more like the other approach you had. I'm hoping this way > is a bit easier to implement (and maybe to use too, once you put a C++ wrapper > on it). I actually like this approach more because it seems to be more consistent with other interfaces.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/9416083/10002
Change committed as 123548
http://codereview.chromium.org/9416083/diff/10002/ppapi/api/private/ppb_netwo... File ppapi/api/private/ppb_network_list_private.idl (right): http://codereview.chromium.org/9416083/diff/10002/ppapi/api/private/ppb_netwo... ppapi/api/private/ppb_network_list_private.idl:54: * the list is immutable. The current networks configuration can be typo: networks configuration -> network configurations http://codereview.chromium.org/9416083/diff/10002/ppapi/api/private/ppb_netwo... ppapi/api/private/ppb_network_list_private.idl:54: * the list is immutable. The current networks configuration can be The NetworkMonitor_Private interface is actually the _only_ way to get the network configuration, right? http://codereview.chromium.org/9416083/diff/10002/ppapi/api/private/ppb_netwo... File ppapi/api/private/ppb_network_monitor_private.idl (right): http://codereview.chromium.org/9416083/diff/10002/ppapi/api/private/ppb_netwo... ppapi/api/private/ppb_network_monitor_private.idl:34: * configuarion) and then later every time network configuration typo: configuration http://codereview.chromium.org/9416083/diff/10002/ppapi/api/private/ppb_netwo... ppapi/api/private/ppb_network_monitor_private.idl:36: * destroyed. If the plugin doesn't have access to the network list Is there any possibility of in-flight notifications reaching the callback after the resource is destroyed? http://codereview.chromium.org/9416083/diff/10002/ppapi/api/private/ppb_netwo... ppapi/api/private/ppb_network_monitor_private.idl:38: * <code>network_list</code> parameter is set to 0. typo: ... parameter set to ... http://codereview.chromium.org/9416083/diff/10002/ppapi/api/private/ppb_netwo... ppapi/api/private/ppb_network_monitor_private.idl:41: * network configuration changes or NULL to stop network monitoring. nit: Network monitoring is stopped by deleting the resource now - presumably NULL is no longer a valid callback value? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
