Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2380773002:
Prevent tracking metrics for cases such as 204s and downloads. (Closed)
|
Created:
4 years, 2 months ago by Bryan McQuade Modified:
4 years, 2 months ago CC:
chromium-reviews, creis+watch_chromium.org, csharrison+watch_chromium.org, darin-cc_chromium.org, jam, loading-reviews+metrics_chromium.org, nasko+codewatch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPrevent tracking metrics for cases such as 204s and downloads.
More generally, ignore tracking metrics for navigations that result
in receiving a successful HTTP response but that don't commit.
The PageLoad UMA implementation currently tracks 204s and downloads as
aborts, as their net error status reported on NavigationHandle is
ERR_ABORTED. In reality these are just loads that terminated without
committing, so they should instead be ignored.
Note that the new tests fail when using browser-side navigation, and
are currently disabled there. See crbug.com/652767 for details.
BUG=651448
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/7226898b5ec5e5d5eb11ef58606e5ca84c435919
Cr-Commit-Position: refs/heads/master@{#423133}
Patch Set 1 #Patch Set 2 : fix comment #Patch Set 3 : add browser tests #Patch Set 4 : address windows test failure #Patch Set 5 : use existing test file #
Total comments: 4
Patch Set 6 : add additional test logic for download management #Patch Set 7 : fix download test on windows #Patch Set 8 : address comments #Patch Set 9 : fix comment, and disable tests for browser side navigation #Patch Set 10 : fix comment, and disable tests for browser side navigation #
Messages
Total messages: 52 (41 generated)
Description was changed from ========== Ignore tracking metrics for cases such as 204s and downloads. More generally, ignore tracking metrics for navigations that result in receiving a successful HTTP response but that don't commit. BUG=TBD ========== to ========== Ignore tracking metrics for cases such as 204s and downloads. More generally, ignore tracking metrics for navigations that result in receiving a successful HTTP response but that don't commit. BUG=TBD CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Description was changed from ========== Ignore tracking metrics for cases such as 204s and downloads. More generally, ignore tracking metrics for navigations that result in receiving a successful HTTP response but that don't commit. BUG=TBD CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Prevent tracking metrics for cases such as 204s and downloads. More generally, ignore tracking metrics for navigations that result in receiving a successful HTTP response but that don't commit. BUG=TBD CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Prevent tracking metrics for cases such as 204s and downloads. More generally, ignore tracking metrics for navigations that result in receiving a successful HTTP response but that don't commit. BUG=TBD CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Prevent tracking metrics for cases such as 204s and downloads. More generally, ignore tracking metrics for navigations that result in receiving a successful HTTP response but that don't commit. BUG=651448 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
bmcquade@chromium.org changed reviewers: + clamy@chromium.org, csharrison@chromium.org
clamy, PTAL for NavigationHandle change, thanks! csharrison, PTAL for page_load_metrics changes, thanks!
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Thanks! A few comments below. https://codereview.chromium.org/2380773002/diff/80001/content/public/browser/... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/2380773002/diff/80001/content/public/browser/... content/public/browser/navigation_handle.h:159: // Returns the response headers for the request or nullptr if there are Please mention that these are the final headers. we don't want people to be confused and think they could get the headers from a redirect (wasn't a problem before since you could only access them after the final response was received). https://codereview.chromium.org/2380773002/diff/80001/content/public/browser/... content/public/browser/navigation_handle.h:160: // none. The headers returned should not be modified, as modifications will nit: none or they have not been received yet.
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! Addressed comments. PTAL. https://codereview.chromium.org/2380773002/diff/80001/content/public/browser/... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/2380773002/diff/80001/content/public/browser/... content/public/browser/navigation_handle.h:159: // Returns the response headers for the request or nullptr if there are On 2016/09/29 at 15:35:57, clamy wrote: > Please mention that these are the final headers. we don't want people to be confused and think they could get the headers from a redirect (wasn't a problem before since you could only access them after the final response was received). Done, thanks! https://codereview.chromium.org/2380773002/diff/80001/content/public/browser/... content/public/browser/navigation_handle.h:160: // none. The headers returned should not be modified, as modifications will On 2016/09/29 at 15:35:57, clamy wrote: > nit: none or they have not been received yet. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM thank you for solving this long standing annoyance!
Description was changed from ========== Prevent tracking metrics for cases such as 204s and downloads. More generally, ignore tracking metrics for navigations that result in receiving a successful HTTP response but that don't commit. BUG=651448 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Prevent tracking metrics for cases such as 204s and downloads. More generally, ignore tracking metrics for navigations that result in receiving a successful HTTP response but that don't commit. The PageLoad UMA implementation currently tracks 204s and downloads as aborts, as their net error status reported on NavigationHandle is ERR_ABORTED. In reality these are just loads that terminated without committing, so they should instead be ignored. BUG=651448 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Thanks! Lgtm. Sorry for the delay, was ooo yesterday.
The CQ bit was checked by bmcquade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for tracking down the issue in PkzNavigate! Still lgtm.
Description was changed from ========== Prevent tracking metrics for cases such as 204s and downloads. More generally, ignore tracking metrics for navigations that result in receiving a successful HTTP response but that don't commit. The PageLoad UMA implementation currently tracks 204s and downloads as aborts, as their net error status reported on NavigationHandle is ERR_ABORTED. In reality these are just loads that terminated without committing, so they should instead be ignored. BUG=651448 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Prevent tracking metrics for cases such as 204s and downloads. More generally, ignore tracking metrics for navigations that result in receiving a successful HTTP response but that don't commit. The PageLoad UMA implementation currently tracks 204s and downloads as aborts, as their net error status reported on NavigationHandle is ERR_ABORTED. In reality these are just loads that terminated without committing, so they should instead be ignored. Note that the new tests fail when using browser-side navigation, and are currently disabled there. See crbug.com/652767 for details. BUG=651448 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by bmcquade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2380773002/#ps180001 (title: "fix comment, and disable tests for browser side navigation")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Prevent tracking metrics for cases such as 204s and downloads. More generally, ignore tracking metrics for navigations that result in receiving a successful HTTP response but that don't commit. The PageLoad UMA implementation currently tracks 204s and downloads as aborts, as their net error status reported on NavigationHandle is ERR_ABORTED. In reality these are just loads that terminated without committing, so they should instead be ignored. Note that the new tests fail when using browser-side navigation, and are currently disabled there. See crbug.com/652767 for details. BUG=651448 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Prevent tracking metrics for cases such as 204s and downloads. More generally, ignore tracking metrics for navigations that result in receiving a successful HTTP response but that don't commit. The PageLoad UMA implementation currently tracks 204s and downloads as aborts, as their net error status reported on NavigationHandle is ERR_ABORTED. In reality these are just loads that terminated without committing, so they should instead be ignored. Note that the new tests fail when using browser-side navigation, and are currently disabled there. See crbug.com/652767 for details. BUG=651448 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/7226898b5ec5e5d5eb11ef58606e5ca84c435919 Cr-Commit-Position: refs/heads/master@{#423133} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/7226898b5ec5e5d5eb11ef58606e5ca84c435919 Cr-Commit-Position: refs/heads/master@{#423133} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
