Add new UseCounter metric

Add new UseCounter metric

Adds a new UMA histogram "WebCore.UseCounter_TEST" which fixes the issue with
data loss on fast shutdown while temporarily preseving the original
semantics in the legacy "WebCore.FeatureObserver" histogram.

Also overhauls the tests to look directly at the histograms.

Note that the semantics of the new UseCounter are still under flux and
being validated, hence the '_TEST' suffix in the histogram name.  Once
validated it'll be renamed to simply WebCore.UseCounter and we will expect
to never change the semantics of the histogram again (without rename).

BUG=597963
Committed: https://crrev.com/631a26b68f615f8282efba7ded2a6f4fac236482
Cr-Commit-Position: refs/heads/master@{#417957}

[Rick Byers, 2016-09-09 17:55:14]

Description was changed from

==========
[WIP] Add new UseCounter metric

Adds a new UMA histogram "WebCore.UseCounter" which fixes the issue with
data loss on fast shutdown while temporarily preseving the original
semantics in the legacy "WebCore.FeatureObserver" histogram.

BUG=597963
==========

to

==========
Add new UseCounter metric

Adds a new UMA histogram "WebCore.UseCounter" which fixes the issue with
data loss on fast shutdown while temporarily preseving the original
semantics in the legacy "WebCore.FeatureObserver" histogram.

Also overhauls the tests to look directly at the histograms.

BUG=597963
==========

[Rick Byers, 2016-09-09 17:55:14]

rbyers@chromium.org changed reviewers:
+ dtapuska@chromium.org

[Rick Byers, 2016-09-09 17:58:05]

Hey Dave,
This is the refactoring you initially looked at as part of
https://codereview.chromium.org/2023903003/.  PTAL.

I still need to address the SVG problem (crbug.com/236262) but that's going to
be ugly (due to the need to temporarily preserve the semantics of the original
histogram).  So I'd like to land this first.  I intent to land both CLs in M55
so ideally the semantics of the new histogram isn't changing across releases
(having a change in dev channel should be fine - will be interesting to see how
much of the difference is accounted for by each issue).

[Rick Byers, 2016-09-09 17:58:10]

The CQ bit was checked by rbyers@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 17:58:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dtapuska, 2016-09-09 18:30:19]

https://codereview.chromium.org/2290733002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounter.cpp (left):

https://codereview.chromium.org/2290733002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.cpp:588: return histogram;
Perhaps we should use temporary names because we haven't solved the SVG Image
problem yet. And we want to verify that this data is going to be more accurate
fist. Then solve the SVG Image problem. And then flip the name to the sensible
name.

https://codereview.chromium.org/2290733002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/2290733002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.cpp:585: static
EnumerationHistogram& useCounterHistogram()
shouldn't these statics really be in an anonymous namespace?

https://codereview.chromium.org/2290733002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.cpp:628:
m_legacyCounter.countFeature(feature);
Can this not go up into the conditional?

https://codereview.chromium.org/2290733002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.cpp:757:
m_legacyCounter.countCSS(property);
likewise why isn't this inside the else?

https://codereview.chromium.org/2290733002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounterTest.cpp (right):

https://codereview.chromium.org/2290733002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounterTest.cpp:141:
histogramTester.expectTotalCount("WebCore.UseCounter.Features", 0);
Can we not repeat the names all over this file? Can we add a constant to the top
for them?

https://codereview.chromium.org/2290733002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2290733002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:65825: +    NOTE: This is in the process
of being superceded by
s/superceded/superseded ?

[Rick Byers, 2016-09-09 19:31:37]

https://codereview.chromium.org/2290733002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounter.cpp (left):

https://codereview.chromium.org/2290733002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.cpp:588: return histogram;
On 2016/09/09 18:30:19, dtapuska wrote:
> Perhaps we should use temporary names because we haven't solved the SVG Image
> problem yet. And we want to verify that this data is going to be more accurate
> fist. Then solve the SVG Image problem. And then flip the name to the sensible
> name.

Done.

https://codereview.chromium.org/2290733002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/2290733002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.cpp:585: static
EnumerationHistogram& useCounterHistogram()
On 2016/09/09 18:30:19, dtapuska wrote:
> shouldn't these statics really be in an anonymous namespace?

I don't think our coding style says to prefer one over the other (it's
equivalent in these cases AFAIK), but it's certainly more modern.  Done.  This
required some shuffling though.

https://codereview.chromium.org/2290733002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.cpp:628:
m_legacyCounter.countFeature(feature);
On 2016/09/09 18:30:19, dtapuska wrote:
> Can this not go up into the conditional?

No, the legacy counter has it's own set of recorded bits.  I think it's
equivalent today but wont be when I fix the SVG issue.

https://codereview.chromium.org/2290733002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.cpp:757:
m_legacyCounter.countCSS(property);
On 2016/09/09 18:30:19, dtapuska wrote:
> likewise why isn't this inside the else?

I added some comments to the declarations to make it clear that the semantics of
the legacy and non-legacy bits are independent.   Better?

https://codereview.chromium.org/2290733002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounterTest.cpp (right):

https://codereview.chromium.org/2290733002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounterTest.cpp:141:
histogramTester.expectTotalCount("WebCore.UseCounter.Features", 0);
On 2016/09/09 18:30:19, dtapuska wrote:
> Can we not repeat the names all over this file? Can we add a constant to the
top
> for them?

Done.

https://codereview.chromium.org/2290733002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2290733002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:65825: +    NOTE: This is in the process
of being superceded by
On 2016/09/09 18:30:19, dtapuska wrote:
> s/superceded/superseded ?

Done.

[dtapuska, 2016-09-09 19:39:24]

lgtm

[Rick Byers, 2016-09-09 20:19:01]

Description was changed from

==========
Add new UseCounter metric

Adds a new UMA histogram "WebCore.UseCounter" which fixes the issue with
data loss on fast shutdown while temporarily preseving the original
semantics in the legacy "WebCore.FeatureObserver" histogram.

Also overhauls the tests to look directly at the histograms.

BUG=597963
==========

to

==========
Add new UseCounter metric

Adds a new UMA histogram "WebCore.UseCounter_TEST" which fixes the issue with
data loss on fast shutdown while temporarily preseving the original
semantics in the legacy "WebCore.FeatureObserver" histogram.

Also overhauls the tests to look directly at the histograms.

Note that the semantics of the new UseCounter are still under flux and being
validated.  Once validated it'll be renamed to simply WebCore.UseCounter.

BUG=597963
==========

[Rick Byers, 2016-09-09 20:22:26]

rbyers@chromium.org changed reviewers:
+ rkaplow@chromium.org

[Rick Byers, 2016-09-09 20:22:27]

+rkaplow for metrics OWNERS.  In particular I want to make sure you're OK with
using a temporary histogram name as we transition from the broken semantics to
the new semantics.

[rkaplow, 2016-09-12 14:57:33]

lgtm

Hey, technically looks fine, although I'm still unclear why you need to set
these up as test histograms. Are you expecting them to likely be not quite right
and will want to collect some data before finalizing the setup?

If that's the case SG, otherwise would be good to clarify.

[Rick Byers, 2016-09-12 15:13:06]

Description was changed from

==========
Add new UseCounter metric

Adds a new UMA histogram "WebCore.UseCounter_TEST" which fixes the issue with
data loss on fast shutdown while temporarily preseving the original
semantics in the legacy "WebCore.FeatureObserver" histogram.

Also overhauls the tests to look directly at the histograms.

Note that the semantics of the new UseCounter are still under flux and being
validated.  Once validated it'll be renamed to simply WebCore.UseCounter.

BUG=597963
==========

to

==========
Add new UseCounter metric

Adds a new UMA histogram "WebCore.UseCounter_TEST" which fixes the issue with
data loss on fast shutdown while temporarily preseving the original
semantics in the legacy "WebCore.FeatureObserver" histogram.

Also overhauls the tests to look directly at the histograms.

Note that the semantics of the new UseCounter are still under flux and being
validated, hence the '_TEST' suffix in the histogram name.  Once validated it'll
be renamed to simply WebCore.UseCounter and we will expect to never change the
semantics of the histogram again without rename (to permit long-term data
analysis).

BUG=597963
==========

[Rick Byers, 2016-09-12 15:15:06]

On 2016/09/12 14:57:33, rkaplow wrote:
> lgtm
> 
> Hey, technically looks fine, although I'm still unclear why you need to set
> these up as test histograms. Are you expecting them to likely be not quite
right
> and will want to collect some data before finalizing the setup?
> 
> If that's the case SG, otherwise would be good to clarify.

Yes, exactly.  Once we commit to the "nice" histogram name, I don't want the
semantics to change.  I want to collect some more data to convince ourselves
there aren't additional outstanding bugs in the semantics.  I expanded on this
in the CL description.

[Rick Byers, 2016-09-12 15:15:43]

Description was changed from

==========
Add new UseCounter metric

Adds a new UMA histogram "WebCore.UseCounter_TEST" which fixes the issue with
data loss on fast shutdown while temporarily preseving the original
semantics in the legacy "WebCore.FeatureObserver" histogram.

Also overhauls the tests to look directly at the histograms.

Note that the semantics of the new UseCounter are still under flux and being
validated, hence the '_TEST' suffix in the histogram name.  Once validated it'll
be renamed to simply WebCore.UseCounter and we will expect to never change the
semantics of the histogram again without rename (to permit long-term data
analysis).

BUG=597963
==========

to

==========
Add new UseCounter metric

Adds a new UMA histogram "WebCore.UseCounter_TEST" which fixes the issue with
data loss on fast shutdown while temporarily preseving the original
semantics in the legacy "WebCore.FeatureObserver" histogram.

Also overhauls the tests to look directly at the histograms.

Note that the semantics of the new UseCounter are still under flux and
being validated, hence the '_TEST' suffix in the histogram name.  Once
validated it'll be renamed to simply WebCore.UseCounter and we will expect
to never change the semantics of the histogram again (without rename).

BUG=597963
==========

[Rick Byers, 2016-09-12 15:15:47]

The CQ bit was checked by rbyers@chromium.org

[commit-bot: I haz the power, 2016-09-12 15:16:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rkaplow, 2016-09-12 15:17:36]

lgtm

sg, thanks for clarifying

[commit-bot: I haz the power, 2016-09-12 16:57:23]

Description was changed from

==========
Add new UseCounter metric

Adds a new UMA histogram "WebCore.UseCounter_TEST" which fixes the issue with
data loss on fast shutdown while temporarily preseving the original
semantics in the legacy "WebCore.FeatureObserver" histogram.

Also overhauls the tests to look directly at the histograms.

Note that the semantics of the new UseCounter are still under flux and
being validated, hence the '_TEST' suffix in the histogram name.  Once
validated it'll be renamed to simply WebCore.UseCounter and we will expect
to never change the semantics of the histogram again (without rename).

BUG=597963
==========

to

==========
Add new UseCounter metric

Adds a new UMA histogram "WebCore.UseCounter_TEST" which fixes the issue with
data loss on fast shutdown while temporarily preseving the original
semantics in the legacy "WebCore.FeatureObserver" histogram.

Also overhauls the tests to look directly at the histograms.

Note that the semantics of the new UseCounter are still under flux and
being validated, hence the '_TEST' suffix in the histogram name.  Once
validated it'll be renamed to simply WebCore.UseCounter and we will expect
to never change the semantics of the histogram again (without rename).

BUG=597963
==========

[commit-bot: I haz the power, 2016-09-12 16:57:24]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-12 17:00:48]

Description was changed from

==========
Add new UseCounter metric

Adds a new UMA histogram "WebCore.UseCounter_TEST" which fixes the issue with
data loss on fast shutdown while temporarily preseving the original
semantics in the legacy "WebCore.FeatureObserver" histogram.

Also overhauls the tests to look directly at the histograms.

Note that the semantics of the new UseCounter are still under flux and
being validated, hence the '_TEST' suffix in the histogram name.  Once
validated it'll be renamed to simply WebCore.UseCounter and we will expect
to never change the semantics of the histogram again (without rename).

BUG=597963
==========

to

==========
Add new UseCounter metric

Adds a new UMA histogram "WebCore.UseCounter_TEST" which fixes the issue with
data loss on fast shutdown while temporarily preseving the original
semantics in the legacy "WebCore.FeatureObserver" histogram.

Also overhauls the tests to look directly at the histograms.

Note that the semantics of the new UseCounter are still under flux and
being validated, hence the '_TEST' suffix in the histogram name.  Once
validated it'll be renamed to simply WebCore.UseCounter and we will expect
to never change the semantics of the histogram again (without rename).

BUG=597963

Committed: https://crrev.com/631a26b68f615f8282efba7ded2a6f4fac236482
Cr-Commit-Position: refs/heads/master@{#417957}
==========

[commit-bot: I haz the power, 2016-09-12 17:00:49]

Patchset 4 (id:??) landed as
https://crrev.com/631a26b68f615f8282efba7ded2a6f4fac236482
Cr-Commit-Position: refs/heads/master@{#417957}

[Nico, 2016-09-13 14:16:46]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2016-09-13 14:16:47]

https://codereview.chromium.org/2290733002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/2290733002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.h:1342: class LegacyCounter {
this needs a CORE_EXPORT, else release component builds won't link (at least
with clang-cl):

FAILED: webkit_unit_tests.exe webkit_unit_tests.exe.pdb 
E:/b/depot_tools/python276_bin/python.exe
../../build/toolchain/win/tool_wrapper.py link-wrapper environment.x64 False
link.exe /nologo /OUT:./webkit_unit_tests.exe /PDB:./webkit_unit_tests.exe.pdb
@./webkit_unit_tests.exe.rsp
UseCounterTest.obj : error LNK2019: unresolved external symbol "public: __cdecl
blink::UseCounter::LegacyCounter::~LegacyCounter(void)"
(??1LegacyCounter@UseCounter@blink@@QEAA@XZ) referenced in function "private:
virtual void __cdecl
blink::UseCounterTest_RecordingFeatures_Test::TestBody(void)"
(?TestBody@UseCounterTest_RecordingFeatures_Test@blink@@EEAAXXZ)
./webkit_unit_tests.exe : fatal error LNK1120: 1 unresolved externals

https://build.chromium.org/p/chromium.fyi/builders/CrWinClang%28shared%29/bui...

[dtapuska, 2016-09-13 14:30:07]

On 2016/09/13 14:16:47, Nico wrote:
>
https://codereview.chromium.org/2290733002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/UseCounter.h (right):
> 
>
https://codereview.chromium.org/2290733002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/UseCounter.h:1342: class LegacyCounter {
> this needs a CORE_EXPORT, else release component builds won't link (at least
> with clang-cl):
> 
> FAILED: webkit_unit_tests.exe webkit_unit_tests.exe.pdb 
> E:/b/depot_tools/python276_bin/python.exe
> ../../build/toolchain/win/tool_wrapper.py link-wrapper environment.x64 False
> link.exe /nologo /OUT:./webkit_unit_tests.exe /PDB:./webkit_unit_tests.exe.pdb
> @./webkit_unit_tests.exe.rsp
> UseCounterTest.obj : error LNK2019: unresolved external symbol "public:
__cdecl
> blink::UseCounter::LegacyCounter::~LegacyCounter(void)"
> (??1LegacyCounter@UseCounter@blink@@QEAA@XZ) referenced in function "private:
> virtual void __cdecl
> blink::UseCounterTest_RecordingFeatures_Test::TestBody(void)"
> (?TestBody@UseCounterTest_RecordingFeatures_Test@blink@@EEAAXXZ)
> ./webkit_unit_tests.exe : fatal error LNK1120: 1 unresolved externals
> 
>
https://build.chromium.org/p/chromium.fyi/builders/CrWinClang%28shared%29/bui...

Posted a patch here: https://codereview.chromium.org/2336253002/ Rick doesn't
seem to be at his desk now.