content/public/browser/browser_plugin_guest_delegate.cc - Issue 2417693002: Allow MimeHandlerViewGuest be embedded inside OOPIFs

Unified Diff: content/public/browser/browser_plugin_guest_delegate.cc

Issue 2417693002: Allow MimeHandlerViewGuest be embedded inside OOPIFs (Closed)

Patch Set: Rebased Created 4 years, 2 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

« content/public/browser/browser_plugin_guest_delegate.h ('K') | « content/public/browser/browser_plugin_guest_delegate.h ('k') | extensions/browser/guest_view/extensions_guest_view_message_filter.cc » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: content/public/browser/browser_plugin_guest_delegate.cc

diff --git a/content/public/browser/browser_plugin_guest_delegate.cc b/content/public/browser/browser_plugin_guest_delegate.cc

index 732df238f91431509e6f998d56cc1508e9a0bdf3..affcdfb858bc3d8f53e3c7ed58d2757b23509354 100644

--- a/content/public/browser/browser_plugin_guest_delegate.cc

+++ b/content/public/browser/browser_plugin_guest_delegate.cc

@@ -36,4 +36,10 @@ bool BrowserPluginGuestDelegate::CanUseCrossProcessFrames() {

return true;

}

+RenderFrameHost* BrowserPluginGuestDelegate::GetEmbedderFrame() {

+ if (GetOwnerWebContents())

+ return GetOwnerWebContents()->GetMainFrame();

nasko 2016/10/27 21:03:40 Why return the main frame here? Isn't this suppose

EhsanK 2016/10/27 22:06:48 The only way we can have A->B->C->GuestView right

nasko 2016/11/09 01:13:07 Why not? I don't think we have any code restrictio

EhsanK 2016/11/15 19:54:22 I double checked and as you said three is ways to

I double checked and as you said three is ways to have <iframe>-ed <webview>, e.g., adding a html file to "web_accessible_resources" and creating <webview> inside <html>. But that <iframe> is not cross process. So the embedder frame will still be the |owner_web_contents_|'s frame.

But I don't think that is a cross-process frame. As long as it is an in-process frame the embedder could be the main frame which is in the same process (i.e., |owner_web_contents_|). I agree however that the actual embedder frame is different from the main frame. But I don't think that matters unless the in-process frame has its own RenderWidgetHost which should not be the case when we <iframe> in the scenario you explained (there is no middle remote frame embedding the <iframe>).

Charlie Reis 2016/11/15 23:45:32 This seems wrong to me. We shouldn't be returning

On 2016/11/15 19:54:22, EhsanK wrote: > On 2016/11/09 01:13:07, nasko wrote: > > On 2016/10/27 22:06:48, EhsanK wrote: > > > On 2016/10/27 21:03:40, nasko (slow) wrote: > > > > Why return the main frame here? Isn't this supposed to be the frame that > > > > contains the GuestView? If you have A->B->C->GuestView, why would we > return > > A > > > > instead of C? > > > > > > The only way we can have A->B->C->GuestView right now is to have <embed > > > src="foo.pdf"> inside the inner most <iframe>. For that, the override of > this > > > method in MimeHandlerViewGuest will return C's RenderFrameHost. > > > > > > I don't think we can have guests in <iframe> unless the guest is a PDF. But > I > > > would like to divert this to lfg@, lazyboy@, wjmaclean@ for confirmation. > > > > Why not? I don't think we have any code restriction enforcing that <webview> > > needs to be always in the top-level frame. Maybe the only enforcement is other > > code making the same assumption and not working otherwise. I still would like > to > > understand it, even though this code doesn't exist here anymore, but the same > is > > moved to GuestViewBase::GetEmbedderFrame. > > > > I see the addition of > > BrowserPluginGuestDelegate::CanBeEmbeddedInsideCrossProcessFrames, but it > > doesn't have to be a cross-process frame, just a regular App v2 with <iframe> > to > > another document in its crx, which ends up having a <webview>. > > I double checked and as you said three is ways to have <iframe>-ed <webview>, > e.g., adding a html file to "web_accessible_resources" and creating <webview> > inside <html>. But that <iframe> is not cross process. So the embedder frame > will still be the |owner_web_contents_|'s frame. > > > I see the addition of > > BrowserPluginGuestDelegate::CanBeEmbeddedInsideCrossProcessFrames, but it > > doesn't have to be a cross-process frame, just a regular App v2 with <iframe> > to > > another document in its crx, which ends up having a <webview>. > But I don't think that is a cross-process frame. As long as it is an in-process > frame the embedder could be the main frame which is in the same process (i.e., > |owner_web_contents_|). I agree however that the actual embedder frame is > different from the main frame. But I don't think that matters unless the > in-process frame has > its own RenderWidgetHost which should not be the case when we <iframe> in the > scenario you explained (there is no middle remote frame embedding the <iframe>).

This seems wrong to me. We shouldn't be returning the wrong frame in these cases, even if it shares a widget and process. Future use cases might care about the frame itself and get the wrong answer. As I'm noting on the GetOwnerFrame() declaration in PS20, I think we should have different APIs if what you really care about is the widget, process, and SiteInstance. We should avoid having a frame-based API if we're not returning the right frame.

+ return nullptr;

} // namespace content