Allow p2p UDP packages to set DSCP

Implement support for p2p socket UDP packages to set the Differntiated Services Code Point for each package. Will be used by webrtc to hopefully improve performance. (See bug for how we will test this.)

BUG=277022
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229975

[Alpha Left Google, 2013-08-07 00:45:16]

Please add a reviewer for net/udp since I'm not an owner. But I prefer to have a
enum include file instead of an int for QoS values.

Also there's compilation error with a test.

https://codereview.chromium.org/22381012/diff/1/net/udp/udp_socket_libevent.h
File net/udp/udp_socket_libevent.h (right):

https://codereview.chromium.org/22381012/diff/1/net/udp/udp_socket_libevent.h...
net/udp/udp_socket_libevent.h:161: int SetToS(int dscp);
Should we provide a enum include file that lists all the QoS values instead of
just an int.

[hubbe, 2013-08-07 17:20:35]

https://codereview.chromium.org/22381012/diff/1/net/udp/udp_socket_libevent.h
File net/udp/udp_socket_libevent.h (right):

https://codereview.chromium.org/22381012/diff/1/net/udp/udp_socket_libevent.h...
net/udp/udp_socket_libevent.h:161: int SetToS(int dscp);
On 2013/08/07 00:45:16, Alpha wrote:
> Should we provide a enum include file that lists all the QoS values instead of
> just an int.

Done.

[Alpha Left Google, 2013-08-07 17:52:40]

Still have compilation errors.

[juberti2, 2013-08-08 00:44:45]

Note that when multiplexing audio and video, we will want to set QoS on a
per-packet basis. The current implementation seems to assume that all options
will be set at binding time.

Not a blocker for this CL, but something to at least start thinking about.

https://codereview.chromium.org/22381012/diff/12001/content/browser/renderer_...
File content/browser/renderer_host/p2p/socket_host_udp.cc (right):

https://codereview.chromium.org/22381012/diff/12001/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_udp.cc:69:
socket_->SetToS(net::DSCP_AF41);
Seems a little dangerous to set this by default. Suggest we expose this as an
API and then we can change libjingle to set it appropriately for the media type
in use.

[hubbe, 2013-08-08 23:23:00]

https://codereview.chromium.org/22381012/diff/12001/content/browser/renderer_...
File content/browser/renderer_host/p2p/socket_host_udp.cc (right):

https://codereview.chromium.org/22381012/diff/12001/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_udp.cc:69:
socket_->SetToS(net::DSCP_AF41);
On 2013/08/08 00:44:45, juberti2 wrote:
> Seems a little dangerous to set this by default. Suggest we expose this as an
> API and then we can change libjingle to set it appropriately for the media
type
> in use.

Changed this CL to plumb dscp through the IPC layer and expose it on a package
level. Assuming this is a good implementation, what would be the next step?

[Alpha Left Google, 2013-08-08 23:28:26]

*/p2p/*: LGTM

Still need owner of net to review changes in UDP sockets. Next step is for
juberti@ to assign someone to add that in libjingle such that WebRTC can make
use of this.

https://codereview.chromium.org/22381012/diff/21001/net/udp/udp_socket_libeve...
File net/udp/udp_socket_libevent.cc (right):

https://codereview.chromium.org/22381012/diff/21001/net/udp/udp_socket_libeve...
net/udp/udp_socket_libevent.cc:534: 
nit: Don't add empty lines.

[Alpha Left Google, 2013-08-08 23:30:09]

On 2013/08/08 23:28:26, Alpha wrote:
> */p2p/*: LGTM
> 
> Still need owner of net to review changes in UDP sockets. Next step is for
> juberti@ to assign someone to add that in libjingle such that WebRTC can make
> use of this.
> 
>
https://codereview.chromium.org/22381012/diff/21001/net/udp/udp_socket_libeve...
> File net/udp/udp_socket_libevent.cc (right):
> 
>
https://codereview.chromium.org/22381012/diff/21001/net/udp/udp_socket_libeve...
> net/udp/udp_socket_libevent.cc:534: 
> nit: Don't add empty lines.

[Alpha Left Google, 2013-08-08 23:30:39]

Next step is to have libjingle provide the DSCP info through this API:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...

[juberti2, 2013-08-09 01:05:23]

lgtm

Overall this looks great, one comment about the API exposed upwards.

https://codereview.chromium.org/22381012/diff/21001/content/renderer/p2p/sock...
File content/renderer/p2p/socket_client.h (right):

https://codereview.chromium.org/22381012/diff/21001/content/renderer/p2p/sock...
content/renderer/p2p/socket_client.h:62: net::DiffServCodePoint dscp);
Not sure it makes sense to use an enumerated type for |dscp|, given that the
supplied values will typically be ints, e.g. as obtained from
http://tools.ietf.org/html/draft-dhesikan-tsvwg-rtcweb-qos-02. If you made it an
int, you could pass values from DiffServCodePoint, but also allow explicit DSCP
settings to be passed without casting.

[hubbe, 2013-08-15 23:31:10]

https://codereview.chromium.org/22381012/diff/21001/content/renderer/p2p/sock...
File content/renderer/p2p/socket_client.h (right):

https://codereview.chromium.org/22381012/diff/21001/content/renderer/p2p/sock...
content/renderer/p2p/socket_client.h:62: net::DiffServCodePoint dscp);
On 2013/08/09 01:05:24, juberti2 wrote:
> Not sure it makes sense to use an enumerated type for |dscp|, given that the
> supplied values will typically be ints, e.g. as obtained from
> http://tools.ietf.org/html/draft-dhesikan-tsvwg-rtcweb-qos-02. If you made it
an
> int, you could pass values from DiffServCodePoint, but also allow explicit
DSCP
> settings to be passed without casting.

Got a third and a fourth opinion, I think people generally prefer the enum.

https://codereview.chromium.org/22381012/diff/21001/net/udp/udp_socket_libeve...
File net/udp/udp_socket_libevent.cc (right):

https://codereview.chromium.org/22381012/diff/21001/net/udp/udp_socket_libeve...
net/udp/udp_socket_libevent.cc:534: 
On 2013/08/08 23:28:26, Alpha wrote:
> nit: Don't add empty lines.

Done.

[hubbe, 2013-08-15 23:39:58]

Adding OWNERS.

content/common/p2p_messages.h security review = cevans
content/renderer/p2p/* = sergeyu
/net/* = cbentzel

[Sergey Ulanov, 2013-08-17 05:50:03]

https://codereview.chromium.org/22381012/diff/34001/content/browser/renderer_...
File content/browser/renderer_host/p2p/socket_dispatcher_host.h (right):

https://codereview.chromium.org/22381012/diff/34001/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_dispatcher_host.h:71:
net::DiffServCodePoint dscp);
nit: |dscp| is not readable. maybe call it |service_code|?

https://codereview.chromium.org/22381012/diff/34001/content/browser/renderer_...
File content/browser/renderer_host/p2p/socket_host_udp.cc (right):

https://codereview.chromium.org/22381012/diff/34001/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_udp.cc:208: } else if
(!IsTransientError(result) && last_dscp_ != net::DSCP_CS0) {
IsTransientError() is used for errors returned from SendTo(). Why do we need it
here?

https://codereview.chromium.org/22381012/diff/34001/content/renderer/p2p/sock...
File content/renderer/p2p/socket_client.h (right):

https://codereview.chromium.org/22381012/diff/34001/content/renderer/p2p/sock...
content/renderer/p2p/socket_client.h:60: void SendWithDscp(const
net::IPEndPoint& address,
Do we expect to change dscp often? Maybe add SetTos(dscp) instead?

Otherwise SendWithDscp() affects all Send() that are called after it that
doesn't seem right.

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

https://codereview.chromium.org/22381012/diff/34001/net/base/net_util.h#newco...
net/base/net_util.h:541: enum DiffServCodePoint {
nit: Is there a better name? code style discourages abbreviations, and this one
is hard to understand without searching it up first.
Maybe something like IPServiceCode or IPTosLevel?

https://codereview.chromium.org/22381012/diff/34001/net/base/net_util.h#newco...
net/base/net_util.h:541: enum DiffServCodePoint {
net_util.h contains lots of unrelated things. maybe create a separate header?
(though it's up to net owners)

https://codereview.chromium.org/22381012/diff/34001/net/base/net_util.h#newco...
net/base/net_util.h:542: DSCP_NO_CHANGE = -1,
DEFAULT?

https://codereview.chromium.org/22381012/diff/34001/net/base/net_util.h#newco...
net/base/net_util.h:566: 
nit: remove extra empty line.

https://codereview.chromium.org/22381012/diff/34001/net/udp/datagram_server_s...
File net/udp/datagram_server_socket.h (right):

https://codereview.chromium.org/22381012/diff/34001/net/udp/datagram_server_s...
net/udp/datagram_server_socket.h:97: // Set the Differentiated Services Code
Point.
nit: This is a strange way to format comments. why one sentence per line? I
understand that comments above are the same, so it would be great to reformat
them as well :-)

https://codereview.chromium.org/22381012/diff/34001/net/udp/datagram_server_s...
net/udp/datagram_server_socket.h:101: virtual int SetToS(DiffServCodePoint dscp)
= 0;
Maybe call it SetTos()? SetToS reads as "set to s" which makes no sense.

https://codereview.chromium.org/22381012/diff/34001/net/udp/datagram_server_s...
net/udp/datagram_server_socket.h:101: virtual int SetToS(DiffServCodePoint dscp)
= 0;
This seem like something that belongs to UDPServerSocket, but not here. It's
specific to datagram sockets that work over IP.

[cbentzel, 2013-08-19 13:54:49]

Please add a bug.

How do you plan to measure whether this actually provides benefit? 

I'm always resistant to additional complexity in the name of optimization if
there is no measurement infrastructure in place.

[juberti2, 2013-08-19 16:06:05]

https://codereview.chromium.org/22381012/diff/34001/content/renderer/p2p/sock...
File content/renderer/p2p/socket_client.h (right):

https://codereview.chromium.org/22381012/diff/34001/content/renderer/p2p/sock...
content/renderer/p2p/socket_client.h:60: void SendWithDscp(const
net::IPEndPoint& address,
On 2013/08/17 05:50:04, Sergey Ulanov wrote:
> Do we expect to change dscp often? Maybe add SetTos(dscp) instead?
> 
> Otherwise SendWithDscp() affects all Send() that are called after it that
> doesn't seem right.

For hierarchically encoded video, we may change dscp as often as every packet.

[hubbe, 2013-08-21 18:51:36]

Updated Cl description, linked to a bug which outlines how we will measure
effectiveness.

https://codereview.chromium.org/22381012/diff/34001/content/browser/renderer_...
File content/browser/renderer_host/p2p/socket_dispatcher_host.h (right):

https://codereview.chromium.org/22381012/diff/34001/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_dispatcher_host.h:71:
net::DiffServCodePoint dscp);
On 2013/08/17 05:50:04, Sergey Ulanov wrote:
> nit: |dscp| is not readable. maybe call it |service_code|?

dscp seems to be a pretty common abbreviation of "Differentiated Services Code
Point", "service_code" doesn't seem to mean anything, at least not to me.

I could see calling it  "ds_field", "dscp_value" or
"differentiated_service_code_point_value". I

https://codereview.chromium.org/22381012/diff/34001/content/browser/renderer_...
File content/browser/renderer_host/p2p/socket_host_udp.cc (right):

https://codereview.chromium.org/22381012/diff/34001/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_udp.cc:208: } else if
(!IsTransientError(result) && last_dscp_ != net::DSCP_CS0) {
On 2013/08/17 05:50:04, Sergey Ulanov wrote:
> IsTransientError() is used for errors returned from SendTo(). Why do we need
it
> here?

On some platforms setsockopt() IP_TOS is a privileged operation. The idea is
that if setsockopt() fails permanently, we want to stop calling it. Since SetToS
returns errors of the same type as SendTo(), it seems reasonable to use the same
function.

https://codereview.chromium.org/22381012/diff/34001/content/renderer/p2p/sock...
File content/renderer/p2p/socket_client.h (right):

https://codereview.chromium.org/22381012/diff/34001/content/renderer/p2p/sock...
content/renderer/p2p/socket_client.h:60: void SendWithDscp(const
net::IPEndPoint& address,
On 2013/08/17 05:50:04, Sergey Ulanov wrote:
> Do we expect to change dscp often? Maybe add SetTos(dscp) instead?
> 
> Otherwise SendWithDscp() affects all Send() that are called after it that
> doesn't seem right.

I'll changing Send() to default to SendWithDscp(..., DSCP_SC0);

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

https://codereview.chromium.org/22381012/diff/34001/net/base/net_util.h#newco...
net/base/net_util.h:541: enum DiffServCodePoint {
On 2013/08/17 05:50:04, Sergey Ulanov wrote:
> nit: Is there a better name? code style discourages abbreviations, and this
one
> is hard to understand without searching it up first.
> Maybe something like IPServiceCode or IPTosLevel?

I can call it DifferentiatedServicesCodePoint.
Adding link to RFC for more detail.

https://codereview.chromium.org/22381012/diff/34001/net/base/net_util.h#newco...
net/base/net_util.h:541: enum DiffServCodePoint {
On 2013/08/17 05:50:04, Sergey Ulanov wrote:
> net_util.h contains lots of unrelated things. maybe create a separate header?
> (though it's up to net owners)

cbenzel, what do you think?

https://codereview.chromium.org/22381012/diff/34001/net/base/net_util.h#newco...
net/base/net_util.h:542: DSCP_NO_CHANGE = -1,
On 2013/08/17 05:50:04, Sergey Ulanov wrote:
> DEFAULT?

Adding DSCP_DEFAULT. (with value 0)

https://codereview.chromium.org/22381012/diff/34001/net/base/net_util.h#newco...
net/base/net_util.h:566: 
On 2013/08/17 05:50:04, Sergey Ulanov wrote:
> nit: remove extra empty line.

Done.

https://codereview.chromium.org/22381012/diff/34001/net/udp/datagram_server_s...
File net/udp/datagram_server_socket.h (right):

https://codereview.chromium.org/22381012/diff/34001/net/udp/datagram_server_s...
net/udp/datagram_server_socket.h:97: // Set the Differentiated Services Code
Point.
On 2013/08/17 05:50:04, Sergey Ulanov wrote:
> nit: This is a strange way to format comments. why one sentence per line? I
> understand that comments above are the same, so it would be great to reformat
> them as well :-)

Old habits die hard. :)
I reformatted this one for now.

https://codereview.chromium.org/22381012/diff/34001/net/udp/datagram_server_s...
net/udp/datagram_server_socket.h:101: virtual int SetToS(DiffServCodePoint dscp)
= 0;
On 2013/08/17 05:50:04, Sergey Ulanov wrote:
> Maybe call it SetTos()? SetToS reads as "set to s" which makes no sense.

Done.
(Although, perhaps SetDSCP would be better...)

https://codereview.chromium.org/22381012/diff/34001/net/udp/datagram_server_s...
net/udp/datagram_server_socket.h:101: virtual int SetToS(DiffServCodePoint dscp)
= 0;
On 2013/08/17 05:50:04, Sergey Ulanov wrote:
> This seem like something that belongs to UDPServerSocket, but not here. It's
> specific to datagram sockets that work over IP.

I can't be sure, because this class doesn't document it's intent. But from what
it looks like, DatagramServerSocket is simple an interface for UDPServerSocket
to make it easy to mock out when testing. Thus, all methods in UDPServerSocket
are overrides from this class.

If DatagramServerSocket is meant to encapsulate datagrams other than UDP/IP,
then I should probably move this function definition. (But in that case, we
should probably move the multicast stuff too, since that is also an IP level
feature, is it not?)

[commit-bot: I haz the power, 2013-08-21 18:52:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hubbe@chromium.org/22381012/48001

[juberti2, 2013-08-21 19:13:13]

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

https://codereview.chromium.org/22381012/diff/34001/net/base/net_util.h#newco...
net/base/net_util.h:541: enum DiffServCodePoint {
On 2013/08/21 18:51:36, hubbe wrote:
> On 2013/08/17 05:50:04, Sergey Ulanov wrote:
> > net_util.h contains lots of unrelated things. maybe create a separate
header?
> > (though it's up to net owners)
> 
> cbenzel, what do you think?

The IETF WG is called "diffserv", so I don't think spelling it out is going to
add readability. 
http://tools.ietf.org/wg/diffserv/

[cbentzel, 2013-08-21 22:00:19]

Small fixes please. Thanks for entering the bug and the description.

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

https://codereview.chromium.org/22381012/diff/34001/net/base/net_util.h#newco...
net/base/net_util.h:541: enum DiffServCodePoint {
On 2013/08/21 19:13:13, juberti2 wrote:
> On 2013/08/21 18:51:36, hubbe wrote:
> > On 2013/08/17 05:50:04, Sergey Ulanov wrote:
> > > net_util.h contains lots of unrelated things. maybe create a separate
> header?
> > > (though it's up to net owners)
> > 
> > cbenzel, what do you think?
> 
> The IETF WG is called "diffserv", so I don't think spelling it out is going to
> add readability. 
> http://tools.ietf.org/wg/diffserv/

Agreed, DiffServ is a common abbreviation.

https://codereview.chromium.org/22381012/diff/34001/net/base/net_util.h#newco...
net/base/net_util.h:541: enum DiffServCodePoint {
On 2013/08/17 05:50:04, Sergey Ulanov wrote:
> net_util.h contains lots of unrelated things. maybe create a separate header?
> (though it's up to net owners)

net_util.h is (like other _util.h methods) a dumping ground for lots of things.

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

https://codereview.chromium.org/22381012/diff/55001/net/base/net_util.h#newco...
net/base/net_util.h:540: // Differentiated Services Code Point, replaces the old
IP_TOS flags.
It's a little confusing to specify "replaces IP_TOS" when some of the methods on
DatagramSocket are called Tos. Could you be consistent about just calling it
DiffServCodePoint?

https://codereview.chromium.org/22381012/diff/55001/net/udp/datagram_server_s...
File net/udp/datagram_server_socket.h (right):

https://codereview.chromium.org/22381012/diff/55001/net/udp/datagram_server_s...
net/udp/datagram_server_socket.h:99: virtual int SetTos(DiffServCodePoint dscp)
= 0;
Nit: SetDiffServCodePoint

https://codereview.chromium.org/22381012/diff/55001/net/udp/udp_socket_libeve...
File net/udp/udp_socket_libevent.h (right):

https://codereview.chromium.org/22381012/diff/55001/net/udp/udp_socket_libeve...
net/udp/udp_socket_libevent.h:24: UDPSocketLibevent(DatagramSocket::BindType
bind_type,
Nit: put a space back in.

https://codereview.chromium.org/22381012/diff/55001/net/udp/udp_socket_libeve...
net/udp/udp_socket_libevent.h:156: int SetTos(DiffServCodePoint dscp);
Nit: SetDiffServeCodePoint

https://codereview.chromium.org/22381012/diff/55001/net/udp/udp_socket_libeve...
net/udp/udp_socket_libevent.h:244: // Differentiated Services Code Point.
This isn't being used.

https://codereview.chromium.org/22381012/diff/55001/net/udp/udp_socket_win.h
File net/udp/udp_socket_win.h (right):

https://codereview.chromium.org/22381012/diff/55001/net/udp/udp_socket_win.h#...
net/udp/udp_socket_win.h:214: // Differentiated Services Code Point.
Not being used.

[hubbe, 2013-08-22 23:41:02]

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

https://codereview.chromium.org/22381012/diff/55001/net/base/net_util.h#newco...
net/base/net_util.h:540: // Differentiated Services Code Point, replaces the old
IP_TOS flags.
On 2013/08/21 22:00:20, cbentzel wrote:
> It's a little confusing to specify "replaces IP_TOS" when some of the methods
on
> DatagramSocket are called Tos. Could you be consistent about just calling it
> DiffServCodePoint?

Done.

https://codereview.chromium.org/22381012/diff/55001/net/udp/datagram_server_s...
File net/udp/datagram_server_socket.h (right):

https://codereview.chromium.org/22381012/diff/55001/net/udp/datagram_server_s...
net/udp/datagram_server_socket.h:99: virtual int SetTos(DiffServCodePoint dscp)
= 0;
On 2013/08/21 22:00:20, cbentzel wrote:
> Nit: SetDiffServCodePoint

Done.

https://codereview.chromium.org/22381012/diff/55001/net/udp/udp_socket_libeve...
File net/udp/udp_socket_libevent.h (right):

https://codereview.chromium.org/22381012/diff/55001/net/udp/udp_socket_libeve...
net/udp/udp_socket_libevent.h:24: UDPSocketLibevent(DatagramSocket::BindType
bind_type,
On 2013/08/21 22:00:20, cbentzel wrote:
> Nit: put a space back in.

Done.

https://codereview.chromium.org/22381012/diff/55001/net/udp/udp_socket_libeve...
net/udp/udp_socket_libevent.h:156: int SetTos(DiffServCodePoint dscp);
On 2013/08/21 22:00:20, cbentzel wrote:
> Nit: SetDiffServeCodePoint

Done.

https://codereview.chromium.org/22381012/diff/55001/net/udp/udp_socket_libeve...
net/udp/udp_socket_libevent.h:244: // Differentiated Services Code Point.
On 2013/08/21 22:00:20, cbentzel wrote:
> This isn't being used.

Oops, fixed.

https://codereview.chromium.org/22381012/diff/55001/net/udp/udp_socket_win.h
File net/udp/udp_socket_win.h (right):

https://codereview.chromium.org/22381012/diff/55001/net/udp/udp_socket_win.h#...
net/udp/udp_socket_win.h:214: // Differentiated Services Code Point.
On 2013/08/21 22:00:20, cbentzel wrote:
> Not being used.

Done.

[cbentzel, 2013-08-23 04:05:34]

https://codereview.chromium.org/22381012/diff/73001/net/udp/udp_socket_libeve...
File net/udp/udp_socket_libevent.cc (right):

https://codereview.chromium.org/22381012/diff/73001/net/udp/udp_socket_libeve...
net/udp/udp_socket_libevent.cc:662: rv = setsockopt(socket_, IPPROTO_IP, IP_TOS,
Looks like this may not work on XP. http://support.microsoft.com/kb/248611

[Sergey Ulanov, 2013-08-23 19:17:30]

Do we still need DSCP_NO_CHANGE? It looks like its not needed anymore because
P2PSocketClient doesn't use it and it doesn't make sense in
DatagramServerSocket::SetDiffServCodePoint().

LGTM otherwise.

[hubbe, 2013-08-23 21:43:10]

On 2013/08/23 19:17:30, Sergey Ulanov wrote:
> Do we still need DSCP_NO_CHANGE? It looks like its not needed anymore because
> P2PSocketClient doesn't use it and it doesn't make sense in
> DatagramServerSocket::SetDiffServCodePoint().
> 
> LGTM otherwise.

DSCP_NO_CHANGE comes in handy for disabling the dscp calls iff they have
permanent failures.
I could use a bool instead of course.
Also, I should probably find a windows machine and verify that the windows code
actually changes
the DSCP values.... :)

[Sergey Ulanov, 2013-08-24 00:00:41]

On 2013/08/23 21:43:10, hubbe wrote:
> On 2013/08/23 19:17:30, Sergey Ulanov wrote:
> > Do we still need DSCP_NO_CHANGE? It looks like its not needed anymore
because
> > P2PSocketClient doesn't use it and it doesn't make sense in
> > DatagramServerSocket::SetDiffServCodePoint().
> > 
> > LGTM otherwise.
> 
> DSCP_NO_CHANGE comes in handy for disabling the dscp calls iff they have
> permanent failures.
> I could use a bool instead of course.
> Also, I should probably find a windows machine and verify that the windows
code
> actually changes
> the DSCP values.... :)

It's used only internally in P2PSocketHostUdp. I think that doesn't justify
making it part of DiffServCodePoint. Adding a separate flag for this case would
be a better solution.

[cbentzel, 2013-08-26 18:25:51]

On 2013/08/23 21:43:10, hubbe wrote:
> On 2013/08/23 19:17:30, Sergey Ulanov wrote:
> > Do we still need DSCP_NO_CHANGE? It looks like its not needed anymore
because
> > P2PSocketClient doesn't use it and it doesn't make sense in
> > DatagramServerSocket::SetDiffServCodePoint().
> > 
> > LGTM otherwise.
> 
> DSCP_NO_CHANGE comes in handy for disabling the dscp calls iff they have
> permanent failures.
> I could use a bool instead of course.
> Also, I should probably find a windows machine and verify that the windows
code
> actually changes
> the DSCP values.... :)

You may need both XP and a Vista/7/8 machine to test.

[Mallinath (Gone from Chromium), 2013-09-12 17:30:32]

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

https://codereview.chromium.org/22381012/diff/80001/net/base/net_util.h#newco...
net/base/net_util.h:542: enum DiffServCodePoint {
Is it possible to use the values defined in ip.h (and corresponding windows
header file) system header file. That will avoid multiple declaration and
avoiding different interpretation of code points.

[juberti2, 2013-09-21 00:06:06]

What is the status of this patch? We have the rest of the libjingle stuff ready
to land for M31.

[hubbe, 2013-09-24 17:10:24]

Getting this to work on windows is taking longer than I thought it would, so for
now I'd like to stub out the windows implementation and test and evaluate if
DSCP is even a good idea using mac and linux.

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

https://codereview.chromium.org/22381012/diff/80001/net/base/net_util.h#newco...
net/base/net_util.h:542: enum DiffServCodePoint {
On 2013/09/12 17:30:33, mallinath2 wrote:
> Is it possible to use the values defined in ip.h (and corresponding windows
> header file) system header file. That will avoid multiple declaration and
> avoiding different interpretation of code points.
> 

netinet/ip.h seems to use old TOS terminology, which will confuse matters I
think.

[juberti2, 2013-09-25 00:03:56]

On 2013/09/24 17:10:24, hubbe wrote:
> Getting this to work on windows is taking longer than I thought it would, so
for
> now I'd like to stub out the windows implementation and test and evaluate if
> DSCP is even a good idea using mac and linux.
> 
> https://codereview.chromium.org/22381012/diff/80001/net/base/net_util.h
> File net/base/net_util.h (right):
> 
>
https://codereview.chromium.org/22381012/diff/80001/net/base/net_util.h#newco...
> net/base/net_util.h:542: enum DiffServCodePoint {
> On 2013/09/12 17:30:33, mallinath2 wrote:
> > Is it possible to use the values defined in ip.h (and corresponding windows
> > header file) system header file. That will avoid multiple declaration and
> > avoiding different interpretation of code points.
> > 
> 
> netinet/ip.h seems to use old TOS terminology, which will confuse matters I
> think.

sgtm, let's land this and try it out on Mac/Linux.

[hubbe1, 2013-09-27 19:41:16]

Ping?
cbentzel?
cevans?



On Mon, Aug 26, 2013 at 11:25 AM, <cbentzel@chromium.org> wrote:

> On 2013/08/23 21:43:10, hubbe wrote:
>
>> On 2013/08/23 19:17:30, Sergey Ulanov wrote:
>> > Do we still need DSCP_NO_CHANGE? It looks like its not needed anymore
>>
> because
>
>> > P2PSocketClient doesn't use it and it doesn't make sense in
>> > DatagramServerSocket::**SetDiffServCodePoint().
>> >
>> > LGTM otherwise.
>>
>
>  DSCP_NO_CHANGE comes in handy for disabling the dscp calls iff they have
>> permanent failures.
>> I could use a bool instead of course.
>> Also, I should probably find a windows machine and verify that the windows
>>
> code
>
>> actually changes
>> the DSCP values.... :)
>>
>
> You may need both XP and a Vista/7/8 machine to test.
>
>
https://codereview.chromium.**org/22381012/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[cbentzel, 2013-10-17 01:32:56]

net/ LGTM

No need to wait for another round of review from me. Apologies for the delay.

https://codereview.chromium.org/22381012/diff/94001/content/browser/renderer_...
File content/browser/renderer_host/p2p/socket_host.cc (right):

https://codereview.chromium.org/22381012/diff/94001/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host.cc:11: #include
"net/udp/datagram_socket.h"
Why is this needed? No other changes were made to this file.

https://codereview.chromium.org/22381012/diff/94001/content/browser/renderer_...
File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):

https://codereview.chromium.org/22381012/diff/94001/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_tcp.cc:276: net::DiffServCodePoint
dscp) {
Do you want to comment that |dscp| is intentionally being ignored?

https://codereview.chromium.org/22381012/diff/94001/net/udp/udp_socket_libeve...
File net/udp/udp_socket_libevent.h (right):

https://codereview.chromium.org/22381012/diff/94001/net/udp/udp_socket_libeve...
net/udp/udp_socket_libevent.h:157: int SetDiffServCodePoint(DiffServCodePoint
dscp);
Comment on what the return value is.

https://codereview.chromium.org/22381012/diff/94001/net/udp/udp_socket_win.cc
File net/udp/udp_socket_win.cc (right):

https://codereview.chromium.org/22381012/diff/94001/net/udp/udp_socket_win.cc...
net/udp/udp_socket_win.cc:759: return FAILED;
NOT_IMPLEMENTED is probably a better choice.

[hubbe, 2013-10-18 20:38:34]

Trying a different security reviewer.

[Tom Sepez, 2013-10-18 21:49:00]

lgtm

[commit-bot: I haz the power, 2013-10-21 17:17:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hubbe@chromium.org/22381012/94001

[commit-bot: I haz the power, 2013-10-21 17:17:11]

Failed to apply patch for
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/renderer_host/p2p/socket_dispatcher_host.cc
  Hunk #1 FAILED at 223.
  1 out of 1 hunk FAILED -- saving rejects to file
content/browser/renderer_host/p2p/socket_dispatcher_host.cc.rej

Patch:       content/browser/renderer_host/p2p/socket_dispatcher_host.cc
Index: content/browser/renderer_host/p2p/socket_dispatcher_host.cc
diff --git a/content/browser/renderer_host/p2p/socket_dispatcher_host.cc
b/content/browser/renderer_host/p2p/socket_dispatcher_host.cc
index
d1a60b451a8ad60b0cff7dfff1f1dff143bb34b7..be523c908d4dcd7f9fdef11128dae79c665165a0
100644
--- a/content/browser/renderer_host/p2p/socket_dispatcher_host.cc
+++ b/content/browser/renderer_host/p2p/socket_dispatcher_host.cc
@@ -223,13 +223,14 @@ void
P2PSocketDispatcherHost::OnAcceptIncomingTcpConnection(
 
 void P2PSocketDispatcherHost::OnSend(int socket_id,
                                      const net::IPEndPoint& socket_address,
-                                     const std::vector<char>& data) {
+                                     const std::vector<char>& data,
+                                     net::DiffServCodePoint dscp) {
   P2PSocketHost* socket = LookupSocket(socket_id);
   if (!socket) {
     LOG(ERROR) << "Received P2PHostMsg_Send for invalid socket_id.";
     return;
   }
-  socket->Send(socket_address, data);
+  socket->Send(socket_address, data, dscp);
 }
 
 void P2PSocketDispatcherHost::OnDestroySocket(int socket_id) {

[hubbe, 2013-10-21 19:39:20]

https://codereview.chromium.org/22381012/diff/94001/content/browser/renderer_...
File content/browser/renderer_host/p2p/socket_host.cc (right):

https://codereview.chromium.org/22381012/diff/94001/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host.cc:11: #include
"net/udp/datagram_socket.h"
On 2013/10/17 01:32:57, cbentzel wrote:
> Why is this needed? No other changes were made to this file.

Done.

https://codereview.chromium.org/22381012/diff/94001/content/browser/renderer_...
File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):

https://codereview.chromium.org/22381012/diff/94001/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_host_tcp.cc:276: net::DiffServCodePoint
dscp) {
On 2013/10/17 01:32:57, cbentzel wrote:
> Do you want to comment that |dscp| is intentionally being ignored?

Done.

https://codereview.chromium.org/22381012/diff/94001/net/udp/udp_socket_libeve...
File net/udp/udp_socket_libevent.h (right):

https://codereview.chromium.org/22381012/diff/94001/net/udp/udp_socket_libeve...
net/udp/udp_socket_libevent.h:157: int SetDiffServCodePoint(DiffServCodePoint
dscp);
On 2013/10/17 01:32:57, cbentzel wrote:
> Comment on what the return value is.

Done.

https://codereview.chromium.org/22381012/diff/94001/net/udp/udp_socket_win.cc
File net/udp/udp_socket_win.cc (right):

https://codereview.chromium.org/22381012/diff/94001/net/udp/udp_socket_win.cc...
net/udp/udp_socket_win.cc:759: return FAILED;
On 2013/10/17 01:32:57, cbentzel wrote:
> NOT_IMPLEMENTED is probably a better choice.

Done.

[commit-bot: I haz the power, 2013-10-21 21:42:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hubbe@chromium.org/22381012/253001

[commit-bot: I haz the power, 2013-10-22 00:34:49]

Change committed as 229975