Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Description • 10 years ago
str

STR:
1. Apply the patch.

If it doesn't apply cleanly then make sure that only the following tests are the only ones available in browser.ini:
[browser_telemetry_button_paintflashing.js]
[browser_telemetry_button_responsive.js]

2. ./mach mochitest-devtools browser/devtools/shared/test/

When the second test starts the browser crashes.

	Nathan Froyd [:froydnj] Assignee
	Comment 1 • 10 years ago

Clearing flag histograms didn't work, because flag histograms aren't
"initialized" to all-zeroes, their zero-bucket contains a 1 upon
initialization.  We need to reproduce this for ::Clear().

	Nathan Froyd [:froydnj] Assignee
	Comment 2 • 10 years ago

Actually, since I showed Mike that his patches still had some test failures, what do folks think the correct behavior of flag histograms should be when clear() is invoked:

1) Revert to false, which is what attachment 8521498 [details] [diff] [review] does;
2) Remain as-is, which I think is what Mike expects, assuming I understand his patches correctly.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 3 • 10 years ago

(In reply to Nathan Froyd (:froydnj) from comment #2)
> Actually, since I showed Mike that his patches still had some test failures,
> what do folks think the correct behavior of flag histograms should be when
> clear() is invoked:
> 
> 1) Revert to false, which is what attachment 8521498 [details] [diff] [review]
> [review] does;
> 2) Remain as-is, which I think is what Mike expects, assuming I understand
> his patches correctly.

I expect you to revert to false.

The patch doesn't quite fix it... after calling clear() any new pings are ignored.

I have attached an xpcshell test that highlights the problem (with your patch applied):

./mach test toolkit/components/telemetry/tests/unit/test_TelemetryFlagClear.js

output:
 0:00.21 PROCESS_OUTPUT: Thread-1 (pid:86265) "Original value:"
 0:00.21 TEST_STATUS: Thread-1 run_test PASS [run_test : 12] "[1,0,0]" == "[1,0,0]"
 0:00.21 PROCESS_OUTPUT: Thread-1 (pid:86265) "Sending ping"
 0:00.21 PROCESS_OUTPUT: Thread-1 (pid:86265) "New value:"
 0:00.21 TEST_STATUS: Thread-1 run_test PASS [run_test : 19] "[0,1,0]" == "[0,1,0]"
 0:00.21 PROCESS_OUTPUT: Thread-1 (pid:86265) "Clearing histogram"
 0:00.21 PROCESS_OUTPUT: Thread-1 (pid:86265) "After clearing:"
 0:00.21 TEST_STATUS: Thread-1 run_test PASS [run_test : 25] "[1,0,0]" == "[1,0,0]"
 0:00.21 PROCESS_OUTPUT: Thread-1 (pid:86265) "After sending ping:"
 0:00.21 TEST_STATUS: Thread-1 run_test FAIL [run_test : 30] "[1,0,0]" == "[0,1,0]"

	Georg Fritzsche [:gfritzsche]
	Comment 5 • 10 years ago

Comment on attachment 8521498 [details] [diff] [review]
correctly re-initialize flag histograms after clearing them

Review of attachment 8521498 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/chromium/src/base/histogram.cc
@@ +152,5 @@
>    SampleSet ss;
>    ss.Resize(*this);
>    sample_ = ss;
> +
> +  if (histogram_type() == FLAG_HISTOGRAM) {

Can't we just make Histogram::Clear() virtual and override from FlagHistogram?

@@ +154,5 @@
>    sample_ = ss;
> +
> +  if (histogram_type() == FLAG_HISTOGRAM) {
> +    size_t zero_index = BucketIndex(0);
> +    static_cast<FlagHistogram*>(this)->LinearHistogram::Accumulate(0, 1, zero_index);

Also need to reset FlagHistogram::mSwitched.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 7 • 10 years ago

I didn't realize that we can now use equal etc. in xpcshell tests... updated.

	Nathan Froyd [:froydnj] Assignee
	Comment 8 • 10 years ago

Indeed, that would be better.  Thanks for the catch on |mSwitched|; that is
probably what was causing the test failures.

	Nathan Froyd [:froydnj] Assignee
	Comment 9 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0c591e12fea
https://hg.mozilla.org/integration/mozilla-inbound/rev/44d6420a3cc7

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/e0c591e12fea
https://hg.mozilla.org/mozilla-central/rev/44d6420a3cc7