Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
217163007:
Introduce RenderFrameProxyHost object and use it in RFHM. (Closed)
|
Created:
6 years, 8 months ago by nasko Modified:
6 years, 8 months ago Reviewers:
Charlie Reis CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org, site-isolation-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionIntroduce RenderFrameProxyHost object and use it in RFHM.
This is the first CL in a series to create RenderFrameProxy(Host) infrastructure. Before the Blink codebase is ready to transform local and remote frames, the proxy objects will keep internally the existing RF/RFH in swapped out state. This CL creates the browser side proxy object and wraps the swapped out RFH.
BUG=357747
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=263352
Patch Set 1 #Patch Set 2 : Some refactoring/renaming. #Patch Set 3 : Proxies shouldn't be scoped_ptrs. #Patch Set 4 : A gross hack to fix CancelPending. #
Total comments: 20
Patch Set 5 : Address Charlie's comments. #
Total comments: 49
Patch Set 6 : Fixes for Charlie's comments. #Patch Set 7 : Add a test for RFH lifetime during CancelPending. #
Total comments: 12
Patch Set 8 : Another round of fixes. #
Messages
Total messages: 11 (0 generated)
Hey Charlie, Can you take a look at this? The goal of this CL is to add RenderFrameProxyHost object type and wrap swapped out RFHs. The swapped_out_hosts_ list is now using the proxy type and I've converted the code to use scoped_ptrs. Thanks! Nasko
Very excited to see this first step-- I'm looking forward to a time when we don't have swapped out RFHs anymore. I didn't have time to do a deep review of render_frame_host_manager.cc, but here's a start. Perhaps we can discuss it more in person tomorrow. https://codereview.chromium.org/217163007/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/217163007/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_impl.h:264: bool created_for_pending_; What was wrong with the active_view_count hack? It'd be really nice to avoid adding this if possible. https://codereview.chromium.org/217163007/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/217163007/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:949: nit: extra line https://codereview.chromium.org/217163007/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1112: static_cast<SiteInstanceImpl*>(old_render_frame_host->GetSiteInstance()); nit: Wrong indent. https://codereview.chromium.org/217163007/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/217163007/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:272: // SiteInstance, if any. Maybe add a TODO comment saying that GetSwappedOutRVH is deprecated in favor of GetRenderFrameProxyHost? https://codereview.chromium.org/217163007/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:335: // Used with FrameTree::ForEach to erase inactive RenderFrameHosts from a I imagine there's a bunch of comments we'll want to update to talk about proxies instead of inactive/swapped out RenderFrameHosts. https://codereview.chromium.org/217163007/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:451: // A map of site instance ID to swapped out RenderFrameHosts. This may to RenderFrameProxyHosts? https://codereview.chromium.org/217163007/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:452: // include pending_render_frame_host_ for navigations to existing entries. Is this part still accurate? https://codereview.chromium.org/217163007/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_proxy_host.h (right): https://codereview.chromium.org/217163007/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:17: class RenderFrameProxyHost { This class needs a big helpful comment. Some things to cover: - Each renderer process with a reference to this frame (other than the one rendering it) has a RenderFrameProxy placeholder for it. - This is the browser-side part of that placeholder. - The placeholder allows us to keep existing window references valid and route cross-process JS calls like postMessage. - Lifetime (for now): Created when RFH swapped out, deleted when swapped back in or unreferenced - Lifetime (planned): RFH and RFPH live at mutually exclusive times (apart from waiting for RFH to unload and exit?) https://codereview.chromium.org/217163007/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:24: DCHECK(render_frame_host_.get()); This is kind of redundant, since we'll crash on the next line anyway if it's null. https://codereview.chromium.org/217163007/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:31: if (render_frame_host_) When is this not null? Does the RFPH ever outlive its reference to the RFH?
A round of fixes. https://codereview.chromium.org/217163007/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/217163007/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_impl.h:264: bool created_for_pending_; On 2014/03/31 23:22:35, Charlie Reis wrote: > What was wrong with the active_view_count hack? It'd be really nice to avoid > adding this if possible. Done. https://codereview.chromium.org/217163007/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/217163007/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:949: On 2014/03/31 23:22:35, Charlie Reis wrote: > nit: extra line Done. https://codereview.chromium.org/217163007/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1112: static_cast<SiteInstanceImpl*>(old_render_frame_host->GetSiteInstance()); On 2014/03/31 23:22:35, Charlie Reis wrote: > nit: Wrong indent. Done. https://codereview.chromium.org/217163007/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/217163007/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:272: // SiteInstance, if any. On 2014/03/31 23:22:35, Charlie Reis wrote: > Maybe add a TODO comment saying that GetSwappedOutRVH is deprecated in favor of > GetRenderFrameProxyHost? Done. https://codereview.chromium.org/217163007/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:335: // Used with FrameTree::ForEach to erase inactive RenderFrameHosts from a On 2014/03/31 23:22:35, Charlie Reis wrote: > I imagine there's a bunch of comments we'll want to update to talk about proxies > instead of inactive/swapped out RenderFrameHosts. Done. https://codereview.chromium.org/217163007/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:451: // A map of site instance ID to swapped out RenderFrameHosts. This may On 2014/03/31 23:22:35, Charlie Reis wrote: > to RenderFrameProxyHosts? Done. https://codereview.chromium.org/217163007/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:452: // include pending_render_frame_host_ for navigations to existing entries. On 2014/03/31 23:22:35, Charlie Reis wrote: > Is this part still accurate? Done. https://codereview.chromium.org/217163007/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_proxy_host.h (right): https://codereview.chromium.org/217163007/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:17: class RenderFrameProxyHost { On 2014/03/31 23:22:35, Charlie Reis wrote: > This class needs a big helpful comment. Some things to cover: > - Each renderer process with a reference to this frame (other than the one > rendering it) has a RenderFrameProxy placeholder for it. > - This is the browser-side part of that placeholder. > - The placeholder allows us to keep existing window references valid and route > cross-process JS calls like postMessage. > - Lifetime (for now): Created when RFH swapped out, deleted when swapped back > in or unreferenced > - Lifetime (planned): RFH and RFPH live at mutually exclusive times (apart from > waiting for RFH to unload and exit?) Done. https://codereview.chromium.org/217163007/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:24: DCHECK(render_frame_host_.get()); On 2014/03/31 23:22:35, Charlie Reis wrote: > This is kind of redundant, since we'll crash on the next line anyway if it's > null. Done. https://codereview.chromium.org/217163007/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:31: if (render_frame_host_) On 2014/03/31 23:22:35, Charlie Reis wrote: > When is this not null? Does the RFPH ever outlive its reference to the RFH? Done.
Thanks. The comment in RFPH is hugely helpful. I still have some questions below, but I like where it's headed. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:454: DCHECK_NE(iter->second->render_frame_host()->GetSiteInstance(), I think it would make sense to put a GetSiteInstance() on RenderFrameProxyHost, since it'll have one of its own long term. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:563: scoped_ptr<RenderFrameHostImpl> rfh = proxy->PassFrameHost(); nit: I liked the swapped_out_rfh name from before. It's a bit more accurate and easy to grep for as we remove this long term. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:932: if (!swapped_out) { Why reverse this block? I'm not opposed, but we should update the comment as well if there's a reason to do it. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1063: old_render_frame_host->set_swapped_out(true); I don't think we want to do this for main frames, do we? I thought RFH::is_swapped_out_ is kind of broken and only used for subframes (i.e., behind the flag). https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1087: // If the pending frame was on the swapped out list, we can remove it. This suggests that the pending can still be on the proxy list, but we've removed the comment about that in the .h file. Which is right? (nit: If we keep this, s/swapped out list/proxy list/) https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1090: if (old_render_frame_host->render_view_host()->IsRenderViewLive()) { One suggestion to make this block of nested ifs slightly easier to parse-- let's invert this so that it becomes: if (!old_render_frame_host->render_view_host()->IsRenderViewLive()) { old_render_frame_host.reset(); return; } // If the old... https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1110: SiteInstanceImpl* old_site_instance = static_cast<SiteInstanceImpl*>( This makes me nervous. Deleting old_render_frame_host can cause the SiteInstance's refcount to drop to 0 and be deleted. We probably shouldn't keep it in a pointer, which might encourage others to use it unsafely at the end of the function. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1131: // swapped out list to simplify the deletion. I'm having trouble wrapping my head around this change (lines 1127-1138). It seems like a big semantic change, but I can't figure out why it's needed. (Also, the last sentence of the comment doesn't seem correct anymore.) Can you help me understand it? https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1141: old_render_frame_host.reset(NULL); The default argument is NULL, so you can just say reset() (here and above). https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1349: if (!site_instance->active_view_count()) { Isn't this backwards? If the view count is 0, we can delete it. Otherwise I think I agree that we can use the view count here rather than checking whether it was swapped out. (In fact, I think there are cases that's an improvement, like a new pending navigation to an existing SiteInstance which then gets canceled.) https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:452: // A map of site instance ID to RenderFrameProxyHosts. I'm actually not sure whether the pending RFH is ever in this map or not. For example, see my question in CommitPending. Can you give me an overview of how this case (navigations to existing entries) has changed? https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:453: typedef base::hash_map<int32, RenderFrameProxyHost*> RenderFrameHostMap; nit: We should probably call this RenderFrameProxyHostMap now. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... File content/browser/frame_host/render_frame_proxy_host.h (right): https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:17: // When a page is rendered across multiple processes, each renderer has a nit: a page's frames are rendered by multiple https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:18: // full copy of the frame tree. It has real frame objects for the frames it is real frame objects -> full RenderFrames https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:19: // responsible for rendering and placeholder objects for frames rendered by placeholder objects (i.e., RenderFrameProxies) https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:20: // other processes - RenderFrameProxy. nit: Blank line after this paragraph. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:21: // This class is the browser-site object for the placeholder. Each node in the browser-side host object https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:22: // frame tree has a RenderFrameHost for the SiteInstance of the frame and a set for the active SiteInstance and a set https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:23: // of RenderFrameProxyHost objects - one for all other SiteInstances associated associated with the frame tree -> with references to this frame https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:35: // alive. RenderFrameProxyHost and RenderFrameHost for the same SiteInstance can A RenderFrameHost and a RenderFrameProxyHost for https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:36: // exist at the same time, but only one will be "active" at a time. I don't understand what "active" means here. I didn't think RenderFrameProxyHost was ever considered active. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:48: // nit: Remove empty comment line. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:58: RenderFrameHostImpl* render_frame_host() { Can you move render_frame_host(), render_view_host(), and PassFrameHost() down a bit into a block with a TODO to remove them? We'll be keeping GetProcess(). https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:69: nit: No blank line here.
Fixes for all comments. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:454: DCHECK_NE(iter->second->render_frame_host()->GetSiteInstance(), On 2014/04/09 21:48:30, Charlie Reis wrote: > I think it would make sense to put a GetSiteInstance() on RenderFrameProxyHost, > since it'll have one of its own long term. Done. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:563: scoped_ptr<RenderFrameHostImpl> rfh = proxy->PassFrameHost(); On 2014/04/09 21:48:30, Charlie Reis wrote: > nit: I liked the swapped_out_rfh name from before. It's a bit more accurate and > easy to grep for as we remove this long term. Done. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:932: if (!swapped_out) { On 2014/04/09 21:48:30, Charlie Reis wrote: > Why reverse this block? I'm not opposed, but we should update the comment as > well if there's a reason to do it. Done. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1063: old_render_frame_host->set_swapped_out(true); On 2014/04/09 21:48:30, Charlie Reis wrote: > I don't think we want to do this for main frames, do we? I thought > RFH::is_swapped_out_ is kind of broken and only used for subframes (i.e., behind > the flag). Done. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1087: // If the pending frame was on the swapped out list, we can remove it. On 2014/04/09 21:48:30, Charlie Reis wrote: > This suggests that the pending can still be on the proxy list, but we've removed > the comment about that in the .h file. Which is right? > > (nit: If we keep this, s/swapped out list/proxy list/) Done. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1090: if (old_render_frame_host->render_view_host()->IsRenderViewLive()) { On 2014/04/09 21:48:30, Charlie Reis wrote: > One suggestion to make this block of nested ifs slightly easier to parse-- let's > invert this so that it becomes: > if (!old_render_frame_host->render_view_host()->IsRenderViewLive()) { > old_render_frame_host.reset(); > return; > } > > // If the old... Done. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1110: SiteInstanceImpl* old_site_instance = static_cast<SiteInstanceImpl*>( On 2014/04/09 21:48:30, Charlie Reis wrote: > This makes me nervous. Deleting old_render_frame_host can cause the > SiteInstance's refcount to drop to 0 and be deleted. We probably shouldn't keep > it in a pointer, which might encourage others to use it unsafely at the end of > the function. Done. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1131: // swapped out list to simplify the deletion. On 2014/04/09 21:48:30, Charlie Reis wrote: > I'm having trouble wrapping my head around this change (lines 1127-1138). It > seems like a big semantic change, but I can't figure out why it's needed. > (Also, the last sentence of the comment doesn't seem correct anymore.) > > Can you help me understand it? As we discussed, I've moved it around to preserve sane scoped_ptr semantics. The old_render_frame_host isn't added in the list, just deleted. The shutdown then will not delete it, since it will never be in the list. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1141: old_render_frame_host.reset(NULL); On 2014/04/09 21:48:30, Charlie Reis wrote: > The default argument is NULL, so you can just say reset() (here and above). Done. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1349: if (!site_instance->active_view_count()) { On 2014/04/09 21:48:30, Charlie Reis wrote: > Isn't this backwards? If the view count is 0, we can delete it. > > Otherwise I think I agree that we can use the view count here rather than > checking whether it was swapped out. (In fact, I think there are cases that's > an improvement, like a new pending navigation to an existing SiteInstance which > then gets canceled.) Done. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:452: // A map of site instance ID to RenderFrameProxyHosts. On 2014/04/09 21:48:30, Charlie Reis wrote: > I'm actually not sure whether the pending RFH is ever in this map or not. For > example, see my question in CommitPending. > > Can you give me an overview of how this case (navigations to existing entries) > has changed? Pending is no longer in that map. It is removed if found in the map and set as the pending, then returned to the map on cancelling navigation if the SiteInstance is being used by others. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:453: typedef base::hash_map<int32, RenderFrameProxyHost*> RenderFrameHostMap; On 2014/04/09 21:48:30, Charlie Reis wrote: > nit: We should probably call this RenderFrameProxyHostMap now. Done. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... File content/browser/frame_host/render_frame_proxy_host.h (right): https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:17: // When a page is rendered across multiple processes, each renderer has a On 2014/04/09 21:48:30, Charlie Reis wrote: > nit: a page's frames are rendered by multiple Done. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:18: // full copy of the frame tree. It has real frame objects for the frames it is On 2014/04/09 21:48:30, Charlie Reis wrote: > real frame objects -> full RenderFrames Done. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:19: // responsible for rendering and placeholder objects for frames rendered by On 2014/04/09 21:48:30, Charlie Reis wrote: > placeholder objects (i.e., RenderFrameProxies) Done. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:20: // other processes - RenderFrameProxy. On 2014/04/09 21:48:30, Charlie Reis wrote: > nit: Blank line after this paragraph. Done. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:21: // This class is the browser-site object for the placeholder. Each node in the On 2014/04/09 21:48:30, Charlie Reis wrote: > browser-side host object Too much "site" isolation ;) https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:22: // frame tree has a RenderFrameHost for the SiteInstance of the frame and a set On 2014/04/09 21:48:30, Charlie Reis wrote: > for the active SiteInstance and a set Done. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:23: // of RenderFrameProxyHost objects - one for all other SiteInstances associated On 2014/04/09 21:48:30, Charlie Reis wrote: > associated with the frame tree -> with references to this frame Done. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:35: // alive. RenderFrameProxyHost and RenderFrameHost for the same SiteInstance can On 2014/04/09 21:48:30, Charlie Reis wrote: > A RenderFrameHost and a RenderFrameProxyHost for Done. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:36: // exist at the same time, but only one will be "active" at a time. On 2014/04/09 21:48:30, Charlie Reis wrote: > I don't understand what "active" means here. I didn't think > RenderFrameProxyHost was ever considered active. The object to use when sending IPC messages. I don't quite like the word active (from previous experiences ;) ), but I'm not sure what is a better one. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:48: // On 2014/04/09 21:48:30, Charlie Reis wrote: > nit: Remove empty comment line. Done. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:58: RenderFrameHostImpl* render_frame_host() { On 2014/04/09 21:48:30, Charlie Reis wrote: > Can you move render_frame_host(), render_view_host(), and PassFrameHost() down a > bit into a block with a TODO to remove them? We'll be keeping GetProcess(). Done. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:69: On 2014/04/09 21:48:30, Charlie Reis wrote: > nit: No blank line here. Done.
I added a test case for CancelPending.
Great. LGTM with the comments below. https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... File content/browser/frame_host/render_frame_proxy_host.h (right): https://codereview.chromium.org/217163007/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:36: // exist at the same time, but only one will be "active" at a time. On 2014/04/10 20:37:36, nasko wrote: > On 2014/04/09 21:48:30, Charlie Reis wrote: > > I don't understand what "active" means here. I didn't think > > RenderFrameProxyHost was ever considered active. > > The object to use when sending IPC messages. I don't quite like the word active > (from previous experiences ;) ), but I'm not sure what is a better one. If I understand the reason this can happen correctly, what about this? "A RenderFrameProxyHost and a RenderFrameHost for the same SiteInstance can exist at the same time, but only while the RenderFrameHost is waiting to be deleted in the background." https://codereview.chromium.org/217163007/diff/110001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/217163007/diff/110001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1091: old_render_frame_host.reset(); Actually, do we even need this line? It's a scoped_ptr so I think we get it for free with the return. https://codereview.chromium.org/217163007/diff/110001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/217163007/diff/110001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:1805: RenderFrameHostImpl* pending_rfh; = NULL; https://codereview.chromium.org/217163007/diff/110001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:1817: ->pending_frame_host(); I would have put -> on the previous line, but I defer to clang_format. https://codereview.chromium.org/217163007/diff/110001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:1820: // Cancel the navigation by simulating user cancelled through beforeunload by simulating a declined beforeunload dialog. https://codereview.chromium.org/217163007/diff/110001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:1831: // Start another cross-site navigation nit: End with period. https://codereview.chromium.org/217163007/diff/110001/content/browser/frame_h... File content/browser/frame_host/render_frame_proxy_host.h (right): https://codereview.chromium.org/217163007/diff/110001/content/browser/frame_h... content/browser/frame_host/render_frame_proxy_host.h:53: SiteInstanceImpl* site_instance, Why not grab this from |render_frame_host| in the constructor for now, and skip the |site_instance| parameter? That prevents callers from ever passing a mismatch.
Moar fixes! https://codereview.chromium.org/217163007/diff/110001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/217163007/diff/110001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1091: old_render_frame_host.reset(); On 2014/04/11 17:42:53, Charlie Reis wrote: > Actually, do we even need this line? It's a scoped_ptr so I think we get it for > free with the return. Done. https://codereview.chromium.org/217163007/diff/110001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/217163007/diff/110001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:1805: RenderFrameHostImpl* pending_rfh; On 2014/04/11 17:42:53, Charlie Reis wrote: > = NULL; Done. https://codereview.chromium.org/217163007/diff/110001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:1817: ->pending_frame_host(); On 2014/04/11 17:42:53, Charlie Reis wrote: > I would have put -> on the previous line, but I defer to clang_format. clang-format dictates this :( I dislike it too. https://codereview.chromium.org/217163007/diff/110001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:1820: // Cancel the navigation by simulating user cancelled through beforeunload On 2014/04/11 17:42:53, Charlie Reis wrote: > by simulating a declined beforeunload dialog. Done. https://codereview.chromium.org/217163007/diff/110001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:1831: // Start another cross-site navigation On 2014/04/11 17:42:53, Charlie Reis wrote: > nit: End with period. Done. https://codereview.chromium.org/217163007/diff/110001/content/browser/frame_h... File content/browser/frame_host/render_frame_proxy_host.h (right): https://codereview.chromium.org/217163007/diff/110001/content/browser/frame_h... content/browser/frame_host/render_frame_proxy_host.h:53: SiteInstanceImpl* site_instance, On 2014/04/11 17:42:53, Charlie Reis wrote: > Why not grab this from |render_frame_host| in the constructor for now, and skip > the |site_instance| parameter? That prevents callers from ever passing a > mismatch. Yeah, good idea. Tricky, but I think we are good.
The CQ bit was checked by nasko@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nasko@chromium.org/217163007/130001
Message was sent while issue was closed.
Change committed as 263352 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
