Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 months, 1 week ago by Bryan McQuade
Modified:
8 months 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!
8 months, 1 week 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 ...
8 months, 1 week 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 ...
8 months, 1 week 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 ...
8 months 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): ...
8 months ago (2017-02-21 21:51:59 UTC) #57
Bryan McQuade
isherman, PTAL for histograms.xml, thanks!
8 months 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. ...
8 months 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 ...
8 months 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, ...
8 months 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, ...
8 months 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
8 months ago (2017-02-22 11:38:44 UTC) #75
Bryan McQuade
mattcary, PTAL for small change to chrome/browser/prerender/prerender_browsertest.cc, thanks!
8 months 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 ...
8 months 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 ...
8 months 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
8 months ago (2017-02-22 14:17:33 UTC) #86
commit-bot: I haz the power
8 months 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 81bcdb8aa