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

Side by Side Diff: content/public/browser/browser_plugin_guest_delegate.cc

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

Patch Set: Rebased Created 4 years, 1 month ago

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

Jump to:

« 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 »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
1 // Copyright 2014 The Chromium Authors. All rights reserved.	1 // Copyright 2014 The Chromium Authors. All rights reserved.

2 // Use of this source code is governed by a BSD-style license that can be	2 // Use of this source code is governed by a BSD-style license that can be

3 // found in the LICENSE file.	3 // found in the LICENSE file.

4	4

5 #include "content/public/browser/browser_plugin_guest_delegate.h"	5 #include "content/public/browser/browser_plugin_guest_delegate.h"

6	6

7 namespace content {	7 namespace content {

8	8

9 bool BrowserPluginGuestDelegate::CanRunInDetachedState() const {	9 bool BrowserPluginGuestDelegate::CanRunInDetachedState() const {

10 return false;	10 return false;

(...skipping 18 matching lines...) Expand all Loading...
29	29

30 bool BrowserPluginGuestDelegate::HandleStopFindingForEmbedder(	30 bool BrowserPluginGuestDelegate::HandleStopFindingForEmbedder(

31 StopFindAction action) {	31 StopFindAction action) {

32 return false;	32 return false;

33 }	33 }

34	34

35 bool BrowserPluginGuestDelegate::CanUseCrossProcessFrames() {	35 bool BrowserPluginGuestDelegate::CanUseCrossProcessFrames() {

36 return true;	36 return true;

37 }	37 }

38	38

	39 RenderFrameHost* BrowserPluginGuestDelegate::GetEmbedderFrame() {

	40 if (GetOwnerWebContents())

	41 return GetOwnerWebContents()->GetMainFrame();
	nasko 2016/10/27 21:03:40 Why return the main frame here? Isn't this suppose 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? EhsanK 2016/10/27 22:06:48 The only way we can have A->B->C->GuestView right Show quoted text 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. nasko 2016/11/09 01:13:07 Why not? I don't think we have any code restrictio Show quoted text 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>. EhsanK 2016/11/15 19:54:22 I double checked and as you said three is ways to Show quoted text 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. Show quoted text > 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>). Charlie Reis 2016/11/15 23:45:32 This seems wrong to me. We shouldn't be returning Show quoted text 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.
	42 return nullptr;

	43 }

	44

39 } // namespace content	45 } // namespace content

OLD	NEW