Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2139243003:
customtabs: Send time to first contentful paint to the caller. (Closed)
|
Created:
4 years, 5 months ago by Benoit L Modified:
4 years, 4 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews, csharrison+watch_chromium.org, lizeb+watch-custom-tabs_chromium.org, 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. |
Descriptioncustomtabs: Send time to first contentful paint to the caller.
Time to First Contentful Paint is a very useful metric to assess page load
performance. It is collected on the native side. This patch forwards it to the
Java side on Android, and makes it available to the embedding app for Custom
Tabs.
BUG=624909
Committed: https://crrev.com/d6fe19b87ba15a78130ec62d87cb0c2c3bae3eaf
Cr-Commit-Position: refs/heads/master@{#408391}
Patch Set 1 #Patch Set 2 : . #
Total comments: 4
Patch Set 3 : Rebase. #Patch Set 4 : Comments and tests. #
Total comments: 9
Patch Set 5 : Address comments. #
Total comments: 14
Patch Set 6 : Address comments. #Messages
Total messages: 28 (10 generated)
bmcquade@chromium.org changed reviewers: + bmcquade@chromium.org
just a couple small suggestions. thanks for working on this! https://codereview.chromium.org/2139243003/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.h (right): https://codereview.chromium.org/2139243003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.h:26: void OnTimingUpdate( we added a new OnFirstContentfulPaint which gets called zero or one times, when a FCP happens. can you use that instead? OTOH maybe we just want to push the whole PageLoadTiming struct over to the java side every time it gets updated, in which case implementing OnTimingUpdate is the right thing to do. https://codereview.chromium.org/2139243003/diff/20001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/2139243003/diff/20001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:112: content::WebContents* web_contents) = 0; the PageLoadMetricsEmbedder implementation now has a web_contents_ member, so you shouldn't need to pass it as a param here.
Description was changed from ========== customtabs: Send time to first contentful paint to the caller. ========== to ========== customtabs: Send time to first contentful paint to the caller. Time to First Contentful Paint is a very useful metric to assess page load performance. It is collected on the native side. This patch forwards it to the Java side on Android, and makes it available to the embedding app for Custom Tabs. BUG=624909 ==========
lizeb@chromium.org changed reviewers: + mariakhomenko@chromium.org
mariakhomenko@chromium.org: I'm sending this with a question. Can you give me your opinion on the observer in Java. On the native side, the observer is tied to a navigation. It has the lifetime of a navigation, not a tab or a WebContents. That's why the Java side is a static class. What do you think? Thanks! https://codereview.chromium.org/2139243003/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.h (right): https://codereview.chromium.org/2139243003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.h:26: void OnTimingUpdate( On 2016/07/21 23:56:13, Bryan McQuade wrote: > we added a new OnFirstContentfulPaint which gets called zero or one times, when > a FCP happens. can you use that instead? OTOH maybe we just want to push the > whole PageLoadTiming struct over to the java side every time it gets updated, in > which case implementing OnTimingUpdate is the right thing to do. Let's start with a simple callback first, using OnFirstContentfulPaint() as you suggested. On the Android side though, I think we want to keep the interface generic, since the public-facing API is only updatable when new versions of the Android support library ship. https://codereview.chromium.org/2139243003/diff/20001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.h (right): https://codereview.chromium.org/2139243003/diff/20001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.h:112: content::WebContents* web_contents) = 0; On 2016/07/21 23:56:13, Bryan McQuade wrote: > the PageLoadMetricsEmbedder implementation now has a web_contents_ member, so > you shouldn't need to pass it as a param here. Ah, thanks! Done.
The design LGTM. I think the static PageMetrics class is fine. https://codereview.chromium.org/2139243003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://codereview.chromium.org/2139243003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:104: private WebContents mWebContents; final on all these? https://codereview.chromium.org/2139243003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:114: public void onFirstContentfulPaint(WebContents webContents, double firstContentfulPaintMs) { what's the reason to use double rather than long for contentfulPaintMs? https://codereview.chromium.org/2139243003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2139243003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:72: public static final int PAGE_LOAD_METRIC = 42; why 42? https://codereview.chromium.org/2139243003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/metrics/PageLoadMetrics.java (right): https://codereview.chromium.org/2139243003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/metrics/PageLoadMetrics.java:20: public class PageLoadMetrics { I think you should declare private constructor for PageLoadMetrics, so that it's clear that this is meant to be a static class.
Thanks for the comments! All done. https://codereview.chromium.org/2139243003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://codereview.chromium.org/2139243003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:104: private WebContents mWebContents; On 2016/07/22 16:57:53, Maria wrote: > final on all these? Done. https://codereview.chromium.org/2139243003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:114: public void onFirstContentfulPaint(WebContents webContents, double firstContentfulPaintMs) { On 2016/07/22 16:57:53, Maria wrote: > what's the reason to use double rather than long for contentfulPaintMs? Done. https://codereview.chromium.org/2139243003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2139243003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:72: public static final int PAGE_LOAD_METRIC = 42; On 2016/07/22 16:57:53, Maria wrote: > why 42? This one is a bit unfortunate. The service is an implementation of an interface in the Android support library. This specifies two callbacks: - onNavigationEvent(int code, Bundle extras) - extraCallback(String name, Bundle args) the meaning of code is in the support library, meaning that we cannot change it at the same time as Chrome, and extraCallback() doesn't currently work due to a bug in the support library (we wrote it, oops). So the simplest solution here is to add a new code for onNavigationEvent(). 42 was mostly chosen to remind myself that it's a hack. wdyt? https://codereview.chromium.org/2139243003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/metrics/PageLoadMetrics.java (right): https://codereview.chromium.org/2139243003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/metrics/PageLoadMetrics.java:20: public class PageLoadMetrics { On 2016/07/22 16:57:53, Maria wrote: > I think you should declare private constructor for PageLoadMetrics, so that it's > clear that this is meant to be a static class. Done.
lgtm https://codereview.chromium.org/2139243003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2139243003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:72: public static final int PAGE_LOAD_METRIC = 42; On 2016/07/25 09:20:00, Benoit L wrote: > On 2016/07/22 16:57:53, Maria wrote: > > why 42? > > This one is a bit unfortunate. > The service is an implementation of an interface in the Android support library. > This specifies two callbacks: > - onNavigationEvent(int code, Bundle extras) > - extraCallback(String name, Bundle args) > > the meaning of code is in the support library, meaning that we cannot change it > at the same time as Chrome, and extraCallback() doesn't currently work due to a > bug in the support library (we wrote it, oops). > > > So the simplest solution here is to add a new code for onNavigationEvent(). 42 > was mostly chosen to remind myself that it's a hack. > > wdyt? How about a TODO() here describing the eventual fix?
Thanks! bmcquade: Can you take a look at chrome/browser/page_load_metrics?
Thanks! This looks good. I left a few comments. My main open question is how we're going to make sure that we only send the first contentful paint notification for the first navigation from the hosting application into the custom tab. We should make sure that we never send a notification for subsequent URLs navigated from within the custom tab, since the embedder will have no way to distinguish those callbacks from the initial page that was navigated to. Do we have logic in place to avoid tracking subsequent navs? https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:115: if (webContents != mWebContents) return; do we expect to hit this early return? in what cases might this happen? https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:117: mConnection.notifyPageLoadMetric( we've found that it's less useful to track first contentful paints for pages that get backgrounded. in particular, once backgrounded, the rendering pipeline gets throttled, which means that even if a page reaches the point where it's ready to paint, it won't actually paint until foregrounded again IIUC. so in that case, time to first contentful paint ends up being time to re-enter foreground. do we have a clean way to track whether the page was backgrounded between nav start and FCP? alternatively we can suppress notifications of FCP if the page was backgrounded on the c++ side, which is straightforward. https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/metrics/PageLoadMetrics.java (right): https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/metrics/PageLoadMetrics.java:46: sObservers.remove(observer); is this safe if a listener removes themselves during a callback dispatch? if not, is there a java class that handles removal during iteration? https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.cc:25: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); if we want to cases where a page is backgrounded before FCP, you can add: if (!WasStartedInForegroundOptionalEventInForeground( timing.first_contentful_paint, info)) { return; } (see earlier comment for more reason as to why we might want to do this) https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.h (right): https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.h:32: bool has_seen_first_contentful_paint_; you can remove this now
https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.cc:12: #include "components/page_load_metrics/browser/page_load_metrics_util.h" same - need to update include path https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.h (right): https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.h:11: #include "components/page_load_metrics/browser/page_load_metrics_observer.h" i just moved page_load_metrics out of components in https://codereview.chromium.org/2177743002, so this include path should now be chrome/browser/page_load_metrics/page_load_metrics_observer.h
Thanks! All done. https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:115: if (webContents != mWebContents) return; On 2016/07/26 14:29:33, Bryan McQuade wrote: > do we expect to hit this early return? in what cases might this happen? Yes. There can be multiple Custom Tabs active at the same time (each having its own WebContents). https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:117: mConnection.notifyPageLoadMetric( On 2016/07/26 14:29:33, Bryan McQuade wrote: > we've found that it's less useful to track first contentful paints for pages > that get backgrounded. in particular, once backgrounded, the rendering pipeline > gets throttled, which means that even if a page reaches the point where it's > ready to paint, it won't actually paint until foregrounded again IIUC. so in > that case, time to first contentful paint ends up being time to re-enter > foreground. > > do we have a clean way to track whether the page was backgrounded between nav > start and FCP? alternatively we can suppress notifications of FCP if the page > was backgrounded on the c++ side, which is straightforward. Thanks for pointing that out! There is a way to know that the page was backgrounded from the hosting application, since Custom Tabs relay the information when a page visibility changes. https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/metrics/PageLoadMetrics.java (right): https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/metrics/PageLoadMetrics.java:46: sObservers.remove(observer); On 2016/07/26 14:29:33, Bryan McQuade wrote: > is this safe if a listener removes themselves during a callback dispatch? if > not, is there a java class that handles removal during iteration? Yes, there is ObserverList in Java as well. I deliberately wanted to avoid it here for now, to keep things simpler. But coming back to it you're absolutely right, it's better to make this more robust right away. Done. https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.cc:12: #include "components/page_load_metrics/browser/page_load_metrics_util.h" On 2016/07/26 23:44:27, Bryan McQuade wrote: > same - need to update include path Done. https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.cc:25: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); On 2016/07/26 14:29:33, Bryan McQuade wrote: > if we want to cases where a page is backgrounded before FCP, you can add: > > if (!WasStartedInForegroundOptionalEventInForeground( > timing.first_contentful_paint, info)) { > return; > } > > (see earlier comment for more reason as to why we might want to do this) I guess there are two ways to look at how to proceed here: (a) Only pass the "raw" events to Java, and let the Java side handle the filtering (b) Provide an already filtered metric to the Java side. I erred on the side of option (a) since it's more flexible, but I'm not firmly convinced it's the best approach. What do you think? https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.h (right): https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.h:11: #include "components/page_load_metrics/browser/page_load_metrics_observer.h" On 2016/07/26 23:44:27, Bryan McQuade wrote: > i just moved page_load_metrics out of components in > https://codereview.chromium.org/2177743002, so this include path should now be > chrome/browser/page_load_metrics/page_load_metrics_observer.h Thanks for pointing that out! Done. https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.h:32: bool has_seen_first_contentful_paint_; On 2016/07/26 14:29:33, Bryan McQuade wrote: > you can remove this now Done.
On 2016/07/27 at 16:23:45, lizeb wrote: > Thanks! > All done. > > https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... > File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): > > https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:115: if (webContents != mWebContents) return; > On 2016/07/26 14:29:33, Bryan McQuade wrote: > > do we expect to hit this early return? in what cases might this happen? > > Yes. > > There can be multiple Custom Tabs active at the same time (each having its own WebContents). > > https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:117: mConnection.notifyPageLoadMetric( > On 2016/07/26 14:29:33, Bryan McQuade wrote: > > we've found that it's less useful to track first contentful paints for pages > > that get backgrounded. in particular, once backgrounded, the rendering pipeline > > gets throttled, which means that even if a page reaches the point where it's > > ready to paint, it won't actually paint until foregrounded again IIUC. so in > > that case, time to first contentful paint ends up being time to re-enter > > foreground. > > > > do we have a clean way to track whether the page was backgrounded between nav > > start and FCP? alternatively we can suppress notifications of FCP if the page > > was backgrounded on the c++ side, which is straightforward. > > Thanks for pointing that out! > > There is a way to know that the page was backgrounded from the hosting application, since Custom Tabs relay the information when a page visibility changes. > > https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... > File chrome/android/java/src/org/chromium/chrome/browser/metrics/PageLoadMetrics.java (right): > > https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/metrics/PageLoadMetrics.java:46: sObservers.remove(observer); > On 2016/07/26 14:29:33, Bryan McQuade wrote: > > is this safe if a listener removes themselves during a callback dispatch? if > > not, is there a java class that handles removal during iteration? > > Yes, there is ObserverList in Java as well. I deliberately wanted to avoid it here for now, to keep things simpler. > > But coming back to it you're absolutely right, it's better to make this more robust right away. > Done. > > https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... > File chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.cc (right): > > https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.cc:12: #include "components/page_load_metrics/browser/page_load_metrics_util.h" > On 2016/07/26 23:44:27, Bryan McQuade wrote: > > same - need to update include path > > Done. > > https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.cc:25: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); > On 2016/07/26 14:29:33, Bryan McQuade wrote: > > if we want to cases where a page is backgrounded before FCP, you can add: > > > > if (!WasStartedInForegroundOptionalEventInForeground( > > timing.first_contentful_paint, info)) { > > return; > > } > > > > (see earlier comment for more reason as to why we might want to do this) > > I guess there are two ways to look at how to proceed here: > (a) Only pass the "raw" events to Java, and let the Java side handle the filtering > (b) Provide an already filtered metric to the Java side. > > I erred on the side of option (a) since it's more flexible, but I'm not firmly convinced it's the best approach. > > What do you think? > > https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... > File chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.h (right): > > https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.h:11: #include "components/page_load_metrics/browser/page_load_metrics_observer.h" > On 2016/07/26 23:44:27, Bryan McQuade wrote: > > i just moved page_load_metrics out of components in > > https://codereview.chromium.org/2177743002, so this include path should now be > > chrome/browser/page_load_metrics/page_load_metrics_observer.h > Thanks for pointing that out! > Done. > > https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.h:32: bool has_seen_first_contentful_paint_; > On 2016/07/26 14:29:33, Bryan McQuade wrote: > > you can remove this now > > Done. Thanks for the fixes! Just wanted to call out one open question - not sure if it has been addressed: My main open question is how we're going to make sure that we only send the first contentful paint notification for the first navigation from the hosting application into the custom tab. We should make sure that we never send a notification for subsequent URLs navigated from within the custom tab, since the embedder will have no way to distinguish those callbacks from the initial page that was navigated to. Do we have logic in place to avoid tracking subsequent navs?
On 2016/07/28 10:58:23, Bryan McQuade wrote: > On 2016/07/27 at 16:23:45, lizeb wrote: > > Thanks! > > All done. > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... > > File > chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java > (right): > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:115: > if (webContents != mWebContents) return; > > On 2016/07/26 14:29:33, Bryan McQuade wrote: > > > do we expect to hit this early return? in what cases might this happen? > > > > Yes. > > > > There can be multiple Custom Tabs active at the same time (each having its own > WebContents). > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:117: > mConnection.notifyPageLoadMetric( > > On 2016/07/26 14:29:33, Bryan McQuade wrote: > > > we've found that it's less useful to track first contentful paints for pages > > > that get backgrounded. in particular, once backgrounded, the rendering > pipeline > > > gets throttled, which means that even if a page reaches the point where it's > > > ready to paint, it won't actually paint until foregrounded again IIUC. so in > > > that case, time to first contentful paint ends up being time to re-enter > > > foreground. > > > > > > do we have a clean way to track whether the page was backgrounded between > nav > > > start and FCP? alternatively we can suppress notifications of FCP if the > page > > > was backgrounded on the c++ side, which is straightforward. > > > > Thanks for pointing that out! > > > > There is a way to know that the page was backgrounded from the hosting > application, since Custom Tabs relay the information when a page visibility > changes. > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... > > File > chrome/android/java/src/org/chromium/chrome/browser/metrics/PageLoadMetrics.java > (right): > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/metrics/PageLoadMetrics.java:46: > sObservers.remove(observer); > > On 2016/07/26 14:29:33, Bryan McQuade wrote: > > > is this safe if a listener removes themselves during a callback dispatch? if > > > not, is there a java class that handles removal during iteration? > > > > Yes, there is ObserverList in Java as well. I deliberately wanted to avoid it > here for now, to keep things simpler. > > > > But coming back to it you're absolutely right, it's better to make this more > robust right away. > > Done. > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... > > File > chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.cc > (right): > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... > > > chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.cc:12: > #include "components/page_load_metrics/browser/page_load_metrics_util.h" > > On 2016/07/26 23:44:27, Bryan McQuade wrote: > > > same - need to update include path > > > > Done. > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... > > > chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.cc:25: > DCHECK_CURRENTLY_ON(content::BrowserThread::UI); > > On 2016/07/26 14:29:33, Bryan McQuade wrote: > > > if we want to cases where a page is backgrounded before FCP, you can add: > > > > > > if (!WasStartedInForegroundOptionalEventInForeground( > > > timing.first_contentful_paint, info)) { > > > return; > > > } > > > > > > (see earlier comment for more reason as to why we might want to do this) > > > > I guess there are two ways to look at how to proceed here: > > (a) Only pass the "raw" events to Java, and let the Java side handle the > filtering > > (b) Provide an already filtered metric to the Java side. > > > > I erred on the side of option (a) since it's more flexible, but I'm not firmly > convinced it's the best approach. > > > > What do you think? > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... > > File > chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.h > (right): > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... > > > chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.h:11: > #include "components/page_load_metrics/browser/page_load_metrics_observer.h" > > On 2016/07/26 23:44:27, Bryan McQuade wrote: > > > i just moved page_load_metrics out of components in > > > https://codereview.chromium.org/2177743002, so this include path should now > be > > > chrome/browser/page_load_metrics/page_load_metrics_observer.h > > Thanks for pointing that out! > > Done. > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... > > > chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.h:32: > bool has_seen_first_contentful_paint_; > > On 2016/07/26 14:29:33, Bryan McQuade wrote: > > > you can remove this now > > > > Done. > > Thanks for the fixes! Just wanted to call out one open question - not sure if it > has been addressed: My main open question is how we're going to make sure that > we only send the first contentful paint notification for the first navigation > from the hosting application into the custom tab. We should make sure that we > never send a notification for subsequent URLs navigated from within the custom > tab, since the embedder will have no way to distinguish those callbacks from the > initial page that was navigated to. Do we have logic in place to avoid tracking > subsequent navs? Sorry, it was in the e-mail thread, but not discussed here. Let's correct that. The embedder receives notifications for page navigations, for each navigation (without the URL, they can only know the URL for the first navigation or when it's explicitely shared). See https://developer.android.com/reference/android/support/customtabs/CustomTabs..., you have NAVIGATION_{STARTED,ABORTED,FAILED,FINISHED} notifications. The clients that are tracking these timings already have to be aware that loading notifications cannot be linked to URL except for the first one (or when it's explicitely shared). For symmetry, I preferred to send the page load metrics notifications for all page loads, since we send the other one for all page loads as well. What do you think?
On 2016/07/28 at 11:07:49, lizeb wrote: > On 2016/07/28 10:58:23, Bryan McQuade wrote: > > On 2016/07/27 at 16:23:45, lizeb wrote: > > > Thanks! > > > All done. > > > > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... > > > File > > chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java > > (right): > > > > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... > > > > > chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:115: > > if (webContents != mWebContents) return; > > > On 2016/07/26 14:29:33, Bryan McQuade wrote: > > > > do we expect to hit this early return? in what cases might this happen? > > > > > > Yes. > > > > > > There can be multiple Custom Tabs active at the same time (each having its own > > WebContents). > > > > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... > > > > > chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:117: > > mConnection.notifyPageLoadMetric( > > > On 2016/07/26 14:29:33, Bryan McQuade wrote: > > > > we've found that it's less useful to track first contentful paints for pages > > > > that get backgrounded. in particular, once backgrounded, the rendering > > pipeline > > > > gets throttled, which means that even if a page reaches the point where it's > > > > ready to paint, it won't actually paint until foregrounded again IIUC. so in > > > > that case, time to first contentful paint ends up being time to re-enter > > > > foreground. > > > > > > > > do we have a clean way to track whether the page was backgrounded between > > nav > > > > start and FCP? alternatively we can suppress notifications of FCP if the > > page > > > > was backgrounded on the c++ side, which is straightforward. > > > > > > Thanks for pointing that out! > > > > > > There is a way to know that the page was backgrounded from the hosting > > application, since Custom Tabs relay the information when a page visibility > > changes. > > > > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... > > > File > > chrome/android/java/src/org/chromium/chrome/browser/metrics/PageLoadMetrics.java > > (right): > > > > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... > > > > > chrome/android/java/src/org/chromium/chrome/browser/metrics/PageLoadMetrics.java:46: > > sObservers.remove(observer); > > > On 2016/07/26 14:29:33, Bryan McQuade wrote: > > > > is this safe if a listener removes themselves during a callback dispatch? if > > > > not, is there a java class that handles removal during iteration? > > > > > > Yes, there is ObserverList in Java as well. I deliberately wanted to avoid it > > here for now, to keep things simpler. > > > > > > But coming back to it you're absolutely right, it's better to make this more > > robust right away. > > > Done. > > > > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... > > > File > > chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.cc > > (right): > > > > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... > > > > > chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.cc:12: > > #include "components/page_load_metrics/browser/page_load_metrics_util.h" > > > On 2016/07/26 23:44:27, Bryan McQuade wrote: > > > > same - need to update include path > > > > > > Done. > > > > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... > > > > > chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.cc:25: > > DCHECK_CURRENTLY_ON(content::BrowserThread::UI); > > > On 2016/07/26 14:29:33, Bryan McQuade wrote: > > > > if we want to cases where a page is backgrounded before FCP, you can add: > > > > > > > > if (!WasStartedInForegroundOptionalEventInForeground( > > > > timing.first_contentful_paint, info)) { > > > > return; > > > > } > > > > > > > > (see earlier comment for more reason as to why we might want to do this) > > > > > > I guess there are two ways to look at how to proceed here: > > > (a) Only pass the "raw" events to Java, and let the Java side handle the > > filtering > > > (b) Provide an already filtered metric to the Java side. > > > > > > I erred on the side of option (a) since it's more flexible, but I'm not firmly > > convinced it's the best approach. > > > > > > What do you think? > > > > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... > > > File > > chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.h > > (right): > > > > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... > > > > > chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.h:11: > > #include "components/page_load_metrics/browser/page_load_metrics_observer.h" > > > On 2016/07/26 23:44:27, Bryan McQuade wrote: > > > > i just moved page_load_metrics out of components in > > > > https://codereview.chromium.org/2177743002, so this include path should now > > be > > > > chrome/browser/page_load_metrics/page_load_metrics_observer.h > > > Thanks for pointing that out! > > > Done. > > > > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... > > > > > chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.h:32: > > bool has_seen_first_contentful_paint_; > > > On 2016/07/26 14:29:33, Bryan McQuade wrote: > > > > you can remove this now > > > > > > Done. > > > > Thanks for the fixes! Just wanted to call out one open question - not sure if it > > has been addressed: My main open question is how we're going to make sure that > > we only send the first contentful paint notification for the first navigation > > from the hosting application into the custom tab. We should make sure that we > > never send a notification for subsequent URLs navigated from within the custom > > tab, since the embedder will have no way to distinguish those callbacks from the > > initial page that was navigated to. Do we have logic in place to avoid tracking > > subsequent navs? > > Sorry, it was in the e-mail thread, but not discussed here. Let's correct that. > The embedder receives notifications for page navigations, for each navigation (without the URL, they can only know the URL for the first navigation or when it's explicitely shared). See https://developer.android.com/reference/android/support/customtabs/CustomTabs..., you have NAVIGATION_{STARTED,ABORTED,FAILED,FINISHED} notifications. The clients that are tracking these timings already have to be aware that loading notifications cannot be linked to URL except for the first one (or when it's explicitely shared). > > For symmetry, I preferred to send the page load metrics notifications for all page loads, since we send the other one for all page loads as well. What do you think? Ok, that seems good to me, thanks! Is the availability of this new metrics going to be documented anywhere? If so, I'd like to make sure we note two things in the docs: * consumers should probably ignore this metric if the page load was backgrounded before the metric is reported, since painting is suppressed while backgrounded, and thus the time to first paint becomes meaningless * consumers should ignore any such events reported after a NAVIGATION_ABORTED/FAILED_FINISHED notification is received.
On 2016/07/28 at 13:19:12, Bryan McQuade wrote: > On 2016/07/28 at 11:07:49, lizeb wrote: > > On 2016/07/28 10:58:23, Bryan McQuade wrote: > > > On 2016/07/27 at 16:23:45, lizeb wrote: > > > > Thanks! > > > > All done. > > > > > > > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... > > > > File > > > chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java > > > (right): > > > > > > > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:115: > > > if (webContents != mWebContents) return; > > > > On 2016/07/26 14:29:33, Bryan McQuade wrote: > > > > > do we expect to hit this early return? in what cases might this happen? > > > > > > > > Yes. > > > > > > > > There can be multiple Custom Tabs active at the same time (each having its own > > > WebContents). > > > > > > > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:117: > > > mConnection.notifyPageLoadMetric( > > > > On 2016/07/26 14:29:33, Bryan McQuade wrote: > > > > > we've found that it's less useful to track first contentful paints for pages > > > > > that get backgrounded. in particular, once backgrounded, the rendering > > > pipeline > > > > > gets throttled, which means that even if a page reaches the point where it's > > > > > ready to paint, it won't actually paint until foregrounded again IIUC. so in > > > > > that case, time to first contentful paint ends up being time to re-enter > > > > > foreground. > > > > > > > > > > do we have a clean way to track whether the page was backgrounded between > > > nav > > > > > start and FCP? alternatively we can suppress notifications of FCP if the > > > page > > > > > was backgrounded on the c++ side, which is straightforward. > > > > > > > > Thanks for pointing that out! > > > > > > > > There is a way to know that the page was backgrounded from the hosting > > > application, since Custom Tabs relay the information when a page visibility > > > changes. > > > > > > > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... > > > > File > > > chrome/android/java/src/org/chromium/chrome/browser/metrics/PageLoadMetrics.java > > > (right): > > > > > > > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/android/java/src... > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/metrics/PageLoadMetrics.java:46: > > > sObservers.remove(observer); > > > > On 2016/07/26 14:29:33, Bryan McQuade wrote: > > > > > is this safe if a listener removes themselves during a callback dispatch? if > > > > > not, is there a java class that handles removal during iteration? > > > > > > > > Yes, there is ObserverList in Java as well. I deliberately wanted to avoid it > > > here for now, to keep things simpler. > > > > > > > > But coming back to it you're absolutely right, it's better to make this more > > > robust right away. > > > > Done. > > > > > > > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... > > > > File > > > chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.cc > > > (right): > > > > > > > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... > > > > > > > chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.cc:12: > > > #include "components/page_load_metrics/browser/page_load_metrics_util.h" > > > > On 2016/07/26 23:44:27, Bryan McQuade wrote: > > > > > same - need to update include path > > > > > > > > Done. > > > > > > > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... > > > > > > > chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.cc:25: > > > DCHECK_CURRENTLY_ON(content::BrowserThread::UI); > > > > On 2016/07/26 14:29:33, Bryan McQuade wrote: > > > > > if we want to cases where a page is backgrounded before FCP, you can add: > > > > > > > > > > if (!WasStartedInForegroundOptionalEventInForeground( > > > > > timing.first_contentful_paint, info)) { > > > > > return; > > > > > } > > > > > > > > > > (see earlier comment for more reason as to why we might want to do this) > > > > > > > > I guess there are two ways to look at how to proceed here: > > > > (a) Only pass the "raw" events to Java, and let the Java side handle the > > > filtering > > > > (b) Provide an already filtered metric to the Java side. > > > > > > > > I erred on the side of option (a) since it's more flexible, but I'm not firmly > > > convinced it's the best approach. > > > > > > > > What do you think? > > > > > > > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... > > > > File > > > chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.h > > > (right): > > > > > > > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... > > > > > > > chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.h:11: > > > #include "components/page_load_metrics/browser/page_load_metrics_observer.h" > > > > On 2016/07/26 23:44:27, Bryan McQuade wrote: > > > > > i just moved page_load_metrics out of components in > > > > > https://codereview.chromium.org/2177743002, so this include path should now > > > be > > > > > chrome/browser/page_load_metrics/page_load_metrics_observer.h > > > > Thanks for pointing that out! > > > > Done. > > > > > > > > > > > https://codereview.chromium.org/2139243003/diff/80001/chrome/browser/page_loa... > > > > > > > chrome/browser/page_load_metrics/observers/android_page_load_metrics_observer.h:32: > > > bool has_seen_first_contentful_paint_; > > > > On 2016/07/26 14:29:33, Bryan McQuade wrote: > > > > > you can remove this now > > > > > > > > Done. > > > > > > Thanks for the fixes! Just wanted to call out one open question - not sure if it > > > has been addressed: My main open question is how we're going to make sure that > > > we only send the first contentful paint notification for the first navigation > > > from the hosting application into the custom tab. We should make sure that we > > > never send a notification for subsequent URLs navigated from within the custom > > > tab, since the embedder will have no way to distinguish those callbacks from the > > > initial page that was navigated to. Do we have logic in place to avoid tracking > > > subsequent navs? > > > > Sorry, it was in the e-mail thread, but not discussed here. Let's correct that. > > The embedder receives notifications for page navigations, for each navigation (without the URL, they can only know the URL for the first navigation or when it's explicitely shared). See https://developer.android.com/reference/android/support/customtabs/CustomTabs..., you have NAVIGATION_{STARTED,ABORTED,FAILED,FINISHED} notifications. The clients that are tracking these timings already have to be aware that loading notifications cannot be linked to URL except for the first one (or when it's explicitely shared). > > > > For symmetry, I preferred to send the page load metrics notifications for all page loads, since we send the other one for all page loads as well. What do you think? > > Ok, that seems good to me, thanks! > > Is the availability of this new metrics going to be documented anywhere? If so, I'd like to make sure we note two things in the docs: > * consumers should probably ignore this metric if the page load was backgrounded before the metric is reported, since painting is suppressed while backgrounded, and thus the time to first paint becomes meaningless > * consumers should ignore any such events reported after a NAVIGATION_ABORTED/FAILED_FINISHED notification is received. Actually, thinking about this a bit more, can we ever see a case where reporting FCP after the first page nav completes is useful? If we can't think of a use case, then I am worried that it only adds negative value to report it past the first navigation, as clients have to write more filtering code, and if they don't, they run the risk of recording the wrong metrics, which is arguably worse than recording nothing at all. Do you have a sense for why we report all load events? Is there value in that for any custom tabs implementers? Could we reconsider the decision to report page events like load and FCP beyond the first navigation?
> > > > Ok, that seems good to me, thanks! > > > > Is the availability of this new metrics going to be documented anywhere? If > so, I'd like to make sure we note two things in the docs: > > * consumers should probably ignore this metric if the page load was > backgrounded before the metric is reported, since painting is suppressed while > backgrounded, and thus the time to first paint becomes meaningless > > * consumers should ignore any such events reported after a > NAVIGATION_ABORTED/FAILED_FINISHED notification is received. Yes, the documentation is/will be in the Android Support library (which is part of Android, with its own release schedule). Note that this is blocked by crbug.com/630303 anyway. I will make sure this is specified in the doc (and CC you on the code review) > > Actually, thinking about this a bit more, can we ever see a case where reporting > FCP after the first page nav completes is useful? If we can't think of a use > case, then I am worried that it only adds negative value to report it past the > first navigation, as clients have to write more filtering code, and if they > don't, they run the risk of recording the wrong metrics, which is arguably worse > than recording nothing at all. > Yes, there are a few: - When the initial URL does a redirect, the client app may want to know the load time of the target, and not of the redirection page (for instance, twitter sending to t.co/...) - When the (optional) action button is clicked, the current URL is shared with the embedding app (for instance, this could be a "share" button). In this case, the embedding application may want to know about the page loading performance. - Finally (but it could be done otherwise), the embedding application can trigger a new page load in the same tab. Then it knows about the URL, and would like to have timings. For instance, the action button on the page could be a "next article" button that triggers a new page load in Custom Tabs. The subsequent load is not the first one for the tab, but the embedding application knows the URL. Of course, we could say that every time the embedding application sends a new Intent, we tag the subsequent page load as the "first" one, but this is more heavyweight. > Do you have a sense for why we report all load events? Is there value in that > for any custom tabs implementers? Could we reconsider the decision to report > page events like load and FCP beyond the first navigation? Within the cases above, the redirect one is the most significant to me, and probably justifies keeping things as they currently are. What do you think?
On 2016/07/28 at 13:31:28, lizeb wrote: > > > > > > Ok, that seems good to me, thanks! > > > > > > Is the availability of this new metrics going to be documented anywhere? If > > so, I'd like to make sure we note two things in the docs: > > > * consumers should probably ignore this metric if the page load was > > backgrounded before the metric is reported, since painting is suppressed while > > backgrounded, and thus the time to first paint becomes meaningless > > > * consumers should ignore any such events reported after a > > NAVIGATION_ABORTED/FAILED_FINISHED notification is received. > > Yes, the documentation is/will be in the Android Support library (which is part of Android, with its own release schedule). Note that this is blocked by crbug.com/630303 anyway. > I will make sure this is specified in the doc (and CC you on the code review) > > > > > Actually, thinking about this a bit more, can we ever see a case where reporting > > FCP after the first page nav completes is useful? If we can't think of a use > > case, then I am worried that it only adds negative value to report it past the > > first navigation, as clients have to write more filtering code, and if they > > don't, they run the risk of recording the wrong metrics, which is arguably worse > > than recording nothing at all. > > > > Yes, there are a few: > - When the initial URL does a redirect, the client app may want to know the load time of the target, and not of the redirection page (for instance, twitter sending to t.co/...) > - When the (optional) action button is clicked, the current URL is shared with the embedding app (for instance, this could be a "share" button). In this case, the embedding application may want to know about the page loading performance. > - Finally (but it could be done otherwise), the embedding application can trigger a new page load in the same tab. Then it knows about the URL, and would like to have timings. For instance, the action button on the page could be a "next article" button that triggers a new page load in Custom Tabs. The subsequent load is not the first one for the tab, but the embedding application knows the URL. Of course, we could say that every time the embedding application sends a new Intent, we tag the subsequent page load as the "first" one, but this is more heavyweight. > > > Do you have a sense for why we report all load events? Is there value in that > > for any custom tabs implementers? Could we reconsider the decision to report > > page events like load and FCP beyond the first navigation? > > Within the cases above, the redirect one is the most significant to me, and probably justifies keeping things as they currently are. What do you think? Ah, thanks! I think this is ok. LGTM, thanks!
The CQ bit was checked by lizeb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lizeb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mariakhomenko@chromium.org Link to the patchset: https://codereview.chromium.org/2139243003/#ps100001 (title: "Address comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== customtabs: Send time to first contentful paint to the caller. Time to First Contentful Paint is a very useful metric to assess page load performance. It is collected on the native side. This patch forwards it to the Java side on Android, and makes it available to the embedding app for Custom Tabs. BUG=624909 ========== to ========== customtabs: Send time to first contentful paint to the caller. Time to First Contentful Paint is a very useful metric to assess page load performance. It is collected on the native side. This patch forwards it to the Java side on Android, and makes it available to the embedding app for Custom Tabs. BUG=624909 Committed: https://crrev.com/d6fe19b87ba15a78130ec62d87cb0c2c3bae3eaf Cr-Commit-Position: refs/heads/master@{#408391} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/d6fe19b87ba15a78130ec62d87cb0c2c3bae3eaf Cr-Commit-Position: refs/heads/master@{#408391} |
