Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2801003003:
Adding histograms for animated CSS properties in histograms.xml (Closed)
DescriptionAdding histograms for animated CSS properties in histograms.xml
Adding Blink.UseCounter.AnimatedCSSProperties and
Blink.UseCounter.SCGImage.AnimatedCSSProperties in histograms.xml in order to
view the result on UMA dashboard.
BUG=458925
Review-Url: https://codereview.chromium.org/2801003003
Cr-Commit-Position: refs/heads/master@{#464104}
Committed: https://chromium.googlesource.com/chromium/src/+/b8b42e14556c2557f386c0861cad1ff6380f1404
Patch Set 1 #
Total comments: 2
Patch Set 2 : Codereview: nit -- modified histogram details as suggested #Patch Set 3 : Codereview: nit -- update description for AnimatedCSSProperties #Messages
Total messages: 15 (7 generated)
lunalu@chromium.org changed reviewers: + rbyers@chromium.org
Hi Rick, PTAL
Description was changed from ========== Adding histograms for animated CSS properties in histograms.xml Adding Blink.UseCounter.AnimatedCSSProperties and Blink.UseCounter.SCGImage.AnimatedCSSProperties in histograms.xml in order to view the result on UMA dashboard. BUG=458925 ========== to ========== Adding histograms for animated CSS properties in histograms.xml Adding Blink.UseCounter.AnimatedCSSProperties and Blink.UseCounter.SCGImage.AnimatedCSSProperties in histograms.xml in order to view the result on UMA dashboard. BUG=458925 ==========
rbyers@chromium.org changed reviewers: + asvitkine@chromium.org
LGTM with nit +asvitkine for OWNERS https://codereview.chromium.org/2801003003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2801003003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:5227: + usage of animated CSS properties only. nit: clarify wording a bit, maybe "This histogram counts CSS properties only when they are animated by a CSS Animation or the Web Animations API.". /cc suzyh: Is that characterization correct? From the code it's obvious to me that the usecounter is used for CSS @keyframe animations, but does Web Animations use the same code path? If not then presumably we should fix it to also call into this useCounter (though that can happen in a follow-up CL - no need to block this one since I'm pretty sure including WebAnimations is the behavior we want from the UseCounter). https://codereview.chromium.org/2801003003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:5280: + Like Blink.UseCounter.CSSProperties but specifically for the case of CSS s/CSSProperties/AnimatedCSSProperties/
lgtm In the future, please combine the histograms.xml change with the C++ change to add the metrics into the same CL.
> https://codereview.chromium.org/2801003003/diff/1/tools/metrics/histograms/hi... > tools/metrics/histograms/histograms.xml:5227: + usage of animated CSS properties only. > nit: clarify wording a bit, maybe "This histogram counts CSS properties only when they are animated by a CSS Animation or the Web Animations API.". > > /cc suzyh: Is that characterization correct? From the code it's obvious to me that the usecounter is used for CSS @keyframe animations, but does Web Animations use the same code path? If not then presumably we should fix it to also call into this useCounter (though that can happen in a follow-up CL - no need to block this one since I'm pretty sure including WebAnimations is the behavior we want from the UseCounter). No, the code path that's calling the UseCounter count method is only triggered by CSS animations. Filed crbug.com/709876 to add counts from the Web Animations API. If I understand your comment correctly, you want them to be counted in the same histogram?
On 2017/04/10 06:19:52, suzyh_UTC10 wrote: > > > https://codereview.chromium.org/2801003003/diff/1/tools/metrics/histograms/hi... > > tools/metrics/histograms/histograms.xml:5227: + usage of animated CSS > properties only. > > nit: clarify wording a bit, maybe "This histogram counts CSS properties only > when they are animated by a CSS Animation or the Web Animations API.". > > > > /cc suzyh: Is that characterization correct? From the code it's obvious to me > that the usecounter is used for CSS @keyframe animations, but does Web > Animations use the same code path? If not then presumably we should fix it to > also call into this useCounter (though that can happen in a follow-up CL - no > need to block this one since I'm pretty sure including WebAnimations is the > behavior we want from the UseCounter). > > No, the code path that's calling the UseCounter count method is only triggered > by CSS animations. Filed crbug.com/709876 to add counts from the Web Animations > API. If I understand your comment correctly, you want them to be counted in the > same histogram? Thanks Suzy. I think it's ultimately up to you (and maybe flackr@ on threaded-rendering) who are going to use this metric. But I assumed it was primarily useful as a "here are how often different properties are animated and so should consider having animation fast-paths" thing. Anyway Luna, perhaps for now just say "This histogram counts CSS properties only when they are animated by a CSS Animation" and we can discuss whether/how to change this in crbug.com/709876.
The CQ bit was checked by lunalu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2801003003/#ps40001 (title: "Codereview: nit -- update description for AnimatedCSSProperties")
Thanks Rick and Suzy. I updated the description and am going to submit the change now.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492019820308330,
"parent_rev": "93cb7df1f935461d1b3c5544f0c7efe38097d8d7", "commit_rev":
"b8b42e14556c2557f386c0861cad1ff6380f1404"}
Message was sent while issue was closed.
Description was changed from ========== Adding histograms for animated CSS properties in histograms.xml Adding Blink.UseCounter.AnimatedCSSProperties and Blink.UseCounter.SCGImage.AnimatedCSSProperties in histograms.xml in order to view the result on UMA dashboard. BUG=458925 ========== to ========== Adding histograms for animated CSS properties in histograms.xml Adding Blink.UseCounter.AnimatedCSSProperties and Blink.UseCounter.SCGImage.AnimatedCSSProperties in histograms.xml in order to view the result on UMA dashboard. BUG=458925 Review-Url: https://codereview.chromium.org/2801003003 Cr-Commit-Position: refs/heads/master@{#464104} Committed: https://chromium.googlesource.com/chromium/src/+/b8b42e14556c2557f386c0861cad... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/b8b42e14556c2557f386c0861cad... |
