Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
600553003:
Enable swapping a frame back in to its parent process (Closed)
|
Created:
6 years, 3 months ago by Nate Chapin Modified:
6 years, 1 month ago CC:
chromium-reviews, mkwst+moarreviews-renderer_chromium.org, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org, site-isolation-reviews_chromium.org, kenrb Base URL:
https://chromium.googlesource.com/chromium/src.git@remoteToLocal Project:
chromium Visibility:
Public. |
DescriptionEnable swapping a frame back in to its parent process
This involves a couple of browser-side plumbing changes, as well as teaching RenderFrameProxy how to swap itself out in favor of a RenderFrameImpl.
BUG=422583
Committed: https://crrev.com/e6adf145eae50a465e44ac625f225aeebafd7837
Cr-Commit-Position: refs/heads/master@{#302193}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 16
Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #Patch Set 14 : #
Total comments: 24
Patch Set 15 : address comments #Patch Set 16 : #
Total comments: 8
Patch Set 17 : #
Total comments: 6
Patch Set 18 : #Messages
Total messages: 35 (6 generated)
japhet@chromium.org changed reviewers: + creis@chromium.org, nasko@chromium.org
This is dependent on https://codereview.chromium.org/574403002/ landing, but I wanted to get this out for review before I disappear on vacation, in case it's a poor direction. https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... File content/browser/frame_host/frame_tree.cc (right): https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:63: if (node->render_manager()->current_frame_host()->GetSiteInstance() == If we're swapping a LocalFrame in place of a RemoteFrame, there is at least one LocalFrame in the target renderer. Don't create a proxy if one isn't needed. https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (left): https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:994: const GURL& current_url = (current_entry) ? current_entry->GetURL() : This code and comment are really old (pre-opensourcing). It's also wrong for site isolation, as current_entry->GetURL() will return the main frame's url, which may not be from the same site as the frame actually being navigated, leading to wrong SiteInstance selection. Moved down a few lines and simplified. https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1157: proxy->SwapOut(new_render_frame_host.get()); This case is where we have a RenderFrameProxyHost, it wasn't created above, and there is no RenderFrameImpl associated with the RenderFrameProxyHost. i.e., this is the case where we are doing a real navigate in what is currently a proxy. Tell it to swap in a real frame. https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... File content/browser/frame_host/render_frame_proxy_host.cc (right): https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... content/browser/frame_host/render_frame_proxy_host.cc:158: Send(new FrameMsg_SwapOut(GetRoutingID(), host->GetRoutingID())); I was lazy and just reused the existing FrameMsg_SwapOut IPC here, where a proxy is being swapped out in favor of a real frame. Is this ok, or should it use a new FrameMsg_SwapIn to make it clear what is being swapped?
Thanks for looking into this! I think Nasko and I had a different take on how this would work (e.g., replacing the RemoteFrame with a LocalFrame at commit time, and having the LocalFrame live off to the side in the meantime). Maybe it's worth having a chat with Nasko before you leave? One other note about a RFHM change below. https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... File content/browser/frame_host/frame_tree.cc (right): https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:63: if (node->render_manager()->current_frame_host()->GetSiteInstance() == On 2014/09/30 21:44:47, Nate Chapin wrote: > If we're swapping a LocalFrame in place of a RemoteFrame, there is at least one > LocalFrame in the target renderer. Don't create a proxy if one isn't needed. Hmm, it feels wrong to be calling this function on nodes that already have a frame in this SiteInstance, but maybe that's how it's written. Nasko, do you think there's a cleaner way to handle this? https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (left): https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:994: const GURL& current_url = (current_entry) ? current_entry->GetURL() : On 2014/09/30 21:44:47, Nate Chapin wrote: > This code and comment are really old (pre-opensourcing). It's also wrong for > site isolation, as current_entry->GetURL() will return the main frame's url, > which may not be from the same site as the frame actually being navigated, > leading to wrong SiteInstance selection. Moved down a few lines and simplified. Ah, this is tricky. We should fix this in its own CL. The comment is definitely stale, and I agree that the code does the wrong thing by looking at the main frame's URL. Good catch. It's tricky to get this right, though, since we're not strict about which URLs are loaded in most SiteInstances. If you have a SiteInstance for a.com and click a link to b.com, it stays in the same SiteInstance. If the user then types another b.com URL into the omnibox (etc), we don't want to force a swap. Instead, perhaps we should grab FrameTreeNode's current_frame_url() instead of the entry's URL? I think that should be zero behavior change for normal Chrome, and fix the subframe case. https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1157: proxy->SwapOut(new_render_frame_host.get()); On 2014/09/30 21:44:47, Nate Chapin wrote: > This case is where we have a RenderFrameProxyHost, it wasn't created above, and > there is no RenderFrameImpl associated with the RenderFrameProxyHost. i.e., this > is the case where we are doing a real navigate in what is currently a proxy. > Tell it to swap in a real frame. I don't think we want to do the swap until the new frame commits. https://codereview.chromium.org/600553003/diff/110001/content/renderer/render... File content/renderer/render_frame_proxy.cc (right): https://codereview.chromium.org/600553003/diff/110001/content/renderer/render... content/renderer/render_frame_proxy.cc:234: void RenderFrameProxy::OnSwapOut(int frame_routing_id) { I don't think we want this, do we? Nasko was pointing out that the swap from RemoteFrame to LocalFrame can happen when the LocalFrame commits, so the browser shouldn't have to send a message to make it happen.
I'm happy to chat with Nasko, but today's my last day in the office until next Tuesday, so it'll probably need to be after I get back. https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (left): https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:994: const GURL& current_url = (current_entry) ? current_entry->GetURL() : On 2014/09/30 23:02:43, Charlie Reis wrote: > On 2014/09/30 21:44:47, Nate Chapin wrote: > > This code and comment are really old (pre-opensourcing). It's also wrong for > > site isolation, as current_entry->GetURL() will return the main frame's url, > > which may not be from the same site as the frame actually being navigated, > > leading to wrong SiteInstance selection. Moved down a few lines and > simplified. > > Ah, this is tricky. We should fix this in its own CL. > > The comment is definitely stale, and I agree that the code does the wrong thing > by looking at the main frame's URL. Good catch. > > It's tricky to get this right, though, since we're not strict about which URLs > are loaded in most SiteInstances. If you have a SiteInstance for a.com and > click a link to b.com, it stays in the same SiteInstance. If the user then > types another b.com URL into the omnibox (etc), we don't want to force a swap. > > Instead, perhaps we should grab FrameTreeNode's current_frame_url() instead of > the entry's URL? I think that should be zero behavior change for normal Chrome, > and fix the subframe case. I can try that. I had also considered making the behavior dependent on kSitePerProcess being specified, but that's messier. https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1157: proxy->SwapOut(new_render_frame_host.get()); On 2014/09/30 23:02:43, Charlie Reis wrote: > On 2014/09/30 21:44:47, Nate Chapin wrote: > > This case is where we have a RenderFrameProxyHost, it wasn't created above, > and > > there is no RenderFrameImpl associated with the RenderFrameProxyHost. i.e., > this > > is the case where we are doing a real navigate in what is currently a proxy. > > Tell it to swap in a real frame. > > I don't think we want to do the swap until the new frame commits. Hrm. I guess that's more accurate.
https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... File content/browser/frame_host/frame_tree.cc (right): https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:63: if (node->render_manager()->current_frame_host()->GetSiteInstance() == On 2014/09/30 23:02:43, Charlie Reis wrote: > On 2014/09/30 21:44:47, Nate Chapin wrote: > > If we're swapping a LocalFrame in place of a RemoteFrame, there is at least > one > > LocalFrame in the target renderer. Don't create a proxy if one isn't needed. > > Hmm, it feels wrong to be calling this function on nodes that already have a > frame in this SiteInstance, but maybe that's how it's written. Nasko, do you > think there's a cleaner way to handle this? CreateRenderFrameProxy already takes care of the case where there is a proxy for this SiteInstance. We shouldn't be creating a proxy for the SiteInstance of the current RFH. What code path triggers that? https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1157: proxy->SwapOut(new_render_frame_host.get()); On 2014/09/30 23:19:44, Nate Chapin wrote: > On 2014/09/30 23:02:43, Charlie Reis wrote: > > On 2014/09/30 21:44:47, Nate Chapin wrote: > > > This case is where we have a RenderFrameProxyHost, it wasn't created above, > > and > > > there is no RenderFrameImpl associated with the RenderFrameProxyHost. i.e., > > this > > > is the case where we are doing a real navigate in what is currently a proxy. > > > Tell it to swap in a real frame. > > > > I don't think we want to do the swap until the new frame commits. > > Hrm. I guess that's more accurate. When we are creating a frame and there exists a proxy for the same SiteInstance, we need to send the routing id of the proxy as part of InitRenderFrame. This will allow the RenderFrameImpl to know which RenderFrameProxy to swap with at time of didCommitProvisionalLoad. https://codereview.chromium.org/600553003/diff/110001/content/renderer/render... File content/renderer/render_frame_proxy.cc (right): https://codereview.chromium.org/600553003/diff/110001/content/renderer/render... content/renderer/render_frame_proxy.cc:234: void RenderFrameProxy::OnSwapOut(int frame_routing_id) { On 2014/09/30 23:02:43, Charlie Reis wrote: > I don't think we want this, do we? Nasko was pointing out that the swap from > RemoteFrame to LocalFrame can happen when the LocalFrame commits, so the browser > shouldn't have to send a message to make it happen. Yes, it will be best if we send the proxy routing id as part of the frame creation message, that way in didCommitProvisionalLoad, the RenderFrame can swap with the RenderFrameProxy.
https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... File content/browser/frame_host/frame_tree.cc (right): https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:63: if (node->render_manager()->current_frame_host()->GetSiteInstance() == On 2014/10/08 16:42:58, nasko wrote: > On 2014/09/30 23:02:43, Charlie Reis wrote: > > On 2014/09/30 21:44:47, Nate Chapin wrote: > > > If we're swapping a LocalFrame in place of a RemoteFrame, there is at least > > one > > > LocalFrame in the target renderer. Don't create a proxy if one isn't needed. > > > > Hmm, it feels wrong to be calling this function on nodes that already have a > > frame in this SiteInstance, but maybe that's how it's written. Nasko, do you > > think there's a cleaner way to handle this? > > CreateRenderFrameProxy already takes care of the case where there is a proxy for > this SiteInstance. > We shouldn't be creating a proxy for the SiteInstance of the current RFH. What > code path triggers that? CreateRenderFrameProxy indeed correctly handles when a proxy exists for this SiteInstance, but it starts by CHECKing that we never ask to create a proxy in the same SiteInstance that we're navigating in. I guess that CHECK is no longer valid?
https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... File content/browser/frame_host/frame_tree.cc (right): https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:63: if (node->render_manager()->current_frame_host()->GetSiteInstance() == On 2014/10/08 20:30:05, Nate Chapin wrote: > On 2014/10/08 16:42:58, nasko wrote: > > On 2014/09/30 23:02:43, Charlie Reis wrote: > > > On 2014/09/30 21:44:47, Nate Chapin wrote: > > > > If we're swapping a LocalFrame in place of a RemoteFrame, there is at > least > > > one > > > > LocalFrame in the target renderer. Don't create a proxy if one isn't > needed. > > > > > > Hmm, it feels wrong to be calling this function on nodes that already have a > > > frame in this SiteInstance, but maybe that's how it's written. Nasko, do > you > > > think there's a cleaner way to handle this? > > > > CreateRenderFrameProxy already takes care of the case where there is a proxy > for > > this SiteInstance. > > We shouldn't be creating a proxy for the SiteInstance of the current RFH. What > > code path triggers that? > > CreateRenderFrameProxy indeed correctly handles when a proxy exists for this > SiteInstance, but it starts by CHECKing that we never ask to create a proxy in > the same SiteInstance that we're navigating in. I guess that CHECK is no longer > valid? I keep hitting that CHECK as well. I think Nasko has a patch in progress to fix it, blocked on some UaF issues.
https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... File content/browser/frame_host/frame_tree.cc (right): https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:63: if (node->render_manager()->current_frame_host()->GetSiteInstance() == On 2014/10/08 20:49:50, Charlie Reis wrote: > On 2014/10/08 20:30:05, Nate Chapin wrote: > > On 2014/10/08 16:42:58, nasko wrote: > > > On 2014/09/30 23:02:43, Charlie Reis wrote: > > > > On 2014/09/30 21:44:47, Nate Chapin wrote: > > > > > If we're swapping a LocalFrame in place of a RemoteFrame, there is at > > least > > > > one > > > > > LocalFrame in the target renderer. Don't create a proxy if one isn't > > needed. > > > > > > > > Hmm, it feels wrong to be calling this function on nodes that already have > a > > > > frame in this SiteInstance, but maybe that's how it's written. Nasko, do > > you > > > > think there's a cleaner way to handle this? > > > > > > CreateRenderFrameProxy already takes care of the case where there is a proxy > > for > > > this SiteInstance. > > > We shouldn't be creating a proxy for the SiteInstance of the current RFH. > What > > > code path triggers that? > > > > CreateRenderFrameProxy indeed correctly handles when a proxy exists for this > > SiteInstance, but it starts by CHECKing that we never ask to create a proxy in > > the same SiteInstance that we're navigating in. I guess that CHECK is no > longer > > valid? > > I keep hitting that CHECK as well. I think Nasko has a patch in progress to fix > it, blocked on some UaF issues. I think the CHECK should be there and is valid. Charlie is correct that I have a CL in place for that. See if patching it in will help: https://codereview.chromium.org/600483003/
On 2014/10/08 20:56:44, nasko wrote: > https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... > File content/browser/frame_host/frame_tree.cc (right): > > https://codereview.chromium.org/600553003/diff/110001/content/browser/frame_h... > content/browser/frame_host/frame_tree.cc:63: if > (node->render_manager()->current_frame_host()->GetSiteInstance() == > On 2014/10/08 20:49:50, Charlie Reis wrote: > > On 2014/10/08 20:30:05, Nate Chapin wrote: > > > On 2014/10/08 16:42:58, nasko wrote: > > > > On 2014/09/30 23:02:43, Charlie Reis wrote: > > > > > On 2014/09/30 21:44:47, Nate Chapin wrote: > > > > > > If we're swapping a LocalFrame in place of a RemoteFrame, there is at > > > least > > > > > one > > > > > > LocalFrame in the target renderer. Don't create a proxy if one isn't > > > needed. > > > > > > > > > > Hmm, it feels wrong to be calling this function on nodes that already > have > > a > > > > > frame in this SiteInstance, but maybe that's how it's written. Nasko, > do > > > you > > > > > think there's a cleaner way to handle this? > > > > > > > > CreateRenderFrameProxy already takes care of the case where there is a > proxy > > > for > > > > this SiteInstance. > > > > We shouldn't be creating a proxy for the SiteInstance of the current RFH. > > What > > > > code path triggers that? > > > > > > CreateRenderFrameProxy indeed correctly handles when a proxy exists for this > > > SiteInstance, but it starts by CHECKing that we never ask to create a proxy > in > > > the same SiteInstance that we're navigating in. I guess that CHECK is no > > longer > > > valid? > > > > I keep hitting that CHECK as well. I think Nasko has a patch in progress to > fix > > it, blocked on some UaF issues. > > I think the CHECK should be there and is valid. Charlie is correct that I have a > CL in place for that. See if patching it in will help: > https://codereview.chromium.org/600483003/ Ok, I think this is ready for another look.
Few comments, but overall I like how this looks! It will be useful to rebase this (ideally in a separate patchset), since it doesn't seem to have picked up a bunch of fixes that have been committed recently. https://codereview.chromium.org/600553003/diff/390001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/600553003/diff/390001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:581: GetRenderFrameProxyHost(GetSiteInstance()); This type of logic should be in RenderFrameHostManager. That would mean that we need to plumb it all the way through WebContentsImpl. I'm a bit torn on what the right approach is, so let's hear what Charlie has to say. Also, we have an invariant that we want to always uphold - there should be no proxy for the same SiteInstance as the active RFH. What we are doing here is exactly against it : ), so let's add a CHECK to ensure that this is the pending RFH and not the current one. https://codereview.chromium.org/600553003/diff/390001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/600553003/diff/390001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:301: EXPECT_NE(proxy_to_parent, child->render_manager()->GetProxyToParent()); Hmm, why is that? The proxy object for the parent shouldn't change on the second cross-process navigation, since the parent stays the same. https://codereview.chromium.org/600553003/diff/390001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:367: // TODO(japhet): This currently causes an assertion in the renderer process. The comment is no longer needed if this is indeed fixed : ). https://codereview.chromium.org/600553003/diff/390001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/600553003/diff/390001/content/renderer/render... content/renderer/render_frame_impl.cc:510: RenderFrameProxy* proxy = nit: I'd rename this to proxy_parent or something to differentiate it from the case in the else clause. https://codereview.chromium.org/600553003/diff/390001/content/renderer/render... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/600553003/diff/390001/content/renderer/render... content/renderer/render_frame_impl.h:110: // point it replaces the given proxy. nit: s/the given proxy/the proxy identified by |proxy_routing_id|/
I'm looking forward to this! A few thoughts below. https://codereview.chromium.org/600553003/diff/390001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/600553003/diff/390001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:581: GetRenderFrameProxyHost(GetSiteInstance()); On 2014/10/27 22:52:54, nasko wrote: > This type of logic should be in RenderFrameHostManager. That would mean that we > need to plumb it all the way through WebContentsImpl. I'm a bit torn on what the > right approach is, so let's hear what Charlie has to say. +1. This should get passed in from RFHM via WebContentsImpl. > Also, we have an invariant that we want to always uphold - there should be no > proxy for the same SiteInstance as the active RFH. What we are doing here is > exactly against it : ), so let's add a CHECK to ensure that this is the pending > RFH and not the current one. Yeah, I was confused by this. We only want it to be a real value when we're swapping from a proxy to an active frame. It's awkward to CHECK that we're the pending RFH, though, since that requires asking the RFHM (not ideal, as above). Instead, let's just document it with a comment. https://codereview.chromium.org/600553003/diff/390001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/600553003/diff/390001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:935: const GURL& current_url = (current_entry) ? frame_tree_node_->current_url() : I'm a little nervous about this because I don't trust frame_tree_node_->current_url() as much as current_entry->GetURL(). I think they should be the same (for main frames), though, and I can see that this is needed for subframes until we get FrameNavigationEntries. It's independent of the rest of this CL, so let's split it out into a different CL with a CHECK_EQ of the two URLs in the main frame case, just to give confidence that there's no behavior change there. (Otherwise this is a dangerous place to introduce regressions.) https://codereview.chromium.org/600553003/diff/390001/content/common/frame_me... File content/common/frame_messages.h (right): https://codereview.chromium.org/600553003/diff/390001/content/common/frame_me... content/common/frame_messages.h:363: // If a valid proxy_routing_id is provided, the new frame will be configured to nit: |proxy_routing_id| https://codereview.chromium.org/600553003/diff/390001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/600553003/diff/390001/content/renderer/render... content/renderer/render_frame_impl.cc:520: } else { Why do we ignore parent_routing_id in this case? Doesn't it need to be linked as above? https://codereview.chromium.org/600553003/diff/390001/content/renderer/render... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/600553003/diff/390001/content/renderer/render... content/renderer/render_frame_impl.h:678: // id of the proxy to replace at commit-time. Unfortunate that we have to store this. I suppose it will go away with PlzNavigate, though, since we won't need the RenderFrameImpl until commit time. Can you clarify that this is cleared at commit time?
https://codereview.chromium.org/600553003/diff/390001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/600553003/diff/390001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:581: GetRenderFrameProxyHost(GetSiteInstance()); On 2014/10/28 16:38:29, Charlie Reis wrote: > On 2014/10/27 22:52:54, nasko wrote: > > This type of logic should be in RenderFrameHostManager. That would mean that > we > > need to plumb it all the way through WebContentsImpl. I'm a bit torn on what > the > > right approach is, so let's hear what Charlie has to say. > > +1. This should get passed in from RFHM via WebContentsImpl. > > > Also, we have an invariant that we want to always uphold - there should be no > > proxy for the same SiteInstance as the active RFH. What we are doing here is > > exactly against it : ), so let's add a CHECK to ensure that this is the > pending > > RFH and not the current one. > > Yeah, I was confused by this. We only want it to be a real value when we're > swapping from a proxy to an active frame. It's awkward to CHECK that we're the > pending RFH, though, since that requires asking the RFHM (not ideal, as above). > Instead, let's just document it with a comment. Done. https://codereview.chromium.org/600553003/diff/390001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/600553003/diff/390001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:935: const GURL& current_url = (current_entry) ? frame_tree_node_->current_url() : On 2014/10/28 16:38:29, Charlie Reis wrote: > I'm a little nervous about this because I don't trust > frame_tree_node_->current_url() as much as current_entry->GetURL(). I think > they should be the same (for main frames), though, and I can see that this is > needed for subframes until we get FrameNavigationEntries. > > It's independent of the rest of this CL, so let's split it out into a different > CL with a CHECK_EQ of the two URLs in the main frame case, just to give > confidence that there's no behavior change there. (Otherwise this is a > dangerous place to introduce regressions.) Ok, will split out and land that first. https://codereview.chromium.org/600553003/diff/390001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/600553003/diff/390001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:301: EXPECT_NE(proxy_to_parent, child->render_manager()->GetProxyToParent()); On 2014/10/27 22:52:54, nasko wrote: > Hmm, why is that? The proxy object for the parent shouldn't change on the second > cross-process navigation, since the parent stays the same. I would have thought that the child changing processes would cause it to have a differnet proxy to its parent. Is that not the case? https://codereview.chromium.org/600553003/diff/390001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:367: // TODO(japhet): This currently causes an assertion in the renderer process. On 2014/10/27 22:52:54, nasko wrote: > The comment is no longer needed if this is indeed fixed : ). Will update to describe what this block actually does. https://codereview.chromium.org/600553003/diff/390001/content/common/frame_me... File content/common/frame_messages.h (right): https://codereview.chromium.org/600553003/diff/390001/content/common/frame_me... content/common/frame_messages.h:363: // If a valid proxy_routing_id is provided, the new frame will be configured to On 2014/10/28 16:38:29, Charlie Reis wrote: > nit: |proxy_routing_id| Done. https://codereview.chromium.org/600553003/diff/390001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/600553003/diff/390001/content/renderer/render... content/renderer/render_frame_impl.cc:510: RenderFrameProxy* proxy = On 2014/10/27 22:52:55, nasko wrote: > nit: I'd rename this to proxy_parent or something to differentiate it from the > case in the else clause. parent_proxy to better match the existing parent_web_frame. Done. https://codereview.chromium.org/600553003/diff/390001/content/renderer/render... content/renderer/render_frame_impl.cc:520: } else { On 2014/10/28 16:38:29, Charlie Reis wrote: > Why do we ignore parent_routing_id in this case? Doesn't it need to be linked > as above? The existing RemoteFrame that we're replacing already knows about its parent, and intializeToReplaceRemoteFrame() below takes care of hooking up the new LocalFrame. https://codereview.chromium.org/600553003/diff/390001/content/renderer/render... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/600553003/diff/390001/content/renderer/render... content/renderer/render_frame_impl.h:110: // point it replaces the given proxy. On 2014/10/27 22:52:55, nasko wrote: > nit: s/the given proxy/the proxy identified by |proxy_routing_id|/ Done. https://codereview.chromium.org/600553003/diff/390001/content/renderer/render... content/renderer/render_frame_impl.h:678: // id of the proxy to replace at commit-time. On 2014/10/28 16:38:29, Charlie Reis wrote: > Unfortunate that we have to store this. I suppose it will go away with > PlzNavigate, though, since we won't need the RenderFrameImpl until commit time. > > Can you clarify that this is cleared at commit time? Done.
Thanks! LGTM with nits below. https://codereview.chromium.org/600553003/diff/390001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/600553003/diff/390001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:301: EXPECT_NE(proxy_to_parent, child->render_manager()->GetProxyToParent()); On 2014/10/28 19:59:20, Nate Chapin wrote: > On 2014/10/27 22:52:54, nasko wrote: > > Hmm, why is that? The proxy object for the parent shouldn't change on the > second > > cross-process navigation, since the parent stays the same. > > I would have thought that the child changing processes would cause it to have a > differnet proxy to its parent. Is that not the case? I'll defer to Nasko (or Ken) here, but let's figure out what the right expectation is before landing. https://codereview.chromium.org/600553003/diff/430001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/600553003/diff/430001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1198: // SiteInstance. If there is, the new RenderFrame needs to be able to talk to nit: talk to -> find https://codereview.chromium.org/600553003/diff/430001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1199: // the proxy it is replacing, so that it can fully intiialize itself. nit: initialize
https://codereview.chromium.org/600553003/diff/390001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/600553003/diff/390001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:301: EXPECT_NE(proxy_to_parent, child->render_manager()->GetProxyToParent()); On 2014/10/28 20:10:10, Charlie Reis wrote: > On 2014/10/28 19:59:20, Nate Chapin wrote: > > On 2014/10/27 22:52:54, nasko wrote: > > > Hmm, why is that? The proxy object for the parent shouldn't change on the > > second > > > cross-process navigation, since the parent stays the same. > > > > I would have thought that the child changing processes would cause it to have > a > > differnet proxy to its parent. Is that not the case? > > I'll defer to Nasko (or Ken) here, but let's figure out what the right > expectation is before landing. When a child frame navigates, it changes process, but the proxy for the parent's process shouldn't change. The reason is that as far as the parent process is concerned, if a child frame navigates from B to C (where parent is A), nothing really changes. I'm now curious, is the proxy changing with your CL? The only case where I can see it is when parent in A and child in B has the child navigating back to A (the case you want to enable), but this test doesn't do that. https://codereview.chromium.org/600553003/diff/430001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/600553003/diff/430001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1200: // NOTE: This is the only time that a RenderFrameProxy can be in the same nit: Since this is code in the browser and SiteInstance is a browser side concept, let's use the *Host version of the objects. RenderFrameProxyHost/RenderFrameHost. https://codereview.chromium.org/600553003/diff/430001/content/renderer/render... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/600553003/diff/430001/content/renderer/render... content/renderer/render_frame_impl.h:107: // is MSG_ROUTING_NONE. If proxy_routing_id is MSG_ROUTING_NONE, it creates nit: || around proxy_routing_id.
https://codereview.chromium.org/600553003/diff/390001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/600553003/diff/390001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:301: EXPECT_NE(proxy_to_parent, child->render_manager()->GetProxyToParent()); On 2014/10/28 21:58:14, nasko wrote: > On 2014/10/28 20:10:10, Charlie Reis wrote: > > On 2014/10/28 19:59:20, Nate Chapin wrote: > > > On 2014/10/27 22:52:54, nasko wrote: > > > > Hmm, why is that? The proxy object for the parent shouldn't change on the > > > second > > > > cross-process navigation, since the parent stays the same. > > > > > > I would have thought that the child changing processes would cause it to > have > > a > > > differnet proxy to its parent. Is that not the case? > > > > I'll defer to Nasko (or Ken) here, but let's figure out what the right > > expectation is before landing. > > When a child frame navigates, it changes process, but the proxy for the parent's > process shouldn't change. The reason is that as far as the parent process is > concerned, if a child frame navigates from B to C (where parent is A), nothing > really changes. > > I'm now curious, is the proxy changing with your CL? The only case where I can > see it is when parent in A and child in B has the child navigating back to A > (the case you want to enable), but this test doesn't do that. Ah, I think I understand this now. The problem has to do with how the test server implements urls like "/cross-site/bar.com/title3.html". Specifically, it navigates to that url, which triggers a redirect to bar.com/title3.html. With this patch in place, that means we swap in to the parent process (which presumably clobbers the proxy_to_parent), then swap back out when the redirect occurs. Thoughts on that behavior?
On 2014/10/29 20:39:42, Nate Chapin wrote: > https://codereview.chromium.org/600553003/diff/390001/content/browser/site_pe... > File content/browser/site_per_process_browsertest.cc (right): > > https://codereview.chromium.org/600553003/diff/390001/content/browser/site_pe... > content/browser/site_per_process_browsertest.cc:301: EXPECT_NE(proxy_to_parent, > child->render_manager()->GetProxyToParent()); > On 2014/10/28 21:58:14, nasko wrote: > > On 2014/10/28 20:10:10, Charlie Reis wrote: > > > On 2014/10/28 19:59:20, Nate Chapin wrote: > > > > On 2014/10/27 22:52:54, nasko wrote: > > > > > Hmm, why is that? The proxy object for the parent shouldn't change on > the > > > > second > > > > > cross-process navigation, since the parent stays the same. > > > > > > > > I would have thought that the child changing processes would cause it to > > have > > > a > > > > differnet proxy to its parent. Is that not the case? > > > > > > I'll defer to Nasko (or Ken) here, but let's figure out what the right > > > expectation is before landing. > > > > When a child frame navigates, it changes process, but the proxy for the > parent's > > process shouldn't change. The reason is that as far as the parent process is > > concerned, if a child frame navigates from B to C (where parent is A), nothing > > really changes. > > > > I'm now curious, is the proxy changing with your CL? The only case where I can > > see it is when parent in A and child in B has the child navigating back to A > > (the case you want to enable), but this test doesn't do that. > > Ah, I think I understand this now. The problem has to do with how the test > server implements urls like "/cross-site/bar.com/title3.html". Specifically, it > navigates to that url, which triggers a redirect to bar.com/title3.html. With > this patch in place, that means we swap in to the parent process (which > presumably clobbers the proxy_to_parent) The URL is relative, right? So the host shouldn't be changing, therefore there shouldn't be a cross-site navigation. > , then swap back out when the redirect occurs. > > Thoughts on that behavior? There should only be one process swap occurring, which happens after the redirect when we are ready to commit.
https://codereview.chromium.org/600553003/diff/390001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/600553003/diff/390001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:301: EXPECT_NE(proxy_to_parent, child->render_manager()->GetProxyToParent()); On 2014/10/29 21:19:21, nasko wrote: > On 2014/10/29 20:39:42, Nate Chapin wrote: > > > https://codereview.chromium.org/600553003/diff/390001/content/browser/site_pe... > > File content/browser/site_per_process_browsertest.cc (right): > > > > > https://codereview.chromium.org/600553003/diff/390001/content/browser/site_pe... > > content/browser/site_per_process_browsertest.cc:301: > EXPECT_NE(proxy_to_parent, > > child->render_manager()->GetProxyToParent()); > > On 2014/10/28 21:58:14, nasko wrote: > > > On 2014/10/28 20:10:10, Charlie Reis wrote: > > > > On 2014/10/28 19:59:20, Nate Chapin wrote: > > > > > On 2014/10/27 22:52:54, nasko wrote: > > > > > > Hmm, why is that? The proxy object for the parent shouldn't change on > > the > > > > > second > > > > > > cross-process navigation, since the parent stays the same. > > > > > > > > > > I would have thought that the child changing processes would cause it to > > > have > > > > a > > > > > differnet proxy to its parent. Is that not the case? > > > > > > > > I'll defer to Nasko (or Ken) here, but let's figure out what the right > > > > expectation is before landing. > > > > > > When a child frame navigates, it changes process, but the proxy for the > > parent's > > > process shouldn't change. The reason is that as far as the parent process is > > > concerned, if a child frame navigates from B to C (where parent is A), > nothing > > > really changes. > > > > > > I'm now curious, is the proxy changing with your CL? The only case where I > can > > > see it is when parent in A and child in B has the child navigating back to A > > > (the case you want to enable), but this test doesn't do that. > > > > Ah, I think I understand this now. The problem has to do with how the test > > server implements urls like "/cross-site/bar.com/title3.html". Specifically, > it > > navigates to that url, which triggers a redirect to bar.com/title3.html. With > > this patch in place, that means we swap in to the parent process (which > > presumably clobbers the proxy_to_parent) > > The URL is relative, right? So the host shouldn't be changing, therefore there > shouldn't be a cross-site navigation. > > > , then swap back out when the redirect occurs. > > > > Thoughts on that behavior? > > There should only be one process swap occurring, which happens after the > redirect when we are ready to commit. Nasko: Actually, I think Nate found an interesting problem here. Suppose the main frame is localhost and the subframe is foo.com. If the subframe then goes to bar.com, the URL looks like localhost/cross-site/bar.com. That means we try to swap it into it's parent process (yuck) before the redirect sends it to a new process. All kinds of things break at that point.
On 2014/10/29 21:28:32, Charlie Reis wrote: > https://codereview.chromium.org/600553003/diff/390001/content/browser/site_pe... > File content/browser/site_per_process_browsertest.cc (right): > > https://codereview.chromium.org/600553003/diff/390001/content/browser/site_pe... > content/browser/site_per_process_browsertest.cc:301: EXPECT_NE(proxy_to_parent, > child->render_manager()->GetProxyToParent()); > On 2014/10/29 21:19:21, nasko wrote: > > On 2014/10/29 20:39:42, Nate Chapin wrote: > > > > > > https://codereview.chromium.org/600553003/diff/390001/content/browser/site_pe... > > > File content/browser/site_per_process_browsertest.cc (right): > > > > > > > > > https://codereview.chromium.org/600553003/diff/390001/content/browser/site_pe... > > > content/browser/site_per_process_browsertest.cc:301: > > EXPECT_NE(proxy_to_parent, > > > child->render_manager()->GetProxyToParent()); > > > On 2014/10/28 21:58:14, nasko wrote: > > > > On 2014/10/28 20:10:10, Charlie Reis wrote: > > > > > On 2014/10/28 19:59:20, Nate Chapin wrote: > > > > > > On 2014/10/27 22:52:54, nasko wrote: > > > > > > > Hmm, why is that? The proxy object for the parent shouldn't change > on > > > the > > > > > > second > > > > > > > cross-process navigation, since the parent stays the same. > > > > > > > > > > > > I would have thought that the child changing processes would cause it > to > > > > have > > > > > a > > > > > > differnet proxy to its parent. Is that not the case? > > > > > > > > > > I'll defer to Nasko (or Ken) here, but let's figure out what the right > > > > > expectation is before landing. > > > > > > > > When a child frame navigates, it changes process, but the proxy for the > > > parent's > > > > process shouldn't change. The reason is that as far as the parent process > is > > > > concerned, if a child frame navigates from B to C (where parent is A), > > nothing > > > > really changes. > > > > > > > > I'm now curious, is the proxy changing with your CL? The only case where I > > can > > > > see it is when parent in A and child in B has the child navigating back to > A > > > > (the case you want to enable), but this test doesn't do that. > > > > > > Ah, I think I understand this now. The problem has to do with how the test > > > server implements urls like "/cross-site/bar.com/title3.html". Specifically, > > it > > > navigates to that url, which triggers a redirect to bar.com/title3.html. > With > > > this patch in place, that means we swap in to the parent process (which > > > presumably clobbers the proxy_to_parent) > > > > The URL is relative, right? So the host shouldn't be changing, therefore there > > shouldn't be a cross-site navigation. > > > > > , then swap back out when the redirect occurs. > > > > > > Thoughts on that behavior? > > > > There should only be one process swap occurring, which happens after the > > redirect when we are ready to commit. > > Nasko: Actually, I think Nate found an interesting problem here. Suppose the > main frame is localhost and the subframe is http://foo.com. If the subframe then goes > to http://bar.com, the URL looks like http://localhost/cross-site/bar.com. That means we try > to swap it into it's parent process (yuck) before the redirect sends it to a new > process. All kinds of things break at that point. Oh, you are right, it is indeed relative, but to the embedded_test_server : (.
https://codereview.chromium.org/600553003/diff/390001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/600553003/diff/390001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:301: EXPECT_NE(proxy_to_parent, child->render_manager()->GetProxyToParent()); On 2014/10/29 21:28:32, Charlie Reis wrote: > On 2014/10/29 21:19:21, nasko wrote: > > On 2014/10/29 20:39:42, Nate Chapin wrote: > > > > > > https://codereview.chromium.org/600553003/diff/390001/content/browser/site_pe... > > > File content/browser/site_per_process_browsertest.cc (right): > > > > > > > > > https://codereview.chromium.org/600553003/diff/390001/content/browser/site_pe... > > > content/browser/site_per_process_browsertest.cc:301: > > EXPECT_NE(proxy_to_parent, > > > child->render_manager()->GetProxyToParent()); > > > On 2014/10/28 21:58:14, nasko wrote: > > > > On 2014/10/28 20:10:10, Charlie Reis wrote: > > > > > On 2014/10/28 19:59:20, Nate Chapin wrote: > > > > > > On 2014/10/27 22:52:54, nasko wrote: > > > > > > > Hmm, why is that? The proxy object for the parent shouldn't change > on > > > the > > > > > > second > > > > > > > cross-process navigation, since the parent stays the same. > > > > > > > > > > > > I would have thought that the child changing processes would cause it > to > > > > have > > > > > a > > > > > > differnet proxy to its parent. Is that not the case? > > > > > > > > > > I'll defer to Nasko (or Ken) here, but let's figure out what the right > > > > > expectation is before landing. > > > > > > > > When a child frame navigates, it changes process, but the proxy for the > > > parent's > > > > process shouldn't change. The reason is that as far as the parent process > is > > > > concerned, if a child frame navigates from B to C (where parent is A), > > nothing > > > > really changes. > > > > > > > > I'm now curious, is the proxy changing with your CL? The only case where I > > can > > > > see it is when parent in A and child in B has the child navigating back to > A > > > > (the case you want to enable), but this test doesn't do that. > > > > > > Ah, I think I understand this now. The problem has to do with how the test > > > server implements urls like "/cross-site/bar.com/title3.html". Specifically, > > it > > > navigates to that url, which triggers a redirect to bar.com/title3.html. > With > > > this patch in place, that means we swap in to the parent process (which > > > presumably clobbers the proxy_to_parent) > > > > The URL is relative, right? So the host shouldn't be changing, therefore there > > shouldn't be a cross-site navigation. > > > > > , then swap back out when the redirect occurs. > > > > > > Thoughts on that behavior? > > > > There should only be one process swap occurring, which happens after the > > redirect when we are ready to commit. > > Nasko: Actually, I think Nate found an interesting problem here. Suppose the > main frame is localhost and the subframe is http://foo.com. If the subframe then goes > to http://bar.com, the URL looks like http://localhost/cross-site/bar.com. That means we try > to swap it into it's parent process (yuck) before the redirect sends it to a new > process. All kinds of things break at that point. Nasko tells me this should be resolved once https://codereview.chromium.org/688363002/ lands, so I'll just wait for it before committing this CL. https://codereview.chromium.org/600553003/diff/430001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/600553003/diff/430001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1198: // SiteInstance. If there is, the new RenderFrame needs to be able to talk to On 2014/10/28 20:10:10, Charlie Reis wrote: > nit: talk to -> find Done. https://codereview.chromium.org/600553003/diff/430001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1199: // the proxy it is replacing, so that it can fully intiialize itself. On 2014/10/28 20:10:10, Charlie Reis wrote: > nit: initialize Done. https://codereview.chromium.org/600553003/diff/430001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1200: // NOTE: This is the only time that a RenderFrameProxy can be in the same On 2014/10/28 21:58:15, nasko wrote: > nit: Since this is code in the browser and SiteInstance is a browser side > concept, let's use the *Host version of the objects. > RenderFrameProxyHost/RenderFrameHost. Done. https://codereview.chromium.org/600553003/diff/430001/content/renderer/render... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/600553003/diff/430001/content/renderer/render... content/renderer/render_frame_impl.h:107: // is MSG_ROUTING_NONE. If proxy_routing_id is MSG_ROUTING_NONE, it creates On 2014/10/28 21:58:15, nasko wrote: > nit: || around proxy_routing_id. Done.
LGTM with nits. https://codereview.chromium.org/600553003/diff/450001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/600553003/diff/450001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:964: const GURL& RenderFrameHostManager::GetCurrentURLForSiteInstance( Returning a const ref here seems risky if anyone tries to later return a GURL allocated in this method. That said, I think we can get rid of this function once we have FrameNavigationEntry, so I'm ok with it for now. https://codereview.chromium.org/600553003/diff/450001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:967: // don't consider using current_entry's url for SiteInstance selection, since nit: Change the second clause to "since current_entry's url is for the main frame and may be from a different site than this frame." Also add: TODO(creis): Remove this when we can check the FrameNavigationEntry's URL. See http://crbug.com/369654. https://codereview.chromium.org/600553003/diff/450001/content/renderer/render... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/600553003/diff/450001/content/renderer/render... content/renderer/render_frame_impl.h:681: int proxy_routing_id_; Let's put a "TODO(creis): Remove this after switching to PlzNavigate." here as well.
The CQ bit was checked by japhet@chromium.org
https://codereview.chromium.org/600553003/diff/450001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/600553003/diff/450001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:964: const GURL& RenderFrameHostManager::GetCurrentURLForSiteInstance( On 2014/10/30 19:36:06, Charlie Reis wrote: > Returning a const ref here seems risky if anyone tries to later return a GURL > allocated in this method. That said, I think we can get rid of this function > once we have FrameNavigationEntry, so I'm ok with it for now. Yeah, I was just matching the types used in the code previously, I agree that it's not ideal long term, but it shouldn't be anyway. https://codereview.chromium.org/600553003/diff/450001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:967: // don't consider using current_entry's url for SiteInstance selection, since On 2014/10/30 19:36:06, Charlie Reis wrote: > nit: Change the second clause to "since current_entry's url is for the main > frame and may be from a different site than this frame." > > Also add: > TODO(creis): Remove this when we can check the FrameNavigationEntry's URL. See > http://crbug.com/369654. Done. https://codereview.chromium.org/600553003/diff/450001/content/renderer/render... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/600553003/diff/450001/content/renderer/render... content/renderer/render_frame_impl.h:681: int proxy_routing_id_; On 2014/10/30 19:36:06, Charlie Reis wrote: > Let's put a "TODO(creis): Remove this after switching to PlzNavigate." here as > well. Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/600553003/470001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
LGTM (needed for frame_messages.h)
The CQ bit was checked by japhet@chromium.org
On 2014/10/30 23:23:22, nasko wrote: > LGTM (needed for frame_messages.h) I'd suggest adding a bug number though, so this CL can be properly linked.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/600553003/470001
The CQ bit was unchecked by japhet@chromium.org
On 2014/10/30 23:24:27, nasko wrote: > On 2014/10/30 23:23:22, nasko wrote: > > LGTM (needed for frame_messages.h) > > I'd suggest adding a bug number though, so this CL can be properly linked. Done, don't know how I missed that...
The CQ bit was checked by japhet@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/600553003/470001
Message was sent while issue was closed.
Committed patchset #18 (id:470001)
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/e6adf145eae50a465e44ac625f225aeebafd7837 Cr-Commit-Position: refs/heads/master@{#302193} |
