Chromium Code Reviews
Help | Chromium Project | Sign in
(2)

Issue 2699933003: Generalize abort tracking to page end state tracking (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 days, 18 hours ago by Bryan McQuade
Modified:
1 day, 2 hours ago
CC:
asvitkine+watch_chromium.org, chromium-reviews, csharrison+watch_chromium.org, loading-reviews+metrics_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Generalize abort tracking to page end state tracking page_load_metrics abort tracking combines tracking of some page 'end state' information, such as navigating away from the page or closing the tab, as well as some intermediate page state information, such as backgrounding the page. this makes the implementation complex and sometimes difficult to reason about. this change refactors abort tracking into tracking page end states, which abort observers can convert into PageAbortInfo which provides a more abort-centric view of the page load. BUG=693725 Review-Url: https://codereview.chromium.org/2699933003 Cr-Commit-Position: refs/heads/master@{#452036} Committed: https://chromium.googlesource.com/chromium/src/+/909ebae98693013efbeb783a143c02c5bf2214f3

Patch Set 1 #

Patch Set 2 : update #

Patch Set 3 : fix browsertest #

Patch Set 4 : more cleanup #

Patch Set 5 : small fixes #

Patch Set 6 : fix build #

Patch Set 7 : cleanup #

Patch Set 8 : cleanup #

Patch Set 9 : cleanup #

Patch Set 10 : add histograms #

Patch Set 11 : whitespace #

Patch Set 12 : cleanup #

Patch Set 13 : cleanup #

Patch Set 14 : reorder to simplify #

Patch Set 15 : fix histogram typo #

Total comments: 8

Patch Set 16 : address comments #

Patch Set 17 : add additional histogram #

Total comments: 4

Patch Set 18 : address comments #

Total comments: 9

Patch Set 19 : address comments #

Total comments: 1

Patch Set 20 : address comment #

Patch Set 21 : fix comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+659 lines, -413 lines) Patch
M chrome/browser/page_load_metrics/metrics_web_contents_observer.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +6 lines, -6 lines 0 comments Download
M chrome/browser/page_load_metrics/metrics_web_contents_observer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 8 chunks +44 lines, -29 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.h View 1 2 3 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc View 1 2 3 4 5 6 7 7 chunks +105 lines, -134 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 4 chunks +53 lines, -6 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +8 lines, -0 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc View 1 2 3 8 chunks +74 lines, -82 lines 0 comments Download
M chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 3 chunks +6 lines, -3 lines 0 comments Download
M chrome/browser/page_load_metrics/page_load_metrics_observer.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +48 lines, -37 lines 0 comments Download
M chrome/browser/page_load_metrics/page_load_metrics_observer.cc View 1 2 3 2 chunks +6 lines, -6 lines 0 comments Download
M chrome/browser/page_load_metrics/page_load_metrics_util.h View 1 2 3 4 5 6 7 3 chunks +59 lines, -0 lines 0 comments Download
M chrome/browser/page_load_metrics/page_load_metrics_util.cc View 1 2 3 4 5 6 7 2 chunks +52 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/page_load_tracker.h View 1 2 3 4 5 6 7 8 8 chunks +39 lines, -35 lines 0 comments Download
M chrome/browser/page_load_metrics/page_load_tracker.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 13 chunks +76 lines, -71 lines 0 comments Download
M chrome/browser/prerender/prerender_browsertest.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 8 chunks +79 lines, -1 line 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 89 (73 generated)
Bryan McQuade
Here is the abort -> endreason refactor. PTAL, thanks!
5 days, 18 hours ago (2017-02-17 22:12:57 UTC) #37
Charlie Harrison
First pass, mostly ignored mechanical changes. https://codereview.chromium.org/2699933003/diff/240001/chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc File chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc (left): https://codereview.chromium.org/2699933003/diff/240001/chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc#oldcode311 chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:311: Why remove the ...
5 days, 18 hours ago (2017-02-17 22:36:49 UTC) #39
Bryan McQuade
Thanks! Addressed comments. PTAL. https://codereview.chromium.org/2699933003/diff/240001/chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc File chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc (left): https://codereview.chromium.org/2699933003/diff/240001/chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc#oldcode311 chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:311: On 2017/02/17 at 22:36:49, Charlie ...
5 days, 16 hours ago (2017-02-18 00:30:16 UTC) #42
Charlie Harrison
LG but I would appreciate a few tests for the new histograms. https://codereview.chromium.org/2699933003/diff/280001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc ...
1 day, 20 hours ago (2017-02-21 19:49:31 UTC) #55
Bryan McQuade
Thanks! Addressed comments & added unit tests & browser test. PTAL. https://codereview.chromium.org/2699933003/diff/280001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right): ...
1 day, 18 hours ago (2017-02-21 21:51:59 UTC) #57
Bryan McQuade
isherman, PTAL for histograms.xml, thanks!
1 day, 18 hours ago (2017-02-21 21:53:47 UTC) #60
Charlie Harrison
LGTM w/ nit https://codereview.chromium.org/2699933003/diff/300001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/2699933003/diff/300001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc#newcode436 chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:436: // provisional / aborted provisional loads. ...
1 day, 18 hours ago (2017-02-21 21:55:10 UTC) #61
Ilya Sherman
https://codereview.chromium.org/2699933003/diff/300001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2699933003/diff/300001/tools/metrics/histograms/histograms.xml#newcode44480 tools/metrics/histograms/histograms.xml:44480: + being backgrounded, for page loads that started in ...
1 day, 17 hours ago (2017-02-21 23:08:29 UTC) #62
Bryan McQuade
Thanks! I addressed comments. PTAL, thanks! https://codereview.chromium.org/2699933003/diff/300001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2699933003/diff/300001/tools/metrics/histograms/histograms.xml#newcode44480 tools/metrics/histograms/histograms.xml:44480: + being backgrounded, ...
1 day, 15 hours ago (2017-02-22 00:57:33 UTC) #66
Ilya Sherman
Metrics LGTM, thanks for the clarifications! https://codereview.chromium.org/2699933003/diff/320001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2699933003/diff/320001/tools/metrics/histograms/histograms.xml#newcode44226 tools/metrics/histograms/histograms.xml:44226: + being backgrounded, ...
1 day, 15 hours ago (2017-02-22 01:33:13 UTC) #68
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2699933003/360001
1 day, 5 hours ago (2017-02-22 11:38:44 UTC) #75
Bryan McQuade
mattcary, PTAL for small change to chrome/browser/prerender/prerender_browsertest.cc, thanks!
1 day, 4 hours ago (2017-02-22 11:45:40 UTC) #78
mattcary
LGTM for prerender browser test. There's been a few CLs where the PageLoadExtraInfo creation needs ...
1 day, 4 hours ago (2017-02-22 11:54:17 UTC) #81
Bryan McQuade
On 2017/02/22 at 11:54:17, mattcary wrote: > LGTM for prerender browser test. > > There's ...
1 day, 4 hours ago (2017-02-22 11:55:18 UTC) #82
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2699933003/360001
1 day, 2 hours ago (2017-02-22 14:17:33 UTC) #86
commit-bot: I haz the power
1 day, 2 hours ago (2017-02-22 14:22:34 UTC) #89
Message was sent while issue was closed.
Committed patchset #21 (id:360001) as
https://chromium.googlesource.com/chromium/src/+/909ebae98693013efbeb783a143c...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f8e48bd