Move CSS animations use counters to UseCounter

Move CSS animations use counters to UseCounter

The use counts for animated CSS properties currently uses a custom
histogram which counts a property once per animation. This patch moves
the use counts to the existing UseCounter framework in order to bring
the counts in line with the CSS property UseCounters. In particular,
this is to ensure that properties are counted once per page instead of
once per animation.

This patch:
- adds a BitVector and corresponding functions to UseCounter
- adds test cases to UseCounterTest, this time with an optional
  legacyHistogram name
- exposes the new 'is counted' UseCounter function to Internals for
  testing
- adds layout tests to verify that the animated CSS property UseCounter
  has been correctly incremented for animations (standard and custom
  CSS properties) and transitions (standard CSS properties only).

One of the layout tests is failing because the standard CSS property
UseCounter currently excludes custom properties. This will be fixed in
a future patch.

BUG=458925
Review-Url: https://codereview.chromium.org/2678143003
Cr-Commit-Position: refs/heads/master@{#455978}
Committed: https://chromium.googlesource.com/chromium/src/+/4ad8268cf8e0bec120786925799ce06591d478b3

[alancutter (OOO until 2018), 2017-02-06 23:54:54]

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

[alancutter (OOO until 2018), 2017-02-06 23:54:54]

lgtm once there are test cases.

[suzyh_UTC10 (ex-contributor), 2017-02-14 06:32:50]

Description was changed from

==========
[WIP] Move CSS animations use counters to UseCounter

The use counts for animated CSS properties currently uses a custom
histogram which counts a property once per animation. This patch moves
the use counts to the existing UseCounter framework in order to bring
the counts in line with the CSS property UseCounters. In particular,
this is to ensure that properties are counted once per page instead of
once per animation.

BUG=458925
==========

to

==========
Move CSS animations use counters to UseCounter

The use counts for animated CSS properties currently uses a custom
histogram which counts a property once per animation. This patch moves
the use counts to the existing UseCounter framework in order to bring
the counts in line with the CSS property UseCounters. In particular,
this is to ensure that properties are counted once per page instead of
once per animation.

This patch:
- adds a BitVector and corresponding functions to UseCounter
- adds test cases to UseCounterTest, this time with an optional
  legacyHistogram name
- exposes the new 'is counted' UseCounter function to Internals for
  testing
- adds layout tests to verify that the animated CSS property UseCounter
  has been correctly incremented for animations (standard and custom
  CSS properties) and transitions (standard CSS properties only).

BUG=458925
==========

[suzyh_UTC10 (ex-contributor), 2017-02-14 06:33:03]

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

[suzyh_UTC10 (ex-contributor), 2017-02-14 06:35:03]

This is now most of the way there, aside from
(a) use counting custom properties in the normal CSS property UseCounter
(deferred to a later patch), and
(b) the BitVector size check question I've brought up with rbyers.

[suzyh_UTC10 (ex-contributor), 2017-02-20 04:11:49]

PTAL

[alancutter (OOO until 2018), 2017-02-20 23:49:39]

https://codereview.chromium.org/2678143003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/animations/animated-css-property-usecounter.html
(right):

https://codereview.chromium.org/2678143003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/animations/animated-css-property-usecounter.html:64:
// FIXME(suzyh): CSS property UseCounter should include custom properties
Tests should be testing what we want to happen and have failing -expected.txt
files for where we still have work to do.

https://codereview.chromium.org/2678143003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/UseCounterTest.cpp (left):

https://codereview.chromium.org/2678143003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/UseCounterTest.cpp:15: // Note that the new
histogram names will change once the semantics stabilize;
I don't know the context around this comment, why does this patch make it go
away?

[suzyh_UTC10 (ex-contributor), 2017-02-20 23:59:57]

On 2017/02/20 at 23:49:39, alancutter wrote:
>
https://codereview.chromium.org/2678143003/diff/120001/third_party/WebKit/Lay...
> File
third_party/WebKit/LayoutTests/animations/animated-css-property-usecounter.html
(right):
> 
>
https://codereview.chromium.org/2678143003/diff/120001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/animations/animated-css-property-usecounter.html:64:
// FIXME(suzyh): CSS property UseCounter should include custom properties
> Tests should be testing what we want to happen and have failing -expected.txt
files for where we still have work to do.

Fair call, I was debating which way to go. I'll change it.


>
https://codereview.chromium.org/2678143003/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/frame/UseCounterTest.cpp (left):
> 
>
https://codereview.chromium.org/2678143003/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/frame/UseCounterTest.cpp:15: // Note that the
new histogram names will change once the semantics stabilize;
> I don't know the context around this comment, why does this patch make it go
away?

It doesn't; it was obsoleted by https://codereview.chromium.org/2614213002 and
I'm just taking the opportunity to remove it.

[suzyh_UTC10 (ex-contributor), 2017-02-21 00:12:32]

Description was changed from

==========
Move CSS animations use counters to UseCounter

The use counts for animated CSS properties currently uses a custom
histogram which counts a property once per animation. This patch moves
the use counts to the existing UseCounter framework in order to bring
the counts in line with the CSS property UseCounters. In particular,
this is to ensure that properties are counted once per page instead of
once per animation.

This patch:
- adds a BitVector and corresponding functions to UseCounter
- adds test cases to UseCounterTest, this time with an optional
  legacyHistogram name
- exposes the new 'is counted' UseCounter function to Internals for
  testing
- adds layout tests to verify that the animated CSS property UseCounter
  has been correctly incremented for animations (standard and custom
  CSS properties) and transitions (standard CSS properties only).

BUG=458925
==========

to

==========
Move CSS animations use counters to UseCounter

The use counts for animated CSS properties currently uses a custom
histogram which counts a property once per animation. This patch moves
the use counts to the existing UseCounter framework in order to bring
the counts in line with the CSS property UseCounters. In particular,
this is to ensure that properties are counted once per page instead of
once per animation.

This patch:
- adds a BitVector and corresponding functions to UseCounter
- adds test cases to UseCounterTest, this time with an optional
  legacyHistogram name
- exposes the new 'is counted' UseCounter function to Internals for
  testing
- adds layout tests to verify that the animated CSS property UseCounter
  has been correctly incremented for animations (standard and custom
  CSS properties) and transitions (standard CSS properties only).

One of the layout tests is failing because the standard CSS property
UseCounter currently excludes custom properties. This will be fixed in
a future patch.

BUG=458925
==========

[suzyh_UTC10 (ex-contributor), 2017-02-21 00:12:56]

On 2017/02/20 at 23:59:57, suzyh wrote:
> On 2017/02/20 at 23:49:39, alancutter wrote:
> >
https://codereview.chromium.org/2678143003/diff/120001/third_party/WebKit/Lay...
> > File
third_party/WebKit/LayoutTests/animations/animated-css-property-usecounter.html
(right):
> > 
> >
https://codereview.chromium.org/2678143003/diff/120001/third_party/WebKit/Lay...
> >
third_party/WebKit/LayoutTests/animations/animated-css-property-usecounter.html:64:
// FIXME(suzyh): CSS property UseCounter should include custom properties
> > Tests should be testing what we want to happen and have failing
-expected.txt files for where we still have work to do.
> 
> Fair call, I was debating which way to go. I'll change it.

Done and updated CL description.

[alancutter (OOO until 2018), 2017-02-21 00:44:08]

lgtm

[Rick Byers, 2017-02-25 15:10:58]

So sorry for the delay!
LGTM

https://codereview.chromium.org/2678143003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/2678143003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/UseCounter.cpp:1307:
"AnimatedCSSFirstUsed", "feature", sampleId);
Note if you'd like to get this data into HTTP Archive data (as the other two
are), you'll probably need to ping pmeenan.  Seems like it would be nice for
consistency.  https://github.com/HTTPArchive/httparchive/issues/59

https://codereview.chromium.org/2678143003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/UseCounterTest.cpp (right):

https://codereview.chromium.org/2678143003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/UseCounterTest.cpp:161:
{kSVGCSSHistogramName, kSVGAnimatedCSSHistogramName}, CSSPropertyOpacity,
worth adding this new histogram to the list of unaffected ones in at least one
other test?  Seems like an unlikely bug, so I'm fine either way - I just like
how you've made it easy to assert nothing unexpected is changing :-)

[suzyh_UTC10 (ex-contributor), 2017-03-06 03:35:15]

+sashab for sanity-check of FrameHost -> Page change after rebase

rbyers: PTAL


On 2017/02/25 at 15:10:58, rbyers wrote:
> So sorry for the delay!
> LGTM
> 
>
https://codereview.chromium.org/2678143003/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/frame/UseCounter.cpp (right):
> 
>
https://codereview.chromium.org/2678143003/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/frame/UseCounter.cpp:1307:
"AnimatedCSSFirstUsed", "feature", sampleId);
> Note if you'd like to get this data into HTTP Archive data (as the other two
are), you'll probably need to ping pmeenan.  Seems like it would be nice for
consistency.  https://github.com/HTTPArchive/httparchive/issues/59

Good idea, pinged.


>
https://codereview.chromium.org/2678143003/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/frame/UseCounterTest.cpp (right):
> 
>
https://codereview.chromium.org/2678143003/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/frame/UseCounterTest.cpp:161:
{kSVGCSSHistogramName, kSVGAnimatedCSSHistogramName}, CSSPropertyOpacity,
> worth adding this new histogram to the list of unaffected ones in at least one
other test?  Seems like an unlikely bug, so I'm fine either way - I just like
how you've made it easy to assert nothing unexpected is changing :-)

Good call: I discovered something. didCommitLoad increments the 'page visit'
bucket count for kAnimatedCSSHistogramName when recording features and CSS
properties. Conversely, it increments kSVGAnimatedCSSHistogramName when
recording features and CSS properties in an SVG context. Having thought about
this some more, it makes sense, so I don't think this is a bug, but it suggests
the tests need tweaking. I've added the animated CSS histograms to the unchanged
histograms lists, keeping default with default and svg with svg as done so far,
but what would you suggest we add to the test to capture this detail? Should the
'unaffected' histograms exclude page visit count? Should there be an extra arg
with a list of 'page counted but otherwise empty' histograms? Are you fine for
me to do that in a separate patch, landing this one as-is?

[suzyh_UTC10 (ex-contributor), 2017-03-06 03:36:42]

suzyh@chromium.org changed reviewers:
+ sashab@chromium.org

[suzyh_UTC10 (ex-contributor), 2017-03-06 03:37:32]

Actually +sashab this time for sanity-check of FrameHost -> Page change after
rebase

[sashab, 2017-03-06 05:24:13]

Ahh thanks for looping me in suzy! Looks great. LGTM

[suzyh_UTC10 (ex-contributor), 2017-03-07 07:23:37]

Ping rbyers PTAL (I suspect the previous message got buried)

[suzyh_UTC10 (ex-contributor), 2017-03-09 23:15:32]

On 2017/03/07 at 07:23:37, suzyh wrote:
> Ping rbyers PTAL (I suspect the previous message got buried)

I've decided to go ahead and land this patch and just note on the bug that
there's this open question about the page visit bucket.

[suzyh_UTC10 (ex-contributor), 2017-03-09 23:15:49]

The CQ bit was checked by suzyh@chromium.org

[suzyh_UTC10 (ex-contributor), 2017-03-09 23:15:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
alancutter@chromium.org, rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/2678143003/#ps160001
(title: "Rebase incl FrameHost->Page change, tweak test")

[commit-bot: I haz the power, 2017-03-09 23:16:42]

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

[commit-bot: I haz the power, 2017-03-10 00:07:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 00:07:31]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[suzyh_UTC10 (ex-contributor), 2017-03-10 02:57:55]

The CQ bit was checked by suzyh@chromium.org

[commit-bot: I haz the power, 2017-03-10 02:58:40]

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

[commit-bot: I haz the power, 2017-03-10 03:13:50]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1489114675838980,
"parent_rev": "ae4a9e6d5186753f2563af342463c9d1fccf91f6", "commit_rev":
"4ad8268cf8e0bec120786925799ce06591d478b3"}

[commit-bot: I haz the power, 2017-03-10 03:14:57]

Description was changed from

==========
Move CSS animations use counters to UseCounter

The use counts for animated CSS properties currently uses a custom
histogram which counts a property once per animation. This patch moves
the use counts to the existing UseCounter framework in order to bring
the counts in line with the CSS property UseCounters. In particular,
this is to ensure that properties are counted once per page instead of
once per animation.

This patch:
- adds a BitVector and corresponding functions to UseCounter
- adds test cases to UseCounterTest, this time with an optional
  legacyHistogram name
- exposes the new 'is counted' UseCounter function to Internals for
  testing
- adds layout tests to verify that the animated CSS property UseCounter
  has been correctly incremented for animations (standard and custom
  CSS properties) and transitions (standard CSS properties only).

One of the layout tests is failing because the standard CSS property
UseCounter currently excludes custom properties. This will be fixed in
a future patch.

BUG=458925
==========

to

==========
Move CSS animations use counters to UseCounter

The use counts for animated CSS properties currently uses a custom
histogram which counts a property once per animation. This patch moves
the use counts to the existing UseCounter framework in order to bring
the counts in line with the CSS property UseCounters. In particular,
this is to ensure that properties are counted once per page instead of
once per animation.

This patch:
- adds a BitVector and corresponding functions to UseCounter
- adds test cases to UseCounterTest, this time with an optional
  legacyHistogram name
- exposes the new 'is counted' UseCounter function to Internals for
  testing
- adds layout tests to verify that the animated CSS property UseCounter
  has been correctly incremented for animations (standard and custom
  CSS properties) and transitions (standard CSS properties only).

One of the layout tests is failing because the standard CSS property
UseCounter currently excludes custom properties. This will be fixed in
a future patch.

BUG=458925

Review-Url: https://codereview.chromium.org/2678143003
Cr-Commit-Position: refs/heads/master@{#455978}
Committed:
https://chromium.googlesource.com/chromium/src/+/4ad8268cf8e0bec120786925799c...
==========

[commit-bot: I haz the power, 2017-03-10 03:15:01]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/4ad8268cf8e0bec120786925799c...