|
|
Created:
5 years, 9 months ago by carlosk Modified:
5 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate: Avoid duplicate SiteInstance creation during navigation.
These changes to RenderFrameHostManager make it so that during a
navigation a new SiteInstance won't be created for a URL if a previously
created one still exists for that same URL. This also follows more
closely how the current navigation implementation works.
BUG=376094
TEST=WebContentsImplTest.ActiveContentsCountChangeBrowsingInstance
Committed: https://crrev.com/078298a8fcafefc5276f58c657e95b4fc1ff4009
Cr-Commit-Position: refs/heads/master@{#324027}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Moved the logic into GetSiteInstanceForNavigation. #
Total comments: 2
Patch Set 3 : The solution as a rewrite of the SiteInstance determination methods (with rebase). #Patch Set 4 : Re-split DetermineSiteInstanceForURL into 2 methods as it was before. #Patch Set 5 : Extract RFH selection logic form GetFrameHostForNavigation. #
Total comments: 21
Patch Set 6 : Address CR comments. #Patch Set 7 : Merged GetSiteInstanceForNavigation with IsCorrectSiteInstanceForURL. #
Total comments: 45
Patch Set 8 : Rebase #Patch Set 9 : Consolidated methods into a simpler change; addressed other CR comments. #
Total comments: 28
Patch Set 10 : #
Total comments: 4
Patch Set 11 : Minor changes. #Patch Set 12 : Add tests to check RFHM::ConvertToSiteInstance works as expected. #
Total comments: 14
Patch Set 13 : General changes and improvements from CR comments. #Patch Set 14 : Rebase + content-exporting SiteInstanceDescriptor. #
Messages
Total messages: 53 (16 generated)
carlosk@chromium.org changed reviewers: + clamy@chromium.org, fdegans@chromium.org, nasko@chromium.org
carlosk@chromium.org changed required reviewers: + clamy@chromium.org, nasko@chromium.org
clamy@, nasko@: PTAL. fdegans@: FYI. As suggested I made a new CL for this change, out off http://crrev.com/953503002. With that CL the test WebContentsImplTest.ActiveContentsCountChangeBrowsingInstance crashes when PlzNavigate is enabled and this change fixes that.
I'll take a look how this fits in the other CL, but on it's own it does an issue, which I pointed out at the relevant place. https://codereview.chromium.org/967383002/diff/1/content/browser/frame_host/r... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/967383002/diff/1/content/browser/frame_host/r... content/browser/frame_host/render_frame_host_manager.cc:1184: speculative_render_frame_host_->GetSiteInstance()->GetSiteURL() == This check doesn't seem quite right. The SiteInstance URL is a "site" URL, therefore it will contain only scheme + registered domain name. The dest_url could be any URL, including port, path, and query string.
https://codereview.chromium.org/967383002/diff/1/content/browser/frame_host/r... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/967383002/diff/1/content/browser/frame_host/r... content/browser/frame_host/render_frame_host_manager.cc:1184: speculative_render_frame_host_->GetSiteInstance()->GetSiteURL() == On 2015/03/02 23:02:09, nasko wrote: > This check doesn't seem quite right. The SiteInstance URL is a "site" URL, > therefore it will contain only scheme + registered domain name. The dest_url > could be any URL, including port, path, and query string. If I follow things right, you probably want to call SiteInstance::GetSiteForURL with the dest_url and compare its output with the GetSiteURL of the speculative RFH's SiteInstance.
creis@chromium.org changed reviewers: + creis@chromium.org
I don't think we should be making this change. It's hiding the fact that we intentionally create SiteInstances in different BrowsingInstances in some cases, and it potentially causes problems if other people call GetSiteInstanceForURL and unexpectedly get back the speculative RFH's SiteInstance. Is the premise of the change that the speculative RFH calls GetSiteInstanceForURL in advance, then we call it again later at commit time? In this case I agree that the SiteInstance returned at commit time won't match the speculative RFH's SiteInstance if we created a SiteInstance in a new BrowsingInstance. That fails the following check in GetFrameHostForNavigation, right? if (!speculative_render_frame_host_ || speculative_render_frame_host_->GetSiteInstance() != dest_site_instance.get()) { I think it would be better to update that check than try to return the speculative RFH's SiteInstance. The check could see whether the SiteInstances were equal, or whether they (1) have the same site URL and (2) are both in new BrowsingInstances that differ from the current one.
On 2015/03/02 23:52:40, Charlie Reis wrote: > I don't think we should be making this change. It's hiding the fact that we > intentionally create SiteInstances in different BrowsingInstances in some cases, > and it potentially causes problems if other people call GetSiteInstanceForURL > and unexpectedly get back the speculative RFH's SiteInstance. > > Is the premise of the change that the speculative RFH calls > GetSiteInstanceForURL in advance, then we call it again later at commit time? > In this case I agree that the SiteInstance returned at commit time won't match > the speculative RFH's SiteInstance if we created a SiteInstance in a new > BrowsingInstance. That fails the following check in GetFrameHostForNavigation, > right? > if (!speculative_render_frame_host_ || > speculative_render_frame_host_->GetSiteInstance() != > dest_site_instance.get()) { > > > I think it would be better to update that check than try to return the > speculative RFH's SiteInstance. The check could see whether the SiteInstances > were equal, or whether they (1) have the same site URL and (2) are both in new > BrowsingInstances that differ from the current one. As GetSiteInstanceForURL is a private method of RFHM I assumed that if someone chooses to call it he/she has to know what they are doing either by looking at the implementation or doc comment. I did try to make what it does clear in its comment, that does mention the new BrowsingInstance part. But I agree this code can be moved somewhere else. The premises for this change are: 1) We don't want new SiteInstances (under a new BrowsingInstance) to be created multiple times for the current navigation if we still have access to a previously, already created one. 2) GetSiteInstanceForURL is called one or more times during a navigation through GetFrameHostForNavigation (currently called twice during a normal navigation, upon request/beforeUnload_ACK and upon commit). 3) If a speculative RFH exists and its SiteInstance is valid for the current URL, we know it is of the "proper type" (has a new BrowsingInstance) as it must have been created by a previous, similar call to GetSiteInstanceForURL during the same navigation First and foremost mind premise 1). If that's *not* the case then I totally agree this change should not go in and the failing test should be changed instead to accommodate the similar but still valid SiteInstances created. Otherwise controlling their re-creation is required (granted maybe in another form than this one). If I understand your line of thought of where this logic should live, you'd prefer it to be moved out of this new method. Then I think the more appropriate place would be in GetSiteInstanceForNavigation or GetSiteInstanceForURL as those two are the ones actually meant to translate a navigation URL (and a bit more stuff) into an appropriate SiteInstance. I uploaded a new CL that has it moved into GetSiteInstanceForNavigation, that seemed more appropriate from my PoV. https://codereview.chromium.org/967383002/diff/1/content/browser/frame_host/r... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/967383002/diff/1/content/browser/frame_host/r... content/browser/frame_host/render_frame_host_manager.cc:1184: speculative_render_frame_host_->GetSiteInstance()->GetSiteURL() == On 2015/03/02 23:19:47, nasko wrote: > On 2015/03/02 23:02:09, nasko wrote: > > This check doesn't seem quite right. The SiteInstance URL is a "site" URL, > > therefore it will contain only scheme + registered domain name. The dest_url > > could be any URL, including port, path, and query string. > > If I follow things right, you probably want to call SiteInstance::GetSiteForURL > with the dest_url and compare its output with the GetSiteURL of the speculative > RFH's SiteInstance. Yes, indeed. This is the right thing to do. If the final decision is to go on with adding this logic, I'll also add a specific test to check this is working as expected.
Sorry for the delay; I wanted to think about the possibilities of what to expose. On 2015/03/03 13:42:41, carlosk wrote: > On 2015/03/02 23:52:40, Charlie Reis wrote: > > I don't think we should be making this change. It's hiding the fact that we > > intentionally create SiteInstances in different BrowsingInstances in some > cases, > > and it potentially causes problems if other people call GetSiteInstanceForURL > > and unexpectedly get back the speculative RFH's SiteInstance. > > > > Is the premise of the change that the speculative RFH calls > > GetSiteInstanceForURL in advance, then we call it again later at commit time? > > In this case I agree that the SiteInstance returned at commit time won't match > > the speculative RFH's SiteInstance if we created a SiteInstance in a new > > BrowsingInstance. That fails the following check in > GetFrameHostForNavigation, > > right? > > if (!speculative_render_frame_host_ || > > speculative_render_frame_host_->GetSiteInstance() != > > dest_site_instance.get()) { > > > > > > I think it would be better to update that check than try to return the > > speculative RFH's SiteInstance. The check could see whether the SiteInstances > > were equal, or whether they (1) have the same site URL and (2) are both in new > > BrowsingInstances that differ from the current one. > > As GetSiteInstanceForURL is a private method of RFHM I assumed that if someone > chooses to call it he/she has to know what they are doing either by looking at > the implementation or doc comment. I did try to make what it does clear in its > comment, that does mention the new BrowsingInstance part. But I agree this code > can be moved somewhere else. I agree that it's a private method, but now that we're calling it in two places, it has the appearance of being a safe method to call. I think we should be careful about entangling it with the call sites. This is especially true because we'd like to have the option to call it from other places as well. For example, we've wanted to use it from CrossSiteResourceHandler in the past to do a policy check on the UI thread. (That particular case may be unnecessary once PlzNavigate lands, but it's an example of why we should keep it independent from its callers.) > > The premises for this change are: > 1) We don't want new SiteInstances (under a new BrowsingInstance) to be created > multiple times for the current navigation if we still have access to a > previously, already created one. You raise an excellent point. By calling GetSiteInstanceForURL multiple times, we're acting like it's idempotent, but it's not. It currently has side effects, like actually creating a SiteInstance in either the current or a new BrowsingInstance. I wonder if we can explore another way to approach this. Could we actually make it idempotent (i.e., no side effects), so that the caller can decide whether to actually create a SiteInstance or just compare against an existing one? I'm not a fan of out-parameters, but either that or a returned struct could tell the caller whether to (1) use a particular existing SiteInstance, (2) create a related SiteInstance with a given site URL, or (3) create a SiteInstance in a new BrowsingInstance with a given site URL. We could even hide the ugliness behind a reasonable (but still private) API. Something like: bool IsCorrectSiteInstanceForURL(...) // idempotent SiteInstance* GetSiteInstanceForURL(...) // potentially state changing void DetermineSiteInstanceForURL(..., out_params) // "really" private idempotent helper for both With that approach, PlzNavigate would call GetSiteInstanceForURL when creating the speculative RFH. We would call the idempotent IsCorrectSiteInstance at commit time and check whether the speculative RFH's SiteInstance matches what we need. I can't tell yet whether that will end up too awkward, but it seems better than tying GetSiteInstanceForURL's behavior to the speculative RFH call sites (patch 1) or trying to predict its output without calling it (patch 2). Thoughts? > 2) GetSiteInstanceForURL is called one or more times during a navigation through > GetFrameHostForNavigation (currently called twice during a normal navigation, > upon request/beforeUnload_ACK and upon commit). > 3) If a speculative RFH exists and its SiteInstance is valid for the current > URL, we know it is of the "proper type" (has a new BrowsingInstance) as it must > have been created by a previous, similar call to GetSiteInstanceForURL during > the same navigation > > First and foremost mind premise 1). If that's *not* the case then I totally > agree this change should not go in and the failing test should be changed > instead to accommodate the similar but still valid SiteInstances created. > Otherwise controlling their re-creation is required (granted maybe in another > form than this one). > > If I understand your line of thought of where this logic should live, you'd > prefer it to be moved out of this new method. Then I think the more appropriate > place would be in GetSiteInstanceForNavigation or GetSiteInstanceForURL as those > two are the ones actually meant to translate a navigation URL (and a bit more > stuff) into an appropriate SiteInstance. I uploaded a new CL that has it moved > into GetSiteInstanceForNavigation, that seemed more appropriate from my PoV. > https://codereview.chromium.org/967383002/diff/20001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/967383002/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1001: SiteInstanceImpl::GetSiteForURL(browser_context, dest_url)) { SiteInstanceImpl::GetSiteForURL is not equivalent to RFHM::GetSiteInstanceForURL, so I think this will have problems. For example, consider the source_instance case that we use for about:blank and data: URLs. GetSiteInstanceForURL puts those in the source SiteInstance, so their site URL won't match GetSiteForURL(browser_context, "about:blank").
Thanks Charlie for the thorough response. I already read through your response (more than once) but I still have to look further into it to understand what you are suggesting would involve. :) But what I think is the most important question I asked, that may completely change the target of this CL, was not answered. It is: > First and foremost mind premise 1). If that's *not* the case then I totally > agree this change should not go in and the failing test should be changed > instead to accommodate the similar but still valid SiteInstances created. > Otherwise controlling their re-creation is required (granted maybe in another > form than this one). So, do we actually care about that? Solely from your answer I'd assume we do but I prefer to be sure before diving in that direction.
On 2015/03/05 16:59:55, carlosk wrote: > Thanks Charlie for the thorough response. I already read through your response > (more than once) but I still have to look further into it to understand what you > are suggesting would involve. :) > > But what I think is the most important question I asked, that may completely > change the target of this CL, was not answered. It is: > > > First and foremost mind premise 1). If that's *not* the case then I totally > > agree this change should not go in and the failing test should be changed > > instead to accommodate the similar but still valid SiteInstances created. > > Otherwise controlling their re-creation is required (granted maybe in another > > form than this one). > > So, do we actually care about that? Solely from your answer I'd assume we do but > I prefer to be sure before diving in that direction. Yes, premise #1 was what my response was about. I agree that we don't want to create new SiteInstances (and then throw them away) just to do a comparison. Hope that helps!
That previous comment did indeed help. :) On 2015/03/05 04:27:11, Charlie Reis wrote: > Sorry for the delay; I wanted to think about the possibilities of what to > expose. > > On 2015/03/03 13:42:41, carlosk wrote: > > On 2015/03/02 23:52:40, Charlie Reis wrote: > > > I don't think we should be making this change. It's hiding the fact that we > > > intentionally create SiteInstances in different BrowsingInstances in some > > cases, > > > and it potentially causes problems if other people call > GetSiteInstanceForURL > > > and unexpectedly get back the speculative RFH's SiteInstance. > > > > > > Is the premise of the change that the speculative RFH calls > > > GetSiteInstanceForURL in advance, then we call it again later at commit > time? > > > In this case I agree that the SiteInstance returned at commit time won't > match > > > the speculative RFH's SiteInstance if we created a SiteInstance in a new > > > BrowsingInstance. That fails the following check in > > GetFrameHostForNavigation, > > > right? > > > if (!speculative_render_frame_host_ || > > > speculative_render_frame_host_->GetSiteInstance() != > > > dest_site_instance.get()) { > > > > > > > > > I think it would be better to update that check than try to return the > > > speculative RFH's SiteInstance. The check could see whether the > SiteInstances > > > were equal, or whether they (1) have the same site URL and (2) are both in > new > > > BrowsingInstances that differ from the current one. > > > > As GetSiteInstanceForURL is a private method of RFHM I assumed that if someone > > chooses to call it he/she has to know what they are doing either by looking at > > the implementation or doc comment. I did try to make what it does clear in its > > comment, that does mention the new BrowsingInstance part. But I agree this > code > > can be moved somewhere else. > > I agree that it's a private method, but now that we're calling it in two places, > it has the appearance of being a safe method to call. I think we should be > careful about entangling it with the call sites. > > This is especially true because we'd like to have the option to call it from > other places as well. For example, we've wanted to use it from > CrossSiteResourceHandler in the past to do a policy check on the UI thread. > (That particular case may be unnecessary once PlzNavigate lands, but it's an > example of why we should keep it independent from its callers.) I wrote my previous comment thinking about the new method I had added (CreateSiteInstanceForURL) instead of the existing one you referred to (GetSiteInstanceForURL). Under this "new" light I agree with your initial concern and it would be better to change GetFrameHostForNavigation to do the extra checks. But we would still be creating more SiteInstances than strictly necessary. An extra note: when you mention it's called from two places I think you are in fact referring to GetSiteInstanceForNavigation, which is called from both GetFrameHostForNavigation and UpdateStateForNavigate, right? Just mind that this duplication only exists temporarily as one is used by "current" navigations and the other by PlzNavigate. The former will disappear eventually. > > The premises for this change are: > > 1) We don't want new SiteInstances (under a new BrowsingInstance) to be > created > > multiple times for the current navigation if we still have access to a > > previously, already created one. > > You raise an excellent point. By calling GetSiteInstanceForURL multiple times, > we're acting like it's idempotent, but it's not. It currently has side effects, > like actually creating a SiteInstance in either the current or a new > BrowsingInstance. > > I wonder if we can explore another way to approach this. Could we actually make > it idempotent (i.e., no side effects), so that the caller can decide whether to > actually create a SiteInstance or just compare against an existing one? I'm not > a fan of out-parameters, but either that or a returned struct could tell the > caller whether to (1) use a particular existing SiteInstance, (2) create a > related SiteInstance with a given site URL, or (3) create a SiteInstance in a > new BrowsingInstance with a given site URL. > > We could even hide the ugliness behind a reasonable (but still private) API. > Something like: > bool IsCorrectSiteInstanceForURL(...) // idempotent > SiteInstance* GetSiteInstanceForURL(...) // potentially state changing > > void DetermineSiteInstanceForURL(..., out_params) // "really" private > idempotent helper for both > > With that approach, PlzNavigate would call GetSiteInstanceForURL when creating > the speculative RFH. We would call the idempotent IsCorrectSiteInstance at > commit time and check whether the speculative RFH's SiteInstance matches what we > need. > > I can't tell yet whether that will end up too awkward, but it seems better than > tying GetSiteInstanceForURL's behavior to the speculative RFH call sites (patch > 1) or trying to predict its output without calling it (patch 2). > > Thoughts? Yes, this would fully solve the problem but I can't help thinking it might be an overkill. This doubt comes from me wondering how "different" the matched SiteInstance cases would be based on the destination URL changes from the 1st call to GetFrameHostForNavigation to the 2nd. I would think these would only (?) change on server redirects and these could be covered by simpler tests (versus all that is currently done in GetSiteInstanceForURL). But as having the precise SiteInstance is a very important security concern in Chrome, this seems to be the better, safer choice. The only improvement I can think of is to allow the SiteInstance verification and creation (when needed) to only call DetermineSiteInstanceForURL once. I guess this would require a new method (for which out_params is an input parameter): SiteInstance* CreatSiteInstanceForURL(out_params, ...) It would be called invariably by GetSiteInstanceForURL but PlzNavigate (GetFrameHostForNavigation) would only call it when a new SiteInstance is actually needed (instead of calling GetSiteInstanceForURL). WDYT? > > 2) GetSiteInstanceForURL is called one or more times during a navigation > through > > GetFrameHostForNavigation (currently called twice during a normal navigation, > > upon request/beforeUnload_ACK and upon commit). > > 3) If a speculative RFH exists and its SiteInstance is valid for the current > > URL, we know it is of the "proper type" (has a new BrowsingInstance) as it > must > > have been created by a previous, similar call to GetSiteInstanceForURL during > > the same navigation > > > > First and foremost mind premise 1). If that's *not* the case then I totally > > agree this change should not go in and the failing test should be changed > > instead to accommodate the similar but still valid SiteInstances created. > > Otherwise controlling their re-creation is required (granted maybe in another > > form than this one). > > > > If I understand your line of thought of where this logic should live, you'd > > prefer it to be moved out of this new method. Then I think the more > appropriate > > place would be in GetSiteInstanceForNavigation or GetSiteInstanceForURL as > those > > two are the ones actually meant to translate a navigation URL (and a bit more > > stuff) into an appropriate SiteInstance. I uploaded a new CL that has it moved > > into GetSiteInstanceForNavigation, that seemed more appropriate from my PoV. https://codereview.chromium.org/967383002/diff/20001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/967383002/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1001: SiteInstanceImpl::GetSiteForURL(browser_context, dest_url)) { On 2015/03/05 04:27:11, Charlie Reis wrote: > SiteInstanceImpl::GetSiteForURL is not equivalent to > RFHM::GetSiteInstanceForURL, so I think this will have problems. > > For example, consider the source_instance case that we use for about:blank and > data: URLs. GetSiteInstanceForURL puts those in the source SiteInstance, so > their site URL won't match GetSiteForURL(browser_context, "about:blank"). Acknowledged.
On 2015/03/06 16:03:54, carlosk wrote: > That previous comment did indeed help. :) > > On 2015/03/05 04:27:11, Charlie Reis wrote: > > Sorry for the delay; I wanted to think about the possibilities of what to > > expose. > > > > On 2015/03/03 13:42:41, carlosk wrote: > > > On 2015/03/02 23:52:40, Charlie Reis wrote: > > > > I don't think we should be making this change. It's hiding the fact that > we > > > > intentionally create SiteInstances in different BrowsingInstances in some > > > cases, > > > > and it potentially causes problems if other people call > > GetSiteInstanceForURL > > > > and unexpectedly get back the speculative RFH's SiteInstance. > > > > > > > > Is the premise of the change that the speculative RFH calls > > > > GetSiteInstanceForURL in advance, then we call it again later at commit > > time? > > > > In this case I agree that the SiteInstance returned at commit time won't > > match > > > > the speculative RFH's SiteInstance if we created a SiteInstance in a new > > > > BrowsingInstance. That fails the following check in > > > GetFrameHostForNavigation, > > > > right? > > > > if (!speculative_render_frame_host_ || > > > > speculative_render_frame_host_->GetSiteInstance() != > > > > dest_site_instance.get()) { > > > > > > > > > > > > I think it would be better to update that check than try to return the > > > > speculative RFH's SiteInstance. The check could see whether the > > SiteInstances > > > > were equal, or whether they (1) have the same site URL and (2) are both in > > new > > > > BrowsingInstances that differ from the current one. > > > > > > As GetSiteInstanceForURL is a private method of RFHM I assumed that if > someone > > > chooses to call it he/she has to know what they are doing either by looking > at > > > the implementation or doc comment. I did try to make what it does clear in > its > > > comment, that does mention the new BrowsingInstance part. But I agree this > > code > > > can be moved somewhere else. > > > > I agree that it's a private method, but now that we're calling it in two > places, > > it has the appearance of being a safe method to call. I think we should be > > careful about entangling it with the call sites. > > > > This is especially true because we'd like to have the option to call it from > > other places as well. For example, we've wanted to use it from > > CrossSiteResourceHandler in the past to do a policy check on the UI thread. > > (That particular case may be unnecessary once PlzNavigate lands, but it's an > > example of why we should keep it independent from its callers.) > > I wrote my previous comment thinking about the new method I had added > (CreateSiteInstanceForURL) instead of the existing one you referred to > (GetSiteInstanceForURL). Under this "new" light I agree with your initial > concern and it would be better to change GetFrameHostForNavigation to do the > extra checks. But we would still be creating more SiteInstances than strictly > necessary. > > An extra note: when you mention it's called from two places I think you are in > fact referring to GetSiteInstanceForNavigation, which is called from both > GetFrameHostForNavigation and UpdateStateForNavigate, right? Just mind that this > duplication only exists temporarily as one is used by "current" navigations and > the other by PlzNavigate. The former will disappear eventually. > > > > The premises for this change are: > > > 1) We don't want new SiteInstances (under a new BrowsingInstance) to be > > created > > > multiple times for the current navigation if we still have access to a > > > previously, already created one. > > > > You raise an excellent point. By calling GetSiteInstanceForURL multiple > times, > > we're acting like it's idempotent, but it's not. It currently has side > effects, > > like actually creating a SiteInstance in either the current or a new > > BrowsingInstance. > > > > I wonder if we can explore another way to approach this. Could we actually > make > > it idempotent (i.e., no side effects), so that the caller can decide whether > to > > actually create a SiteInstance or just compare against an existing one? I'm > not > > a fan of out-parameters, but either that or a returned struct could tell the > > caller whether to (1) use a particular existing SiteInstance, (2) create a > > related SiteInstance with a given site URL, or (3) create a SiteInstance in a > > new BrowsingInstance with a given site URL. > > > > We could even hide the ugliness behind a reasonable (but still private) API. > > Something like: > > bool IsCorrectSiteInstanceForURL(...) // idempotent > > SiteInstance* GetSiteInstanceForURL(...) // potentially state changing > > > > void DetermineSiteInstanceForURL(..., out_params) // "really" private > > idempotent helper for both > > > > With that approach, PlzNavigate would call GetSiteInstanceForURL when creating > > the speculative RFH. We would call the idempotent IsCorrectSiteInstance at > > commit time and check whether the speculative RFH's SiteInstance matches what > we > > need. > > > > I can't tell yet whether that will end up too awkward, but it seems better > than > > tying GetSiteInstanceForURL's behavior to the speculative RFH call sites > (patch > > 1) or trying to predict its output without calling it (patch 2). > > > > Thoughts? > > Yes, this would fully solve the problem but I can't help thinking it might be an > overkill. This doubt comes from me wondering how "different" the matched > SiteInstance cases would be based on the destination URL changes from the 1st > call to GetFrameHostForNavigation to the 2nd. I would think these would only (?) > change on server redirects and these could be covered by simpler tests (versus > all that is currently done in GetSiteInstanceForURL). > > But as having the precise SiteInstance is a very important security concern in > Chrome, this seems to be the better, safer choice. > > The only improvement I can think of is to allow the SiteInstance verification > and creation (when needed) to only call DetermineSiteInstanceForURL once. I > guess this would require a new method (for which out_params is an input > parameter): > > SiteInstance* CreatSiteInstanceForURL(out_params, ...) > > It would be called invariably by GetSiteInstanceForURL but PlzNavigate > (GetFrameHostForNavigation) would only call it when a new SiteInstance is > actually needed (instead of calling GetSiteInstanceForURL). > > WDYT? And of course a more appropriate name for that method would be something like GetAppropriateSiteInstance because it won't necessarily create a new one. > > > 2) GetSiteInstanceForURL is called one or more times during a navigation > > through > > > GetFrameHostForNavigation (currently called twice during a normal > navigation, > > > upon request/beforeUnload_ACK and upon commit). > > > 3) If a speculative RFH exists and its SiteInstance is valid for the current > > > URL, we know it is of the "proper type" (has a new BrowsingInstance) as it > > must > > > have been created by a previous, similar call to GetSiteInstanceForURL > during > > > the same navigation > > > > > > First and foremost mind premise 1). If that's *not* the case then I totally > > > agree this change should not go in and the failing test should be changed > > > instead to accommodate the similar but still valid SiteInstances created. > > > Otherwise controlling their re-creation is required (granted maybe in > another > > > form than this one). > > > > > > If I understand your line of thought of where this logic should live, you'd > > > prefer it to be moved out of this new method. Then I think the more > > appropriate > > > place would be in GetSiteInstanceForNavigation or GetSiteInstanceForURL as > > those > > > two are the ones actually meant to translate a navigation URL (and a bit > more > > > stuff) into an appropriate SiteInstance. I uploaded a new CL that has it > moved > > > into GetSiteInstanceForNavigation, that seemed more appropriate from my PoV. > > https://codereview.chromium.org/967383002/diff/20001/content/browser/frame_ho... > File content/browser/frame_host/render_frame_host_manager.cc (right): > > https://codereview.chromium.org/967383002/diff/20001/content/browser/frame_ho... > content/browser/frame_host/render_frame_host_manager.cc:1001: > SiteInstanceImpl::GetSiteForURL(browser_context, dest_url)) { > On 2015/03/05 04:27:11, Charlie Reis wrote: > > SiteInstanceImpl::GetSiteForURL is not equivalent to > > RFHM::GetSiteInstanceForURL, so I think this will have problems. > > > > For example, consider the source_instance case that we use for about:blank and > > data: URLs. GetSiteInstanceForURL puts those in the source SiteInstance, so > > their site URL won't match GetSiteForURL(browser_context, "about:blank"). > > Acknowledged.
On 2015/03/06 16:09:34, carlosk (OoO until March 16th) wrote: > On 2015/03/06 16:03:54, carlosk wrote: > > That previous comment did indeed help. :) > > > > On 2015/03/05 04:27:11, Charlie Reis wrote: > > > Sorry for the delay; I wanted to think about the possibilities of what to > > > expose. > > > > > > On 2015/03/03 13:42:41, carlosk wrote: > > > > On 2015/03/02 23:52:40, Charlie Reis wrote: > > > > > I don't think we should be making this change. It's hiding the fact > that > > we > > > > > intentionally create SiteInstances in different BrowsingInstances in > some > > > > cases, > > > > > and it potentially causes problems if other people call > > > GetSiteInstanceForURL > > > > > and unexpectedly get back the speculative RFH's SiteInstance. > > > > > > > > > > Is the premise of the change that the speculative RFH calls > > > > > GetSiteInstanceForURL in advance, then we call it again later at commit > > > time? > > > > > In this case I agree that the SiteInstance returned at commit time won't > > > match > > > > > the speculative RFH's SiteInstance if we created a SiteInstance in a new > > > > > BrowsingInstance. That fails the following check in > > > > GetFrameHostForNavigation, > > > > > right? > > > > > if (!speculative_render_frame_host_ || > > > > > speculative_render_frame_host_->GetSiteInstance() != > > > > > dest_site_instance.get()) { > > > > > > > > > > > > > > > I think it would be better to update that check than try to return the > > > > > speculative RFH's SiteInstance. The check could see whether the > > > SiteInstances > > > > > were equal, or whether they (1) have the same site URL and (2) are both > in > > > new > > > > > BrowsingInstances that differ from the current one. > > > > > > > > As GetSiteInstanceForURL is a private method of RFHM I assumed that if > > someone > > > > chooses to call it he/she has to know what they are doing either by > looking > > at > > > > the implementation or doc comment. I did try to make what it does clear in > > its > > > > comment, that does mention the new BrowsingInstance part. But I agree this > > > code > > > > can be moved somewhere else. > > > > > > I agree that it's a private method, but now that we're calling it in two > > places, > > > it has the appearance of being a safe method to call. I think we should be > > > careful about entangling it with the call sites. > > > > > > This is especially true because we'd like to have the option to call it from > > > other places as well. For example, we've wanted to use it from > > > CrossSiteResourceHandler in the past to do a policy check on the UI thread. > > > (That particular case may be unnecessary once PlzNavigate lands, but it's an > > > example of why we should keep it independent from its callers.) > > > > I wrote my previous comment thinking about the new method I had added > > (CreateSiteInstanceForURL) instead of the existing one you referred to > > (GetSiteInstanceForURL). Under this "new" light I agree with your initial > > concern and it would be better to change GetFrameHostForNavigation to do the > > extra checks. But we would still be creating more SiteInstances than strictly > > necessary. Agreed. > > > > An extra note: when you mention it's called from two places I think you are in > > fact referring to GetSiteInstanceForNavigation, which is called from both > > GetFrameHostForNavigation and UpdateStateForNavigate, right? Just mind that > this > > duplication only exists temporarily as one is used by "current" navigations > and > > the other by PlzNavigate. The former will disappear eventually. I meant that PlzNavigate calls GetFrameHostForNavigation twice: once for the speculative RFH from RFHM::BeginNavigation (via NavigatorImpl::BeginNavigation) and once at commit time from NavigatorImpl::CommitNavigation. That's why it's useful to have an idempotent version that we can call at commit time. > > > > > > The premises for this change are: > > > > 1) We don't want new SiteInstances (under a new BrowsingInstance) to be > > > created > > > > multiple times for the current navigation if we still have access to a > > > > previously, already created one. > > > > > > You raise an excellent point. By calling GetSiteInstanceForURL multiple > > times, > > > we're acting like it's idempotent, but it's not. It currently has side > > effects, > > > like actually creating a SiteInstance in either the current or a new > > > BrowsingInstance. > > > > > > I wonder if we can explore another way to approach this. Could we actually > > make > > > it idempotent (i.e., no side effects), so that the caller can decide whether > > to > > > actually create a SiteInstance or just compare against an existing one? I'm > > not > > > a fan of out-parameters, but either that or a returned struct could tell the > > > caller whether to (1) use a particular existing SiteInstance, (2) create a > > > related SiteInstance with a given site URL, or (3) create a SiteInstance in > a > > > new BrowsingInstance with a given site URL. > > > > > > We could even hide the ugliness behind a reasonable (but still private) API. > > > > Something like: > > > bool IsCorrectSiteInstanceForURL(...) // idempotent > > > SiteInstance* GetSiteInstanceForURL(...) // potentially state changing > > > > > > void DetermineSiteInstanceForURL(..., out_params) // "really" private > > > idempotent helper for both > > > > > > With that approach, PlzNavigate would call GetSiteInstanceForURL when > creating > > > the speculative RFH. We would call the idempotent IsCorrectSiteInstance at > > > commit time and check whether the speculative RFH's SiteInstance matches > what > > we > > > need. > > > > > > I can't tell yet whether that will end up too awkward, but it seems better > > than > > > tying GetSiteInstanceForURL's behavior to the speculative RFH call sites > > (patch > > > 1) or trying to predict its output without calling it (patch 2). > > > > > > Thoughts? > > > > Yes, this would fully solve the problem but I can't help thinking it might be > an > > overkill. This doubt comes from me wondering how "different" the matched > > SiteInstance cases would be based on the destination URL changes from the 1st > > call to GetFrameHostForNavigation to the 2nd. I would think these would only > (?) > > change on server redirects and these could be covered by simpler tests (versus > > all that is currently done in GetSiteInstanceForURL). I don't think that having a separate version of that check would make things simpler. It would be harder to maintain the two versions and more likely that we would have security problems if they disagreed. Server redirects can exercise a lot of the cases (e.g., crossing security boundaries for hosted apps or the Chrome Web Store), so I think it makes sense to use the full check both times. > > > > But as having the precise SiteInstance is a very important security concern in > > Chrome, this seems to be the better, safer choice. Agreed. > > > > The only improvement I can think of is to allow the SiteInstance verification > > and creation (when needed) to only call DetermineSiteInstanceForURL once. That's a tradeoff between calling the decision logic twice vs hiding the out_params ugliness from callers. We'll have to see how clean or ugly the code turns out to decide. > I > > guess this would require a new method (for which out_params is an input > > parameter): > > > > SiteInstance* CreatSiteInstanceForURL(out_params, ...) > > > > It would be called invariably by GetSiteInstanceForURL but PlzNavigate > > (GetFrameHostForNavigation) would only call it when a new SiteInstance is > > actually needed (instead of calling GetSiteInstanceForURL). > > > > WDYT? > > And of course a more appropriate name for that method would be something like > GetAppropriateSiteInstance because it won't necessarily create a new one. If we're going to break them down like this, maybe we should just have: // Private inside RFHM: struct SiteInstanceDescriptor { SiteInstance* site_instance; // optional, for the cases we know it already GURL site_url; // in case |site_instance| is null bool is_related_to_current; // for knowing whether to create in a new BrowsingInstance or not }; SiteInstanceDescriptor DetermineSiteInstanceForURL(...); // All the policy logic, with no side effects. SiteInstance* GetSiteInstanceForDescriptor(SiteInstanceDescriptor); // just returns or creates a SI. In cases that we want the real SiteInstance to be created, we call both (e.g., in UpdateStateForNavigate, BeginNavigation). In cases that we just want to compare, we call the first one (e.g., in CommitNavigation). If the comparison fails, we call the second.
Thanks! On 2015/03/06 23:15:32, Charlie Reis wrote: > On 2015/03/06 16:09:34, carlosk (OoO until March 16th) wrote: > > On 2015/03/06 16:03:54, carlosk wrote: > > > That previous comment did indeed help. :) > > > > > > On 2015/03/05 04:27:11, Charlie Reis wrote: > > > > Sorry for the delay; I wanted to think about the possibilities of what to > > > > expose. > > > > > > > > On 2015/03/03 13:42:41, carlosk wrote: > > > > > On 2015/03/02 23:52:40, Charlie Reis wrote: > > > > > > I don't think we should be making this change. It's hiding the fact > > that > > > we > > > > > > intentionally create SiteInstances in different BrowsingInstances in > > some > > > > > cases, > > > > > > and it potentially causes problems if other people call > > > > GetSiteInstanceForURL > > > > > > and unexpectedly get back the speculative RFH's SiteInstance. > > > > > > > > > > > > Is the premise of the change that the speculative RFH calls > > > > > > GetSiteInstanceForURL in advance, then we call it again later at > commit > > > > time? > > > > > > In this case I agree that the SiteInstance returned at commit time > won't > > > > match > > > > > > the speculative RFH's SiteInstance if we created a SiteInstance in a > new > > > > > > BrowsingInstance. That fails the following check in > > > > > GetFrameHostForNavigation, > > > > > > right? > > > > > > if (!speculative_render_frame_host_ || > > > > > > speculative_render_frame_host_->GetSiteInstance() != > > > > > > dest_site_instance.get()) { > > > > > > > > > > > > > > > > > > I think it would be better to update that check than try to return the > > > > > > speculative RFH's SiteInstance. The check could see whether the > > > > SiteInstances > > > > > > were equal, or whether they (1) have the same site URL and (2) are > both > > in > > > > new > > > > > > BrowsingInstances that differ from the current one. > > > > > > > > > > As GetSiteInstanceForURL is a private method of RFHM I assumed that if > > > someone > > > > > chooses to call it he/she has to know what they are doing either by > > looking > > > at > > > > > the implementation or doc comment. I did try to make what it does clear > in > > > its > > > > > comment, that does mention the new BrowsingInstance part. But I agree > this > > > > code > > > > > can be moved somewhere else. > > > > > > > > I agree that it's a private method, but now that we're calling it in two > > > places, > > > > it has the appearance of being a safe method to call. I think we should > be > > > > careful about entangling it with the call sites. > > > > > > > > This is especially true because we'd like to have the option to call it > from > > > > other places as well. For example, we've wanted to use it from > > > > CrossSiteResourceHandler in the past to do a policy check on the UI > thread. > > > > (That particular case may be unnecessary once PlzNavigate lands, but it's > an > > > > example of why we should keep it independent from its callers.) > > > > > > I wrote my previous comment thinking about the new method I had added > > > (CreateSiteInstanceForURL) instead of the existing one you referred to > > > (GetSiteInstanceForURL). Under this "new" light I agree with your initial > > > concern and it would be better to change GetFrameHostForNavigation to do the > > > extra checks. But we would still be creating more SiteInstances than > strictly > > > necessary. > > Agreed. > > > > > > > An extra note: when you mention it's called from two places I think you are > in > > > fact referring to GetSiteInstanceForNavigation, which is called from both > > > GetFrameHostForNavigation and UpdateStateForNavigate, right? Just mind that > > this > > > duplication only exists temporarily as one is used by "current" navigations > > and > > > the other by PlzNavigate. The former will disappear eventually. > > I meant that PlzNavigate calls GetFrameHostForNavigation twice: once for the > speculative RFH from RFHM::BeginNavigation (via NavigatorImpl::BeginNavigation) > and once at commit time from NavigatorImpl::CommitNavigation. > > That's why it's useful to have an idempotent version that we can call at commit > time. Acknowledged. > > > > > The premises for this change are: > > > > > 1) We don't want new SiteInstances (under a new BrowsingInstance) to be > > > > created > > > > > multiple times for the current navigation if we still have access to a > > > > > previously, already created one. > > > > > > > > You raise an excellent point. By calling GetSiteInstanceForURL multiple > > > times, > > > > we're acting like it's idempotent, but it's not. It currently has side > > > effects, > > > > like actually creating a SiteInstance in either the current or a new > > > > BrowsingInstance. > > > > > > > > I wonder if we can explore another way to approach this. Could we > actually > > > make > > > > it idempotent (i.e., no side effects), so that the caller can decide > whether > > > to > > > > actually create a SiteInstance or just compare against an existing one? > I'm > > > not > > > > a fan of out-parameters, but either that or a returned struct could tell > the > > > > caller whether to (1) use a particular existing SiteInstance, (2) create a > > > > related SiteInstance with a given site URL, or (3) create a SiteInstance > in > > a > > > > new BrowsingInstance with a given site URL. > > > > > > > > We could even hide the ugliness behind a reasonable (but still private) > API. > > > > > > Something like: > > > > bool IsCorrectSiteInstanceForURL(...) // idempotent > > > > SiteInstance* GetSiteInstanceForURL(...) // potentially state changing > > > > > > > > void DetermineSiteInstanceForURL(..., out_params) // "really" private > > > > idempotent helper for both > > > > > > > > With that approach, PlzNavigate would call GetSiteInstanceForURL when > > creating > > > > the speculative RFH. We would call the idempotent IsCorrectSiteInstance > at > > > > commit time and check whether the speculative RFH's SiteInstance matches > > what > > > we > > > > need. > > > > > > > > I can't tell yet whether that will end up too awkward, but it seems better > > > than > > > > tying GetSiteInstanceForURL's behavior to the speculative RFH call sites > > > (patch > > > > 1) or trying to predict its output without calling it (patch 2). > > > > > > > > Thoughts? > > > > > > Yes, this would fully solve the problem but I can't help thinking it might > be > > an > > > overkill. This doubt comes from me wondering how "different" the matched > > > SiteInstance cases would be based on the destination URL changes from the > 1st > > > call to GetFrameHostForNavigation to the 2nd. I would think these would only > > (?) > > > change on server redirects and these could be covered by simpler tests > (versus > > > all that is currently done in GetSiteInstanceForURL). > > I don't think that having a separate version of that check would make things > simpler. It would be harder to maintain the two versions and more likely that > we would have security problems if they disagreed. Server redirects can > exercise a lot of the cases (e.g., crossing security boundaries for hosted apps > or the Chrome Web Store), so I think it makes sense to use the full check both > times. > > > > > > > But as having the precise SiteInstance is a very important security concern > in > > > Chrome, this seems to be the better, safer choice. > > Agreed. > > > > > > > The only improvement I can think of is to allow the SiteInstance > verification > > > and creation (when needed) to only call DetermineSiteInstanceForURL once. > > That's a tradeoff between calling the decision logic twice vs hiding the > out_params ugliness from callers. We'll have to see how clean or ugly the code > turns out to decide. > > > I > > > guess this would require a new method (for which out_params is an input > > > parameter): > > > > > > SiteInstance* CreatSiteInstanceForURL(out_params, ...) > > > > > > It would be called invariably by GetSiteInstanceForURL but PlzNavigate > > > (GetFrameHostForNavigation) would only call it when a new SiteInstance is > > > actually needed (instead of calling GetSiteInstanceForURL). > > > > > > WDYT? > > > > And of course a more appropriate name for that method would be something like > > GetAppropriateSiteInstance because it won't necessarily create a new one. > > If we're going to break them down like this, maybe we should just have: > > // Private inside RFHM: > struct SiteInstanceDescriptor { > SiteInstance* site_instance; // optional, for the cases we know it already > GURL site_url; // in case |site_instance| is null > bool is_related_to_current; // for knowing whether to create in a new > BrowsingInstance or not > }; > > SiteInstanceDescriptor DetermineSiteInstanceForURL(...); // All the policy > logic, with no side effects. > SiteInstance* GetSiteInstanceForDescriptor(SiteInstanceDescriptor); // just > returns or creates a SI. > > In cases that we want the real SiteInstance to be created, we call both (e.g., > in UpdateStateForNavigate, BeginNavigation). > In cases that we just want to compare, we call the first one (e.g., in > CommitNavigation). If the comparison fails, we call the second. I agree with your concern on the ugliness of that solution and I'll start with a change in the models of your first proposal. If we ever get to the point of having to worry about performance here (not likely I guess) we can revisit my suggestion.
This implements what was discussed previously. Right now render_frame_host_manager.cc has 2200+ LoC. I think the SiteInstance selection logic that now exists in DetermineSiteInstanceForURL(Internal) is orthogonal enough to be extracted to another file. Even if this new class (NavigationSiteInstanceSelector?) should have a tight connection with RFHM it would carry a very clear, specific task and would simplify RFHM subtracting about 200 LoC. If you agree with this, I'd do it in a following CL as this one is already complex enough. https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (left): https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:777: request.common_params().url); I fixed this bug (this is the 2nd time I think) of re-defining |should_reuse_web_ui_| inside GetFrameHostForNavigation. Seemed too simple of a change to do it in another CL. https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:820: CommitPending(); This early commit when the current frame is not live might be run *before* commit time, on a possibly non-final URL. This is a bug, right? https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:824: default: { NOTREACHED(); } I already added the adequate line breaks on my machine. https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1137: return descriptor.existent_site_instance == candidate_instance; Confirming: there should be no two SiteInstances under the same browsing instance with the same site URL, correct? IIUC, if two SiteInstances have the same site URL and the same browsing instance, they should in fact be the exact same SiteInstance. https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1157: SiteInstanceDescriptor RenderFrameHostManager::DetermineSiteInstanceForURL( I am returning SiteInstanceDescriptor by value here and in the *Internal method because it makes everything simpler. The coding style guide doesn't mention anything against it and even seems to consider it preferable (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Copyable_Mova...). The GURL is the only variable sized member we might worry about. WDYT? https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1338: } There's this |current_entry| change here in case the last committed entry is an interstitial. As the current entry is used before in DetermineSiteInstanceForURL, is this special treatment also needed there? https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:26: struct SiteInstanceDescriptor; Instead of the lose |out_params| I preferred creating the struct to hold them all concisely together. https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:488: RenderFrameHostToUse* rfh_to_use); The added logic to reuse the speculative RFH made GetFrameHostForNavigation too complex and it made sense to split it in two. Now we have: * ChooseFrameHostForNavigation that knows how to chose the RFH that should be used for a navigation. * GetFrameHostForNavigation that updates internal state depending on the RFH selected by ChooseFrameHostForNavigation. On this method signature: it follows the style guide but I don't like the way it looks. The enum result, the more important information, is an out parameter but the less important smart pointer is actually returned. Possible alternatives: switch the pointer to an out param (sounds dangerous?), return a std::pair with the enum and the pointer or return a struct (practically the same as the std::pair). WDYT? https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:537: bool force_browsing_instance_swap); I adapted this comment above so that it now fits what DetermineSiteInstanceForURL does. I tried merging the logic from GetSiteInstanceForNavigation and GetSiteInstanceForURL into a single DetermineSiteInstanceForURL method but the CHECK in the end of GetSiteInstanceForNavigation (asserts that the current SI is not used when a browsing instance change should happen) gets too complex. So I reverted back to having two separate methods as before.
Thanks! I'm not sure we have yet the right interface for the logic of selecting site instances. That said, I would second the idea of extracting this code to another file, as I think it would make it clearer, and all in all I'm having some troubles following the logic of the change. https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1157: SiteInstanceDescriptor RenderFrameHostManager::DetermineSiteInstanceForURL( On 2015/03/23 14:31:18, carlosk wrote: > I am returning SiteInstanceDescriptor by value here and in the *Internal method > because it makes everything simpler. The coding style guide doesn't mention > anything against it and even seems to consider it preferable > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Copyable_Mova...). > The GURL is the only variable sized member we might worry about. WDYT? Seems reasonable. https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:26: struct SiteInstanceDescriptor; On 2015/03/23 14:31:18, carlosk wrote: > Instead of the lose |out_params| I preferred creating the struct to hold them > all concisely together. Unnamed namespaces are forbidden in header files. If you want a struct for your parameters that are only used by private functions, you should declare it as a nested class inside the private part of your class. https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:429: enum RenderFrameHostToUse { This should be prefixed by a PlzNavigate comment to show that it is only used by PlzNavigate. https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:488: RenderFrameHostToUse* rfh_to_use); On 2015/03/23 14:31:18, carlosk wrote: > The added logic to reuse the speculative RFH made GetFrameHostForNavigation too > complex and it made sense to split it in two. Now we have: > * ChooseFrameHostForNavigation that knows how to chose the RFH that should be > used for a navigation. > * GetFrameHostForNavigation that updates internal state depending on the RFH > selected by ChooseFrameHostForNavigation. > > On this method signature: it follows the style guide but I don't like the way it > looks. The enum result, the more important information, is an out parameter but > the less important smart pointer is actually returned. > > Possible alternatives: switch the pointer to an out param (sounds dangerous?), > return a std::pair with the enum and the pointer or return a struct (practically > the same as the std::pair). WDYT? I don't like the signature of this method either, and I think we may not be doing the best logical split here. In particular, I find the split GetFrameHostForNavigation/ChooseFrameHostForNavigation hard to follow. https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:491: SiteInstance* GetSiteInstanceForNavigation(const GURL& dest_url, Is GetSiteInstanceForNavigation idempotent in the end? I think the caller code would be nicer if it were. In particular, we would not have to do the ChooseFrameHostForNavigation/GetFrameHostForNavigation split. How about we have GetSiteInstanceForNavigation take the current instances as parameters (both the current or speculative). It would then check if either of these can be used for the navigation through IsCorrectSiteInstanceForURL and return it if true. If not, it would create a new one. Then we keep the logic in GetFrameHostForNavigation untouched, while not making the one in GetSiteInstanceForNavigation much more complex. https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:529: SiteInstanceDescriptor DetermineSiteInstanceForURLInternal( Why do we have a DetermineSiteInstanceForURL and a DetermineSiteInstanceForURL considering that they are both private?
Thanks. https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:26: struct SiteInstanceDescriptor; On 2015/03/23 15:26:44, clamy wrote: > On 2015/03/23 14:31:18, carlosk wrote: > > Instead of the lose |out_params| I preferred creating the struct to hold them > > all concisely together. > > Unnamed namespaces are forbidden in header files. If you want a struct for your > parameters that are only used by private functions, you should declare it as a > nested class inside the private part of your class. Done. https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:429: enum RenderFrameHostToUse { On 2015/03/23 15:26:44, clamy wrote: > This should be prefixed by a PlzNavigate comment to show that it is only used by > PlzNavigate. Done. I also moved the friend-related macro below to tag along the other friend definitions. https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:488: RenderFrameHostToUse* rfh_to_use); On 2015/03/23 15:26:44, clamy wrote: > On 2015/03/23 14:31:18, carlosk wrote: > > The added logic to reuse the speculative RFH made GetFrameHostForNavigation > too > > complex and it made sense to split it in two. Now we have: > > * ChooseFrameHostForNavigation that knows how to chose the RFH that should be > > used for a navigation. > > * GetFrameHostForNavigation that updates internal state depending on the RFH > > selected by ChooseFrameHostForNavigation. > > > > On this method signature: it follows the style guide but I don't like the way > it > > looks. The enum result, the more important information, is an out parameter > but > > the less important smart pointer is actually returned. > > > > Possible alternatives: switch the pointer to an out param (sounds dangerous?), > > return a std::pair with the enum and the pointer or return a struct > (practically > > the same as the std::pair). WDYT? > > I don't like the signature of this method either, and I think we may not be > doing the best logical split here. In particular, I find the split > GetFrameHostForNavigation/ChooseFrameHostForNavigation hard to follow. What I think is nice about the two methods is that it's very clear what the possible RFH choices are (due to the enum), how they are chosen and what happens in each case (in the switch block). https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:491: SiteInstance* GetSiteInstanceForNavigation(const GURL& dest_url, On 2015/03/23 15:26:44, clamy wrote: > Is GetSiteInstanceForNavigation idempotent in the end? I think the caller code > would be nicer if it were. In particular, we would not have to do the > ChooseFrameHostForNavigation/GetFrameHostForNavigation split. > > How about we have GetSiteInstanceForNavigation take the current instances as > parameters (both the current or speculative). It would then check if either of > these can be used for the navigation through IsCorrectSiteInstanceForURL and > return it if true. If not, it would create a new one. Then we keep the logic in > GetFrameHostForNavigation untouched, while not making the one in > GetSiteInstanceForNavigation much more complex. GetSiteInstanceForNavigation is not idempotent as it must comply with the current behavior (it will create new SiteInstances and has no knowledge of the speculative RFH). IsCorrectSiteInstanceForURL and DetermineSiteInstanceForURL are idempotent. We would achieve what you are suggesting by merging IsCorrectSiteInstanceForURL into GetSiteInstanceForNavigation and keeping |candidate_instance| as an optional parameter (PlzNavigate would use it, normal navigations wouldn't). That would indeed allow GetFrameHostForNavigation to be kept unchanged. WDYT? https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:529: SiteInstanceDescriptor DetermineSiteInstanceForURLInternal( On 2015/03/23 15:26:44, clamy wrote: > Why do we have a DetermineSiteInstanceForURL and a DetermineSiteInstanceForURL > considering that they are both private? I guess you are asking about the DetermineSiteInstanceForURL and DetermineSiteInstanceForURLInternal, right? These map directly from GetSiteInstanceForNavigation and GetSiteInstanceForURL, respectively. The reason why I kept this logic in two methods is explained in the comment below.
Just and extra note. https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:824: default: { NOTREACHED(); } On 2015/03/23 14:31:18, carlosk wrote: > I already added the adequate line breaks on my machine. Surprisingly, this is the formating that git cl format enforces.
Sorry-- I tried to get to this today but didn't have time, due to an unexpected security bug that came up. This looks like it will take some time to review carefully. I'll aim to fit it in tomorrow. One note: I'm not eager to move this logic out of RFHM. This class is where SiteInstance decisions are made, and it exists largely for that purpose. Let's leave it in this class until we have a chance to see how this API ends up.
Patchset #7 (id:120001) has been deleted
carlosk@chromium.org changed required reviewers: + creis@chromium.org - nasko@chromium.org
Following clamy@'s suggestion I merged IsCorrectSiteInstanceForURL into GetSiteInstanceForNavigation (see comments on this) what made this change simpler overall as GetFrameHostForNavigation requires very little modification. Even though I liked the previous split of GetFrameHostForNavigation, this version keeps its implementation simpler as the reuse-speculative-RFH-for-its-site-instance-is-still-valid case is handled transparently. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1063: bool RenderFrameHostManager::SiteInstanceMatchesDescription( The merge of GetSiteInstanceForNavigation with IsCorrectSiteInstanceForURL was on the API level but I kept their logic separate (SiteInstanceMatchesDescription is the new IsCorrectSiteInstanceForURL) because it is simpler to follow it that way. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1249: current_instance_impl->SetSite(dest_url); This whole method seems to be mostly read-only (I didn't look through all the calls from here though) apart from this SetSite here. Is this something we should change? https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:514: bool SiteInstanceMatchesDescription(SiteInstance* candidate_instance, I wanted to move this method into an unnamed namespace but it would require un-privatizing SiteInstanceDescriptor. So... WDYT?
Round of comments below. I can see where this is heading, and I think it might work. Let's try to avoid unrelated changes here whenever we can and focus on the goal: making this code idempotent. (Note: I didn't have time to go through all of the discussion in the review comments, so I apologize if I missed some questions.) https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (left): https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:776: bool should_reuse_web_ui_ = ShouldReuseWebUI(current_navigation_entry, Thanks for catching this! This is a potentially nasty bug. Please pull it out into its own CL ASAP. (In general, please don't mix unrelated bug fixes into large CLs like this.) Do you know what the implications are? Are they specific to PlzNavigate? https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1036: return current_instance; In many places, we intentionally return early, to make the rest of the function easier to read and reason about. For example, it isn't indented as far, and you don't have to worry about reading further ahead. Please don't change this. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:748: ? speculative_render_frame_host_->GetSiteInstance() Style nit: All of the other uses of the ternary operator in this file put it at the end of the previous line. Please stay consistent (unless git cl format explicitly overrides this). https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1043: BrowserContext* browser_context = The rest of this function feels like it should be a general utility function that converts a SiteInstanceDescriptor into a SiteInstance. That would make it easier to write unit tests for. (That might also let us keep GetSiteInstanceForNavigation as it was before, eliminate DetermineSiteInstanceForURL, and drop the "Internal" from DetermineSiteInstanceForURLInternal's name. We would just call the utility function at each return point of GetSiteInstanceForNavigation.) https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1078: if (descriptor.new_site_is_related_to_current == candidate_is_related) { This code feels like it's trying to be too clever, which tends to make things harder to debug. Let's make an effort to make it easier to understand. I'd suggest using != here and returning false early. Then return whether the site URLs match. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1080: SiteInstanceImpl::GetSiteForURL( This seems wrong to me. We shouldn't need to call GetSiteForURL here, since we should only be storing site URLs in SiteInstanceDescriptor. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1098: bool force_browsing_instance_swap = false; This isn't needed this early. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1136: CHECK_NE(current_instance, descriptor.existent_site_instance); This new check is not sufficient. If descriptor has a URL and is related, it might match current_instance. (Even if that's not possible given how the code is currently structured, we should enforce it in this CHECK to make sure it doesn't regress. This gives us a clear way to reason about this point of the code.) https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1249: current_instance_impl->SetSite(dest_url); On 2015/03/24 15:30:06, carlosk wrote: > This whole method seems to be mostly read-only (I didn't look through all the > calls from here though) apart from this SetSite here. Is this something we > should change? Yes, Camille had a CL that ran into this as well. I don't have time to look it up right now, but we might be able to revisit this case. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:437: // can whether point to an existent one or store the details needed to create nit: can point (drop "whether") s/existent/existing/ (everywhere) https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:455: bool new_site_is_related_to_current; "site" doesn't fit well in this name. This descriptor is about SiteInstances, not sites. Let's call this new_is_related_to_current instead. In that sense, "new_site_url" means "the site URL for the new SiteInstance" and "new_is_related_to_current" means "the new SiteInstance is related to the current SiteInstance." https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:512: // Returns true if |candidate_instance| confirms to the SiteInstance specified "confirms to" isn't grammatically correct. "matches" would work. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:514: bool SiteInstanceMatchesDescription(SiteInstance* candidate_instance, On 2015/03/24 15:30:06, carlosk wrote: > I wanted to move this method into an unnamed namespace but it would require > un-privatizing SiteInstanceDescriptor. So... WDYT? Please keep SiteInstanceDescriptor private. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:519: // SiteInstance. It will check ShouldTransitionCrossSite and This second sentence is an implementation detail and isn't needed here anymore. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:521: // |source_instance| is the SiteInstance of the frame that initiated the nit: Add an empty comment line above this. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:522: // navigation. |dest_instance|, is a predetermined SiteInstance Remove comma. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:523: // that'll be used if not null. Internally, |current_instance| is the that'll -> that will https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:528: // This is a helper function for GetSiteInstanceForNavigation and nit: Add an empty comment line above this. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:537: SiteInstanceDescriptor DetermineSiteInstanceForURLInternal( These functions are already private internal details. Please just have the latter one and drop the word "Internal" from the name.
Patchset #8 (id:160001) has been deleted
Thanks! With the latest suggestions from creis@ the change is now smaller as both RFHM::GetFrameHostForNavigation and RFHM::GetSiteInstanceForNavigation contain only minor modifications. Note: the idempotent behavior is achieved by providing the potential |candidate_instance| to GetSiteInstanceForNavigation. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (left): https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:776: bool should_reuse_web_ui_ = ShouldReuseWebUI(current_navigation_entry, On 2015/03/25 00:09:25, Charlie Reis wrote: > Thanks for catching this! This is a potentially nasty bug. Please pull it out > into its own CL ASAP. (In general, please don't mix unrelated bug fixes into > large CLs like this.) > > Do you know what the implications are? Are they specific to PlzNavigate? It's been pulled into crrev.com/1012863004. This only affects PlzNavigate. What would happen is that in a same-site navigation where the current WebUI should be reused, we'd get a crash when calling CommitPending when trying to get the |speculative_web_ui_|. I couldn't find any content unit tests that would cause this state though. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1036: return current_instance; On 2015/03/25 00:09:25, Charlie Reis wrote: > In many places, we intentionally return early, to make the rest of the function > easier to read and reason about. For example, it isn't indented as far, and you > don't have to worry about reading further ahead. Please don't change this. Done. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:748: ? speculative_render_frame_host_->GetSiteInstance() On 2015/03/25 00:09:25, Charlie Reis wrote: > Style nit: All of the other uses of the ternary operator in this file put it at > the end of the previous line. Please stay consistent (unless git cl format > explicitly overrides this). This has been git-cl-format-ed. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1043: BrowserContext* browser_context = On 2015/03/25 00:09:25, Charlie Reis wrote: > The rest of this function feels like it should be a general utility function > that converts a SiteInstanceDescriptor into a SiteInstance. That would make it > easier to write unit tests for. > > (That might also let us keep GetSiteInstanceForNavigation as it was before, > eliminate DetermineSiteInstanceForURL, and drop the "Internal" from > DetermineSiteInstanceForURLInternal's name. We would just call the utility > function at each return point of GetSiteInstanceForNavigation.) I consolidated some methods as you suggested and reverted GetSiteInstanceForNavigation to something very similar to what it was before. I am not sure of what you meant with "general utility function". It sounded like it should be moved out of RFHM but that would require un-privatizing SiteInstanceDescriptor and you mentioned that would not be a good thing to do. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1078: if (descriptor.new_site_is_related_to_current == candidate_is_related) { On 2015/03/25 00:09:25, Charlie Reis wrote: > This code feels like it's trying to be too clever, which tends to make things > harder to debug. Let's make an effort to make it easier to understand. > > I'd suggest using != here and returning false early. > > Then return whether the site URLs match. As I finally merged this method into the new descriptor conversion method, this logic was slightly changed and it made more sense to keep the "==". This is where I think large files worsen the way we code. For better organization and clarity I'd prefer to keep this logic smaller and in two separate methods. But as RFHM already has too many methods, adding yet another one has the opposite effect. Extracting the SiteInstance logic might not be the right thing to do but we should consider other options. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1080: SiteInstanceImpl::GetSiteForURL( On 2015/03/25 00:09:25, Charlie Reis wrote: > This seems wrong to me. We shouldn't need to call GetSiteForURL here, since we > should only be storing site URLs in SiteInstanceDescriptor. You meant that this conversion (from destination URL to site URL) should have already happened when creating the descriptor, right? I updated the code to do that. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1098: bool force_browsing_instance_swap = false; On 2015/03/25 00:09:25, Charlie Reis wrote: > This isn't needed this early. It needed to be defined here but with the elimination of the if-block (due to your other comment on returning early) I updated this part accordingly. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1136: CHECK_NE(current_instance, descriptor.existent_site_instance); On 2015/03/25 00:09:25, Charlie Reis wrote: > This new check is not sufficient. If descriptor has a URL and is related, it > might match current_instance. > > (Even if that's not possible given how the code is currently structured, we > should enforce it in this CHECK to make sure it doesn't regress. This gives us > a clear way to reason about this point of the code.) It is indeed a possible outcome and I updated the code to cover it. But just checking: that would represent an invalid SiteInstance, right? My undertanding is that if a SI should be related to another one and have the same site URL they should in fact be the exact same instance so that pointer comparisons would return true. Hence we being able to pointer-compare SiteInstances for equality. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1249: current_instance_impl->SetSite(dest_url); On 2015/03/25 00:09:25, Charlie Reis wrote: > On 2015/03/24 15:30:06, carlosk wrote: > > This whole method seems to be mostly read-only (I didn't look through all the > > calls from here though) apart from this SetSite here. Is this something we > > should change? > > Yes, Camille had a CL that ran into this as well. I don't have time to look it > up right now, but we might be able to revisit this case. Acknowledged. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:437: // can whether point to an existent one or store the details needed to create On 2015/03/25 00:09:25, Charlie Reis wrote: > nit: can point (drop "whether") > > s/existent/existing/ (everywhere) Done. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:455: bool new_site_is_related_to_current; On 2015/03/25 00:09:26, Charlie Reis wrote: > "site" doesn't fit well in this name. This descriptor is about SiteInstances, > not sites. > > Let's call this new_is_related_to_current instead. In that sense, > "new_site_url" means "the site URL for the new SiteInstance" and > "new_is_related_to_current" means "the new SiteInstance is related to the > current SiteInstance." Done. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:512: // Returns true if |candidate_instance| confirms to the SiteInstance specified On 2015/03/25 00:09:25, Charlie Reis wrote: > "confirms to" isn't grammatically correct. "matches" would work. It was a typo: I meant to write "conforms". But "matches" sounds good as well. Done. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:514: bool SiteInstanceMatchesDescription(SiteInstance* candidate_instance, On 2015/03/25 00:09:25, Charlie Reis wrote: > On 2015/03/24 15:30:06, carlosk wrote: > > I wanted to move this method into an unnamed namespace but it would require > > un-privatizing SiteInstanceDescriptor. So... WDYT? > > Please keep SiteInstanceDescriptor private. Acknowledged. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:519: // SiteInstance. It will check ShouldTransitionCrossSite and On 2015/03/25 00:09:25, Charlie Reis wrote: > This second sentence is an implementation detail and isn't needed here anymore. Done. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:521: // |source_instance| is the SiteInstance of the frame that initiated the On 2015/03/25 00:09:25, Charlie Reis wrote: > nit: Add an empty comment line above this. Done. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:522: // navigation. |dest_instance|, is a predetermined SiteInstance On 2015/03/25 00:09:25, Charlie Reis wrote: > Remove comma. Done. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:523: // that'll be used if not null. Internally, |current_instance| is the On 2015/03/25 00:09:25, Charlie Reis wrote: > that'll -> that will Done. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:528: // This is a helper function for GetSiteInstanceForNavigation and On 2015/03/25 00:09:26, Charlie Reis wrote: > nit: Add an empty comment line above this. Done. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:537: SiteInstanceDescriptor DetermineSiteInstanceForURLInternal( On 2015/03/25 00:09:25, Charlie Reis wrote: > These functions are already private internal details. Please just have the > latter one and drop the word "Internal" from the name. I explained this here: https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho... But following the suggestion from another comment the *Internal version was in fact eliminated.
Thanks-- it's much easier to reason about the CL now that it's focused on the primary change. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1043: BrowserContext* browser_context = On 2015/03/30 14:37:37, carlosk wrote: > On 2015/03/25 00:09:25, Charlie Reis wrote: > > The rest of this function feels like it should be a general utility function > > that converts a SiteInstanceDescriptor into a SiteInstance. That would make > it > > easier to write unit tests for. > > > > (That might also let us keep GetSiteInstanceForNavigation as it was before, > > eliminate DetermineSiteInstanceForURL, and drop the "Internal" from > > DetermineSiteInstanceForURLInternal's name. We would just call the utility > > function at each return point of GetSiteInstanceForNavigation.) > > I consolidated some methods as you suggested and reverted > GetSiteInstanceForNavigation to something very similar to what it was before. > > I am not sure of what you meant with "general utility function". It sounded like > it should be moved out of RFHM but that would require un-privatizing > SiteInstanceDescriptor and you mentioned that would not be a good thing to do. Yes, I meant a private method in RFHM, as you've done. Thanks. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1078: if (descriptor.new_site_is_related_to_current == candidate_is_related) { On 2015/03/30 14:37:37, carlosk wrote: > On 2015/03/25 00:09:25, Charlie Reis wrote: > > This code feels like it's trying to be too clever, which tends to make things > > harder to debug. Let's make an effort to make it easier to understand. > > > > I'd suggest using != here and returning false early. > > > > Then return whether the site URLs match. > > As I finally merged this method into the new descriptor conversion method, this > logic was slightly changed and it made more sense to keep the "==". > > This is where I think large files worsen the way we code. For better > organization and clarity I'd prefer to keep this logic smaller and in two > separate methods. But as RFHM already has too many methods, adding yet another > one has the opposite effect. Extracting the SiteInstance logic might not be the > right thing to do but we should consider other options. I don't agree with this conclusion. There are times when it makes sense to break up a class, but even when we have a large class that doesn't prevent us from creating helper methods. https://codereview.chromium.org/967383002/diff/140001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1136: CHECK_NE(current_instance, descriptor.existent_site_instance); On 2015/03/30 14:37:37, carlosk wrote: > On 2015/03/25 00:09:25, Charlie Reis wrote: > > This new check is not sufficient. If descriptor has a URL and is related, it > > might match current_instance. > > > > (Even if that's not possible given how the code is currently structured, we > > should enforce it in this CHECK to make sure it doesn't regress. This gives > us > > a clear way to reason about this point of the code.) > > It is indeed a possible outcome and I updated the code to cover it. > > But just checking: that would represent an invalid SiteInstance, right? My > undertanding is that if a SI should be related to another one and have the same > site URL they should in fact be the exact same instance so that pointer > comparisons would return true. Hence we being able to pointer-compare > SiteInstances for equality. I was referring to the case that descriptor.existent_site_instance was null but its site URL referred to current_instance. That's a valid descriptor but would indicate a bug in the code. That said, I agree that we can do a pointer comparison on the SiteInstances if we convert from the descriptor before the CHECK_NE. https://codereview.chromium.org/967383002/diff/200001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (left): https://codereview.chromium.org/967383002/diff/200001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1038: // Determine if we need a new BrowsingInstance for this entry. If true, this nit: Please do not add churn on these lines just to remove a space. Same below. https://codereview.chromium.org/967383002/diff/200001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/967383002/diff/200001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:904: GURL site_url, This is the wrong name if it's not already the Site URL. Perhaps url or dest_url would be better. https://codereview.chromium.org/967383002/diff/200001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1076: SiteInstanceDescriptor descriptor = SiteInstanceDescriptor(current_instance); descriptor isn't a useful name, since it's not clear which one it's for. new_instance or new_instance_descriptor would be better, as before. https://codereview.chromium.org/967383002/diff/200001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1095: return ConvertToSiteInstance(descriptor, candidate_instance); If we put the ConvertToSiteInstance call earlier (line 1083), could we leave the force_swap CHECK as it was originally (doing a pointer compare as you suggest)? https://codereview.chromium.org/967383002/diff/200001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1270: BrowserContext* browser_context = We don't need this until the last line, and most of the branches don't need it at all. Let's move it to the end. https://codereview.chromium.org/967383002/diff/200001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1274: if (candidate_instance) { This control flow seems odd to me. The only case that really matters in this block is returning candidate_instance if neither it nor descriptor are related to the current and if their sites match. The existing_site_instance case is handled fine by line 1292, so at least we can move that up before this block. In the is_related case, GetRelatedSiteInstance will return candidate_instance if their sites match, so that seems like it's covered by line 1295. (Note: there's a race possible where that might not be true, as noted on site_instance_map_ in browsing_instance.h. However, I don't think that race is necessary in PlzNavigate, since we should always be able to assign sites in advance. That's something we should aim for if possible.) https://codereview.chromium.org/967383002/diff/200001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1280: render_frame_host_->GetSiteInstance()->IsRelatedSiteInstance( You've already declared current_instance, so there's no need to call GetSiteInstance() again. https://codereview.chromium.org/967383002/diff/200001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1295: if (descriptor.new_is_related_to_current) { No braces. https://codereview.chromium.org/967383002/diff/200001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/967383002/diff/200001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:436: // one. Sounds good. Can you also mention that the purpose of the struct is to allow checks and comparisons without necessarily creating a new SiteInstance? https://codereview.chromium.org/967383002/diff/200001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:448: // In case |existing_site_instance| is null, specify a new site URL. nit: Blank lines between these members. https://codereview.chromium.org/967383002/diff/200001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:510: // SiteInstance. Also mention that the SiteInstance can be obtained from the descriptor using ConvertToSiteInstance. https://codereview.chromium.org/967383002/diff/200001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:513: // navigation. |dest_instance| is a predetermined SiteInstance that will be Please do not reorder these sentences, so that they continue to match the order of the source/current/dest parameters. https://codereview.chromium.org/967383002/diff/200001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:533: // description, it is returned verbatim. s/verbatim/as is/
Thanks. https://chromiumcodereview.appspot.com/967383002/diff/140001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/967383002/diff/140001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:1136: CHECK_NE(current_instance, descriptor.existent_site_instance); On 2015/03/31 06:18:18, Charlie Reis wrote: > On 2015/03/30 14:37:37, carlosk wrote: > > On 2015/03/25 00:09:25, Charlie Reis wrote: > > > This new check is not sufficient. If descriptor has a URL and is related, > it > > > might match current_instance. > > > > > > (Even if that's not possible given how the code is currently structured, we > > > should enforce it in this CHECK to make sure it doesn't regress. This gives > > us > > > a clear way to reason about this point of the code.) > > > > It is indeed a possible outcome and I updated the code to cover it. > > > > But just checking: that would represent an invalid SiteInstance, right? My > > undertanding is that if a SI should be related to another one and have the > same > > site URL they should in fact be the exact same instance so that pointer > > comparisons would return true. Hence we being able to pointer-compare > > SiteInstances for equality. > > I was referring to the case that descriptor.existent_site_instance was null but > its site URL referred to current_instance. That's a valid descriptor but would > indicate a bug in the code. Yes, I had understood that. > That said, I agree that we can do a pointer comparison on the SiteInstances if > we convert from the descriptor before the CHECK_NE. I was not suggesting a change here but just trying to confirm if that would be a representation of an invalid SiteInstance as I thought. And it was as I thought. https://chromiumcodereview.appspot.com/967383002/diff/200001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (left): https://chromiumcodereview.appspot.com/967383002/diff/200001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:1038: // Determine if we need a new BrowsingInstance for this entry. If true, this On 2015/03/31 06:18:18, Charlie Reis wrote: > nit: Please do not add churn on these lines just to remove a space. Same below. I reverted it here but as below I'm also adding further clarifications I let the change as is. https://chromiumcodereview.appspot.com/967383002/diff/200001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/967383002/diff/200001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:904: GURL site_url, On 2015/03/31 06:18:18, Charlie Reis wrote: > This is the wrong name if it's not already the Site URL. Perhaps url or > dest_url would be better. Done. https://chromiumcodereview.appspot.com/967383002/diff/200001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:1076: SiteInstanceDescriptor descriptor = SiteInstanceDescriptor(current_instance); On 2015/03/31 06:18:18, Charlie Reis wrote: > descriptor isn't a useful name, since it's not clear which one it's for. > new_instance or new_instance_descriptor would be better, as before. Done. https://chromiumcodereview.appspot.com/967383002/diff/200001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:1095: return ConvertToSiteInstance(descriptor, candidate_instance); On 2015/03/31 06:18:18, Charlie Reis wrote: > If we put the ConvertToSiteInstance call earlier (line 1083), could we leave the > force_swap CHECK as it was originally (doing a pointer compare as you suggest)? Done. https://chromiumcodereview.appspot.com/967383002/diff/200001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:1270: BrowserContext* browser_context = On 2015/03/31 06:18:18, Charlie Reis wrote: > We don't need this until the last line, and most of the branches don't need it > at all. Let's move it to the end. Done. https://chromiumcodereview.appspot.com/967383002/diff/200001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:1274: if (candidate_instance) { On 2015/03/31 06:18:18, Charlie Reis wrote: > This control flow seems odd to me. The only case that really matters in this > block is returning candidate_instance if neither it nor descriptor are related > to the current and if their sites match. > > The existing_site_instance case is handled fine by line 1292, so at least we can > move that up before this block. > > In the is_related case, GetRelatedSiteInstance will return candidate_instance if > their sites match, so that seems like it's covered by line 1295. (Note: there's > a race possible where that might not be true, as noted on site_instance_map_ in > browsing_instance.h. However, I don't think that race is necessary in > PlzNavigate, since we should always be able to assign sites in advance. That's > something we should aim for if possible.) Yes, I didn't notice the coincidences in logic that you pointed out here. I updated accordingly and added notes to clarify the logic in regards to the candidate. I'm not sure about the race conditions but it seems to me that they won't happen as all SiteInstance manipulations in PlzNavigate are handled (serially) by the UI thread. https://chromiumcodereview.appspot.com/967383002/diff/200001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:1280: render_frame_host_->GetSiteInstance()->IsRelatedSiteInstance( On 2015/03/31 06:18:18, Charlie Reis wrote: > You've already declared current_instance, so there's no need to call > GetSiteInstance() again. Done. https://chromiumcodereview.appspot.com/967383002/diff/200001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:1295: if (descriptor.new_is_related_to_current) { On 2015/03/31 06:18:19, Charlie Reis wrote: > No braces. Done. https://chromiumcodereview.appspot.com/967383002/diff/200001/content/browser/... File content/browser/frame_host/render_frame_host_manager.h (right): https://chromiumcodereview.appspot.com/967383002/diff/200001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:436: // one. On 2015/03/31 06:18:19, Charlie Reis wrote: > Sounds good. Can you also mention that the purpose of the struct is to allow > checks and comparisons without necessarily creating a new SiteInstance? Done. https://chromiumcodereview.appspot.com/967383002/diff/200001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:448: // In case |existing_site_instance| is null, specify a new site URL. On 2015/03/31 06:18:19, Charlie Reis wrote: > nit: Blank lines between these members. Done. https://chromiumcodereview.appspot.com/967383002/diff/200001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:510: // SiteInstance. On 2015/03/31 06:18:19, Charlie Reis wrote: > Also mention that the SiteInstance can be obtained from the descriptor using > ConvertToSiteInstance. Done. https://chromiumcodereview.appspot.com/967383002/diff/200001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:513: // navigation. |dest_instance| is a predetermined SiteInstance that will be On 2015/03/31 06:18:19, Charlie Reis wrote: > Please do not reorder these sentences, so that they continue to match the order > of the source/current/dest parameters. Reordered them as the original file. It made sense before as I had changed the signature to not passing the current one. Hence the "Internally" that is now removed. https://chromiumcodereview.appspot.com/967383002/diff/200001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:533: // description, it is returned verbatim. On 2015/03/31 06:18:19, Charlie Reis wrote: > s/verbatim/as is/ Done.
Great. This looks good, and it's ready to land apart from tests. I'd suggest some unit tests in render_frame_host_manager_unittest.cc that exercise ConvertToSiteInstance. (Is there also an easy way to test that we're no longer creating an extra SiteInstance in PlzNavigate for the unrelated case, or is that tricky?) https://codereview.chromium.org/967383002/diff/200001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (left): https://codereview.chromium.org/967383002/diff/200001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1038: // Determine if we need a new BrowsingInstance for this entry. If true, this On 2015/03/31 16:38:19, carlosk wrote: > On 2015/03/31 06:18:18, Charlie Reis wrote: > > nit: Please do not add churn on these lines just to remove a space. Same > below. > > I reverted it here but as below I'm also adding further clarifications I let the > change as is. That's fine in the cases where you're actually changing the comment, but there's a bunch of comments below where the only change is the removal of a space. Please remove that unnecessary churn. (To make this concrete, it matters when we're digging through source history to find why a comment was added. It's distracting to have to sift through unrelated CLs that simply change spacing.) https://codereview.chromium.org/967383002/diff/220001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/967383002/diff/220001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1086: // If |force_swap| is true, we must use a different SiteInstance than the nit: Blank line before. https://codereview.chromium.org/967383002/diff/220001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/967383002/diff/220001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:454: // be created in new BrowsingInstance or not. nit: in a
Patchset #11 (id:240001) has been deleted
Thanks once more. https://chromiumcodereview.appspot.com/967383002/diff/200001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (left): https://chromiumcodereview.appspot.com/967383002/diff/200001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:1038: // Determine if we need a new BrowsingInstance for this entry. If true, this On 2015/03/31 20:19:58, Charlie Reis wrote: > On 2015/03/31 16:38:19, carlosk wrote: > > On 2015/03/31 06:18:18, Charlie Reis wrote: > > > nit: Please do not add churn on these lines just to remove a space. Same > > below. > > > > I reverted it here but as below I'm also adding further clarifications I let > the > > change as is. > > That's fine in the cases where you're actually changing the comment, but there's > a bunch of comments below where the only change is the removal of a space. > Please remove that unnecessary churn. (To make this concrete, it matters when > we're digging through source history to find why a comment was added. It's > distracting to have to sift through unrelated CLs that simply change spacing.) Sorry about that, I forgot about those. The plain space removals are now reverted. https://chromiumcodereview.appspot.com/967383002/diff/220001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/967383002/diff/220001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:1086: // If |force_swap| is true, we must use a different SiteInstance than the On 2015/03/31 20:19:58, Charlie Reis wrote: > nit: Blank line before. Done. https://chromiumcodereview.appspot.com/967383002/diff/220001/content/browser/... File content/browser/frame_host/render_frame_host_manager.h (right): https://chromiumcodereview.appspot.com/967383002/diff/220001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:454: // be created in new BrowsingInstance or not. On 2015/03/31 20:19:58, Charlie Reis wrote: > nit: in a Done.
Did you see my request for tests in the last comment? Sorry if that slipped by.
On 2015/04/01 16:08:28, Charlie Reis wrote: > Did you see my request for tests in the last comment? Sorry if that slipped by. My bad. Tests are included in the latest patch. https://chromiumcodereview.appspot.com/967383002/diff/280001/content/browser/... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://chromiumcodereview.appspot.com/967383002/diff/280001/content/browser/... content/browser/frame_host/navigator_impl_unittest.cc:1026: } I used a lot of scoping here to avoid mixing up things from one step to the next. The variables that I wanted to keep are all defined at the top test level.
Great-- that's a nice and comprehensive test. LGTM with nits. https://codereview.chromium.org/967383002/diff/280001/content/browser/frame_h... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/967383002/diff/280001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:1010: TEST_F(NavigatorTestWithBrowserSideNavigation, Let's give a roadmap for the test in a comment before this line. Something like: Tests several cases for converting SiteInstanceDescriptors into SiteInstances: 1) Pointer to current. 2) Pointer to unrelated. 3) Same-site URL, related. 4) Cross-site URL, related. 5) Same-site URL, unrelated (with and without candidate SiteInstances). 6) Cross-site URL, unrelated (with candidate SiteInstances). (We can then number each of the cases below to make it easy to skim.) https://codereview.chromium.org/967383002/diff/280001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:1026: } On 2015/04/02 17:15:05, carlosk wrote: > I used a lot of scoping here to avoid mixing up things from one step to the > next. The variables that I wanted to keep are all defined at the top test level. Acknowledged. https://codereview.chromium.org/967383002/diff/280001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:1068: SiteInstanceImpl::GetSiteForURL(browser_context(), kUrlSameSiteAs2), nit: s/SiteInstanceImpl/SiteInstance/ Also, I'm noticing that we tend to put BrowserContext as the first parameter on methods like this, with the URL (etc) following. Let's do the same on the SiteInstanceDescriptor to be consistent. https://codereview.chromium.org/967383002/diff/280001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:1107: // Should return converted_instance_1 nit: End with period. Also mention "because the site matches and it is unrelated to the current SiteInstance." https://codereview.chromium.org/967383002/diff/280001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:1123: EXPECT_NE(unrelated_instance.get(), converted_instance.get()); As in case #5, can we add a line to ensure that ConvertToSiteInstance(rfhm, descriptor, unrelated_instance.get()) returns unrelated_instance? https://codereview.chromium.org/967383002/diff/280001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/967383002/diff/280001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1280: if (candidate_instance && nit: Add a comment saying that at this point, we need to return an unrelated SiteInstance for |new_site_url|. That makes this check and the code below it easy to follow. https://codereview.chromium.org/967383002/diff/280001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/967383002/diff/280001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:427: enum RenderFrameHostToUse { This is dead code now, right? Sorry for missing that before.
Patchset #13 (id:300001) has been deleted
Thanks! https://chromiumcodereview.appspot.com/967383002/diff/280001/content/browser/... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://chromiumcodereview.appspot.com/967383002/diff/280001/content/browser/... content/browser/frame_host/navigator_impl_unittest.cc:1010: TEST_F(NavigatorTestWithBrowserSideNavigation, On 2015/04/02 21:23:25, Charlie Reis wrote: > Let's give a roadmap for the test in a comment before this line. Something > like: > > Tests several cases for converting SiteInstanceDescriptors into SiteInstances: > 1) Pointer to current. > 2) Pointer to unrelated. > 3) Same-site URL, related. > 4) Cross-site URL, related. > 5) Same-site URL, unrelated (with and without candidate SiteInstances). > 6) Cross-site URL, unrelated (with candidate SiteInstances). > > (We can then number each of the cases below to make it easy to skim.) Done. https://chromiumcodereview.appspot.com/967383002/diff/280001/content/browser/... content/browser/frame_host/navigator_impl_unittest.cc:1068: SiteInstanceImpl::GetSiteForURL(browser_context(), kUrlSameSiteAs2), On 2015/04/02 21:23:25, Charlie Reis wrote: > nit: s/SiteInstanceImpl/SiteInstance/ > > Also, I'm noticing that we tend to put BrowserContext as the first parameter on > methods like this, with the URL (etc) following. Let's do the same on the > SiteInstanceDescriptor to be consistent. Done. Also replaced SiteInstanceImpl::Create + SetSite with SiteInstance::CreateForURL so I didn't need a SiteInstanceImpl pointer anymore. And I held back my temptation to s/SiteInstanceImpl::GetSiteForURL/SiteInstance::GetSiteForURL/ all over the place. :) https://chromiumcodereview.appspot.com/967383002/diff/280001/content/browser/... content/browser/frame_host/navigator_impl_unittest.cc:1107: // Should return converted_instance_1 On 2015/04/02 21:23:25, Charlie Reis wrote: > nit: End with period. > Also mention "because the site matches and it is unrelated to the current > SiteInstance." Done. https://chromiumcodereview.appspot.com/967383002/diff/280001/content/browser/... content/browser/frame_host/navigator_impl_unittest.cc:1123: EXPECT_NE(unrelated_instance.get(), converted_instance.get()); On 2015/04/02 21:23:25, Charlie Reis wrote: > As in case #5, can we add a line to ensure that ConvertToSiteInstance(rfhm, > descriptor, unrelated_instance.get()) returns unrelated_instance? Done. https://chromiumcodereview.appspot.com/967383002/diff/280001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/967383002/diff/280001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:1280: if (candidate_instance && On 2015/04/02 21:23:25, Charlie Reis wrote: > nit: Add a comment saying that at this point, we need to return an unrelated > SiteInstance for |new_site_url|. That makes this check and the code below it > easy to follow. Done. https://chromiumcodereview.appspot.com/967383002/diff/280001/content/browser/... File content/browser/frame_host/render_frame_host_manager.h (right): https://chromiumcodereview.appspot.com/967383002/diff/280001/content/browser/... content/browser/frame_host/render_frame_host_manager.h:427: enum RenderFrameHostToUse { On 2015/04/02 21:23:25, Charlie Reis wrote: > This is dead code now, right? Sorry for missing that before. Indeed. I forgot about it too. Removed.
The CQ bit was checked by carlosk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/967383002/#ps320001 (title: "General changes and improvements from CR comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/967383002/320001
The CQ bit was unchecked by commit-bot@chromium.org
All required reviewers (with asterisk prefixes) have not yet approved this CL. No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Thanks, still LGTM. Looks like clamy@ is marked as a required reviewer as well.
carlosk@chromium.org changed required reviewers: - clamy@chromium.org
Removed clamy@'s asterisk.
The CQ bit was checked by carlosk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/967383002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by carlosk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/967383002/#ps340001 (title: "Rebase + content-exporting SiteInstanceDescriptor.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/967383002/340001
Message was sent while issue was closed.
Committed patchset #14 (id:340001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/078298a8fcafefc5276f58c657e95b4fc1ff4009 Cr-Commit-Position: refs/heads/master@{#324027} |