Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10377170:
Browser Plugin: browser process side changes (Closed)
|
Created:
8 years, 7 months ago by Fady Samuel Modified:
8 years, 7 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jochen+watch-content_chromium.org Visibility:
Public. |
DescriptionBrowser Plugin: browser process side changes
Major classes:
BrowserPluginHost: This is the host-side of both the guest and the embedder
renderers. The embedder BrowserPluginHost keeps track of its guests, and cleans
them up when it navigates away, or whe tabs are switched or when it crashes. The
guest BrowserPluginHost knows about its embdder, and about its instance ID, as well
as some initialization parameters such as URL and initial size.
BUG=117897
TEST=manually
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=138475
Patch Set 1 #Patch Set 2 : Removed example #Patch Set 3 : More comments #Patch Set 4 : Updated to not expose GetPendingRenderViewHost #
Total comments: 26
Patch Set 5 : Updated according to creis@ #
Total comments: 25
Patch Set 6 : Updated according to creis@ #
Total comments: 8
Patch Set 7 : Updated according to jam@ #
Total comments: 16
Patch Set 8 : Clean-up according to creis@ #
Total comments: 1
Patch Set 9 : fixed nit #Messages
Total messages: 25 (0 generated)
Hi all, jam@: Please look at style, layering and overall structure. I left WebContentsImpL::GetPendingRenderViewHost because it felt less brittle to me than updating the BrowserPluginHost on changes to the pending RenderViewHost. I have to touch many more places to make sure that the pending_render_view_host is not stale in BrowserPluginHost. This doesn't seem unprecedented, there's already a GetPendingSiteInstance exposed, and I can use that instead of introducing GetPendingRenderViewHost, I think. mmocny@: Please look at BrowserPluginHost::Observe, does that make sense? We "hide" guests if the embedder is hidden. Is there any practical value to doing this? creis@: Please look at overall structure. The last changes I made made OnMapInstance fairly small and not particular useful. I might remove this call all together and have the guest talk directly to the embedder but I'd like to leave that to a separate patch in order to expedite landing this code. This patch depends on landing: http://codereview.chromium.org/9968097/
On 2012/05/16 16:22:09, Fady Samuel wrote: > Hi all, > > jam@: Please look at style, layering and overall structure. I left > WebContentsImpL::GetPendingRenderViewHost because it felt less brittle to me > than updating the BrowserPluginHost on changes to the pending RenderViewHost. I > have to touch many more places to make sure that the pending_render_view_host is > not stale in BrowserPluginHost. This doesn't seem unprecedented, there's already > a GetPendingSiteInstance exposed, and I can use that instead of introducing > GetPendingRenderViewHost, I think. for all the various reasons that were mentioned before, exposing pending RVH is not possible. > > mmocny@: Please look at BrowserPluginHost::Observe, does that make sense? We > "hide" guests if the embedder is hidden. Is there any practical value to doing > this? > > creis@: Please look at overall structure. The last changes I made made > OnMapInstance fairly small and not particular useful. I might remove this call > all together and have the guest talk directly to the embedder but I'd like to > leave that to a separate patch in order to expedite landing this code. > > This patch depends on landing: http://codereview.chromium.org/9968097/
Fady: Hiding guests is absolutely necessary for resource management (as well as to pause rendering and decrease process priority etc, just like any other tab). Furthermore, we should have a discussion about how to identify guests so that they can share allocations with their embedder (or at least have lower prioritization) in the GpuMemoryManager. The Observe function itself looks fine, but I think there may be memory issues if an app has very many browser tabs (which is currently nearly equivalent to having very many browser windows, right?). On 2012/05/16 16:29:48, John Abd-El-Malek wrote: > On 2012/05/16 16:22:09, Fady Samuel wrote: > > Hi all, > > > > jam@: Please look at style, layering and overall structure. I left > > WebContentsImpL::GetPendingRenderViewHost because it felt less brittle to me > > than updating the BrowserPluginHost on changes to the pending RenderViewHost. > I > > have to touch many more places to make sure that the pending_render_view_host > is > > not stale in BrowserPluginHost. This doesn't seem unprecedented, there's > already > > a GetPendingSiteInstance exposed, and I can use that instead of introducing > > GetPendingRenderViewHost, I think. > > for all the various reasons that were mentioned before, exposing pending RVH is > not possible. > > > > > mmocny@: Please look at BrowserPluginHost::Observe, does that make sense? We > > "hide" guests if the embedder is hidden. Is there any practical value to doing > > this? > > > > creis@: Please look at overall structure. The last changes I made made > > OnMapInstance fairly small and not particular useful. I might remove this call > > all together and have the guest talk directly to the embedder but I'd like to > > leave that to a separate patch in order to expedite landing this code. > > > > This patch depends on landing: http://codereview.chromium.org/9968097/
On 2012/05/16 17:20:25, mmocny wrote: > Fady: Hiding guests is absolutely necessary for resource management (as well as > to pause rendering and decrease process priority etc, just like any other tab). > Furthermore, we should have a discussion about how to identify guests so that > they can share allocations with their embedder (or at least have lower > prioritization) in the GpuMemoryManager. > > The Observe function itself looks fine, but I think there may be memory issues > if an app has very many browser tabs (which is currently nearly equivalent to > having very many browser windows, right?). > An app with a lot of browser tags is a similar problem to having many many tabs.
On 2012/05/16 21:14:30, Fady Samuel wrote: > On 2012/05/16 17:20:25, mmocny wrote: > > Fady: Hiding guests is absolutely necessary for resource management (as well > as > > to pause rendering and decrease process priority etc, just like any other > tab). > > Furthermore, we should have a discussion about how to identify guests so that > > they can share allocations with their embedder (or at least have lower > > prioritization) in the GpuMemoryManager. > > > > The Observe function itself looks fine, but I think there may be memory issues > > if an app has very many browser tabs (which is currently nearly equivalent to > > having very many browser windows, right?). > > > > An app with a lot of browser tags is a similar problem to having many many tabs. Sorry, pressed send too early: Sure, what plumbing do you need?
On 2012/05/16 16:29:48, John Abd-El-Malek wrote: > On 2012/05/16 16:22:09, Fady Samuel wrote: > > Hi all, > > > > jam@: Please look at style, layering and overall structure. I left > > WebContentsImpL::GetPendingRenderViewHost because it felt less brittle to me > > than updating the BrowserPluginHost on changes to the pending RenderViewHost. > I > > have to touch many more places to make sure that the pending_render_view_host > is > > not stale in BrowserPluginHost. This doesn't seem unprecedented, there's > already > > a GetPendingSiteInstance exposed, and I can use that instead of introducing > > GetPendingRenderViewHost, I think. > > for all the various reasons that were mentioned before, exposing pending RVH is > not possible. > > > > > mmocny@: Please look at BrowserPluginHost::Observe, does that make sense? We > > "hide" guests if the embedder is hidden. Is there any practical value to doing > > this? > > > > creis@: Please look at overall structure. The last changes I made made > > OnMapInstance fairly small and not particular useful. I might remove this call > > all together and have the guest talk directly to the embedder but I'd like to > > leave that to a separate patch in order to expedite landing this code. > > > > This patch depends on landing: http://codereview.chromium.org/9968097/ Done. Not exposing GetPendingRenderViewHost anymore. I'm not entirely sure this will work correctly in all cases, however.
On 2012/05/16 21:15:29, Fady Samuel wrote: > On 2012/05/16 21:14:30, Fady Samuel wrote: > > On 2012/05/16 17:20:25, mmocny wrote: > > > Fady: Hiding guests is absolutely necessary for resource management (as > well > > as > > > to pause rendering and decrease process priority etc, just like any other > > tab). > > > Furthermore, we should have a discussion about how to identify guests so > that > > > they can share allocations with their embedder (or at least have lower > > > prioritization) in the GpuMemoryManager. > > > > > > The Observe function itself looks fine, but I think there may be memory > issues > > > if an app has very many browser tabs (which is currently nearly equivalent > to > > > having very many browser windows, right?). > > > > > > > An app with a lot of browser tags is a similar problem to having many many > tabs. > Many *visible* tabs (i.e. windows), which is less common than many background tabs. > Sorry, pressed send too early: Sure, what plumbing do you need? I think we can add that in a subsequent patch, we should discuss.
https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/bro... File content/browser/browser_plugin/browser_plugin_host.cc (right): https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/bro... content/browser/browser_plugin/browser_plugin_host.cc:30: web_contents->GetRenderViewHost())); This needs another space of indent. https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/bro... content/browser/browser_plugin/browser_plugin_host.cc:78: web_contents()->GetRenderViewHost()->GetView()->SetSize( This isn't safe. We need to know who sent us the message so that it gets delivered to the right RenderViewHost. For example, the active RVH could have changed since the message was sent. Could RenderViewHostImpl itself receive and process this message? https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/bro... content/browser/browser_plugin/browser_plugin_host.cc:97: web_contents()->GetSiteInstance(), Also not safe. The SiteInstance could have just changed. We need to know more about who is sending the message. https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/bro... content/browser/browser_plugin/browser_plugin_host.cc:113: guest_observer->web_contents()->GetController().LoadURL( I'm skeptical about this and the one below. Where are we getting a string from that is causing us to navigate? It's not consistent with the other ways that we initiate navigations. https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/bro... content/browser/browser_plugin/browser_plugin_host.cc:116: PAGE_TRANSITION_HOME_PAGE, I don't think Home Page is the right transition here. https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/bro... content/browser/browser_plugin/browser_plugin_host.cc:141: guest_web_contents->GetRenderViewHost(); No, this isn't safe for the reasons stated above. We can't assume that the message came from the pending RVH just because one exists. We need to know which RVH it came from. I think it would be safer to process these messages in RenderViewHostImpl, and if we need to call a method here to handle them, then pass in "this". For examples, see OnMsgNavigate, OnMsgUpdateState, OnMsgUpdateTitle, etc. https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/bro... File content/browser/browser_plugin/browser_plugin_host.h (right): https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/bro... content/browser/browser_plugin/browser_plugin_host.h:25: // on navigation, crashes, and 'hides" guests when it hides. nit: 'hides" -> "hides" DRAFT: Should that be swaps out instead of hides? https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/bro... content/browser/browser_plugin/browser_plugin_host.h:40: // Associate a guest by its container ID. nit: by -> with https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/bro... content/browser/browser_plugin/browser_plugin_host.h:61: void set_url(const GURL& url) { url_ = url; } This makes me nervous. Why does the BrowserPluginHost need to keep track of the URL? This is something that can change in a lot of ways and mean different things at different points in time (e.g., while loads are pending). Best to leave it within WebContents if possible, to avoid any risk of spoofing-like bugs. https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/bro... content/browser/browser_plugin/browser_plugin_host.h:68: void OnCrossProcessNavigation(RenderViewHost* dest_rvh); This naming is unclear: is it when the navigation starts or finishes? https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/bro... content/browser/browser_plugin/browser_plugin_host.h:116: RenderViewHost* pending_render_view_host_; I'm still not a fan of keeping track of this outside WebContentsImpl. I'd rather see anything that depends on it get pushed inside RenderViewHostImpl or WebContentsImpl if possible. https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/web... File content/browser/web_contents/web_contents_impl.cc (right): https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/web... content/browser/web_contents/web_contents_impl.cc:1136: browser_plugin_host()->OnCrossProcessNavigation(dest_render_view_host); Should avoid this. https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/web... File content/browser/web_contents/web_contents_impl.h (right): https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/web... content/browser/web_contents/web_contents_impl.h:602: // Manages the browser plugin instances hosted by this WebContents nit: End with a period.
Addressed all but MapInstance issues. This should be a message that goes through the pepper channel. But since this probably requires a change to PluginInstance, I'd prefer deferring this to a subsequent cl. https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/bro... File content/browser/browser_plugin/browser_plugin_host.cc (right): https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/bro... content/browser/browser_plugin/browser_plugin_host.cc:30: web_contents->GetRenderViewHost())); On 2012/05/17 21:24:05, creis wrote: > This needs another space of indent. Done. https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/bro... content/browser/browser_plugin/browser_plugin_host.cc:78: web_contents()->GetRenderViewHost()->GetView()->SetSize( On 2012/05/17 21:24:05, creis wrote: > This isn't safe. We need to know who sent us the message so that it gets > delivered to the right RenderViewHost. For example, the active RVH could have > changed since the message was sent. > > Could RenderViewHostImpl itself receive and process this message? Done. https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/bro... content/browser/browser_plugin/browser_plugin_host.cc:97: web_contents()->GetSiteInstance(), On 2012/05/17 21:24:05, creis wrote: > Also not safe. The SiteInstance could have just changed. We need to know more > about who is sending the message. Done. https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/bro... content/browser/browser_plugin/browser_plugin_host.cc:113: guest_observer->web_contents()->GetController().LoadURL( On 2012/05/17 21:24:05, creis wrote: > I'm skeptical about this and the one below. Where are we getting a string from > that is causing us to navigate? It's not consistent with the other ways that we > initiate navigations. The string is coming from the browser plugin's src attribute on the embedder page. https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/bro... content/browser/browser_plugin/browser_plugin_host.cc:116: PAGE_TRANSITION_HOME_PAGE, On 2012/05/17 21:24:05, creis wrote: > I don't think Home Page is the right transition here. changed to PAGE_TRANSITION_AUTO_SUBFRAME as per offline discussion. https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/bro... content/browser/browser_plugin/browser_plugin_host.cc:141: guest_web_contents->GetRenderViewHost(); On 2012/05/17 21:24:05, creis wrote: > No, this isn't safe for the reasons stated above. We can't assume that the > message came from the pending RVH just because one exists. We need to know > which RVH it came from. > > I think it would be safer to process these messages in RenderViewHostImpl, and > if we need to call a method here to handle them, then pass in "this". For > examples, see OnMsgNavigate, OnMsgUpdateState, OnMsgUpdateTitle, etc. This is tricky. OnMapInstance is coming from the embedder RenderView. guest_web_contents is a different RenderView in a different process that's in the process of navigating. I don't think this can be simply moved to RenderViewHostImpl. I added a TODO explaining what I think is the correct solution but it will probably require a change to Pepper's PluginInstance so that the embedder can send a message to the guest via the pepper channel to "CompleteNavigation" which basically means associate the PP_Instance with the RenderViewImpl, request input events and initialize the graphics context. https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/bro... File content/browser/browser_plugin/browser_plugin_host.h (right): https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/bro... content/browser/browser_plugin/browser_plugin_host.h:25: // on navigation, crashes, and 'hides" guests when it hides. On 2012/05/17 21:24:05, creis wrote: > nit: 'hides" -> "hides" > DRAFT: Should that be swaps out instead of hides? Fixed. That will make tabbing back to the embedder render view less responsive because it will have to swap in the guest RenderViews again? https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/bro... content/browser/browser_plugin/browser_plugin_host.h:40: // Associate a guest by its container ID. On 2012/05/17 21:24:05, creis wrote: > nit: by -> with Done. https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/web... File content/browser/web_contents/web_contents_impl.cc (right): https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/web... content/browser/web_contents/web_contents_impl.cc:1136: browser_plugin_host()->OnCrossProcessNavigation(dest_render_view_host); On 2012/05/17 21:24:05, creis wrote: > Should avoid this. See TODO above BrowserPluginHost::OnMapInstance. I'd like to fix this issue in a separate cl, if you're OK with that. I have a fairly good idea of how to do it. https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/web... File content/browser/web_contents/web_contents_impl.h (right): https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/web... content/browser/web_contents/web_contents_impl.h:602: // Manages the browser plugin instances hosted by this WebContents On 2012/05/17 21:24:05, creis wrote: > nit: End with a period. Done.
I would have preferred to take care of the OnMapInstance change in this CL, but I'll be ok with a TODO and bug for it if necessary. https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/bro... File content/browser/browser_plugin/browser_plugin_host.cc (right): https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/bro... content/browser/browser_plugin/browser_plugin_host.cc:113: guest_observer->web_contents()->GetController().LoadURL( On 2012/05/18 17:37:52, Fady Samuel wrote: > On 2012/05/17 21:24:05, creis wrote: > > I'm skeptical about this and the one below. Where are we getting a string > from > > that is causing us to navigate? It's not consistent with the other ways that > we > > initiate navigations. > > The string is coming from the browser plugin's src attribute on the embedder > page. Ok, I think it'll do for now. https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/bro... File content/browser/browser_plugin/browser_plugin_host.h (right): https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/bro... content/browser/browser_plugin/browser_plugin_host.h:25: // on navigation, crashes, and 'hides" guests when it hides. On 2012/05/18 17:37:52, Fady Samuel wrote: > On 2012/05/17 21:24:05, creis wrote: > > nit: 'hides" -> "hides" > > DRAFT: Should that be swaps out instead of hides? > > Fixed. That will make tabbing back to the embedder render view less responsive > because it will have to swap in the guest RenderViews again? Ah, sorry about the draft comment. I was going to check whether "hiding" was referring to simply hiding the tab in favor of another or to navigating away. I think it's fine as is. https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/web... File content/browser/web_contents/web_contents_impl.cc (right): https://chromiumcodereview.appspot.com/10377170/diff/6001/content/browser/web... content/browser/web_contents/web_contents_impl.cc:1136: browser_plugin_host()->OnCrossProcessNavigation(dest_render_view_host); On 2012/05/18 17:37:52, Fady Samuel wrote: > On 2012/05/17 21:24:05, creis wrote: > > Should avoid this. > > See TODO above BrowserPluginHost::OnMapInstance. I'd like to fix this issue in a > separate cl, if you're OK with that. I have a fairly good idea of how to do it. Please put a TODO here as well, since we can't keep this line. It's not necessarily a cross-process navigation at this point, and we don't want to encourage others to listen in a similar way. https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/br... File content/browser/browser_plugin/browser_plugin_host.cc (right): https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/br... content/browser/browser_plugin/browser_plugin_host.cc:32: web_contents->GetRenderViewHost())); Just want to sanity check that we know this is the right RenderViewHost. Which web_contents is this, and are we certain that it hasn't changed its RVH already? https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/br... content/browser/browser_plugin/browser_plugin_host.cc:101: guest_observer->set_url(url); Does the observer need to know the url? The fewer things that keep it as state the better. https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/br... content/browser/browser_plugin/browser_plugin_host.cc:127: // TODO(fsamuel): This handler is all kinds of bad and could be racy. Please add more of an explanation why this is racy: the message could be coming from either the current or pending RVH, whether or not there is a pending one. I want to make sure we don't forget about this TODO, so please file a bug for it and include the number here as well. https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/br... content/browser/browser_plugin/browser_plugin_host.cc:154: embedder()->GetRenderProcessHost()->Send( This looks unsafe as well, as does the GetPendingSiteInstance call below. Can we pass in the actual RenderProcessHost(s) or their IDs instead? Sorry to be strict on this, but I've spent much too long debugging races from problems like this. https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/br... content/browser/browser_plugin/browser_plugin_host.cc:157: //embedder_web_contents->GetRenderProcessHost()->GetID(), Remove commented code. https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/br... File content/browser/browser_plugin/browser_plugin_host.h (right): https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/br... content/browser/browser_plugin/browser_plugin_host.h:34: BrowserPluginHost(WebContentsImpl* web_contents); Please add a comment saying which web_contents this is, the embedder or the guest. https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/br... content/browser/browser_plugin/browser_plugin_host.h:63: void set_url(const GURL& url) { url_ = url; } Please remove url_, since this isn't used in the patch and doesn't seem like a concept this class should know about. https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/br... content/browser/browser_plugin/browser_plugin_host.h:70: void OnCrossProcessNavigation(RenderViewHost* dest_rvh); Add TODO to remove, referencing OnMapInstance. Also change the name to OnPendingNavigation, since this isn't necessarily cross-process, and it happens on the start of the navigation and not the completion. https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/br... content/browser/browser_plugin/browser_plugin_host.h:82: virtual void DidCommitProvisionalLoadForFrame( Add TODO to remove, referencing OnMapInstance. https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/br... content/browser/browser_plugin/browser_plugin_host.h:118: RenderViewHost* pending_render_view_host_; Add TODO to remove, referencing OnMapInstance. We can't keep this concept here. https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/re... File content/browser/renderer_host/render_view_host_impl.cc (right): https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/re... content/browser/renderer_host/render_view_host_impl.cc:129: int render_view_id) { Wrong indent. https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/re... content/browser/renderer_host/render_view_host_impl.cc:1837: Remove blank line.
Updated. Note this cl depends on landing https://chromiumcodereview.appspot.com/10411033/ first. https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/br... File content/browser/browser_plugin/browser_plugin_host.cc (right): https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/br... content/browser/browser_plugin/browser_plugin_host.cc:32: web_contents->GetRenderViewHost())); On 2012/05/18 20:35:24, creis wrote: > Just want to sanity check that we know this is the right RenderViewHost. Which > web_contents is this, and are we certain that it hasn't changed its RVH already? Using the new NOTIFICATION_WEB_CONTENTS_VISIBLITY_CHANGE. https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/br... content/browser/browser_plugin/browser_plugin_host.cc:101: guest_observer->set_url(url); On 2012/05/18 20:35:24, creis wrote: > Does the observer need to know the url? The fewer things that keep it as state > the better. Done. Initial size will go away in a subsequent patch too. https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/br... content/browser/browser_plugin/browser_plugin_host.cc:127: // TODO(fsamuel): This handler is all kinds of bad and could be racy. On 2012/05/18 20:35:24, creis wrote: > Please add more of an explanation why this is racy: the message could be coming > from either the current or pending RVH, whether or not there is a pending one. > > I want to make sure we don't forget about this TODO, so please file a bug for it > and include the number here as well. I don't think that's the issue. The problem here isn't where the message is coming from (it's always coming from the embedder) but rather, where the message is going to: it needs to go to the new render process that we're navigating to. Between the time we start a new pending navigation and call MapInstance, it may be the case that the pending_render_view_host is no longer valid, I think. I'm really not sure what problems might arise here, but I'll file the bug to get rid of this potentially buggy way of handling things. https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/br... content/browser/browser_plugin/browser_plugin_host.cc:154: embedder()->GetRenderProcessHost()->Send( On 2012/05/18 20:35:24, creis wrote: > This looks unsafe as well, as does the GetPendingSiteInstance call below. Can > we pass in the actual RenderProcessHost(s) or their IDs instead? Sorry to be > strict on this, but I've spent much too long debugging races from problems like > this. Yes, the guest BrowserPluginHost can keep track of the embedder's RenderProcessHost on creation in NavigateGuestFromEmbedder. https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/br... content/browser/browser_plugin/browser_plugin_host.cc:157: //embedder_web_contents->GetRenderProcessHost()->GetID(), On 2012/05/18 20:35:24, creis wrote: > Remove commented code. Done. https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/br... File content/browser/browser_plugin/browser_plugin_host.h (right): https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/br... content/browser/browser_plugin/browser_plugin_host.h:34: BrowserPluginHost(WebContentsImpl* web_contents); On 2012/05/18 20:35:24, creis wrote: > Please add a comment saying which web_contents this is, the embedder or the > guest. Both. I added a comment to say that WebContentsImpl owns BrowserPluginHost. https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/br... content/browser/browser_plugin/browser_plugin_host.h:63: void set_url(const GURL& url) { url_ = url; } On 2012/05/18 20:35:24, creis wrote: > Please remove url_, since this isn't used in the patch and doesn't seem like a > concept this class should know about. Done. https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/br... content/browser/browser_plugin/browser_plugin_host.h:63: void set_url(const GURL& url) { url_ = url; } On 2012/05/18 20:35:24, creis wrote: > Please remove url_, since this isn't used in the patch and doesn't seem like a > concept this class should know about. Done. https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/br... content/browser/browser_plugin/browser_plugin_host.h:70: void OnCrossProcessNavigation(RenderViewHost* dest_rvh); On 2012/05/18 20:35:24, creis wrote: > Add TODO to remove, referencing OnMapInstance. Also change the name to > OnPendingNavigation, since this isn't necessarily cross-process, and it happens > on the start of the navigation and not the completion. Done. https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/br... content/browser/browser_plugin/browser_plugin_host.h:82: virtual void DidCommitProvisionalLoadForFrame( On 2012/05/18 20:35:24, creis wrote: > Add TODO to remove, referencing OnMapInstance. Unrelated. We discussed this offline. This is used to monitor frame navigations so that we can clean up guests appropriately. Added a comment to point this out. https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/br... content/browser/browser_plugin/browser_plugin_host.h:118: RenderViewHost* pending_render_view_host_; On 2012/05/18 20:35:24, creis wrote: > Add TODO to remove, referencing OnMapInstance. We can't keep this concept here. Done. https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/re... File content/browser/renderer_host/render_view_host_impl.cc (right): https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/re... content/browser/renderer_host/render_view_host_impl.cc:129: int render_view_id) { On 2012/05/18 20:35:24, creis wrote: > Wrong indent. Done. https://chromiumcodereview.appspot.com/10377170/diff/11002/content/browser/re... content/browser/renderer_host/render_view_host_impl.cc:1837: On 2012/05/18 20:35:24, creis wrote: > Remove blank line. Done.
there's a lot of plumbing in various classes to get the ConnectEmbedderToChannel/NavigateGuestFromEmbedder messages across. It's better to create a RenderViewHostObserver per RVH for the browser plugin feature. you can have BrowserPluginHost listen to NOTIFICATION_RENDER_VIEW_HOST_CREATED and create it then. the benefits is that you don't need to put all this plumbing in RVH, WCI, and RVHD. http://codereview.chromium.org/10377170/diff/18001/content/browser/browser_pl... File content/browser/browser_plugin/browser_plugin_host.h (right): http://codereview.chromium.org/10377170/diff/18001/content/browser/browser_pl... content/browser/browser_plugin/browser_plugin_host.h:12: #include "content/public/browser/notification_observer.h" nit: order (this is caught by lint btw, please look at the Lint column in rietveld after uploading) http://codereview.chromium.org/10377170/diff/18001/content/browser/browser_pl... content/browser/browser_plugin/browser_plugin_host.h:30: class BrowserPluginHost: public WebContentsObserver, nit: "BrowserPluginHost :" http://codereview.chromium.org/10377170/diff/18001/content/browser/browser_pl... content/browser/browser_plugin/browser_plugin_host.h:36: BrowserPluginHost(WebContentsImpl* web_contents); nit: explicit http://codereview.chromium.org/10377170/diff/18001/content/browser/browser_pl... content/browser/browser_plugin/browser_plugin_host.h:52: void AddGuest(WebContentsImpl* guest, int64 frame_id); please put all the methods that are only called within this class in the private section. having the public section as minimal as possible is good for readability, so that it's obvious what this class provides to others and what is internal implementation details
Moved stuff into BrowserPluginHostHelper to avoid plumbing in WebContentsImpl, RenderViewHostDelegate and RenderViewHostImpl. Thanks John! https://chromiumcodereview.appspot.com/10377170/diff/18001/content/browser/br... File content/browser/browser_plugin/browser_plugin_host.h (right): https://chromiumcodereview.appspot.com/10377170/diff/18001/content/browser/br... content/browser/browser_plugin/browser_plugin_host.h:12: #include "content/public/browser/notification_observer.h" On 2012/05/21 15:10:55, John Abd-El-Malek wrote: > nit: order (this is caught by lint btw, please look at the Lint column in > rietveld after uploading) Done. https://chromiumcodereview.appspot.com/10377170/diff/18001/content/browser/br... content/browser/browser_plugin/browser_plugin_host.h:30: class BrowserPluginHost: public WebContentsObserver, On 2012/05/21 15:10:55, John Abd-El-Malek wrote: > nit: "BrowserPluginHost :" Done. https://chromiumcodereview.appspot.com/10377170/diff/18001/content/browser/br... content/browser/browser_plugin/browser_plugin_host.h:36: BrowserPluginHost(WebContentsImpl* web_contents); On 2012/05/21 15:10:55, John Abd-El-Malek wrote: > nit: explicit Done. https://chromiumcodereview.appspot.com/10377170/diff/18001/content/browser/br... content/browser/browser_plugin/browser_plugin_host.h:52: void AddGuest(WebContentsImpl* guest, int64 frame_id); On 2012/05/21 15:10:55, John Abd-El-Malek wrote: > please put all the methods that are only called within this class in the private > section. having the public section as minimal as possible is good for > readability, so that it's obvious what this class provides to others and what is > internal implementation details Done.
Thanks, I like the new observer class. https://chromiumcodereview.appspot.com/10377170/diff/15010/content/browser/br... File content/browser/browser_plugin/browser_plugin_host.cc (right): https://chromiumcodereview.appspot.com/10377170/diff/15010/content/browser/br... content/browser/browser_plugin/browser_plugin_host.cc:236: new BrowserPluginHostHelper(this, render_view_host); Who owns this, and when will it be deleted? https://chromiumcodereview.appspot.com/10377170/diff/15010/content/browser/br... File content/browser/browser_plugin/browser_plugin_host.h (right): https://chromiumcodereview.appspot.com/10377170/diff/15010/content/browser/br... content/browser/browser_plugin/browser_plugin_host.h:29: // such as initial size. It's taken me a few readings of the code to understand that this class is used in both the embedder and the guest. That feels odd to me and might be worth reconsidering, but if there's enough overlap between the cases, it might make sense. Please try to clarify this comment, though, since the first sentence implies it's just used by the embedder. Do we have enough DCHECKs like the one in OnNavigateFromGuest to ensure that methods are only called in the right context? https://chromiumcodereview.appspot.com/10377170/diff/15010/content/browser/br... content/browser/browser_plugin/browser_plugin_host.h:47: void NavigateGuestFromEmbedder(RenderViewHost* render_view_host, Please add a comment to this explaining what it's intended use is. It sounds like it's simply for sending a navigate request to the guest, but really it creates a WebContents in the guest and then navigates it. I'm guessing it should only be called when first creating the guest. https://chromiumcodereview.appspot.com/10377170/diff/15010/content/browser/we... File content/browser/web_contents/web_contents_impl.cc (right): https://chromiumcodereview.appspot.com/10377170/diff/15010/content/browser/we... content/browser/web_contents/web_contents_impl.cc:1152: // Tell BrowserPluginHost about the pending cross-process navigation. Please add TODO to remove this and list the bug number.
lgtm http://codereview.chromium.org/10377170/diff/15010/content/browser/browser_pl... File content/browser/browser_plugin/browser_plugin_host.cc (right): http://codereview.chromium.org/10377170/diff/15010/content/browser/browser_pl... content/browser/browser_plugin/browser_plugin_host.cc:236: new BrowserPluginHostHelper(this, render_view_host); On 2012/05/21 18:20:44, creis wrote: > Who owns this, and when will it be deleted? RenderViewHostObservers get deleted automatically when a RVH goes away http://codereview.chromium.org/10377170/diff/15010/content/browser/browser_pl... File content/browser/browser_plugin/browser_plugin_host_helper.cc (right): http://codereview.chromium.org/10377170/diff/15010/content/browser/browser_pl... content/browser/browser_plugin/browser_plugin_host_helper.cc:11: #include "ipc/ipc_message_macros.h" don't include this, it comes by default through the messages.h file http://codereview.chromium.org/10377170/diff/15010/content/browser/browser_pl... content/browser/browser_plugin/browser_plugin_host_helper.cc:61: DCHECK(render_view_host()->GetView()); nit: no need to null check this, if it's null the next line will crash http://codereview.chromium.org/10377170/diff/15010/content/browser/browser_pl... File content/browser/browser_plugin/browser_plugin_host_helper.h (right): http://codereview.chromium.org/10377170/diff/15010/content/browser/browser_pl... content/browser/browser_plugin/browser_plugin_host_helper.h:44: DISALLOW_COPY_AND_ASSIGN(BrowserPluginHostHelper); nit: usually a blank line above this
https://chromiumcodereview.appspot.com/10377170/diff/15010/content/browser/br... File content/browser/browser_plugin/browser_plugin_host.cc (right): https://chromiumcodereview.appspot.com/10377170/diff/15010/content/browser/br... content/browser/browser_plugin/browser_plugin_host.cc:236: new BrowserPluginHostHelper(this, render_view_host); On 2012/05/21 18:30:24, John Abd-El-Malek wrote: > On 2012/05/21 18:20:44, creis wrote: > > Who owns this, and when will it be deleted? > > RenderViewHostObservers get deleted automatically when a RVH goes away Added a comment. https://chromiumcodereview.appspot.com/10377170/diff/15010/content/browser/br... File content/browser/browser_plugin/browser_plugin_host.h (right): https://chromiumcodereview.appspot.com/10377170/diff/15010/content/browser/br... content/browser/browser_plugin/browser_plugin_host.h:29: // such as initial size. On 2012/05/21 18:20:44, creis wrote: > It's taken me a few readings of the code to understand that this class is used > in both the embedder and the guest. That feels odd to me and might be worth > reconsidering, but if there's enough overlap between the cases, it might make > sense. Please try to clarify this comment, though, since the first sentence > implies it's just used by the embedder. > > Do we have enough DCHECKs like the one in OnNavigateFromGuest to ensure that > methods are only called in the right context? Added a sentence. In a completely general implementation (not quite there yet), a guest can also be an embedder (this wil be required for an OOP iframe). Yes, the appropriate DCHECK is in OnNavigateFromGuest. This is used by guest-initiated navigations. https://chromiumcodereview.appspot.com/10377170/diff/15010/content/browser/br... content/browser/browser_plugin/browser_plugin_host.h:47: void NavigateGuestFromEmbedder(RenderViewHost* render_view_host, On 2012/05/21 18:20:44, creis wrote: > Please add a comment to this explaining what it's intended use is. It sounds > like it's simply for sending a navigate request to the guest, but really it > creates a WebContents in the guest and then navigates it. I'm guessing it > should only be called when first creating the guest. Done. https://chromiumcodereview.appspot.com/10377170/diff/15010/content/browser/br... File content/browser/browser_plugin/browser_plugin_host_helper.cc (right): https://chromiumcodereview.appspot.com/10377170/diff/15010/content/browser/br... content/browser/browser_plugin/browser_plugin_host_helper.cc:11: #include "ipc/ipc_message_macros.h" On 2012/05/21 18:30:24, John Abd-El-Malek wrote: > don't include this, it comes by default through the messages.h file Done. https://chromiumcodereview.appspot.com/10377170/diff/15010/content/browser/br... content/browser/browser_plugin/browser_plugin_host_helper.cc:61: DCHECK(render_view_host()->GetView()); On 2012/05/21 18:30:24, John Abd-El-Malek wrote: > nit: no need to null check this, if it's null the next line will crash Done. https://chromiumcodereview.appspot.com/10377170/diff/15010/content/browser/br... File content/browser/browser_plugin/browser_plugin_host_helper.h (right): https://chromiumcodereview.appspot.com/10377170/diff/15010/content/browser/br... content/browser/browser_plugin/browser_plugin_host_helper.h:44: DISALLOW_COPY_AND_ASSIGN(BrowserPluginHostHelper); On 2012/05/21 18:30:24, John Abd-El-Malek wrote: > nit: usually a blank line above this Done. https://chromiumcodereview.appspot.com/10377170/diff/15010/content/browser/we... File content/browser/web_contents/web_contents_impl.cc (right): https://chromiumcodereview.appspot.com/10377170/diff/15010/content/browser/we... content/browser/web_contents/web_contents_impl.cc:1152: // Tell BrowserPluginHost about the pending cross-process navigation. On 2012/05/21 18:20:44, creis wrote: > Please add TODO to remove this and list the bug number. Done.
LGTM! https://chromiumcodereview.appspot.com/10377170/diff/15010/content/browser/br... File content/browser/browser_plugin/browser_plugin_host.h (right): https://chromiumcodereview.appspot.com/10377170/diff/15010/content/browser/br... content/browser/browser_plugin/browser_plugin_host.h:29: // such as initial size. On 2012/05/21 19:22:17, Fady Samuel wrote: > On 2012/05/21 18:20:44, creis wrote: > > It's taken me a few readings of the code to understand that this class is used > > in both the embedder and the guest. That feels odd to me and might be worth > > reconsidering, but if there's enough overlap between the cases, it might make > > sense. Please try to clarify this comment, though, since the first sentence > > implies it's just used by the embedder. > > > > Do we have enough DCHECKs like the one in OnNavigateFromGuest to ensure that > > methods are only called in the right context? > > Added a sentence. In a completely general implementation (not quite there yet), > a guest can also be an embedder (this wil be required for an OOP iframe). Ah, of course. > Yes, > the appropriate DCHECK is in OnNavigateFromGuest. This is used by > guest-initiated navigations. Thanks. https://chromiumcodereview.appspot.com/10377170/diff/21001/content/browser/br... File content/browser/browser_plugin/browser_plugin_host.cc (right): https://chromiumcodereview.appspot.com/10377170/diff/21001/content/browser/br... content/browser/browser_plugin/browser_plugin_host.cc:236: // BrowserPluginHostHelper is destoryed when its associated RenderViewHost nit: destroyed
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10377170/7006
Try job failure for 10377170-7006 (retry) on linux_chromeos for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10377170/7006
Try job failure for 10377170-7006 (retry) on linux_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10377170/7006
Try job failure for 10377170-7006 (retry) on win_rel for step "sync_unit_tests". It's a second try, previously, step "sync_unit_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10377170/7006
Try job failure for 10377170-7006 (retry) on win_rel for step "sync_unit_tests". It's a second try, previously, step "sync_unit_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... |
