Implement UMA and internal data structure for tracking EWOULDBLOCK.

Implement UMA and internal data structure for tracking EWOULDBLOCK.

There are currently 2 sources of EWOULDBLOCK. One is from the system which we don't have any monitoring in place. The other is generated by the socket implementation of WebRTC. This change adds UMA for both places. The UMAs added here are

1. max consecutive EWOULDBLOCK that we receive from the system. It's protocol dependent.
2. max consecutive EWOULDBLOCK that WebRTC socket generates. Protocol independent.
3. % of packet loss in WebRTC socket layer due to its own EWOULDBLOCK generation. Protocol independent.

Tested in a call and the dtor is invoked when a call is terminated. However, if a user just closes the renderer abruptly, the dtor is not invoked such that these UMA data points will be lost.

BUG=427555
Committed: https://crrev.com/fd669ae63f860c24399867d79dcbcca528c13b6f
Cr-Commit-Position: refs/heads/master@{#301575}

[guoweis2, 2014-10-23 18:36:26]

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

[guoweis2, 2014-10-23 18:36:26]

Please take a look.

[juberti2, 2014-10-23 19:38:47]

Overall this LG. For jmidata, we'll want to track this in cricket::Connection as
well.

https://codereview.chromium.org/677473002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):

https://codereview.chromium.org/677473002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/p2p/socket_host_tcp.cc:367: if
(write_queue_.size() > send_queue_length_max_) {
I think we should record the total byte size rather than number of packets.

https://codereview.chromium.org/677473002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/p2p/socket_host_udp.cc (right):

https://codereview.chromium.org/677473002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/p2p/socket_host_udp.cc:260: result =
socket_->SendTo(
If I understand this code right, we will never return EWOULDBLOCK here, we will
always send the packet and get a notification in OnSend when the send completes.
Perhaps it would be good to understand how long that takes, i.e. record the
elapsed time in OnSend?

This also means that our timestamping code is doing the wrong thing, we need to
update timestamps whenever we pull a packet off send_queue_, since the stamp is
now incorrect.

https://codereview.chromium.org/677473002/diff/1/content/renderer/p2p/ipc_soc...
File content/renderer/p2p/ipc_socket_factory.cc (right):

https://codereview.chromium.org/677473002/diff/1/content/renderer/p2p/ipc_soc...
content/renderer/p2p/ipc_socket_factory.cc:169: uint32 max_ewb_sequence_;
I would call this max_discard_sequence_ and cur_discard_sequence_

https://codereview.chromium.org/677473002/diff/1/content/renderer/p2p/ipc_soc...
content/renderer/p2p/ipc_socket_factory.cc:173: uint32 packets_dropped_;
Call this packets_discarded_

https://codereview.chromium.org/677473002/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/677473002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:38797: +    The maximum length of
consecutive EWOULDBLOCKs generated by WebRTC's socket
Rewrite this to refer to discarded packets instead of EWOULDBLOCK (here and
below)

[guoweis2, 2014-10-24 06:05:52]

Patchset #2 (id:20001) has been deleted

[guoweis2, 2014-10-24 06:08:55]

PTAL. 

I'm using UMA_HISTOGRAM_COUNTS_10000 to track the consecutive discarded bytes.
Do you think it might be too small?

https://codereview.chromium.org/677473002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):

https://codereview.chromium.org/677473002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/p2p/socket_host_tcp.cc:367: if
(write_queue_.size() > send_queue_length_max_) {
On 2014/10/23 19:38:46, juberti2 wrote:
> I think we should record the total byte size rather than number of packets.

Done. Also changed the WebRTC's socket layer to count bytes instead of packets

https://codereview.chromium.org/677473002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/p2p/socket_host_udp.cc (right):

https://codereview.chromium.org/677473002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/p2p/socket_host_udp.cc:260: result =
socket_->SendTo(
On 2014/10/23 19:38:46, juberti2 wrote:
> If I understand this code right, we will never return EWOULDBLOCK here, we
will
> always send the packet and get a notification in OnSend when the send
completes.
> Perhaps it would be good to understand how long that takes, i.e. record the
> elapsed time in OnSend?
> 
> This also means that our timestamping code is doing the wrong thing, we need
to
> update timestamps whenever we pull a packet off send_queue_, since the stamp
is
> now incorrect.

The code should send synchronously most of the time and only calls OnSend when
it can't do that.

For the timestamping issue, I have filed a separate bug 426753 to track
separately.

https://codereview.chromium.org/677473002/diff/1/content/renderer/p2p/ipc_soc...
File content/renderer/p2p/ipc_socket_factory.cc (right):

https://codereview.chromium.org/677473002/diff/1/content/renderer/p2p/ipc_soc...
content/renderer/p2p/ipc_socket_factory.cc:169: uint32 max_ewb_sequence_;
On 2014/10/23 19:38:46, juberti2 wrote:
> I would call this max_discard_sequence_ and cur_discard_sequence_

Done. Also changed to bytes.

https://codereview.chromium.org/677473002/diff/1/content/renderer/p2p/ipc_soc...
content/renderer/p2p/ipc_socket_factory.cc:173: uint32 packets_dropped_;
On 2014/10/23 19:38:46, juberti2 wrote:
> Call this packets_discarded_

Done. Also changed to bytes.

https://codereview.chromium.org/677473002/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/677473002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:38797: +    The maximum length of
consecutive EWOULDBLOCKs generated by WebRTC's socket
On 2014/10/23 19:38:46, juberti2 wrote:
> Rewrite this to refer to discarded packets instead of EWOULDBLOCK (here and
> below)

Done and changed to bytes.

[guoweis2, 2014-10-27 17:25:26]

guoweis@google.com changed reviewers:
+ asvitkine@chromium.org, sergeyu@chromium.org

[guoweis2, 2014-10-27 17:25:26]

I'd like to roll this into M39. Would appreciate it if you could review it asap.

asvitkine@chromium.org: Please OWNER review changes in histograms.xml

sergeyu@chromium.org: Please OWNER review changes in the rest of files

[Alexei Svitkine (slow), 2014-10-27 17:45:21]

If you're planning to merge this back, this probably should have a crbug.com.

https://codereview.chromium.org/677473002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/p2p/socket_host.cc (right):

https://codereview.chromium.org/677473002/diff/40001/content/browser/renderer...
content/browser/renderer_host/p2p/socket_host.cc:675: }
Nit: Add an empty line between methods.

https://codereview.chromium.org/677473002/diff/40001/content/browser/renderer...
content/browser/renderer_host/p2p/socket_host.cc:688:
DCHECK(send_bytes_delayed_cur_ >= 0);
Nit: Can you use a DCHECK_GE?

https://codereview.chromium.org/677473002/diff/40001/content/renderer/p2p/ipc...
File content/renderer/p2p/ipc_socket_factory.cc (right):

https://codereview.chromium.org/677473002/diff/40001/content/renderer/p2p/ipc...
content/renderer/p2p/ipc_socket_factory.cc:118: void
IncrementDiscardCounters(uint32 size);
This should have a comment. What is |size|?

https://codereview.chromium.org/677473002/diff/40001/content/renderer/p2p/ipc...
content/renderer/p2p/ipc_socket_factory.cc:232:
(static_cast<double>(total_packets_)) * 100.0));
The usual way to do this is:

(packets_discarded_ * 100) / total_packets

The histogram value is an int anyway, so you don't need to do any double
conversion or division.

Same for the other histograms above where you log a percentage.

https://codereview.chromium.org/677473002/diff/40001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/677473002/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:38927: +    WebRTC socket.
Mention when this is logged.

Same for the others.

[guoweis2, 2014-10-27 18:07:07]

PTAL

https://codereview.chromium.org/677473002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/p2p/socket_host.cc (right):

https://codereview.chromium.org/677473002/diff/40001/content/browser/renderer...
content/browser/renderer_host/p2p/socket_host.cc:675: }
On 2014/10/27 17:45:21, Alexei Svitkine wrote:
> Nit: Add an empty line between methods.

Done.

https://codereview.chromium.org/677473002/diff/40001/content/browser/renderer...
content/browser/renderer_host/p2p/socket_host.cc:688:
DCHECK(send_bytes_delayed_cur_ >= 0);
On 2014/10/27 17:45:21, Alexei Svitkine wrote:
> Nit: Can you use a DCHECK_GE?

Done.

https://codereview.chromium.org/677473002/diff/40001/content/renderer/p2p/ipc...
File content/renderer/p2p/ipc_socket_factory.cc (right):

https://codereview.chromium.org/677473002/diff/40001/content/renderer/p2p/ipc...
content/renderer/p2p/ipc_socket_factory.cc:118: void
IncrementDiscardCounters(uint32 size);
On 2014/10/27 17:45:21, Alexei Svitkine wrote:
> This should have a comment. What is |size|?

Done.

https://codereview.chromium.org/677473002/diff/40001/content/renderer/p2p/ipc...
content/renderer/p2p/ipc_socket_factory.cc:232:
(static_cast<double>(total_packets_)) * 100.0));
On 2014/10/27 17:45:21, Alexei Svitkine wrote:
> The usual way to do this is:
> 
> (packets_discarded_ * 100) / total_packets
> 
> The histogram value is an int anyway, so you don't need to do any double
> conversion or division.
> 
> Same for the other histograms above where you log a percentage.

Done.

https://codereview.chromium.org/677473002/diff/40001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/677473002/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:38927: +    WebRTC socket.
On 2014/10/27 17:45:21, Alexei Svitkine wrote:
> Mention when this is logged.
> 
> Same for the others.

Done.

[Alexei Svitkine (slow), 2014-10-27 18:10:05]

histograms LGTM, but I'll leave it to your other reviewers to verify the core
logic

https://codereview.chromium.org/677473002/diff/60001/content/renderer/p2p/ipc...
File content/renderer/p2p/ipc_socket_factory.cc (right):

https://codereview.chromium.org/677473002/diff/60001/content/renderer/p2p/ipc...
content/renderer/p2p/ipc_socket_factory.cc:121: void
IncrementDiscardCounters(uint32 bytesDiscarded);
Nit: bytes_discarded.

Update .cc file and comment too.

[guoweis2, 2014-10-27 18:14:09]

On 2014/10/27 18:10:05, Alexei Svitkine wrote:
> histograms LGTM, but I'll leave it to your other reviewers to verify the core
> logic
> 
>
https://codereview.chromium.org/677473002/diff/60001/content/renderer/p2p/ipc...
> File content/renderer/p2p/ipc_socket_factory.cc (right):
> 
>
https://codereview.chromium.org/677473002/diff/60001/content/renderer/p2p/ipc...
> content/renderer/p2p/ipc_socket_factory.cc:121: void
> IncrementDiscardCounters(uint32 bytesDiscarded);
> Nit: bytes_discarded.
> 
> Update .cc file and comment too.

Fixed bytes_discarded. Thanks.

[juberti2, 2014-10-27 22:24:29]

lgtm with name updates

https://codereview.chromium.org/677473002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/p2p/socket_host.cc (right):

https://codereview.chromium.org/677473002/diff/80001/content/browser/renderer...
content/browser/renderer_host/p2p/socket_host.cc:484: int loss_rate =
(send_packets_delayed_total_ * 100) / send_packets_total_;
Not really "loss rate", more like "delayed rate"

https://codereview.chromium.org/677473002/diff/80001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/677473002/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:38794: +<histogram
name="WebRTC.MaxApplicationConsecutiveBytesDiscard">
ApplicationMaxConsecutiveBytesDiscarded

https://codereview.chromium.org/677473002/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:38803: +<histogram
name="WebRTC.MaxSystemConsecutiveDelayedBytes">
SystemMaxConsecutiveBytesDelayed

https://codereview.chromium.org/677473002/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:38822: +<histogram
name="WebRTC.PacketDiscardByApplication" units="%">
ApplicationTotalPacketsDiscarded

https://codereview.chromium.org/677473002/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:38925: +<histogram
name="WebRTC.SystemDelayedPackets" units="%">
SystemPercentPacketsDelayed

[guoweis2, 2014-10-27 22:53:18]

guoweis@google.com changed reviewers:
+ davidben@chromium.org, jamesr@chromium.org
- asvitkine@chromium.org, juberti@chromium.org, sergeyu@chromium.org

[guoweis2, 2014-10-27 22:53:19]

Hi,

I'd like to get this in tonight to catch M39 beta train tomorrow.

jamesr@ or davidben@, since sergeyu is OOO, could you rubber lgtm
ipc_socket_factory.cc for me? 

All files have been reviewed by juberti and asvitkine

https://codereview.chromium.org/677473002/diff/60001/content/renderer/p2p/ipc...
File content/renderer/p2p/ipc_socket_factory.cc (right):

https://codereview.chromium.org/677473002/diff/60001/content/renderer/p2p/ipc...
content/renderer/p2p/ipc_socket_factory.cc:121: void
IncrementDiscardCounters(uint32 bytesDiscarded);
On 2014/10/27 18:10:05, Alexei Svitkine wrote:
> Nit: bytes_discarded.
> 
> Update .cc file and comment too.

Done.

https://codereview.chromium.org/677473002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/p2p/socket_host.cc (right):

https://codereview.chromium.org/677473002/diff/80001/content/browser/renderer...
content/browser/renderer_host/p2p/socket_host.cc:484: int loss_rate =
(send_packets_delayed_total_ * 100) / send_packets_total_;
On 2014/10/27 22:24:29, juberti2 wrote:
> Not really "loss rate", more like "delayed rate"

Done.

https://codereview.chromium.org/677473002/diff/80001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/677473002/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:38794: +<histogram
name="WebRTC.MaxApplicationConsecutiveBytesDiscard">
On 2014/10/27 22:24:29, juberti2 wrote:
> ApplicationMaxConsecutiveBytesDiscarded

Done.

https://codereview.chromium.org/677473002/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:38803: +<histogram
name="WebRTC.MaxSystemConsecutiveDelayedBytes">
On 2014/10/27 22:24:29, juberti2 wrote:
> SystemMaxConsecutiveBytesDelayed

Done.

https://codereview.chromium.org/677473002/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:38822: +<histogram
name="WebRTC.PacketDiscardByApplication" units="%">
On 2014/10/27 22:24:29, juberti2 wrote:
> ApplicationTotalPacketsDiscarded

Done.

https://codereview.chromium.org/677473002/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:38925: +<histogram
name="WebRTC.SystemDelayedPackets" units="%">
On 2014/10/27 22:24:29, juberti2 wrote:
> SystemPercentPacketsDelayed

Done.

[davidben, 2014-10-27 23:20:26]

https://codereview.chromium.org/677473002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/p2p/socket_host.cc (right):

https://codereview.chromium.org/677473002/diff/100001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_host.cc:473: DCHECK(protocol_type_ !=
P2PSocketHost::UNKNOWN);
Nit: DCHECK_NE(protocol_type_, P2pSocketHost::UNKNOWN); ?

https://codereview.chromium.org/677473002/diff/100001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_host.cc:473: DCHECK(protocol_type_ !=
P2PSocketHost::UNKNOWN);
Seems this should be a parameter to P2PSocketHost's constructor. Then you
wouldn't need the UNKNOWN type and the compiler would enforce that every
subclass initialized it.

https://codereview.chromium.org/677473002/diff/100001/content/renderer/p2p/ip...
File content/renderer/p2p/ipc_socket_factory.cc (right):

https://codereview.chromium.org/677473002/diff/100001/content/renderer/p2p/ip...
content/renderer/p2p/ipc_socket_factory.cc:120: // cur_discard_bytes_sequence_
counting.
The zero special-case seems pretty different. Why not its own function?
ResetDiscardCounter() or something.

[guoweis2, 2014-10-27 23:48:49]

PTAL

https://codereview.chromium.org/677473002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/p2p/socket_host.cc (right):

https://codereview.chromium.org/677473002/diff/100001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_host.cc:473: DCHECK(protocol_type_ !=
P2PSocketHost::UNKNOWN);
On 2014/10/27 23:20:26, David Benjamin wrote:
> Seems this should be a parameter to P2PSocketHost's constructor. Then you
> wouldn't need the UNKNOWN type and the compiler would enforce that every
> subclass initialized it.

Done.

https://codereview.chromium.org/677473002/diff/100001/content/renderer/p2p/ip...
File content/renderer/p2p/ipc_socket_factory.cc (right):

https://codereview.chromium.org/677473002/diff/100001/content/renderer/p2p/ip...
content/renderer/p2p/ipc_socket_factory.cc:120: // cur_discard_bytes_sequence_
counting.
On 2014/10/27 23:20:26, David Benjamin wrote:
> The zero special-case seems pretty different. Why not its own function?
> ResetDiscardCounter() or something.

Done.

[davidben, 2014-10-28 00:11:14]

patch set 6 lgtm

[jamesr, 2014-10-28 00:24:15]

https://codereview.chromium.org/677473002/diff/120001/content/renderer/p2p/ip...
File content/renderer/p2p/ipc_socket_factory.cc (right):

https://codereview.chromium.org/677473002/diff/120001/content/renderer/p2p/ip...
content/renderer/p2p/ipc_socket_factory.cc:118: // Keep track of the max
discarded byte sequence in the IpcPacketSocket
what does 'max' mean here? maximum length? maximum value?

https://codereview.chromium.org/677473002/diff/120001/content/renderer/p2p/ip...
content/renderer/p2p/ipc_socket_factory.cc:122: // Reset
cur_discard_bytes_sequence_ to 0.
not a useful comment. if this just sets one variable to 0, why is it even a
function?

https://codereview.chromium.org/677473002/diff/120001/content/renderer/p2p/ip...
content/renderer/p2p/ipc_socket_factory.cc:172: // Track the maximum & current
consecutive bytes discarded due to not enough
use words in comments, not symbols.

https://codereview.chromium.org/677473002/diff/120001/content/renderer/p2p/ip...
content/renderer/p2p/ipc_socket_factory.cc:174: uint32
max_discard_bytes_sequence_;
we use uint32_t, etc, not the _t-less versions

if this is representing a number of bytes it should be size_t.  it's unclear if
that's what it means, however

https://codereview.chromium.org/677473002/diff/120001/content/renderer/p2p/ip...
content/renderer/p2p/ipc_socket_factory.cc:175: uint32
cur_discard_bytes_sequence_;
don't use abbreviations. if 'cur' means current, say 'current'

https://codereview.chromium.org/677473002/diff/120001/content/renderer/p2p/ip...
content/renderer/p2p/ipc_socket_factory.cc:177: // Track total count of packet
drops and attempt of packet sends.
this isn't very grammatical, could you try to rephrase? ideally this would be a
sentence

https://codereview.chromium.org/677473002/diff/120001/content/renderer/p2p/ip...
content/renderer/p2p/ipc_socket_factory.cc:235:
UMA_HISTOGRAM_PERCENTAGE("WebRTC.ApplicationPercenetPacketsDiscarded",
'Percenet' - you probably mean Percent right?

[guoweis2, 2014-10-28 04:43:58]

On 2014/10/28 00:24:15, jamesr wrote:
>
https://codereview.chromium.org/677473002/diff/120001/content/renderer/p2p/ip...
> File content/renderer/p2p/ipc_socket_factory.cc (right):
> 
>
https://codereview.chromium.org/677473002/diff/120001/content/renderer/p2p/ip...
> content/renderer/p2p/ipc_socket_factory.cc:118: // Keep track of the max
> discarded byte sequence in the IpcPacketSocket
> what does 'max' mean here? maximum length? maximum value?
> 
>
https://codereview.chromium.org/677473002/diff/120001/content/renderer/p2p/ip...
> content/renderer/p2p/ipc_socket_factory.cc:122: // Reset
> cur_discard_bytes_sequence_ to 0.
> not a useful comment. if this just sets one variable to 0, why is it even a
> function?
> 
>
https://codereview.chromium.org/677473002/diff/120001/content/renderer/p2p/ip...
> content/renderer/p2p/ipc_socket_factory.cc:172: // Track the maximum & current
> consecutive bytes discarded due to not enough
> use words in comments, not symbols.
> 
>
https://codereview.chromium.org/677473002/diff/120001/content/renderer/p2p/ip...
> content/renderer/p2p/ipc_socket_factory.cc:174: uint32
> max_discard_bytes_sequence_;
> we use uint32_t, etc, not the _t-less versions
> 
> if this is representing a number of bytes it should be size_t.  it's unclear
if
> that's what it means, however
> 
>
https://codereview.chromium.org/677473002/diff/120001/content/renderer/p2p/ip...
> content/renderer/p2p/ipc_socket_factory.cc:175: uint32
> cur_discard_bytes_sequence_;
> don't use abbreviations. if 'cur' means current, say 'current'
> 
>
https://codereview.chromium.org/677473002/diff/120001/content/renderer/p2p/ip...
> content/renderer/p2p/ipc_socket_factory.cc:177: // Track total count of packet
> drops and attempt of packet sends.
> this isn't very grammatical, could you try to rephrase? ideally this would be
a
> sentence
> 
>
https://codereview.chromium.org/677473002/diff/120001/content/renderer/p2p/ip...
> content/renderer/p2p/ipc_socket_factory.cc:235:
> UMA_HISTOGRAM_PERCENTAGE("WebRTC.ApplicationPercenetPacketsDiscarded",
> 'Percenet' - you probably mean Percent right?

I have addressed all the issue in the most recent batch. I'll do one more
browser testing and land it tonight.

[guoweis2, 2014-10-28 04:44:10]

https://codereview.chromium.org/677473002/diff/120001/content/renderer/p2p/ip...
File content/renderer/p2p/ipc_socket_factory.cc (right):

https://codereview.chromium.org/677473002/diff/120001/content/renderer/p2p/ip...
content/renderer/p2p/ipc_socket_factory.cc:118: // Keep track of the max
discarded byte sequence in the IpcPacketSocket
On 2014/10/28 00:24:15, jamesr wrote:
> what does 'max' mean here? maximum length? maximum value?

Rephrased this.

https://codereview.chromium.org/677473002/diff/120001/content/renderer/p2p/ip...
content/renderer/p2p/ipc_socket_factory.cc:122: // Reset
cur_discard_bytes_sequence_ to 0.
On 2014/10/28 00:24:15, jamesr wrote:
> not a useful comment. if this just sets one variable to 0, why is it even a
> function?

Removed this function.

https://codereview.chromium.org/677473002/diff/120001/content/renderer/p2p/ip...
content/renderer/p2p/ipc_socket_factory.cc:172: // Track the maximum & current
consecutive bytes discarded due to not enough
On 2014/10/28 00:24:15, jamesr wrote:
> use words in comments, not symbols.

Done.

https://codereview.chromium.org/677473002/diff/120001/content/renderer/p2p/ip...
content/renderer/p2p/ipc_socket_factory.cc:174: uint32
max_discard_bytes_sequence_;
On 2014/10/28 00:24:15, jamesr wrote:
> we use uint32_t, etc, not the _t-less versions
> 
> if this is representing a number of bytes it should be size_t.  it's unclear
if
> that's what it means, however

Done.

https://codereview.chromium.org/677473002/diff/120001/content/renderer/p2p/ip...
content/renderer/p2p/ipc_socket_factory.cc:175: uint32
cur_discard_bytes_sequence_;
On 2014/10/28 00:24:15, jamesr wrote:
> don't use abbreviations. if 'cur' means current, say 'current'

Done.

https://codereview.chromium.org/677473002/diff/120001/content/renderer/p2p/ip...
content/renderer/p2p/ipc_socket_factory.cc:177: // Track total count of packet
drops and attempt of packet sends.
On 2014/10/28 00:24:15, jamesr wrote:
> this isn't very grammatical, could you try to rephrase? ideally this would be
a
> sentence

Done.

https://codereview.chromium.org/677473002/diff/120001/content/renderer/p2p/ip...
content/renderer/p2p/ipc_socket_factory.cc:235:
UMA_HISTOGRAM_PERCENTAGE("WebRTC.ApplicationPercenetPacketsDiscarded",
On 2014/10/28 00:24:15, jamesr wrote:
> 'Percenet' - you probably mean Percent right?

Thanks for catching this. My test should have caught this but I'll double check
why it doesn't.

[jamesr, 2014-10-28 04:46:23]

lgtm

[guoweis2, 2014-10-28 05:10:18]

The CQ bit was checked by guoweis@google.com

[commit-bot: I haz the power, 2014-10-28 05:11:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/677473002/140001

[commit-bot: I haz the power, 2014-10-28 06:29:09]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2014-10-28 06:30:04]

Patchset 7 (id:??) landed as
https://crrev.com/fd669ae63f860c24399867d79dcbcca528c13b6f
Cr-Commit-Position: refs/heads/master@{#301575}