Adds UMA histograms for WebRTC.DataChannelMaxRetransmits WebRTC.DataChannelMaxRetransmitTime.

Adds UMA histograms for WebRTC.DataChannelMaxRetransmits WebRTC.DataChannelMaxRetransmitTime.

BUG=366316
R=isherman@chromium.org, jiayl@chromium.org

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

[perkj_chrome, 2014-05-07 08:52:20]

Can you please review?

[jiayl, 2014-05-07 16:25:07]

https://codereview.chromium.org/274453004/diff/20001/content/renderer/media/r...
File content/renderer/media/rtc_data_channel_handler.cc (right):

https://codereview.chromium.org/274453004/diff/20001/content/renderer/media/r...
content/renderer/media/rtc_data_channel_handler.cc:51: maxRetransmits(), 1,
min should be 0

https://codereview.chromium.org/274453004/diff/20001/content/renderer/media/r...
content/renderer/media/rtc_data_channel_handler.cc:54: maxRetransmitTime(), 1,
min should be 0

[perkj_chrome, 2014-05-07 17:53:56]

Ilya - would you mind taking a look as well?

https://codereview.chromium.org/274453004/diff/20001/content/renderer/media/r...
File content/renderer/media/rtc_data_channel_handler.cc (right):

https://codereview.chromium.org/274453004/diff/20001/content/renderer/media/r...
content/renderer/media/rtc_data_channel_handler.cc:51: maxRetransmits(), 1,
On 2014/05/07 16:25:07, jiayl wrote:
> min should be 0
oops - I thought this was the size of each slot in the histogram.

https://codereview.chromium.org/274453004/diff/20001/content/renderer/media/r...
content/renderer/media/rtc_data_channel_handler.cc:54: maxRetransmitTime(), 1,
On 2014/05/07 16:25:07, jiayl wrote:
> min should be 0

Done.

[jiayl, 2014-05-07 17:59:24]

lgtm

[Ilya Sherman, 2014-05-08 00:27:01]

https://codereview.chromium.org/274453004/diff/40001/content/renderer/media/r...
File content/renderer/media/rtc_data_channel_handler.cc (right):

https://codereview.chromium.org/274453004/diff/40001/content/renderer/media/r...
content/renderer/media/rtc_data_channel_handler.cc:52:
std::numeric_limits<unsigned short>::max(), 50);
That's a mighty large upper bound.  Do you really expect the full range to be
used?

https://codereview.chromium.org/274453004/diff/40001/content/renderer/media/r...
content/renderer/media/rtc_data_channel_handler.cc:55:
std::numeric_limits<unsigned short>::max(), 50);
Hmm, did you consider using one of the UMA_HISTOGRAM_TIMES macros?  Though, I
guess the max value you have set is only about a minute.  That still strikes me
as high for a networking time value, but maybe it's plausible here?

[perkj_chrome, 2014-05-08 13:59:44]

Please see explanation below.

https://codereview.chromium.org/274453004/diff/40001/content/renderer/media/r...
File content/renderer/media/rtc_data_channel_handler.cc (right):

https://codereview.chromium.org/274453004/diff/40001/content/renderer/media/r...
content/renderer/media/rtc_data_channel_handler.cc:52:
std::numeric_limits<unsigned short>::max(), 50);
On 2014/05/08 00:27:02, Ilya Sherman wrote:
> That's a mighty large upper bound.  Do you really expect the full range to be
> used?

Both of these values are unsigned shorts that can be specified during
configurations from JS. So these are simply histograms of what the user has set.


std::numeric_limits<unsigned short>::max() has been used as not defined in the
code.

https://codereview.chromium.org/274453004/diff/40001/content/renderer/media/r...
content/renderer/media/rtc_data_channel_handler.cc:55:
std::numeric_limits<unsigned short>::max(), 50);
On 2014/05/08 00:27:02, Ilya Sherman wrote:
> Hmm, did you consider using one of the UMA_HISTOGRAM_TIMES macros?  Though, I
> guess the max value you have set is only about a minute.  That still strikes
me
> as high for a networking time value, but maybe it's plausible here?


dito, see explanation above.

[Ilya Sherman, 2014-05-09 00:18:50]

https://codereview.chromium.org/274453004/diff/40001/content/renderer/media/r...
File content/renderer/media/rtc_data_channel_handler.cc (right):

https://codereview.chromium.org/274453004/diff/40001/content/renderer/media/r...
content/renderer/media/rtc_data_channel_handler.cc:52:
std::numeric_limits<unsigned short>::max(), 50);
On 2014/05/08 13:59:45, perkj wrote:
> On 2014/05/08 00:27:02, Ilya Sherman wrote:
> > That's a mighty large upper bound.  Do you really expect the full range to
be
> > used?
> 
> Both of these values are unsigned shorts that can be specified during
> configurations from JS. So these are simply histograms of what the user has
set.

Yes, I see that.  However, all histograms have overflow buckets, so you don't
need to have your last non-overflow bucket be so large if you don't think it's
likely that users will actually use such large values.  OTOH, I guess it doesn't
really hurt; so if you prefer to keep things as they are, then LGTM.

[perkj_chrome, 2014-05-09 08:36:25]

The CQ bit was checked by perkj@chromium.org

[commit-bot: I haz the power, 2014-05-09 08:40:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/perkj@chromium.org/274453004/40001

[perkj_chrome, 2014-05-09 11:10:02]

Committed patchset #2 manually as r269224 (presubmit successful).