Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2931023002:
[TooManyTabs] Add TabNavigationThrottle (Closed)
Description[TooManyTabs] Add TabNavigationThrottle
This is the 1st CL for staggered background tab opening feature. It adds
TabNavigationThrottle, which plumbs navigation information into
TabManager and enables TabManager to defer navigation for background
tabs. This CL uses a simple algorithm, which defers background tab's
navigation when some tab is loading. After a tab finishes loading, it
will resume another pending navigation. TabManager will also resume a
pending navigation when a background tab is selected.
This feature is currently implemented behind a flag. This feature will
not be enabled until the corresponding metrics are ready.
This CL focuses on plumbing navigation into TabManager. The actual
algorithm to delay background tab opening is simple. But it should be
fine because the whole feature is behind a flag right now. There will be
follow-up CLs to consider system resources and refine the algorithm.
Design doc:
https://docs.google.com/document/d/1_CS5kcY00CE3pzDTtW5Ny221gWfk4mFXYGiezoYSCBI/
BUG=730098
Review-Url: https://codereview.chromium.org/2931023002
Cr-Commit-Position: refs/heads/master@{#485870}
Committed: https://chromium.googlesource.com/chromium/src/+/6edd49c516047daa73d069c600bf9b2a09493bb7
Patch Set 1 #
Total comments: 51
Patch Set 2 : review fix #
Total comments: 18
Patch Set 3 : rebase #Patch Set 4 : review fix #
Total comments: 10
Patch Set 5 : rebase #Patch Set 6 : nit fix #Patch Set 7 : add unit tests #Patch Set 8 : rebase #Patch Set 9 : rebase #Patch Set 10 : add more tests #
Total comments: 11
Patch Set 11 : review fix #
Total comments: 4
Patch Set 12 : add comments #Messages
Total messages: 54 (25 generated)
Description was changed from ========== [TooManyTabs] Add TabNavigationThrottle BUG=730098 ========== to ========== [TooManyTabs] Add TabNavigationThrottle This is the 1st CL for staggered background tab opening feature. It adds TabNavigationThrottle, which plumbs navigation information into TabManager and enables TabManager to defer navigation for background tabs. This CL uses a simple algorithm, which defers background tab's navigation when some tab is loading. After a tab finishes loading, it will resume another pending navigation. TabManager will also resume a pending navigation when a background tab is selected. This feature is currently implemented behind a flag. This feature will not be enabled until the corresponding metrics are ready. This CL focuses on plumbing navigation into TabManager. The actual algorithm to delay background tab opening is simple. But it should be fine because the whole feature is behind a flag right now. There will be follow-up CLs to consider system resources and refine the algorithm. Design doc: https://docs.google.com/document/d/1_CS5kcY00CE3pzDTtW5Ny221gWfk4mFXYGiezoYSCBI/ BUG=730098 ==========
zhenw@chromium.org changed reviewers: + chrisha@chromium.org, creis@chromium.org
Charlie and Chris, PTAL. :) Will add tests after figuring out the following 2 problems. Charlie, I have some question about some corner cases: 1. navigation may be canceled 2. pop-out window For the 1st one, how do I know when a navigation is canceled? Will that lead to NavigationThrottle being destructed and I can just handle it in TabNavigationThrottle's destructor? For the 2nd one, is there a way to get a WebContents' parent or child? I do not find any function in WebContents for this purpose.
creis@chromium.org changed reviewers: + kenrb@chromium.org, nasko@chromium.org
[+nasko, kenrb] Thanks-- this seems like a good start. I'd like to get Nasko to review it as well since he's probably better aware of best practices with NavigationThrottles, but I've left some feedback in the meantime. Ken, one question for you about tab visibility below. On 2017/06/09 01:12:06, Zhen Wang wrote: > Charlie and Chris, PTAL. :) > > Will add tests after figuring out the following 2 problems. > > Charlie, I have some question about some corner cases: > 1. navigation may be canceled > 2. pop-out window > > For the 1st one, how do I know when a navigation is canceled? Will that lead to > NavigationThrottle being destructed and I can just handle it in > TabNavigationThrottle's destructor? Yes, I think the throttle will be deleted if the navigation fails. Nasko, is that right? > For the 2nd one, is there a way to get a WebContents' parent or child? I do not > find any function in WebContents for this purpose. Parent/child isn't quite the right terminology. I think you're looking for GetOpener(), to ensure it's null. I left a few thoughts on additional criteria below. https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_manager.cc (right): https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.cc:897: switches::kStaggeredBackgroundTabOpen)) { We shouldn't be creating the throttle if this flag isn't present. https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.cc:901: if (!navigation_handle) Is this possible? https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.cc:905: if (navigation_handle->GetWebContents()->IsVisible()) This also seems like it could be checked before creating the throttle. https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.cc:936: loading_contents_ = navigation_handle->GetWebContents(); Keep in mind that there may have been an existing loading tab, and we aren't keeping track of it after this. That means that the next deferred tab will start loading as soon as |contents| finishes, even if the previous |loading_contents_| is still loading (for an arbitrary number of previous tabs, if the user cycles through a few tabs). We'd obviously want to stop deferring any tab the user visits, but it does mean that we'll start loading additional background tabs earlier than expected. Maybe that's ok, for the sake of simplicity? https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_manager.h (right): https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.h:66: // support for new platforms is added. We should probably add a description of the new behavior being added. https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.h:168: // can be easily reloaded and hence makes a good choice to discard. We may not need to expose this; see my question about the call site. https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc (right): https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc:40: // TODO(zhenw): Switch to use tab done loading signal when it is ready. What does this refer to? Is there a bug number you can cite? https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_navigation_throttle.cc (right): https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_navigation_throttle.cc:22: // chrome:// UI URLs, for main frames. I'm curious, what's the motivation for this? We've found that SchemeIsHTTPOrHTTPS is often the wrong thing to be asking-- it excludes a lot of related cases like filesystem: and blob: URLs, view-source:, file:, etc. What sort of things are you intending to exclude, and why? https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_navigation_throttle.cc:37: return base::MakeUnique<TabNavigationThrottle>(navigation_handle); It seems like we're creating this throttle in a lot more cases than necessary. Right now it looks like almost all main frame navigations (with a few exceptions based on destination URL), but my impression is that it should only be for the first navigation in a newly created tab, in a previously unused SiteInstance, with no opener. That was my understanding from the design doc, anyway. Would it make sense to narrow down to some of these criteria, using navigation_handle->GetWebContents()? You could ask the WebContents about GetOpener() and GetController().IsInitialNavigation(). We might be able to check SiteInstanceImpl::active_frame_count() as well. https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_navigation_throttle.h (right): https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_navigation_throttle.h:16: // TabManager to control navigation for background tabs. Can you elaborate on this a bit more? "Controlling navigation" sounds broad, but this is just about delaying navigations to reduce contention. https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_navigation_throttle.h:17: class TabNavigationThrottle : public content::NavigationThrottle { This name seems too broad, since most navigation throttles are about navigations in tabs. Would BackgroundTabNavigationThrottle be more accurate? https://codereview.chromium.org/2931023002/diff/1/chrome/common/chrome_switch... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/2931023002/diff/1/chrome/common/chrome_switch... chrome/common/chrome_switches.cc:799: const char kStaggeredBackgroundTabOpen[] = "staggered-background-tab-open"; Please document what this does. https://codereview.chromium.org/2931023002/diff/1/content/public/browser/web_... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/2931023002/diff/1/content/public/browser/web_... content/public/browser/web_contents.h:445: virtual bool IsVisible() const = 0; I think there's probably a better way to check this. Ken, do you know?
https://codereview.chromium.org/2931023002/diff/1/content/public/browser/web_... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/2931023002/diff/1/content/public/browser/web_... content/public/browser/web_contents.h:445: virtual bool IsVisible() const = 0; On 2017/06/10 00:53:37, Charlie Reis wrote: > I think there's probably a better way to check this. Ken, do you know? The call site should be able to use: navigation_handle->GetWebContents()->GetTopLevelRenderWidgetHostView()->IsShowing();
https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_manager.cc (right): https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.cc:909: if (!loading_contents_) { Wouldn't it be simpler to just check the pending_navigations_ size for 0? Or is the goal to keep background WebContents delayed until they are made visible? If we just delay them, so that we don't overload the system, but then resume them once we have resources, we might not need to keep track of the currently loading WebContents. https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.cc:935: if (navigation_handle && navigation_handle->GetWebContents() == contents) { We better ensure we don't put nullptr into the pending_navigations_ list. It is much easier to skip those null checks. https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_manager.h (right): https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.h:316: bool ShouldThrottleNavigation( nit: s/Throttle/Delay/ as in this case we only care about delaying the navigation for later. https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc (right): https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc:39: void TabManager::WebContentsData::DidStopLoading() { Just FYI, DidStartLoading and DidStopLoading can be fired if an iframe navigates inside a WebContents, so it doesn't imply the main document is navigating away. https://codereview.chromium.org/2931023002/diff/1/chrome/common/chrome_switch... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/2931023002/diff/1/chrome/common/chrome_switch... chrome/common/chrome_switches.cc:799: const char kStaggeredBackgroundTabOpen[] = "staggered-background-tab-open"; On 2017/06/10 00:53:37, Charlie Reis wrote: > Please document what this does. Also, should it be a command line flag or a base::Feature? The latter is much easier to work with and especially easy to field trial, which I imagine we would like to do at some point. https://codereview.chromium.org/2931023002/diff/1/content/public/browser/web_... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/2931023002/diff/1/content/public/browser/web_... content/public/browser/web_contents.h:445: virtual bool IsVisible() const = 0; On 2017/06/12 15:26:22, kenrb wrote: > On 2017/06/10 00:53:37, Charlie Reis wrote: > > I think there's probably a better way to check this. Ken, do you know? > > The call site should be able to use: > navigation_handle->GetWebContents()->GetTopLevelRenderWidgetHostView()->IsShowing(); Doesn't that distribute implementation logic outside of WebContents? Is there a downside to wrapping this call into a "IsShowing()" method on the WebContents itself?
Thanks for the review! I uploaded a new patch. PTAL https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_manager.cc (right): https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.cc:897: switches::kStaggeredBackgroundTabOpen)) { On 2017/06/10 00:53:36, Charlie Reis wrote: > We shouldn't be creating the throttle if this flag isn't present. Done. https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.cc:901: if (!navigation_handle) On 2017/06/10 00:53:36, Charlie Reis wrote: > Is this possible? I am not sure if this is ever possible. That's why I check it here. Should I use a DCHECK? https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.cc:905: if (navigation_handle->GetWebContents()->IsVisible()) On 2017/06/10 00:53:36, Charlie Reis wrote: > This also seems like it could be checked before creating the throttle. Done. https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.cc:909: if (!loading_contents_) { On 2017/06/12 23:22:02, nasko (slow) wrote: > Wouldn't it be simpler to just check the pending_navigations_ size for 0? Or is > the goal to keep background WebContents delayed until they are made visible? If > we just delay them, so that we don't overload the system, but then resume them > once we have resources, we might not need to keep track of the currently loading > WebContents. |pending_navigations_| tracks which tabs has not sent out requests. |loading_contents_| is the tab which has sent out request and started loading. There could be a case where |pending_navigations_| is empty and there is some tab loading. In this case, we want to delay the next tab's navigation. A smarter way is to allow loading the next background tab when all loading tabs have finished their work. I added a TODO to |loading_contents_| in the header file. This CL tries to focus on getting the navigation part correct. We will have follow-up CLs to make the algorithm smarter. https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.cc:935: if (navigation_handle && navigation_handle->GetWebContents() == contents) { On 2017/06/12 23:22:02, nasko (slow) wrote: > We better ensure we don't put nullptr into the pending_navigations_ list. It is > much easier to skip those null checks. When a navigation gets cancelled, will the corresponding navigation handle be deleted? Or should I explicitly track that when the NavigationThrottle is destructed and notify TabManager to clean up? https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.cc:936: loading_contents_ = navigation_handle->GetWebContents(); On 2017/06/10 00:53:36, Charlie Reis wrote: > Keep in mind that there may have been an existing loading tab, and we aren't > keeping track of it after this. That means that the next deferred tab will > start loading as soon as |contents| finishes, even if the previous > |loading_contents_| is still loading (for an arbitrary number of previous tabs, > if the user cycles through a few tabs). > > We'd obviously want to stop deferring any tab the user visits, but it does mean > that we'll start loading additional background tabs earlier than expected. > Maybe that's ok, for the sake of simplicity? You are right. I think the best way is to know how many tabs are in loading state. And do not start loading any background tab until all loading tabs are finished. We are currently enhancing TabManager to know the loading states of a tab (https://crrev.com/2936003002/). This CL tries to just use some simple algorithm and focus on navigation part. We will have follow-up CLs to make the algorithm smarter. I added a TODO to loading_contents_ in the header file. https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_manager.h (right): https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.h:66: // support for new platforms is added. On 2017/06/10 00:53:36, Charlie Reis wrote: > We should probably add a description of the new behavior being added. Done. https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.h:316: bool ShouldThrottleNavigation( On 2017/06/12 23:22:02, nasko (slow) wrote: > nit: s/Throttle/Delay/ as in this case we only care about delaying the > navigation for later. Done. https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc (right): https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc:39: void TabManager::WebContentsData::DidStopLoading() { On 2017/06/12 23:22:02, nasko (slow) wrote: > Just FYI, DidStartLoading and DidStopLoading can be fired if an iframe navigates > inside a WebContents, so it doesn't imply the main document is navigating away. Thanks for pointing this out! I switched to use DidStartNavigation, which contains navigation_handle, so we can check if it is main frame. DidStopLoading here is fine as a temporary solution. We will switch to use tab done loading signal later. https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc:40: // TODO(zhenw): Switch to use tab done loading signal when it is ready. On 2017/06/10 00:53:36, Charlie Reis wrote: > What does this refer to? Is there a bug number you can cite? It is actually using the same bug number as this CL. lpy@ is actively working on that right now and here is a design doc: https://docs.google.com/document/d/1l98CsGcTqSTLDsYTJsdj5-_Pn1yq03eqYuvF-0F_F0k/ https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_navigation_throttle.cc (right): https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_navigation_throttle.cc:22: // chrome:// UI URLs, for main frames. On 2017/06/10 00:53:36, Charlie Reis wrote: > I'm curious, what's the motivation for this? > > We've found that SchemeIsHTTPOrHTTPS is often the wrong thing to be asking-- it > excludes a lot of related cases like filesystem: and blob: URLs, view-source:, > file:, etc. What sort of things are you intending to exclude, and why? My actual intention is to exclude all non-tab navigations, e.g., extension. But I am not sure if filtering extensions:// is enough. Do you know what is the correct filter for tab navigation? https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_navigation_throttle.cc:37: return base::MakeUnique<TabNavigationThrottle>(navigation_handle); On 2017/06/10 00:53:36, Charlie Reis wrote: > It seems like we're creating this throttle in a lot more cases than necessary. > Right now it looks like almost all main frame navigations (with a few exceptions > based on destination URL), but my impression is that it should only be for the > first navigation in a newly created tab, in a previously unused SiteInstance, > with no opener. That was my understanding from the design doc, anyway. > > Would it make sense to narrow down to some of these criteria, using > navigation_handle->GetWebContents()? You could ask the WebContents about > GetOpener() and GetController().IsInitialNavigation(). We might be able to > check SiteInstanceImpl::active_frame_count() as well. Thanks for the pointers! Added restriction on first navigation and opener case. Why do we check SiteInstanceImpl::active_frame_count()? What is this use case? https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_navigation_throttle.h (right): https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_navigation_throttle.h:16: // TabManager to control navigation for background tabs. On 2017/06/10 00:53:36, Charlie Reis wrote: > Can you elaborate on this a bit more? "Controlling navigation" sounds broad, > but this is just about delaying navigations to reduce contention. I replaced "control" with "delay". We indeed just consider delaying the navigation. https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_navigation_throttle.h:17: class TabNavigationThrottle : public content::NavigationThrottle { On 2017/06/10 00:53:36, Charlie Reis wrote: > This name seems too broad, since most navigation throttles are about navigations > in tabs. Would BackgroundTabNavigationThrottle be more accurate? SG. Renamed. https://codereview.chromium.org/2931023002/diff/1/chrome/common/chrome_switch... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/2931023002/diff/1/chrome/common/chrome_switch... chrome/common/chrome_switches.cc:799: const char kStaggeredBackgroundTabOpen[] = "staggered-background-tab-open"; On 2017/06/12 23:22:02, nasko (slow) wrote: > On 2017/06/10 00:53:37, Charlie Reis wrote: > > Please document what this does. > > Also, should it be a command line flag or a base::Feature? The latter is much > easier to work with and especially easy to field trial, which I imagine we would > like to do at some point. Ah, right. I changed it to base::Feature now. https://codereview.chromium.org/2931023002/diff/1/content/public/browser/web_... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/2931023002/diff/1/content/public/browser/web_... content/public/browser/web_contents.h:445: virtual bool IsVisible() const = 0; On 2017/06/12 23:22:02, nasko (slow) wrote: > On 2017/06/12 15:26:22, kenrb wrote: > > On 2017/06/10 00:53:37, Charlie Reis wrote: > > > I think there's probably a better way to check this. Ken, do you know? > > > > The call site should be able to use: > > > navigation_handle->GetWebContents()->GetTopLevelRenderWidgetHostView()->IsShowing(); > > Doesn't that distribute implementation logic outside of WebContents? Is there a > downside to wrapping this call into a "IsShowing()" method on the WebContents > itself? This is one option for us to know if a tab is visible. Since TabManager is also observing TabStripModel, we can also track tab strip events explicitly. But I think it is better to ask WebContents for this information directly. I thought IsVisible() or IsShowing() would be some property WebContents should provide. Is there any downside of adding this? By the way, is |should_normally_be_visible_| the same as GetTopLevelRenderWidgetHostView()->IsShowing()?
https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_manager.cc (right): https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.cc:935: if (navigation_handle && navigation_handle->GetWebContents() == contents) { On 2017/06/13 23:33:20, Zhen Wang wrote: > On 2017/06/12 23:22:02, nasko (slow) wrote: > > We better ensure we don't put nullptr into the pending_navigations_ list. It > is > > much easier to skip those null checks. > > When a navigation gets cancelled, will the corresponding navigation handle be > deleted? Or should I explicitly track that when the NavigationThrottle is > destructed and notify TabManager to clean up? The WebContentsObserver::DidFinishNavigation will be dispatched, indicating that the navigation didn't commit. Since NavigationHandle owns all the throttles, they will be destroyed after the DidFinishNavigation notification is delivered. Your code should be equipped to handle that case. You already listen to DidStartNavigation, so I would suggest adding a DidFinishNavigation implementation that removes the handle from the pending_navigations_ list. The implementation might do various checks based on success/failure of the navigation, but unconditionally should remove the handle from the list. https://codereview.chromium.org/2931023002/diff/1/content/public/browser/web_... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/2931023002/diff/1/content/public/browser/web_... content/public/browser/web_contents.h:445: virtual bool IsVisible() const = 0; On 2017/06/13 23:33:21, Zhen Wang wrote: > On 2017/06/12 23:22:02, nasko (slow) wrote: > > On 2017/06/12 15:26:22, kenrb wrote: > > > On 2017/06/10 00:53:37, Charlie Reis wrote: > > > > I think there's probably a better way to check this. Ken, do you know? > > > > > > The call site should be able to use: > > > > > > navigation_handle->GetWebContents()->GetTopLevelRenderWidgetHostView()->IsShowing(); > > > > Doesn't that distribute implementation logic outside of WebContents? Is there > a > > downside to wrapping this call into a "IsShowing()" method on the WebContents > > itself? > > This is one option for us to know if a tab is visible. Since TabManager is also > observing TabStripModel, we can also track tab strip events explicitly. > > But I think it is better to ask WebContents for this information directly. I > thought IsVisible() or IsShowing() would be some property WebContents should > provide. Is there any downside of adding this? > > By the way, is |should_normally_be_visible_| the same as > GetTopLevelRenderWidgetHostView()->IsShowing()? I defer to kenrb@ on this one. https://codereview.chromium.org/2931023002/diff/20001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager.h (right): https://codereview.chromium.org/2931023002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager.h:394: // TODO(zhenw): This is a native algorithm. It should be updated to use nit: Did you mean "naive" instead of "native"?
https://codereview.chromium.org/2931023002/diff/1/content/public/browser/web_... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/2931023002/diff/1/content/public/browser/web_... content/public/browser/web_contents.h:445: virtual bool IsVisible() const = 0; On 2017/06/13 23:33:21, Zhen Wang wrote: > On 2017/06/12 23:22:02, nasko (slow) wrote: > > On 2017/06/12 15:26:22, kenrb wrote: > > > On 2017/06/10 00:53:37, Charlie Reis wrote: > > > > I think there's probably a better way to check this. Ken, do you know? > > > > > > The call site should be able to use: > > > > > > navigation_handle->GetWebContents()->GetTopLevelRenderWidgetHostView()->IsShowing(); > > > > Doesn't that distribute implementation logic outside of WebContents? Is there > a > > downside to wrapping this call into a "IsShowing()" method on the WebContents > > itself? > > This is one option for us to know if a tab is visible. Since TabManager is also > observing TabStripModel, we can also track tab strip events explicitly. > > But I think it is better to ask WebContents for this information directly. I > thought IsVisible() or IsShowing() would be some property WebContents should > provide. Is there any downside of adding this? > > By the way, is |should_normally_be_visible_| the same as > GetTopLevelRenderWidgetHostView()->IsShowing()? Adding that method might make sense here. The only divergence between should_normally_be_visible_ and RWHV::IsShowing(), as far as I am aware, is that IsShowing() will return false while a top-level page is still loading, even though should_normally_be_visible_ might return true. Given what you are using it for, the latter seems like it is more relevant.
https://codereview.chromium.org/2931023002/diff/20001/chrome/browser/resource... File chrome/browser/resource_coordinator/background_tab_navigation_throttle.cc (right): https://codereview.chromium.org/2931023002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/background_tab_navigation_throttle.cc:43: // chrome:// UI URLs, for main frames. I would suggest something even stronger: chrome:// tabs should actually just be permanently delayed until made visible, as they don't require any remote data at all. https://codereview.chromium.org/2931023002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/background_tab_navigation_throttle.cc:50: // Do not delay the NTP, which in some cases has an HTTPS URL. Why? If an NTP is opened as a background tab it should be delayed indefinitely, much like any other chrome:// tab (we should arrange to set the title somehow, however). https://codereview.chromium.org/2931023002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/background_tab_navigation_throttle.cc:57: Where are we detecting if the new tab is in the background or not? Couldn't we skip creating the throttle entirely in this case? https://codereview.chromium.org/2931023002/diff/20001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager.h (right): https://codereview.chromium.org/2931023002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager.h:318: // Returns true if the navigation should be delay. delayed* https://codereview.chromium.org/2931023002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager.h:396: content::WebContents* loading_contents_; We have loading states being worked on in another CL. This is duplicating some of that effort (ie, the HasFinishedLoadingTab), and maybe these CLs should be stacked? I think we need to maintain a few lists of tabs internally, and manage transitions of tabs between these states (or decorate each webcontents with the state...) pending_load (background tab opening, session restore created, mass bookmark folder opening, etc) loading loaded discarded (We'll have to add other states as lifecycles come online) https://codereview.chromium.org/2931023002/diff/20001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.h (right): https://codereview.chromium.org/2931023002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:119: bool has_finished_loading; Another CL is adding a loading state... can this work be stacked on top of that? Otherwise we'll have multiple mechanisms for doing the same thing...
Thanks! I uploaded a new patch. PTAL. https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_manager.cc (right): https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.cc:901: if (!navigation_handle) On 2017/06/13 23:33:20, Zhen Wang wrote: > On 2017/06/10 00:53:36, Charlie Reis wrote: > > Is this possible? > > I am not sure if this is ever possible. That's why I check it here. Should I use > a DCHECK? Ping https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.cc:935: if (navigation_handle && navigation_handle->GetWebContents() == contents) { On 2017/06/15 20:01:58, nasko (slow) wrote: > On 2017/06/13 23:33:20, Zhen Wang wrote: > > On 2017/06/12 23:22:02, nasko (slow) wrote: > > > We better ensure we don't put nullptr into the pending_navigations_ list. It > > is > > > much easier to skip those null checks. > > > > When a navigation gets cancelled, will the corresponding navigation handle be > > deleted? Or should I explicitly track that when the NavigationThrottle is > > destructed and notify TabManager to clean up? > > The WebContentsObserver::DidFinishNavigation will be dispatched, indicating that > the navigation didn't commit. Since NavigationHandle owns all the throttles, > they will be destroyed after the DidFinishNavigation notification is delivered. > Your code should be equipped to handle that case. You already listen to > DidStartNavigation, so I would suggest adding a DidFinishNavigation > implementation that removes the handle from the pending_navigations_ list. The > implementation might do various checks based on success/failure of the > navigation, but unconditionally should remove the handle from the list. Thanks! Now observing DidFinishNavigation. https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_navigation_throttle.cc (right): https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_navigation_throttle.cc:22: // chrome:// UI URLs, for main frames. On 2017/06/13 23:33:21, Zhen Wang wrote: > On 2017/06/10 00:53:36, Charlie Reis wrote: > > I'm curious, what's the motivation for this? > > > > We've found that SchemeIsHTTPOrHTTPS is often the wrong thing to be asking-- > it > > excludes a lot of related cases like filesystem: and blob: URLs, view-source:, > > file:, etc. What sort of things are you intending to exclude, and why? > > My actual intention is to exclude all non-tab navigations, e.g., extension. But > I am not sure if filtering extensions:// is enough. Do you know what is the > correct filter for tab navigation? Ping. https://codereview.chromium.org/2931023002/diff/1/content/public/browser/web_... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/2931023002/diff/1/content/public/browser/web_... content/public/browser/web_contents.h:445: virtual bool IsVisible() const = 0; > Adding that method might make sense here. The only divergence between > should_normally_be_visible_ and RWHV::IsShowing(), as far as I am aware, is that > IsShowing() will return false while a top-level page is still loading, even > though should_normally_be_visible_ might return true. Given what you are using > it for, the latter seems like it is more relevant. Right. We will need to know if it is visible before loading. For here, I will keep the name |IsVisible()|, so to distinguish from the |IsShown()|. Let me know if any new name is better. https://codereview.chromium.org/2931023002/diff/20001/chrome/browser/resource... File chrome/browser/resource_coordinator/background_tab_navigation_throttle.cc (right): https://codereview.chromium.org/2931023002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/background_tab_navigation_throttle.cc:43: // chrome:// UI URLs, for main frames. On 2017/06/15 21:02:24, chrisha wrote: > I would suggest something even stronger: > > chrome:// tabs should actually just be permanently delayed until made visible, > as they don't require any remote data at all. That sounds good to me. For this CL, it tries to focus on the navigation part. The algorithm used is very simple. I will have follow-up CLs to use better algorithms. This will probably be updated to use a different filtering method. I am waiting for Charlie's reply on this. https://codereview.chromium.org/2931023002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/background_tab_navigation_throttle.cc:50: // Do not delay the NTP, which in some cases has an HTTPS URL. On 2017/06/15 21:02:24, chrisha wrote: > Why? If an NTP is opened as a background tab it should be delayed indefinitely, > much like any other chrome:// tab (we should arrange to set the title somehow, > however). I was copying this part from another navigation throttle. But I think your point makes sense. For the title, I am not quite sure how to do it. Because before loading, we do not even know the title and favicon. https://codereview.chromium.org/2931023002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/background_tab_navigation_throttle.cc:57: On 2017/06/15 21:02:24, chrisha wrote: > Where are we detecting if the new tab is in the background or not? Couldn't we > skip creating the throttle entirely in this case? This is done above after the feature flag check (line 27). https://codereview.chromium.org/2931023002/diff/20001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager.h (right): https://codereview.chromium.org/2931023002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager.h:318: // Returns true if the navigation should be delay. On 2017/06/15 21:02:24, chrisha wrote: > delayed* Done. https://codereview.chromium.org/2931023002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager.h:394: // TODO(zhenw): This is a native algorithm. It should be updated to use On 2017/06/15 20:01:58, nasko (slow) wrote: > nit: Did you mean "naive" instead of "native"? Right. Updated to naive. Thanks for catching this! https://codereview.chromium.org/2931023002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager.h:396: content::WebContents* loading_contents_; On 2017/06/15 21:02:24, chrisha wrote: > We have loading states being worked on in another CL. This is duplicating some > of that effort (ie, the HasFinishedLoadingTab), and maybe these CLs should be > stacked? > > I think we need to maintain a few lists of tabs internally, and manage > transitions of tabs between these states (or decorate each webcontents with the > state...) > > pending_load (background tab opening, session restore created, mass bookmark > folder opening, etc) > loading > loaded > discarded > > (We'll have to add other states as lifecycles come online) Now I have rebased this CL on the other CL and I changed this to a list to track all currently loading tabs. We can add other lists gradually later. https://codereview.chromium.org/2931023002/diff/20001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.h (right): https://codereview.chromium.org/2931023002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:119: bool has_finished_loading; On 2017/06/15 21:02:24, chrisha wrote: > Another CL is adding a loading state... can this work be stacked on top of that? > Otherwise we'll have multiple mechanisms for doing the same thing... I agree. I intended to use the other CL's loading state. This was uploaded before that one was ready. Now removed.
Just a couple of nits. https://codereview.chromium.org/2931023002/diff/60001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager.h (right): https://codereview.chromium.org/2931023002/diff/60001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager.h:159: // clean up the navigation handles bookkept before. nit: s/navigation handles/NavigationHandle objects/ https://codereview.chromium.org/2931023002/diff/60001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager.h:340: // Remove the pending navigation for the provided web contents. Return the nit: s/web contents/WebContents/
This was mostly looking good, but I just learned that NavThrottles only come into play *after* a backing renderer process has been created for a tab. This seems like too little too late... maybe we should go back to the needs_reload plan? https://codereview.chromium.org/2931023002/diff/20001/chrome/browser/resource... File chrome/browser/resource_coordinator/background_tab_navigation_throttle.cc (right): https://codereview.chromium.org/2931023002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/background_tab_navigation_throttle.cc:50: // Do not delay the NTP, which in some cases has an HTTPS URL. On 2017/06/19 23:00:12, Zhen Wang wrote: > On 2017/06/15 21:02:24, chrisha wrote: > > Why? If an NTP is opened as a background tab it should be delayed > indefinitely, > > much like any other chrome:// tab (we should arrange to set the title somehow, > > however). > > I was copying this part from another navigation throttle. But I think your point > makes sense. > > For the title, I am not quite sure how to do it. Because before loading, we do > not even know the title and favicon. Take a peek at what session restore does, it might work here as well: https://cs.chromium.org/chromium/src/components/favicon/core/favicon_driver_i... I'd say we should always be setting the favicon when possible, even if the load is deferred. I'm not sure if there's a similar way for us to set the expected title... maybe from the history database? I'd at least leave a TODO to explore this. https://codereview.chromium.org/2931023002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/background_tab_navigation_throttle.cc:57: On 2017/06/19 23:00:12, Zhen Wang wrote: > On 2017/06/15 21:02:24, chrisha wrote: > > Where are we detecting if the new tab is in the background or not? Couldn't we > > skip creating the throttle entirely in this case? > > This is done above after the feature flag check (line 27). Clearly I'm blind :) https://codereview.chromium.org/2931023002/diff/20001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager.h (right): https://codereview.chromium.org/2931023002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager.h:396: content::WebContents* loading_contents_; On 2017/06/19 23:00:12, Zhen Wang wrote: > On 2017/06/15 21:02:24, chrisha wrote: > > We have loading states being worked on in another CL. This is duplicating some > > of that effort (ie, the HasFinishedLoadingTab), and maybe these CLs should be > > stacked? > > > > I think we need to maintain a few lists of tabs internally, and manage > > transitions of tabs between these states (or decorate each webcontents with > the > > state...) > > > > pending_load (background tab opening, session restore created, mass bookmark > > folder opening, etc) > > loading > > loaded > > discarded > > > > (We'll have to add other states as lifecycles come online) > > Now I have rebased this CL on the other CL and I changed this to a list to track > all currently loading tabs. We can add other lists gradually later. sgtm https://codereview.chromium.org/2931023002/diff/60001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager.cc (right): https://codereview.chromium.org/2931023002/diff/60001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager.cc:928: Remove blank line. https://codereview.chromium.org/2931023002/diff/60001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager.cc:935: Remove blank line. https://codereview.chromium.org/2931023002/diff/60001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager.h (right): https://codereview.chromium.org/2931023002/diff/60001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager.h:341: // removed navigation handle. Return nullptr if not exists. if it doesn't exist.
On 2017/06/20 18:26:54, chrisha wrote: > This was mostly looking good, but I just learned that NavThrottles only come > into play *after* a backing renderer process has been created for a tab. This > seems like too little too late... maybe we should go back to the needs_reload > plan? Yes, process is allocated before NavThrottles kick in, which is mostly by design. With PlzNavigate, we actually have the capability of not launching the process until we unblock the navigation and have it ready to commit, however it is not the default behavior. With the current navigation code, it isn't even possible. With PlzNavigate, we start a process creation in parallel with the network request, but we do it to avoid the time taken to create a process after we receive the data from the network. It is quite possible that we don't create this speculative process if the tab is backgrounded. Maybe we can make it even configurable through ContentBrowserClient or WebContentsDelegate, or fully encapsulated inside content. There are some options : ), but let's discuss those in email, rather than code review comments.
The CQ bit was checked by zhenw@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: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by zhenw@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: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
Given that we've decided (email thread) to continue down this path, is there anything else that needs to happen in this CL? Or is it good to go as is?
On 2017/07/05 17:43:34, chrisha wrote: > Given that we've decided (email thread) to continue down this path, is there > anything else that needs to happen in this CL? Or is it good to go as is? I added unit tests to the throttle. And I am adding tests to TabManager now. Will upload a new patch and request reviews today or tomorrow. Thanks for checking! :)
The CQ bit was checked by zhenw@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...
I uploaded a new patch with more tests. PTAL :) https://codereview.chromium.org/2931023002/diff/20001/chrome/browser/resource... File chrome/browser/resource_coordinator/background_tab_navigation_throttle.cc (right): https://codereview.chromium.org/2931023002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/background_tab_navigation_throttle.cc:50: // Do not delay the NTP, which in some cases has an HTTPS URL. On 2017/06/20 18:26:54, chrisha wrote: > On 2017/06/19 23:00:12, Zhen Wang wrote: > > On 2017/06/15 21:02:24, chrisha wrote: > > > Why? If an NTP is opened as a background tab it should be delayed > > indefinitely, > > > much like any other chrome:// tab (we should arrange to set the title > somehow, > > > however). > > > > I was copying this part from another navigation throttle. But I think your > point > > makes sense. > > > > For the title, I am not quite sure how to do it. Because before loading, we do > > not even know the title and favicon. > > Take a peek at what session restore does, it might work here as well: > > https://cs.chromium.org/chromium/src/components/favicon/core/favicon_driver_i... > > I'd say we should always be setting the favicon when possible, even if the load > is deferred. > > I'm not sure if there's a similar way for us to set the expected title... maybe > from the history database? I'd at least leave a TODO to explore this. I added a TODO for this in tab_manager.cc. https://codereview.chromium.org/2931023002/diff/60001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager.cc (right): https://codereview.chromium.org/2931023002/diff/60001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager.cc:928: On 2017/06/20 18:26:54, chrisha wrote: > Remove blank line. Done. https://codereview.chromium.org/2931023002/diff/60001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager.cc:935: On 2017/06/20 18:26:54, chrisha wrote: > Remove blank line. Done. https://codereview.chromium.org/2931023002/diff/60001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager.h (right): https://codereview.chromium.org/2931023002/diff/60001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager.h:159: // clean up the navigation handles bookkept before. On 2017/06/20 15:59:39, nasko (slow) wrote: > nit: s/navigation handles/NavigationHandle objects/ Done. https://codereview.chromium.org/2931023002/diff/60001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager.h:340: // Remove the pending navigation for the provided web contents. Return the On 2017/06/20 15:59:39, nasko (slow) wrote: > nit: s/web contents/WebContents/ Done. https://codereview.chromium.org/2931023002/diff/60001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager.h:341: // removed navigation handle. Return nullptr if not exists. On 2017/06/20 18:26:54, chrisha wrote: > if it doesn't exist. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Sorry-- I got sidetracked for quite a while with traveling and other issues. I also didn't realize you'd pinged me for replies since it was mixed in with all the other discussions. Please feel free to ping me on chat if you're waiting for me in the future! A few replies and questions below. https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_manager.cc (right): https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.cc:901: if (!navigation_handle) On 2017/06/19 23:00:12, Zhen Wang wrote: > On 2017/06/13 23:33:20, Zhen Wang wrote: > > On 2017/06/10 00:53:36, Charlie Reis wrote: > > > Is this possible? > > > > I am not sure if this is ever possible. That's why I check it here. Should I > use > > a DCHECK? > > Ping Let's remove it if we don't know that it's needed. Adding unnecessary null checks makes it harder to reason about how the code is meant to be used. https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_manager.h (right): https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.h:168: // can be easily reloaded and hence makes a good choice to discard. On 2017/06/10 00:53:36, Charlie Reis (slow) wrote: > We may not need to expose this; see my question about the call site. I'm still hoping we don't need this. (See https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo...) https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_navigation_throttle.cc (right): https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_navigation_throttle.cc:22: // chrome:// UI URLs, for main frames. On 2017/06/19 23:00:12, Zhen Wang wrote: > On 2017/06/13 23:33:21, Zhen Wang wrote: > > On 2017/06/10 00:53:36, Charlie Reis wrote: > > > I'm curious, what's the motivation for this? > > > > > > We've found that SchemeIsHTTPOrHTTPS is often the wrong thing to be asking-- > > it > > > excludes a lot of related cases like filesystem: and blob: URLs, > view-source:, > > > file:, etc. What sort of things are you intending to exclude, and why? > > > > My actual intention is to exclude all non-tab navigations, e.g., extension. > But > > I am not sure if filtering extensions:// is enough. Do you know what is the > > correct filter for tab navigation? > > Ping. I'm not sure I understand what you mean. chrome-extension:// URLs can load in normal tabs as well, such as Options pages. Conversely, I think you can also have SharedWorkers from HTTPS URLs that load in background WebContents and aren't in normal tabs. A URL check is not the right way to determine if it's loading in a normal tab. Someone with more knowledge of the chrome/ layer can probably tell you the right way to determine that. I think there might be some UserData you could look for? (Maybe ask sky@ or avi@?) https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_navigation_throttle.cc:37: return base::MakeUnique<TabNavigationThrottle>(navigation_handle); On 2017/06/13 23:33:21, Zhen Wang wrote: > On 2017/06/10 00:53:36, Charlie Reis wrote: > > It seems like we're creating this throttle in a lot more cases than necessary. > > > Right now it looks like almost all main frame navigations (with a few > exceptions > > based on destination URL), but my impression is that it should only be for the > > first navigation in a newly created tab, in a previously unused SiteInstance, > > with no opener. That was my understanding from the design doc, anyway. > > > > Would it make sense to narrow down to some of these criteria, using > > navigation_handle->GetWebContents()? You could ask the WebContents about > > GetOpener() and GetController().IsInitialNavigation(). We might be able to > > check SiteInstanceImpl::active_frame_count() as well. > > Thanks for the pointers! Added restriction on first navigation and opener case. > > Why do we check SiteInstanceImpl::active_frame_count()? What is this use case? SiteInstanceImpl::active_frame_count() says how many frames might be able to reach and script this page (i.e., are in the same process and could have references to each other). It would be similar to checking the opener of the WebContents, but covers more scenarios (e.g., frames that could find each other by name). If it's easy to add to the list of checks, maybe alongside the HasOpener() call, I'd suggest including it. https://codereview.chromium.org/2931023002/diff/180001/chrome/browser/resourc... File chrome/browser/resource_coordinator/background_tab_navigation_throttle.cc (right): https://codereview.chromium.org/2931023002/diff/180001/chrome/browser/resourc... chrome/browser/resource_coordinator/background_tab_navigation_throttle.cc:49: !TabManager::IsInternalPage(navigation_handle->GetURL())) { As noted earlier, I don't understand why we need this URL check. Maybe there's a better way to handle it? https://codereview.chromium.org/2931023002/diff/180001/chrome/browser/resourc... File chrome/browser/resource_coordinator/tab_manager.cc (right): https://codereview.chromium.org/2931023002/diff/180001/chrome/browser/resourc... chrome/browser/resource_coordinator/tab_manager.cc:1024: DCHECK(GetWebContentsData(contents)->tab_loading_state() == TAB_IS_LOADED); nit: Does DCHECK_EQ work here? That would give a better failure message. (Same below.) https://codereview.chromium.org/2931023002/diff/180001/chrome/browser/resourc... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc (right): https://codereview.chromium.org/2931023002/diff/180001/chrome/browser/resourc... chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc:47: g_browser_process->GetTabManager()->OnLoadingDone(web_contents()); Can we stick with the existing names (i.e., OnDidStopLoading here and OnDidFinishNavigation below)? There's already a lot of terms like this floating around, so it's a bit confusing to introduce new terms like LoadingDone and NavigationDone. https://codereview.chromium.org/2931023002/diff/180001/chrome/common/chrome_f... File chrome/common/chrome_features.cc (right): https://codereview.chromium.org/2931023002/diff/180001/chrome/common/chrome_f... chrome/common/chrome_features.cc:357: // Enables delaying background tabs' opening process in order to improve nit: "process" is ambiguous here (vs RenderProcessHost). Maybe "Enables delaying the first navigation in background tabs in order to..."? https://codereview.chromium.org/2931023002/diff/180001/content/public/browser... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/2931023002/diff/180001/content/public/browser... content/public/browser/web_contents.h:446: virtual bool IsVisible() const = 0; I still find this confusing vs RWHV::IsShowing(), since both the name and implementation are subtly different. Can you update the comment to say how this differs from RenderWidgetHostView::IsShowing() (i.e., returns true even before the page loads)?
The CQ bit was checked by zhenw@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...
https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_manager.cc (right): https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.cc:901: if (!navigation_handle) On 2017/07/06 23:50:30, Charlie Reis (slow) wrote: > On 2017/06/19 23:00:12, Zhen Wang wrote: > > On 2017/06/13 23:33:20, Zhen Wang wrote: > > > On 2017/06/10 00:53:36, Charlie Reis wrote: > > > > Is this possible? > > > > > > I am not sure if this is ever possible. That's why I check it here. Should I > > use > > > a DCHECK? > > > > Ping > > Let's remove it if we don't know that it's needed. Adding unnecessary null > checks makes it harder to reason about how the code is meant to be used. Done. https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_manager.h (right): https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager.h:168: // can be easily reloaded and hence makes a good choice to discard. On 2017/07/06 23:50:30, Charlie Reis (slow) wrote: > On 2017/06/10 00:53:36, Charlie Reis (slow) wrote: > > We may not need to expose this; see my question about the call site. > > I'm still hoping we don't need this. (See > https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo...) Now moved back to private as we do not need this URL check in throttle now. https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_navigation_throttle.cc (right): https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_navigation_throttle.cc:37: return base::MakeUnique<TabNavigationThrottle>(navigation_handle); On 2017/07/06 23:50:30, Charlie Reis (slow) wrote: > On 2017/06/13 23:33:21, Zhen Wang wrote: > > On 2017/06/10 00:53:36, Charlie Reis wrote: > > > It seems like we're creating this throttle in a lot more cases than > necessary. > > > > > Right now it looks like almost all main frame navigations (with a few > > exceptions > > > based on destination URL), but my impression is that it should only be for > the > > > first navigation in a newly created tab, in a previously unused > SiteInstance, > > > with no opener. That was my understanding from the design doc, anyway. > > > > > > Would it make sense to narrow down to some of these criteria, using > > > navigation_handle->GetWebContents()? You could ask the WebContents about > > > GetOpener() and GetController().IsInitialNavigation(). We might be able to > > > check SiteInstanceImpl::active_frame_count() as well. > > > > Thanks for the pointers! Added restriction on first navigation and opener > case. > > > > Why do we check SiteInstanceImpl::active_frame_count()? What is this use case? > > SiteInstanceImpl::active_frame_count() says how many frames might be able to > reach and script this page (i.e., are in the same process and could have > references to each other). It would be similar to checking the opener of the > WebContents, but covers more scenarios (e.g., frames that could find each other > by name). If it's easy to add to the list of checks, maybe alongside the > HasOpener() call, I'd suggest including it. active_frame_count is not exposed as a public content API, i.e., it is on content::SiteInstanceImpl, but not content::SiteInstance. Can I actually use it in //chrome/browser? https://codereview.chromium.org/2931023002/diff/180001/chrome/browser/resourc... File chrome/browser/resource_coordinator/background_tab_navigation_throttle.cc (right): https://codereview.chromium.org/2931023002/diff/180001/chrome/browser/resourc... chrome/browser/resource_coordinator/background_tab_navigation_throttle.cc:49: !TabManager::IsInternalPage(navigation_handle->GetURL())) { On 2017/07/06 23:50:30, Charlie Reis (slow) wrote: > As noted earlier, I don't understand why we need this URL check. Maybe there's > a better way to handle it? Maybe we do not need any URL check here. I was just trying to be conservative. I removed the check now. We can add back some specific check here later if needed. https://codereview.chromium.org/2931023002/diff/180001/chrome/browser/resourc... File chrome/browser/resource_coordinator/tab_manager.cc (right): https://codereview.chromium.org/2931023002/diff/180001/chrome/browser/resourc... chrome/browser/resource_coordinator/tab_manager.cc:1024: DCHECK(GetWebContentsData(contents)->tab_loading_state() == TAB_IS_LOADED); On 2017/07/06 23:50:30, Charlie Reis (slow) wrote: > nit: Does DCHECK_EQ work here? That would give a better failure message. (Same > below.) Done. https://codereview.chromium.org/2931023002/diff/180001/chrome/browser/resourc... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc (right): https://codereview.chromium.org/2931023002/diff/180001/chrome/browser/resourc... chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc:47: g_browser_process->GetTabManager()->OnLoadingDone(web_contents()); On 2017/07/06 23:50:30, Charlie Reis (slow) wrote: > Can we stick with the existing names (i.e., OnDidStopLoading here and > OnDidFinishNavigation below)? There's already a lot of terms like this floating > around, so it's a bit confusing to introduce new terms like LoadingDone and > NavigationDone. Done. https://codereview.chromium.org/2931023002/diff/180001/chrome/common/chrome_f... File chrome/common/chrome_features.cc (right): https://codereview.chromium.org/2931023002/diff/180001/chrome/common/chrome_f... chrome/common/chrome_features.cc:357: // Enables delaying background tabs' opening process in order to improve On 2017/07/06 23:50:31, Charlie Reis (slow) wrote: > nit: "process" is ambiguous here (vs RenderProcessHost). Maybe "Enables > delaying the first navigation in background tabs in order to..."? Updated https://codereview.chromium.org/2931023002/diff/180001/content/public/browser... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/2931023002/diff/180001/content/public/browser... content/public/browser/web_contents.h:446: virtual bool IsVisible() const = 0; On 2017/07/06 23:50:31, Charlie Reis (slow) wrote: > I still find this confusing vs RWHV::IsShowing(), since both the name and > implementation are subtly different. Can you update the comment to say how this > differs from RenderWidgetHostView::IsShowing() (i.e., returns true even before > the page loads)? Added comments now. Ken, can you double check if this makes sense? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! LGTM at a high level, if Ken is ok with IsVisible(). https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_navigation_throttle.cc (right): https://codereview.chromium.org/2931023002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_navigation_throttle.cc:37: return base::MakeUnique<TabNavigationThrottle>(navigation_handle); On 2017/07/07 18:06:39, Zhen Wang wrote: > On 2017/07/06 23:50:30, Charlie Reis (slow) wrote: > > On 2017/06/13 23:33:21, Zhen Wang wrote: > > > On 2017/06/10 00:53:36, Charlie Reis wrote: > > > > It seems like we're creating this throttle in a lot more cases than > > necessary. > > > > > > > Right now it looks like almost all main frame navigations (with a few > > > exceptions > > > > based on destination URL), but my impression is that it should only be for > > the > > > > first navigation in a newly created tab, in a previously unused > > SiteInstance, > > > > with no opener. That was my understanding from the design doc, anyway. > > > > > > > > Would it make sense to narrow down to some of these criteria, using > > > > navigation_handle->GetWebContents()? You could ask the WebContents about > > > > GetOpener() and GetController().IsInitialNavigation(). We might be able > to > > > > check SiteInstanceImpl::active_frame_count() as well. > > > > > > Thanks for the pointers! Added restriction on first navigation and opener > > case. > > > > > > Why do we check SiteInstanceImpl::active_frame_count()? What is this use > case? > > > > SiteInstanceImpl::active_frame_count() says how many frames might be able to > > reach and script this page (i.e., are in the same process and could have > > references to each other). It would be similar to checking the opener of the > > WebContents, but covers more scenarios (e.g., frames that could find each > other > > by name). If it's easy to add to the list of checks, maybe alongside the > > HasOpener() call, I'd suggest including it. > > active_frame_count is not exposed as a public content API, i.e., it is on > content::SiteInstanceImpl, but not content::SiteInstance. Can I actually use it > in //chrome/browser? Ah, I'd forgotten we're not in content here. Nevermind, we can skip it. https://codereview.chromium.org/2931023002/diff/180001/chrome/browser/resourc... File chrome/browser/resource_coordinator/background_tab_navigation_throttle.cc (right): https://codereview.chromium.org/2931023002/diff/180001/chrome/browser/resourc... chrome/browser/resource_coordinator/background_tab_navigation_throttle.cc:49: !TabManager::IsInternalPage(navigation_handle->GetURL())) { On 2017/07/07 18:06:39, Zhen Wang wrote: > On 2017/07/06 23:50:30, Charlie Reis (slow) wrote: > > As noted earlier, I don't understand why we need this URL check. Maybe > there's > > a better way to handle it? > > Maybe we do not need any URL check here. I was just trying to be conservative. I > removed the check now. We can add back some specific check here later if needed. Acknowledged. https://codereview.chromium.org/2931023002/diff/200001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2931023002/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:3247: #if !defined(OS_ANDROID) Please include a comment for why this isn't enabled on Android. https://codereview.chromium.org/2931023002/diff/200001/chrome/browser/resourc... File chrome/browser/resource_coordinator/background_tab_navigation_throttle.h (right): https://codereview.chromium.org/2931023002/diff/200001/chrome/browser/resourc... chrome/browser/resource_coordinator/background_tab_navigation_throttle.h:16: // and enables TabManager to delay navigation for background tabs. Might want to mention here as well that it's not used on Android?
Thanks! Ken, can you double check the IsVisible() change? https://codereview.chromium.org/2931023002/diff/200001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2931023002/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:3247: #if !defined(OS_ANDROID) On 2017/07/07 22:07:56, Charlie Reis (slow) wrote: > Please include a comment for why this isn't enabled on Android. Done. https://codereview.chromium.org/2931023002/diff/200001/chrome/browser/resourc... File chrome/browser/resource_coordinator/background_tab_navigation_throttle.h (right): https://codereview.chromium.org/2931023002/diff/200001/chrome/browser/resourc... chrome/browser/resource_coordinator/background_tab_navigation_throttle.h:16: // and enables TabManager to delay navigation for background tabs. On 2017/07/07 22:07:56, Charlie Reis (slow) wrote: > Might want to mention here as well that it's not used on Android? Done.
lgtm, that method seems reasonable.
Thanks for the thorough unittests! Regarding IsVisible: We couldn't determine that state via the associated TabStripModel? Or has the contents not yet been added to the model at that point?
On 2017/07/11 14:44:08, chrisha wrote: > Thanks for the thorough unittests! > > Regarding IsVisible: We couldn't determine that state via the associated > TabStripModel? Or has the contents not yet been added to the model at that > point? It is possible to track the visibility ourselves by using TabStropModel and then store the visibility in TabManager::WebContentsData. But that will be redundant information.
On 2017/07/11 16:47:38, Zhen Wang wrote: > On 2017/07/11 14:44:08, chrisha wrote: > > Thanks for the thorough unittests! > > > > Regarding IsVisible: We couldn't determine that state via the associated > > TabStripModel? Or has the contents not yet been added to the model at that > > point? > > It is possible to track the visibility ourselves by using TabStropModel and then > store the visibility in TabManager::WebContentsData. But that will be redundant > information. Why would we have to persist it? We can't just query the associated TabStripModel directly?
On 2017/07/11 17:56:53, chrisha wrote: > On 2017/07/11 16:47:38, Zhen Wang wrote: > > On 2017/07/11 14:44:08, chrisha wrote: > > > Thanks for the thorough unittests! > > > > > > Regarding IsVisible: We couldn't determine that state via the associated > > > TabStripModel? Or has the contents not yet been added to the model at that > > > point? > > > > It is possible to track the visibility ourselves by using TabStropModel and > then > > store the visibility in TabManager::WebContentsData. But that will be > redundant > > information. > > Why would we have to persist it? We can't just query the associated > TabStripModel directly? Oh, I actually meant using TabStripModelObserver's APIs like ActiveTabChanged, etc. In this case, we have to persist it. Sorry for the confusion. If using TabStripModel, do you mean using TabStripModel::IsTabSelected? Currently IsVisible() is used in the throttle, which is mostly tied to NavigationHandle and WebContents concepts, but not tabs. Technically, I guess we can query TabStripModel from there. But I am not sure if it is desired. Adding IsVisible to WebContents seems to be the reasonable way to me.
lgtm!
The CQ bit was checked by zhenw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/2931023002/#ps220001 (title: "add comments")
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by zhenw@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1499834937566810,
"parent_rev": "e069330ac4897a770a57d56fda01d33cc6931a2c", "commit_rev":
"6edd49c516047daa73d069c600bf9b2a09493bb7"}
Message was sent while issue was closed.
Description was changed from ========== [TooManyTabs] Add TabNavigationThrottle This is the 1st CL for staggered background tab opening feature. It adds TabNavigationThrottle, which plumbs navigation information into TabManager and enables TabManager to defer navigation for background tabs. This CL uses a simple algorithm, which defers background tab's navigation when some tab is loading. After a tab finishes loading, it will resume another pending navigation. TabManager will also resume a pending navigation when a background tab is selected. This feature is currently implemented behind a flag. This feature will not be enabled until the corresponding metrics are ready. This CL focuses on plumbing navigation into TabManager. The actual algorithm to delay background tab opening is simple. But it should be fine because the whole feature is behind a flag right now. There will be follow-up CLs to consider system resources and refine the algorithm. Design doc: https://docs.google.com/document/d/1_CS5kcY00CE3pzDTtW5Ny221gWfk4mFXYGiezoYSCBI/ BUG=730098 ========== to ========== [TooManyTabs] Add TabNavigationThrottle This is the 1st CL for staggered background tab opening feature. It adds TabNavigationThrottle, which plumbs navigation information into TabManager and enables TabManager to defer navigation for background tabs. This CL uses a simple algorithm, which defers background tab's navigation when some tab is loading. After a tab finishes loading, it will resume another pending navigation. TabManager will also resume a pending navigation when a background tab is selected. This feature is currently implemented behind a flag. This feature will not be enabled until the corresponding metrics are ready. This CL focuses on plumbing navigation into TabManager. The actual algorithm to delay background tab opening is simple. But it should be fine because the whole feature is behind a flag right now. There will be follow-up CLs to consider system resources and refine the algorithm. Design doc: https://docs.google.com/document/d/1_CS5kcY00CE3pzDTtW5Ny221gWfk4mFXYGiezoYSCBI/ BUG=730098 Review-Url: https://codereview.chromium.org/2931023002 Cr-Commit-Position: refs/heads/master@{#485870} Committed: https://chromium.googlesource.com/chromium/src/+/6edd49c516047daa73d069c600bf... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/6edd49c516047daa73d069c600bf... |
