chrome/browser/android/tab_android.cc - Issue 2894973002: Provide WebContents::CreateParams to tab helpers.

Unified Diff: chrome/browser/android/tab_android.cc

Issue 2894973002: Provide WebContents::CreateParams to tab helpers. (Closed)

Patch Set: rebase Created 3 years, 7 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

« no previous file with comments | « android_webview/browser/aw_web_contents_delegate.cc ('k') | chrome/browser/android/tab_web_contents_delegate_android.cc » ('j') | chrome/browser/page_load_metrics/metrics_web_contents_observer.cc » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: chrome/browser/android/tab_android.cc

diff --git a/chrome/browser/android/tab_android.cc b/chrome/browser/android/tab_android.cc

index 8b2dad9eaaf917c3a279ccc1ad3ff0dbd96aaa15..005669d1972a18eecc865d99ba76532548b442e1 100644

--- a/chrome/browser/android/tab_android.cc

+++ b/chrome/browser/android/tab_android.cc

@@ -119,7 +119,7 @@ TabAndroid* TabAndroid::GetNativeTab(JNIEnv* env, const JavaRef<jobject>& obj) {

void TabAndroid::AttachTabHelpers(content::WebContents* web_contents) {

DCHECK(web_contents);

- TabHelpers::AttachTabHelpers(web_contents);

+ TabHelpers::AttachTabHelpers(web_contents, base::nullopt);

Avi (use Gerrit) 2017/05/23 15:38:59 Is this not susceptible to the same issue? You may

Bryan McQuade 2017/05/23 20:29:57 This path doesn't appear to be the one that's exer

boliu 2017/05/23 21:16:28 It comes from https://cs.chromium.org/chromium/src

Bryan McQuade 2017/05/25 03:19:12 Copying the earlier discussion into the appropriat

Copying the earlier discussion into the appropriate comment thread, to make it clearer that this has been addressed: The focus of this CL is addressing an issue caused by the specific callpath in chrome::Navigate(). With PlzNavigate enabled, in this specific codepath, for navigations in a new foreground tab, the NavigationHandle is created before the WebContents has been added to the tab strip. This causes page load metrics to infer that the navigation started in the background, since the WC reports that it is not yet foregrounded at the time the navigatoin starts, even though the issue is just an ordering issue: the WebContents is foregrounded, but the code to add it to the tabstrip runs right after the NavigationHandle is created. With this patch, page load metrics is able to ask the CreateParams whether the WC will be initially hidden or not, which allows us to accurately gather the initial foreground / background state of the WC, without being subject to the order of execution issues we face currently. While it would be nice to provide CreateParams for all possible code paths, it's only in chrome::Navigate that there's an execution ordering inversion, so there is not a current need for the CreateParams in any of the other callpaths at this time. I went ahead and added CreateParams to other codepaths where there was no cost to do so (such as holding on to a CreateParams for a longer period of time, increasing memory footprint) as part of this change. Should there be a need to have CreateParams for some of these other codepaths in the future, we can revisit at that time, but until that's the case, we should avoid keeping the CreateParams around (using the UserData approach you suggested for instance) for any longer than needed.

Maria 2017/05/25 05:12:20 So do the metrics work correctly on Android? Is th

So do the metrics work correctly on Android? Is that something that's been checked? On 2017/05/25 03:19:12, Bryan McQuade wrote:

> On 2017/05/23 at 21:16:28, boliu wrote: > > On 2017/05/23 20:29:57, Bryan McQuade wrote: > > > On 2017/05/23 at 15:38:59, Avi (ping after 24h) wrote: > > > > Is this not susceptible to the same issue? You may need to plumb this > further > > > down this call chain... > > > > > > This path doesn't appear to be the one that's exercised for the specific > bug, > > > but I agree it'd be better to provide the CreateParams here if available. > > > > > > Tracing the call chain, this is called from TabAndroid::InitWebContents, > which > > > gets the webcontents via WebContents::FromJavaWebContents (JNI). I wasn't > able > > > to figure out where if at all CreateParams are specified for constructing > these > > > WebContents, and it doesn't appear that they'd be available here given we've > > > crossed the Java/Native boundary. > > > > > > So, I think we're unfortunately unable to get access to the CreateParams > here, > > > as best I can tell. Happy to add support here if there's a way to do it, > though. > > > > It comes from > https://cs.chromium.org/chromium/src/chrome/browser/android/web_contents_fact... > > > > I think probably should figure out a way to plumb the CreateParams here if > it's needed. UserData maybe? > > Copying the earlier discussion into the appropriate comment thread, to make it > clearer that this has been addressed: > > The focus of this CL is addressing an issue caused by the specific callpath in > chrome::Navigate(). With PlzNavigate enabled, in this specific codepath, for > navigations in a new foreground tab, the NavigationHandle is created before the > WebContents has been added to the tab strip. This causes page load metrics to > infer that the navigation started in the background, since the WC reports that > it is not yet foregrounded at the time the navigatoin starts, even though the > issue is just an ordering issue: the WebContents is foregrounded, but the code > to add it to the tabstrip runs right after the NavigationHandle is created. With > this patch, page load metrics is able to ask the CreateParams whether the WC > will be initially hidden or not, which allows us to accurately gather the > initial foreground / background state of the WC, without being subject to the > order of execution issues we face currently. > > While it would be nice to provide CreateParams for all possible code paths, it's > only in chrome::Navigate that there's an execution ordering inversion, so there > is not a current need for the CreateParams in any of the other callpaths at this > time. I went ahead and added CreateParams to other codepaths where there was no > cost to do so (such as holding on to a CreateParams for a longer period of time, > increasing memory footprint) as part of this change. > > Should there be a need to have CreateParams for some of these other codepaths in > the future, we can revisit at that time, but until that's the case, we should > avoid keeping the CreateParams around (using the UserData approach you suggested > for instance) for any longer than needed.

Bryan McQuade 2017/05/26 00:39:44 Metrics work on Android as well. This change inclu

On 2017/05/25 at 05:12:20, Maria wrote: > So do the metrics work correctly on Android? Is that something that's been checked? > > On 2017/05/25 03:19:12, Bryan McQuade wrote: > > On 2017/05/23 at 21:16:28, boliu wrote: > > > On 2017/05/23 20:29:57, Bryan McQuade wrote: > > > > On 2017/05/23 at 15:38:59, Avi (ping after 24h) wrote: > > > > > Is this not susceptible to the same issue? You may need to plumb this > > further > > > > down this call chain... > > > > > > > > This path doesn't appear to be the one that's exercised for the specific > > bug, > > > > but I agree it'd be better to provide the CreateParams here if available. > > > > > > > > Tracing the call chain, this is called from TabAndroid::InitWebContents, > > which > > > > gets the webcontents via WebContents::FromJavaWebContents (JNI). I wasn't > > able > > > > to figure out where if at all CreateParams are specified for constructing > > these > > > > WebContents, and it doesn't appear that they'd be available here given we've > > > > crossed the Java/Native boundary. > > > > > > > > So, I think we're unfortunately unable to get access to the CreateParams > > here, > > > > as best I can tell. Happy to add support here if there's a way to do it, > > though. > > > > > > It comes from > > https://cs.chromium.org/chromium/src/chrome/browser/android/web_contents_fact... > > > > > > I think probably should figure out a way to plumb the CreateParams here if > > it's needed. UserData maybe? > > > > Copying the earlier discussion into the appropriate comment thread, to make it > > clearer that this has been addressed: > > > > The focus of this CL is addressing an issue caused by the specific callpath in > > chrome::Navigate(). With PlzNavigate enabled, in this specific codepath, for > > navigations in a new foreground tab, the NavigationHandle is created before the > > WebContents has been added to the tab strip. This causes page load metrics to > > infer that the navigation started in the background, since the WC reports that > > it is not yet foregrounded at the time the navigatoin starts, even though the > > issue is just an ordering issue: the WebContents is foregrounded, but the code > > to add it to the tabstrip runs right after the NavigationHandle is created. With > > this patch, page load metrics is able to ask the CreateParams whether the WC > > will be initially hidden or not, which allows us to accurately gather the > > initial foreground / background state of the WC, without being subject to the > > order of execution issues we face currently. > > > > While it would be nice to provide CreateParams for all possible code paths, it's > > only in chrome::Navigate that there's an execution ordering inversion, so there > > is not a current need for the CreateParams in any of the other callpaths at this > > time. I went ahead and added CreateParams to other codepaths where there was no > > cost to do so (such as holding on to a CreateParams for a longer period of time, > > increasing memory footprint) as part of this change. > > > > Should there be a need to have CreateParams for some of these other codepaths in > > the future, we can revisit at that time, but until that's the case, we should > > avoid keeping the CreateParams around (using the UserData approach you suggested > > for instance) for any longer than needed.

Metrics work on Android as well. This change includes a unit test for the case fixed by the change, and that test passes on android.

}

TabAndroid::TabAndroid(JNIEnv* env, const JavaRef<jobject>& obj)