Randomly select a single rappor sample when more than one is collected.

Randomly select a single rappor sample when more than one is collected.

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

[Alexei Svitkine (slow), 2014-07-30 17:12:47]

Please file a crbug for this work.

https://codereview.chromium.org/419683014/diff/1/components/rappor/bloom_filt...
File components/rappor/bloom_filter.cc (right):

https://codereview.chromium.org/419683014/diff/1/components/rappor/bloom_filt...
components/rappor/bloom_filter.cc:43: DCHECK_EQ(bytes.size(), bytes_.size());
Nit: Swap params (expected size should be on the left).

https://codereview.chromium.org/419683014/diff/1/components/rappor/rappor_met...
File components/rappor/rappor_metric.cc (right):

https://codereview.chromium.org/419683014/diff/1/components/rappor/rappor_met...
components/rappor/rappor_metric.cc:16: parameters_(parameters),
Init sample_count_ here.

https://codereview.chromium.org/419683014/diff/1/components/rappor/rappor_met...
components/rappor/rappor_metric.cc:28: ++sample_count;
Should be sample_count_, not sample_count.

https://codereview.chromium.org/419683014/diff/1/components/rappor/rappor_met...
components/rappor/rappor_metric.cc:29: // Replace the previous sample with a 1
in sample_count chance so that each
Nit: sample_count_.

https://codereview.chromium.org/419683014/diff/1/components/rappor/rappor_met...
components/rappor/rappor_metric.cc:33: bloom_filter_.AddString(str);
Any reason why we can't just change AddString() to SetString() which does Clear
internally?

[Steven Holte, 2014-08-04 23:30:27]

https://codereview.chromium.org/419683014/diff/1/components/rappor/bloom_filt...
File components/rappor/bloom_filter.cc (right):

https://codereview.chromium.org/419683014/diff/1/components/rappor/bloom_filt...
components/rappor/bloom_filter.cc:43: DCHECK_EQ(bytes.size(), bytes_.size());
On 2014/07/30 17:12:47, Alexei Svitkine wrote:
> Nit: Swap params (expected size should be on the left).

Done.

https://codereview.chromium.org/419683014/diff/1/components/rappor/rappor_met...
File components/rappor/rappor_metric.cc (right):

https://codereview.chromium.org/419683014/diff/1/components/rappor/rappor_met...
components/rappor/rappor_metric.cc:16: parameters_(parameters),
On 2014/07/30 17:12:47, Alexei Svitkine wrote:
> Init sample_count_ here.

Done.

https://codereview.chromium.org/419683014/diff/1/components/rappor/rappor_met...
components/rappor/rappor_metric.cc:28: ++sample_count;
On 2014/07/30 17:12:47, Alexei Svitkine wrote:
> Should be sample_count_, not sample_count.

Done.

https://codereview.chromium.org/419683014/diff/1/components/rappor/rappor_met...
components/rappor/rappor_metric.cc:29: // Replace the previous sample with a 1
in sample_count chance so that each
On 2014/07/30 17:12:47, Alexei Svitkine wrote:
> Nit: sample_count_.

Done.

https://codereview.chromium.org/419683014/diff/1/components/rappor/rappor_met...
components/rappor/rappor_metric.cc:33: bloom_filter_.AddString(str);
On 2014/07/30 17:12:47, Alexei Svitkine wrote:
> Any reason why we can't just change AddString() to SetString() which does
Clear
> internally?

Done.

[Alexei Svitkine (slow), 2014-08-05 13:22:05]

LGTM

https://codereview.chromium.org/419683014/diff/20001/components/rappor/rappor...
File components/rappor/rappor_metric.h (right):

https://codereview.chromium.org/419683014/diff/20001/components/rappor/rappor...
components/rappor/rappor_metric.h:38: // A random sample will be used when
reporting this metric.
Nit: expand comment to mention "when more than one sample is added for the same
metric"

[Steven Holte, 2014-08-05 20:32:32]

https://codereview.chromium.org/419683014/diff/20001/components/rappor/rappor...
File components/rappor/rappor_metric.h (right):

https://codereview.chromium.org/419683014/diff/20001/components/rappor/rappor...
components/rappor/rappor_metric.h:38: // A random sample will be used when
reporting this metric.
On 2014/08/05 13:22:05, Alexei Svitkine wrote:
> Nit: expand comment to mention "when more than one sample is added for the
same
> metric"

Done.

[Steven Holte, 2014-08-05 20:32:44]

The CQ bit was checked by holte@chromium.org

[commit-bot: I haz the power, 2014-08-05 20:34:32]

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

[commit-bot: I haz the power, 2014-08-06 00:45:53]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-06 06:08:14]

Change committed as 287698