Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
753173002:
Preparation steps for adding speculative renderer creation. (Closed)
DescriptionPreparation steps for adding speculative renderer creation.
Generalized the RenderFrameHost and WebUI creation methods to allow for the
following step of speculatively creating renderers in browser side navigations
for the PlzNavigate project (spliting off of crrev.com/701953006).
BUG=376094
TBR=creis@chromium.org
Committed: https://crrev.com/37f79379aa9254a5070b8b276486e2842c4b1001
Cr-Commit-Position: refs/heads/master@{#305989}
Patch Set 1 #Patch Set 2 : Minor missing things. #
Total comments: 14
Patch Set 3 : Address CR comments and makes a RFHM method private. #
Total comments: 13
Patch Set 4 : TODO comments removed and new lines added. #
Total comments: 20
Patch Set 5 : Address a new round of CR comments: renames, comments and minor changes. #
Messages
Total messages: 17 (2 generated)
carlosk@chromium.org changed reviewers: + clamy@chromium.org, creis@chromium.org, nasko@chromium.org
Dear reviewers, PTAL. This is the split off of crrev.com/701953006 as previously discussed. It provides the initial set of changes to dissociate the creation of new RenderFrames from the pending_* members of RFHM.
Some initial coments. https://codereview.chromium.org/753173002/diff/20001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/753173002/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1026: if (delegate_->IsHidden()) nit: empty line before the if statement https://codereview.chromium.org/753173002/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1028: int opener_route_id = nit: Same here, empty line above it. Unrelated statements are easier to read if they are visually separated. https://codereview.chromium.org/753173002/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1095: int* routing_id_ptr) { Why take in this as param? The callers can use the returned RFH to get whichever routing ID they are interested in. https://codereview.chromium.org/753173002/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1109: if (routing_id_ptr) Not passing in this pointer as argument will also make the code a lot easier to read, no need for if checks all over the place. https://codereview.chromium.org/753173002/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1113: // never create it in the same SiteInstance as our current RFH. nit: "never create" appears twice. https://codereview.chromium.org/753173002/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1199: return nullptr; Is this a valid return value for a scoped_ptr? https://codereview.chromium.org/753173002/diff/20001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/753173002/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:302: // not NULL it will be set to the routing id of the *view* associated with the nit: null or nullptr
Thanks nasko@. Replied to your comments and added a patch. After discussing with clamy@ I also made RFHM::SetPendingWebUI private as it didn't seem to make sense to have it public. https://codereview.chromium.org/753173002/diff/20001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/753173002/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1026: if (delegate_->IsHidden()) On 2014/11/24 17:00:27, nasko wrote: > nit: empty line before the if statement Done. https://codereview.chromium.org/753173002/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1028: int opener_route_id = On 2014/11/24 17:00:28, nasko wrote: > nit: Same here, empty line above it. Unrelated statements are easier to read if > they are visually separated. Done. https://codereview.chromium.org/753173002/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1095: int* routing_id_ptr) { On 2014/11/24 17:00:27, nasko wrote: > Why take in this as param? The callers can use the returned RFH to get whichever > routing ID they are interested in. Because if the RFH is flagged to be created swapped out its pointer is not returned because its ownership is immediately given to the proxy. As this is always the case for the external calls (not from RHFM itself) and it's them that need the routing ID, it is still needed. https://codereview.chromium.org/753173002/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1109: if (routing_id_ptr) On 2014/11/24 17:00:27, nasko wrote: > Not passing in this pointer as argument will also make the code a lot easier to > read, no need for if checks all over the place. I don't think that's possible as explained above. https://codereview.chromium.org/753173002/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1113: // never create it in the same SiteInstance as our current RFH. On 2014/11/24 17:00:27, nasko wrote: > nit: "never create" appears twice. Done. https://codereview.chromium.org/753173002/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1199: return nullptr; On 2014/11/24 17:00:27, nasko wrote: > Is this a valid return value for a scoped_ptr? I was also in doubt but: a) the code compiled without issue (and a lot of times code didn't compile with scoped_ptr because of tall checks it has around it) b) All tests (automatic and my manual ones) did work so far. https://codereview.chromium.org/753173002/diff/20001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/753173002/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:302: // not NULL it will be set to the routing id of the *view* associated with the On 2014/11/24 17:00:28, nasko wrote: > nit: null or nullptr Done.
Another round of comments. https://codereview.chromium.org/753173002/diff/40001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/753173002/diff/40001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:562: void RenderFrameHostManager::DiscardRenderFrameHost( Now that I think about it more, I'm not sure why all this work will be needed for the speculative RFH. Since the new RFH wouldn't have loaded any document or be swapped out already, there is no reason for us to swap it out, just kill it. Also, since we know we created a speculative RFH, we don't need to stash it into a proxy either, so I think we don't need all this code for speculative RFH. https://codereview.chromium.org/753173002/diff/40001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1032: // TODO(carlosk): does this "earlier" call for CancelPending affects anything? I didn't add the same comment about empty line to each occurrence. I expected one here and after CancelPending below. https://codereview.chromium.org/753173002/diff/40001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1110: bool success = true; Why do you need to pre-declare success here? It is only used in the else block where it used to be declared before. https://codereview.chromium.org/753173002/diff/40001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1185: // TODO(carlosk): verify there's no problem that now this RFH will only Let's verify this TODO before committing this code. It has to have a clear answer. https://codereview.chromium.org/753173002/diff/40001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1197: if (success && !swapped_out) { Why not return always if new_render_frame_host is non-null?
Thanks for this new round. https://codereview.chromium.org/753173002/diff/40001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/753173002/diff/40001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:562: void RenderFrameHostManager::DiscardRenderFrameHost( On 2014/11/24 23:59:08, nasko wrote: > Now that I think about it more, I'm not sure why all this work will be needed > for the speculative RFH. Since the new RFH wouldn't have loaded any document or > be swapped out already, there is no reason for us to swap it out, just kill it. > Also, since we know we created a speculative RFH, we don't need to stash it into > a proxy either, so I think we don't need all this code for speculative RFH. The same thing is done for non-utilized pending RFHs and that's why it's replicated here. Mind that the speculative RFH might have been resurrected from a swapped out state so we might not have just created it and it might have loaded documents before being swapped out. IDK if these facts make any difference to what you're suggesting so let me know if I should still remove this logic for the speculative case. https://codereview.chromium.org/753173002/diff/40001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1032: // TODO(carlosk): does this "earlier" call for CancelPending affects anything? On 2014/11/24 23:59:08, nasko wrote: > I didn't add the same comment about empty line to each occurrence. I expected > one here and after CancelPending below. Acknowledged and done. https://codereview.chromium.org/753173002/diff/40001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1034: // successfully created. I'm also removing this TODO here. My testing showed no issues but I can't know for sure by looking at the code if there are any. I'm not super worried as the pending case should go away at some point... Any comments? https://codereview.chromium.org/753173002/diff/40001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1110: bool success = true; On 2014/11/24 23:59:08, nasko wrote: > Why do you need to pre-declare success here? It is only used in the else block > where it used to be declared before. It's also used in the last top-level if-block to decide what value this method should be return. https://codereview.chromium.org/753173002/diff/40001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1185: // TODO(carlosk): verify there's no problem that now this RFH will only On 2014/11/24 23:59:08, nasko wrote: > Let's verify this TODO before committing this code. It has to have a clear > answer. Yes. For this case I found no problems in any of my testing and i went over the call tree for it and it doesn't seem they chain to a call to try and get the pending RFH for non-test code (that one might in deeper levels but we'll have to deal with it anyway). I'm removing this this one already but let me know if any of you have extra comments. https://codereview.chromium.org/753173002/diff/40001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1197: if (success && !swapped_out) { On 2014/11/24 23:59:08, nasko wrote: > Why not return always if new_render_frame_host is non-null? Because of line 1179. If that one fails new_render_frame_host is not null but should not be returned but simply destroyed. Hence the success check here.
Thanks! Lgtm
I think it is unfortunate that the code becomes as complex as it is and we should revisit it when we have more complete implementation. LGTM for now to make progress. https://codereview.chromium.org/753173002/diff/40001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/753173002/diff/40001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:562: void RenderFrameHostManager::DiscardRenderFrameHost( On 2014/11/25 10:54:30, carlosk wrote: > On 2014/11/24 23:59:08, nasko wrote: > > Now that I think about it more, I'm not sure why all this work will be needed > > for the speculative RFH. Since the new RFH wouldn't have loaded any document > or > > be swapped out already, there is no reason for us to swap it out, just kill > it. > > Also, since we know we created a speculative RFH, we don't need to stash it > into > > a proxy either, so I think we don't need all this code for speculative RFH. > > The same thing is done for non-utilized pending RFHs and that's why it's > replicated here. Mind that the speculative RFH might have been resurrected from > a swapped out state so we might not have just created it and it might have > loaded documents before being swapped out. > > IDK if these facts make any difference to what you're suggesting so let me know > if I should still remove this logic for the speculative case. One thing is clear, we don't need to call rfh->SwapOut(proxy) in the case of speculative RFH deletion. I think it might still be fine to leave this factored out.
I'm taking a look now; please wait for my review. (Sorry for the delay!) https://codereview.chromium.org/753173002/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/753173002/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:143: WebUIImpl* new_web_ui = delegate_->CreateWebUIForRenderManager(url); If this were a scoped_ptr, we wouldn't need the delete and it would be more explicit that the caller takes ownership. https://codereview.chromium.org/753173002/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/753173002/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:307: const WebUIImpl* web_ui, DRAFT: This seems like a strange type. https://codereview.chromium.org/753173002/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:308: int* routing_id_ptr); We should put "view" in the name of this parameter, now that we can. https://codereview.chromium.org/753173002/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:494: void DiscardRenderFrameHost( DiscardUnusedRenderFrameHost (or DiscardUnusedFrame) would be clearer (vs SwapOutOldFrame).
On 2014/11/26 00:42:54, Charlie Reis (OOO until Dec 1) wrote: > I'm taking a look now; please wait for my review. (Sorry for the delay!) Oops, didn't mean for the comments so far to be sent just yet. More on the way. :) > https://codereview.chromium.org/753173002/diff/60001/content/browser/frame_ho... > content/browser/frame_host/render_frame_host_manager.h:307: const WebUIImpl* > web_ui, > DRAFT: This seems like a strange type. Ignore this for now. I need to look more closely.
Ok, here's the rest of the comments. Thanks for splitting all of this out into a smaller CL-- it should help the other one be more legible. The comments here are mostly minor. I'll be out for the Thanksgiving holiday until December 1. If your next patch is basically "Done" for all of these comments, then feel free to mark me TBR and commit. If other questions come up, I'll approve when I come back. Thanks! https://codereview.chromium.org/753173002/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/753173002/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:568: // delete the RFH, just swap it out and it can be reused at a later point. nit: This is a run-on sentence, so use a semicolon or period here rather than a comma. https://codereview.chromium.org/753173002/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:577: render_frame_host->SwapOut(proxy); This makes sense when called by CancelPending. Are you planning to use this method for speculative RFHs as well? In that case I don't think we'll need to call SwapOut. Either it was already swapped out (and we didn't swap it in) or we created it from scratch and we can delete it. (That can be a topic for the next CL, though.) https://codereview.chromium.org/753173002/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1037: pending_render_frame_host_ = I see that it could be surprising that this method affects the pending_render_frame_host_ (even before this CL). Could you add something to its declaration comment about that? https://codereview.chromium.org/753173002/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/753173002/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:307: const WebUIImpl* web_ui, On 2014/11/26 00:42:54, Charlie Reis (OOO until Dec 1) wrote: > DRAFT: This seems like a strange type. At the moment, it's hard to tell what this parameter is for. const& would be more common than const*, and it's confusing that it's a raw pointer next to an out parameter (so at first it looked like another out parameter). I suggest dropping the const and moving it to be the second parameter. Please also document it in the comment. https://codereview.chromium.org/753173002/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:409: // to |bindings|. The caller is responsible for the instance ownership. Yes, should return a scoped_ptr then. https://codereview.chromium.org/753173002/diff/60001/content/browser/site_ins... File content/browser/site_instance_impl.cc (right): https://codereview.chromium.org/753173002/diff/60001/content/browser/site_ins... content/browser/site_instance_impl.cc:144: DCHECK(!browsing_instance_->HasSiteInstance(url)); I think this can fail, right? See the comment in BrowsingInstance::RegisterSiteInstance. That will likely change in my planned SiteInstance site URL changes, but I'm not sure it makes sense to add this until then.
Thanks once more. On 2014/11/26 00:13:03, nasko (Out until Dec 08) wrote: > I think it is unfortunate that the code becomes as complex as it is and we > should revisit it when we have more complete implementation. > LGTM for now to make progress. The issue here is trying to keep the new and old code paths the same. Once we finally decide to separate them and work on killing the old navigation I think everything will get much simpler. On 2014/11/26 01:06:40, Charlie Reis (OOO until Dec 1) wrote: > Ok, here's the rest of the comments. Thanks for splitting all of this out into > a smaller CL-- it should help the other one be more legible. The comments here > are mostly minor. > > I'll be out for the Thanksgiving holiday until December 1. If your next patch > is basically "Done" for all of these comments, then feel free to mark me TBR and > commit. If other questions come up, I'll approve when I come back. Thanks! Thanks. I'll go ahead and to it. https://codereview.chromium.org/753173002/diff/40001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/753173002/diff/40001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:562: void RenderFrameHostManager::DiscardRenderFrameHost( On 2014/11/26 00:13:02, nasko (Out until Dec 08) wrote: > On 2014/11/25 10:54:30, carlosk wrote: > > On 2014/11/24 23:59:08, nasko wrote: > > > Now that I think about it more, I'm not sure why all this work will be > needed > > > for the speculative RFH. Since the new RFH wouldn't have loaded any document > > or > > > be swapped out already, there is no reason for us to swap it out, just kill > > it. > > > Also, since we know we created a speculative RFH, we don't need to stash it > > into > > > a proxy either, so I think we don't need all this code for speculative RFH. > > > > The same thing is done for non-utilized pending RFHs and that's why it's > > replicated here. Mind that the speculative RFH might have been resurrected > from > > a swapped out state so we might not have just created it and it might have > > loaded documents before being swapped out. > > > > IDK if these facts make any difference to what you're suggesting so let me > know > > if I should still remove this logic for the speculative case. > > One thing is clear, we don't need to call rfh->SwapOut(proxy) in the case of > speculative RFH deletion. I think it might still be fine to leave this factored > out. Acknowledged. https://codereview.chromium.org/753173002/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/753173002/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:143: WebUIImpl* new_web_ui = delegate_->CreateWebUIForRenderManager(url); On 2014/11/26 00:42:54, Charlie Reis (OOO until Dec 1) wrote: > If this were a scoped_ptr, we wouldn't need the delete and it would be more > explicit that the caller takes ownership. Understood and as it involved changing only one extra file (the others were already in this CL) I just went ahead and changed it. https://codereview.chromium.org/753173002/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:568: // delete the RFH, just swap it out and it can be reused at a later point. On 2014/11/26 01:06:40, Charlie Reis (OOO until Dec 1) wrote: > nit: This is a run-on sentence, so use a semicolon or period here rather than a > comma. Done. https://codereview.chromium.org/753173002/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:577: render_frame_host->SwapOut(proxy); On 2014/11/26 01:06:40, Charlie Reis (OOO until Dec 1) wrote: > This makes sense when called by CancelPending. > > Are you planning to use this method for speculative RFHs as well? In that case > I don't think we'll need to call SwapOut. Either it was already swapped out > (and we didn't swap it in) or we created it from scratch and we can delete it. > (That can be a topic for the next CL, though.) Yes, it should be a topic for the other CL. But at this point if the RFH was previously swapped out it did already went through the process of being swapped-in (at least I think that's what happens in RFHM::CreateRenderFrame in the if-block beginning with: if (proxy && proxy->render_frame_host()) { Isn't that so? Also at this point we have no idea if the RFH was newly created or not. https://codereview.chromium.org/753173002/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1037: pending_render_frame_host_ = On 2014/11/26 01:06:40, Charlie Reis (OOO until Dec 1) wrote: > I see that it could be surprising that this method affects the > pending_render_frame_host_ (even before this CL). Could you add something to > its declaration comment about that? Done: changed the name and updated the comment. https://codereview.chromium.org/753173002/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/753173002/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:307: const WebUIImpl* web_ui, On 2014/11/26 01:06:40, Charlie Reis (OOO until Dec 1) wrote: > On 2014/11/26 00:42:54, Charlie Reis (OOO until Dec 1) wrote: > > DRAFT: This seems like a strange type. > > At the moment, it's hard to tell what this parameter is for. const& would be > more common than const*, and it's confusing that it's a raw pointer next to an > out parameter (so at first it looked like another out parameter). > > I suggest dropping the const and moving it to be the second parameter. Please > also document it in the comment. Done. I agree it was confusing and I was in doubt on how to handle that one. It is an input parameter but it's an optional one hence it being a const pointer. https://codereview.chromium.org/753173002/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:308: int* routing_id_ptr); On 2014/11/26 00:42:54, Charlie Reis (OOO until Dec 1) wrote: > We should put "view" in the name of this parameter, now that we can. Done. https://codereview.chromium.org/753173002/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:409: // to |bindings|. The caller is responsible for the instance ownership. On 2014/11/26 01:06:40, Charlie Reis (OOO until Dec 1) wrote: > Yes, should return a scoped_ptr then. Done. I also removed that extra comment as the scoped_ptr makes ownership explicit. https://codereview.chromium.org/753173002/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:494: void DiscardRenderFrameHost( On 2014/11/26 00:42:54, Charlie Reis (OOO until Dec 1) wrote: > DiscardUnusedRenderFrameHost (or DiscardUnusedFrame) would be clearer (vs > SwapOutOldFrame). Done. https://codereview.chromium.org/753173002/diff/60001/content/browser/site_ins... File content/browser/site_instance_impl.cc (right): https://codereview.chromium.org/753173002/diff/60001/content/browser/site_ins... content/browser/site_instance_impl.cc:144: DCHECK(!browsing_instance_->HasSiteInstance(url)); On 2014/11/26 01:06:40, Charlie Reis (OOO until Dec 1) wrote: > I think this can fail, right? See the comment in > BrowsingInstance::RegisterSiteInstance. > > That will likely change in my planned SiteInstance site URL changes, but I'm not > sure it makes sense to add this until then. Yes, you are right. I had read that note but later on I assumed this was an error case. Removed.
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/753173002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/37f79379aa9254a5070b8b276486e2842c4b1001 Cr-Commit-Position: refs/heads/master@{#305989}
Message was sent while issue was closed.
Thanks. Still LGTM. https://codereview.chromium.org/753173002/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/753173002/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:577: render_frame_host->SwapOut(proxy); On 2014/11/27 11:02:43, carlosk wrote: > On 2014/11/26 01:06:40, Charlie Reis (OOO until Dec 1) wrote: > > This makes sense when called by CancelPending. > > > > Are you planning to use this method for speculative RFHs as well? In that > case > > I don't think we'll need to call SwapOut. Either it was already swapped out > > (and we didn't swap it in) or we created it from scratch and we can delete it. > > > (That can be a topic for the next CL, though.) > > Yes, it should be a topic for the other CL. Sounds good. > > But at this point if the RFH was previously swapped out it did already went > through the process of being swapped-in (at least I think that's what happens in > RFHM::CreateRenderFrame in the if-block beginning with: > > if (proxy && proxy->render_frame_host()) { > > Isn't that so? No, that block just deletes the proxy. We don't actually change the state away from STATE_SWAPPED_OUT until we get to the RFHI::Navigate call. > > Also at this point we have no idea if the RFH was newly created or not. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
