Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1918373002:
Metrics for document.write script blocking (Closed)
|
Created:
4 years, 7 months ago by shivanisha Modified:
4 years, 7 months ago CC:
asvitkine+watch_chromium.org, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, csharrison+watch_chromium.org, dglazkov+blink, gavinp+loader_chromium.org, Nate Chapin, loading-reviews+metrics_chromium.org, loading-reviews_chromium.org, tyoshino+watch_chromium.org, Yoav Weiss Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis CL logs the metrics to see the impact of document.write script blocking
feature. The metrics are specifically for pages that have a blockable
document.written script.
The CL also has a metric to count the number of reloads for such pages to
check if there is any increase in number of reloads possibly implying
page breaks.
BUG=603152
Committed: https://crrev.com/e6340ed747c7d4dbeedff9c963b6bd27825dd03d
Cr-Commit-Position: refs/heads/master@{#391526}
Patch Set 1 #
Total comments: 28
Patch Set 2 : Feedback incorporated. #
Total comments: 13
Patch Set 3 : Rebased and feedback incorporated. #Patch Set 4 : rebased and feedback incorporated. #
Total comments: 10
Patch Set 5 : Line length and comment feedback incorporated. #
Total comments: 4
Patch Set 6 : Tests added and flags corrected. #
Total comments: 8
Patch Set 7 : Adding browser tests and feedback incorporated. #Patch Set 8 : Added browser tests and feedback incorporated. #
Total comments: 1
Patch Set 9 : Bryan's comment: Returning disallowFetch from shouldDisallowFetchForMainFrameScript #Messages
Total messages: 46 (5 generated)
shivanisha@chromium.org changed reviewers: + bmcquade@chromium.org, csharrison@chromium.org, japhet@chromium.org, jkarlin@chromium.org
Don't we also want to measure the TTFCP?
On 2016/04/26 at 18:40:26, jkarlin wrote: > Don't we also want to measure the TTFCP? Measured using the following: PageLoad.Clients.DocWrite.Block.Timing2.DOMLoadingToFirstContentfulPaint PageLoad.Clients.DocWrite.Block.Timing2.ParseStartToFirstContentfulPaint Do you mean some other histogram apart from these? I did not include NavigationToFirstContentfulPaint since the impact of blocking document.written script will only be after parsing starts.
On 2016/04/26 18:58:01, shivanisha wrote: > On 2016/04/26 at 18:40:26, jkarlin wrote: > > Don't we also want to measure the TTFCP? > > Measured using the following: > PageLoad.Clients.DocWrite.Block.Timing2.DOMLoadingToFirstContentfulPaint > PageLoad.Clients.DocWrite.Block.Timing2.ParseStartToFirstContentfulPaint > Do you mean some other histogram apart from these? > I did not include NavigationToFirstContentfulPaint since the impact of blocking > document.written script will only be after parsing starts. Ah, I was looking at histograms.xml and didn't see it there, but it's part of the suffix. Never mind :)
Looks very good, thanks! A few small comments. https://codereview.chromium.org/1918373002/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1918373002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:47: "DOMLoadingToDOMContentLoadedEventFired"; we're moving away from DOMLoadingToDOMContentLoadedEventFired to ParseDuration instead. let's remove DOMLoadingToDOMContentLoadedEventFired which should be essentially the same as ParseDuration. I'll put together a CL to remove DOMLoading from our code soon. https://codereview.chromium.org/1918373002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:49: "PageLoad.Clients.DocWrite.Block.Timing2.DOMLoadingToFirstContentfulPaint"; same - let's prefer ParseStartToFirstContentfulPaint and remove this one https://codereview.chromium.org/1918373002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:75: "Background"; looks like this can be pulled up onto the previous line https://codereview.chromium.org/1918373002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1918373002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:34602: +<histogram name="PageLoad.Clients.DocWrite.Block.Reloads"> maybe ReloadCount rather than Reloads to be explicit that it's a counter?
Looks good here as well. Some comments. https://codereview.chromium.org/1918373002/diff/1/components/page_load_metric... File components/page_load_metrics/browser/page_load_metrics_util.cc (right): https://codereview.chromium.org/1918373002/diff/1/components/page_load_metric... components/page_load_metrics/browser/page_load_metrics_util.cc:35: WasStartedInForegroundEventInForeground(parse_stop, info)); No need for extra parens https://codereview.chromium.org/1918373002/diff/1/components/page_load_metric... File components/page_load_metrics/browser/page_load_metrics_util.h (right): https://codereview.chromium.org/1918373002/diff/1/components/page_load_metric... components/page_load_metrics/browser/page_load_metrics_util.h:31: bool ParseInForeground(const base::TimeDelta& parse_start, ParseInForeground sounds like a command but this is a question. Instead use WasParseInForeground. https://codereview.chromium.org/1918373002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1918373002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:111: const bool isReload = (loadType == FrameLoadTypeReload || loadType == FrameLoadTypeReloadBypassingCache || loadType == FrameLoadTypeSame); unnecessary parens https://codereview.chromium.org/1918373002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:113: // Recording this metric since increase in number of reloads for pages s/since increase/since an increase/ https://codereview.chromium.org/1918373002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1918373002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:34606: + Reloads in pages that have blockable document.written scripts. When is a script blockable? https://codereview.chromium.org/1918373002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:91796: + the feature is enabled."/> If what feature is enabled?
https://codereview.chromium.org/1918373002/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1918373002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:71: "Background"; Where are any of the .Background metrics recorded? https://codereview.chromium.org/1918373002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:187: if (WasStartedInForegroundEventInForeground(timing.first_contentful_paint, Did you mean for this extra condition? It looks the same as above.
https://codereview.chromium.org/1918373002/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1918373002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:71: "Background"; On 2016/04/26 at 20:29:11, jkarlin wrote: > Where are any of the .Background metrics recorded? They are recorded in the same function as foreground metrics : DocumentWritePageLoadMetricsObserver::LogDocumentWriteBlockData when WasStartedInForegroundEventInForeground or ParseInForeground return false. https://codereview.chromium.org/1918373002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:187: if (WasStartedInForegroundEventInForeground(timing.first_contentful_paint, On 2016/04/26 at 20:29:11, jkarlin wrote: > Did you mean for this extra condition? It looks the same as above. This is definitely a typing error. Thanks for catching this.
One comment re: the .Background histograms. https://codereview.chromium.org/1918373002/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1918373002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:71: "Background"; On 2016/04/26 at 20:40:13, shivanisha wrote: > On 2016/04/26 at 20:29:11, jkarlin wrote: > > Where are any of the .Background metrics recorded? > > They are recorded in the same function as foreground metrics : DocumentWritePageLoadMetricsObserver::LogDocumentWriteBlockData > when WasStartedInForegroundEventInForeground or ParseInForeground return false. We may see some pages that would have been backgrounded before an event to move into the foreground bucket as a result of our work to speed up pages, so I thought it'd be useful to log both fg and bg to see this effect directly (we may expect to see a reduction in number of background events). It's a lot of histograms and I'm mixed, but in general I think it's better to log more data than to be wondering what a histogram might have looked like and wishing you'd added it a month earlier.
https://codereview.chromium.org/1918373002/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1918373002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:47: "DOMLoadingToDOMContentLoadedEventFired"; On 2016/04/26 at 19:35:44, Bryan McQuade wrote: > we're moving away from DOMLoadingToDOMContentLoadedEventFired to ParseDuration instead. let's remove DOMLoadingToDOMContentLoadedEventFired which should be essentially the same as ParseDuration. > > I'll put together a CL to remove DOMLoading from our code soon. done. https://codereview.chromium.org/1918373002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:49: "PageLoad.Clients.DocWrite.Block.Timing2.DOMLoadingToFirstContentfulPaint"; On 2016/04/26 at 19:35:44, Bryan McQuade wrote: > same - let's prefer ParseStartToFirstContentfulPaint and remove this one done. https://codereview.chromium.org/1918373002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:75: "Background"; On 2016/04/26 at 19:35:44, Bryan McQuade wrote: > looks like this can be pulled up onto the previous line done. https://codereview.chromium.org/1918373002/diff/1/components/page_load_metric... File components/page_load_metrics/browser/page_load_metrics_util.cc (right): https://codereview.chromium.org/1918373002/diff/1/components/page_load_metric... components/page_load_metrics/browser/page_load_metrics_util.cc:35: WasStartedInForegroundEventInForeground(parse_stop, info)); On 2016/04/26 at 20:16:21, jkarlin wrote: > No need for extra parens removed extra parens. https://codereview.chromium.org/1918373002/diff/1/components/page_load_metric... File components/page_load_metrics/browser/page_load_metrics_util.h (right): https://codereview.chromium.org/1918373002/diff/1/components/page_load_metric... components/page_load_metrics/browser/page_load_metrics_util.h:31: bool ParseInForeground(const base::TimeDelta& parse_start, On 2016/04/26 at 20:16:21, jkarlin wrote: > ParseInForeground sounds like a command but this is a question. Instead use WasParseInForeground. done. https://codereview.chromium.org/1918373002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1918373002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:111: const bool isReload = (loadType == FrameLoadTypeReload || loadType == FrameLoadTypeReloadBypassingCache || loadType == FrameLoadTypeSame); On 2016/04/26 at 20:16:21, jkarlin wrote: > unnecessary parens removed. https://codereview.chromium.org/1918373002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:113: // Recording this metric since increase in number of reloads for pages On 2016/04/26 at 20:16:21, jkarlin wrote: > s/since increase/since an increase/ done. https://codereview.chromium.org/1918373002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1918373002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:34602: +<histogram name="PageLoad.Clients.DocWrite.Block.Reloads"> On 2016/04/26 at 19:35:44, Bryan McQuade wrote: > maybe ReloadCount rather than Reloads to be explicit that it's a counter? done. https://codereview.chromium.org/1918373002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:34606: + Reloads in pages that have blockable document.written scripts. On 2016/04/26 at 20:16:21, jkarlin wrote: > When is a script blockable? Updated to mention cross-origin and synchronous. https://codereview.chromium.org/1918373002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:91796: + the feature is enabled."/> On 2016/04/26 at 20:16:21, jkarlin wrote: > If what feature is enabled? Updated to mention "document.write script blocking feature".
A few more comments https://codereview.chromium.org/1918373002/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1918373002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:71: "Background"; On 2016/04/26 20:40:13, shivanisha wrote: > On 2016/04/26 at 20:29:11, jkarlin wrote: > > Where are any of the .Background metrics recorded? > > They are recorded in the same function as foreground metrics : > DocumentWritePageLoadMetricsObserver::LogDocumentWriteBlockData > when WasStartedInForegroundEventInForeground or ParseInForeground return false. My bad. I was searching for ParseBlockedOnScriptLoadBackground but Background is at the beginning of the const name instead of the end. End of a long day :) These const strings are odd for UMA. We don't typically do this. What's the motivation? https://codereview.chromium.org/1918373002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:71: "Background"; On 2016/04/26 20:50:14, Bryan McQuade wrote: > On 2016/04/26 at 20:40:13, shivanisha wrote: > > On 2016/04/26 at 20:29:11, jkarlin wrote: > > > Where are any of the .Background metrics recorded? > > > > They are recorded in the same function as foreground metrics : > DocumentWritePageLoadMetricsObserver::LogDocumentWriteBlockData > > when WasStartedInForegroundEventInForeground or ParseInForeground return > false. > > We may see some pages that would have been backgrounded before an event to move > into the foreground bucket as a result of our work to speed up pages, so I > thought it'd be useful to log both fg and bg to see this effect directly (we may > expect to see a reduction in number of background events). It's a lot of > histograms and I'm mixed, but in general I think it's better to log more data > than to be wondering what a histogram might have looked like and wishing you'd > added it a month earlier. Acknowledged. https://codereview.chromium.org/1918373002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1918373002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:100: WebLoadingBehaviorDocumentWriteBlockReload) { DCHECK(!info.metadata.behavior_flags & blink::WebLoadingBehaviorFlag::WebLoadingBehaviorDocumentWriteBlock) to verify that on reload we don't block stuff. https://codereview.chromium.org/1918373002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:168: if (!timing.parse_start.is_zero()) { Rather than wrap the whole thing in this condition, prefer: if (timing.parse_start.is_zero()) return; https://codereview.chromium.org/1918373002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:194: if (!timing.parse_stop.is_zero()) { Could early return here if parsing hasn't completed, up to you. https://codereview.chromium.org/1918373002/diff/20001/components/page_load_me... File components/page_load_metrics/browser/page_load_metrics_util.cc (right): https://codereview.chromium.org/1918373002/diff/20001/components/page_load_me... components/page_load_metrics/browser/page_load_metrics_util.cc:29: // foreground, record a foreground histogram. This comment belongs in the header. https://codereview.chromium.org/1918373002/diff/20001/components/page_load_me... File components/page_load_metrics/browser/page_load_metrics_util.h (right): https://codereview.chromium.org/1918373002/diff/20001/components/page_load_me... components/page_load_metrics/browser/page_load_metrics_util.h:31: bool WasParseInForeground(const base::TimeDelta& parse_start, This methods needs documentation
Basically looks good, thanks! 2 small changes. https://codereview.chromium.org/1918373002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1918373002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:62: "PageLoad.Clients.DocWrite.Block.Reloads"; let's update this to ReloadCount as well https://codereview.chromium.org/1918373002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1918373002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:107: // Do not block scripts if it is a page reload. This is to enable pages to looks like you may need to rebase now that this logic is committed
just giving some context on const char histogram names. https://codereview.chromium.org/1918373002/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1918373002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:71: "Background"; On 2016/04/27 at 14:28:16, jkarlin wrote: > On 2016/04/26 20:40:13, shivanisha wrote: > > On 2016/04/26 at 20:29:11, jkarlin wrote: > > > Where are any of the .Background metrics recorded? > > > > They are recorded in the same function as foreground metrics : > > DocumentWritePageLoadMetricsObserver::LogDocumentWriteBlockData > > when WasStartedInForegroundEventInForeground or ParseInForeground return false. > > My bad. I was searching for ParseBlockedOnScriptLoadBackground but Background is at the beginning of the const name instead of the end. End of a long day :) > > These const strings are odd for UMA. We don't typically do this. What's the motivation? This has been the convention in page load metrics. It allows for avoiding duplication of string constants between implementation in tests. We've taken the approach of doing this for all histograms, rather than just the ones exposed to tests, to be consistent and keep all name declarations in one place.
https://codereview.chromium.org/1918373002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1918373002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:62: "PageLoad.Clients.DocWrite.Block.Reloads"; On 2016/04/27 at 14:29:24, Bryan McQuade wrote: > let's update this to ReloadCount as well My bad. Sure. Updated. https://codereview.chromium.org/1918373002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:100: WebLoadingBehaviorDocumentWriteBlockReload) { On 2016/04/27 at 14:28:16, jkarlin wrote: > DCHECK(!info.metadata.behavior_flags & blink::WebLoadingBehaviorFlag::WebLoadingBehaviorDocumentWriteBlock) to verify that on reload we don't block stuff. Done. https://codereview.chromium.org/1918373002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:168: if (!timing.parse_start.is_zero()) { On 2016/04/27 at 14:28:16, jkarlin wrote: > Rather than wrap the whole thing in this condition, prefer: > > if (timing.parse_start.is_zero()) > return; Good point. done. https://codereview.chromium.org/1918373002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:194: if (!timing.parse_stop.is_zero()) { On 2016/04/27 at 14:28:16, jkarlin wrote: > Could early return here if parsing hasn't completed, up to you. done https://codereview.chromium.org/1918373002/diff/20001/components/page_load_me... File components/page_load_metrics/browser/page_load_metrics_util.cc (right): https://codereview.chromium.org/1918373002/diff/20001/components/page_load_me... components/page_load_metrics/browser/page_load_metrics_util.cc:29: // foreground, record a foreground histogram. On 2016/04/27 at 14:28:16, jkarlin wrote: > This comment belongs in the header. Moved. https://codereview.chromium.org/1918373002/diff/20001/components/page_load_me... File components/page_load_metrics/browser/page_load_metrics_util.h (right): https://codereview.chromium.org/1918373002/diff/20001/components/page_load_me... components/page_load_metrics/browser/page_load_metrics_util.h:31: bool WasParseInForeground(const base::TimeDelta& parse_start, On 2016/04/27 at 14:28:17, jkarlin wrote: > This methods needs documentation Added.
LGTM
https://codereview.chromium.org/1918373002/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1918373002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:170: // The metrics are dependent on parsing to have started. Comment not needed, code is self explanatory. https://codereview.chromium.org/1918373002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:194: kBackgroundHistogramDocWriteBlockParseBlockedOnScriptLoadDocumentWrite, There is a hard limit of 80 characters, please rename (as well for other lines below that exceed 80 chars). https://codereview.chromium.org/1918373002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:198: // The remaining metrics are dependent on parse to be completed. No need for this comment https://codereview.chromium.org/1918373002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:212: kHistogramDocWriteBlockParseBlockedOnScriptLoadDocumentWriteParseComplete, Line over 80 chars https://codereview.chromium.org/1918373002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:223: kBackgroundHistogramDocWriteBlockParseBlockedOnScriptLoadDocWriteComplete, Line over 80 chars
https://codereview.chromium.org/1918373002/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1918373002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:170: // The metrics are dependent on parsing to have started. On 2016/04/27 at 18:41:31, jkarlin wrote: > Comment not needed, code is self explanatory. removed. https://codereview.chromium.org/1918373002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:194: kBackgroundHistogramDocWriteBlockParseBlockedOnScriptLoadDocumentWrite, On 2016/04/27 at 18:41:30, jkarlin wrote: > There is a hard limit of 80 characters, please rename (as well for other lines below that exceed 80 chars). done. I somehow thought git cl format will always result in lines <= 80 chars by indenting them less :) https://codereview.chromium.org/1918373002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:198: // The remaining metrics are dependent on parse to be completed. On 2016/04/27 at 18:41:30, jkarlin wrote: > No need for this comment removed. https://codereview.chromium.org/1918373002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:212: kHistogramDocWriteBlockParseBlockedOnScriptLoadDocumentWriteParseComplete, On 2016/04/27 at 18:41:30, jkarlin wrote: > Line over 80 chars renamed https://codereview.chromium.org/1918373002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:223: kBackgroundHistogramDocWriteBlockParseBlockedOnScriptLoadDocWriteComplete, On 2016/04/27 at 18:41:31, jkarlin wrote: > Line over 80 chars renamed.
lgtm
shivanisha@chromium.org changed reviewers: + holte@chromium.org
On 2016/04/29 at 12:54:24, jkarlin wrote: > lgtm Charles, PTAL (chrome/browser/page_load_metrics/* and components/page_load_metrics/*) Nate, PTAL (third_party/WebKit/*) Steven, PTAL (histograms.xml) Thanks !
Looks good! https://codereview.chromium.org/1918373002/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/1918373002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer_unittest.cc:118: } Can you add a test that doesn't log any histogram (i.e. assert that the behavior flag is required)? https://codereview.chromium.org/1918373002/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebLoadingBehaviorFlag.h (right): https://codereview.chromium.org/1918373002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebLoadingBehaviorFlag.h:23: WebLoadingBehaviorDocumentWriteBlock = 1 << 1, Looks like you are clobbering WebLoadingBehaviorServiceWorkerControlled (which should probably have a test to ensure this never happens).
https://codereview.chromium.org/1918373002/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/1918373002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer_unittest.cc:118: } On 2016/04/29 at 13:37:53, csharrison wrote: > Can you add a test that doesn't log any histogram (i.e. assert that the behavior flag is required)? I overloaded the existing test NoPossiblePreload to check that, by adding the condition in AssertNoHistogramsLogged. I think it will be better to create two separate assert functions AssertNoPreloadHistogramsLogged and AssertNoBlockHistogramsLogged. Will do that. https://codereview.chromium.org/1918373002/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebLoadingBehaviorFlag.h (right): https://codereview.chromium.org/1918373002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebLoadingBehaviorFlag.h:23: WebLoadingBehaviorDocumentWriteBlock = 1 << 1, On 2016/04/29 at 13:37:53, csharrison wrote: > Looks like you are clobbering WebLoadingBehaviorServiceWorkerControlled (which should probably have a test to ensure this never happens). My bad. Thanks for catching this. I have added a test to check no two values are equal (DocumentWritePageLoadMetricsObserverTest.ValidFeatureFlags). Only thing I am concerned about is that for the test to be extensible to new values , it needs updation when someone adds a new value to the enum, else it will always pass for the existing values. I have added a comment to do so.
On 2016/04/29 15:42:37, shivanisha wrote: > https://codereview.chromium.org/1918373002/diff/80001/chrome/browser/page_loa... > File > chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer_unittest.cc > (right): > > https://codereview.chromium.org/1918373002/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer_unittest.cc:118: > } > On 2016/04/29 at 13:37:53, csharrison wrote: > > Can you add a test that doesn't log any histogram (i.e. assert that the > behavior flag is required)? > > I overloaded the existing test NoPossiblePreload to check that, by adding the > condition in AssertNoHistogramsLogged. I think it will be better to create two > separate assert functions AssertNoPreloadHistogramsLogged and > AssertNoBlockHistogramsLogged. Will do that. > > https://codereview.chromium.org/1918373002/diff/80001/third_party/WebKit/publ... > File third_party/WebKit/public/platform/WebLoadingBehaviorFlag.h (right): > > https://codereview.chromium.org/1918373002/diff/80001/third_party/WebKit/publ... > third_party/WebKit/public/platform/WebLoadingBehaviorFlag.h:23: > WebLoadingBehaviorDocumentWriteBlock = 1 << 1, > On 2016/04/29 at 13:37:53, csharrison wrote: > > Looks like you are clobbering WebLoadingBehaviorServiceWorkerControlled (which > should probably have a test to ensure this never happens). > > My bad. Thanks for catching this. > I have added a test to check no two values are equal > (DocumentWritePageLoadMetricsObserverTest.ValidFeatureFlags). Only thing I am > concerned about is that for the test to be extensible to new values , it needs > updation when someone adds a new value to the enum, else it will always pass for > the existing values. I have added a comment to do so. histograms lgtm
blink LGTM
sorry for the late comment - just want to make sure we're observing loading behavior in all the cases we intend. https://codereview.chromium.org/1918373002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1918373002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:122: document.loader()->didObserveLoadingBehavior(WebLoadingBehaviorFlag::WebLoadingBehaviorDocumentWriteBlock); i notice this will only trigger if the setting is enabled, which means we won't get any hits in our control population. is that intended? do we instead want to observe loading behavior as long as we would have blocked the script load, regardless of whether the setting is enabled? similar question for the reload case.
https://codereview.chromium.org/1918373002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1918373002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:122: document.loader()->didObserveLoadingBehavior(WebLoadingBehaviorFlag::WebLoadingBehaviorDocumentWriteBlock); On 2016/04/30 at 01:10:09, Bryan McQuade wrote: > i notice this will only trigger if the setting is enabled, which means we won't get any hits in our control population. is that intended? do we instead want to observe loading behavior as long as we would have blocked the script load, regardless of whether the setting is enabled? similar question for the reload case. Thanks for the catch. I will upload the fixed version and also check if I can add a browser test to test this.
On 2016/04/30 at 19:11:12, shivanisha wrote: > https://codereview.chromium.org/1918373002/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): > > https://codereview.chromium.org/1918373002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:122: document.loader()->didObserveLoadingBehavior(WebLoadingBehaviorFlag::WebLoadingBehaviorDocumentWriteBlock); > On 2016/04/30 at 01:10:09, Bryan McQuade wrote: > > i notice this will only trigger if the setting is enabled, which means we won't get any hits in our control population. is that intended? do we instead want to observe loading behavior as long as we would have blocked the script load, regardless of whether the setting is enabled? similar question for the reload case. > > Thanks for the catch. I will upload the fixed version and also check if I can add a browser test to test this. After the fix, the following scenarios will be logging doc.write blocking feature-specific metrics irrespective of whether the session is part of experiment or control groups: - If disallowFetchForDocWrittenScriptsInMainFrame is enabled, (which we are intending for Canary). The metrics can then be compared between control and experiment groups without needing any other filter. - If disallowFetchForDocWrittenScriptsInMainFrameOnSlowConnections is enabled, (which we are intending for Dev). The metrics should then be compared between control and experiment groups with the connection type filter as 2G but I am not sure if that's possible (see statement from https://groups.google.com/a/google.com/forum/#!searchin/chrome-loading/connec... "One caveat to remember: until we enable UMA uploads on cellular, for now UMA only reports from Wifi; although it does buffer reports to some degree, this probably skews things a bit. ") If the connection type in Finch does not let us classify for 2G, then the metrics for disallowFetchForDocWrittenScriptsInMainFrameOnSlowConnections will be for all connection types which for Experiment group would not necessarily mean that the script was blocked. Thoughts on this?
SGTM On Mon, May 2, 2016 at 12:36 PM, Bryan McQuade <bmcquade@chromium.org> wrote: > Thanks! Good point about 2G / slow network condition complicating this. > The UMA team has since updated the UMA dash to support slicing by > connection type, and this works in the finch dash as well, so I think we > should now be ok for this. > > RE: only reporting on wifi, they do still persist UMA records to disk when > the user is on cellular, and then upload later when the user goes on wifi. > It's not ideal, as it misses users who never use wifi, but should be > sufficient for us. > > So overall I think we should be fine here as long as we restrict analysis > to 2G when using the finch dash when analyzing the finch trial > for disallowFetchForDocWrittenScriptsInMainFrameOnSlowConnections. > > WDYT? > > On Mon, May 2, 2016 at 12:31 PM <shivanisha@chromium.org> wrote: > >> On 2016/04/30 at 19:11:12, shivanisha wrote: >> > >> >> https://codereview.chromium.org/1918373002/diff/100001/third_party/WebKit/Sou... >> > File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp >> (right): >> > >> > >> >> https://codereview.chromium.org/1918373002/diff/100001/third_party/WebKit/Sou... >> > third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:122: >> >> document.loader()->didObserveLoadingBehavior(WebLoadingBehaviorFlag::WebLoadingBehaviorDocumentWriteBlock); >> > On 2016/04/30 at 01:10:09, Bryan McQuade wrote: >> > > i notice this will only trigger if the setting is enabled, which >> means we >> won't get any hits in our control population. is that intended? do we >> instead >> want to observe loading behavior as long as we would have blocked the >> script >> load, regardless of whether the setting is enabled? similar question for >> the >> reload case. >> > >> > Thanks for the catch. I will upload the fixed version and also check if >> I can >> add a browser test to test this. >> >> After the fix, the following scenarios will be logging doc.write blocking >> feature-specific metrics irrespective of whether the session is part of >> experiment or control groups: >> - If disallowFetchForDocWrittenScriptsInMainFrame is enabled, (which we >> are >> intending for Canary). The metrics can then be compared between control >> and >> experiment groups without needing any other filter. >> - If disallowFetchForDocWrittenScriptsInMainFrameOnSlowConnections is >> enabled, >> (which we are intending for Dev). The metrics should then be compared >> between >> control and experiment groups with the connection type filter as 2G but I >> am not >> sure if that's possible (see statement from >> >> https://groups.google.com/a/google.com/forum/#!searchin/chrome-loading/connec... >> "One caveat to remember: until we enable UMA uploads on cellular, for now >> UMA >> only reports from Wifi; although it does buffer reports to some degree, >> this >> probably skews things a bit. ") >> If the connection type in Finch does not let us classify for 2G, then the >> metrics for disallowFetchForDocWrittenScriptsInMainFrameOnSlowConnections >> will >> be for all connection types which for Experiment group would not >> necessarily >> mean that the script was blocked. >> >> Thoughts on this? >> >> https://codereview.chromium.org/1918373002/ >> > -- Shivani -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
SGTM On Mon, May 2, 2016 at 12:36 PM, Bryan McQuade <bmcquade@chromium.org> wrote: > Thanks! Good point about 2G / slow network condition complicating this. > The UMA team has since updated the UMA dash to support slicing by > connection type, and this works in the finch dash as well, so I think we > should now be ok for this. > > RE: only reporting on wifi, they do still persist UMA records to disk when > the user is on cellular, and then upload later when the user goes on wifi. > It's not ideal, as it misses users who never use wifi, but should be > sufficient for us. > > So overall I think we should be fine here as long as we restrict analysis > to 2G when using the finch dash when analyzing the finch trial > for disallowFetchForDocWrittenScriptsInMainFrameOnSlowConnections. > > WDYT? > > On Mon, May 2, 2016 at 12:31 PM <shivanisha@chromium.org> wrote: > >> On 2016/04/30 at 19:11:12, shivanisha wrote: >> > >> >> https://codereview.chromium.org/1918373002/diff/100001/third_party/WebKit/Sou... >> > File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp >> (right): >> > >> > >> >> https://codereview.chromium.org/1918373002/diff/100001/third_party/WebKit/Sou... >> > third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:122: >> >> document.loader()->didObserveLoadingBehavior(WebLoadingBehaviorFlag::WebLoadingBehaviorDocumentWriteBlock); >> > On 2016/04/30 at 01:10:09, Bryan McQuade wrote: >> > > i notice this will only trigger if the setting is enabled, which >> means we >> won't get any hits in our control population. is that intended? do we >> instead >> want to observe loading behavior as long as we would have blocked the >> script >> load, regardless of whether the setting is enabled? similar question for >> the >> reload case. >> > >> > Thanks for the catch. I will upload the fixed version and also check if >> I can >> add a browser test to test this. >> >> After the fix, the following scenarios will be logging doc.write blocking >> feature-specific metrics irrespective of whether the session is part of >> experiment or control groups: >> - If disallowFetchForDocWrittenScriptsInMainFrame is enabled, (which we >> are >> intending for Canary). The metrics can then be compared between control >> and >> experiment groups without needing any other filter. >> - If disallowFetchForDocWrittenScriptsInMainFrameOnSlowConnections is >> enabled, >> (which we are intending for Dev). The metrics should then be compared >> between >> control and experiment groups with the connection type filter as 2G but I >> am not >> sure if that's possible (see statement from >> >> https://groups.google.com/a/google.com/forum/#!searchin/chrome-loading/connec... >> "One caveat to remember: until we enable UMA uploads on cellular, for now >> UMA >> only reports from Wifi; although it does buffer reports to some degree, >> this >> probably skews things a bit. ") >> If the connection type in Finch does not let us classify for 2G, then the >> metrics for disallowFetchForDocWrittenScriptsInMainFrameOnSlowConnections >> will >> be for all connection types which for Experiment group would not >> necessarily >> mean that the script was blocked. >> >> Thoughts on this? >> >> https://codereview.chromium.org/1918373002/ >> > -- Shivani -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thanks! Good point about 2G / slow network condition complicating this. The UMA team has since updated the UMA dash to support slicing by connection type, and this works in the finch dash as well, so I think we should now be ok for this. RE: only reporting on wifi, they do still persist UMA records to disk when the user is on cellular, and then upload later when the user goes on wifi. It's not ideal, as it misses users who never use wifi, but should be sufficient for us. So overall I think we should be fine here as long as we restrict analysis to 2G when using the finch dash when analyzing the finch trial for disallowFetchForDocWrittenScriptsInMainFrameOnSlowConnections. WDYT? On Mon, May 2, 2016 at 12:31 PM <shivanisha@chromium.org> wrote: > On 2016/04/30 at 19:11:12, shivanisha wrote: > > > > https://codereview.chromium.org/1918373002/diff/100001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): > > > > > > https://codereview.chromium.org/1918373002/diff/100001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:122: > > document.loader()->didObserveLoadingBehavior(WebLoadingBehaviorFlag::WebLoadingBehaviorDocumentWriteBlock); > > On 2016/04/30 at 01:10:09, Bryan McQuade wrote: > > > i notice this will only trigger if the setting is enabled, which means > we > won't get any hits in our control population. is that intended? do we > instead > want to observe loading behavior as long as we would have blocked the > script > load, regardless of whether the setting is enabled? similar question for > the > reload case. > > > > Thanks for the catch. I will upload the fixed version and also check if > I can > add a browser test to test this. > > After the fix, the following scenarios will be logging doc.write blocking > feature-specific metrics irrespective of whether the session is part of > experiment or control groups: > - If disallowFetchForDocWrittenScriptsInMainFrame is enabled, (which we are > intending for Canary). The metrics can then be compared between control and > experiment groups without needing any other filter. > - If disallowFetchForDocWrittenScriptsInMainFrameOnSlowConnections is > enabled, > (which we are intending for Dev). The metrics should then be compared > between > control and experiment groups with the connection type filter as 2G but I > am not > sure if that's possible (see statement from > > https://groups.google.com/a/google.com/forum/#!searchin/chrome-loading/connec... > "One caveat to remember: until we enable UMA uploads on cellular, for now > UMA > only reports from Wifi; although it does buffer reports to some degree, > this > probably skews things a bit. ") > If the connection type in Finch does not let us classify for 2G, then the > metrics for disallowFetchForDocWrittenScriptsInMainFrameOnSlowConnections > will > be for all connection types which for Experiment group would not > necessarily > mean that the script was blocked. > > Thoughts on this? > > https://codereview.chromium.org/1918373002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thanks! Good point about 2G / slow network condition complicating this. The UMA team has since updated the UMA dash to support slicing by connection type, and this works in the finch dash as well, so I think we should now be ok for this. RE: only reporting on wifi, they do still persist UMA records to disk when the user is on cellular, and then upload later when the user goes on wifi. It's not ideal, as it misses users who never use wifi, but should be sufficient for us. So overall I think we should be fine here as long as we restrict analysis to 2G when using the finch dash when analyzing the finch trial for disallowFetchForDocWrittenScriptsInMainFrameOnSlowConnections. WDYT? On Mon, May 2, 2016 at 12:31 PM <shivanisha@chromium.org> wrote: > On 2016/04/30 at 19:11:12, shivanisha wrote: > > > > https://codereview.chromium.org/1918373002/diff/100001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): > > > > > > https://codereview.chromium.org/1918373002/diff/100001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:122: > > document.loader()->didObserveLoadingBehavior(WebLoadingBehaviorFlag::WebLoadingBehaviorDocumentWriteBlock); > > On 2016/04/30 at 01:10:09, Bryan McQuade wrote: > > > i notice this will only trigger if the setting is enabled, which means > we > won't get any hits in our control population. is that intended? do we > instead > want to observe loading behavior as long as we would have blocked the > script > load, regardless of whether the setting is enabled? similar question for > the > reload case. > > > > Thanks for the catch. I will upload the fixed version and also check if > I can > add a browser test to test this. > > After the fix, the following scenarios will be logging doc.write blocking > feature-specific metrics irrespective of whether the session is part of > experiment or control groups: > - If disallowFetchForDocWrittenScriptsInMainFrame is enabled, (which we are > intending for Canary). The metrics can then be compared between control and > experiment groups without needing any other filter. > - If disallowFetchForDocWrittenScriptsInMainFrameOnSlowConnections is > enabled, > (which we are intending for Dev). The metrics should then be compared > between > control and experiment groups with the connection type filter as 2G but I > am not > sure if that's possible (see statement from > > https://groups.google.com/a/google.com/forum/#!searchin/chrome-loading/connec... > "One caveat to remember: until we enable UMA uploads on cellular, for now > UMA > only reports from Wifi; although it does buffer reports to some degree, > this > probably skews things a bit. ") > If the connection type in Finch does not let us classify for 2G, then the > metrics for disallowFetchForDocWrittenScriptsInMainFrameOnSlowConnections > will > be for all connection types which for Experiment group would not > necessarily > mean that the script was blocked. > > Thoughts on this? > > https://codereview.chromium.org/1918373002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Where in the finch dashboard can you restrict to 2G? I only see that in the histograms dashboard. On Mon, May 2, 2016 at 12:40 PM, Shivani Sharma <shivanisha@google.com> wrote: > SGTM > > On Mon, May 2, 2016 at 12:36 PM, Bryan McQuade <bmcquade@chromium.org> > wrote: > >> Thanks! Good point about 2G / slow network condition complicating this. >> The UMA team has since updated the UMA dash to support slicing by >> connection type, and this works in the finch dash as well, so I think we >> should now be ok for this. >> >> RE: only reporting on wifi, they do still persist UMA records to disk >> when the user is on cellular, and then upload later when the user goes on >> wifi. It's not ideal, as it misses users who never use wifi, but should be >> sufficient for us. >> >> So overall I think we should be fine here as long as we restrict analysis >> to 2G when using the finch dash when analyzing the finch trial >> for disallowFetchForDocWrittenScriptsInMainFrameOnSlowConnections. >> >> WDYT? >> >> On Mon, May 2, 2016 at 12:31 PM <shivanisha@chromium.org> wrote: >> >>> On 2016/04/30 at 19:11:12, shivanisha wrote: >>> > >>> >>> https://codereview.chromium.org/1918373002/diff/100001/third_party/WebKit/Sou... >>> > File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp >>> (right): >>> > >>> > >>> >>> https://codereview.chromium.org/1918373002/diff/100001/third_party/WebKit/Sou... >>> > third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:122: >>> >>> document.loader()->didObserveLoadingBehavior(WebLoadingBehaviorFlag::WebLoadingBehaviorDocumentWriteBlock); >>> > On 2016/04/30 at 01:10:09, Bryan McQuade wrote: >>> > > i notice this will only trigger if the setting is enabled, which >>> means we >>> won't get any hits in our control population. is that intended? do we >>> instead >>> want to observe loading behavior as long as we would have blocked the >>> script >>> load, regardless of whether the setting is enabled? similar question for >>> the >>> reload case. >>> > >>> > Thanks for the catch. I will upload the fixed version and also check >>> if I can >>> add a browser test to test this. >>> >>> After the fix, the following scenarios will be logging doc.write blocking >>> feature-specific metrics irrespective of whether the session is part of >>> experiment or control groups: >>> - If disallowFetchForDocWrittenScriptsInMainFrame is enabled, (which we >>> are >>> intending for Canary). The metrics can then be compared between control >>> and >>> experiment groups without needing any other filter. >>> - If disallowFetchForDocWrittenScriptsInMainFrameOnSlowConnections is >>> enabled, >>> (which we are intending for Dev). The metrics should then be compared >>> between >>> control and experiment groups with the connection type filter as 2G but >>> I am not >>> sure if that's possible (see statement from >>> >>> https://groups.google.com/a/google.com/forum/#!searchin/chrome-loading/connec... >>> "One caveat to remember: until we enable UMA uploads on cellular, for >>> now UMA >>> only reports from Wifi; although it does buffer reports to some degree, >>> this >>> probably skews things a bit. ") >>> If the connection type in Finch does not let us classify for 2G, then the >>> metrics for >>> disallowFetchForDocWrittenScriptsInMainFrameOnSlowConnections will >>> be for all connection types which for Experiment group would not >>> necessarily >>> mean that the script was blocked. >>> >>> Thoughts on this? >>> >>> https://codereview.chromium.org/1918373002/ >>> >> > > > -- > Shivani > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Where in the finch dashboard can you restrict to 2G? I only see that in the histograms dashboard. On Mon, May 2, 2016 at 12:40 PM, Shivani Sharma <shivanisha@google.com> wrote: > SGTM > > On Mon, May 2, 2016 at 12:36 PM, Bryan McQuade <bmcquade@chromium.org> > wrote: > >> Thanks! Good point about 2G / slow network condition complicating this. >> The UMA team has since updated the UMA dash to support slicing by >> connection type, and this works in the finch dash as well, so I think we >> should now be ok for this. >> >> RE: only reporting on wifi, they do still persist UMA records to disk >> when the user is on cellular, and then upload later when the user goes on >> wifi. It's not ideal, as it misses users who never use wifi, but should be >> sufficient for us. >> >> So overall I think we should be fine here as long as we restrict analysis >> to 2G when using the finch dash when analyzing the finch trial >> for disallowFetchForDocWrittenScriptsInMainFrameOnSlowConnections. >> >> WDYT? >> >> On Mon, May 2, 2016 at 12:31 PM <shivanisha@chromium.org> wrote: >> >>> On 2016/04/30 at 19:11:12, shivanisha wrote: >>> > >>> >>> https://codereview.chromium.org/1918373002/diff/100001/third_party/WebKit/Sou... >>> > File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp >>> (right): >>> > >>> > >>> >>> https://codereview.chromium.org/1918373002/diff/100001/third_party/WebKit/Sou... >>> > third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:122: >>> >>> document.loader()->didObserveLoadingBehavior(WebLoadingBehaviorFlag::WebLoadingBehaviorDocumentWriteBlock); >>> > On 2016/04/30 at 01:10:09, Bryan McQuade wrote: >>> > > i notice this will only trigger if the setting is enabled, which >>> means we >>> won't get any hits in our control population. is that intended? do we >>> instead >>> want to observe loading behavior as long as we would have blocked the >>> script >>> load, regardless of whether the setting is enabled? similar question for >>> the >>> reload case. >>> > >>> > Thanks for the catch. I will upload the fixed version and also check >>> if I can >>> add a browser test to test this. >>> >>> After the fix, the following scenarios will be logging doc.write blocking >>> feature-specific metrics irrespective of whether the session is part of >>> experiment or control groups: >>> - If disallowFetchForDocWrittenScriptsInMainFrame is enabled, (which we >>> are >>> intending for Canary). The metrics can then be compared between control >>> and >>> experiment groups without needing any other filter. >>> - If disallowFetchForDocWrittenScriptsInMainFrameOnSlowConnections is >>> enabled, >>> (which we are intending for Dev). The metrics should then be compared >>> between >>> control and experiment groups with the connection type filter as 2G but >>> I am not >>> sure if that's possible (see statement from >>> >>> https://groups.google.com/a/google.com/forum/#!searchin/chrome-loading/connec... >>> "One caveat to remember: until we enable UMA uploads on cellular, for >>> now UMA >>> only reports from Wifi; although it does buffer reports to some degree, >>> this >>> probably skews things a bit. ") >>> If the connection type in Finch does not let us classify for 2G, then the >>> metrics for >>> disallowFetchForDocWrittenScriptsInMainFrameOnSlowConnections will >>> be for all connection types which for Experiment group would not >>> necessarily >>> mean that the script was blocked. >>> >>> Thoughts on this? >>> >>> https://codereview.chromium.org/1918373002/ >>> >> > > > -- > Shivani > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/05/02 16:51:17, jkarlin wrote: > Where in the finch dashboard can you restrict to 2G? I only see that in the > histograms dashboard. > It's a the blue "Add filter" above versions, selecting "Connection type == 3"
Sorry for the late response, reserving stamp for when you respond to bmcquade@s concern about metrics logging. https://codereview.chromium.org/1918373002/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/1918373002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer_unittest.cc:139: TEST_F(DocumentWritePageLoadMetricsObserverTest, ValidFeatureFlags) { This test does not belong here. Probably it belongs in blink/platform. It is not really that necessary though. This kind of mistake will usually be caught in code review / other tests. Up to you. https://codereview.chromium.org/1918373002/diff/100001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_util.cc (right): https://codereview.chromium.org/1918373002/diff/100001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_util.cc:21: bool WasParseInForeground(const base::TimeDelta& parse_start, ditto here. https://codereview.chromium.org/1918373002/diff/100001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_util.h (right): https://codereview.chromium.org/1918373002/diff/100001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_util.h:28: bool WasStartedInForegroundEventInForeground(const base::TimeDelta& event, Don't pass time values by const reference, pass by value. If you edit any function that passes by const ref, convert it. This is because the time values are intentionally made to only store 64 bit integers.
Changes in Patch 8 over earlier patches are: - Added browser tests to test the histogram counts in various scenarios. - Metrics should be logged independent of blink/finch setting enabled if there is a blockable script. - Charles' feedback. https://codereview.chromium.org/1918373002/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/1918373002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer_unittest.cc:139: TEST_F(DocumentWritePageLoadMetricsObserverTest, ValidFeatureFlags) { On 2016/05/02 at 21:48:28, csharrison wrote: > This test does not belong here. Probably it belongs in blink/platform. It is not really that necessary though. This kind of mistake will usually be caught in code review / other tests. Up to you. Agree its not really needed. Also might not be practically maintainable as any new value will need to be added here. Removing the test. Also removing the comment from the definition of WebLoadingBehaviorFlag. https://codereview.chromium.org/1918373002/diff/100001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_util.cc (right): https://codereview.chromium.org/1918373002/diff/100001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_util.cc:21: bool WasParseInForeground(const base::TimeDelta& parse_start, On 2016/05/02 at 21:48:28, csharrison wrote: > ditto here. done. https://codereview.chromium.org/1918373002/diff/100001/components/page_load_m... File components/page_load_metrics/browser/page_load_metrics_util.h (right): https://codereview.chromium.org/1918373002/diff/100001/components/page_load_m... components/page_load_metrics/browser/page_load_metrics_util.h:28: bool WasStartedInForegroundEventInForeground(const base::TimeDelta& event, On 2016/05/02 at 21:48:28, csharrison wrote: > Don't pass time values by const reference, pass by value. If you edit any function that passes by const ref, convert it. > > This is because the time values are intentionally made to only store 64 bit integers. Agree that will be consistent with most usages of TimeDelta in the code. Changed.
LGTM thanks! https://codereview.chromium.org/1918373002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1918373002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:121: if (!disallowFetch) this could be updated to just return disallowFetch;
LGTM
The CQ bit was checked by shivanisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkarlin@chromium.org, japhet@chromium.org, holte@chromium.org, bmcquade@chromium.org Link to the patchset: https://codereview.chromium.org/1918373002/#ps160001 (title: "Bryan's comment: Returning disallowFetch from shouldDisallowFetchForMainFrameScript")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1918373002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1918373002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== This CL logs the metrics to see the impact of document.write script blocking feature. The metrics are specifically for pages that have a blockable document.written script. The CL also has a metric to count the number of reloads for such pages to check if there is any increase in number of reloads possibly implying page breaks. BUG=603152 ========== to ========== This CL logs the metrics to see the impact of document.write script blocking feature. The metrics are specifically for pages that have a blockable document.written script. The CL also has a metric to count the number of reloads for such pages to check if there is any increase in number of reloads possibly implying page breaks. BUG=603152 Committed: https://crrev.com/e6340ed747c7d4dbeedff9c963b6bd27825dd03d Cr-Commit-Position: refs/heads/master@{#391526} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/e6340ed747c7d4dbeedff9c963b6bd27825dd03d Cr-Commit-Position: refs/heads/master@{#391526} |
