S13nServiceWorker: GetPageTransition and GetURLChainSize are not implemented |
|||
Issue descriptionThese are only used for UMA, but the log spam is pretty annoying. SWControlleeRequestHandler makes a SWURLRequestJob or SWURLLoaderJob then looks up a SW registration. If an active SW is found, it wants to know the page transition/url chain size for UMA for controlled page loads. In non-S13nSW, it gets this directly from SWURLRequestJob's URLRequest. In S13nSW, it asks SWURLLoaderJob for them, but it's not implemented. The SWControlleeRequestHandler should already know about the page transition as it's part of ResourceRequest (right now..) given to MaybeCreateLoader. Unfortunately, SWURLLoaderJob can't know about URL chain as far as I can tell. Only way back in the NavigationURLLoaderNetworkService do we know the number of redirects, via NavigationURLLoaderNetworkService::URLLoaderRequestController's |redirect_limit_|. So it seems NavigationURLLoaderNS needs to tell SWControlleeRequestHandler the # of redirects of the current request, by passing it to MaybeCreateLoader(). On the other handle, SWControlledPageLoad metrics are pretty strange. They are recorded for each network request so during redirects it gets counted multiple times.
,
Jan 30 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4eb5b28c438d15de0030ee4beccccf088be979ed commit 4eb5b28c438d15de0030ee4beccccf088be979ed Author: Matt Falkenhagen <falken@chromium.org> Date: Tue Jan 30 22:19:06 2018 service worker: Move PageTransition/RedirectChain metrics out of SWControlleeRequestHandler. These metrics don't work well at the ServiceWorkerControlleeRequestHandler layer for S13nServiceWorker, for the reasons in the bug. Actually they are pretty suspect for non-S13nServiceWorker too because we are recording them on every URL request, not just committed page load, so we are double-counting on redirects. This patch: 1) Moves the PageTransition metric to ServiceWorkerPageLoadMetricsObserver. This is a better fit also because page transition info is expected to be removed from ResourceRequest as per the TODO there. 2) Removes the RedirectChain metric. We haven't been using it. If needed we can always add one to ServiceWorkerPageLoadMetricsObserver alongside PageTransition. R=bmcquade, kinuko Bug: 805805 Change-Id: I58029612f8c7697ec280bcc80bec97f6f64d9dd9 Reviewed-on: https://chromium-review.googlesource.com/886102 Reviewed-by: Jesse Doherty <jwd@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Bryan McQuade <bmcquade@chromium.org> Commit-Queue: Matt Falkenhagen <falken@chromium.org> Cr-Commit-Position: refs/heads/master@{#533042} [modify] https://crrev.com/4eb5b28c438d15de0030ee4beccccf088be979ed/chrome/browser/page_load_metrics/observers/service_worker_page_load_metrics_observer.cc [modify] https://crrev.com/4eb5b28c438d15de0030ee4beccccf088be979ed/content/browser/service_worker/service_worker_controllee_request_handler.cc [modify] https://crrev.com/4eb5b28c438d15de0030ee4beccccf088be979ed/content/browser/service_worker/service_worker_metrics.cc [modify] https://crrev.com/4eb5b28c438d15de0030ee4beccccf088be979ed/content/browser/service_worker/service_worker_metrics.h [modify] https://crrev.com/4eb5b28c438d15de0030ee4beccccf088be979ed/content/browser/service_worker/service_worker_url_job_wrapper.cc [modify] https://crrev.com/4eb5b28c438d15de0030ee4beccccf088be979ed/content/browser/service_worker/service_worker_url_job_wrapper.h [modify] https://crrev.com/4eb5b28c438d15de0030ee4beccccf088be979ed/content/browser/service_worker/service_worker_url_loader_job.cc [modify] https://crrev.com/4eb5b28c438d15de0030ee4beccccf088be979ed/content/browser/service_worker/service_worker_url_loader_job.h [modify] https://crrev.com/4eb5b28c438d15de0030ee4beccccf088be979ed/tools/metrics/histograms/histograms.xml
,
Jan 31 2018
,
Jan 31 2018
|
|||
►
Sign in to add a comment |
|||
Comment 1 by falken@chromium.org
, Jan 25 2018Status: Started (was: Untriaged)