Add support for counting same-document AMP loads.
Though we can't track timing metrics like FCP for same-document loads,
we can at least track how freqently they occur. This can help to
understand how significant the performance of non-same-document AMP
loads is, relative to the set of all AMP loads (both same doc and non
same doc).
BUG=722690
Review-Url: https://codereview.chromium.org/2888673002
Cr-Commit-Position: refs/heads/master@{#472802}
Committed: https://chromium.googlesource.com/chromium/src/+/db2e320286b9752ed94f1bc6aa5e87eabe8eba6f
Description was changed from ========== Add support for counting same-document AMP loads. BUG= ========== to ...
3 years, 7 months ago
(2017-05-16 19:32:58 UTC)
#4
Description was changed from
==========
Add support for counting same-document AMP loads.
BUG=
==========
to
==========
Add support for counting same-document AMP loads.
Though we can't track timing metrics like FCP for same-document loads,
we can at least track how freqently they occur. This can help to
understand how significant the performance of non-same-document AMP
loads is, relative to the set of all AMP loads (both same doc and non
same doc).
BUG=
==========
Bryan McQuade
Description was changed from ========== Add support for counting same-document AMP loads. Though we can't ...
3 years, 7 months ago
(2017-05-16 19:33:19 UTC)
#5
Description was changed from
==========
Add support for counting same-document AMP loads.
Though we can't track timing metrics like FCP for same-document loads,
we can at least track how freqently they occur. This can help to
understand how significant the performance of non-same-document AMP
loads is, relative to the set of all AMP loads (both same doc and non
same doc).
BUG=
==========
to
==========
Add support for counting same-document AMP loads.
Though we can't track timing metrics like FCP for same-document loads,
we can at least track how freqently they occur. This can help to
understand how significant the performance of non-same-document AMP
loads is, relative to the set of all AMP loads (both same doc and non
same doc).
BUG=722690
==========
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2888673002/60001
3 years, 7 months ago
(2017-05-16 19:33:31 UTC)
#6
https://codereview.chromium.org/2888673002/diff/60001/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h (right): https://codereview.chromium.org/2888673002/diff/60001/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h#newcode30 chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:30: void OnDidFinishSubFrameNavigation( On 2017/05/16 at 22:32:10, RyanSturm wrote: > ...
3 years, 7 months ago
(2017-05-17 12:38:34 UTC)
#14
https://codereview.chromium.org/2888673002/diff/60001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h
(right):
https://codereview.chromium.org/2888673002/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:30:
void OnDidFinishSubFrameNavigation(
On 2017/05/16 at 22:32:10, RyanSturm wrote:
> Is the OnDidFinishSubFrameNavigation change unrelated to the AMP change? It's
fine to leave in; I'm just making sure I'm not missing something
Yeah - unrelated - I didn't want to make the new method I was adding return
ObservePolicy, but also wanted these 2 to be consistent. I can't think of a case
where an observer would want to stop observing in this callback, so think void
is a better return value for both.
https://codereview.chromium.org/2888673002/diff/60001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/amp_page_load_metrics_observer.cc
(right):
https://codereview.chromium.org/2888673002/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/amp_page_load_metrics_observer.cc:102:
if (url == committed_url_)
On 2017/05/16 at 22:32:10, RyanSturm wrote:
> IIUC, this check seems odd. Specifically, if a user navigates to a page that
can show some amp views, switching back and forth between two amp views we will
log uma for each switch, but if you navigate directly to the one of the amp
views initially and switch between the two, it will only count half as many.
Yeah, you're right - we want to ignore nav updates where the URL doesn't change,
but navigating back to the committed URL should still count as a view if it's an
amp document. Updated the logic to reflect this.
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/455935)
3 years, 7 months ago
(2017-05-17 12:47:13 UTC)
#18
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/429866)
3 years, 7 months ago
(2017-05-18 02:51:08 UTC)
#28
CQ is committing da patch. Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495118213731400, "parent_rev": "b19aa78ac188365efe6b54c3889d67e9225b10d2", "commit_rev": "db2e320286b9752ed94f1bc6aa5e87eabe8eba6f"}
3 years, 7 months ago
(2017-05-18 14:42:31 UTC)
#32
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495118213731400,
"parent_rev": "b19aa78ac188365efe6b54c3889d67e9225b10d2", "commit_rev":
"db2e320286b9752ed94f1bc6aa5e87eabe8eba6f"}
commit-bot: I haz the power
Description was changed from ========== Add support for counting same-document AMP loads. Though we can't ...
3 years, 7 months ago
(2017-05-18 14:42:43 UTC)
#33
Message was sent while issue was closed.
Description was changed from
==========
Add support for counting same-document AMP loads.
Though we can't track timing metrics like FCP for same-document loads,
we can at least track how freqently they occur. This can help to
understand how significant the performance of non-same-document AMP
loads is, relative to the set of all AMP loads (both same doc and non
same doc).
BUG=722690
==========
to
==========
Add support for counting same-document AMP loads.
Though we can't track timing metrics like FCP for same-document loads,
we can at least track how freqently they occur. This can help to
understand how significant the performance of non-same-document AMP
loads is, relative to the set of all AMP loads (both same doc and non
same doc).
BUG=722690
Review-Url: https://codereview.chromium.org/2888673002
Cr-Commit-Position: refs/heads/master@{#472802}
Committed:
https://chromium.googlesource.com/chromium/src/+/db2e320286b9752ed94f1bc6aa5e...
==========
commit-bot: I haz the power
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/db2e320286b9752ed94f1bc6aa5e87eabe8eba6f
3 years, 7 months ago
(2017-05-18 14:42:45 UTC)
#34
Issue 2888673002: Add support for counting same-document AMP loads.
(Closed)
Created 3 years, 7 months ago by Bryan McQuade
Modified 3 years, 7 months ago
Reviewers: Ilya Sherman, RyanSturm
Base URL:
Comments: 6