New issue
Advanced search Search tips

Issue 703696 link

Starred by 1 user
Status: Fixed
Owner:
ryansturm@chromium.org
Closed: Mar 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug



Sign in to add a comment

AMP PageLoadMetrics UMA does not record corectly

Project Member Reported by ryansturm@chromium.org, Mar 21 2017 
Throughout the AMP Page UMA recording there is a common pattern:


if (!WasStartedInForegroundOptionalEventInForeground(
          timing.dom_content_loaded_event_start, info))
  return;

Specifically, the problem here is that every callback checks dom_content_loaded_event_start instead of the corresponding event for the observer event.
Project Member

Comment 1 by bugdroid1@chromium.org, Mar 28 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5d2208ce695e4352801e8cb2578ea86b22de9707

commit 5d2208ce695e4352801e8cb2578ea86b22de9707
Author: ryansturm <ryansturm@chromium.org>
Date: Tue Mar 28 04:45:16 2017

Fixing reporting of AMP/previews page load information

AMP page load information is skewed based on checking the wrong event's
foreground state. I am re-versioning the UMA because the new UMA will be
vastly different based on this bug.

This bug applies to previews as well, but the UMA does not need re-versioning.

BUG= 703696 

Review-Url: https://codereview.chromium.org/2761353002
Cr-Commit-Position: refs/heads/master@{#460014}

[modify] https://crrev.com/5d2208ce695e4352801e8cb2578ea86b22de9707/chrome/browser/page_load_metrics/observers/amp_page_load_metrics_observer.cc
[modify] https://crrev.com/5d2208ce695e4352801e8cb2578ea86b22de9707/chrome/browser/page_load_metrics/observers/amp_page_load_metrics_observer_unittest.cc
[modify] https://crrev.com/5d2208ce695e4352801e8cb2578ea86b22de9707/chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.cc
[modify] https://crrev.com/5d2208ce695e4352801e8cb2578ea86b22de9707/tools/metrics/histograms/histograms.xml

Comment 2 by ryansturm@chromium.org, Mar 28 2017

Labels: Merge-Request-58
Project Member

Comment 3 by sheriffbot@chromium.org, Mar 29 2017

Labels: -Merge-Request-58 Hotlist-Merge-Approved Merge-Approved-58
Your change meets the bar and is auto-approved for M58. Please go ahead and merge the CL to branch 3029 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 4 by bengr@chromium.org, Mar 29 2017

Status: Started (was: Assigned)
Project Member

Comment 5 by bugdroid1@chromium.org, Mar 30 2017

Labels: -merge-approved-58 merge-merged-3029
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e17de5894702a3cb9e69878893abf4963ae8d9e8

commit e17de5894702a3cb9e69878893abf4963ae8d9e8
Author: Ryan Sturm <ryansturm@chromium.org>
Date: Thu Mar 30 18:04:57 2017

Fixing reporting of AMP/previews page load information

AMP page load information is skewed based on checking the wrong event's
foreground state. I am re-versioning the UMA because the new UMA will be
vastly different based on this bug.

This bug applies to previews as well, but the UMA does not need re-versioning.

BUG= 703696 

Review-Url: https://codereview.chromium.org/2761353002
Cr-Commit-Position: refs/heads/master@{#460014}
(cherry picked from commit 5d2208ce695e4352801e8cb2578ea86b22de9707)

Review-Url: https://codereview.chromium.org/2786883004 .
Cr-Commit-Position: refs/branch-heads/3029@{#494}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/e17de5894702a3cb9e69878893abf4963ae8d9e8/chrome/browser/page_load_metrics/observers/amp_page_load_metrics_observer.cc
[modify] https://crrev.com/e17de5894702a3cb9e69878893abf4963ae8d9e8/chrome/browser/page_load_metrics/observers/amp_page_load_metrics_observer_unittest.cc
[modify] https://crrev.com/e17de5894702a3cb9e69878893abf4963ae8d9e8/chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.cc
[modify] https://crrev.com/e17de5894702a3cb9e69878893abf4963ae8d9e8/tools/metrics/histograms/histograms.xml

Comment 6 by ryansturm@chromium.org, Mar 30 2017

Status: Fixed (was: Started)
►

Sign in to add a comment