Adding tools menu responsiveness metric.

Adding tools menu responsiveness metric.

BUG=723910
Review-Url: https://codereview.chromium.org/2894513002
Cr-Commit-Position: refs/heads/master@{#473314}
Committed: https://chromium.googlesource.com/chromium/src/+/b04bf69ea958add88d418420802b2188cb099217

[PL, 2017-05-18 01:06:48]

peterlaurens@chromium.org changed reviewers:
+ isherman@chromium.org, rohitrao@chromium.org

[PL, 2017-05-18 01:06:48]

Hello,

This is a request for a review of a new metric capturing the amount of wallclock
time it takes to present the tools menu to the user in iOS.

This is part of a larger effort to improve our understanding of the performance
of Chrome for iOS under real use.

@Rohit: Please review or delegate a reviewer for the iOS source files.
@Ilya: Please review or delegate a reviewer for the histograms.xml change,
specifically the naming of the histogram or any other conventions I may have
overlooked.

Your help is much appreciated!

Thanks!

- PL

[rohitrao (ping after 24h), 2017-05-18 01:16:25]

https://codereview.chromium.org/2894513002/diff/1/ios/chrome/browser/ui/brows...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2894513002/diff/1/ios/chrome/browser/ui/brows...
ios/chrome/browser/ui/browser_view_controller.mm:3388: NSTimeInterval startTime
= [NSDate timeIntervalSinceReferenceDate];
Another option would be to add this startTime to ToolsMenuConfiguration and read
it from there when the animation completes.  It's not really menu configuration
data, but it would simplify the logic a bit and avoid adding a new notification.
 It's also a bit safer if we ever fail to run the toolsmenu animation for some
reason, since we wouldn't have a block that could leak.

Thoughts?  Which do you prefer?

[Ilya Sherman, 2017-05-18 03:00:26]

https://codereview.chromium.org/2894513002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2894513002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:76268: +    presentation of the tools
menu, and the actual appearance of the menu on
Which tools menu does this refer to?  Would it make sense to place this
histogram under the "Toolbar" top-level category?  That seems to be where the
code lives.

[PL, 2017-05-18 23:36:34]

Hello,

Thanks Rohit and Ilya for your comments and guidance!

I've updated the CL with your feedback and some discussion.

PTAL!

- Peter

https://codereview.chromium.org/2894513002/diff/1/ios/chrome/browser/ui/brows...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2894513002/diff/1/ios/chrome/browser/ui/brows...
ios/chrome/browser/ui/browser_view_controller.mm:3388: NSTimeInterval startTime
= [NSDate timeIntervalSinceReferenceDate];
On 2017/05/18 01:16:25, rohitrao (ping after 24h) wrote:
> Another option would be to add this startTime to ToolsMenuConfiguration and
read
> it from there when the animation completes.  It's not really menu
configuration
> data, but it would simplify the logic a bit and avoid adding a new
notification.
>  It's also a bit safer if we ever fail to run the toolsmenu animation for some
> reason, since we wouldn't have a block that could leak.
> 
> Thoughts?  Which do you prefer?

Done! This is a smart idea... the configuration isn't stored anywhere at
present, but I can just store the value from the configuration in the view
controller and use it.

I do think this will result in a smaller amount of code... although it does
introduce two instance variables purely for the use of metrics (whereas before I
had none, if you consider the 'didShow' notification could be used for something
non-metric related).

https://codereview.chromium.org/2894513002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2894513002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:76268: +    presentation of the tools
menu, and the actual appearance of the menu on
On 2017/05/18 03:00:25, Ilya Sherman wrote:
> Which tools menu does this refer to?  Would it make sense to place this
> histogram under the "Toolbar" top-level category?  That seems to be where the
> code lives.

Good idea! This is the main menu of controls for Bling, accessed by the three
dot menu in the navigation bar. However our tools menu could exist separately
from a toolbar type object (although the main (only?) way to trigger it is
through a tap on a button in the toolbar).

Looking at the other Toolbar.* items, there's this one:

Toolbar.AppMenuTimeToAction

... which seems to be similar (but not identical) to the type of metric we want
to capture, so maybe it's ok.

Would you then suggest a name like:

"Toolbar.ShowToolsMenuResponsiveness" ?

Thanks!

[rohitrao (ping after 24h), 2017-05-19 00:07:07]

ios/ lgtm, thanks!

[Ilya Sherman, 2017-05-19 00:09:19]

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

https://codereview.chromium.org/2894513002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:75595: +    screen. Includes the
animation duration.
Please document within this <summary> what the "tools menu" is, as it's not
entirely obvious from context.  Other than that, metrics LGTM.  Thanks!

[PL, 2017-05-19 00:33:43]

Thanks!

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

https://codereview.chromium.org/2894513002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:75595: +    screen. Includes the
animation duration.
On 2017/05/19 00:09:19, Ilya Sherman wrote:
> Please document within this <summary> what the "tools menu" is, as it's not
> entirely obvious from context.  Other than that, metrics LGTM.  Thanks!

Done! Thanks!

[PL, 2017-05-19 00:33:50]

The CQ bit was checked by peterlaurens@chromium.org

[PL, 2017-05-19 00:33:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/2894513002/#ps40001
(title: "Tweak to metric description summary")

[commit-bot: I haz the power, 2017-05-19 00:36:16]

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-05-19 02:01:23]

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

[commit-bot: I haz the power, 2017-05-19 02:01:24]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[PL, 2017-05-19 18:10:08]

The CQ bit was checked by peterlaurens@chromium.org

[commit-bot: I haz the power, 2017-05-19 18:11:26]

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-05-19 21:06:06]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495217408130670,
"parent_rev": "89600799e76809b549bb76cb3fe6556537618ef5", "commit_rev":
"b04bf69ea958add88d418420802b2188cb099217"}

[commit-bot: I haz the power, 2017-05-19 21:06:19]

Description was changed from

==========
Adding tools menu responsiveness metric.

BUG=723910
==========

to

==========
Adding tools menu responsiveness metric.

BUG=723910

Review-Url: https://codereview.chromium.org/2894513002
Cr-Commit-Position: refs/heads/master@{#473314}
Committed:
https://chromium.googlesource.com/chromium/src/+/b04bf69ea958add88d418420802b...
==========

[commit-bot: I haz the power, 2017-05-19 21:06:21]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/b04bf69ea958add88d418420802b...