Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(231)

Issue 2034223002: Add immediate variants of doc.write page load metrics (Closed)

Created:
4 years, 6 months ago by shivanisha
Modified:
4 years, 6 months ago
CC:
asvitkine+watch_chromium.org, chromium-reviews, csharrison+watch_chromium.org, loading-reviews+metrics_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Document.write page load metrics observer to log metrics as timing and metadata are updated and not just on OnComplete(). Keeping the OnComplete() logging intact for a few more relaeses and will phase them out eventually. BUG=611813 Committed: https://crrev.com/842111dfe1b7b620ffe46782ac05bd587ef92488 Cr-Commit-Position: refs/heads/master@{#400680}

Patch Set 1 #

Patch Set 2 : Add immediate metrics for document.write observer. #

Patch Set 3 : Add immediate metrics for doc.write observer #

Patch Set 4 : Add immediate metrics for doc.write observer. #

Total comments: 4

Patch Set 5 : Reload to be counted only once. #

Total comments: 2

Patch Set 6 : Bryan's latest feedback incorporated. #

Total comments: 2

Patch Set 7 : Code comment added. #

Messages

Total messages: 17 (5 generated)
shivanisha
4 years, 6 months ago (2016-06-06 19:31:37 UTC) #2
Bryan McQuade
this looks good, thanks! one question about making sure we don't log multiple times. https://codereview.chromium.org/2034223002/diff/60001/chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc ...
4 years, 6 months ago (2016-06-14 09:01:38 UTC) #3
shivanisha
https://codereview.chromium.org/2034223002/diff/60001/chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2034223002/diff/60001/chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc#newcode165 chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:165: UMA_HISTOGRAM_COUNTS(internal::kHistogramDocWriteBlockReloadCount, 1); On 2016/06/14 at 09:01:38, Bryan McQuade wrote: ...
4 years, 6 months ago (2016-06-16 15:04:44 UTC) #4
Bryan McQuade
https://codereview.chromium.org/2034223002/diff/60001/chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2034223002/diff/60001/chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc#newcode165 chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:165: UMA_HISTOGRAM_COUNTS(internal::kHistogramDocWriteBlockReloadCount, 1); On 2016/06/16 at 15:04:43, shivanisha wrote: > ...
4 years, 6 months ago (2016-06-16 15:29:51 UTC) #5
shivanisha
https://codereview.chromium.org/2034223002/diff/60001/chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2034223002/diff/60001/chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc#newcode165 chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:165: UMA_HISTOGRAM_COUNTS(internal::kHistogramDocWriteBlockReloadCount, 1); On 2016/06/16 at 15:29:51, Bryan McQuade wrote: ...
4 years, 6 months ago (2016-06-16 21:14:07 UTC) #6
Bryan McQuade
lgtm LGTM, thanks! https://codereview.chromium.org/2034223002/diff/80001/chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2034223002/diff/80001/chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc#newcode160 chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:160: if (info.metadata.behavior_flags & i'm pretty sure ...
4 years, 6 months ago (2016-06-17 13:24:03 UTC) #7
shivanisha
csharrison@, PTAL for owner approval, holte@, PTAL at histograms.xml. Thanks!
4 years, 6 months ago (2016-06-17 20:42:35 UTC) #9
Charlie Harrison
Not a full review, but Bryan is a committer now so his stamp is enough. ...
4 years, 6 months ago (2016-06-17 21:36:19 UTC) #10
Steven Holte
lgtm
4 years, 6 months ago (2016-06-18 00:21:57 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2034223002/120001
4 years, 6 months ago (2016-06-20 13:53:51 UTC) #14
commit-bot: I haz the power
Committed patchset #7 (id:120001)
4 years, 6 months ago (2016-06-20 15:18:16 UTC) #15
commit-bot: I haz the power
4 years, 6 months ago (2016-06-20 15:19:45 UTC) #17
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/842111dfe1b7b620ffe46782ac05bd587ef92488
Cr-Commit-Position: refs/heads/master@{#400680}

Powered by Google App Engine
This is Rietveld 408576698