[Vertical Tabs] Add perf metrics for Tabstrip animations [chromium/src : main]
Tom Lukaszewicz (Gerrit)
Tom Lukaszewicz voted and added 2 comments![]( "Open in Gerrit")
Votes added by Tom Lukaszewicz
| Code-Review | +1 |
2 comments
class AnimationPerfReporter : public ui::CompositorObserver {optional: I know we so this with the classes above but would it work to forward declare this in `private:` and move the full declaration and definition into the .cc file? Current approach is file also
Related details
- Caroline Rising
- Eshwar Stalin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Caroline Rising (Gerrit)
Caroline Rising voted Code-Review+1![]( "Open in Gerrit")
- Eshwar Stalin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eshwar Stalin (Gerrit)
Eshwar Stalin added 1 comment![]( "Open in Gerrit")
class AnimationPerfReporter : public ui::CompositorObserver {+dfr...@chromium.org I wanted to get some quick performance numbers for VTS animation. Per our discussion last week, would like your help to roll these metrics logging directly into the framework. But until that's available wanted to add this directly to the feature.
Related details
- Dana Fried
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Dana Fried added 4 comments![]( "Open in Gerrit")
BrowserAnimationUpdate animation_status_;
const base::TimeDelta total_animation_time_;Please fix this WARNING reported by ClangTidy: check: modernize-use-default-member-init
use default member initializer for 'an...
check: modernize-use-default-member-init
use default member initializer for 'animation_status_' (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-default-member-init.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-use-default-member-init` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
class AnimationPerfReporter : public ui::CompositorObserver {optional: I know we so this with the classes above but would it work to forward declare this in `private:` and move the full declaration and definition into the .cc file? Current approach is file also
This does seem preferable, since so many things include this file.
class AnimationPerfReporter : public ui::CompositorObserver {+dfr...@chromium.org I wanted to get some quick performance numbers for VTS animation. Per our discussion last week, would like your help to roll these metrics logging directly into the framework. But until that's available wanted to add this directly to the feature.
Acknowledged
VerticalTabStripRegionView::AnimationPerfReporter::AnimationPerfReporter(Is it possible to reuse/share what is being used by side panel instead of copy-pasting code, or would that just be more work?
Related details
- Eshwar Stalin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eshwar Stalin (Gerrit)
Eshwar Stalin added 3 comments![]( "Open in Gerrit")
BrowserAnimationUpdate animation_status_;
const base::TimeDelta total_animation_time_;Please fix this WARNING reported by ClangTidy: check: modernize-use-default-member-init
use default member initializer for 'an...
check: modernize-use-default-member-init
use default member initializer for 'animation_status_' (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-default-member-init.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-use-default-member-init` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
Done
class AnimationPerfReporter : public ui::CompositorObserver {Dana Friedoptional: I know we so this with the classes above but would it work to forward declare this in `private:` and move the full declaration and definition into the .cc file? Current approach is file also
This does seem preferable, since so many things include this file.
Done
VerticalTabStripRegionView::AnimationPerfReporter::AnimationPerfReporter(Is it possible to reuse/share what is being used by side panel instead of copy-pasting code, or would that just be more work?
I think we eventually want to move this into the animation controller framework and eliminate this. So that's how we can clean this up. But this is interim until that is available.
Related details
- Dana Fried
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Dana Fried voted Code-Review+1![]( "Open in Gerrit")
- Eshwar Stalin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eshwar Stalin (Gerrit)
Eshwar Stalin voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[Vertical Tabs] Add perf metrics for Tabstrip animations
Adding performance metrics for the different Tabstrip animations
- M chrome/browser/ui/views/frame/vertical_tab_strip_region_view.cc
- M chrome/browser/ui/views/frame/vertical_tab_strip_region_view.h
- M tools/metrics/histograms/metadata/tab/histograms.xml
Code-Review: +1 by Dana Fried, +1 by Thomas Lukaszewicz, +1 by Caroline Rising
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |