Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1871393002:
Add NavigationToFirstContentfulPaint UMA for ServiceWorker controlled pages. (Closed)
|
Created:
4 years, 8 months ago by horo Modified:
4 years, 7 months ago Reviewers:
Kunihiko Sakamoto, nhiroki, Charlie Harrison, kinuko, Bryan McQuade, Nate Chapin, Ilya Sherman CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, csharrison+watch_chromium.org, dglazkov+blink, 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. |
DescriptionAdd NavigationToFirstContentfulPaint UMA for ServiceWorker controlled pages.
Currently we don't have PageLoad UMAs for Service Worker controlled pages.
We will do some experiments which may affect the performance of Service Worker controlled pages.
This change will add PageLoad.Clients.ServiceWorker.Timing2.NavigationToFirstContentfulPaint
BUG=581613
Committed: https://crrev.com/e3107a132dcd87e9310627ccb3ea15fea8bd16a1
Cr-Commit-Position: refs/heads/master@{#390001}
Patch Set 1 #
Total comments: 2
Patch Set 2 : incorporated ksakamoto's comment #
Total comments: 2
Patch Set 3 : diff from https://codereview.chromium.org/1857443002/#ps110001 #
Total comments: 4
Patch Set 4 : Add comment and check ShouldSendMetrics() #
Total comments: 2
Patch Set 5 : Check page_timing_metrics_sender_ #
Total comments: 10
Patch Set 6 : incorporated bmcquade's comment #
Total comments: 4
Patch Set 7 : incorporated bmcquade's comment #
Total comments: 3
Messages
Total messages: 62 (27 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Description was changed from ========== SW metrix BUG= ========== to ========== Add NavigationToFirstContentfulPaint UMA for ServiceWorker controlled pages. We will do some experiment which may affect the performance on Service Worker controlled pages. Currently we don't have performance UMAs for Service Worker controlled pages. This change will add PageLoad.Timing2.NavigationToFirstContentfulPaint.ServiceWorker. BUG=581613 ==========
Description was changed from ========== Add NavigationToFirstContentfulPaint UMA for ServiceWorker controlled pages. We will do some experiment which may affect the performance on Service Worker controlled pages. Currently we don't have performance UMAs for Service Worker controlled pages. This change will add PageLoad.Timing2.NavigationToFirstContentfulPaint.ServiceWorker. BUG=581613 ========== to ========== Add NavigationToFirstContentfulPaint UMA for ServiceWorker controlled pages. We will do some experiments which may affect the performance on Service Worker controlled pages. Currently we don't have performance UMAs for Service Worker controlled pages. This change will add PageLoad.Timing2.NavigationToFirstContentfulPaint.ServiceWorker. BUG=581613 ==========
Description was changed from ========== Add NavigationToFirstContentfulPaint UMA for ServiceWorker controlled pages. We will do some experiments which may affect the performance on Service Worker controlled pages. Currently we don't have performance UMAs for Service Worker controlled pages. This change will add PageLoad.Timing2.NavigationToFirstContentfulPaint.ServiceWorker. BUG=581613 ========== to ========== Add NavigationToFirstContentfulPaint UMA for ServiceWorker controlled pages. We will do some experiments which may affect the performance of Service Worker controlled pages. Currently we don't have performance UMAs for Service Worker controlled pages. This change will add PageLoad.Timing2.NavigationToFirstContentfulPaint.ServiceWorker. BUG=581613 ==========
Description was changed from ========== Add NavigationToFirstContentfulPaint UMA for ServiceWorker controlled pages. We will do some experiments which may affect the performance of Service Worker controlled pages. Currently we don't have performance UMAs for Service Worker controlled pages. This change will add PageLoad.Timing2.NavigationToFirstContentfulPaint.ServiceWorker. BUG=581613 ========== to ========== Add NavigationToFirstContentfulPaint UMA for ServiceWorker controlled pages. We will do some experiments which may affect the performance of Service Worker controlled pages. Currently we don't have PageLoad UMAs for Service Worker controlled pages. This change will add PageLoad.Timing2.NavigationToFirstContentfulPaint.ServiceWorker. BUG=581613 ==========
Description was changed from ========== Add NavigationToFirstContentfulPaint UMA for ServiceWorker controlled pages. We will do some experiments which may affect the performance of Service Worker controlled pages. Currently we don't have PageLoad UMAs for Service Worker controlled pages. This change will add PageLoad.Timing2.NavigationToFirstContentfulPaint.ServiceWorker. BUG=581613 ========== to ========== Add NavigationToFirstContentfulPaint UMA for ServiceWorker controlled pages. Currently we don't have PageLoad UMAs for Service Worker controlled pages. We will do some experiments which may affect the performance of Service Worker controlled pages. This change will add PageLoad.Timing2.NavigationToFirstContentfulPaint.ServiceWorker. BUG=581613 ==========
horo@chromium.org changed reviewers: + ksakamoto@chromium.org, nhiroki@chromium.org
ksakamoto@ Could you please review this?
ksakamoto@chromium.org changed reviewers: + bmcquade@chromium.org, csharrison@chromium.org
+csharrison, bmcquade I'm not sure if it's OK to add this metric to CorePageLoadMetricsObserver, or you should create a new observer. Charles / Bryan would be able to give you some advice. https://codereview.chromium.org/1871393002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/timing/PerformanceTiming.h (right): https://codereview.chromium.org/1871393002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceTiming.h:63: unsigned long long workerStart() const; Can you add a comment saying that this is a Navigation Timing Level 2's attribute and not exposed to JS currently.
Patchset #2 (id:80002) has been deleted
Patchset #2 (id:110001) has been deleted
Thank you! https://codereview.chromium.org/1871393002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/timing/PerformanceTiming.h (right): https://codereview.chromium.org/1871393002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceTiming.h:63: unsigned long long workerStart() const; On 2016/04/12 08:23:12, Kunihiko Sakamoto wrote: > Can you add a comment saying that this is a Navigation Timing Level 2's > attribute and not exposed to JS currently. Done.
Thanks! This seems like an important addition to our metrics. Given that you are really trying to send a flag indicating whether the page being loaded used a service worker, I think the new WebLoadingBehaviorFlag may be a better match than using a base::Time. We do send a base::Time for navigation_start but I think we want to get rid of that if we can, and use only TimeDeltas in the PageLoadTiming struct. WebLoadingBehaviorFlag seems like a better match here. See what you think. https://codereview.chromium.org/1871393002/diff/130001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1871393002/diff/130001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:100: const char kServiceWorkerHistogramFirstContentfulPaint[] = I wonder if this should instead be under PageLoad.Clients.ServiceWorker.* Take a look at https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pag... If we only anticipate ever adding a single service worker specific metric, then maybe this is ok. But my hunch is we may want to measure various SW metrics, in which case having a separate observer and client makes more sense. https://codereview.chromium.org/1871393002/diff/130001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:288: if (!timing.worker_start.is_null()) { I'm disinclined to use absolute times in our metrics. It sounds like your goal is to communicate whether or not a worker was used for a page. For that, you may want to use the new WebLoadingBehaviorFlag to annotate page load metrics with whether a SW was active for that page on the blink side. You can see how that works here: https://codereview.chromium.org/1846143003 and in this pending change for the metrics side: https://codereview.chromium.org/1857443002 You may want to email Kinuko and Charles to see which they think makes sense. My hunch is that's better than sending a base::Time which is really only intended to be used as a flag to indicate that a worker was used for a page load.
Description was changed from ========== Add NavigationToFirstContentfulPaint UMA for ServiceWorker controlled pages. Currently we don't have PageLoad UMAs for Service Worker controlled pages. We will do some experiments which may affect the performance of Service Worker controlled pages. This change will add PageLoad.Timing2.NavigationToFirstContentfulPaint.ServiceWorker. BUG=581613 ========== to ========== Add NavigationToFirstContentfulPaint UMA for ServiceWorker controlled pages. Currently we don't have PageLoad UMAs for Service Worker controlled pages. We will do some experiments which may affect the performance of Service Worker controlled pages. This change will add PageLoad.Clients.ServiceWorker.Timing2.NavigationToFirstContentfulPaint BUG=581613 ==========
horo@chromium.org changed reviewers: + kinuko@chromium.org
+kinuko, csharrison Using WebLoadingBehaviorFlag seems good for my purpose I uploaded patch set 3 which is using WebLoadingBehaviorFlag.
Yeah, using WebLoadingBehaviorFlag here sgtm, and having UMA for SW controlled page looks great. https://codereview.chromium.org/1871393002/diff/150001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebLoadingBehaviorFlag.h (right): https://codereview.chromium.org/1871393002/diff/150001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebLoadingBehaviorFlag.h:18: WebLoadingBehaviorServiceWorkerControlled = 1 << 1, nit: in general we'd better have a short comment for each in public/ (though we only have obvious ones so far)
Nice change! https://codereview.chromium.org/1871393002/diff/150001/components/page_load_m... File components/page_load_metrics/renderer/metrics_render_frame_observer.cc (right): https://codereview.chromium.org/1871393002/diff/150001/components/page_load_m... components/page_load_metrics/renderer/metrics_render_frame_observer.cc:44: if (page_timing_metrics_sender_) This worries me slightly. We try to scope page_timing_metrics_sender_ to a committed page, so that we never send IPC for a provisional load. If you are sometimes calling DidObserveLoadingBehavior before the commit point (from contents point of view), then won't you miss this window occasionally and not update the behavior? Looking at the code now, it seems like you're instrumenting after content should know we've commit. Still, can you add a comment explaining when the sender_ will be invalid?
https://codereview.chromium.org/1871393002/diff/150001/components/page_load_m... File components/page_load_metrics/renderer/metrics_render_frame_observer.cc (right): https://codereview.chromium.org/1871393002/diff/150001/components/page_load_m... components/page_load_metrics/renderer/metrics_render_frame_observer.cc:44: if (page_timing_metrics_sender_) On 2016/04/13 12:59:13, csharrison wrote: > This worries me slightly. We try to scope page_timing_metrics_sender_ to a > committed page, so that we never send IPC for a provisional load. > > If you are sometimes calling DidObserveLoadingBehavior before the commit point > (from contents point of view), then won't you miss this window occasionally and > not update the behavior? > > Looking at the code now, it seems like you're instrumenting after content should > know we've commit. Still, can you add a comment explaining when the sender_ will > be invalid? DidCommitProvisionalLoad() doesn't create page_timing_metrics_sender_ if ShouldSendMetrics() returns false. For example, when the mime type isn't HTML mime type, page_timing_metrics_sender_ is null when DidObserveLoadingBehavior() is called. So I think we should check ShouldSendMetrics() or page_timing_metrics_sender_ here. But as you say, checking only page_timing_metrics_sender_ may cause IPCs for a provisional load. So I changed to check ShouldSendMetrics() here. https://codereview.chromium.org/1871393002/diff/150001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebLoadingBehaviorFlag.h (right): https://codereview.chromium.org/1871393002/diff/150001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebLoadingBehaviorFlag.h:18: WebLoadingBehaviorServiceWorkerControlled = 1 << 1, On 2016/04/13 08:39:28, kinuko wrote: > nit: in general we'd better have a short comment for each in public/ (though we > only have obvious ones so far) Done.
https://codereview.chromium.org/1871393002/diff/170001/components/page_load_m... File components/page_load_metrics/renderer/metrics_render_frame_observer.cc (right): https://codereview.chromium.org/1871393002/diff/170001/components/page_load_m... components/page_load_metrics/renderer/metrics_render_frame_observer.cc:44: if (!ShouldSendMetrics()) Actually, I think I retract my comment. This won't work for an in-page navigation. Additionally, the logic in DidChangePerformanceTiming looks at !page_timing_metrics_sender_, so perhaps it's best to do the conservative check for consistency. I'm adding this check because clusterfuzz found the bug in my CL.
https://codereview.chromium.org/1871393002/diff/170001/components/page_load_m... File components/page_load_metrics/renderer/metrics_render_frame_observer.cc (right): https://codereview.chromium.org/1871393002/diff/170001/components/page_load_m... components/page_load_metrics/renderer/metrics_render_frame_observer.cc:44: if (!ShouldSendMetrics()) On 2016/04/14 13:29:56, csharrison wrote: > Actually, I think I retract my comment. This won't work for an in-page > navigation. > > Additionally, the logic in DidChangePerformanceTiming looks at > !page_timing_metrics_sender_, so perhaps it's best to do the conservative check > for consistency. I'm adding this check because clusterfuzz found the bug in my > CL. Changed to check page_timing_metrics_sender_
nice change! https://codereview.chromium.org/1871393002/diff/190001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/service_worker_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1871393002/diff/190001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/service_worker_page_load_metrics_observer.cc:29: LogDocumentWriteEvaluatorData(timing, info); rename to LogServiceWorkerHistograms https://codereview.chromium.org/1871393002/diff/190001/chrome/chrome_browser.... File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1871393002/diff/190001/chrome/chrome_browser.... chrome/chrome_browser.gypi:422: 'browser/page_load_metrics/observers/service_worker_page_load_metrics_observer.cc', should go before stale_while... to retain abc order https://codereview.chromium.org/1871393002/diff/190001/components/page_load_m... File components/page_load_metrics/renderer/metrics_render_frame_observer.cc (right): https://codereview.chromium.org/1871393002/diff/190001/components/page_load_m... components/page_load_metrics/renderer/metrics_render_frame_observer.cc:44: if (!page_timing_metrics_sender_) charles just landed a patch to fix this so you can omit it once you sync. https://codereview.chromium.org/1871393002/diff/190001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/1871393002/diff/190001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:432: if (client()->isControlledByServiceWorker(*m_documentLoader)) i guess it's somewhat subtle, but this has to happen after commit in order for our metrics tracking logic to handle it properly. looks like you've got that covered but just want to make sure you are aware. https://codereview.chromium.org/1871393002/diff/190001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebLoadingBehaviorFlag.h (right): https://codereview.chromium.org/1871393002/diff/190001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebLoadingBehaviorFlag.h:17: WebLoadingBehaviorDocumentWriteEvaluator = 1 << 0, while here, can you add the comment // Indicates that the page used the document.write evaluator to preload scan for resources inserted via document.write.
Thank you. https://codereview.chromium.org/1871393002/diff/190001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/service_worker_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1871393002/diff/190001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/service_worker_page_load_metrics_observer.cc:29: LogDocumentWriteEvaluatorData(timing, info); On 2016/04/14 15:10:26, Bryan McQuade wrote: > rename to LogServiceWorkerHistograms Done. https://codereview.chromium.org/1871393002/diff/190001/chrome/chrome_browser.... File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1871393002/diff/190001/chrome/chrome_browser.... chrome/chrome_browser.gypi:422: 'browser/page_load_metrics/observers/service_worker_page_load_metrics_observer.cc', On 2016/04/14 15:10:26, Bryan McQuade wrote: > should go before stale_while... to retain abc order Done. https://codereview.chromium.org/1871393002/diff/190001/components/page_load_m... File components/page_load_metrics/renderer/metrics_render_frame_observer.cc (right): https://codereview.chromium.org/1871393002/diff/190001/components/page_load_m... components/page_load_metrics/renderer/metrics_render_frame_observer.cc:44: if (!page_timing_metrics_sender_) On 2016/04/14 15:10:26, Bryan McQuade wrote: > charles just landed a patch to fix this so you can omit it once you sync. I see. Synced. https://codereview.chromium.org/1871393002/diff/190001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/1871393002/diff/190001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:432: if (client()->isControlledByServiceWorker(*m_documentLoader)) On 2016/04/14 15:10:26, Bryan McQuade wrote: > i guess it's somewhat subtle, but this has to happen after commit in order for > our metrics tracking logic to handle it properly. looks like you've got that > covered but just want to make sure you are aware. Added comment. https://codereview.chromium.org/1871393002/diff/190001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebLoadingBehaviorFlag.h (right): https://codereview.chromium.org/1871393002/diff/190001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebLoadingBehaviorFlag.h:17: WebLoadingBehaviorDocumentWriteEvaluator = 1 << 0, On 2016/04/14 15:10:26, Bryan McQuade wrote: > while here, can you add the comment > // Indicates that the page used the document.write evaluator to preload scan for > resources inserted via document.write. Done.
Ping?
Looks good. https://codereview.chromium.org/1871393002/diff/210001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/service_worker_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1871393002/diff/210001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/service_worker_page_load_metrics_observer.cc:5: #include "chrome/browser/page_load_metrics/observers/service_worker_page_load_metrics_observer.h" nit: Add a linebreak here. https://codereview.chromium.org/1871393002/diff/210001/components/page_load_m... File components/page_load_metrics/renderer/metrics_render_frame_observer.cc (right): https://codereview.chromium.org/1871393002/diff/210001/components/page_load_m... components/page_load_metrics/renderer/metrics_render_frame_observer.cc:44: if (!ShouldSendMetrics()) I don't think this is necessary. For in-page navigations, ShouldSendMetrics can return true but we won't have any sender_.
https://codereview.chromium.org/1871393002/diff/210001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/service_worker_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1871393002/diff/210001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/service_worker_page_load_metrics_observer.cc:5: #include "chrome/browser/page_load_metrics/observers/service_worker_page_load_metrics_observer.h" On 2016/04/19 13:33:38, csharrison wrote: > nit: Add a linebreak here. Done. https://codereview.chromium.org/1871393002/diff/210001/components/page_load_m... File components/page_load_metrics/renderer/metrics_render_frame_observer.cc (right): https://codereview.chromium.org/1871393002/diff/210001/components/page_load_m... components/page_load_metrics/renderer/metrics_render_frame_observer.cc:44: if (!ShouldSendMetrics()) On 2016/04/19 13:33:38, csharrison wrote: > I don't think this is necessary. For in-page navigations, ShouldSendMetrics can > return true but we won't have any sender_. Done.
Ping?
page_load_metrics lgtm, thanks for adding this.
horo@chromium.org changed reviewers: + isherman@chromium.org, japhet@chromium.org
japhet@ Could you please review third_party/WebKit/Source/core/loader/FrameLoader.cpp and third_party/WebKit/public/platform/WebLoadingBehaviorFlag.h? isherman@ Could you please review tools/metrics/histograms/histograms.xml?
histograms lgtm
WebKit changes lgtm (unless Nate disagrees)
https://codereview.chromium.org/1871393002/diff/230001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/1871393002/diff/230001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:432: // didObserveLoadingBehavior() must be called after dispatchDidCommitLoad() is called for the metrics tracking logic to handle it properly. nit: could we / do we have assertion somewhere to make sure this?
lgtm
Thank you! https://codereview.chromium.org/1871393002/diff/230001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/1871393002/diff/230001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:432: // didObserveLoadingBehavior() must be called after dispatchDidCommitLoad() is called for the metrics tracking logic to handle it properly. On 2016/04/26 13:15:43, kinuko wrote: > nit: could we / do we have assertion somewhere to make sure this? DocumentLoader::didObserveLoadingBehavior() has "ASSERT(m_state >= Committed)" check. https://chromium.googlesource.com/chromium/src/+/b4d6bf98e54db861b8d9adc7642c...
The CQ bit was checked by horo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1871393002/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1871393002/230001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by horo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1871393002/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1871393002/230001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by horo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1871393002/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1871393002/230001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by horo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1871393002/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1871393002/230001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by horo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1871393002/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1871393002/230001
Message was sent while issue was closed.
Description was changed from ========== Add NavigationToFirstContentfulPaint UMA for ServiceWorker controlled pages. Currently we don't have PageLoad UMAs for Service Worker controlled pages. We will do some experiments which may affect the performance of Service Worker controlled pages. This change will add PageLoad.Clients.ServiceWorker.Timing2.NavigationToFirstContentfulPaint BUG=581613 ========== to ========== Add NavigationToFirstContentfulPaint UMA for ServiceWorker controlled pages. Currently we don't have PageLoad UMAs for Service Worker controlled pages. We will do some experiments which may affect the performance of Service Worker controlled pages. This change will add PageLoad.Clients.ServiceWorker.Timing2.NavigationToFirstContentfulPaint BUG=581613 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:230001)
Message was sent while issue was closed.
Description was changed from ========== Add NavigationToFirstContentfulPaint UMA for ServiceWorker controlled pages. Currently we don't have PageLoad UMAs for Service Worker controlled pages. We will do some experiments which may affect the performance of Service Worker controlled pages. This change will add PageLoad.Clients.ServiceWorker.Timing2.NavigationToFirstContentfulPaint BUG=581613 ========== to ========== Add NavigationToFirstContentfulPaint UMA for ServiceWorker controlled pages. Currently we don't have PageLoad UMAs for Service Worker controlled pages. We will do some experiments which may affect the performance of Service Worker controlled pages. This change will add PageLoad.Clients.ServiceWorker.Timing2.NavigationToFirstContentfulPaint BUG=581613 Committed: https://crrev.com/e3107a132dcd87e9310627ccb3ea15fea8bd16a1 Cr-Commit-Position: refs/heads/master@{#390001} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/e3107a132dcd87e9310627ccb3ea15fea8bd16a1 Cr-Commit-Position: refs/heads/master@{#390001}
Message was sent while issue was closed.
https://codereview.chromium.org/1871393002/diff/230001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/1871393002/diff/230001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:432: // didObserveLoadingBehavior() must be called after dispatchDidCommitLoad() is called for the metrics tracking logic to handle it properly. On 2016/04/27 02:30:15, horo wrote: > On 2016/04/26 13:15:43, kinuko wrote: > > nit: could we / do we have assertion somewhere to make sure this? > > DocumentLoader::didObserveLoadingBehavior() has "ASSERT(m_state >= Committed)" > check. > > https://chromium.googlesource.com/chromium/src/+/b4d6bf98e54db861b8d9adc7642c... I know, but we don't take the document loader path this case do we? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
