Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9722008:
Add CreateFromIPv[46]Address() in PPB_NetAddress_Private. (Closed)
|
Created:
8 years, 9 months ago by Sergey Ulanov Modified:
8 years, 9 months ago Reviewers:
dmichael (off chromium), Dmitry Polukhin, noelallen_use_chromium, noelallen1, viettrungluu CC:
chromium-reviews, piman+watch_chromium.org, darin-cc_chromium.org, yzshen+watch_chromium.org, ihf+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd CreateFromIPv[46]Address() in PPB_NetAddress_Private.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=128322
Patch Set 1 : - #
Total comments: 11
Patch Set 2 : #
Total comments: 22
Patch Set 3 : #Patch Set 4 : #
Total comments: 14
Patch Set 5 : #Patch Set 6 : - #Patch Set 7 : #
Messages
Total messages: 27 (0 generated)
http://codereview.chromium.org/9722008/diff/1007/ppapi/api/private/ppb_net_ad... File ppapi/api/private/ppb_net_address_private.idl (right): http://codereview.chromium.org/9722008/diff/1007/ppapi/api/private/ppb_net_ad... ppapi/api/private/ppb_net_address_private.idl:12: M19 = 1.0 Not sure if I need to add 1.1 because it's still M19
http://codereview.chromium.org/9722008/diff/1007/ppapi/api/private/ppb_net_ad... File ppapi/api/private/ppb_net_address_private.idl (right): http://codereview.chromium.org/9722008/diff/1007/ppapi/api/private/ppb_net_ad... ppapi/api/private/ppb_net_address_private.idl:12: M19 = 1.0 On 2012/03/16 21:32:53, sergeyu wrote: > Not sure if I need to add 1.1 because it's still M19 Yes you do. http://codereview.chromium.org/9722008/diff/1007/ppapi/api/private/ppb_net_ad... ppapi/api/private/ppb_net_address_private.idl:107: [version=1.0] Should be version 1.1. http://codereview.chromium.org/9722008/diff/1007/ppapi/cpp/private/net_addres... File ppapi/cpp/private/net_address_private.cc (right): http://codereview.chromium.org/9722008/diff/1007/ppapi/cpp/private/net_addres... ppapi/cpp/private/net_address_private.cc:15: And you need to support 0.1, 1.0, and 1.1.
http://codereview.chromium.org/9722008/diff/1007/ppapi/api/private/ppb_net_ad... File ppapi/api/private/ppb_net_address_private.idl (right): http://codereview.chromium.org/9722008/diff/1007/ppapi/api/private/ppb_net_ad... ppapi/api/private/ppb_net_address_private.idl:12: M19 = 1.0 On 2012/03/16 22:19:15, viettrungluu wrote: > On 2012/03/16 21:32:53, sergeyu wrote: > > Not sure if I need to add 1.1 because it's still M19 > > Yes you do. Ok, then what label should I use here for 1.1? If I use M19 for both 1.1 and 1.0 then generator.py generates C headers for 1.1, but not for 1.0
+noelallen: is there a way to do 2 interfaces that are both valid for one version of Chrome with the IDL? Should he fake it by using m18 for 1.0? On Fri, Mar 16, 2012 at 4:31 PM, <sergeyu@chromium.org> wrote: > > http://codereview.chromium.**org/9722008/diff/1007/ppapi/** > api/private/ppb_net_address_**private.idl<http://codereview.chromium.org/9722008/diff/1007/ppapi/api/private/ppb_net_address_private.idl> > File ppapi/api/private/ppb_net_**address_private.idl (right): > > http://codereview.chromium.**org/9722008/diff/1007/ppapi/** > api/private/ppb_net_address_**private.idl#newcode12<http://codereview.chromium.org/9722008/diff/1007/ppapi/api/private/ppb_net_address_private.idl#newcode12> > ppapi/api/private/ppb_net_**address_private.idl:12: M19 = 1.0 > On 2012/03/16 22:19:15, viettrungluu wrote: > >> On 2012/03/16 21:32:53, sergeyu wrote: >> > Not sure if I need to add 1.1 because it's still M19 >> > > Yes you do. >> > > Ok, then what label should I use here for 1.1? > If I use M19 for both 1.1 and 1.0 then generator.py generates C headers > for 1.1, but not for 1.0 > > http://codereview.chromium.**org/9722008/<http://codereview.chromium.org/9722... >
+noelallen - Noel, can you please take a look at the Dave's question above?
http://codereview.chromium.org/9722008/diff/1007/ppapi/api/private/ppb_net_ad... File ppapi/api/private/ppb_net_address_private.idl (right): http://codereview.chromium.org/9722008/diff/1007/ppapi/api/private/ppb_net_ad... ppapi/api/private/ppb_net_address_private.idl:12: M19 = 1.0 It's a question of what you are planning to ship. We shipped M17 apparently. There is no M18? We could change M19=1.0 to M18_X and add M19=1.1 or you can add M19.1=1.1 But the list of labels should be clean for shipping. Those are only transition numbers. On 2012/03/16 22:31:17, sergeyu wrote: > On 2012/03/16 22:19:15, viettrungluu wrote: > > On 2012/03/16 21:32:53, sergeyu wrote: > > > Not sure if I need to add 1.1 because it's still M19 > > > > Yes you do. > > Ok, then what label should I use here for 1.1? > If I use M19 for both 1.1 and 1.0 then generator.py generates C headers for 1.1, > but not for 1.0
http://codereview.chromium.org/9722008/diff/1007/ppapi/api/private/ppb_net_ad... File ppapi/api/private/ppb_net_address_private.idl (right): http://codereview.chromium.org/9722008/diff/1007/ppapi/api/private/ppb_net_ad... ppapi/api/private/ppb_net_address_private.idl:12: M19 = 1.0 Here's my understanding. A new function was added to an emerging interface version M19. The definition of M19 is not set in stone until we release it (basically branch to dev). So as long as this CL also updates all users of this API (the M19 version) such as any PPAPI unit tests, any NaCl proxy, other PPAPI plugins, etc... then it should be safe to add to the end. vlettrungluu - Are you using this interface somewhere else, is that why you want the version to be 1.1? On 2012/03/20 00:48:58, noelallen1 wrote: > It's a question of what you are planning to ship. > > We shipped M17 apparently. There is no M18? > We could change M19=1.0 to M18_X and add M19=1.1 or you can add M19.1=1.1 > > But the list of labels should be clean for shipping. Those are only transition > numbers. > > On 2012/03/16 22:31:17, sergeyu wrote: > > On 2012/03/16 22:19:15, viettrungluu wrote: > > > On 2012/03/16 21:32:53, sergeyu wrote: > > > > Not sure if I need to add 1.1 because it's still M19 > > > > > > Yes you do. > > > > Ok, then what label should I use here for 1.1? > > If I use M19 for both 1.1 and 1.0 then generator.py generates C headers for > 1.1, > > but not for 1.0 >
http://codereview.chromium.org/9722008/diff/1007/ppapi/api/private/ppb_net_ad... File ppapi/api/private/ppb_net_address_private.idl (right): http://codereview.chromium.org/9722008/diff/1007/ppapi/api/private/ppb_net_ad... ppapi/api/private/ppb_net_address_private.idl:12: M19 = 1.0 On 2012/03/20 01:01:22, noelallen1 wrote: > Here's my understanding. > > A new function was added to an emerging interface version M19. The definition > of M19 is not set in stone until we release it (basically branch to dev). > > So as long as this CL also updates all users of this API (the M19 version) such > as any PPAPI unit tests, any NaCl proxy, other PPAPI plugins, etc... then it > should be safe to add to the end. > > vlettrungluu - Are you using this interface somewhere else, is that why you want > the version to be 1.1? Version 1.0 is already being used by Pepper Flash, and we obviously don't have the ability to update/ship that in absolute lockstep with Chrome. In general, one should never assume that one has the ability to "update all users". Version numbers themselves are cheap and -- once an interface has been implemented so that it's at all usable -- any (ABI-level) incompatible change should come with a version number change. > > > On 2012/03/20 00:48:58, noelallen1 wrote: > > It's a question of what you are planning to ship. > > > > We shipped M17 apparently. There is no M18? > > We could change M19=1.0 to M18_X and add M19=1.1 or you can add M19.1=1.1 > > > > But the list of labels should be clean for shipping. Those are only > transition > > numbers. > > > > On 2012/03/16 22:31:17, sergeyu wrote: > > > On 2012/03/16 22:19:15, viettrungluu wrote: > > > > On 2012/03/16 21:32:53, sergeyu wrote: > > > > > Not sure if I need to add 1.1 because it's still M19 > > > > > > > > Yes you do. > > > > > > Ok, then what label should I use here for 1.1? > > > If I use M19 for both 1.1 and 1.0 then generator.py generates C headers for > > 1.1, > > > but not for 1.0 > > > http://codereview.chromium.org/9722008/diff/1007/ppapi/api/private/ppb_net_ad... ppapi/api/private/ppb_net_address_private.idl:12: M19 = 1.0 On 2012/03/20 00:48:58, noelallen1 wrote: > It's a question of what you are planning to ship. > > We shipped M17 apparently. There is no M18? > We could change M19=1.0 to M18_X and add M19=1.1 or you can add M19.1=1.1 > > But the list of labels should be clean for shipping. Those are only transition > numbers. If we put "M19 = 1.1", will the version 1.0 struct will still appear in the generated header? (I know that the 0.1 struct will, but is that because 0.1 < 1.1, or because "M17 = 0.1" is in the list?) > > On 2012/03/16 22:31:17, sergeyu wrote: > > On 2012/03/16 22:19:15, viettrungluu wrote: > > > On 2012/03/16 21:32:53, sergeyu wrote: > > > > Not sure if I need to add 1.1 because it's still M19 > > > > > > Yes you do. > > > > Ok, then what label should I use here for 1.1? > > If I use M19 for both 1.1 and 1.0 then generator.py generates C headers for > 1.1, > > but not for 1.0 >
http://codereview.chromium.org/9722008/diff/1007/ppapi/api/private/ppb_net_ad... File ppapi/api/private/ppb_net_address_private.idl (right): http://codereview.chromium.org/9722008/diff/1007/ppapi/api/private/ppb_net_ad... ppapi/api/private/ppb_net_address_private.idl:12: M19 = 1.0 On 2012/03/20 12:51:34, viettrungluu wrote: > On 2012/03/20 01:01:22, noelallen1 wrote: > > Here's my understanding. > > > > A new function was added to an emerging interface version M19. The definition > > of M19 is not set in stone until we release it (basically branch to dev). > > > > So as long as this CL also updates all users of this API (the M19 version) > such > > as any PPAPI unit tests, any NaCl proxy, other PPAPI plugins, etc... then it > > should be safe to add to the end. > > > > vlettrungluu - Are you using this interface somewhere else, is that why you > want > > the version to be 1.1? > > Version 1.0 is already being used by Pepper Flash, and we obviously don't have > the ability to update/ship that in absolute lockstep with Chrome. Yes, but this particular CL just adds new methods to the interface, so this won't break old plugins that already use 1.0. > In general, one should never assume that one has the ability to "update all > users". Version numbers themselves are cheap and -- once an interface has been > implemented so that it's at all usable -- any (ABI-level) incompatible change > should come with a version number change. > > > > > > > On 2012/03/20 00:48:58, noelallen1 wrote: > > > It's a question of what you are planning to ship. > > > > > > We shipped M17 apparently. There is no M18? > > > We could change M19=1.0 to M18_X and add M19=1.1 or you can add M19.1=1.1 > > > > > > But the list of labels should be clean for shipping. Those are only > > transition > > > numbers. > > > > > > On 2012/03/16 22:31:17, sergeyu wrote: > > > > On 2012/03/16 22:19:15, viettrungluu wrote: > > > > > On 2012/03/16 21:32:53, sergeyu wrote: > > > > > > Not sure if I need to add 1.1 because it's still M19 > > > > > > > > > > Yes you do. > > > > > > > > Ok, then what label should I use here for 1.1? > > > > If I use M19 for both 1.1 and 1.0 then generator.py generates C headers > for > > > 1.1, > > > > but not for 1.0 > > > > > > http://codereview.chromium.org/9722008/diff/1007/ppapi/api/private/ppb_net_ad... ppapi/api/private/ppb_net_address_private.idl:12: M19 = 1.0 On 2012/03/20 12:51:34, viettrungluu wrote: > On 2012/03/20 00:48:58, noelallen1 wrote: > > It's a question of what you are planning to ship. > > > > We shipped M17 apparently. There is no M18? > > We could change M19=1.0 to M18_X and add M19=1.1 or you can add M19.1=1.1 > > > > But the list of labels should be clean for shipping. Those are only > transition > > numbers. > > If we put "M19 = 1.1", will the version 1.0 struct will still appear in the > generated header? No. 1.0 will be in the header only when 1.0 and 1.1 have different labels. > (I know that the 0.1 struct will, but is that because 0.1 < > 1.1, or because "M17 = 0.1" is in the list?) because M17 = 0.1 and M17 != M19. Each generated interface has to have a unique label. As Noel noted above, we could have M19_0=1.0 and M19_1=1.1, but it would have to be cleaned up before M19 branches. Given that addition of new methods to an interface cannot not break any existing plugins, I don't see any problems with just keeping the version at 1.0. > > > > > On 2012/03/16 22:31:17, sergeyu wrote: > > > On 2012/03/16 22:19:15, viettrungluu wrote: > > > > On 2012/03/16 21:32:53, sergeyu wrote: > > > > > Not sure if I need to add 1.1 because it's still M19 > > > > > > > > Yes you do. > > > > > > Ok, then what label should I use here for 1.1? > > > If I use M19 for both 1.1 and 1.0 then generator.py generates C headers for > > 1.1, > > > but not for 1.0 > > >
On Tue, Mar 20, 2012 at 10:52 AM, <sergeyu@chromium.org> wrote: > > http://codereview.chromium.**org/9722008/diff/1007/ppapi/** > api/private/ppb_net_address_**private.idl<http://codereview.chromium.org/9722008/diff/1007/ppapi/api/private/ppb_net_address_private.idl> > File ppapi/api/private/ppb_net_**address_private.idl (right): > > http://codereview.chromium.**org/9722008/diff/1007/ppapi/** > api/private/ppb_net_address_**private.idl#newcode12<http://codereview.chromium.org/9722008/diff/1007/ppapi/api/private/ppb_net_address_private.idl#newcode12> > ppapi/api/private/ppb_net_**address_private.idl:12: M19 = 1.0 > On 2012/03/20 12:51:34, viettrungluu wrote: > >> On 2012/03/20 01:01:22, noelallen1 wrote: >> > Here's my understanding. >> > >> > A new function was added to an emerging interface version M19. The >> > definition > >> > of M19 is not set in stone until we release it (basically branch to >> > dev). > >> > >> > So as long as this CL also updates all users of this API (the M19 >> > version) > >> such >> > as any PPAPI unit tests, any NaCl proxy, other PPAPI plugins, etc... >> > then it > >> > should be safe to add to the end. >> > >> > vlettrungluu - Are you using this interface somewhere else, is that >> > why you > >> want >> > the version to be 1.1? >> > > Version 1.0 is already being used by Pepper Flash, and we obviously >> > don't have > >> the ability to update/ship that in absolute lockstep with Chrome. >> > > Yes, but this particular CL just adds new methods to the interface, so > this won't break old plugins that already use 1.0. > > > In general, one should never assume that one has the ability to >> > "update all > >> users". Version numbers themselves are cheap and -- once an interface >> > has been > >> implemented so that it's at all usable -- any (ABI-level) incompatible >> > change > >> should come with a version number change. >> > > > >> > >> > On 2012/03/20 00:48:58, noelallen1 wrote: >> > > It's a question of what you are planning to ship. >> > > >> > > We shipped M17 apparently. There is no M18? >> > > We could change M19=1.0 to M18_X and add M19=1.1 or you can add >> > M19.1=1.1 > >> > > >> > > But the list of labels should be clean for shipping. Those are >> > only > >> > transition >> > > numbers. >> > > >> > > On 2012/03/16 22:31:17, sergeyu wrote: >> > > > On 2012/03/16 22:19:15, viettrungluu wrote: >> > > > > On 2012/03/16 21:32:53, sergeyu wrote: >> > > > > > Not sure if I need to add 1.1 because it's still M19 >> > > > > >> > > > > Yes you do. >> > > > >> > > > Ok, then what label should I use here for 1.1? >> > > > If I use M19 for both 1.1 and 1.0 then generator.py generates C >> > headers > >> for >> > > 1.1, >> > > > but not for 1.0 >> > > >> > >> > > > http://codereview.chromium.**org/9722008/diff/1007/ppapi/** > api/private/ppb_net_address_**private.idl#newcode12<http://codereview.chromium.org/9722008/diff/1007/ppapi/api/private/ppb_net_address_private.idl#newcode12> > ppapi/api/private/ppb_net_**address_private.idl:12: M19 = 1.0 > On 2012/03/20 12:51:34, viettrungluu wrote: > >> On 2012/03/20 00:48:58, noelallen1 wrote: >> > It's a question of what you are planning to ship. >> > >> > We shipped M17 apparently. There is no M18? >> > We could change M19=1.0 to M18_X and add M19=1.1 or you can add >> > M19.1=1.1 > >> > >> > But the list of labels should be clean for shipping. Those are only >> transition >> > numbers. >> > > If we put "M19 = 1.1", will the version 1.0 struct will still appear >> > in the > >> generated header? >> > > No. 1.0 will be in the header only when 1.0 and 1.1 have different > labels. > > > (I know that the 0.1 struct will, but is that because 0.1 < >> 1.1, or because "M17 = 0.1" is in the list?) >> > > because M17 = 0.1 and M17 != M19. Each generated interface has to have a > unique label. > > As Noel noted above, we could have M19_0=1.0 and M19_1=1.1, but it would > have to be cleaned up before M19 branches. Given that addition of new > methods to an interface cannot not break any existing plugins, I don't > see any problems with just keeping the version at 1.0. But it *will* break if Flash uses the new method but is pushed out to an older version of Chrome that doesn't support it. I.e., the plugin has no way of knowing whether that method is valid. It asks for version 1.0 and gets a valid struct. If it thinks that there's a method in that last spot, but it's on older chrome, it will try to dereference a function pointer in garbage memory. In general, we need to rev the version even when "only" adding new functions to the end of an interface. > > > > > >> > On 2012/03/16 22:31:17, sergeyu wrote: >> > > On 2012/03/16 22:19:15, viettrungluu wrote: >> > > > On 2012/03/16 21:32:53, sergeyu wrote: >> > > > > Not sure if I need to add 1.1 because it's still M19 >> > > > >> > > > Yes you do. >> > > >> > > Ok, then what label should I use here for 1.1? >> > > If I use M19 for both 1.1 and 1.0 then generator.py generates C >> > headers for > >> > 1.1, >> > > but not for 1.0 >> > >> > > > http://codereview.chromium.**org/9722008/<http://codereview.chromium.org/9722... >
On Tue, Mar 20, 2012 at 10:05 AM, David Michael <dmichael@chromium.org>wrote: > On Tue, Mar 20, 2012 at 10:52 AM, <sergeyu@chromium.org> wrote: > >> >> >> As Noel noted above, we could have M19_0=1.0 and M19_1=1.1, but it would >> have to be cleaned up before M19 branches. Given that addition of new >> methods to an interface cannot not break any existing plugins, I don't >> see any problems with just keeping the version at 1.0. > > But it *will* break if Flash uses the new method but is pushed out to an > older version of Chrome that doesn't support it. I.e., the plugin has no > way of knowing whether that method is valid. It asks for version 1.0 and > gets a valid struct. If it thinks that there's a method in that last spot, > but it's on older chrome, it will try to dereference a function pointer in > garbage memory. > Right, but most likely flash will not need to use these methods until M19 ships (it never needed these methods before). > > In general, we need to rev the version even when "only" adding new > functions to the end of an interface. >
Ok, I've added version 1.1 and now there are two labels M19_0 and M19_1
http://codereview.chromium.org/9722008/diff/11001/ppapi/cpp/private/net_addre... File ppapi/cpp/private/net_address_private.cc (right): http://codereview.chromium.org/9722008/diff/11001/ppapi/cpp/private/net_addre... ppapi/cpp/private/net_address_private.cc:32: return has_interface<PPB_NetAddress_Private_1_0>() || You should update all this. http://codereview.chromium.org/9722008/diff/11001/ppapi/cpp/private/net_addre... ppapi/cpp/private/net_address_private.cc:39: if (has_interface<PPB_NetAddress_Private_1_0>()) { And add a 1.1 case here. http://codereview.chromium.org/9722008/diff/11001/ppapi/cpp/private/net_addre... ppapi/cpp/private/net_address_private.cc:53: if (has_interface<PPB_NetAddress_Private_1_0>()) { Etc. http://codereview.chromium.org/9722008/diff/11001/ppapi/cpp/private/net_addre... ppapi/cpp/private/net_address_private.cc:155: } Probably this should either return a bool indicating success/failure, or at least set set |*addr_out| to something predictable (and invalid), in the case that 1.1 isn't available. (I guess the same could be said for |GetAnyAddress()|. Oops.) http://codereview.chromium.org/9722008/diff/11001/ppapi/cpp/private/net_addre... ppapi/cpp/private/net_address_private.cc:159: void NetAddressPrivate::CreateFromIPv6Address( " http://codereview.chromium.org/9722008/diff/11001/ppapi/shared_impl/private/n... File ppapi/shared_impl/private/net_address_private_impl.cc (right): http://codereview.chromium.org/9722008/diff/11001/ppapi/shared_impl/private/n... ppapi/shared_impl/private/net_address_private_impl.cc:338: memset(a, 0, sizeof(*a)); Might as well zero all of |data| (as in |GetAnyAddress()| above). http://codereview.chromium.org/9722008/diff/11001/ppapi/shared_impl/private/n... ppapi/shared_impl/private/net_address_private_impl.cc:350: memset(a, 0, sizeof(*a)); " http://codereview.chromium.org/9722008/diff/11001/ppapi/shared_impl/private/n... ppapi/shared_impl/private/net_address_private_impl.cc:354: a->sin6_scope_id = scope_id; Maybe we should include a GetScopeID() in the 1.1 interface as well?
http://codereview.chromium.org/9722008/diff/11001/ppapi/cpp/private/net_addre... File ppapi/cpp/private/net_address_private.cc (right): http://codereview.chromium.org/9722008/diff/11001/ppapi/cpp/private/net_addre... ppapi/cpp/private/net_address_private.cc:32: return has_interface<PPB_NetAddress_Private_1_0>() || On 2012/03/21 17:42:55, viettrungluu wrote: > You should update all this. Done. http://codereview.chromium.org/9722008/diff/11001/ppapi/cpp/private/net_addre... ppapi/cpp/private/net_address_private.cc:39: if (has_interface<PPB_NetAddress_Private_1_0>()) { On 2012/03/21 17:42:55, viettrungluu wrote: > And add a 1.1 case here. Done. http://codereview.chromium.org/9722008/diff/11001/ppapi/cpp/private/net_addre... ppapi/cpp/private/net_address_private.cc:53: if (has_interface<PPB_NetAddress_Private_1_0>()) { On 2012/03/21 17:42:55, viettrungluu wrote: > Etc. Done. http://codereview.chromium.org/9722008/diff/11001/ppapi/cpp/private/net_addre... ppapi/cpp/private/net_address_private.cc:155: } On 2012/03/21 17:42:55, viettrungluu wrote: > Probably this should either return a bool indicating success/failure, or at > least set set |*addr_out| to something predictable (and invalid), in the case > that 1.1 isn't available. Yes, agree. Done. > (I guess the same could be said for |GetAnyAddress()|. Oops.) Right, that's why didn't make it return bool in the first place. Is it too late to fix GetAnyAddress()? http://codereview.chromium.org/9722008/diff/11001/ppapi/cpp/private/net_addre... ppapi/cpp/private/net_address_private.cc:159: void NetAddressPrivate::CreateFromIPv6Address( On 2012/03/21 17:42:55, viettrungluu wrote: > " Done. http://codereview.chromium.org/9722008/diff/11001/ppapi/shared_impl/private/n... File ppapi/shared_impl/private/net_address_private_impl.cc (right): http://codereview.chromium.org/9722008/diff/11001/ppapi/shared_impl/private/n... ppapi/shared_impl/private/net_address_private_impl.cc:338: memset(a, 0, sizeof(*a)); On 2012/03/21 17:42:55, viettrungluu wrote: > Might as well zero all of |data| (as in |GetAnyAddress()| above). Done. http://codereview.chromium.org/9722008/diff/11001/ppapi/shared_impl/private/n... ppapi/shared_impl/private/net_address_private_impl.cc:350: memset(a, 0, sizeof(*a)); On 2012/03/21 17:42:55, viettrungluu wrote: > " Done. http://codereview.chromium.org/9722008/diff/11001/ppapi/shared_impl/private/n... ppapi/shared_impl/private/net_address_private_impl.cc:354: a->sin6_scope_id = scope_id; On 2012/03/21 17:42:55, viettrungluu wrote: > Maybe we should include a GetScopeID() in the 1.1 interface as well? Done.
http://codereview.chromium.org/9722008/diff/11001/ppapi/api/private/ppb_net_a... File ppapi/api/private/ppb_net_address_private.idl (right): http://codereview.chromium.org/9722008/diff/11001/ppapi/api/private/ppb_net_a... ppapi/api/private/ppb_net_address_private.idl:117: void CreateFromIPv6Address([in] uint8_t[16] ip, It seems like the natural way to represent this is an array of 8 uint16_ts. That's what your test seems to want, as well. Is there a reason you are using uint8_t? http://codereview.chromium.org/9722008/diff/18001/ppapi/cpp/private/net_addre... File ppapi/cpp/private/net_address_private.cc (right): http://codereview.chromium.org/9722008/diff/18001/ppapi/cpp/private/net_addre... ppapi/cpp/private/net_address_private.cc:43: } Is it really necessary to do the fallback here (and elsewhere in the file)? Can't you just call the oldest 1 that we know is supported? http://codereview.chromium.org/9722008/diff/18001/ppapi/shared_impl/private/n... File ppapi/shared_impl/private/net_address_private_impl.cc (right): http://codereview.chromium.org/9722008/diff/18001/ppapi/shared_impl/private/n... ppapi/shared_impl/private/net_address_private_impl.cc:349: sockaddr_in* a = reinterpret_cast<sockaddr_in*>(addr_out->data); nit: a is not a very good name
http://codereview.chromium.org/9722008/diff/11001/ppapi/api/private/ppb_net_a... File ppapi/api/private/ppb_net_address_private.idl (right): http://codereview.chromium.org/9722008/diff/11001/ppapi/api/private/ppb_net_a... ppapi/api/private/ppb_net_address_private.idl:117: void CreateFromIPv6Address([in] uint8_t[16] ip, On 2012/03/21 19:33:17, dmichael wrote: > It seems like the natural way to represent this is an array of 8 uint16_ts. I don't agree it's more natural than uint8_t. E.g. in6_addr represents IPv6 addresses as sequence of 16 bytes. So is net::IPAddressSequence. And there is no need to worry about byte order when you represent address as a sequence of bytes. > That's what your test seems to want, as well. Is there a reason you are using > uint8_t? That test was there before, if I wrote it I wouldn't use uint16_t. http://codereview.chromium.org/9722008/diff/18001/ppapi/shared_impl/private/n... File ppapi/shared_impl/private/net_address_private_impl.cc (right): http://codereview.chromium.org/9722008/diff/18001/ppapi/shared_impl/private/n... ppapi/shared_impl/private/net_address_private_impl.cc:349: sockaddr_in* a = reinterpret_cast<sockaddr_in*>(addr_out->data); On 2012/03/21 19:33:17, dmichael wrote: > nit: a is not a very good name It's consistent with the rest of the code in this file.
http://codereview.chromium.org/9722008/diff/11001/ppapi/api/private/ppb_net_a... File ppapi/api/private/ppb_net_address_private.idl (right): http://codereview.chromium.org/9722008/diff/11001/ppapi/api/private/ppb_net_a... ppapi/api/private/ppb_net_address_private.idl:117: void CreateFromIPv6Address([in] uint8_t[16] ip, On 2012/03/21 19:44:51, sergeyu wrote: > On 2012/03/21 19:33:17, dmichael wrote: > > It seems like the natural way to represent this is an array of 8 uint16_ts. > > I don't agree it's more natural than uint8_t. E.g. in6_addr represents IPv6 > addresses as sequence of 16 bytes. So is net::IPAddressSequence. Sorry, I meant net::IPAddressNumber. > And there is no > need to worry about byte order when you represent address as a sequence of > bytes. > > > That's what your test seems to want, as well. Is there a reason you are using > > uint8_t? > > That test was there before, if I wrote it I wouldn't use uint16_t.
Anyway, LGTM with (the many silly) nits addressed. http://codereview.chromium.org/9722008/diff/11001/ppapi/api/private/ppb_net_a... File ppapi/api/private/ppb_net_address_private.idl (right): http://codereview.chromium.org/9722008/diff/11001/ppapi/api/private/ppb_net_a... ppapi/api/private/ppb_net_address_private.idl:117: void CreateFromIPv6Address([in] uint8_t[16] ip, On 2012/03/21 19:44:51, sergeyu wrote: > On 2012/03/21 19:33:17, dmichael wrote: > > It seems like the natural way to represent this is an array of 8 uint16_ts. > > I don't agree it's more natural than uint8_t. E.g. in6_addr represents IPv6 > addresses as sequence of 16 bytes. So is net::IPAddressSequence. And there is no > need to worry about byte order when you represent address as a sequence of > bytes. I agree that it should be an array of |uint8_t|s; that way there's no question of host-versus-network byte order. > > > That's what your test seems to want, as well. Is there a reason you are using > > uint8_t? > > That test was there before, if I wrote it I wouldn't use uint16_t. That's a case of the common, convenient human representation versus the standard representation. http://codereview.chromium.org/9722008/diff/11001/ppapi/cpp/private/net_addre... File ppapi/cpp/private/net_address_private.cc (right): http://codereview.chromium.org/9722008/diff/11001/ppapi/cpp/private/net_addre... ppapi/cpp/private/net_address_private.cc:155: } On 2012/03/21 18:14:51, sergeyu wrote: > On 2012/03/21 17:42:55, viettrungluu wrote: > > Probably this should either return a bool indicating success/failure, or at > > least set set |*addr_out| to something predictable (and invalid), in the case > > that 1.1 isn't available. > > Yes, agree. Done. > > > (I guess the same could be said for |GetAnyAddress()|. Oops.) > > Right, that's why didn't make it return bool in the first place. Is it too late > to fix GetAnyAddress()? Fixing GetAnyAddress() should be fine. In general, changes to the C++ wrappers of the following form should be fine (esp. for Dev interfaces): - Changes that don't break users of the wrappers (adding a bool return should fall into this category). - Changes that break users in an obvious way (e.g., during compile!) and for which the changes to the code using the wrappers aren't too onerous. http://codereview.chromium.org/9722008/diff/18001/ppapi/cpp/private/net_addre... File ppapi/cpp/private/net_address_private.cc (right): http://codereview.chromium.org/9722008/diff/18001/ppapi/cpp/private/net_addre... ppapi/cpp/private/net_address_private.cc:43: } On 2012/03/21 19:33:17, dmichael wrote: > Is it really necessary to do the fallback here (and elsewhere in the file)? > Can't you just call the oldest 1 that we know is supported? I like the explicit fallback. It makes maintenance and eventual removal of the fallbacks much easier: Once M19 ships to stable, we can go through and do a pile of mass-deletes. http://codereview.chromium.org/9722008/diff/18001/ppapi/shared_impl/private/n... File ppapi/shared_impl/private/net_address_private_impl.cc (right): http://codereview.chromium.org/9722008/diff/18001/ppapi/shared_impl/private/n... ppapi/shared_impl/private/net_address_private_impl.cc:90: if (address_size >= sizeof(a->sin_addr.s_addr)) { While you're here, could you get rid of the extra space before the '{'? (Since I noticed it just now....) http://codereview.chromium.org/9722008/diff/18001/ppapi/shared_impl/private/n... ppapi/shared_impl/private/net_address_private_impl.cc:349: sockaddr_in* a = reinterpret_cast<sockaddr_in*>(addr_out->data); On 2012/03/21 19:44:51, sergeyu wrote: > On 2012/03/21 19:33:17, dmichael wrote: > > nit: a is not a very good name > It's consistent with the rest of the code in this file. Right, and since it's probably my fault: I think think it's okay due to the proximity of usage; I don't really think |addr_as_sockaddr_in| or whatever would be a great improvement. (Okay, I admit it: I used |a| simply to avoid lots of ridiculous line breaks.) http://codereview.chromium.org/9722008/diff/18001/ppapi/tests/test_net_addres... File ppapi/tests/test_net_address_private.cc (right): http://codereview.chromium.org/9722008/diff/18001/ppapi/tests/test_net_addres... ppapi/tests/test_net_address_private.cc:23: reinterpret_cast<const uint8_t*>(host), port, &addr); Eh? Is this cast really needed? http://codereview.chromium.org/9722008/diff/18001/ppapi/tests/test_net_addres... ppapi/tests/test_net_address_private.cc:33: ip[i * 2 + 1] = host[i] & 0xFF; Nit (here and elsewhere): apparently, all the other hex numbers in this file are lowercase. http://codereview.chromium.org/9722008/diff/18001/ppapi/tests/test_net_addres... ppapi/tests/test_net_address_private.cc:69: uint8_t localhost_ip[4] = {127, 0, 0, 1}; Nit (here and elsewhere): I think we usually want a space after the '{' and before the '}' (and this is the case for the existing array in the file).
lgtm modulo viettrungluu's comments
Sergey, are you going to add NaCl bindings for new functions?
lgtm It looks like the NaCl proxy is doing the right thing for 0.1 and 1.0, so just won't support 1.1 and the new functions with this CL. Given that, I think it's reasonable to update the NaCl proxy in another CL. http://codereview.chromium.org/9722008/diff/18001/ppapi/cpp/private/net_addre... File ppapi/cpp/private/net_address_private.cc (right): http://codereview.chromium.org/9722008/diff/18001/ppapi/cpp/private/net_addre... ppapi/cpp/private/net_address_private.cc:43: } On 2012/03/22 04:54:34, viettrungluu wrote: > On 2012/03/21 19:33:17, dmichael wrote: > > Is it really necessary to do the fallback here (and elsewhere in the file)? > > Can't you just call the oldest 1 that we know is supported? > > I like the explicit fallback. It makes maintenance and eventual removal of the > fallbacks much easier: Once M19 ships to stable, we can go through and do a pile > of mass-deletes. For what it's worth, for stable/public interfaces, we should not do fallback like this except when the more recent interface provides more or better functionality. Since we have to maintain stable/public interfaces anyway, there's no need to come back and remove usage of the old interface. In that case, the fallback stuff is just unnecessary code. But I agree with your assessment for a private interface if you don't plan to support the older versions in the future.
http://codereview.chromium.org/9722008/diff/11001/ppapi/cpp/private/net_addre... File ppapi/cpp/private/net_address_private.cc (right): http://codereview.chromium.org/9722008/diff/11001/ppapi/cpp/private/net_addre... ppapi/cpp/private/net_address_private.cc:155: } On 2012/03/22 04:54:34, viettrungluu wrote: > On 2012/03/21 18:14:51, sergeyu wrote: > > On 2012/03/21 17:42:55, viettrungluu wrote: > > > Probably this should either return a bool indicating success/failure, or at > > > least set set |*addr_out| to something predictable (and invalid), in the > case > > > that 1.1 isn't available. > > > > Yes, agree. Done. > > > > > (I guess the same could be said for |GetAnyAddress()|. Oops.) > > > > Right, that's why didn't make it return bool in the first place. Is it too > late > > to fix GetAnyAddress()? > > Fixing GetAnyAddress() should be fine. > > In general, changes to the C++ wrappers of the following form should be fine > (esp. for Dev interfaces): > - Changes that don't break users of the wrappers (adding a bool return should > fall into this category). > - Changes that break users in an obvious way (e.g., during compile!) and for > which the changes to the code using the wrappers aren't too onerous. Done. http://codereview.chromium.org/9722008/diff/18001/ppapi/shared_impl/private/n... File ppapi/shared_impl/private/net_address_private_impl.cc (right): http://codereview.chromium.org/9722008/diff/18001/ppapi/shared_impl/private/n... ppapi/shared_impl/private/net_address_private_impl.cc:90: if (address_size >= sizeof(a->sin_addr.s_addr)) { On 2012/03/22 04:54:34, viettrungluu wrote: > While you're here, could you get rid of the extra space before the '{'? (Since I > noticed it just now....) Done. http://codereview.chromium.org/9722008/diff/18001/ppapi/tests/test_net_addres... File ppapi/tests/test_net_address_private.cc (right): http://codereview.chromium.org/9722008/diff/18001/ppapi/tests/test_net_addres... ppapi/tests/test_net_address_private.cc:23: reinterpret_cast<const uint8_t*>(host), port, &addr); On 2012/03/22 04:54:34, viettrungluu wrote: > Eh? Is this cast really needed? No. Removed. http://codereview.chromium.org/9722008/diff/18001/ppapi/tests/test_net_addres... ppapi/tests/test_net_address_private.cc:33: ip[i * 2 + 1] = host[i] & 0xFF; On 2012/03/22 04:54:34, viettrungluu wrote: > Nit (here and elsewhere): apparently, all the other hex numbers in this file are > lowercase. Done. http://codereview.chromium.org/9722008/diff/18001/ppapi/tests/test_net_addres... ppapi/tests/test_net_address_private.cc:69: uint8_t localhost_ip[4] = {127, 0, 0, 1}; On 2012/03/22 04:54:34, viettrungluu wrote: > Nit (here and elsewhere): I think we usually want a space after the '{' and > before the '}' (and this is the case for the existing array in the file). Done.
On 2012/03/22 12:15:58, Dmitry Polukhin wrote: > Sergey, are you going to add NaCl bindings for new functions? Will do it in a separate CL, probably today.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/9722008/25001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/9722008/25010
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/9722008/24006
Change committed as 128322 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
