Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2895013002:
Perf metric tracking the wallclock time it takes to show a new tab page. (Closed)
|
Created:
3 years, 7 months ago by PL Modified:
3 years, 6 months ago CC:
chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, asvitkine+watch_chromium.org, marq+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPerf metric tracking the wallclock time it takes to show a new tab page.
BUG=724682
Review-Url: https://codereview.chromium.org/2895013002
Cr-Commit-Position: refs/heads/master@{#478086}
Committed: https://chromium.googlesource.com/chromium/src/+/90ac0d3da58ffe49a74ddf0bca158b4961cf1931
Patch Set 1 #
Total comments: 6
Patch Set 2 : Add new metric for tracking new tabs from tab switcher button, and review feedback" #
Total comments: 4
Patch Set 3 : Review nits #
Messages
Total messages: 20 (9 generated)
peterlaurens@chromium.org changed reviewers: + isherman@chromium.org, rohitrao@chromium.org
Hello! This is another metric to help us understand the performance of our software in the wild. This metric tracks the wallclock time from the initial request for a new tab page to the final presentation of it. @Rohit: Your help reviewing the ios/ implementation would be most appreciated! @Ilya: Your help reviewing the histogram naming would be most appreciated! Many thanks to you both! - Peter
I'll take a look at this on Monday morning. Please ping me if you don't have a response by the time you get in.
On 2017/05/19 23:18:43, rohitrao (ping after 24h) wrote: > I'll take a look at this on Monday morning. Please ping me if you don't have a > response by the time you get in. Absolutely! Thanks Rohit!
https://codereview.chromium.org/2895013002/diff/1/ios/chrome/browser/ui/brows... File ios/chrome/browser/ui/browser_view_controller.mm (right): https://codereview.chromium.org/2895013002/diff/1/ios/chrome/browser/ui/brows... ios/chrome/browser/ui/browser_view_controller.mm:1211: __weak __typeof(self) weakSelf = self; Could we capture the value of isOffTheRecord instead of capturing self? https://codereview.chromium.org/2895013002/diff/1/ios/chrome/browser/ui/brows... ios/chrome/browser/ui/browser_view_controller.mm:1614: if (!inBackground) { What happens when we open a new tab in the background? Is it possible for tabWasAddedCompletionBlock to be non-nil when a tab is opened in the background? Currently we only run the block for new foreground tabs.
Metrics LG % a comment: https://codereview.chromium.org/2895013002/diff/1/ios/chrome/browser/ui/brows... File ios/chrome/browser/ui/browser_view_controller.mm (right): https://codereview.chromium.org/2895013002/diff/1/ios/chrome/browser/ui/brows... ios/chrome/browser/ui/browser_view_controller.mm:1216: : "Toolbar.Menu.NewTabPresentationDuration", Names passed to UMA_HISTOGRAM macros must be runtime constants. Please either move the if-stmt out of the macro, or use the non-macro version, base::UmaHistogramTimes().
Hi Rohit, Ilya, Thanks for your guidance on this. I realized this morning that there was another significant way to create new tabs that was not covered here. As well as the menu item "New Tab", there's also the plus button [ + ] in the tab switcher. Unfortunately these two bits of UI use different paths to create and present the new tab. I'd like to cover both separately and so have created another couple of histogram items for this purpose. I think this needs another look from both of you... thank you for your help! - PL https://codereview.chromium.org/2895013002/diff/1/ios/chrome/browser/ui/brows... File ios/chrome/browser/ui/browser_view_controller.mm (right): https://codereview.chromium.org/2895013002/diff/1/ios/chrome/browser/ui/brows... ios/chrome/browser/ui/browser_view_controller.mm:1211: __weak __typeof(self) weakSelf = self; On 2017/05/22 12:55:05, rohitrao (ping after 24h) wrote: > Could we capture the value of isOffTheRecord instead of capturing self? Absolutely! Done! https://codereview.chromium.org/2895013002/diff/1/ios/chrome/browser/ui/brows... ios/chrome/browser/ui/browser_view_controller.mm:1216: : "Toolbar.Menu.NewTabPresentationDuration", On 2017/05/22 20:45:45, Ilya Sherman wrote: > Names passed to UMA_HISTOGRAM macros must be runtime constants. Please either > move the if-stmt out of the macro, or use the non-macro version, > base::UmaHistogramTimes(). Ah thanks! I've broken these out into two separate calls now. https://codereview.chromium.org/2895013002/diff/1/ios/chrome/browser/ui/brows... ios/chrome/browser/ui/browser_view_controller.mm:1614: if (!inBackground) { On 2017/05/22 12:55:05, rohitrao (ping after 24h) wrote: > What happens when we open a new tab in the background? Is it possible for > tabWasAddedCompletionBlock to be non-nil when a tab is opened in the background? > Currently we only run the block for new foreground tabs. This is a good question. The answer is (I believe): nothing. The newTab: method appears to only be able to create foreground tabs. So we shouldn't be in a position where we set this block and then somehow get on screen without running it. Also, if that did happen somehow, the block would just be overwritten by the next call to newTab without it being run in any erroneous state. I do think you're right picking up on this issue though, it could be clearer when this block will run, so I've renamed it, and just to be super safe, it will now get reset whether the presented tab is foreground or background. Let me know what you think!
Thanks! Metrics LGTM % comments. https://codereview.chromium.org/2895013002/diff/20001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/browser_view_controller.mm (right): https://codereview.chromium.org/2895013002/diff/20001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/browser_view_controller.mm:1214: double duration = [NSDate timeIntervalSinceReferenceDate] - startTime; Optional nit: Mebbe define a TimeDelta here, outside the if/else https://codereview.chromium.org/2895013002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2895013002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:75912: + e.g. by tapping the New Incognito Tab entry in the main tools menu, and it Should "e.g. " be "i.e." for all of these histograms, or are there alternate UI flows as well?
code lgtm
https://codereview.chromium.org/2895013002/diff/20001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/browser_view_controller.mm (right): https://codereview.chromium.org/2895013002/diff/20001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/browser_view_controller.mm:1214: double duration = [NSDate timeIntervalSinceReferenceDate] - startTime; On 2017/05/23 22:55:22, Ilya Sherman wrote: > Optional nit: Mebbe define a TimeDelta here, outside the if/else Done! https://codereview.chromium.org/2895013002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2895013002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:75912: + e.g. by tapping the New Incognito Tab entry in the main tools menu, and it On 2017/05/23 22:55:23, Ilya Sherman wrote: > Should "e.g. " be "i.e." for all of these histograms, or are there alternate UI > flows as well? There are other flows - for example the plus button will also create a new tab and be tracked by this metric. There might also be other ways in the future.
The CQ bit was checked by peterlaurens@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by peterlaurens@chromium.org
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/2895013002/#ps40001 (title: "Review nits")
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": 1496956138306300,
"parent_rev": "08465dd44a9546be7d8fc87f0e1b9c00413cfedf", "commit_rev":
"90ac0d3da58ffe49a74ddf0bca158b4961cf1931"}
Message was sent while issue was closed.
Description was changed from ========== Perf metric tracking the wallclock time it takes to show a new tab page. BUG=724682 ========== to ========== Perf metric tracking the wallclock time it takes to show a new tab page. BUG=724682 Review-Url: https://codereview.chromium.org/2895013002 Cr-Commit-Position: refs/heads/master@{#478086} Committed: https://chromium.googlesource.com/chromium/src/+/90ac0d3da58ffe49a74ddf0bca15... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/90ac0d3da58ffe49a74ddf0bca15... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
