|
|
Created:
5 years, 3 months ago by carlosk Modified:
5 years, 1 month ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, creis+watch_chromium.org, darin-cc_chromium.org, Evan Stade Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove WebUI ownership from the RenderFrameHostManager to the RenderFrameHost.
This change refactors the current ownership of the WebUI instance,
moving it from the RenderFrameHostManager to the RenderFrameHost which
is directly associated with the WebUI.
The design document for this change is here:
https://docs.google.com/a/chromium.org/document/d/1jrge5ulZ1YtqSR3WkNXzrtxhd3JXcWm8_zWBhEjaJtI
BUG=508850
Committed: https://crrev.com/e5c678bf5b084988a60a4636342414f2831c1695
Cr-Commit-Position: refs/heads/master@{#358050}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Latest round of fixes. #Patch Set 3 : More test fixes and improvements but the code is stil a WIP! #
Total comments: 8
Patch Set 4 : Rebase (post Blink repo merge) #Patch Set 5 : Rebase #Patch Set 6 : Still in a WIP state but wanted to upload the latest changes. #Patch Set 7 : Got to an almost working state and addressed reviewer comments. #
Total comments: 27
Patch Set 8 : Removed InitializeWebUI #
Total comments: 53
Patch Set 9 : Latest code review changes. #Patch Set 10 : Fixed uninitialized variable and missing call to WebUIImpl::RenderViewReused. #
Total comments: 2
Patch Set 11 : Rebase. #
Total comments: 11
Patch Set 12 : Doesn't try to create a WebUI if URL is invalid (fixes unit_tests). #
Total comments: 10
Patch Set 13 : Rebase (that seems to fix last week's renderer crash). #Patch Set 14 : Reverted to having RFHI::InitializeWebUI and other minor changes. #Patch Set 15 : Trying to fix the Mac test errors. #Patch Set 16 : Fixed ChromeOS errors caused by improper timing of RenderView(Created|Reused) calls. #
Total comments: 27
Patch Set 17 : Changes to comments and small code fixes from nasko@'s comments. #
Total comments: 1
Patch Set 18 : Simplified the code mainly by consolidating WebUIImpl::RenderView* calls. #
Total comments: 17
Patch Set 19 : Rebased and fixed conflicts with http://crbug.com/1403343002 #Patch Set 20 : Minor fixes and changes from CR comments. #
Total comments: 16
Patch Set 21 : One more round of minor fixes and changes from CR comments. #Patch Set 22 : Rebased and fixed conflicts of the revert of http://crbug.com/1403343002 :/ #
Total comments: 36
Patch Set 23 : Rebased and fixed conflicts. #Patch Set 24 : Addressing some of the current CR comments (before moving on to eliminate the pending WebUI). #
Total comments: 1
Patch Set 25 : Rebase. #Patch Set 26 : WIP: removing the pending WebUI (failing tests; no PlzNavigate support). #Patch Set 27 : WIP: removed the pending WebUI from RFHI (did not yet remove the pending WebUI API from RFHM). #Patch Set 28 : Rebase to fix patch errors. #
Total comments: 17
Patch Set 29 : Addressing lastest and previously not responded comments. #Patch Set 30 : Added one more active WebUI transition special case. #Patch Set 31 : Rename method RFHM::pending_web_ui to GetNavigatingWebUI. #Patch Set 32 : Rebase to fix patch issues. #Patch Set 33 : Fixed header comments. #
Total comments: 10
Patch Set 34 : Rebase. #Patch Set 35 : Minor changes to adress latest CR comments. #
Total comments: 19
Patch Set 36 : Addressed latest comments by nasko@. #
Total comments: 2
Patch Set 37 : Minor comment updates. #Dependent Patchsets: Messages
Total messages: 62 (14 generated)
clamy@chromium.org changed reviewers: + clamy@chromium.org
Thanks! A few comments on this version. https://codereview.chromium.org/1352813006/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/1352813006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_delegate.h:170: virtual scoped_ptr<WebUIImpl> CreateWebUIForRenderManager(const GURL& url); This function should be renamed, since it's not coming from the RenderManager anymore. https://codereview.chromium.org/1352813006/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1352813006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.h:811: // Note: These are not used in PlzNavigate. Which case would warrant having this in the current navigation logic but not in PlzNavigate? I think one of the goals of this refactoring is to avoid having differences between the current architecture and the PlzNavigate one. https://codereview.chromium.org/1352813006/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:986: DCHECK(speculative_web_ui() == render_frame_host_->pending_web_ui()); This should be DCHECK(!speculative_render_frame_host_) and then use render_frame_host_->pending_web_ui() instead of speculative_web_ui() to make it easier to understand what's happening there. https://codereview.chromium.org/1352813006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:1057: render_frame_host_->DiscardPendingWebUI(); What if you committed a created WebUI at the start of the navigation? https://codereview.chromium.org/1352813006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:1731: new_render_frame_host->InitializeWebUI(url, bindings); Why do we need to do that instead of using a RFH creation method that takes those as parameters? https://codereview.chromium.org/1352813006/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/1352813006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.h:258: WebUIImpl* web_ui() const { return render_frame_host_->web_ui(); } If we move WebUIs out of the RFHM then I think these accessors should move as well. The goal here is to remove the concept of WebUIs from the RFHM.
carlosk@chromium.org changed reviewers: + creis@chromium.org, dbeam@chromium.org, fdegans@chromium.org, nasko@chromium.org
Adding interested people for voluntary early feedback. Mind this is still a WIP so there are still broken tests, code style violations, lack of comments and the like.
Some perfcrastination and few small nits. Most of my comments are in the design doc. https://codereview.chromium.org/1352813006/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/1352813006/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_delegate.h:171: // returns NULL. nit: null. https://codereview.chromium.org/1352813006/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1031: web_ui_type_ = WebUI::kNoWebUI; Are we sure this is safe to do? What if the renderer needs to send IPCs up to the WebUI component as part of cleanup? https://codereview.chromium.org/1352813006/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1980: should_reuse_web_ui_ = Some textual comment explaining when exactly we should reuse the WebUI will be very useful. One can parse the code that follows, but having explicit verbal intent stated prior can help ensure correctness. https://codereview.chromium.org/1352813006/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1352813006/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:3784: // TODO(carlosk): I'm testing moving this into RenderFrameHostImpl::InitializeWebUI This code doesn't really belong here, as WebContents shouldn't need to know about state management done by RFHM. I'm all for moving it elsewhere.
Patchset #4 (id:60001) has been deleted
Patchset #7 (id:140001) has been deleted
Thanks! I think this is looking way better now but there's still failing tests (only in release; same tests pass in debug) and as you'll see in the comments there's a couple changes that will go in in the next patch set. Nonetheless I'd appreciate your input on how things look right now as I don't expect much changes when I address the mentioned issues. https://codereview.chromium.org/1352813006/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/1352813006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_delegate.h:170: virtual scoped_ptr<WebUIImpl> CreateWebUIForRenderManager(const GURL& url); On 2015/09/17 17:04:37, clamy wrote: > This function should be renamed, since it's not coming from the RenderManager > anymore. Done. Thanks for noticing this. https://codereview.chromium.org/1352813006/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1352813006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.h:811: // Note: These are not used in PlzNavigate. On 2015/09/17 17:04:37, clamy wrote: > Which case would warrant having this in the current navigation logic but not in > PlzNavigate? I think one of the goals of this refactoring is to avoid having > differences between the current architecture and the PlzNavigate one. The comment wasn't yet updated after copy/pasting this code (it is now). I am using the same members for both implementations. https://codereview.chromium.org/1352813006/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:986: DCHECK(speculative_web_ui() == render_frame_host_->pending_web_ui()); On 2015/09/17 17:04:37, clamy wrote: > This should be DCHECK(!speculative_render_frame_host_) and then use > render_frame_host_->pending_web_ui() instead of speculative_web_ui() to make it > easier to understand what's happening there. I added the DCHECK you suggested. But what I am testing here is specifically that speculative_web_ui() returns the correct value so it must stay. https://codereview.chromium.org/1352813006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:1057: render_frame_host_->DiscardPendingWebUI(); On 2015/09/17 17:04:38, clamy wrote: > What if you committed a created WebUI at the start of the navigation? I assume you are referring to the early committing of the pending RFH when the current renderer is not live. If that's the case there would not be a pending WebUI as for cross-site navigations there's never a pending WebUI set on any RFH (a new RFH always come with a new *active* WebUI when one is needed). And this follows the same behavior of today: we don't "revert" early committed RFH/WebUI. https://codereview.chromium.org/1352813006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:1731: new_render_frame_host->InitializeWebUI(url, bindings); On 2015/09/17 17:04:37, clamy wrote: > Why do we need to do that instead of using a RFH creation method that takes > those as parameters? I did this mainly to avoid having to pump down (even deeper) the URL and past_bindings needed to create a WebUI into the RFH creation call chain creating even more changes in RFHM and adding other files. It would also increase the already large number of parameters in the RFH constructor. But as this method must always be called along with the RFH constructor I agree it would be better that way. I will make this change for the following patch set and see how it goes. https://codereview.chromium.org/1352813006/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/1352813006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.h:258: WebUIImpl* web_ui() const { return render_frame_host_->web_ui(); } On 2015/09/17 17:04:38, clamy wrote: > If we move WebUIs out of the RFHM then I think these accessors should move as > well. The goal here is to remove the concept of WebUIs from the RFHM. I partially agree. I did this initially to make this change simpler as these 3 getters are used in almost 40 different places. The problem is that if we eliminate them from there we have to replicate the what-RFH-to-get-the-speculative/pending-webui-from-logic from the new implementations for every caller. And this seems the correct place for that. I added a future work entry in the design doc to go over this point in a future change. One thing we might want to consider anyway is unifying the pending/speculative getters as they are basically all used behind a is-plz-navigate-enabled check that could be moved into the unified method. https://codereview.chromium.org/1352813006/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/1352813006/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_delegate.h:171: // returns NULL. On 2015/09/18 18:40:19, nasko (slow to review) wrote: > nit: null. Done. But should I use null or nullptr in comments? I used the latter for now. https://codereview.chromium.org/1352813006/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1031: web_ui_type_ = WebUI::kNoWebUI; On 2015/09/18 18:40:19, nasko (slow to review) wrote: > Are we sure this is safe to do? What if the renderer needs to send IPCs up to > the WebUI component as part of cleanup? In the cases where SwapOut is called -- when committing a navigation and when discarding the pending/speculative RFH -- the associated WebUI is currently destroyed before this call happens. So this seems to be safe. https://codereview.chromium.org/1352813006/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1980: should_reuse_web_ui_ = On 2015/09/18 18:40:19, nasko (slow to review) wrote: > Some textual comment explaining when exactly we should reuse the WebUI will be > very useful. One can parse the code that follows, but having explicit verbal > intent stated prior can help ensure correctness. Done. https://codereview.chromium.org/1352813006/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1352813006/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:3784: // TODO(carlosk): I'm testing moving this into RenderFrameHostImpl::InitializeWebUI On 2015/09/18 18:40:19, nasko (slow to review) wrote: > This code doesn't really belong here, as WebContents shouldn't need to know > about state management done by RFHM. I'm all for moving it elsewhere. Acknowledged and done. https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_delegate.h:170: virtual scoped_ptr<WebUIImpl> CreateMainFrameWebUI(const GURL& url); This seemed an appropriate name for this already a method named WebContentsImpl::CreateSubframeWebUI. But I realized later this might be called for subframes as well so I'll rename it in the next patch set. https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1979: web_ui_->RenderViewCreated(render_view_host_); With this change the order of creation of the RVH and WebUI changed to this order: first the RVH, then the WebUI. Consequentially I had to change where WebUIImpl::RenderViewCreated is called from (used to be from the WebContentsImpl). For this to work I trust that: 1) InitializeWebUI will only be called for brand new RFH instances; and 2) If this is a new RFH instance for the root of the frame tree then its RVH must also have been newly created. Similarly I moved the calls to WebUIImpl::RenderViewReused from the RFHM to RFHI::UpdatePendingWebUI (further below). For this case the situation is more straightforward: if a pending WebUI is being created it must be that the RFH is being reused (and consequentially its RVH). WDYT about both of these? https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1663: RenderFrameHostManager::CreateRenderFrameInternal(SiteInstance* instance, I chose to leave the implementation of CreateRenderFrameInternal here to be diff-friendly. Should it be moved to the correct place (header-order-wise) in a following CL? https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1915: render_view_host->GetProcess()->GetID())); In the previous patch set I tried to centralize this logic along with the part that gives proper bindings to WebUI processes (that used to exist here and in 2 other places in this file). Moving the latter worked correctly but not this one above. It failed in a single test: RenderFrameHostManagerTest.ForceSwapAfterWebUIBindings. It would trigger this CHECK because when using process-per-tab a renderer process with WebUI bindings is allowed to navigate to an about://blank page. Is this really WAI? 2nd question: the code that i successfully calls GetProcess()->IsForGuestsOnly(). Here it calls that starting from the RVH but in the other 2 cases it calls it on the RFH. In my unified version I used the RFH as it seemed the OOPIF-correct way (even though the consequent allowing of bindings always happens on the RVH). Is that important at all and does it make sense?
Thanks! I really like the direction the patch is taking: the logic for WebUIs really seems clearer. There are a few things to iron out, please find my questions and comments below. https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1979: web_ui_->RenderViewCreated(render_view_host_); On 2015/09/30 19:37:27, carlosk wrote: > With this change the order of creation of the RVH and WebUI changed to this > order: first the RVH, then the WebUI. Consequentially I had to change where > WebUIImpl::RenderViewCreated is called from (used to be from the > WebContentsImpl). For this to work I trust that: > > 1) InitializeWebUI will only be called for brand new RFH instances; and > 2) If this is a new RFH instance for the root of the frame tree then its RVH > must also have been newly created. > > Similarly I moved the calls to WebUIImpl::RenderViewReused from the RFHM to > RFHI::UpdatePendingWebUI (further below). For this case the situation is more > straightforward: if a pending WebUI is being created it must be that the RFH is > being reused (and consequentially its RVH). > > WDYT about both of these? I think the assumptions make sense, provided: - they are well documented for callers of the function. - you add checks to make sure callers call these functions in the right way (do not trust them if possible :). https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:480: // bindings are not being set differently this time around. If it's not a history entry, what should the value of past_bindings be? https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:486: // reused. Should you also provide past bindings when navigating to a history entry? https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:821: scoped_ptr<WebUIImpl> web_ui_; Would it be possible to modify the WebUI so that it returns its type. It's a bit annoying to have to store those 2 parameters together. https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:830: // one will exist to be committed). Then, is it equivalent to !pending_web_ui_? https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1008: speculative_render_frame_host_->CommitPendingWebUI(); Is the CommitPendingWebUI method idempotent? (that would be nice, it would make the commit logic easier) https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1663: RenderFrameHostManager::CreateRenderFrameInternal(SiteInstance* instance, On 2015/09/30 19:37:27, carlosk wrote: > I chose to leave the implementation of CreateRenderFrameInternal here to be > diff-friendly. Should it be moved to the correct place (header-order-wise) in a > following CL? Acknowledged. https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:262: : render_frame_host_->pending_web_ui(); Does this work with a reused WebUI in the current RFH? https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:271: : render_frame_host_->pending_web_ui(); Same question here. https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:339: scoped_ptr<RenderFrameHostImpl> CreateRenderFrame(SiteInstance* instance, Is this methode public? If so, how do you ensure its caller will properly initialize the WebUI for the RFH?
Thanks! Still working on the failing tests. https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1979: web_ui_->RenderViewCreated(render_view_host_); On 2015/10/01 13:15:13, clamy wrote: > On 2015/09/30 19:37:27, carlosk wrote: > > With this change the order of creation of the RVH and WebUI changed to this > > order: first the RVH, then the WebUI. Consequentially I had to change where > > WebUIImpl::RenderViewCreated is called from (used to be from the > > WebContentsImpl). For this to work I trust that: > > > > 1) InitializeWebUI will only be called for brand new RFH instances; and > > 2) If this is a new RFH instance for the root of the frame tree then its RVH > > must also have been newly created. > > > > Similarly I moved the calls to WebUIImpl::RenderViewReused from the RFHM to > > RFHI::UpdatePendingWebUI (further below). For this case the situation is more > > straightforward: if a pending WebUI is being created it must be that the RFH > is > > being reused (and consequentially its RVH). > > > > WDYT about both of these? > > I think the assumptions make sense, provided: > - they are well documented for callers of the function. > - you add checks to make sure callers call these functions in the right way (do > not trust them if possible :). InitializeWebUI was removed and its logic moved to the RFHI constructor so I added comments about this there. Similarly I added comments to the UpdatePendingWebUI method. With the elimination of InitializeWebUI, 1) is not an issue anymore. But I don't know how could I CHECK for 2) nor for the UpdatePendingWebUI case. I would need to detect somehow if the RVH passed in is really new or if it's been already provided before to setup a WebUI. Creating an internal registry for that seems to be an overkill. https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:480: // bindings are not being set differently this time around. On 2015/10/01 13:15:13, clamy wrote: > If it's not a history entry, what should the value of past_bindings be? This method was removed. https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:486: // reused. On 2015/10/01 13:15:13, clamy wrote: > Should you also provide past bindings when navigating to a history entry? I improved all WebUI methods comments to make them clearer. https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:821: scoped_ptr<WebUIImpl> web_ui_; On 2015/10/01 13:15:13, clamy wrote: > Would it be possible to modify the WebUI so that it returns its type. It's a bit > annoying to have to store those 2 parameters together. I agree and it is possible. But nasko@ mentioning once that having GetType methods is discouraged and it would be needed in that case. So nasko@, WDYT? https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:830: // one will exist to be committed). On 2015/10/01 13:15:13, clamy wrote: > Then, is it equivalent to !pending_web_ui_? No because if this is false and the pending WebUI is nullptr, that will cause the current WebUI to be destroyed and none to be put in its place. https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1008: speculative_render_frame_host_->CommitPendingWebUI(); On 2015/10/01 13:15:13, clamy wrote: > Is the CommitPendingWebUI method idempotent? (that would be nice, it would make > the commit logic easier) I don't understand your point here. It's a commit operation so there is a change in internal state when a call happens. If a new call is made immediately after the state changes again. What is the behavior you are suggesting? https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:262: : render_frame_host_->pending_web_ui(); On 2015/10/01 13:15:13, clamy wrote: > Does this work with a reused WebUI in the current RFH? Yes, the logic for that is inside RFHI::pending_web_ui(). https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:271: : render_frame_host_->pending_web_ui(); On 2015/10/01 13:15:13, clamy wrote: > Same question here. Same answer here. :) https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:339: scoped_ptr<RenderFrameHostImpl> CreateRenderFrame(SiteInstance* instance, On 2015/10/01 13:15:13, clamy wrote: > Is this methode public? If so, how do you ensure its caller will properly > initialize the WebUI for the RFH? This is public and all existing use cases didn't initialize a WebUI. That's why I eliminated the parameter as it seemed to only be necessary for the private use cases which are now covered by CreateRenderFrameInternal.
I really like how RFHM is becoming simpler with this move. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:226: web_ui_->RenderViewCreated(render_view_host_); Urgh! This is ugly and we need to figure out why it is needed and make it better. Not in this CL, just general comment. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1985: new_web_ui_type != WebUI::kNoWebUI && web_ui_type_ == new_web_ui_type; This will be more readable if the boolean is initialized to false and an if statement with the checks turns it to true. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2327: web_ui = delegate_->CreateWebUIForRenderFrameHost(dest_url); Why do we need the delegate to create it. Can't we just create it here? https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:477: // |dest_url|. If it doesn't demand a Web, the pending will be cleared. If the "Web"? Did you mean "WebUI"? https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:478: // a new WebUI is required and its type matches an existing pending one it nit: s/an existing/the existing/ https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:480: // cleared. Otherwise a new WebUI instance is created and replaces the If there is no pending, there is nothing to replace, so let's say "and is set as the pending one." https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:483: // should be provided through |past_bindings|. It will allow verifying that nit: past implies that the history entry is prior to the current one. It could very well be that we are navigating forward in history. Let's find a more neutral wording/naming for this. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:486: // If a WebUI is created and this RenderFrameHost is live, Does that last sentence bring any information the caller of this method needs to know? It looks like an implementation detail to me, and if so, it shouldn't be here. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:494: // will be cleared. Overall the comment on a method should be describing in high level its purpose. Let's avoid implementation details and put those types of comments in the body of the method. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:497: // Destroys the pending WebUI and cleans up any related data. Ah! This is awesome comment! : ) https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:831: // is a Web UI page. Otherwise they will be nullptr and WebUI::kNoWebUI, nit: s/Web UI/WebUI/ https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1058: render_frame_host_->DiscardPendingWebUI(); Why are we touching the pending WebUI of the current RFH in PlzNavigate mode? Do we avoid creating a speculative RFH in case the active RFH already hosts WebUI page? https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1667: int bindings, I wonder if we can avoid creating this extra method and passing the URL and bindings. Can't we explicitly call the WebUI methods on the RFH once it is created? https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1902: WebUIImpl* dest_web_ui) { Why pass this as a parameter? Can't we just get it from the RFH? https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:270: ? speculative_render_frame_host_->web_ui() Speculative RFH will never have a pending_web_ui? https://codereview.chromium.org/1352813006/diff/180001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4331: scoped_ptr<WebUIImpl> WebContentsImpl::CreateWebUIForRenderFrameHost( I wonder why we still need this method. Can't we just do the same call in RFH itself?
Thanks! A few more comments. https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1979: web_ui_->RenderViewCreated(render_view_host_); On 2015/10/01 16:00:22, carlosk wrote: > On 2015/10/01 13:15:13, clamy wrote: > > On 2015/09/30 19:37:27, carlosk wrote: > > > With this change the order of creation of the RVH and WebUI changed to this > > > order: first the RVH, then the WebUI. Consequentially I had to change where > > > WebUIImpl::RenderViewCreated is called from (used to be from the > > > WebContentsImpl). For this to work I trust that: > > > > > > 1) InitializeWebUI will only be called for brand new RFH instances; and > > > 2) If this is a new RFH instance for the root of the frame tree then its RVH > > > must also have been newly created. > > > > > > Similarly I moved the calls to WebUIImpl::RenderViewReused from the RFHM to > > > RFHI::UpdatePendingWebUI (further below). For this case the situation is > more > > > straightforward: if a pending WebUI is being created it must be that the RFH > > is > > > being reused (and consequentially its RVH). > > > > > > WDYT about both of these? > > > > I think the assumptions make sense, provided: > > - they are well documented for callers of the function. > > - you add checks to make sure callers call these functions in the right way > (do > > not trust them if possible :). > > InitializeWebUI was removed and its logic moved to the RFHI constructor so I > added comments about this there. Similarly I added comments to the > UpdatePendingWebUI method. > > With the elimination of InitializeWebUI, 1) is not an issue anymore. But I don't > know how could I CHECK for 2) nor for the UpdatePendingWebUI case. I would need > to detect somehow if the RVH passed in is really new or if it's been already > provided before to setup a WebUI. Creating an internal registry for that seems > to be an overkill. Acknowledged. But this needs to be really well documented. https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1008: speculative_render_frame_host_->CommitPendingWebUI(); On 2015/10/01 16:00:22, carlosk wrote: > On 2015/10/01 13:15:13, clamy wrote: > > Is the CommitPendingWebUI method idempotent? (that would be nice, it would > make > > the commit logic easier) > > I don't understand your point here. It's a commit operation so there is a change > in internal state when a call happens. If a new call is made immediately after > the state changes again. > > What is the behavior you are suggesting? I was thinking of something along the lines of having CommitPendingWebUI be a CommitPendingWebUIIfNecesssary function. Inside RFH, instead of having should_reuse_web_ui, you'd have a has_pending_web_ui boolean (note that the pending WebUI can be nullptr). If !has_pending_web_ui, then CommitPendingWebUIIfNecesssary is a no-op, otherwise you replace the web_ui by whatever pending_web_ui including nullptr (and has_pending_web_ui becomes false again). This way, we can call CommitPendingWebUIIfNecesssary as many times as we want, without having to check if we have a pending_web_ui. I'm thinking this may make the code slightly simpler. Wdyt? https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:339: scoped_ptr<RenderFrameHostImpl> CreateRenderFrame(SiteInstance* instance, On 2015/10/01 16:00:22, carlosk wrote: > On 2015/10/01 13:15:13, clamy wrote: > > Is this methode public? If so, how do you ensure its caller will properly > > initialize the WebUI for the RFH? > > This is public and all existing use cases didn't initialize a WebUI. That's why > I eliminated the parameter as it seemed to only be necessary for the private use > cases which are now covered by CreateRenderFrameInternal. Acknowledged. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_delegate.h:167: // Creates a WebUI object for a main frame navigating to the given URL. If no Is this only called by main frames? Do subframes call something else/nothing? https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:494: // will be cleared. On 2015/10/01 20:05:16, nasko (slow to review) wrote: > Overall the comment on a method should be describing in high level its purpose. > Let's avoid implementation details and put those types of comments in the body > of the method. Also as mentioned in the other patchset, we may want to transform this method in a CommitPendingWebUIIfNecessary method, which would effectively hide these implementation details from the caller. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:833: scoped_ptr<WebUIImpl> web_ui_; @nasko: we were wondering about introducing a function on WebUIImpl to have it return its type. This would avoid having to store it here alongside the WebUI. Carlos mentioned you were opposed to a GetType method. Is that the case?
Thanks! Replied to all comments but there's still tests to fix and other questions I'll ask next. I will also ask specifics about the WebUI type issue to the owners. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_delegate.h:167: // Creates a WebUI object for a main frame navigating to the given URL. If no On 2015/10/02 14:20:01, clamy wrote: > Is this only called by main frames? Do subframes call something else/nothing? Gah! I missed updating this comment! Clarifying: this is called for any navigation that goes through the RFHM::UpdateStateForNavigate/GetFrameHostForNavigation related methods. IIUC they are used both for main frames and subframes hence the rename from CreateMainFrameWebUI. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:226: web_ui_->RenderViewCreated(render_view_host_); On 2015/10/01 20:05:16, nasko (slow to review) wrote: > Urgh! This is ugly and we need to figure out why it is needed and make it > better. Not in this CL, just general comment. Acknowledged. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1985: new_web_ui_type != WebUI::kNoWebUI && web_ui_type_ == new_web_ui_type; On 2015/10/01 20:05:16, nasko (slow to review) wrote: > This will be more readable if the boolean is initialized to false and an if > statement with the checks turns it to true. Done. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2327: web_ui = delegate_->CreateWebUIForRenderFrameHost(dest_url); On 2015/10/01 20:05:16, nasko (slow to review) wrote: > Why do we need the delegate to create it. Can't we just create it here? There is some code sharing for the creation logic in WebContentsImpl with another WebUI creation method (CreateSubframeWebUI) invoked from the UberUI WebUI. The latter doesn't seem to hold a reference to the RFH what would complicate moving all logic here. The WebUI constructor also needs a pointer to the WebContents (WebContentsImpl passes |this|) so the RFH might not be considered the right place architecturally to do that. But I'm not sure of this... So I'm thinking it should stay as is. WDYT? WebContentsImpl::CreateWebUI and its callers: https://goo.gl/86By1k https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:477: // |dest_url|. If it doesn't demand a Web, the pending will be cleared. If the On 2015/10/01 20:05:16, nasko (slow to review) wrote: > "Web"? Did you mean "WebUI"? Done. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:478: // a new WebUI is required and its type matches an existing pending one it On 2015/10/01 20:05:16, nasko (slow to review) wrote: > nit: s/an existing/the existing/ Done. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:480: // cleared. Otherwise a new WebUI instance is created and replaces the On 2015/10/01 20:05:16, nasko (slow to review) wrote: > If there is no pending, there is nothing to replace, so let's say "and is set as > the pending one." Done. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:483: // should be provided through |past_bindings|. It will allow verifying that On 2015/10/01 20:05:16, nasko (slow to review) wrote: > nit: past implies that the history entry is prior to the current one. It could > very well be that we are navigating forward in history. Let's find a more > neutral wording/naming for this. Changing to |entry_bindings| here and everywhere else. It works well both for an existing NavigationEntry with the "past" value and for a newly created which will be it set to kInvalidBindings. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:486: // If a WebUI is created and this RenderFrameHost is live, On 2015/10/01 20:05:16, nasko (slow to review) wrote: > Does that last sentence bring any information the caller of this method needs to > know? It looks like an implementation detail to me, and if so, it shouldn't be > here. Indeed. Moved it to inside the method implementation. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:494: // will be cleared. On 2015/10/02 14:20:01, clamy wrote: > On 2015/10/01 20:05:16, nasko (slow to review) wrote: > > Overall the comment on a method should be describing in high level its > purpose. > > Let's avoid implementation details and put those types of comments in the body > > of the method. > > Also as mentioned in the other patchset, we may want to transform this method in > a CommitPendingWebUIIfNecessary method, which would effectively hide these > implementation details from the caller. Updated the comment. As discussed offline the change to CommitPendingWebUIIfNecessary has its downsides as well so keeping this method as is. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:497: // Destroys the pending WebUI and cleans up any related data. On 2015/10/01 20:05:16, nasko (slow to review) wrote: > Ah! This is awesome comment! : ) Acknowledged. :) https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:831: // is a Web UI page. Otherwise they will be nullptr and WebUI::kNoWebUI, On 2015/10/01 20:05:16, nasko (slow to review) wrote: > nit: s/Web UI/WebUI/ Fixed this here and in a few other places. But there's many more throughout the files I touched. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:833: scoped_ptr<WebUIImpl> web_ui_; On 2015/10/02 14:20:01, clamy wrote: > @nasko: we were wondering about introducing a function on WebUIImpl to have it > return its type. This would avoid having to store it here alongside the WebUI. > Carlos mentioned you were opposed to a GetType method. Is that the case? We talked offline about this and for now I'll keep this part as is. I will contacting the owners of the WebUI dir and see what they think as there's some related logic that is quite weird at first look. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1058: render_frame_host_->DiscardPendingWebUI(); On 2015/10/01 20:05:16, nasko (slow to review) wrote: > Why are we touching the pending WebUI of the current RFH in PlzNavigate mode? Do > we avoid creating a speculative RFH in case the active RFH already hosts WebUI > page? For *both* navigation implementations, when the current RFH/SI is reused but it's WebUI has to change, a pending one will be created in it. There will be no pending/speculative RFH in this case. That section of the design doc I mentioned in the previous comment should make this clearer. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1667: int bindings, On 2015/10/01 20:05:16, nasko (slow to review) wrote: > I wonder if we can avoid creating this extra method and passing the URL and > bindings. Can't we explicitly call the WebUI methods on the RFH once it is > created? That's the solution I had before. PTAL at the discussion about it (https://codereview.chromium.org/1352813006/diff/1/content/browser/frame_host/...) and let me know WDYT. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1902: WebUIImpl* dest_web_ui) { On 2015/10/01 20:05:16, nasko (slow to review) wrote: > Why pass this as a parameter? Can't we just get it from the RFH? No, because the RFH is not yet set as current/pending/speculative at this point for some code paths and so we can't get the WebUI instance we need to act on here. For instance when this is called from CreateRenderFrameInternal; see the modifications there. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:270: ? speculative_render_frame_host_->web_ui() On 2015/10/01 20:05:16, nasko (slow to review) wrote: > Speculative RFH will never have a pending_web_ui? Never, ever! I added a section to the design doc that clearly describes this and other "main rules" of the new architecture: https://docs.google.com/document/d/1jrge5ulZ1YtqSR3WkNXzrtxhd3JXcWm8_zWBhEjaJ... PS: In fact, the really correct answer is: only momentarily. :) https://codereview.chromium.org/1352813006/diff/180001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4331: scoped_ptr<WebUIImpl> WebContentsImpl::CreateWebUIForRenderFrameHost( On 2015/10/01 20:05:16, nasko (slow to review) wrote: > I wonder why we still need this method. Can't we just do the same call in RFH > itself? Replied to this in another comment.
I fixed a couple of issues that made browser_tests finally pass. Still unit_tests to go... A few comments and new questions below. https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1915: render_view_host->GetProcess()->GetID())); On 2015/09/30 19:37:27, carlosk wrote: > In the previous patch set I tried to centralize this logic along with the part > that gives proper bindings to WebUI processes (that used to exist here and in 2 > other places in this file). Moving the latter worked correctly but not this one > above. It failed in a single test: > RenderFrameHostManagerTest.ForceSwapAfterWebUIBindings. It would trigger this > CHECK because when using process-per-tab a renderer process with WebUI bindings > is allowed to navigate to an about://blank page. Is this really WAI? > > 2nd question: the code that i successfully calls > GetProcess()->IsForGuestsOnly(). Here it calls that starting from the RVH but in > the other 2 cases it calls it on the RFH. In my unified version I used the RFH > as it seemed the OOPIF-correct way (even though the consequent allowing of > bindings always happens on the RVH). Is that important at all and does it make > sense? Bump on this: - The first part I already know the answer for: about://blank and a few others is allowed to use a render process with WebUI bindings. See WebUIControllerFactoryRegistry::IsURLAcceptableForWebUI. - The 2nd part is still a doubt I have even though things are working just fine (so it might not matter). https://codereview.chromium.org/1352813006/diff/240001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/240001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:198: pending_web_ui_type_(WebUI::kNoWebUI), This uninitialized variable was the cause of some of the test failures. https://codereview.chromium.org/1352813006/diff/240001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:239: pending_web_ui_type_ = WebUI::kNoWebUI; This is just for keeping consistency. https://codereview.chromium.org/1352813006/diff/240001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2020: frame_tree_node_->IsMainFrame()); This fixes more tests because it is expectedthe when reusing the current WebUI, RenderViewReused should be called on it.
https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:226: web_ui_->RenderViewCreated(render_view_host_); On 2015/10/01 20:05:16, nasko (slow to review) wrote: > Urgh! This is ugly and we need to figure out why it is needed and make it > better. Not in this CL, just general comment. this CL may make it irrelevant https://codereview.chromium.org/1352813006/diff/240001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/240001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2020: frame_tree_node_->IsMainFrame()); On 2015/10/06 17:03:21, carlosk wrote: > This fixes more tests because it is expectedthe when reusing the current WebUI, > RenderViewReused should be called on it. kinda glad i added those now. hopefully they haven't been a pain in your neck. fwiw: it doesn't really matter where we log things for the LogWebUIUrl() as long as those tests pass (i.e. visiting a page logs the URL, refreshing re-logs the URL, visiting the uber page logs both URLs, refreshing the uber page logs both URLs, navigating inside the uber page logs only the new frame URL)
I'm sorry for the delays, I should be back to more regular reviews at the end of this week. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:226: web_ui_->RenderViewCreated(render_view_host_); On 2015/10/06 17:50:33, Dan Beam wrote: > On 2015/10/01 20:05:16, nasko (slow to review) wrote: > > Urgh! This is ugly and we need to figure out why it is needed and make it > > better. Not in this CL, just general comment. > > this CL may make it irrelevant Care to add some context why it will be irrelevant? I'm all for making it so, but want to understand what is its purpose and why this CL will help. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2327: web_ui = delegate_->CreateWebUIForRenderFrameHost(dest_url); On 2015/10/05 16:59:57, carlosk wrote: > On 2015/10/01 20:05:16, nasko (slow to review) wrote: > > Why do we need the delegate to create it. Can't we just create it here? > > There is some code sharing for the creation logic in WebContentsImpl with > another WebUI creation method (CreateSubframeWebUI) invoked from the UberUI > WebUI. The latter doesn't seem to hold a reference to the RFH what would > complicate moving all logic here. > > The WebUI constructor also needs a pointer to the WebContents (WebContentsImpl > passes |this|) so the RFH might not be considered the right place > architecturally to do that. But I'm not sure of this... > > So I'm thinking it should stay as is. WDYT? Ah, I missed that. Yeah, passing WebContents as a parameter to WebUI creation means that it stays in WebContents. > WebContentsImpl::CreateWebUI and its callers: https://goo.gl/86By1k https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1667: int bindings, On 2015/10/05 16:59:58, carlosk wrote: > On 2015/10/01 20:05:16, nasko (slow to review) wrote: > > I wonder if we can avoid creating this extra method and passing the URL and > > bindings. Can't we explicitly call the WebUI methods on the RFH once it is > > created? > > That's the solution I had before. PTAL at the discussion about it > (https://codereview.chromium.org/1352813006/diff/1/content/browser/frame_host/...) > and let me know WDYT. The argument that we always have to call the extra method is valid. I'm not excited about the internal method either. It might be useful to ping creis@ about this and see his plans on session history and how the URL/bindings will be stored. Ideally, we should have one method and not needing the indirection to Internal, but I haven't looked at all the callers. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1902: WebUIImpl* dest_web_ui) { On 2015/10/05 16:59:58, carlosk wrote: > On 2015/10/01 20:05:16, nasko (slow to review) wrote: > > Why pass this as a parameter? Can't we just get it from the RFH? > > No, because the RFH is not yet set as current/pending/speculative at this point > for some code paths and so we can't get the WebUI instance we need to act on > here. For instance when this is called from CreateRenderFrameInternal; see the > modifications there. I think we can do better, but let's revisit this in a later CL as things get simplified. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:270: ? speculative_render_frame_host_->web_ui() On 2015/10/05 16:59:58, carlosk wrote: > On 2015/10/01 20:05:16, nasko (slow to review) wrote: > > Speculative RFH will never have a pending_web_ui? > > Never, ever! Are we enforcing this with a CHECK somehow? > I added a section to the design doc that clearly describes this and other "main > rules" of the new architecture: > https://docs.google.com/document/d/1jrge5ulZ1YtqSR3WkNXzrtxhd3JXcWm8_zWBhEjaJ... > > PS: In fact, the really correct answer is: only momentarily. :) https://codereview.chromium.org/1352813006/diff/180001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4331: scoped_ptr<WebUIImpl> WebContentsImpl::CreateWebUIForRenderFrameHost( On 2015/10/05 16:59:58, carlosk wrote: > On 2015/10/01 20:05:16, nasko (slow to review) wrote: > > I wonder why we still need this method. Can't we just do the same call in RFH > > itself? > > Replied to this in another comment. Acknowledged. https://codereview.chromium.org/1352813006/diff/220001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:226: // If a WebUI is created and this RenderFrameHost is for the main frame, it is nit: Empty line between code and comment lines. https://codereview.chromium.org/1352813006/diff/240001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/240001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:228: // assumed that the associated RenderViewHost has also just been created and I'm not sure this is a safe assumption to make. The RenderViewHost can be reused if it was kept alive due to scripting relationships between tabs. https://codereview.chromium.org/1352813006/diff/240001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1352813006/diff/240001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:481: // provided |dest_url|. If it doesn't demand a WebUI, the pending one is nit: s/demand/require/ https://codereview.chromium.org/1352813006/diff/240001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:492: // Updates the active WebUI with the pending data set by the last call to nit: s/data/one/
https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:226: web_ui_->RenderViewCreated(render_view_host_); On 2015/10/07 23:49:32, nasko (slow to review) wrote: > On 2015/10/06 17:50:33, Dan Beam wrote: > > On 2015/10/01 20:05:16, nasko (slow to review) wrote: > > > Urgh! This is ugly and we need to figure out why it is needed and make it > > > better. Not in this CL, just general comment. > > > > this CL may make it irrelevant > > Care to add some context why it will be irrelevant? I'm all for making it so, > but want to understand what is its purpose and why this CL will help. I originally thought this was solely to support subframe webUIs but no, this'll continue to be necessary as handlers are created before renderframes (sorry!)
https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:226: web_ui_->RenderViewCreated(render_view_host_); On 2015/10/07 23:55:11, Dan Beam wrote: > On 2015/10/07 23:49:32, nasko (slow to review) wrote: > > On 2015/10/06 17:50:33, Dan Beam wrote: > > > On 2015/10/01 20:05:16, nasko (slow to review) wrote: > > > > Urgh! This is ugly and we need to figure out why it is needed and make it > > > > better. Not in this CL, just general comment. > > > > > > this CL may make it irrelevant > > > > Care to add some context why it will be irrelevant? I'm all for making it so, > > but want to understand what is its purpose and why this CL will help. > > I originally thought this was solely to support subframe webUIs but no, this'll > continue to be necessary as handlers are created before renderframes (sorry!) if handlers are created before renderframes**
Thanks for all comments. dbeam@ and creis@: there's a questions for you in these comments if you could PTAL. I uploaded a new patch that fixed the 4 failing unit_tests but in the meantime something else changed and now I have crashes on the renderer when running RenderFrameHostManagerTest.WebUIGetsBindings (from content_browsertests) and maybe more. So one more thing to investigate next week... :/ https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:226: web_ui_->RenderViewCreated(render_view_host_); On 2015/10/08 00:01:35, Dan Beam wrote: > On 2015/10/07 23:55:11, Dan Beam wrote: > > On 2015/10/07 23:49:32, nasko (slow to review) wrote: > > > On 2015/10/06 17:50:33, Dan Beam wrote: > > > > On 2015/10/01 20:05:16, nasko (slow to review) wrote: > > > > > Urgh! This is ugly and we need to figure out why it is needed and make > it > > > > > better. Not in this CL, just general comment. > > > > > > > > this CL may make it irrelevant > > > > > > Care to add some context why it will be irrelevant? I'm all for making it > so, > > > but want to understand what is its purpose and why this CL will help. > > > > I originally thought this was solely to support subframe webUIs but no, > this'll > > continue to be necessary as handlers are created before renderframes (sorry!) > > if handlers are created before renderframes** OK, so I'm keeping them. But it's still not clear to me when exactly to call RenderViewCreated versus RenderViewReused. Are my if-expressions here and in UpdatePendingWebUI enough? For instance, @nasko mentioned an existing RenderView could be reused for a new RFHI being created for the main frame. With the this implementation that would cause RenderViewCreated to be invoked. Would that be a problem? @dbeam, can you shed some light on this? https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1667: int bindings, On 2015/10/07 23:49:32, nasko (slow to review) wrote: > On 2015/10/05 16:59:58, carlosk wrote: > > On 2015/10/01 20:05:16, nasko (slow to review) wrote: > > > I wonder if we can avoid creating this extra method and passing the URL and > > > bindings. Can't we explicitly call the WebUI methods on the RFH once it is > > > created? > > > > That's the solution I had before. PTAL at the discussion about it > > > (https://codereview.chromium.org/1352813006/diff/1/content/browser/frame_host/...) > > and let me know WDYT. > > The argument that we always have to call the extra method is valid. I'm not > excited about the internal method either. It might be useful to ping creis@ > about this and see his plans on session history and how the URL/bindings will be > stored. Ideally, we should have one method and not needing the indirection to > Internal, but I haven't looked at all the callers. OK, I'll ping cresi@. Passing the extra parameters is the cost for eliminating InitializeWebUI which was called in all RFH constructions. It seemed the right thing to do (but this might change depending on dbeam@'s answer on the RenderViewCreated|Reused logic). But the new internal method was added to simplify the call for current external callers who would never try to create a WebUI anyway. Another option is to revert to a single public method and make callers pass default values for URL and bindings. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1902: WebUIImpl* dest_web_ui) { On 2015/10/07 23:49:32, nasko (slow to review) wrote: > On 2015/10/05 16:59:58, carlosk wrote: > > On 2015/10/01 20:05:16, nasko (slow to review) wrote: > > > Why pass this as a parameter? Can't we just get it from the RFH? > > > > No, because the RFH is not yet set as current/pending/speculative at this > point > > for some code paths and so we can't get the WebUI instance we need to act on > > here. For instance when this is called from CreateRenderFrameInternal; see the > > modifications there. > > I think we can do better, but let's revisit this in a later CL as things get > simplified. Acknowledged. Let's talk and figure out what can be done. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:270: ? speculative_render_frame_host_->web_ui() On 2015/10/07 23:49:33, nasko (slow to review) wrote: > On 2015/10/05 16:59:58, carlosk wrote: > > On 2015/10/01 20:05:16, nasko (slow to review) wrote: > > > Speculative RFH will never have a pending_web_ui? > > > > Never, ever! > > Are we enforcing this with a CHECK somehow? > > > I added a section to the design doc that clearly describes this and other > "main > > rules" of the new architecture: > > > https://docs.google.com/document/d/1jrge5ulZ1YtqSR3WkNXzrtxhd3JXcWm8_zWBhEjaJ... > > > > PS: In fact, the really correct answer is: only momentarily. :) There were some but I added a few more. https://codereview.chromium.org/1352813006/diff/220001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:226: // If a WebUI is created and this RenderFrameHost is for the main frame, it is On 2015/10/07 23:49:33, nasko (slow to review) wrote: > nit: Empty line between code and comment lines. Done here and a few more places. https://codereview.chromium.org/1352813006/diff/240001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/240001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:228: // assumed that the associated RenderViewHost has also just been created and On 2015/10/07 23:49:33, nasko (slow to review) wrote: > I'm not sure this is a safe assumption to make. The RenderViewHost can be reused > if it was kept alive due to scripting relationships between tabs. I'm asking about this to dbeam@ as the rules for when to call RenderViewCreated and RenderViewReused are not clear to me. If this should not be adequate I might need to move these calls back to the RFHM. https://codereview.chromium.org/1352813006/diff/240001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2020: frame_tree_node_->IsMainFrame()); On 2015/10/06 17:50:33, Dan Beam wrote: > On 2015/10/06 17:03:21, carlosk wrote: > > This fixes more tests because it is expectedthe when reusing the current > WebUI, > > RenderViewReused should be called on it. > > kinda glad i added those now. > > hopefully they haven't been a pain in your neck. > > fwiw: it doesn't really matter where we log things for the LogWebUIUrl() as long > as those tests pass (i.e. visiting a page logs the URL, refreshing re-logs the > URL, visiting the uber page logs both URLs, refreshing the uber page logs both > URLs, navigating inside the uber page logs only the new frame URL) Acknowledged and thanks for adding them! https://codereview.chromium.org/1352813006/diff/240001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1352813006/diff/240001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:481: // provided |dest_url|. If it doesn't demand a WebUI, the pending one is On 2015/10/07 23:49:33, nasko (slow to review) wrote: > nit: s/demand/require/ Done. https://codereview.chromium.org/1352813006/diff/240001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:492: // Updates the active WebUI with the pending data set by the last call to On 2015/10/07 23:49:33, nasko (slow to review) wrote: > nit: s/data/one/ Done.
whenever I see var_name = ComplexType(); var_name_extra = p_o_d; I think "why doesn't ComplexType::extra_ exist?" so, in that spirit: why do we constantly keep WebUI::TypeID and WebUI separate? https://codereview.chromium.org/1352813006/diff/260001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2023: CHECK_IMPLIES(!pending_web_ui_, pending_web_ui_type_ == WebUI::kNoWebUI); why can't this just be: CHECK_EQ(!pending_web_ui_, pending_web_ui_type_ == WebUI::kNoWebUI); https://codereview.chromium.org/1352813006/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2366: if (web_ui && entry_bindings != NavigationEntryImpl::kInvalidBindings && why if (web_ui) after CHECK(web_ui)? https://codereview.chromium.org/1352813006/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2370: *web_ui_type = WebUI::kNoWebUI; why not return web_ui.Pass(); here (or nullptr)? https://codereview.chromium.org/1352813006/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2381: render_view_host_->AllowBindings(new_bindings); can this be? render_view_host_->AllowBindings(web_ui->GetBindings()); https://codereview.chromium.org/1352813006/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2386: CHECK_IMPLIES(!web_ui, *web_ui_type == WebUI::kNoWebUI); same nit re: CHECK_EQ() instead
Thanks! On 2015/10/10 00:02:08, Dan Beam wrote: > whenever I see > > var_name = ComplexType(); > var_name_extra = p_o_d; > > I think "why doesn't ComplexType::extra_ exist?" > > so, in that spirit: why do we constantly keep WebUI::TypeID and WebUI separate? I am considering changing it. I started a discussion by email with the owners of WebUI about a week ago and they included you in the thread. It should be waiting for you in your inbox. But the specific question I mentioned you is here: https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... Also to assist creis@ finding his, it is here: https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... I noticed I referred to your respective LDAPs incorrectly in both questions. I'm sorry about that. https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1667: int bindings, On 2015/10/09 17:29:10, carlosk wrote: > On 2015/10/07 23:49:32, nasko (slow to review) wrote: > > On 2015/10/05 16:59:58, carlosk wrote: > > > On 2015/10/01 20:05:16, nasko (slow to review) wrote: > > > > I wonder if we can avoid creating this extra method and passing the URL > and > > > > bindings. Can't we explicitly call the WebUI methods on the RFH once it is > > > > created? > > > > > > That's the solution I had before. PTAL at the discussion about it > > > > > > (https://codereview.chromium.org/1352813006/diff/1/content/browser/frame_host/...) > > > and let me know WDYT. > > > > The argument that we always have to call the extra method is valid. I'm not > > excited about the internal method either. It might be useful to ping creis@ > > about this and see his plans on session history and how the URL/bindings will > be > > stored. Ideally, we should have one method and not needing the indirection to > > Internal, but I haven't looked at all the callers. > > OK, I'll ping cresi@. > > Passing the extra parameters is the cost for eliminating InitializeWebUI which > was called in all RFH constructions. It seemed the right thing to do (but this > might change depending on dbeam@'s answer on the RenderViewCreated|Reused > logic). > > But the new internal method was added to simplify the call for current external > callers who would never try to create a WebUI anyway. Another option is to > revert to a single public method and make callers pass default values for URL > and bindings. After a VC with Nasko I reverted to having the RFHI::InitializeWebUI and eliminated the two extra creation parameters. I am still keeping the CreateRenderFrameInternal because the public version doesn't need to pass those parameters (none of the current public use cases needs to create a WebUI). https://codereview.chromium.org/1352813006/diff/260001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2023: CHECK_IMPLIES(!pending_web_ui_, pending_web_ui_type_ == WebUI::kNoWebUI); On 2015/10/10 00:02:07, Dan Beam wrote: > why can't this just be: > > CHECK_EQ(!pending_web_ui_, pending_web_ui_type_ == WebUI::kNoWebUI); Done. I used that form because it seemed easier to follow. Also this one requires one to be attentive to compare proper boolean values. In this case of pointer vs boolean the compiler will scream if that's not properly done but if the first argument was an int it would require proper conversion. https://codereview.chromium.org/1352813006/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2366: if (web_ui && entry_bindings != NavigationEntryImpl::kInvalidBindings && On 2015/10/10 00:02:08, Dan Beam wrote: > why if (web_ui) after CHECK(web_ui)? Done. https://codereview.chromium.org/1352813006/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2370: *web_ui_type = WebUI::kNoWebUI; On 2015/10/10 00:02:07, Dan Beam wrote: > why not return web_ui.Pass(); here (or nullptr)? Done. https://codereview.chromium.org/1352813006/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2381: render_view_host_->AllowBindings(new_bindings); On 2015/10/10 00:02:07, Dan Beam wrote: > can this be? > > render_view_host_->AllowBindings(web_ui->GetBindings()); I'm assuming you are asking about replacing the code from 2378 to 2382 with that single line, right? This code here unification of 3 similar sections that were extracted from RFHM. Two of them had only the single line format as you suggested and one had this if-check around it. I chose to use the latter as looking at the implementation of AllowBindings it sends an IPC and doesn't compare the values being set to the current ones, so this if-check can actually avoid that unneeded IPC. https://codereview.chromium.org/1352813006/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2386: CHECK_IMPLIES(!web_ui, *web_ui_type == WebUI::kNoWebUI); On 2015/10/10 00:02:07, Dan Beam wrote: > same nit re: CHECK_EQ() instead Done.
https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:226: web_ui_->RenderViewCreated(render_view_host_); On 2015/10/09 17:29:10, carlosk wrote: > On 2015/10/08 00:01:35, Dan Beam wrote: > > On 2015/10/07 23:55:11, Dan Beam wrote: > > > On 2015/10/07 23:49:32, nasko (slow to review) wrote: > > > > On 2015/10/06 17:50:33, Dan Beam wrote: > > > > > On 2015/10/01 20:05:16, nasko (slow to review) wrote: > > > > > > Urgh! This is ugly and we need to figure out why it is needed and make > > it > > > > > > better. Not in this CL, just general comment. > > > > > > > > > > this CL may make it irrelevant > > > > > > > > Care to add some context why it will be irrelevant? I'm all for making it > > so, > > > > but want to understand what is its purpose and why this CL will help. > > > > > > I originally thought this was solely to support subframe webUIs but no, > > this'll > > > continue to be necessary as handlers are created before renderframes > (sorry!) > > > > if handlers are created before renderframes** > > OK, so I'm keeping them. But it's still not clear to me when exactly to call > RenderViewCreated versus RenderViewReused. Are my if-expressions here and in > UpdatePendingWebUI enough? > > For instance, @nasko mentioned an existing RenderView could be reused for a new > RFHI being created for the main frame. With the this implementation that would > cause RenderViewCreated to be invoked. Would that be a problem? > > @dbeam, can you shed some light on this? RenderViewCreated() is useful as the WebUI exists before the RenderView. when the RenderView comes up it may require some initial state to be sent to it (i.e. here's your bookmarks!). on refresh (or same-origin navigation?), the same initial state may be required to be sent even though the C++ browser-side handler was never torn down/recreated. /cc estade@ as he added this performance optimization: http://codereview.chromium.org/10154004 As long as the C++ handlers don't get torn down and rebuilt too often, it doesn't matter to me how we accomplish this behavior (i.e. if we just call RenderFrameCreated each time rather than RenderFrameReused).
https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1667: int bindings, On 2015/10/13 14:40:26, carlosk wrote: > On 2015/10/09 17:29:10, carlosk wrote: > > On 2015/10/07 23:49:32, nasko (slow to review) wrote: > > > On 2015/10/05 16:59:58, carlosk wrote: > > > > On 2015/10/01 20:05:16, nasko (slow to review) wrote: > > > > > I wonder if we can avoid creating this extra method and passing the URL > > and > > > > > bindings. Can't we explicitly call the WebUI methods on the RFH once it > is > > > > > created? > > > > > > > > That's the solution I had before. PTAL at the discussion about it > > > > > > > > > > (https://codereview.chromium.org/1352813006/diff/1/content/browser/frame_host/...) > > > > and let me know WDYT. > > > > > > The argument that we always have to call the extra method is valid. I'm not > > > excited about the internal method either. It might be useful to ping creis@ > > > about this and see his plans on session history and how the URL/bindings > will > > be > > > stored. Ideally, we should have one method and not needing the indirection > to > > > Internal, but I haven't looked at all the callers. > > > > OK, I'll ping cresi@. > > > > Passing the extra parameters is the cost for eliminating InitializeWebUI which > > was called in all RFH constructions. It seemed the right thing to do (but this > > might change depending on dbeam@'s answer on the RenderViewCreated|Reused > > logic). > > > > But the new internal method was added to simplify the call for current > external > > callers who would never try to create a WebUI anyway. Another option is to > > revert to a single public method and make callers pass default values for URL > > and bindings. > > After a VC with Nasko I reverted to having the RFHI::InitializeWebUI and > eliminated the two extra creation parameters. > > I am still keeping the CreateRenderFrameInternal because the public version > doesn't need to pass those parameters (none of the current public use cases > needs to create a WebUI). I chatted with Nasko to get the context, and it sounds like you two have a direction forward. We agreed I should hold off on the review for now, to avoid having too many cooks in the kitchen. :)
Thanks. On 2015/10/13 16:24:21, Dan Beam wrote: > https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... > File content/browser/frame_host/render_frame_host_impl.cc (right): > > https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... > content/browser/frame_host/render_frame_host_impl.cc:226: > web_ui_->RenderViewCreated(render_view_host_); > On 2015/10/09 17:29:10, carlosk wrote: > > On 2015/10/08 00:01:35, Dan Beam wrote: > > > On 2015/10/07 23:55:11, Dan Beam wrote: > > > > On 2015/10/07 23:49:32, nasko (slow to review) wrote: > > > > > On 2015/10/06 17:50:33, Dan Beam wrote: > > > > > > On 2015/10/01 20:05:16, nasko (slow to review) wrote: > > > > > > > Urgh! This is ugly and we need to figure out why it is needed and > make > > > it > > > > > > > better. Not in this CL, just general comment. > > > > > > > > > > > > this CL may make it irrelevant > > > > > > > > > > Care to add some context why it will be irrelevant? I'm all for making > it > > > so, > > > > > but want to understand what is its purpose and why this CL will help. > > > > > > > > I originally thought this was solely to support subframe webUIs but no, > > > this'll > > > > continue to be necessary as handlers are created before renderframes > > (sorry!) > > > > > > if handlers are created before renderframes** > > > > OK, so I'm keeping them. But it's still not clear to me when exactly to call > > RenderViewCreated versus RenderViewReused. Are my if-expressions here and in > > UpdatePendingWebUI enough? > > > > For instance, @nasko mentioned an existing RenderView could be reused for a > new > > RFHI being created for the main frame. With the this implementation that would > > cause RenderViewCreated to be invoked. Would that be a problem? > > > > @dbeam, can you shed some light on this? > > RenderViewCreated() is useful as the WebUI exists before the RenderView. when > the RenderView comes up it may require some initial state to be sent to it (i.e. > here's your bookmarks!). This is not true anymore after this change lands: as the WebUI is created by and after the RFHI is created, which is created after the RenderView. How does this affects these methods? > on refresh (or same-origin navigation?), the same initial state may be required > to be sent even though the C++ browser-side handler was never torn > down/recreated. This is what I need to know precisely what the rules are, otherwise I might call the wrong method for the situation. Is it only when there is simply a reload (same WebUI instance will be kept for the same RFHI/RenderView) or is it also used when the WebUI changes but the RenderView is kept the same? What are you referring to as handler? > /cc estade@ as he added this performance optimization: > http://codereview.chromium.org/10154004 I'll wait for his feedback as well. > As long as the C++ handlers don't get torn down and rebuilt too often, it > doesn't matter to me how we accomplish this behavior (i.e. if we just call > RenderFrameCreated each time rather than RenderFrameReused). So if I understood this correctly: - RenderFrameReused is just an optimization. - RenderFrameCreated is a superset of RenderFrameReused and it could always be called instead of the latter Are these correct? On 2015/10/13 17:03:27, Charlie Reis wrote: > https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... > File content/browser/frame_host/render_frame_host_manager.cc (right): > > https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... > content/browser/frame_host/render_frame_host_manager.cc:1667: int bindings, > On 2015/10/13 14:40:26, carlosk wrote: > > On 2015/10/09 17:29:10, carlosk wrote: > > > On 2015/10/07 23:49:32, nasko (slow to review) wrote: > > > > On 2015/10/05 16:59:58, carlosk wrote: > > > > > On 2015/10/01 20:05:16, nasko (slow to review) wrote: > > > > > > I wonder if we can avoid creating this extra method and passing the > URL > > > and > > > > > > bindings. Can't we explicitly call the WebUI methods on the RFH once > it > > is > > > > > > created? > > > > > > > > > > That's the solution I had before. PTAL at the discussion about it > > > > > > > > > > > > > > > (https://codereview.chromium.org/1352813006/diff/1/content/browser/frame_host/...) > > > > > and let me know WDYT. > > > > > > > > The argument that we always have to call the extra method is valid. I'm > not > > > > excited about the internal method either. It might be useful to ping > creis@ > > > > about this and see his plans on session history and how the URL/bindings > > will > > > be > > > > stored. Ideally, we should have one method and not needing the indirection > > to > > > > Internal, but I haven't looked at all the callers. > > > > > > OK, I'll ping cresi@. > > > > > > Passing the extra parameters is the cost for eliminating InitializeWebUI > which > > > was called in all RFH constructions. It seemed the right thing to do (but > this > > > might change depending on dbeam@'s answer on the RenderViewCreated|Reused > > > logic). > > > > > > But the new internal method was added to simplify the call for current > > external > > > callers who would never try to create a WebUI anyway. Another option is to > > > revert to a single public method and make callers pass default values for > URL > > > and bindings. > > > > After a VC with Nasko I reverted to having the RFHI::InitializeWebUI and > > eliminated the two extra creation parameters. > > > > I am still keeping the CreateRenderFrameInternal because the public version > > doesn't need to pass those parameters (none of the current public use cases > > needs to create a WebUI). > > I chatted with Nasko to get the context, and it sounds like you two have a > direction forward. We agreed I should hold off on the review for now, to avoid > having too many cooks in the kitchen. :) SGTM. I'll wait on Nasko's answer then.
On 2015/10/13 17:35:06, carlosk wrote: > Thanks. > > On 2015/10/13 16:24:21, Dan Beam wrote: > > > https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... > > File content/browser/frame_host/render_frame_host_impl.cc (right): > > > > > https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... > > content/browser/frame_host/render_frame_host_impl.cc:226: > > web_ui_->RenderViewCreated(render_view_host_); > > On 2015/10/09 17:29:10, carlosk wrote: > > > On 2015/10/08 00:01:35, Dan Beam wrote: > > > > On 2015/10/07 23:55:11, Dan Beam wrote: > > > > > On 2015/10/07 23:49:32, nasko (slow to review) wrote: > > > > > > On 2015/10/06 17:50:33, Dan Beam wrote: > > > > > > > On 2015/10/01 20:05:16, nasko (slow to review) wrote: > > > > > > > > Urgh! This is ugly and we need to figure out why it is needed and > > make > > > > it > > > > > > > > better. Not in this CL, just general comment. > > > > > > > > > > > > > > this CL may make it irrelevant > > > > > > > > > > > > Care to add some context why it will be irrelevant? I'm all for making > > it > > > > so, > > > > > > but want to understand what is its purpose and why this CL will help. > > > > > > > > > > I originally thought this was solely to support subframe webUIs but no, > > > > this'll > > > > > continue to be necessary as handlers are created before renderframes > > > (sorry!) > > > > > > > > if handlers are created before renderframes** > > > > > > OK, so I'm keeping them. But it's still not clear to me when exactly to call > > > RenderViewCreated versus RenderViewReused. Are my if-expressions here and in > > > UpdatePendingWebUI enough? > > > > > > For instance, @nasko mentioned an existing RenderView could be reused for a > > new > > > RFHI being created for the main frame. With the this implementation that > would > > > cause RenderViewCreated to be invoked. Would that be a problem? > > > > > > @dbeam, can you shed some light on this? > > > > RenderViewCreated() is useful as the WebUI exists before the RenderView. when > > the RenderView comes up it may require some initial state to be sent to it > (i.e. > > here's your bookmarks!). > > This is not true anymore after this change lands: as the WebUI is created by and > after the RFHI is created, which is created after the RenderView. How does this > affects these methods? > > > on refresh (or same-origin navigation?), the same initial state may be > required > > to be sent even though the C++ browser-side handler was never torn > > down/recreated. > > This is what I need to know precisely what the rules are, otherwise I might call > the wrong method for the situation. Is it only when there is simply a reload > (same WebUI instance will be kept for the same RFHI/RenderView) or is it also > used when the WebUI changes but the RenderView is kept the same? read the CL description I linked you to: http://codereview.chromium.org/10154004 > > What are you referring to as handler? WebUIs and WebUIMessageHandlers > > > /cc estade@ as he added this performance optimization: > > http://codereview.chromium.org/10154004 > > I'll wait for his feedback as well. > > > As long as the C++ handlers don't get torn down and rebuilt too often, it > > doesn't matter to me how we accomplish this behavior (i.e. if we just call > > RenderFrameCreated each time rather than RenderFrameReused). > > So if I understood this correctly: > - RenderFrameReused is just an optimization. > - RenderFrameCreated is a superset of RenderFrameReused and it could always be > called instead of the latter > > Are these correct? > > On 2015/10/13 17:03:27, Charlie Reis wrote: > > > https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... > > File content/browser/frame_host/render_frame_host_manager.cc (right): > > > > > https://codereview.chromium.org/1352813006/diff/180001/content/browser/frame_... > > content/browser/frame_host/render_frame_host_manager.cc:1667: int bindings, > > On 2015/10/13 14:40:26, carlosk wrote: > > > On 2015/10/09 17:29:10, carlosk wrote: > > > > On 2015/10/07 23:49:32, nasko (slow to review) wrote: > > > > > On 2015/10/05 16:59:58, carlosk wrote: > > > > > > On 2015/10/01 20:05:16, nasko (slow to review) wrote: > > > > > > > I wonder if we can avoid creating this extra method and passing the > > URL > > > > and > > > > > > > bindings. Can't we explicitly call the WebUI methods on the RFH once > > it > > > is > > > > > > > created? > > > > > > > > > > > > That's the solution I had before. PTAL at the discussion about it > > > > > > > > > > > > > > > > > > > > > (https://codereview.chromium.org/1352813006/diff/1/content/browser/frame_host/...) > > > > > > and let me know WDYT. > > > > > > > > > > The argument that we always have to call the extra method is valid. I'm > > not > > > > > excited about the internal method either. It might be useful to ping > > creis@ > > > > > about this and see his plans on session history and how the URL/bindings > > > will > > > > be > > > > > stored. Ideally, we should have one method and not needing the > indirection > > > to > > > > > Internal, but I haven't looked at all the callers. > > > > > > > > OK, I'll ping cresi@. > > > > > > > > Passing the extra parameters is the cost for eliminating InitializeWebUI > > which > > > > was called in all RFH constructions. It seemed the right thing to do (but > > this > > > > might change depending on dbeam@'s answer on the RenderViewCreated|Reused > > > > logic). > > > > > > > > But the new internal method was added to simplify the call for current > > > external > > > > callers who would never try to create a WebUI anyway. Another option is to > > > > revert to a single public method and make callers pass default values for > > URL > > > > and bindings. > > > > > > After a VC with Nasko I reverted to having the RFHI::InitializeWebUI and > > > eliminated the two extra creation parameters. > > > > > > I am still keeping the CreateRenderFrameInternal because the public version > > > doesn't need to pass those parameters (none of the current public use cases > > > needs to create a WebUI). > > > > I chatted with Nasko to get the context, and it sounds like you two have a > > direction forward. We agreed I should hold off on the review for now, to > avoid > > having too many cooks in the kitchen. :) > > SGTM. I'll wait on Nasko's answer then.
https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:480: // Initializes the first WebUI for a recently created RenderFrameHost, if one Does the "recently created" bit matter? https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:492: // pending WebUI instance is set. As before, this comment seems to describe the implementation. Can you abstract it such that it is less verbose? https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:666: // NavigationEntryImpl::kInvalidBindings). If no WebUI applies, returns Again, no need to include the text in (). This is implementation detail. https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:840: // The associated WebUIImpl and its type. They will be set if the current page This is RenderFrameHost, it cannot have a page : ). Let's use "the current document is from WebUI source." https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1705: new_instance, url, bindings, create_render_frame_flags, nullptr); Why can't this be CreateRenderFrame, then call to InitializeWebUI? https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1760: new_render_frame_host->CommitPendingWebUI(); This is only done in case of reusing existing RFH. https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1786: new_render_frame_host->InitializeWebUI(url, bindings); This is done only on new RFHs. Combined with the comment above, it means that we can potentially have an output parameter from this method and call either Update&Commit or Initialize once this method returns. This will kill off the url and bindings parameter, which I really don't like to have. Also, as I'm very close to swapped out removal, once it is complete, the other branch will disappear entirely, so we will know that we always have to call Initialize after this method and the output parameter can disappear and get back to much simpler code. https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1984: dest_web_ui->RenderViewCreated(render_view_host); Why can't we move this call to right after we've created the RenderFrame and initialized the WebUI? Also, looking over the usage of this in WebUI, it seems that eventually this needs to move to RenderFrameCreated, which makes it even more suitable for moving out of here. https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (left): https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:365: WebUIImpl* web_ui, /me likes this parameter being gone! : ) https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:256: // Returns the current committed WebUI or NULL if none applies. nit: Let's update comments as we go. s/NULL/null/ or s/NULL/nullptr/, whichever you prefer. https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:75: if (!(should_create_webui_ && HasWebUIScheme(url))) Why not invert the code such that you don't have ! and return the casted this, otherwise return kNoWebUI. Also, a comment on why this is needed will be good to have. As I read through this, I'm not quite sure why this is done.
Thanks! Based on nasko@'s latest comments I made some simplifications regarding the moment the WebUI is setup and when its RenderView* notification methods are called. The main assumption made was that WebUIImpl::RenderViewCreated could be used when the RFH is newly created (or swapped back in), even if the RenderView already existed before. It seemed to be OK to do by looking at the current implementations of that method by WebUIController sub-classes and it makes sense as the new WebUI might not yet have "interacted" with that RenderView before. Also this is another step forward moving from RVH to RFH for navigation operations. https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:480: // Initializes the first WebUI for a recently created RenderFrameHost, if one On 2015/10/14 23:54:14, nasko (slow to review) wrote: > Does the "recently created" bit matter? Removed it. It was not really true and the text that follows is clear enough: "No WebUI must have been set before and it must be called only once" https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:492: // pending WebUI instance is set. On 2015/10/14 23:54:14, nasko (slow to review) wrote: > As before, this comment seems to describe the implementation. Can you abstract > it such that it is less verbose? Simplified the comment. https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:666: // NavigationEntryImpl::kInvalidBindings). If no WebUI applies, returns On 2015/10/14 23:54:14, nasko (slow to review) wrote: > Again, no need to include the text in (). This is implementation detail. Done. https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:840: // The associated WebUIImpl and its type. They will be set if the current page On 2015/10/14 23:54:14, nasko (slow to review) wrote: > This is RenderFrameHost, it cannot have a page : ). Let's use "the current > document is from WebUI source." Done. https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1705: new_instance, url, bindings, create_render_frame_flags, nullptr); On 2015/10/14 23:54:15, nasko (slow to review) wrote: > Why can't this be CreateRenderFrame, then call to InitializeWebUI? See below. https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1760: new_render_frame_host->CommitPendingWebUI(); On 2015/10/14 23:54:14, nasko (slow to review) wrote: > This is only done in case of reusing existing RFH. See below #2. https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1786: new_render_frame_host->InitializeWebUI(url, bindings); On 2015/10/14 23:54:15, nasko (slow to review) wrote: > This is done only on new RFHs. Combined with the comment above, it means that we > can potentially have an output parameter from this method and call either > Update&Commit or Initialize once this method returns. This will kill off the url > and bindings parameter, which I really don't like to have. > > Also, as I'm very close to swapped out removal, once it is complete, the other > branch will disappear entirely, so we will know that we always have to call > Initialize after this method and the output parameter can disappear and get back > to much simpler code. The problem with initializing the WebUI outside of this call is that we would also need to move out the two different WebUI operations that happen inside InitRenderView called below. As those happen in two slightly different situations we'd need to add out parameters for those states to both the CreateRenderFrameInternal and InitRenderView. IMO this is not a good exchange. Note that the two parameters are not a regression (apart from being two); they are there to replace the previous WebUI pointer. But... See below #3. https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1984: dest_web_ui->RenderViewCreated(render_view_host); On 2015/10/14 23:54:15, nasko (slow to review) wrote: > Why can't we move this call to right after we've created the RenderFrame and > initialized the WebUI? That is what I had before. But this call *really* needs to happen after an actual RenderView was created on the renderer process. > Also, looking over the usage of this in WebUI, it seems that eventually this > needs to move to RenderFrameCreated, which makes it even more suitable for > moving out of here. I think that's true (especially considering that WebUIImpl::RenderViewReused has a is-main-frame bool parameter) but I can't elaborate further. What I know right now is that the failures on the CertificateViewer tests for ChromeOS were due to this call happening before the RenderView was initialized in the renderer. Looking over the implementations of RenderViewCreated and RenderViewReused I noticed that: - It seems that whenever RenderViewCreated is not called but there is a RenderView in place, RenderViewReused should be called and I'm updating this code to do so. - It seems that some WebUIController implementations override only RenderViewCreated and not RenderViewReused... I wonder if that isn't in fact an error. - But also all (two) implementations of RenderViewReused do exactly what their RenderViewCreated already do... So maybe what we could do it to always call RenderViewCreated when a RFH is "created" even if it's swapped back in and the RenderView already existed. This would certainly simplify the heck of all of this... And it seems to work. https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (left): https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:365: WebUIImpl* web_ui, On 2015/10/14 23:54:15, nasko (slow to review) wrote: > /me likes this parameter being gone! : ) Acknowledged. https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:256: // Returns the current committed WebUI or NULL if none applies. On 2015/10/14 23:54:15, nasko (slow to review) wrote: > nit: Let's update comments as we go. s/NULL/null/ or s/NULL/nullptr/, whichever > you prefer. Done. https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:75: if (!(should_create_webui_ && HasWebUIScheme(url))) On 2015/10/14 23:54:15, nasko (slow to review) wrote: > Why not invert the code such that you don't have ! and return the casted this, > otherwise return kNoWebUI. > > Also, a comment on why this is needed will be good to have. As I read through > this, I'm not quite sure why this is done. This is done so that this code actually conforms to what this method implementation is expected to do. This was a bad mock in the first place but it worked because the previous logic didn't rely on this type value having the correct value returned. You can see I simply copy/pasted the same logic used above to get the WebUIController instance. I'll invert both implementations and add a comment. https://codereview.chromium.org/1352813006/diff/360001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1352813006/diff/360001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:485: void InitializeWebUI(const GURL& dest_url, int entry_bindings); As the WebUIImpl::RenderView(Created|Reused) calls were moved out of RFHI, this initialization method was not needed anymore. https://codereview.chromium.org/1352813006/diff/380001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/380001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1089: render_frame_host_->DiscardPendingWebUI(); Calling this here is incorrect for PlzNavigate. This will be fixed in a follow up change. https://codereview.chromium.org/1352813006/diff/380001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1867: DCHECK(new_render_frame_host->render_view_host()->IsRenderViewLive()); I added this check here as it seemed to be true by looking at the code. If tests prove me right this will allow further simplifications down this code path where renderer process were supposedly being initialized and should require notifications to be sent to WebUI instances. https://codereview.chromium.org/1352813006/diff/380001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/1352813006/diff/380001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:654: void InitializeWebUI(const GURL& url, int bindings, RenderFrameHostImpl* rfh); Had to add this to consolidate common code between both navigation implementations.
https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:492: // pending WebUI instance is set. On 2015/10/15 16:34:07, carlosk wrote: > On 2015/10/14 23:54:14, nasko (slow to review) wrote: > > As before, this comment seems to describe the implementation. Can you abstract > > it such that it is less verbose? > > Simplified the comment. This now is an awesome comment! Thank you! https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1984: dest_web_ui->RenderViewCreated(render_view_host); On 2015/10/15 16:34:07, carlosk wrote: > On 2015/10/14 23:54:15, nasko (slow to review) wrote: > > Why can't we move this call to right after we've created the RenderFrame and > > initialized the WebUI? > > That is what I had before. But this call *really* needs to happen after an > actual RenderView was created on the renderer process. > > > Also, looking over the usage of this in WebUI, it seems that eventually this > > needs to move to RenderFrameCreated, which makes it even more suitable for > > moving out of here. > > I think that's true (especially considering that WebUIImpl::RenderViewReused has > a is-main-frame bool parameter) but I can't elaborate further. What I know right > now is that the failures on the CertificateViewer tests for ChromeOS were due to > this call happening before the RenderView was initialized in the renderer. RenderViewCreated as a notification means exactly that - a RenderView object was created on the renderer side. This should follow in time after we send the IPC to create the RenderView. > Looking over the implementations of RenderViewCreated and RenderViewReused I > noticed that: > - It seems that whenever RenderViewCreated is not called but there is a > RenderView in place, RenderViewReused should be called and I'm updating this > code to do so. Correct. I would expect that RenderViewReused is different, because some state has already been setup in the renderer process as part of RenderViewCreated. However, let's file a bug for all of this to move to RenderFrameCreated and I expect as part of this work we can clarify what is happening and why. > - It seems that some WebUIController implementations override only > RenderViewCreated and not RenderViewReused... I wonder if that isn't in fact an > error. I'd leave this to dbeam@ to answer. > - But also all (two) implementations of RenderViewReused do exactly what their > RenderViewCreated already do... > > So maybe what we could do it to always call RenderViewCreated when a RFH is > "created" even if it's swapped back in and the RenderView already existed. This > would certainly simplify the heck of all of this... And it seems to work. I'd love it if this is the case and dbeam@ doesn't scream. Removing the RenderViewReused code will be further simplification we can make. https://codereview.chromium.org/1352813006/diff/380001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/380001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:397: // be invoked here for the *correct* WebUI instance. Are this and the same TODO below supposed to be resolved in this CL? https://codereview.chromium.org/1352813006/diff/380001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1089: render_frame_host_->DiscardPendingWebUI(); On 2015/10/15 16:34:08, carlosk wrote: > Calling this here is incorrect for PlzNavigate. This will be fixed in a follow > up change. Acknowledged. https://codereview.chromium.org/1352813006/diff/380001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1609: InitializeWebUI(url, bindings, pending_render_frame_host_.get()); Now that I think a bit more about it, this call can be made to only take url and bindings. Then moved out of this and the create speculative one into the respective state updating methods - UpdateStateForNavigate and GetFrameHostForNavigation. At both of those call sites we have the proper URL and bindings parameters to pass and at the end of the day the result will be identical. In the future, when we have only PlzNavigate and move the WebUI ownership to NavigationRequest, these methods won't need to be updated. Rather, we will just call Create*RenderFrameHost, get the result and do rfh.TakeWebUI(NavRequest.WebUI.Pass()). https://codereview.chromium.org/1352813006/diff/380001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1720: RenderFrameHostImpl* rfh) { Can't we just pick pending vs speculative inside this method based on the command line flag? Why do we need it as a parameter? https://codereview.chromium.org/1352813006/diff/380001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1867: DCHECK(new_render_frame_host->render_view_host()->IsRenderViewLive()); On 2015/10/15 16:34:08, carlosk wrote: > I added this check here as it seemed to be true by looking at the code. If tests > prove me right this will allow further simplifications down this code path where > renderer process were supposedly being initialized and should require > notifications to be sent to WebUI instances. Acknowledged. https://codereview.chromium.org/1352813006/diff/380001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/1352813006/diff/380001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:606: const GURL& url, Let's order the parameters on this method and CraeteSpeculativeRenderFrameHost to be consistent. Pick one and change the other. https://codereview.chromium.org/1352813006/diff/380001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:654: void InitializeWebUI(const GURL& url, int bindings, RenderFrameHostImpl* rfh); On 2015/10/15 16:34:08, carlosk wrote: > Had to add this to consolidate common code between both navigation > implementations. Can you put a "// TODO(nasko): Revise when https://crbug.com/357747 is fixed." for me? https://codereview.chromium.org/1352813006/diff/380001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/1352813006/diff/380001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:72: return NULL; nullptr
Thanks. https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1984: dest_web_ui->RenderViewCreated(render_view_host); On 2015/10/15 17:40:09, nasko (slow to review) wrote: > On 2015/10/15 16:34:07, carlosk wrote: > > On 2015/10/14 23:54:15, nasko (slow to review) wrote: > > > Why can't we move this call to right after we've created the RenderFrame and > > > initialized the WebUI? > > > > That is what I had before. But this call *really* needs to happen after an > > actual RenderView was created on the renderer process. > > > > > Also, looking over the usage of this in WebUI, it seems that eventually this > > > needs to move to RenderFrameCreated, which makes it even more suitable for > > > moving out of here. > > > > I think that's true (especially considering that WebUIImpl::RenderViewReused > has > > a is-main-frame bool parameter) but I can't elaborate further. What I know > right > > now is that the failures on the CertificateViewer tests for ChromeOS were due > to > > this call happening before the RenderView was initialized in the renderer. > > RenderViewCreated as a notification means exactly that - a RenderView object was > created on the renderer side. This should follow in time after we send the IPC > to create the RenderView. > > > Looking over the implementations of RenderViewCreated and RenderViewReused I > > noticed that: > > - It seems that whenever RenderViewCreated is not called but there is a > > RenderView in place, RenderViewReused should be called and I'm updating this > > code to do so. > > Correct. I would expect that RenderViewReused is different, because some state > has already been setup in the renderer process as part of RenderViewCreated. > However, let's file a bug for all of this to move to RenderFrameCreated and I > expect as part of this work we can clarify what is happening and why. http://crbug.com/544982 > > - It seems that some WebUIController implementations override only > > RenderViewCreated and not RenderViewReused... I wonder if that isn't in fact > an > > error. > > I'd leave this to dbeam@ to answer. > > > - But also all (two) implementations of RenderViewReused do exactly what their > > RenderViewCreated already do... > > > > So maybe what we could do it to always call RenderViewCreated when a RFH is > > "created" even if it's swapped back in and the RenderView already existed. > This > > would certainly simplify the heck of all of this... And it seems to work. > > I'd love it if this is the case and dbeam@ doesn't scream. Removing the > RenderViewReused code will be further simplification we can make. I think these method names are a bit misleading, if I'm right about what I'll describe next. It seems to me that RenderViewCreated should be called whenever the WebUI hasn't yet "interacted" with the RenderView being assigned to it and not only when one has actually just been created. And it seems that this patch will do that now. For RenderViewReused it seems it should be called when the WebUI has already interacted with that RenderView before, so RenderViewCreated should have been previously called for the same pair of WebUI and RenderView. In this case it will not happen when we keep the SiteInstance/RVHI but change the WebUI: RenderViewReused will be called without a previous call to RenderViewCreated for the same pair. So seeing RenderViewReused and RenderViewCreated implemented exactly the same is a relief... :) https://codereview.chromium.org/1352813006/diff/380001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/380001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:397: // be invoked here for the *correct* WebUI instance. On 2015/10/15 17:40:09, nasko (slow to review) wrote: > Are this and the same TODO below supposed to be resolved in this CL? My intention was not that but I was wrong so thanks for this comment! :) With my moving of the RenderViewCreated calls out of the code path started by InitRenderView that *must* be done here! So I'm adding the proper calls. Even so I think there is still an issue here with interwoven navigation requests where we might notify the wrong WebUI instance. I will investigate a bit more and file a bug on that if that' s really the case. https://codereview.chromium.org/1352813006/diff/380001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1609: InitializeWebUI(url, bindings, pending_render_frame_host_.get()); On 2015/10/15 17:40:09, nasko (slow to review) wrote: > Now that I think a bit more about it, this call can be made to only take url and > bindings. Then moved out of this and the create speculative one into the > respective state updating methods - UpdateStateForNavigate and > GetFrameHostForNavigation. At both of those call sites we have the proper URL > and bindings parameters to pass and at the end of the day the result will be > identical. > In the future, when we have only PlzNavigate and move the WebUI ownership to > NavigationRequest, these methods won't need to be updated. Rather, we will just > call Create*RenderFrameHost, get the result and do > rfh.TakeWebUI(NavRequest.WebUI.Pass()). Done. What I like most about this format is that, for PlzNavigate, WebUI updates are *all* triggered from RFHM::GetFrameHostForNavigation (except for the final commit, obviously). https://codereview.chromium.org/1352813006/diff/380001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1720: RenderFrameHostImpl* rfh) { On 2015/10/15 17:40:09, nasko (slow to review) wrote: > Can't we just pick pending vs speculative inside this method based on the > command line flag? Why do we need it as a parameter? Done. https://codereview.chromium.org/1352813006/diff/380001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/1352813006/diff/380001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:606: const GURL& url, On 2015/10/15 17:40:09, nasko (slow to review) wrote: > Let's order the parameters on this method and CraeteSpeculativeRenderFrameHost > to be consistent. Pick one and change the other. Done. https://codereview.chromium.org/1352813006/diff/380001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:654: void InitializeWebUI(const GURL& url, int bindings, RenderFrameHostImpl* rfh); On 2015/10/15 17:40:09, nasko (slow to review) wrote: > On 2015/10/15 16:34:08, carlosk wrote: > > Had to add this to consolidate common code between both navigation > > implementations. > > Can you put a "// TODO(nasko): Revise when https://crbug.com/357747 is fixed." > for me? Done. https://codereview.chromium.org/1352813006/diff/380001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/1352813006/diff/380001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:72: return NULL; On 2015/10/15 17:40:10, nasko (slow to review) wrote: > nullptr Done.
i think this CL is becoming a cluster https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1984: dest_web_ui->RenderViewCreated(render_view_host); On 2015/10/19 17:21:01, carlosk wrote: > On 2015/10/15 17:40:09, nasko (slow to review) wrote: > > On 2015/10/15 16:34:07, carlosk wrote: > > > On 2015/10/14 23:54:15, nasko (slow to review) wrote: > > > > Why can't we move this call to right after we've created the RenderFrame > and > > > > initialized the WebUI? > > > > > > That is what I had before. But this call *really* needs to happen after an > > > actual RenderView was created on the renderer process. > > > > > > > Also, looking over the usage of this in WebUI, it seems that eventually > this > > > > needs to move to RenderFrameCreated, which makes it even more suitable for > > > > moving out of here. > > > > > > I think that's true (especially considering that WebUIImpl::RenderViewReused > > has > > > a is-main-frame bool parameter) but I can't elaborate further. What I know > > right > > > now is that the failures on the CertificateViewer tests for ChromeOS were > due > > to > > > this call happening before the RenderView was initialized in the renderer. > > > > RenderViewCreated as a notification means exactly that - a RenderView object > was > > created on the renderer side. This should follow in time after we send the IPC > > to create the RenderView. > > > > > Looking over the implementations of RenderViewCreated and RenderViewReused I > > > noticed that: > > > - It seems that whenever RenderViewCreated is not called but there is a > > > RenderView in place, RenderViewReused should be called and I'm updating this > > > code to do so. > > > > Correct. I would expect that RenderViewReused is different, because some state > > has already been setup in the renderer process as part of RenderViewCreated. ^ this > > However, let's file a bug for all of this to move to RenderFrameCreated and I > > expect as part of this work we can clarify what is happening and why. > > http://crbug.com/544982 > > > > - It seems that some WebUIController implementations override only > > > RenderViewCreated and not RenderViewReused... I wonder if that isn't in fact > > an > > > error. > > > > I'd leave this to dbeam@ to answer. some handlers are re-entrant or agnostic to the state of their RenderFrame > > > > > - But also all (two) implementations of RenderViewReused do exactly what > their > > > RenderViewCreated already do... > > > > > > So maybe what we could do it to always call RenderViewCreated when a RFH is > > > "created" even if it's swapped back in and the RenderView already existed. > > This > > > would certainly simplify the heck of all of this... And it seems to work. > > > > I'd love it if this is the case and dbeam@ doesn't scream. Removing the > > RenderViewReused code will be further simplification we can make. as long as re-using handlers still works and doesn't cause crashing about re-adding observers or otherwise re-running initialization code, I'm fine with it. > > I think these method names are a bit misleading, if I'm right about what I'll > describe next. > > It seems to me that RenderViewCreated should be called whenever the WebUI hasn't > yet "interacted" with the RenderView being assigned to it and not only when one > has actually just been created. And it seems that this patch will do that now. do you mean make WebUIs or WebUIMessageHandlers optionally subscribe to a notification about this if they care? that's basically what the default (empty) implementation does, no? > > For RenderViewReused it seems it should be called when the WebUI has already > interacted with that RenderView before, so RenderViewCreated should have been > previously called for the same pair of WebUI and RenderView. In this case it > will not happen when we keep the SiteInstance/RVHI but change the WebUI: > RenderViewReused will be called without a previous call to RenderViewCreated for > the same pair. > > So seeing RenderViewReused and RenderViewCreated implemented exactly the same is > a relief... :) Y'all probably already know this but: Navigating to some URLs create WebUIs (which in turn create WebUIMessageHandlers). This code runs before we're sure a Render{Frame,View} is created, and it's useful (or required) to know when the RenderFrame comes online. WebUI and WebUIMessageHandlers commonly observe priviledged browser-side services and notify the RenderFrame via ExecuteJavascript() if things change.[1] Currently, if there's no RenderFrame yet, the notification gets dropped.[2] Creating WebUIs and WebUIMessageHandlers early is good for performance (lets us kick off hard, async things earlier) and code simplicity (the JS can call chrome.send() immediately). Re-using WebUIs is also good for performance as they generally don't need to be torn down and recreated on same-origin navigation, however care must be taken to not re-run initialization code (i.e. we've had crashes from double-calling AddObserver()).[3] Currently, the "uber" page creates all the WebUI + WebUIMessageHandlers for any subframe (i.e. all of history, settings, help, extensions) but lazily creates the RenderFrame.[4][5] This can be wasteful and is probably doing too much early initialization. estade@ and I are fine to change to lazy WebUI creation for the uber page (i.e. as a new <iframe src="chrome://settings-frame"> is created dynamically in JS, the browser side gets an IPC about frame navigation and triggers WebUI creation based on that URL). See also: https://code.google.com/p/chromium/issues/detail?id=516690#c23 [1] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... [2] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... [3] https://chromiumcodereview.appspot.com/9693032 [4] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... [5] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
Mostly nits about using DCHECK instead of CHECK. Couple of questions to answer, but I think this is very close to being ready. I'm not sure what dbeam@'s comment "i think this CL is becoming a cluster" means - positive or negative? I'd really like to understand if this is seen as good change or not by others. https://codereview.chromium.org/1352813006/diff/420001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/420001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2011: CHECK_EQ(!pending_web_ui_, pending_web_ui_type_ == WebUI::kNoWebUI); DCHECK_EQ https://codereview.chromium.org/1352813006/diff/420001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2016: CHECK(!pending_web_ui_ && pending_web_ui_type_ == WebUI::kNoWebUI); DCHECK https://codereview.chromium.org/1352813006/diff/420001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2349: CHECK(web_ui); DCHECK https://codereview.chromium.org/1352813006/diff/420001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/420001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1014: if (pending_web_ui() && render_frame_host_->IsRenderFrameLive()) { Doesn't this depend on whether we had a different WebUI object before calling UpdatePendingWebUI? If there wasn't one, shouldn't this call RenderViewCreated? https://codereview.chromium.org/1352813006/diff/420001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1035: // must be updated to make sure it matches the current URL. Why does it have to be committed? It isn't immediately obvious, so a comment clarifying it will be useful to have. https://codereview.chromium.org/1352813006/diff/420001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1724: CHECK(rfh); DCHECK https://codereview.chromium.org/1352813006/diff/420001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2280: render_frame_host_->DiscardPendingWebUI(); Can't this move either closer to InitializeWebUI or even be folded into it? At least CreatePendingRenderFrameHost() should be doable before this code, as the two shouldn't intersect in members they operate on. If moved just before InitializeWebUI, it will allow us to have all WebUI code in one place. https://codereview.chromium.org/1352813006/diff/420001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/1352813006/diff/420001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:650: // TODO(nasko): Revise when https://crbug.com/357747 is fixed." nit: Extra quotes at the end which aren't needed.
On 2015/10/20 01:13:22, nasko (slow to review) wrote: > I'm not sure what dbeam@'s comment "i think this CL is becoming a cluster" means > - positive or negative? I'd really like to understand if this is seen as good > change or not by others. the change is fine, I just think it's becoming hard to review due to the large diff and number of rounds of comments anyways, if you don't find it hard to follow: ignore me
On 2015/10/20 02:21:25, Dan Beam wrote: > On 2015/10/20 01:13:22, nasko (slow to review) wrote: > > I'm not sure what dbeam@'s comment "i think this CL is becoming a cluster" > means > > - positive or negative? I'd really like to understand if this is seen as good > > change or not by others. > > the change is fine, I just think it's becoming hard to review due to the large > diff and number of rounds of comments > > anyways, if you don't find it hard to follow: ignore me Yes, changes that take a few rounds and have lots of diffs are hard to follow. This is why I occasionally review the full CL from scratch and I think as it is, it looks easier to understand than before. Since I would like creis@ to take a peek at the bindings checks, he can give us another view.
Thanks! On 2015/10/20 03:58:46, nasko (slow to review) wrote: > On 2015/10/20 02:21:25, Dan Beam wrote: > > On 2015/10/20 01:13:22, nasko (slow to review) wrote: > > > I'm not sure what dbeam@'s comment "i think this CL is becoming a cluster" > > means > > > - positive or negative? I'd really like to understand if this is seen as > good > > > change or not by others. > > > > the change is fine, I just think it's becoming hard to review due to the large > > diff and number of rounds of comments > > > > anyways, if you don't find it hard to follow: ignore me > > Yes, changes that take a few rounds and have lots of diffs are hard to follow. > This is why I occasionally review the full CL from scratch and I think as it is, > it looks easier to understand than before. Since I would like creis@ to take a > peek at the bindings checks, he can give us another view. I'm glad there's still hope! :) From the previous round of replies it seems my conclusions around the use of the RenderView notifications are about right. If that's really the case this CL should create no regressions so further changes and fixes can be postponed to follow up CLs. https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1984: dest_web_ui->RenderViewCreated(render_view_host); On 2015/10/19 19:22:10, Dan Beam wrote: > On 2015/10/19 17:21:01, carlosk wrote: > > On 2015/10/15 17:40:09, nasko (slow to review) wrote: > > > On 2015/10/15 16:34:07, carlosk wrote: > > > > On 2015/10/14 23:54:15, nasko (slow to review) wrote: > > > > > Why can't we move this call to right after we've created the RenderFrame > > and > > > > > initialized the WebUI? > > > > > > > > That is what I had before. But this call *really* needs to happen after an > > > > actual RenderView was created on the renderer process. > > > > > > > > > Also, looking over the usage of this in WebUI, it seems that eventually > > this > > > > > needs to move to RenderFrameCreated, which makes it even more suitable > for > > > > > moving out of here. > > > > > > > > I think that's true (especially considering that > WebUIImpl::RenderViewReused > > > has > > > > a is-main-frame bool parameter) but I can't elaborate further. What I know > > > right > > > > now is that the failures on the CertificateViewer tests for ChromeOS were > > due > > > to > > > > this call happening before the RenderView was initialized in the renderer. > > > > > > RenderViewCreated as a notification means exactly that - a RenderView object > > was > > > created on the renderer side. This should follow in time after we send the > IPC > > > to create the RenderView. > > > > > > > Looking over the implementations of RenderViewCreated and RenderViewReused > I > > > > noticed that: > > > > - It seems that whenever RenderViewCreated is not called but there is a > > > > RenderView in place, RenderViewReused should be called and I'm updating > this > > > > code to do so. > > > > > > Correct. I would expect that RenderViewReused is different, because some > state > > > has already been setup in the renderer process as part of RenderViewCreated. > > ^ this > > > > However, let's file a bug for all of this to move to RenderFrameCreated and > I > > > expect as part of this work we can clarify what is happening and why. > > > > http://crbug.com/544982 > > > > > > - It seems that some WebUIController implementations override only > > > > RenderViewCreated and not RenderViewReused... I wonder if that isn't in > fact > > > an > > > > error. > > > > > > I'd leave this to dbeam@ to answer. > > some handlers are re-entrant or agnostic to the state of their RenderFrame > > > > > > > > - But also all (two) implementations of RenderViewReused do exactly what > > their > > > > RenderViewCreated already do... > > > > > > > > So maybe what we could do it to always call RenderViewCreated when a RFH > is > > > > "created" even if it's swapped back in and the RenderView already existed. > > > This > > > > would certainly simplify the heck of all of this... And it seems to work. > > > > > > I'd love it if this is the case and dbeam@ doesn't scream. Removing the > > > RenderViewReused code will be further simplification we can make. > > as long as re-using handlers still works and doesn't cause crashing about > re-adding observers or otherwise re-running initialization code, I'm fine with > it. Are there tests that cover these situations? Trybots were green after these changes were uploaded. > > I think these method names are a bit misleading, if I'm right about what I'll > > describe next. > > > > It seems to me that RenderViewCreated should be called whenever the WebUI > hasn't > > yet "interacted" with the RenderView being assigned to it and not only when > one > > has actually just been created. And it seems that this patch will do that now. > > do you mean make WebUIs or WebUIMessageHandlers optionally subscribe to a > notification about this if they care? that's basically what the default (empty) > implementation does, no? No, what I meant is that looking at the current code RenderViewCreated would not be called in a situation where I think it should be. In a cross-site navigation to a WebUI URL a new WebUI is always created [10] but not necessarily a new RenderView is. Calls to RenderViewCreated happen only when a RenderView is actually created [11] [12] but not in cases when one is reused [13]. This could theoretically cause a WebUI to be assigned to a RenderView without any calls to the RenderView* notifications. I say theoretically because the code path exists but I'm not sure it happens in practice. With this change RenderViewCreated will always be called for both cases of creating or reusing an existing RenderView. And the other situation that seems wrong is what I describe next... > > For RenderViewReused it seems it should be called when the WebUI has already > > interacted with that RenderView before, so RenderViewCreated should have been > > previously called for the same pair of WebUI and RenderView. In this case it > > will not happen when we keep the SiteInstance/RVHI but change the WebUI: > > RenderViewReused will be called without a previous call to RenderViewCreated > for > > the same pair. > > > > So seeing RenderViewReused and RenderViewCreated implemented exactly the same > is > > a relief... :) This is already the case with the current implementation [14] and my changes don't change this. So if my descriptions of how I think RenderViewCreated and RenderViewReused should be used are correct then this is an error. WDYT? > Y'all probably already know this but: > > Navigating to some URLs create WebUIs (which in turn create > WebUIMessageHandlers). This code runs before we're sure a Render{Frame,View} is > created, and it's useful (or required) to know when the RenderFrame comes > online. > > WebUI and WebUIMessageHandlers commonly observe priviledged browser-side > services and notify the RenderFrame via ExecuteJavascript() if things change.[1] > Currently, if there's no RenderFrame yet, the notification gets dropped.[2] > > Creating WebUIs and WebUIMessageHandlers early is good for performance (lets us > kick off hard, async things earlier) and code simplicity (the JS can call > chrome.send() immediately). Re-using WebUIs is also good for performance as > they generally don't need to be torn down and recreated on same-origin > navigation, however care must be taken to not re-run initialization code (i.e. > we've had crashes from double-calling AddObserver()).[3] > > Currently, the "uber" page creates all the WebUI + WebUIMessageHandlers for any > subframe (i.e. all of history, settings, help, extensions) but lazily creates > the RenderFrame.[4][5] This can be wasteful and is probably doing too much > early initialization. > > estade@ and I are fine to change to lazy WebUI creation for the uber page (i.e. > as a new <iframe src="chrome://settings-frame"> is created dynamically in JS, > the browser side gets an IPC about frame navigation and triggers WebUI creation > based on that URL). I had just an idea of how it worked so this was very instructional. Thanks! And again I'm assuming these are still working as all trybots tests were green (right now that's not the case but it seems not be due to my changes). > See also: https://code.google.com/p/chromium/issues/detail?id=516690#c23 I don't have permissions to see this issue. > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > [2] > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... > [3] https://chromiumcodereview.appspot.com/9693032 > [4] > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > [5] > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... [10] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... [11] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... [12] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... [13] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... [14] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... https://codereview.chromium.org/1352813006/diff/420001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/420001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2011: CHECK_EQ(!pending_web_ui_, pending_web_ui_type_ == WebUI::kNoWebUI); On 2015/10/20 01:13:21, nasko (slow to review) wrote: > DCHECK_EQ Done. https://codereview.chromium.org/1352813006/diff/420001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2016: CHECK(!pending_web_ui_ && pending_web_ui_type_ == WebUI::kNoWebUI); On 2015/10/20 01:13:21, nasko (slow to review) wrote: > DCHECK Done. https://codereview.chromium.org/1352813006/diff/420001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2349: CHECK(web_ui); On 2015/10/20 01:13:21, nasko (slow to review) wrote: > DCHECK Done. https://codereview.chromium.org/1352813006/diff/420001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/420001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1014: if (pending_web_ui() && render_frame_host_->IsRenderFrameLive()) { On 2015/10/20 01:13:21, nasko (slow to review) wrote: > Doesn't this depend on whether we had a different WebUI object before calling > UpdatePendingWebUI? If there wasn't one, shouldn't this call RenderViewCreated? That's exactly what I'm proposing as an error and trying to figure out with my questions around the correct situations to call those two methods. But again, this is not a regression: - This is a fix because the current code was missing this snippet for PlzNavigate - This keeps the same rationale used for regular navigations [1] [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... https://codereview.chromium.org/1352813006/diff/420001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1035: // must be updated to make sure it matches the current URL. On 2015/10/20 01:13:22, nasko (slow to review) wrote: > Why does it have to be committed? It isn't immediately obvious, so a comment > clarifying it will be useful to have. Because as described in the design doc [1], the pending/speculative RFH should only have an active WebUI (only the current RFH can carry a pending WebUI). This is "by definition" to maintain sanity. Updated this comment and the equivalent section in InitializeWebUI [1] https://docs.google.com/document/d/1jrge5ulZ1YtqSR3WkNXzrtxhd3JXcWm8_zWBhEjaJ... https://codereview.chromium.org/1352813006/diff/420001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1724: CHECK(rfh); On 2015/10/20 01:13:22, nasko (slow to review) wrote: > DCHECK Done. https://codereview.chromium.org/1352813006/diff/420001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2280: render_frame_host_->DiscardPendingWebUI(); On 2015/10/20 01:13:22, nasko (slow to review) wrote: > Can't this move either closer to InitializeWebUI or even be folded into it? At > least CreatePendingRenderFrameHost() should be doable before this code, as the > two shouldn't intersect in members they operate on. If moved just before > InitializeWebUI, it will allow us to have all WebUI code in one place. Done. https://codereview.chromium.org/1352813006/diff/420001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/1352813006/diff/420001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:650: // TODO(nasko): Revise when https://crbug.com/357747 is fixed." On 2015/10/20 01:13:22, nasko (slow to review) wrote: > nit: Extra quotes at the end which aren't needed. Done.
most of this zooms over my head, but lgtm if it helps! https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_delegate.h:12: #include "content/browser/webui/web_ui_impl.h" why not forward declare instead?
Thanks-- I'm thrilled to see the WebUI state move out of RFHM! A few questions and some nits below, but no major concerns. https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1040: web_ui_type_ = WebUI::kNoWebUI; Sanity check: Why is this the right time to revoke WebUI? The RenderFrameHost will still be running its unload handler in the background. Are there no cases that it will try to use the WebUI object during unload? https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1043: pending_web_ui_.reset(); Can we make these 4 lines into a private helper (ResetWebUI), also used in ~RenderFrameHostImpl? https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:220: // Returns the pending WebUI, or null of none applies. nit: s/of/if/ https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:654: // |entry_bindings|. If no WebUI applies, returns null and resets web_ui_type (Mention that we only check for a bindings match if entry_bindings is not kInvalidBindings.) https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:658: WebUI::TypeID* web_ui_type); Hmm, it's unfortunate that WebUI doesn't know its own type, since that requires us to have an out parameter here (and store the type separately below). Ah well; perhaps not something to tackle here. https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:835: // same-site navigation to a WebUI page (reusing this RenderFrameHost). Nasko, Dan, and I haven't been able to think of a case where this happens. When does a navigation change the WebUI object without changing the RFH? https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:397: if (pending_web_ui()) { This doesn't look safe. pending_web_ui() can return a WebUI from the current RFH when dest_render_frame_host is a different pending RFH. We should just ask dest_render_frame_host directly. https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:656: // If current RenderFrameHost has a pending WebUI, commit it. Could this be handled inside RenderFrameHostImpl::OnDidCommitProvisionalLoad? RFHM::CommitPending seems like it could be specific to cross-process navigations now, without having to worry about WebUI on same-process navigations. https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1720: RenderFrameHostImpl* rfh; nit: Initialize to nullptr. (Safer in case future changes turn the else branch into an else if, drop the else, etc.) https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1735: // - When (and if) it commits there's no need to also commit the WebUI. s/When (and if)/If/ https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1743: // If the ongoing navigation is not to a WebUI or the RenderView is in a Should this block be inside RenderFrameHost (e.g., UpdatePendingWebUI), so that happens no matter who calls UpdatePendingWebUI (even after future changes)? I worry about new code omitting this check. https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2097: render_frame_host_->CommitPendingWebUI(); As mentioned earlier, it would be great to avoid having this case in RFHM::CommitPending if possible. https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:257: WebUIImpl* web_ui() const { return render_frame_host_->web_ui(); } I don't think we should keep this, since it implies that RFHM is still in charge of this state. RFHM should just call web_ui() on the desired RFH. https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:262: : render_frame_host_->pending_web_ui(); This seems problematic to me. Can we avoid it? It's very confusing to return either the pending RFH's current WebUI or the current RFH's pending WebUI. These are different concepts that should not be hidden behind the same accessor, and callers should know which one they're looking for. Worse, it might be possible for both to exist (e.g., a pending cross-process navigation to a WebUI page is in progress, and then a non-user-gesture, same-process WebUI navigation occurs without pre-empting the first navigation). https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:268: WebUIImpl* speculative_web_ui() const { Same concerns here. https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:643: // navigation. They might be destroyed and re-created later if the navigation nit: s/They/It/
Patchset #24 (id:500001) has been deleted
Thanks for the latest round of comments. Given the change in RFHM that make navigations between different types of WebUIs to always create a new RFH (always cross-site) this patch will require some adaptations, comprising mainly the elimination of the now unneeded pending WebUI from the RFH. Before moving on with that I wanted to flush out the replies and changes I already had in place (un-replied comments by creis@ will be answered later). https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_delegate.h:12: #include "content/browser/webui/web_ui_impl.h" On 2015/10/22 20:56:31, Dan Beam wrote: > why not forward declare instead? I tried that but the compiler screams at me because of scoped_ptr (it apparently requires the type to be complete). https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1040: web_ui_type_ = WebUI::kNoWebUI; On 2015/10/23 06:39:25, Charlie Reis wrote: > Sanity check: Why is this the right time to revoke WebUI? The RenderFrameHost > will still be running its unload handler in the background. Are there no cases > that it will try to use the WebUI object during unload? Comparing with the previous behavior, RFHI:SwapOut is called only by two methods from RFHM (excluding tests) [1]: - RFHM::SwapOutOldFrame: which itself is only called by RFHM::CommitPending for the previously active RFH after the new one was put in place and any previous WebUI replaced. - RFHM::DiscardUnusedFrame which was always used to discard a speculative or pending RFH so I don't think we need to care about the unload handler for them. Given these two this location seemed good enough. WDYT? [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1043: pending_web_ui_.reset(); On 2015/10/23 06:39:25, Charlie Reis wrote: > Can we make these 4 lines into a private helper (ResetWebUI), also used in > ~RenderFrameHostImpl? Done. https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:220: // Returns the pending WebUI, or null of none applies. On 2015/10/23 06:39:25, Charlie Reis wrote: > nit: s/of/if/ Done. https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:654: // |entry_bindings|. If no WebUI applies, returns null and resets web_ui_type On 2015/10/23 06:39:25, Charlie Reis wrote: > (Mention that we only check for a bindings match if entry_bindings is not > kInvalidBindings.) Done. https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:658: WebUI::TypeID* web_ui_type); On 2015/10/23 06:39:25, Charlie Reis wrote: > Hmm, it's unfortunate that WebUI doesn't know its own type, since that requires > us to have an out parameter here (and store the type separately below). Ah > well; perhaps not something to tackle here. Yes. It was already brought up before and it was agreed it should be done separately. https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:835: // same-site navigation to a WebUI page (reusing this RenderFrameHost). On 2015/10/23 06:39:25, Charlie Reis wrote: > Nasko, Dan, and I haven't been able to think of a case where this happens. When > does a navigation change the WebUI object without changing the RFH? (this behavior was changed before I sent out this reply but I'll leave my original explanation here) In terms of the code path for that it would happen when all of these are true: * A non-default process model is in use (single-process or process-per-tab) [A] so that ShouldTransitionCrossSite returns false [1] * We navigate between WebUI URLs that use the same bindings so that ShouldSwapBrowsingInstancesForNavigation doesn't return an early true [2] * And finally the two WebUIs should be considered of different types [3] I had the impression before I was able to reproduce this with the default process model but I couldn't do that now. With --process-per-tab it was simple to repro this navigating from chrome://version to chrome://history. [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... [2] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... [3] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:397: if (pending_web_ui()) { On 2015/10/23 06:39:25, Charlie Reis wrote: > This doesn't look safe. pending_web_ui() can return a WebUI from the current > RFH when dest_render_frame_host is a different pending RFH. We should just ask > dest_render_frame_host directly. I am uncertain about this. It seems this would generally be triggered when navigating same-site, re-using the current RFH but its RenderFrame is not live. In that case notifying the current RFH's WebUI is precisely what is needed. Also the new logic seems to behave the same way as the current when triggered by the following call stack which would start right above here: - RenderFrameHostManager::InitRenderView - WebContentsImpl::CreateRenderViewForRenderManager - RenderViewHostImpl::CreateRenderView - WebContentsImpl::RenderViewCreated [1] [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1720: RenderFrameHostImpl* rfh; On 2015/10/23 06:39:26, Charlie Reis wrote: > nit: Initialize to nullptr. (Safer in case future changes turn the else branch > into an else if, drop the else, etc.) Done. https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1735: // - When (and if) it commits there's no need to also commit the WebUI. On 2015/10/23 06:39:25, Charlie Reis wrote: > s/When (and if)/If/ Done. https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1743: // If the ongoing navigation is not to a WebUI or the RenderView is in a On 2015/10/23 06:39:26, Charlie Reis wrote: > Should this block be inside RenderFrameHost (e.g., UpdatePendingWebUI), so that > happens no matter who calls UpdatePendingWebUI (even after future changes)? I > worry about new code omitting this check. Done. I wanted this from the start and tried it quite a few times without success. But seems to have worked now from my local tests (I think it was before the bigger changes to InitRenderView). Let's see what the trybots will say... https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:257: WebUIImpl* web_ui() const { return render_frame_host_->web_ui(); } On 2015/10/23 06:39:26, Charlie Reis wrote: > I don't think we should keep this, since it implies that RFHM is still in charge > of this state. RFHM should just call web_ui() on the desired RFH. Yes, this has been brought up before and it will be done in a following change. The reason I'm postponing this is to manage the size of this one. https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:643: // navigation. They might be destroyed and re-created later if the navigation On 2015/10/23 06:39:26, Charlie Reis wrote: > nit: s/They/It/ Done. https://codereview.chromium.org/1352813006/diff/520001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/520001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1017: frame_tree_node_->IsMainFrame()); This is a bugfix.
Patchset #25 (id:540001) has been deleted
Few comments. https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1044: ResetWebUI(); This should probably be done when we receive the SwapOut ack. Otherwise the WebUI object will be gone and if the page is calling any JS in its unload handler that communicates with the WebUI, it will fail. https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2011: WebUIImpl* prev_web_ui_ptr = web_ui_.get(); nit: No need to add _ptr in the variable name. https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2031: return prev_web_ui_ptr; The return type of this method is a boolean value, not a pointer. https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1031: should_reuse_web_ui_ = render_frame_host_->web_ui(); Wouldn't UpdateWebUI above create one if needed? So if before there wasn't one or one that didn't match the type, the value of should_reuse_web_ui_ will be incorrect? https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2308: pending_render_frame_host_->web_ui()->RenderViewCreated( I know the naming will be weird, but can't we fold those RenderViewCreated/Reused notifications inside UpdateWebUI? https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2371: // If a change in WebUI happened, check this is an acceptable case. In general, what happens if this is not an acceptable case? DCHECK is compiled out in release builds, so will we have a security problem if we continue with unacceptable case? https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:1182: EXPECT_TRUE(manager2->pending_web_ui()); This should be manager2->current_frame_host()->web_ui().
Thanks. I just wanted to reply to all open comments before moving on with the other changes (removing the pending_web_ui method). https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:656: // If current RenderFrameHost has a pending WebUI, commit it. On 2015/10/23 06:39:25, Charlie Reis wrote: > Could this be handled inside RenderFrameHostImpl::OnDidCommitProvisionalLoad? > RFHM::CommitPending seems like it could be specific to cross-process navigations > now, without having to worry about WebUI on same-process navigations. See reply in CommitPending. https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2097: render_frame_host_->CommitPendingWebUI(); On 2015/10/23 06:39:25, Charlie Reis wrote: > As mentioned earlier, it would be great to avoid having this case in > RFHM::CommitPending if possible. As we discussed offline, this change is not that simple because of the location bar focus behavior that takes palce here and would require changes in RFHM and/or RFHI delegates. https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:262: : render_frame_host_->pending_web_ui(); On 2015/10/23 06:39:26, Charlie Reis wrote: > This seems problematic to me. Can we avoid it? > > It's very confusing to return either the pending RFH's current WebUI or the > current RFH's pending WebUI. These are different concepts that should not be > hidden behind the same accessor, and callers should know which one they're > looking for. > > Worse, it might be possible for both to exist (e.g., a pending cross-process > navigation to a WebUI page is in progress, and then a non-user-gesture, > same-process WebUI navigation occurs without pre-empting the first navigation). Will be working on this next. But note that the issues you describe here did already apply to the existing code. https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:268: WebUIImpl* speculative_web_ui() const { On 2015/10/23 06:39:26, Charlie Reis wrote: > Same concerns here. Will be working on this next. https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1044: ResetWebUI(); On 2015/10/29 22:13:15, nasko (slow to review) wrote: > This should probably be done when we receive the SwapOut ack. Otherwise the > WebUI object will be gone and if the page is calling any JS in its unload > handler that communicates with the WebUI, it will fail. See similar question and my answer here: https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2011: WebUIImpl* prev_web_ui_ptr = web_ui_.get(); On 2015/10/29 22:13:15, nasko (slow to review) wrote: > nit: No need to add _ptr in the variable name. Done. https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2031: return prev_web_ui_ptr; On 2015/10/29 22:13:15, nasko (slow to review) wrote: > The return type of this method is a boolean value, not a pointer. Fixed. Trybots also brought that one to my attention. https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1031: should_reuse_web_ui_ = render_frame_host_->web_ui(); On 2015/10/29 22:13:15, nasko (slow to review) wrote: > Wouldn't UpdateWebUI above create one if needed? So if before there wasn't one > or one that didn't match the type, the value of should_reuse_web_ui_ will be > incorrect? Conceptually this is incorrect as you described but, by chance, there is no side effect in practice. The result from a call to pending_web_ui() would still return the correct WebUI for the navigation. I renamed this boolean to current_web_ui_is_navigating_ to fix the conceptual error. But his brought up another error: as a new WebUI can finally be created here, the notification section below must be aware of that and call RenderViewCreated instead. What is now done. https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2308: pending_render_frame_host_->web_ui()->RenderViewCreated( On 2015/10/29 22:13:15, nasko (slow to review) wrote: > I know the naming will be weird, but can't we fold those > RenderViewCreated/Reused notifications inside UpdateWebUI? That's what I tried before and I had to move them back out here. Finally the knowledge of if and when to call one or the other is only fully known at this level, not inside RFHI. So to not have these calls spread between these two files, which would be confusing, I find it preferable to have them all here. https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2371: // If a change in WebUI happened, check this is an acceptable case. On 2015/10/29 22:13:15, nasko (slow to review) wrote: > In general, what happens if this is not an acceptable case? DCHECK is compiled > out in release builds, so will we have a security problem if we continue with > unacceptable case? Right now the navigation would simply continue in release builds. If this is really considered a security issue I could change this to a CHECK instead. But I wouldn't feel comfortable with that right now because I am not sure I mapped all the "special cases" -- as we're calling them -- where these transitions are allowed, what cause crashes in the real world. I based them from running tests and they might not cover all possibilities. And as the logic that determines site instance changes is complex it's really hard to not do it empirically. https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:1182: EXPECT_TRUE(manager2->pending_web_ui()); On 2015/10/29 22:13:15, nasko (slow to review) wrote: > This should be manager2->current_frame_host()->web_ui(). Once I eliminate pending_web_ui() then that will be done.
In the interest of having a checkpoint on this long lasting refactor work I made some final adjustments: - Renamed the method RFHM::pending_web_ui to GetNavigatingWebUI that more accurately describes what it does - Updated comments that were now wrong given the latest updates - Fixed another missing special case of a WebUI transition in the current RFH. I also started another change (https://codereview.chromium.org/1418853003) based off of this one to carry on the work of removing the WebUI getters from RFHM. So I propose this change at its current state, of course with whatever other adjustments deemed necessary to make it LGTM-able.
Few more comments. https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1040: web_ui_type_ = WebUI::kNoWebUI; On 2015/10/27 14:35:44, carlosk wrote: > On 2015/10/23 06:39:25, Charlie Reis wrote: > > Sanity check: Why is this the right time to revoke WebUI? The RenderFrameHost > > will still be running its unload handler in the background. Are there no > cases > > that it will try to use the WebUI object during unload? > > Comparing with the previous behavior, RFHI:SwapOut is called only by two methods > from RFHM (excluding tests) [1]: > - RFHM::SwapOutOldFrame: which itself is only called by RFHM::CommitPending for > the previously active RFH after the new one was put in place and any previous > WebUI replaced. I'm not sure how that makes much of a difference. The unload handler can be considered complete only at OnSwapOut. Until then, unload handler could be sending IPCs to the browser process. We might not have any of those today, but I'd rather make this code work properly than rely on no such WebUI existing. > - RFHM::DiscardUnusedFrame which was always used to discard a speculative or > pending RFH so I don't think we need to care about the unload handler for them. > > Given these two this location seemed good enough. WDYT? > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2371: // If a change in WebUI happened, check this is an acceptable case. On 2015/10/30 10:35:23, carlosk wrote: > On 2015/10/29 22:13:15, nasko (slow to review) wrote: > > In general, what happens if this is not an acceptable case? DCHECK is compiled > > out in release builds, so will we have a security problem if we continue with > > unacceptable case? > > Right now the navigation would simply continue in release builds. If this is > really considered a security issue I could change this to a CHECK instead. > > But I wouldn't feel comfortable with that right now because I am not sure I > mapped all the "special cases" -- as we're calling them -- where these > transitions are allowed, what cause crashes in the real world. I based them from > running tests and they might not cover all possibilities. And as the logic that > determines site instance changes is complex it's really hard to not do it > empirically. I think it should be a CHECK. Allocating WebUI where one shouldn't be is a serious issue. We can monitor crashes to ensure we've covered all cases. https://codereview.chromium.org/1352813006/diff/720001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/720001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2026: ResetWebUI(); Can't we early return from here? It will make also the code below less indented. https://codereview.chromium.org/1352813006/diff/720001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1352813006/diff/720001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:485: // be set to NavigationEntryImpl::kInvalidBindings so that no checks are made. nit: s/made/done/ https://codereview.chromium.org/1352813006/diff/720001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/720001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:671: // Even when there is no pending RVH, there may be a pending Web UI. This comment doesn't make as much sense with your patch. https://codereview.chromium.org/1352813006/diff/720001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2329: DCHECK(pending_render_frame_host_); This DCHECK is redundant. The call to UpdateWebUI would've resulted in dereferencing a null pointer.
fdegans@chromium.org changed reviewers: - fdegans@chromium.org
Thanks! https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1040: web_ui_type_ = WebUI::kNoWebUI; On 2015/11/02 17:00:35, nasko (slow to review) wrote: > On 2015/10/27 14:35:44, carlosk wrote: > > On 2015/10/23 06:39:25, Charlie Reis wrote: > > > Sanity check: Why is this the right time to revoke WebUI? The > RenderFrameHost > > > will still be running its unload handler in the background. Are there no > > cases > > > that it will try to use the WebUI object during unload? > > > > Comparing with the previous behavior, RFHI:SwapOut is called only by two > methods > > from RFHM (excluding tests) [1]: > > - RFHM::SwapOutOldFrame: which itself is only called by RFHM::CommitPending > for > > the previously active RFH after the new one was put in place and any previous > > WebUI replaced. > > I'm not sure how that makes much of a difference. The unload handler can be > considered complete only at OnSwapOut. Until then, unload handler could be > sending IPCs to the browser process. We might not have any of those today, but > I'd rather make this code work properly than rely on no such WebUI existing. > > > - RFHM::DiscardUnusedFrame which was always used to discard a speculative or > > pending RFH so I don't think we need to care about the unload handler for > them. > > > > Given these two this location seemed good enough. WDYT? > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... > OK, I moved the destruction of the WebUI to when the swap-out ack is received. To add to the work on making sure this makes a difference: *if* received IPCs come through WebContentsImpl::OnMessageReceived then they will not reach a non-active or non-pending RFH's WebUI given its implementation uses WebContentsImpl::GetWebUI(). See https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1044: ResetWebUI(); On 2015/10/30 10:35:23, carlosk wrote: > On 2015/10/29 22:13:15, nasko (slow to review) wrote: > > This should probably be done when we receive the SwapOut ack. Otherwise the > > WebUI object will be gone and if the page is calling any JS in its unload > > handler that communicates with the WebUI, it will fail. > > See similar question and my answer here: > https://codereview.chromium.org/1352813006/diff/460001/content/browser/frame_... Done (as discussed in the link above). https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2371: // If a change in WebUI happened, check this is an acceptable case. On 2015/11/02 17:00:35, nasko (slow to review) wrote: > On 2015/10/30 10:35:23, carlosk wrote: > > On 2015/10/29 22:13:15, nasko (slow to review) wrote: > > > In general, what happens if this is not an acceptable case? DCHECK is > compiled > > > out in release builds, so will we have a security problem if we continue > with > > > unacceptable case? > > > > Right now the navigation would simply continue in release builds. If this is > > really considered a security issue I could change this to a CHECK instead. > > > > But I wouldn't feel comfortable with that right now because I am not sure I > > mapped all the "special cases" -- as we're calling them -- where these > > transitions are allowed, what cause crashes in the real world. I based them > from > > running tests and they might not cover all possibilities. And as the logic > that > > determines site instance changes is complex it's really hard to not do it > > empirically. > > I think it should be a CHECK. Allocating WebUI where one shouldn't be is a > serious issue. We can monitor crashes to ensure we've covered all cases. Done. https://codereview.chromium.org/1352813006/diff/720001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/720001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2026: ResetWebUI(); On 2015/11/02 17:00:36, nasko (slow to review) wrote: > Can't we early return from here? It will make also the code below less indented. No because of the bindings checks further below. Unless I extract parts of this back into separate method. https://codereview.chromium.org/1352813006/diff/720001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1352813006/diff/720001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:485: // be set to NavigationEntryImpl::kInvalidBindings so that no checks are made. On 2015/11/02 17:00:36, nasko (slow to review) wrote: > nit: s/made/done/ Done. https://codereview.chromium.org/1352813006/diff/720001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/720001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:671: // Even when there is no pending RVH, there may be a pending Web UI. On 2015/11/02 17:00:36, nasko (slow to review) wrote: > This comment doesn't make as much sense with your patch. Done. https://codereview.chromium.org/1352813006/diff/720001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2329: DCHECK(pending_render_frame_host_); On 2015/11/02 17:00:36, nasko (slow to review) wrote: > This DCHECK is redundant. The call to UpdateWebUI would've resulted in > dereferencing a null pointer. I moved this up where it matters.
Patchset #34 (id:740001) has been deleted
Patchset #34 (id:760001) has been deleted
I think it is mostly there, just a few minor fixes. https://codereview.chromium.org/1352813006/diff/720001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/720001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2026: ResetWebUI(); On 2015/11/03 09:50:30, carlosk wrote: > On 2015/11/02 17:00:36, nasko (slow to review) wrote: > > Can't we early return from here? It will make also the code below less > indented. > > No because of the bindings checks further below. Unless I extract parts of this > back into separate method. No, let's keep it all in one place, as bindings checks will be moving around in the future. https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1082: speculative_render_frame_host_->web_ui()->RenderViewCreated( The comment above says reuser, yet the call is to RenderViewCreated. If we still call this method, it seems to me that the code to UpdateWebUI can be pulled after the if statement, as it is identical and the checks are very similar. https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2413: frame_tree_node_->IsMainFrame()); All of the new code here is very similar to the code for PlzNavigate. I'm not sure if it is worthwhile trying to consolidate it, but it will likely be useful to put a comment that if anyone is changing this code, should also change the PlzNavigate version of it. https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2647: // Switching WebUI from one type to another is never acceptable. It is acceptable, as long as we change RFHs, right? https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2655: // - A restore navigation to a WebUI URL. Let's leave an empty line between the different cases, as it is easier to visually distinguish them. https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2664: // - Navigating back from a special renderer controlled URL. Let's drop the "controlled" word and just have it as "special renderer URL". https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2675: // - Navigating to a special, renderer controlled URL. Same comment as above. https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2679: return true; So this method always returns true? Why is it then that we do a CHECK on its result? It will never hit. Don't we want to return false if none of the above is the case?
Patchset #37 (id:840001) has been deleted
Patchset #37 (id:860001) has been deleted
Patchset #36 (id:820001) has been deleted
Thanks. Good comments that made me identify a few more issues that needed fixing. https://codereview.chromium.org/1352813006/diff/720001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1352813006/diff/720001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2026: ResetWebUI(); On 2015/11/04 17:16:10, nasko (slow to review) wrote: > On 2015/11/03 09:50:30, carlosk wrote: > > On 2015/11/02 17:00:36, nasko (slow to review) wrote: > > > Can't we early return from here? It will make also the code below less > > indented. > > > > No because of the bindings checks further below. Unless I extract parts of > this > > back into separate method. > > No, let's keep it all in one place, as bindings checks will be moving around in > the future. Acknowledged. https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1082: speculative_render_frame_host_->web_ui()->RenderViewCreated( On 2015/11/04 17:16:10, nasko (slow to review) wrote: > The comment above says reuser, yet the call is to RenderViewCreated. If we still > call this method, it seems to me that the code to UpdateWebUI can be pulled > after the if statement, as it is identical and the checks are very similar. We reuse the speculative RFH but if a new WebUI is created it will be the first time it interacts with the speculative RFH's RVH, hence the call to RenderViewCreated. But indeed that makes so that we can merge the two versions of this update-and-notify logic as you described. Nice catch. Done. https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2413: frame_tree_node_->IsMainFrame()); On 2015/11/04 17:16:10, nasko (slow to review) wrote: > All of the new code here is very similar to the code for PlzNavigate. I'm not > sure if it is worthwhile trying to consolidate it, but it will likely be useful > to put a comment that if anyone is changing this code, should also change the > PlzNavigate version of it. My first look at consolidating this made me choose not to. But on a 2nd look, now that we have the single GetNavigatingWebUI, it looks much nicer! Also I think a comment like that is easily overlooked. So done! https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2647: // Switching WebUI from one type to another is never acceptable. On 2015/11/04 17:16:10, nasko (slow to review) wrote: > It is acceptable, as long as we change RFHs, right? This method only cares about WebUI transitions on the current RFH (see comment above and documentation in header file) so no, switching types is never acceptable, hence the DCHECK below. https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2655: // - A restore navigation to a WebUI URL. On 2015/11/04 17:16:10, nasko (slow to review) wrote: > Let's leave an empty line between the different cases, as it is easier to > visually distinguish them. Done. https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2664: // - Navigating back from a special renderer controlled URL. On 2015/11/04 17:16:10, nasko (slow to review) wrote: > Let's drop the "controlled" word and just have it as "special renderer URL". Done. https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2675: // - Navigating to a special, renderer controlled URL. On 2015/11/04 17:16:10, nasko (slow to review) wrote: > Same comment as above. Done. https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2679: return true; On 2015/11/04 17:16:10, nasko (slow to review) wrote: > So this method always returns true? Why is it then that we do a CHECK on its > result? It will never hit. Don't we want to return false if none of the above is > the case? This made sense then these were called from inside DCHECKs: a) Returning a bool allowed me to keep then in the DCHECK and so only be called from debug builds b) DCHECK-ing from inside them would make it easier to identify what case was failing. But now that it is in fact called from a CHECK those reasons are not valid anymore. Furthermore having these DCHECKs inside are wrong. I renamed the method and made it consistent again.
LGTM with a couple of nits. https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1082: speculative_render_frame_host_->web_ui()->RenderViewCreated( On 2015/11/04 21:43:59, carlosk wrote: > On 2015/11/04 17:16:10, nasko (slow to review) wrote: > > The comment above says reuser, yet the call is to RenderViewCreated. If we > still > > call this method, it seems to me that the code to UpdateWebUI can be pulled > > after the if statement, as it is identical and the checks are very similar. > > We reuse the speculative RFH but if a new WebUI is created it will be the first > time it interacts with the speculative RFH's RVH, hence the call to > RenderViewCreated. > > But indeed that makes so that we can merge the two versions of this > update-and-notify logic as you described. Nice catch. > > Done. > Awesome! https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2413: frame_tree_node_->IsMainFrame()); On 2015/11/04 21:43:59, carlosk wrote: > On 2015/11/04 17:16:10, nasko (slow to review) wrote: > > All of the new code here is very similar to the code for PlzNavigate. I'm not > > sure if it is worthwhile trying to consolidate it, but it will likely be > useful > > to put a comment that if anyone is changing this code, should also change the > > PlzNavigate version of it. > > My first look at consolidating this made me choose not to. But on a 2nd look, > now that we have the single GetNavigatingWebUI, it looks much nicer! Also I > think a comment like that is easily overlooked. > > So done! Acknowledged. https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2647: // Switching WebUI from one type to another is never acceptable. On 2015/11/04 21:43:59, carlosk wrote: > On 2015/11/04 17:16:10, nasko (slow to review) wrote: > > It is acceptable, as long as we change RFHs, right? > > This method only cares about WebUI transitions on the current RFH (see comment > above and documentation in header file) so no, switching types is never > acceptable, hence the DCHECK below. Let's clarify it in the comment then. Appending " for the same RenderFrameHost." https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2679: return true; On 2015/11/04 21:43:59, carlosk wrote: > On 2015/11/04 17:16:10, nasko (slow to review) wrote: > > So this method always returns true? Why is it then that we do a CHECK on its > > result? It will never hit. Don't we want to return false if none of the above > is > > the case? > > This made sense then these were called from inside DCHECKs: > a) Returning a bool allowed me to keep then in the DCHECK and so only be called > from debug builds > b) DCHECK-ing from inside them would make it easier to identify what case was > failing. > > But now that it is in fact called from a CHECK those reasons are not valid > anymore. Furthermore having these DCHECKs inside are wrong. > > I renamed the method and made it consistent again. Cool! https://codereview.chromium.org/1352813006/diff/880001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/1352813006/diff/880001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:708: void UpdateWebUIOnCurrentFrameHost(const GURL& dest_url, Can you add a comment about what this method does?
Thanks. Final changes are in and checking Commit now... https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:2647: // Switching WebUI from one type to another is never acceptable. On 2015/11/05 00:36:38, nasko (slow to review) wrote: > On 2015/11/04 21:43:59, carlosk wrote: > > On 2015/11/04 17:16:10, nasko (slow to review) wrote: > > > It is acceptable, as long as we change RFHs, right? > > > > This method only cares about WebUI transitions on the current RFH (see comment > > above and documentation in header file) so no, switching types is never > > acceptable, hence the DCHECK below. > > Let's clarify it in the comment then. Appending " for the same RenderFrameHost." Done. https://codereview.chromium.org/1352813006/diff/880001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/1352813006/diff/880001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:708: void UpdateWebUIOnCurrentFrameHost(const GURL& dest_url, On 2015/11/05 00:36:38, nasko (slow to review) wrote: > Can you add a comment about what this method does? Done.
The CQ bit was checked by carlosk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/1352813006/#ps900001 (title: "Minor comment updates.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1352813006/900001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1352813006/900001
Message was sent while issue was closed.
Committed patchset #37 (id:900001)
Message was sent while issue was closed.
Patchset 37 (id:??) landed as https://crrev.com/e5c678bf5b084988a60a4636342414f2831c1695 Cr-Commit-Position: refs/heads/master@{#358050}
Message was sent while issue was closed.
A revert of this CL (patchset #37 id:900001) has been created in https://codereview.chromium.org/1422043007/ by carlosk@chromium.org. The reason for reverting is: Due to crashes in RenderFrameHostManager::CheckWebUITransition.. |