proposed interface change at //net for webrtc

proposed interface change for net_util.h for webrtc

BUG=411086

[guoweis_left_chromium, 2014-09-06 04:48:33]

Patchset #2 (id:20001) has been deleted

[guoweis_left_chromium, 2014-09-06 05:19:50]

Patchset #2 (id:40001) has been deleted

[Ryan Sleevi, 2014-09-08 18:15:44]

rsleevi@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Ryan Sleevi, 2014-09-08 18:15:45]

https://codereview.chromium.org/538243003/diff/60001/net/base/net_util.h
File net/base/net_util.h (right):

https://codereview.chromium.org/538243003/diff/60001/net/base/net_util.h#newc...
net/base/net_util.h:446: // This enum follows the IFA definition in if_addr.h
This doesn't really add value for non-Linux Users.

You need to be explicit about what is documented.

https://codereview.chromium.org/538243003/diff/60001/net/base/net_util.h#newc...
net/base/net_util.h:449: IPV6_ADDRESS_FLAG_PERMANENT      = 0x80,
Why did you skip a bunch of flag values? Are you trying to be constant with some
other definition? Why?

From a mask perspective, does it make sense to be more readable of
1 << 1
1 << 2
1 << 3

etc

https://codereview.chromium.org/538243003/diff/60001/net/base/net_util.h#newc...
net/base/net_util.h:470: int ipv6_flags;
Document

[guoweis_webrtc, 2014-09-08 22:44:00]

guoweis@webrtc.org changed reviewers:
+ juberti@chromium.org, lorenzo@google.com, pauljensen@chromium.org

[guoweis_webrtc, 2014-09-08 22:44:00]

Hi,

Following up our discussion this morning, here is the code change to expose
LocalLinks and IPv6 flags. I'm working on to determine whether Permanent is
common across platforms. 

Thanks,
Guowei

[guoweis_webrtc, 2014-09-10 16:54:02]

On 2014/09/08 22:44:00, guoweis wrote:
> Hi,
> 
> Following up our discussion this morning, here is the code change to expose
> LocalLinks and IPv6 flags. I'm working on to determine whether Permanent is
> common across platforms. 
> 
> Thanks,
> Guowei

Hi, 

Any update on this? I'm hoping to get this feature turned on by M39 so your
review feedback is very appreciated.

Thanks,
Guowei

[pauljensen, 2014-09-10 17:55:19]

I don't think this builds on non-posix hosts...they call NewtworkInterface's
constructor, which you've changed.

I'm super swamped until Tuesday, so that's probably the earliest I can get
around to reviewing this more thoroughly.

This looks like it's going in the right direction.

I'm not a fan of those flags being passed around in an "int".
I'd support one of:
1. all of:
     a. defining the flags in an enum
     b. typedef'ing a type for the flags
     c. including a MAX value in the enum
     d. adding a COMPILE_ASSERT to check that MAX fits in the typedef'd type
2. all of:
     a. typedef'ing a type for the flags
     b. defining each flag as "static const typedefed_type FLAG"
3. all of:
     a. initialize the flags field to 0 in the constructor
     b. adding setFlag(IPv6AddressFlag) and getFlag(IPv6AddressFlag) accessors
     c. make the flags field a private member

https://codereview.chromium.org/538243003/diff/80001/net/base/net_util.h
File net/base/net_util.h (right):

https://codereview.chromium.org/538243003/diff/80001/net/base/net_util.h#newc...
net/base/net_util.h:441: // Only expose a subset of IPv6 address flags which is
actionable to
Remove "Only expose".
I'm not sure "IPv6 address flags" is a known term/quantity.  Is this something
from an RFC?  Maybe use the term "IPv6 address attributes".

https://codereview.chromium.org/538243003/diff/80001/net/base/net_util.h#newc...
net/base/net_util.h:442: // application layer. Addresses with flags which are
not combination
Insert "a" in between "not combination"

[lorenzo, 2014-09-11 06:06:10]

I had expected to find code to convert between the ifaddrmsg structures
reported/returned by AddressTrackerLinux and the InterfaceAddress objects that
webrtc deals with. Is that code in another CL?

https://codereview.chromium.org/538243003/diff/80001/net/base/net_util.h
File net/base/net_util.h (right):

https://codereview.chromium.org/538243003/diff/80001/net/base/net_util.h#newc...
net/base/net_util.h:441: // Only expose a subset of IPv6 address flags which is
actionable to
On 2014/09/10 17:55:18, pauljensen wrote:
> Remove "Only expose".
> I'm not sure "IPv6 address flags" is a known term/quantity.  Is this something
> from an RFC?  Maybe use the term "IPv6 address attributes".

I don't think "address flags" is a standard term. "attributes" is fine.

https://codereview.chromium.org/538243003/diff/80001/net/base/net_util.h#newc...
net/base/net_util.h:443: // of listed below will be filtered out from
GetNetworkList.
I think this comment should just describe the flags, not talk about what
GetNetworkList will or will not filter out. That information should be in the
comments for GetNetworkList.

https://codereview.chromium.org/538243003/diff/80001/net/base/net_util.h#newc...
net/base/net_util.h:444: enum IPv6AddressFlag {
Is there a reason for this enum to be IPv6-specific? I would imagine that there
are other flags you might want to associate with IP addresses.

https://codereview.chromium.org/538243003/diff/80001/net/base/net_util_posix.cc
File net/base/net_util_posix.cc (right):

https://codereview.chromium.org/538243003/diff/80001/net/base/net_util_posix....
net/base/net_util_posix.cc:145: address, network_prefix));
What about the flags? Will this compile?

[guoweis_left_chromium, 2014-09-11 06:12:35]

On 2014/09/11 06:06:10, lorenzo wrote:
> I had expected to find code to convert between the ifaddrmsg structures
> reported/returned by AddressTrackerLinux and the InterfaceAddress objects that
> webrtc deals with. Is that code in another CL?
> 
> https://codereview.chromium.org/538243003/diff/80001/net/base/net_util.h
> File net/base/net_util.h (right):
> 
>
https://codereview.chromium.org/538243003/diff/80001/net/base/net_util.h#newc...
> net/base/net_util.h:441: // Only expose a subset of IPv6 address flags which
is
> actionable to
> On 2014/09/10 17:55:18, pauljensen wrote:
> > Remove "Only expose".
> > I'm not sure "IPv6 address flags" is a known term/quantity.  Is this
something
> > from an RFC?  Maybe use the term "IPv6 address attributes".
> 
> I don't think "address flags" is a standard term. "attributes" is fine.
> 
>
https://codereview.chromium.org/538243003/diff/80001/net/base/net_util.h#newc...
> net/base/net_util.h:443: // of listed below will be filtered out from
> GetNetworkList.
> I think this comment should just describe the flags, not talk about what
> GetNetworkList will or will not filter out. That information should be in the
> comments for GetNetworkList.
> 
>
https://codereview.chromium.org/538243003/diff/80001/net/base/net_util.h#newc...
> net/base/net_util.h:444: enum IPv6AddressFlag {
> Is there a reason for this enum to be IPv6-specific? I would imagine that
there
> are other flags you might want to associate with IP addresses.
> 
>
https://codereview.chromium.org/538243003/diff/80001/net/base/net_util_posix.cc
> File net/base/net_util_posix.cc (right):
> 
>
https://codereview.chromium.org/538243003/diff/80001/net/base/net_util_posix....
> net/base/net_util_posix.cc:145: address, network_prefix));
> What about the flags? Will this compile?

Thanks for feedbacks. My bad, for this change, I only verified on linux in terms
of build. I'll get these issues addressed this week so Paul could take another
look next week.

[guoweis_webrtc, 2014-09-12 23:55:20]

guoweis@webrtc.org changed reviewers:
- juberti@chromium.org, rsleevi@chromium.org

[guoweis_webrtc, 2014-09-12 23:55:22]

Please see the response to the comments. 

@Paul: I don't understand your point above about the attributes. Since these
attributes could be combined, I don't think, for example, concepts like MAX
applies very well here. 

The current possibility of attribute combinations include 
TEMP|DEPRECATED, TEMP, NONE

However, defining them in this way could make future addition hard.

https://codereview.chromium.org/538243003/diff/60001/net/base/net_util.h
File net/base/net_util.h (right):

https://codereview.chromium.org/538243003/diff/60001/net/base/net_util.h#newc...
net/base/net_util.h:446: // This enum follows the IFA definition in if_addr.h
On 2014/09/08 18:15:44, Ryan Sleevi wrote:
> This doesn't really add value for non-Linux Users.
> 
> You need to be explicit about what is documented.

Removed that line.

https://codereview.chromium.org/538243003/diff/60001/net/base/net_util.h#newc...
net/base/net_util.h:449: IPV6_ADDRESS_FLAG_PERMANENT      = 0x80,
On 2014/09/08 18:15:44, Ryan Sleevi wrote:
> Why did you skip a bunch of flag values? Are you trying to be constant with
some
> other definition? Why?
> 
> From a mask perspective, does it make sense to be more readable of
> 1 << 1
> 1 << 2
> 1 << 3
> 
> etc

Done.

https://codereview.chromium.org/538243003/diff/60001/net/base/net_util.h#newc...
net/base/net_util.h:470: int ipv6_flags;
On 2014/09/08 18:15:44, Ryan Sleevi wrote:
> Document

I put a comment after the variable 
// Combination of |IPv6AddressFlag|.

Please let me know if this doesn't serve the purpose.

https://codereview.chromium.org/538243003/diff/80001/net/base/net_util.h
File net/base/net_util.h (right):

https://codereview.chromium.org/538243003/diff/80001/net/base/net_util.h#newc...
net/base/net_util.h:441: // Only expose a subset of IPv6 address flags which is
actionable to
On 2014/09/10 17:55:18, pauljensen wrote:
> Remove "Only expose".
> I'm not sure "IPv6 address flags" is a known term/quantity.  Is this something
> from an RFC?  Maybe use the term "IPv6 address attributes".

Done. Changed the name to IPAddressAttributes.

https://codereview.chromium.org/538243003/diff/80001/net/base/net_util.h#newc...
net/base/net_util.h:441: // Only expose a subset of IPv6 address flags which is
actionable to
On 2014/09/10 17:55:18, pauljensen wrote:
> Remove "Only expose".
> I'm not sure "IPv6 address flags" is a known term/quantity.  Is this something
> from an RFC?  Maybe use the term "IPv6 address attributes".

Changed the name to IPAddressAttributes.

https://codereview.chromium.org/538243003/diff/80001/net/base/net_util.h#newc...
net/base/net_util.h:442: // application layer. Addresses with flags which are
not combination
On 2014/09/10 17:55:18, pauljensen wrote:
> Insert "a" in between "not combination"

Done.

https://codereview.chromium.org/538243003/diff/80001/net/base/net_util.h#newc...
net/base/net_util.h:443: // of listed below will be filtered out from
GetNetworkList.
On 2014/09/11 06:06:10, lorenzo wrote:
> I think this comment should just describe the flags, not talk about what
> GetNetworkList will or will not filter out. That information should be in the
> comments for GetNetworkList.

Done.

https://codereview.chromium.org/538243003/diff/80001/net/base/net_util.h#newc...
net/base/net_util.h:444: enum IPv6AddressFlag {
On 2014/09/11 06:06:10, lorenzo wrote:
> Is there a reason for this enum to be IPv6-specific? I would imagine that
there
> are other flags you might want to associate with IP addresses.

Changed the enum name to IPAddressAttributes

https://codereview.chromium.org/538243003/diff/80001/net/base/net_util_posix.cc
File net/base/net_util_posix.cc (right):

https://codereview.chromium.org/538243003/diff/80001/net/base/net_util_posix....
net/base/net_util_posix.cc:145: address, network_prefix));
On 2014/09/11 06:06:10, lorenzo wrote:
> What about the flags? Will this compile?

My bad. I have ensured trybot passed this time.

[guoweis_left_chromium, 2014-09-13 07:22:38]

Patchset #5 (id:120001) has been deleted

[guoweis_left_chromium, 2014-09-13 15:22:59]

Patchset #4 (id:100001) has been deleted

[pauljensen, 2014-09-18 14:29:54]

https://codereview.chromium.org/538243003/diff/140001/net/base/net_util.h
File net/base/net_util.h (right):

https://codereview.chromium.org/538243003/diff/140001/net/base/net_util.h#new...
net/base/net_util.h:441: // A subset of IP address attributes which is
actionable to
is->are
to->by the?
Also add "Currently unimplemented for all hosts; IP_ADDRESS_ATTRIBUTE_NONE is
always returned." and update as you add implementations.

https://codereview.chromium.org/538243003/diff/140001/net/base/net_util.h#new...
net/base/net_util.h:461: NetworkInterface(const std::string& name,
Dead?

[pauljensen, 2014-09-18 14:39:35]

On 2014/09/12 23:55:22, guoweis wrote:
> @Paul: I don't understand your point above about the attributes. Since these
> attributes could be combined, I don't think, for example, concepts like MAX
> applies very well here.
Forget it.  I was forgetting that enums fit in ints.

[guoweis2, 2014-09-18 21:05:45]

On 2014/09/18 14:39:35, pauljensen wrote:
> On 2014/09/12 23:55:22, guoweis wrote:
> > @Paul: I don't understand your point above about the attributes. Since these
> > attributes could be combined, I don't think, for example, concepts like MAX
> > applies very well here.
> Forget it.  I was forgetting that enums fit in ints.

I realized that I was using webrtc account to create this CL. Migrate to
https://codereview.chromium.org/580283002 instead.