Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2694183004:
Adding TabRestore PLM UMA (Closed)
|
Created:
3 years, 10 months ago by RyanSturm Modified:
3 years, 10 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews, creis+watch_chromium.org, csharrison+watch_chromium.org, darin-cc_chromium.org, jam, loading-reviews+metrics_chromium.org, nasko+codewatch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding TabRestore PLM UMA
Adds bytes used UMA for TabRestores. There will be follow-up CLs to add
various other UMA to this observer and CorePageLoadMetricsObserver.
TabRestores here only include the first navigation entry after a tab
restore (forward-back navigations that were restored are excluded). The
goal is to evaluate how expensive TabRestores are for data sensitive
users.
BUG=686887
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2694183004
Cr-Commit-Position: refs/heads/master@{#452041}
Committed: https://chromium.googlesource.com/chromium/src/+/248635a7e9e48fdebb242b60e81c46e90fca35e7
Patch Set 1 #
Total comments: 10
Patch Set 2 : rebase + comments #Messages
Total messages: 43 (27 generated)
Description was changed from ========== Adding TabRestore PLM UMA Adds bytes used UMA for TabRestores. There will be follow-up CLs to add various other UMA to this observer and CorePageLoadMetricsObserver. TabRestores here only include the first navigation entry after a tab restore (forward-back navigations that were restored are excluded). The goal is to evaluate how expensive TabRestores are for data sensitive users. BUG=686887 ========== to ========== Adding TabRestore PLM UMA Adds bytes used UMA for TabRestores. There will be follow-up CLs to add various other UMA to this observer and CorePageLoadMetricsObserver. TabRestores here only include the first navigation entry after a tab restore (forward-back navigations that were restored are excluded). The goal is to evaluate how expensive TabRestores are for data sensitive users. BUG=686887 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by ryansturm@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.
ryansturm@chromium.org changed reviewers: + asvitkine@chromium.org, bmcquade@chromium.org, clamy@chromium.org
clamy: PTAL @ navigation_handle* changes bmcquade: PTAL @ new tab_restore observer asvitkine: PTAL @ histograms
histograms lgtm https://codereview.chromium.org/2694183004/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2694183004/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer.cc:39: if (!IsTabRestore(navigation_handle)) { Nit: No {} Same for other 1-liners.
LGTM, thanks! https://codereview.chromium.org/2694183004/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2694183004/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer.cc:39: if (!IsTabRestore(navigation_handle)) { On 2017/02/15 at 21:18:37, Alexei Svitkine (slow) wrote: > Nit: No {} > > Same for other 1-liners. could also use a ternary operator: return IsTabRestore() ? CONTINUE_OBSERVING : STOP_OBSERVING; up to you though - either style fine by me. https://codereview.chromium.org/2694183004/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2694183004/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer_unittest.cc:40: bool is_restore_; nit: const bool is_restore_; https://codereview.chromium.org/2694183004/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer_unittest.cc:128: bool is_restore_; since exec ordering is a bit subtle, can we make this a base::Optional<bool> so if we try to read from the value it'll blow up if we read from it before the value was set?
https://codereview.chromium.org/2694183004/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2694183004/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer.cc:62: if (!info.committed_url.is_empty()) { I'm about to land https://codereview.chromium.org/2692373003 which removes the committed_url field (replacing with just 'url'). You'll want to change this line to: if (info.did_commit) { https://codereview.chromium.org/2694183004/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2694183004/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer_unittest.cc:36: bool IsTabRestore(content::NavigationHandle* navigation_handle) override { clamy, is there a way to make a NavigationHandle report that it's associated with tab restore for unit test purposes? If so, it'd be nice to use the real NavigationHandle impl and not have to stub this out.
The CQ bit was checked by ryansturm@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.
clamy, PTAL I couldn't find a good way to marry the page load metrics harness with Restore semantics, so I had to override a class method, is there a way to set the restore state on NavigationHandle using the RenderViewTestHarness methods? https://codereview.chromium.org/2694183004/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2694183004/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer.cc:39: if (!IsTabRestore(navigation_handle)) { On 2017/02/16 17:52:50, Bryan McQuade wrote: > On 2017/02/15 at 21:18:37, Alexei Svitkine (slow) wrote: > > Nit: No {} > > > > Same for other 1-liners. > > could also use a ternary operator: > return IsTabRestore() ? CONTINUE_OBSERVING : STOP_OBSERVING; > > up to you though - either style fine by me. Done. https://codereview.chromium.org/2694183004/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer.cc:62: if (!info.committed_url.is_empty()) { On 2017/02/16 18:32:54, Bryan McQuade wrote: > I'm about to land https://codereview.chromium.org/2692373003 which removes the > committed_url field (replacing with just 'url'). You'll want to change this line > to: > > if (info.did_commit) { Done. https://codereview.chromium.org/2694183004/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2694183004/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer_unittest.cc:40: bool is_restore_; On 2017/02/16 17:52:50, Bryan McQuade wrote: > nit: const bool is_restore_; Done. https://codereview.chromium.org/2694183004/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer_unittest.cc:128: bool is_restore_; On 2017/02/16 17:52:50, Bryan McQuade wrote: > since exec ordering is a bit subtle, can we make this a base::Optional<bool> so > if we try to read from the value it'll blow up if we read from it before the > value was set? Done.
On 2017/02/17 02:21:24, Ryan Sturm wrote: > clamy, PTAL content/ lgtm. > > I couldn't find a good way to marry the page load metrics harness with Restore > semantics, so I had to override a class method, is there a way to set the > restore state on NavigationHandle using the RenderViewTestHarness methods? Unfortunately it's not possible. The only way I see to configure the restore type is to start the navigation using the Restore method of the NavigationController. > > https://codereview.chromium.org/2694183004/diff/1/chrome/browser/page_load_me... > File > chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer.cc > (right): > > https://codereview.chromium.org/2694183004/diff/1/chrome/browser/page_load_me... > chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer.cc:39: > if (!IsTabRestore(navigation_handle)) { > On 2017/02/16 17:52:50, Bryan McQuade wrote: > > On 2017/02/15 at 21:18:37, Alexei Svitkine (slow) wrote: > > > Nit: No {} > > > > > > Same for other 1-liners. > > > > could also use a ternary operator: > > return IsTabRestore() ? CONTINUE_OBSERVING : STOP_OBSERVING; > > > > up to you though - either style fine by me. > > Done. > > https://codereview.chromium.org/2694183004/diff/1/chrome/browser/page_load_me... > chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer.cc:62: > if (!info.committed_url.is_empty()) { > On 2017/02/16 18:32:54, Bryan McQuade wrote: > > I'm about to land https://codereview.chromium.org/2692373003 which removes the > > committed_url field (replacing with just 'url'). You'll want to change this > line > > to: > > > > if (info.did_commit) { > > Done. > > https://codereview.chromium.org/2694183004/diff/1/chrome/browser/page_load_me... > File > chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer_unittest.cc > (right): > > https://codereview.chromium.org/2694183004/diff/1/chrome/browser/page_load_me... > chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer_unittest.cc:40: > bool is_restore_; > On 2017/02/16 17:52:50, Bryan McQuade wrote: > > nit: const bool is_restore_; > > Done. > > https://codereview.chromium.org/2694183004/diff/1/chrome/browser/page_load_me... > chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer_unittest.cc:128: > bool is_restore_; > On 2017/02/16 17:52:50, Bryan McQuade wrote: > > since exec ordering is a bit subtle, can we make this a base::Optional<bool> > so > > if we try to read from the value it'll blow up if we read from it before the > > value was set? > > Done.
The CQ bit was checked by ryansturm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmcquade@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2694183004/#ps20001 (title: "rebase + comments")
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
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_site_isolation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by ryansturm@chromium.org
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
Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
The CQ bit was checked by ryansturm@chromium.org
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
Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by ryansturm@chromium.org
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
Exceeded global retry quota
The CQ bit was checked by ryansturm@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 ryansturm@chromium.org
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": 20001, "attempt_start_ts": 1487773861924830,
"parent_rev": "b3e0b3d224ae19607c192ee99fc85d146baf4053", "commit_rev":
"81f90e00d6f1b865ce1449213c53fe26c7931952"}
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487773861924830,
"parent_rev": "7879e03c771acbe667842fb1b4bdf406054b5146", "commit_rev":
"248635a7e9e48fdebb242b60e81c46e90fca35e7"}
Message was sent while issue was closed.
Description was changed from ========== Adding TabRestore PLM UMA Adds bytes used UMA for TabRestores. There will be follow-up CLs to add various other UMA to this observer and CorePageLoadMetricsObserver. TabRestores here only include the first navigation entry after a tab restore (forward-back navigations that were restored are excluded). The goal is to evaluate how expensive TabRestores are for data sensitive users. BUG=686887 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Adding TabRestore PLM UMA Adds bytes used UMA for TabRestores. There will be follow-up CLs to add various other UMA to this observer and CorePageLoadMetricsObserver. TabRestores here only include the first navigation entry after a tab restore (forward-back navigations that were restored are excluded). The goal is to evaluate how expensive TabRestores are for data sensitive users. BUG=686887 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2694183004 Cr-Commit-Position: refs/heads/master@{#452041} Committed: https://chromium.googlesource.com/chromium/src/+/248635a7e9e48fdebb242b60e81c... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/248635a7e9e48fdebb242b60e81c... |
