Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2165523004:
Force MimeHandlerView to always use BrowserPlugin (Closed)
|
Created:
4 years, 5 months ago by EhsanK Modified:
4 years, 3 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, chromium-apps-reviews_chromium.org, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionForce MimeHandlerView to always use BrowserPlugin
Currently, with the flag --use-cross-process-frames-for-guests switched
on, MimeHandlerView still creates a BrowserPlugin while on the browser
side we expect an OOPIF based implementation. This causes PDF viewer to
crash.
On the other hand, GuestViews will soon use OOPIF. Hence, temporarily,
we need to make MimeHandlerView an exception and force it to use
BrowserPlugin's codepath even when the switch above is turned on.
BUG=563285
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/1beabeccb02b9c6c7e4706950dba275c95e18aa8
Cr-Commit-Position: refs/heads/master@{#420712}
Patch Set 1 #Patch Set 2 : Return Top-Level WebContents View's NativeView rather than Owners #
Total comments: 1
Patch Set 3 : Using |attached_| to verify if |delegate_| is valid (used in IsMimeHandlerViewGuest()) #Patch Set 4 : Using a boolean to determine whether or not BrowserPluginGuest is for MimeHandlerViewGuest #
Total comments: 7
Patch Set 5 : Addressing lfg@'s comments #Patch Set 6 : Rebased #
Total comments: 4
Patch Set 7 : Addressing lazyboy@'s and lfg@'s comments #
Total comments: 4
Patch Set 8 : Added BrowserPluginGuestDelegateMode::DelegateMode(FromWebContents)? #
Total comments: 8
Patch Set 9 : Rebased #Patch Set 10 : Addressing lfg@'s comments #Patch Set 11 : Fixed a typo #
Total comments: 16
Patch Set 12 : Addressing lfg@'s comments #Patch Set 13 : Rebased #Patch Set 14 : Two quick fixes (One outstanding from lfg@'s comments) #
Total comments: 41
Patch Set 15 : Addressing creis@'s Comments #Patch Set 16 : Fixed an Error in Code #
Total comments: 6
Patch Set 17 : Remove Usages of UseCrossProcessFramesForGuests in content/browser/* #Patch Set 18 : Fixed typos #
Total comments: 8
Patch Set 19 : Changed a (newly added) TEST_F to TEST_P #Patch Set 20 : Addressing creis@'s comments #Patch Set 21 : Added the missing files #
Total comments: 4
Patch Set 22 : Addressing creis@'s comments #
Total comments: 2
Messages
Total messages: 105 (58 generated)
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ekaramad@chromium.org changed reviewers: + lazyboy@chromium.org
Istiaque, since we are perhaps launching OOPIF-webview sooner than MimeHandlerView can be fixed, we need to circumvent this. Could you please review this and if it makes sense, also suggest a content reviewer?
ekaramad@chromium.org changed reviewers: + lfg@chromium.org
Adding lfg@. Lucas: Could you please take a look? Thanks!
On 2016/07/19 15:46:06, EhsanK wrote: > Adding lfg@. > Lucas: Could you please take a look? Thanks! Just one general question before I do a full pass. Is it possible to have a WebContents with both a MimeHandlerView and another GuestView (for example a <webview>)? I'm thinking something like a Chrome App with both a <webview> element and an embedded pdf.
On 2016/07/19 16:02:49, lfg wrote: > On 2016/07/19 15:46:06, EhsanK wrote: > > Adding lfg@. > > Lucas: Could you please take a look? Thanks! > > Just one general question before I do a full pass. Is it possible to have a > WebContents with both a MimeHandlerView and another GuestView (for example a > <webview>)? I'm thinking something like a Chrome App with both a <webview> > element and an embedded pdf. I think you are suggesting having an embedder WebContents with two guest WebContents one for <webview> and one for MimeHandlerView, right?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/07/19 16:09:41, EhsanK wrote: > On 2016/07/19 16:02:49, lfg wrote: > > On 2016/07/19 15:46:06, EhsanK wrote: > > > Adding lfg@. > > > Lucas: Could you please take a look? Thanks! > > > > Just one general question before I do a full pass. Is it possible to have a > > WebContents with both a MimeHandlerView and another GuestView (for example a > > <webview>)? I'm thinking something like a Chrome App with both a <webview> > > element and an embedded pdf. > > I think you are suggesting having an embedder WebContents with two guest > WebContents one for <webview> and one for MimeHandlerView, right? Right, is that possible? If it is, then does this patch works in that use case?
Description was changed from ========== Force MimeHandlerView to always use BrowserPlugin Currently, with the flag --use-cross-process-frames-for-guests switched on, MimeHandlerView still creates a BrowserPlugin while on the browser side we expect an OOPIF based implementation. This causes PDF viewer to crash. On the other hand, GuestViews will soon use OOPIF. Hence, temporarily, we need to make MimeHandlerView an exception and force it to use BrowserPlugin's codepath even when the switch above is turned on. BUG=563285 ========== to ========== Force MimeHandlerView to always use BrowserPlugin Currently, with the flag --use-cross-process-frames-for-guests switched on, MimeHandlerView still creates a BrowserPlugin while on the browser side we expect an OOPIF based implementation. This causes PDF viewer to crash. On the other hand, GuestViews will soon use OOPIF. Hence, temporarily, we need to make MimeHandlerView an exception and force it to use BrowserPlugin's codepath even when the switch above is turned on. BUG=563285 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Fixing some crashes. With the new fix we can now show a PDF inside a <webview> with use-cross-process... on. Lucas: I could have pdf and <webview> on the same page. The problem was <webview> navigating to PDF which was causing crashes as the native view for owner was not implemented (RWHVCF). https://codereview.chromium.org/2165523004/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/2165523004/diff/20001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_guest.cc:357: return guest_->GetTopLevelRenderWidgetHostView()->GetNativeView(); To be precise, I should also add UseCrossProcessFramesForGuest in the if condition. But I think we can simply only return top-level native view instead, given that the old logic was to return the owner's anyway.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/07/19 21:48:44, EhsanK wrote: > Fixing some crashes. > > With the new fix we can now show a PDF inside a <webview> with > use-cross-process... on. > > Lucas: I could have pdf and <webview> on the same page. The problem was > <webview> navigating to PDF which was causing crashes as the native view for > owner was not implemented (RWHVCF). I just tried patching in your CL and I get a browser crash when trying to load any PDF. The crash is because of an invalid cast from RenderWidgetHostViewChildFrame to RenderWidgetHostViewGuest. Have you tried running with --use-cross-process-frames-for-guests ?
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/07/20 15:57:52, lfg wrote: > On 2016/07/19 21:48:44, EhsanK wrote: > > Fixing some crashes. > > > > With the new fix we can now show a PDF inside a <webview> with > > use-cross-process... on. > > > > Lucas: I could have pdf and <webview> on the same page. The problem was > > <webview> navigating to PDF which was causing crashes as the native view for > > owner was not implemented (RWHVCF). > > I just tried patching in your CL and I get a browser crash when trying to load > any PDF. The crash is because of an invalid cast from > RenderWidgetHostViewChildFrame to RenderWidgetHostViewGuest. Have you tried > running with --use-cross-process-frames-for-guests ? Before, in BrowserPluginGuest::IsMimeHandlerViewGuest() I was using !!delegate_ to determine whether |delegate_| is actually alive before the call to delegate_->IsMimeHandlerViewGuest(). Unfortunately, during destruction |delegate_| becomes dangling (is this a bug?). This was failing one test WebViewTest.InterstitialTeardown. Instead I used |attached_| to make sure it is actually alive. But this messed with initialization since from the time BrowserPluginGuest is created to the time is actually "attached" |attached_| is false which was leading to creating a RWHVCF instead of a RWHVG somehow. By keeping a boolean the crash seems to be fixed and I believe the test should be fine as well.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Mostly looks good, just a couple of questions. https://codereview.chromium.org/2165523004/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/2165523004/diff/60001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_guest.cc:357: return guest_->GetTopLevelRenderWidgetHostView()->GetNativeView(); What's the problem when returning the OwnerRenderWidgetHostView() ? https://codereview.chromium.org/2165523004/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2165523004/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4992: (!GetBrowserPluginGuest() || Why do you need the null check? And you can use browser_plugin_guest_ directly, like the other functions around here.
PTAL. Thanks! https://codereview.chromium.org/2165523004/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/2165523004/diff/60001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_guest.cc:357: return guest_->GetTopLevelRenderWidgetHostView()->GetNativeView(); On 2016/07/20 19:49:57, lfg wrote: > What's the problem when returning the OwnerRenderWidgetHostView() ? When we show a PDF inside an OOPIF-<webview>, the owner's RWHV is a RWHVChildFrame. But RWHVCF::GetNativeView() is NOTIMPLEMENTED(). Now my question is, what stops us from using top-level in all scenarios? i.e., just return GetTopLevelRenderWidgetHost()->GetNativeView() regardless of whether this is a mime handler or not. https://codereview.chromium.org/2165523004/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2165523004/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4992: (!GetBrowserPluginGuest() || On 2016/07/20 19:49:57, lfg wrote: > Why do you need the null check? > > And you can use browser_plugin_guest_ directly, like the other functions around > here. |browser_plugin_guest_| is null when we are in an outer WebContents. When in an inner WebContents, we also want to make sure that the guest is MimeHandlerViewGuest. Following your suggestion, I am using |browser_plugin_guest_| rather than GetBrowserPluginGuest(). Thanks!
https://codereview.chromium.org/2165523004/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/2165523004/diff/60001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_guest.cc:357: return guest_->GetTopLevelRenderWidgetHostView()->GetNativeView(); On 2016/07/20 20:49:19, EhsanK wrote: > On 2016/07/20 19:49:57, lfg wrote: > > What's the problem when returning the OwnerRenderWidgetHostView() ? > > When we show a PDF inside an OOPIF-<webview>, the owner's RWHV is a > RWHVChildFrame. But RWHVCF::GetNativeView() is NOTIMPLEMENTED(). In that case, shouldn't IsMimeHandlerViewGuest() return false? > Now my question is, what stops us from using top-level in all scenarios? i.e., > just return GetTopLevelRenderWidgetHost()->GetNativeView() regardless of whether > this is a mime handler or not. Right, the other question I have is why are we calling GetNativeView() on the guest? I'd rather try to clean this up before adding GetTopLevelRenderWidgetHostView() to RenderWidgetHostViewGuest. https://codereview.chromium.org/2165523004/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2165523004/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4992: (!GetBrowserPluginGuest() || On 2016/07/20 20:49:19, EhsanK wrote: > On 2016/07/20 19:49:57, lfg wrote: > > Why do you need the null check? > > > > And you can use browser_plugin_guest_ directly, like the other functions > around > > here. > > |browser_plugin_guest_| is null when we are in an outer WebContents. When in an > inner WebContents, we also want to make sure that the guest is > MimeHandlerViewGuest. > > Following your suggestion, I am using |browser_plugin_guest_| rather than > GetBrowserPluginGuest(). Thanks! Acknowledged.
Lucas: I am not sure if we can easily get rid of GetNativeView() as there is applications of it in the code, e.g., ctor of RWHVG uses it to update screen info. Also used for context menus. I will dig more while I am waiting for lazyboy@ reviews and also James' patch to land a GetTopLevel... so that I can rebase/reuse his. https://codereview.chromium.org/2165523004/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/2165523004/diff/60001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_guest.cc:357: return guest_->GetTopLevelRenderWidgetHostView()->GetNativeView(); On 2016/07/20 21:05:34, lfg wrote: > On 2016/07/20 20:49:19, EhsanK wrote: > > On 2016/07/20 19:49:57, lfg wrote: > > > What's the problem when returning the OwnerRenderWidgetHostView() ? > > > > When we show a PDF inside an OOPIF-<webview>, the owner's RWHV is a > > RWHVChildFrame. But RWHVCF::GetNativeView() is NOTIMPLEMENTED(). > > In that case, shouldn't IsMimeHandlerViewGuest() return false? > > > Now my question is, what stops us from using top-level in all scenarios? > i.e., > > just return GetTopLevelRenderWidgetHost()->GetNativeView() regardless of > whether > > this is a mime handler or not. > > Right, the other question I have is why are we calling GetNativeView() on the > guest? I'd rather try to clean this up before adding > GetTopLevelRenderWidgetHostView() to RenderWidgetHostViewGuest. This is called from tests as well as used in sending screen rects. That being said, whether or not we keep this in, wjmclean@ is about to add something like GetTopLevelRWHV() to RWHVCF. I can then rebase after his CL lands.
https://codereview.chromium.org/2165523004/diff/100001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/2165523004/diff/100001/chrome/browser/rendere... chrome/browser/renderer_context_menu/render_view_context_menu.cc:510: if (!content::BrowserPluginGuestMode::UseCrossProcessFramesForGuests() || I think the CL has too many BPGuestMode::UseCrossProcessFramesForGuests() && IsMimeHandlerView... type of constructs, I suggest you consider adding a method BrowserPluginGuestMode::UseCrossProcessFramesForWebContents(WebContents*), since this is in content, you'd want to go through browser_plugin_delegate to decide whether it is mime handler view in addition to checking the switch value. Also there are other places than this CL diff where UseCrossProcessFramesForGuests is used, do we not want to update those places? https://codereview.chromium.org/2165523004/diff/100001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/2165523004/diff/100001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_guest.cc:356: if (guest_->IsMimeHandlerViewGuest()) I'm not sure I followed why this was needed. Please add a comment why GetOwnerRWHV doesn't work.
On 2016/07/21 19:53:06, EhsanK wrote: > Lucas: I am not sure if we can easily get rid of GetNativeView() as there is > applications of it in the code, e.g., ctor of RWHVG uses it to update screen > info. Also used for context menus. RWHVCF does not use it, so we should be able to get rid of it. But we don't need to do it on this CL. > > Right, the other question I have is why are we calling GetNativeView() on the > > guest? I'd rather try to clean this up before adding > > GetTopLevelRenderWidgetHostView() to RenderWidgetHostViewGuest. > > This is called from tests as well as used in sending screen rects. That being > said, whether or not we keep this in, wjmclean@ is about to add something like > GetTopLevelRWHV() to RWHVCF. I can then rebase after his CL lands. I'm still not convinced that this should be in RWHV. The whole design behind CrossProcessFrameConnector is to isolate RWHV from knowledge of the frame tree. Methods such as GetTopLevelRWHV() do not belong here. We should either figure out why GetOwner..() doesn't work and fix that or fix RWHVG to not rely on GetNativeView().
PTAL. I also made MimeHandlerViewTests run with OOPIF guest view flag. They all seem to pass on Widows. Thanks! https://codereview.chromium.org/2165523004/diff/100001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/2165523004/diff/100001/chrome/browser/rendere... chrome/browser/renderer_context_menu/render_view_context_menu.cc:510: if (!content::BrowserPluginGuestMode::UseCrossProcessFramesForGuests() || There is more instances of UseCross... but I found them not relevant. I will prepare a list of all those that I have not changed and we can discuss them offline. Also, I added a method to BPGM as suggested. It will get GuestView from WebContents and check if it is MimeHandlerViewGuest. However, this method cannot be used that much around the code base because the map which holds (web_contents, guest_view) is not updated from the get go. It happens sometime during initialization. https://codereview.chromium.org/2165523004/diff/100001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/2165523004/diff/100001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_guest.cc:356: if (guest_->IsMimeHandlerViewGuest()) On 2016/07/22 01:03:30, lazyboy wrote: > I'm not sure I followed why this was needed. > > Please add a comment why GetOwnerRWHV doesn't work. If we have --use-cross-process-frames-for-guests and create an app with a <webview> and set the webview.src to a pdf url, then: 1- webview will have a RenderWidgetHostViewChildFrame 2- webview is the embedder 3- PDF will have a RenderWidgetHostViewGuest. Now GetOwneRenderWidgetHostView()->GetNativeView() crashes because of the NOTREACHED() in RWHVCF. After discussion with Lucas we decided to implement GetNativeView() for RWHVCF instead. So this is reset to master.
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Just a couple of comments. https://codereview.chromium.org/2165523004/diff/120001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2165523004/diff/120001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:217: return frame_connector_->GetRootRenderWidgetHostView()->GetNativeView(); I think this should be using GetParentRenderWidgetHostView. https://codereview.chromium.org/2165523004/diff/120001/content/public/common/... File content/public/common/DEPS (right): https://codereview.chromium.org/2165523004/diff/120001/content/public/common/... content/public/common/DEPS:6: "+content/public/browser" This is wrong. We shouldn't be including browser into common. Please, add a new file in content/public/browser/ to do this check.
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Added a new function to check if a WebContents is either using or not using X-process frames, or none (not a guest). This hopefully helps with readability and removing it later should be easier. All MimeHandlerViewTests are passing locally. https://codereview.chromium.org/2165523004/diff/120001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2165523004/diff/120001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:217: return frame_connector_->GetRootRenderWidgetHostView()->GetNativeView(); On 2016/07/28 18:29:31, lfg wrote: > I think this should be using GetParentRenderWidgetHostView. Acknowledged. https://codereview.chromium.org/2165523004/diff/120001/content/public/common/... File content/public/common/DEPS (right): https://codereview.chromium.org/2165523004/diff/120001/content/public/common/... content/public/common/DEPS:6: "+content/public/browser" On 2016/07/28 18:29:31, lfg wrote: > This is wrong. We shouldn't be including browser into common. Please, add a new > file in content/public/browser/ to do this check. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2165523004/diff/140001/content/public/browser... File content/public/browser/browser_plugin_guest_delegate_mode.cc (right): https://codereview.chromium.org/2165523004/diff/140001/content/public/browser... content/public/browser/browser_plugin_guest_delegate_mode.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/2165523004/diff/140001/content/public/browser... File content/public/browser/browser_plugin_guest_delegate_mode.h (right): https://codereview.chromium.org/2165523004/diff/140001/content/public/browser... content/public/browser/browser_plugin_guest_delegate_mode.h:6: #define CONTENT_PUBLIC_COMMON_BROWSER_PLUGIN_GUEST_DELEGATE_MODE_H_ Update defines. https://codereview.chromium.org/2165523004/diff/140001/content/public/browser... content/public/browser/browser_plugin_guest_delegate_mode.h:15: class CONTENT_EXPORT BrowserPluginGuestDelegateMode { This name is confusing, it's not obvious what it means. Why not just use BrowserPluginGuestMode just like the other function? If it's because of the name clash, I think it's fine to just use a namespace here, the class doesn't really provide anything here. https://codereview.chromium.org/2165523004/diff/140001/content/public/browser... content/public/browser/browser_plugin_guest_delegate_mode.h:17: enum DelegateMode { This seems overly complicated and doesn't add much. What's the difference in behavior between NONE and NOT_USING_CROSS_PROCESS_FRAMES? There shouldn't be any. The enum makes the code harder to understand instead of easier.
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Apologies this patch took a while. Please take another look. Thanks! https://codereview.chromium.org/2165523004/diff/140001/content/public/browser... File content/public/browser/browser_plugin_guest_delegate_mode.cc (right): https://codereview.chromium.org/2165523004/diff/140001/content/public/browser... content/public/browser/browser_plugin_guest_delegate_mode.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/07/30 00:05:25, lfg wrote: > 2016 Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Answered comments. PTAL. https://codereview.chromium.org/2165523004/diff/140001/content/public/browser... File content/public/browser/browser_plugin_guest_delegate_mode.h (right): https://codereview.chromium.org/2165523004/diff/140001/content/public/browser... content/public/browser/browser_plugin_guest_delegate_mode.h:6: #define CONTENT_PUBLIC_COMMON_BROWSER_PLUGIN_GUEST_DELEGATE_MODE_H_ On 2016/07/30 00:05:25, lfg wrote: > Update defines. Done. https://codereview.chromium.org/2165523004/diff/140001/content/public/browser... content/public/browser/browser_plugin_guest_delegate_mode.h:15: class CONTENT_EXPORT BrowserPluginGuestDelegateMode { On 2016/07/30 00:05:25, lfg wrote: > This name is confusing, it's not obvious what it means. Why not just use > BrowserPluginGuestMode just like the other function? If it's because of the name > clash, I think it's fine to just use a namespace here, the class doesn't really > provide anything here. Done. https://codereview.chromium.org/2165523004/diff/140001/content/public/browser... content/public/browser/browser_plugin_guest_delegate_mode.h:17: enum DelegateMode { On 2016/07/30 00:05:25, lfg wrote: > This seems overly complicated and doesn't add much. What's the difference in > behavior between NONE and NOT_USING_CROSS_PROCESS_FRAMES? There shouldn't be > any. The enum makes the code harder to understand instead of easier. Done.
https://codereview.chromium.org/2165523004/diff/200001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/2165523004/diff/200001/chrome/browser/rendere... chrome/browser/renderer_context_menu/render_view_context_menu.cc:512: WebContents::FromRenderFrameHost(render_frame_host); No need to redefine web_contents. https://codereview.chromium.org/2165523004/diff/200001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/2165523004/diff/200001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:114: is_mime_handler_view_guest_ = delegate_->IsMimeHandlerViewGuest(); Let's initialize this with the other member variables instead of the constructor's body. https://codereview.chromium.org/2165523004/diff/200001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:377: RenderWidgetHostView* BrowserPluginGuest::GetTopLevelRenderWidgetHostView() { Same comment as above. https://codereview.chromium.org/2165523004/diff/200001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.h (right): https://codereview.chromium.org/2165523004/diff/200001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.h:160: RenderWidgetHostView* GetTopLevelRenderWidgetHostView(); I think we aren't using this anymore, right? https://codereview.chromium.org/2165523004/diff/200001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2165523004/diff/200001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:212: // TODO(ekarmaad): To accomodate MimeHandlerViewGuest while embedded inside Typo on your ldap. https://codereview.chromium.org/2165523004/diff/200001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2165523004/diff/200001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:1573: !BrowserPluginGuestMode::UseCrossProcessFramesForGuests())) { Can we write this as (browser_plugin_guest_ && !guest_mode::IsCrossProcessFrameGuest(this))? https://codereview.chromium.org/2165523004/diff/200001/content/content_browse... File content/content_browser.gypi (right): https://codereview.chromium.org/2165523004/diff/200001/content/content_browse... content/content_browser.gypi:150: 'public/browser/browser_plugin_guest_delegate.h', Typo. https://codereview.chromium.org/2165523004/diff/200001/content/public/browser... File content/public/browser/guest_mode.h (right): https://codereview.chromium.org/2165523004/diff/200001/content/public/browser... content/public/browser/guest_mode.h:5: #ifndef CONTENT_PUBLIC_COMMON_BROWSER_GUEST_MODE_H_ Fix include guards.
Thanks for the reviews Lucas. PTAL. https://codereview.chromium.org/2165523004/diff/200001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/2165523004/diff/200001/chrome/browser/rendere... chrome/browser/renderer_context_menu/render_view_context_menu.cc:512: WebContents::FromRenderFrameHost(render_frame_host); On 2016/09/02 18:29:40, lfg wrote: > No need to redefine web_contents. Done. https://codereview.chromium.org/2165523004/diff/200001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/2165523004/diff/200001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:114: is_mime_handler_view_guest_ = delegate_->IsMimeHandlerViewGuest(); On 2016/09/02 18:29:40, lfg wrote: > Let's initialize this with the other member variables instead of the > constructor's body. Done. https://codereview.chromium.org/2165523004/diff/200001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:377: RenderWidgetHostView* BrowserPluginGuest::GetTopLevelRenderWidgetHostView() { On 2016/09/02 18:29:40, lfg wrote: > Same comment as above. Done. https://codereview.chromium.org/2165523004/diff/200001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.h (right): https://codereview.chromium.org/2165523004/diff/200001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.h:160: RenderWidgetHostView* GetTopLevelRenderWidgetHostView(); On 2016/09/02 18:29:40, lfg wrote: > I think we aren't using this anymore, right? Acknowledged. https://codereview.chromium.org/2165523004/diff/200001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2165523004/diff/200001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:212: // TODO(ekarmaad): To accomodate MimeHandlerViewGuest while embedded inside On 2016/09/02 18:29:40, lfg wrote: > Typo on your ldap. Done. https://codereview.chromium.org/2165523004/diff/200001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2165523004/diff/200001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:1573: !BrowserPluginGuestMode::UseCrossProcessFramesForGuests())) { On 2016/09/02 18:29:40, lfg wrote: > Can we write this as (browser_plugin_guest_ && > !guest_mode::IsCrossProcessFrameGuest(this))? Yes. Thanks. https://codereview.chromium.org/2165523004/diff/200001/content/content_browse... File content/content_browser.gypi (right): https://codereview.chromium.org/2165523004/diff/200001/content/content_browse... content/content_browser.gypi:150: 'public/browser/browser_plugin_guest_delegate.h', On 2016/09/02 18:29:41, lfg wrote: > Typo. Done. https://codereview.chromium.org/2165523004/diff/200001/content/public/browser... File content/public/browser/guest_mode.h (right): https://codereview.chromium.org/2165523004/diff/200001/content/public/browser... content/public/browser/guest_mode.h:5: #ifndef CONTENT_PUBLIC_COMMON_BROWSER_GUEST_MODE_H_ On 2016/09/02 18:29:41, lfg wrote: > Fix include guards. Acknowledged.
Thanks, lgtm.
ekaramad@chromium.org changed reviewers: + creis@chromium.org
Thanks! Adding creis@ for content/ reviews. Charlie, could you please take a look or suggest another reviewer?
Description was changed from ========== Force MimeHandlerView to always use BrowserPlugin Currently, with the flag --use-cross-process-frames-for-guests switched on, MimeHandlerView still creates a BrowserPlugin while on the browser side we expect an OOPIF based implementation. This causes PDF viewer to crash. On the other hand, GuestViews will soon use OOPIF. Hence, temporarily, we need to make MimeHandlerView an exception and force it to use BrowserPlugin's codepath even when the switch above is turned on. BUG=563285 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Force MimeHandlerView to always use BrowserPlugin Currently, with the flag --use-cross-process-frames-for-guests switched on, MimeHandlerView still creates a BrowserPlugin while on the browser side we expect an OOPIF based implementation. This causes PDF viewer to crash. On the other hand, GuestViews will soon use OOPIF. Hence, temporarily, we need to make MimeHandlerView an exception and force it to use BrowserPlugin's codepath even when the switch above is turned on. BUG=563285 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
On 2016/09/02 21:09:17, EhsanK wrote: > Thanks! > > Adding creis@ for content/ reviews. > > Charlie, could you please take a look or suggest another reviewer? Thanks! I've started looking over it and I'll have some feedback on how we structure this. I'll try to send it on Tuesday when I get back.
creis@chromium.org changed reviewers: + wjmaclean@chromium.org
I'm excited about the CL overall, as it will let us ship OOPIF-based guests sooner without addressing the PDF case. Then we can remove the old code once PDF has been ported over. (Hopefully that won't take too long, since it's not good to have to support both modes.) I think we should improve how we're exposing the mode, though. A few questions on that below. Also adding wjmaclean@ for the GetNativeView() change. https://codereview.chromium.org/2165523004/diff/260001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/2165523004/diff/260001/chrome/browser/rendere... chrome/browser/renderer_context_menu/render_view_context_menu.cc:511: // based on OOPIF (https://crbug.com/563285). This looks like the wrong bug. Did you mean https://crbug.com/642826? https://codereview.chromium.org/2165523004/diff/260001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (left): https://codereview.chromium.org/2165523004/diff/260001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:117: if (BrowserPluginGuestMode::UseCrossProcessFramesForGuests()) { It's unclear to me why the old one still exists. Is it ever safe to call the old one directly anymore? It seems to me like we should be replacing it with a version that takes the WebContents. https://codereview.chromium.org/2165523004/diff/260001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.h (right): https://codereview.chromium.org/2165523004/diff/260001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.h:257: // TODO(ekaramad): Remove this once https://crbug.com/642826 is resovled. nit: resolved https://codereview.chromium.org/2165523004/diff/260001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.h:258: bool is_mime_handler_view_guest() const { Content/ doesn't know about MIMEHandlerView, so it shouldn't have methods with that name. Can we phrase it in terms of BrowserPlugin vs OOPIF? https://codereview.chromium.org/2165523004/diff/260001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2165523004/diff/260001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:202: // TODO(ekaramad): To accomodate MimeHandlerViewGuest while embedded inside I don't understand-- can you elaborate why this case needs this change? I'm concerned that this will conflict with wjmaclean's planned fix for https://crbug.com/644294. Can you coordinate with him? https://codereview.chromium.org/2165523004/diff/260001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:204: // RenderWidgetHostViewGuest. Remove this once https://crbug.com/563285 is I thought this CL was going to fix https://crbug.com/563285. Did you mean to say https://crbug.com/642826? https://codereview.chromium.org/2165523004/diff/260001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2165523004/diff/260001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:1572: if (browser_plugin_guest_ && Why does this still have the browser_plugin_guest_ null check when the previous one doesn't? https://codereview.chromium.org/2165523004/diff/260001/content/public/browser... File content/public/browser/browser_plugin_guest_delegate.h (right): https://codereview.chromium.org/2165523004/diff/260001/content/public/browser... content/public/browser/browser_plugin_guest_delegate.h:100: virtual bool IsMimeHandlerViewGuest(); As before, we can't phrase this in terms of MIMEHandlerView, which content/ doesn't know about. Can we phrase it in terms of BrowserPlugin vs OOPIF instead? https://codereview.chromium.org/2165523004/diff/260001/content/public/browser... File content/public/browser/guest_mode.h (right): https://codereview.chromium.org/2165523004/diff/260001/content/public/browser... content/public/browser/guest_mode.h:15: namespace guest_mode { Hmm, I'm not a fan of introducing a new namespace and file for this. Why not put it in BrowserPluginGuestMode instead? https://codereview.chromium.org/2165523004/diff/260001/content/public/browser... content/public/browser/guest_mode.h:18: // BrowserPlugin-based MimeHandlerView to coexit with the rest of guests when nit: coexist https://codereview.chromium.org/2165523004/diff/260001/content/public/browser... content/public/browser/guest_mode.h:21: bool CONTENT_EXPORT IsCrossProcessFrameGuest(WebContents* web_contents); I'm finding this confusing, both in name and how it relates to UseCrossProcessFramesForGuests. Is it ever safe to call UseCrossProcessFramesForGuests directly anymore? Maybe we should just be replacing that. https://codereview.chromium.org/2165523004/diff/260001/extensions/browser/gue... File extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc (right): https://codereview.chromium.org/2165523004/diff/260001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc:142: // (https://crbug.com/563285). Wrong bug?
I don't think the changes to GetNativeView() will cause me problems, though I would like us to consider getting it from the root render widget view instead. https://codereview.chromium.org/2165523004/diff/260001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2165523004/diff/260001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:204: // RenderWidgetHostViewGuest. Remove this once https://crbug.com/563285 is Do we really want to remove this afterwards? It seems quite reasonable to me that a RWHVCF should provide a NativeView* when asked (often this is just done to query window settings, though in the case where it's done to change settings we should be careful to ensure there are no conflicts). https://codereview.chromium.org/2165523004/diff/260001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:207: return frame_connector_->GetParentRenderWidgetHostView()->GetNativeView(); GetRootRenderWidgetHostView() instead of GetParentRenderWidgetHostView()? https://codereview.chromium.org/2165523004/diff/260001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:208: return nullptr; At some point it would be nice to always return a non-null value (does a RWHVCF ever *not* have a 'parent'), but for now we'll still need to make sure that consumers check the return value.
https://codereview.chromium.org/2165523004/diff/260001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2165523004/diff/260001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:207: return frame_connector_->GetParentRenderWidgetHostView()->GetNativeView(); On 2016/09/07 13:32:33, wjmaclean wrote: > GetRootRenderWidgetHostView() instead of GetParentRenderWidgetHostView()? I think parent is better than root in this case. It allows any frame in the tree to view, intercept and modify the request if needed. For example, RenderWidgetHostViewGuest has its own NativeView. In that case, (hypothetically, since currently there's no OOPIFs inside guests), we would return the wrong NativeView by bypassing all the frames in the tree. When I added GetRootRenderWidgetHostView(), the intention was to solve a specific problem: GetBoundsInRootWindow(), which can't be done otherwise; but other than that, I think it should be used with care. https://codereview.chromium.org/2165523004/diff/260001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:208: return nullptr; On 2016/09/07 13:32:33, wjmaclean wrote: > At some point it would be nice to always return a non-null value (does a RWHVCF > ever *not* have a 'parent'), but for now we'll still need to make sure that > consumers check the return value. That will happen during construction/destruction, where the frame_connector_ is null. The only way to guarantee that this is non-null is by guaranteeing that the callers never call this when the frame is being initialized/destroyed.
On 2016/09/07 14:55:18, lfg wrote: > https://codereview.chromium.org/2165523004/diff/260001/content/browser/frame_... > File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): > > https://codereview.chromium.org/2165523004/diff/260001/content/browser/frame_... > content/browser/frame_host/render_widget_host_view_child_frame.cc:207: return > frame_connector_->GetParentRenderWidgetHostView()->GetNativeView(); > On 2016/09/07 13:32:33, wjmaclean wrote: > > GetRootRenderWidgetHostView() instead of GetParentRenderWidgetHostView()? > > I think parent is better than root in this case. It allows any frame in the tree > to view, intercept and modify the request if needed. > > For example, RenderWidgetHostViewGuest has its own NativeView. In that case, > (hypothetically, since currently there's no OOPIFs inside guests), we would > return the wrong NativeView by bypassing all the frames in the tree. > > When I added GetRootRenderWidgetHostView(), the intention was to solve a > specific problem: GetBoundsInRootWindow(), which can't be done otherwise; but > other than that, I think it should be used with care. > > https://codereview.chromium.org/2165523004/diff/260001/content/browser/frame_... > content/browser/frame_host/render_widget_host_view_child_frame.cc:208: return > nullptr; > On 2016/09/07 13:32:33, wjmaclean wrote: > > At some point it would be nice to always return a non-null value (does a > RWHVCF > > ever *not* have a 'parent'), but for now we'll still need to make sure that > > consumers check the return value. > > That will happen during construction/destruction, where the frame_connector_ is > null. The only way to guarantee that this is non-null is by guaranteeing that > the callers never call this when the frame is being initialized/destroyed. Yeah, I forgot that we'll still have to consider RenderWidgetHostViewGuest here. Ok, I'm fine with parent then. I appreciate the comment about when the frame_connector is null, though in those cases isn't there still some reasonable window we can associate the view with? For example, can the view ever be created without a relevant WebContents? If not, then could we use the main frame's window?
On 2016/09/07 15:35:55, wjmaclean wrote: > > I appreciate the comment about when the frame_connector is null, though in those > cases isn't there still some reasonable window we can associate the view with? > For example, can the view ever be created without a relevant WebContents? If > not, then could we use the main frame's window? The parent definitely exists, it's just inaccessible from the child frame. I think this is by design, since the RenderWidgetHostViewChildFrame only talks with the CrossProcessFrameConnector. Perhaps we could look into ways to solve this/rework the lifetime of the objects if it becomes an issue.
On 2016/09/07 15:43:40, lfg wrote: > On 2016/09/07 15:35:55, wjmaclean wrote: > > > > I appreciate the comment about when the frame_connector is null, though in > those > > cases isn't there still some reasonable window we can associate the view with? > > For example, can the view ever be created without a relevant WebContents? If > > not, then could we use the main frame's window? > > The parent definitely exists, it's just inaccessible from the child frame. I > think this is by design, since the RenderWidgetHostViewChildFrame only talks > with the CrossProcessFrameConnector. Perhaps we could look into ways to solve > this/rework the lifetime of the objects if it becomes an issue. Yes, this is sort of the lines I was thinking along. Let's discuss this offline.
Thanks for the reviews. Charlie: I guess the main issue here is whether we should convert all the browser side usags of UseCrossProcessFramesForGuests to act on WebContents rather than just looking at the switch. I will have to spend some time on that. I decided to just fix smaller issues in this patch. I will submit another patch with all the fixes of UseCrossProcessFramesForGuests or a justification on why it might not be possible in some cases. https://codereview.chromium.org/2165523004/diff/260001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/2165523004/diff/260001/chrome/browser/rendere... chrome/browser/renderer_context_menu/render_view_context_menu.cc:511: // based on OOPIF (https://crbug.com/563285). On 2016/09/07 00:22:29, Charlie Reis wrote: > This looks like the wrong bug. Did you mean https://crbug.com/642826 Yes. I guess I forgot to update the comment after filing the new bug. Thanks. https://codereview.chromium.org/2165523004/diff/260001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (left): https://codereview.chromium.org/2165523004/diff/260001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:117: if (BrowserPluginGuestMode::UseCrossProcessFramesForGuests()) { On 2016/09/07 00:22:29, Charlie Reis wrote: > It's unclear to me why the old one still exists. Is it ever safe to call the > old one directly anymore? It seems to me like we should be replacing it with a > version that takes the WebContents. I tried changing the only methods which were blocking the way. I will re-investigate all occurrences of the old one and see if they should/must be changed. https://codereview.chromium.org/2165523004/diff/260001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.h (right): https://codereview.chromium.org/2165523004/diff/260001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.h:257: // TODO(ekaramad): Remove this once https://crbug.com/642826 is resovled. On 2016/09/07 00:22:29, Charlie Reis wrote: > nit: resolved Done. https://codereview.chromium.org/2165523004/diff/260001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.h:258: bool is_mime_handler_view_guest() const { On 2016/09/07 00:22:29, Charlie Reis wrote: > Content/ doesn't know about MIMEHandlerView, so it shouldn't have methods with > that name. Can we phrase it in terms of BrowserPlugin vs OOPIF? Good idea! How about CanUseCrossProcessFrames() for the delegate and can_use_cross_process_frames() for BPG itself? https://codereview.chromium.org/2165523004/diff/260001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2165523004/diff/260001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:202: // TODO(ekaramad): To accomodate MimeHandlerViewGuest while embedded inside On 2016/09/07 00:22:29, Charlie Reis wrote: > I don't understand-- can you elaborate why this case needs this change? > > I'm concerned that this will conflict with wjmaclean's planned fix for > https://crbug.com/644294. Can you coordinate with him? The issue leading to this change is loading a pdf inside a <webview> with --use-cross-process-frames-for-guests. The PDF is a BrowserPlugin and RWHVGuest needs native view for size update. That will crash if RWHVCF returns null. Here RWHVCF is the parent RWHV for the guest. This is one example of nested guests. https://codereview.chromium.org/2165523004/diff/260001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:204: // RenderWidgetHostViewGuest. Remove this once https://crbug.com/563285 is On 2016/09/07 00:22:29, Charlie Reis wrote: > I thought this CL was going to fix https://crbug.com/563285. Did you mean to > say https://crbug.com/642826 Yes. Initially I did not have a tracking bug for PDF. The bug 536285 is about PDF crash in the presence of oopif flag. This CL technically fixes that. But for tracking the actual MimeHandlerView based on OOPIF issue I filed the new bug but failed to update all comments accordingly. https://codereview.chromium.org/2165523004/diff/260001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:207: return frame_connector_->GetParentRenderWidgetHostView()->GetNativeView(); On 2016/09/07 14:55:18, lfg wrote: > On 2016/09/07 13:32:33, wjmaclean wrote: > > GetRootRenderWidgetHostView() instead of GetParentRenderWidgetHostView()? > > I think parent is better than root in this case. It allows any frame in the tree > to view, intercept and modify the request if needed. > > For example, RenderWidgetHostViewGuest has its own NativeView. In that case, > (hypothetically, since currently there's no OOPIFs inside guests), we would > return the wrong NativeView by bypassing all the frames in the tree. > > When I added GetRootRenderWidgetHostView(), the intention was to solve a > specific problem: GetBoundsInRootWindow(), which can't be done otherwise; but > other than that, I think it should be used with care. Also, but returning the parent, we always end up with the root anyway, won't we? https://codereview.chromium.org/2165523004/diff/260001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2165523004/diff/260001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:1572: if (browser_plugin_guest_ && On 2016/09/07 00:22:29, Charlie Reis wrote: > Why does this still have the browser_plugin_guest_ null check when the previous > one doesn't? The null check is inherent to the method call. So the call to guest_mode::IsCrossProcessFrameGuest(wc) returns true if 1) |wc| is a guest (has |browser_plugin_guest_| 2) |wc| is OOPIF, i.e., we have the switch AND |wc| is not a MimeHandlerViewGuest's web contents. So when the method returns false, the web contents is either not guest or it is a guest but not OOPIF. https://codereview.chromium.org/2165523004/diff/260001/content/public/browser... File content/public/browser/browser_plugin_guest_delegate.h (right): https://codereview.chromium.org/2165523004/diff/260001/content/public/browser... content/public/browser/browser_plugin_guest_delegate.h:100: virtual bool IsMimeHandlerViewGuest(); On 2016/09/07 00:22:29, Charlie Reis wrote: > As before, we can't phrase this in terms of MIMEHandlerView, which content/ > doesn't know about. Can we phrase it in terms of BrowserPlugin vs OOPIF > instead? Acknowledged. https://codereview.chromium.org/2165523004/diff/260001/content/public/browser... File content/public/browser/guest_mode.h (right): https://codereview.chromium.org/2165523004/diff/260001/content/public/browser... content/public/browser/guest_mode.h:15: namespace guest_mode { On 2016/09/07 00:22:29, Charlie Reis wrote: > Hmm, I'm not a fan of introducing a new namespace and file for this. Why not > put it in BrowserPluginGuestMode instead? That one lives in content/public/common while this is in content/public/browser. Should I introduce a new class? Meanwhile, I am investigating the removal of UseCrossProcessFramesForGuest(). https://codereview.chromium.org/2165523004/diff/260001/content/public/browser... content/public/browser/guest_mode.h:18: // BrowserPlugin-based MimeHandlerView to coexit with the rest of guests when On 2016/09/07 00:22:29, Charlie Reis wrote: > nit: coexist Acknowledged. https://codereview.chromium.org/2165523004/diff/260001/content/public/browser... content/public/browser/guest_mode.h:21: bool CONTENT_EXPORT IsCrossProcessFrameGuest(WebContents* web_contents); On 2016/09/07 00:22:29, Charlie Reis wrote: > I'm finding this confusing, both in name and how it relates to > UseCrossProcessFramesForGuests. > > Is it ever safe to call UseCrossProcessFramesForGuests directly anymore? Maybe > we should just be replacing that. Hmm, this method is supposed to return true when |web_contents| is a guest and is OOPIF. That is why I named it like this. I am however open to any better suggestions for naming. I am also investigating removing the older method BrowserPluginGuestMode::UseCrossProcessFramesForGuests(). https://codereview.chromium.org/2165523004/diff/260001/content/public/browser... content/public/browser/guest_mode.h:21: bool CONTENT_EXPORT IsCrossProcessFrameGuest(WebContents* web_contents); On 2016/09/07 00:22:29, Charlie Reis wrote: > I'm finding this confusing, both in name and how it relates to > UseCrossProcessFramesForGuests. > > Is it ever safe to call UseCrossProcessFramesForGuests directly anymore? Maybe > we should just be replacing that. As lfg@ was suggesting offline, on the renderer side there are two usages of UserCrossProcessFramesForGuests(). But still, I will try to figure out if we can replace all the browser side occurrences with the new method which verifies it for an individual WebContents. https://codereview.chromium.org/2165523004/diff/260001/extensions/browser/gue... File extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc (right): https://codereview.chromium.org/2165523004/diff/260001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc:142: // (https://crbug.com/563285). On 2016/09/07 00:22:29, Charlie Reis wrote: > Wrong bug? Yes. Thanks! Done.
Very sorry for the delay, and thanks for updating it. I think this is looking better, and we mainly need to resolve whether the old policy method can be removed (at least from the browser process). https://codereview.chromium.org/2165523004/diff/260001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.h (right): https://codereview.chromium.org/2165523004/diff/260001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.h:258: bool is_mime_handler_view_guest() const { On 2016/09/08 18:14:57, EhsanK wrote: > On 2016/09/07 00:22:29, Charlie Reis wrote: > > Content/ doesn't know about MIMEHandlerView, so it shouldn't have methods with > > that name. Can we phrase it in terms of BrowserPlugin vs OOPIF? > > Good idea! How about CanUseCrossProcessFrames() for the delegate and > can_use_cross_process_frames() for BPG itself? That sounds better, thanks. https://codereview.chromium.org/2165523004/diff/260001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2165523004/diff/260001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:1572: if (browser_plugin_guest_ && On 2016/09/08 18:14:58, EhsanK wrote: > On 2016/09/07 00:22:29, Charlie Reis wrote: > > Why does this still have the browser_plugin_guest_ null check when the > previous > > one doesn't? > > The null check is inherent to the method call. So the call to > guest_mode::IsCrossProcessFrameGuest(wc) returns true if > 1) |wc| is a guest (has |browser_plugin_guest_| > 2) |wc| is OOPIF, i.e., we have the switch AND |wc| is not a > MimeHandlerViewGuest's web contents. > > So when the method returns false, the web contents is either not guest or it is > a guest but not OOPIF. That doesn't answer why line 1563 is missing the null check. If it's needed here, I would imagine it's needed there as well. https://codereview.chromium.org/2165523004/diff/260001/content/public/browser... File content/public/browser/guest_mode.h (right): https://codereview.chromium.org/2165523004/diff/260001/content/public/browser... content/public/browser/guest_mode.h:15: namespace guest_mode { On 2016/09/08 18:14:58, EhsanK wrote: > On 2016/09/07 00:22:29, Charlie Reis wrote: > > Hmm, I'm not a fan of introducing a new namespace and file for this. Why not > > put it in BrowserPluginGuestMode instead? > > That one lives in content/public/common while this is in content/public/browser. > Should I introduce a new class? Yes, let's at least use the same pattern, even if we can't put it in content/public/common. > Meanwhile, I am investigating the removal of UseCrossProcessFramesForGuest(). https://codereview.chromium.org/2165523004/diff/260001/content/public/browser... content/public/browser/guest_mode.h:21: bool CONTENT_EXPORT IsCrossProcessFrameGuest(WebContents* web_contents); On 2016/09/08 18:14:58, EhsanK wrote: > On 2016/09/07 00:22:29, Charlie Reis wrote: > > I'm finding this confusing, both in name and how it relates to > > UseCrossProcessFramesForGuests. > > > > Is it ever safe to call UseCrossProcessFramesForGuests directly anymore? > Maybe > > we should just be replacing that. > > As lfg@ was suggesting offline, on the renderer side there are two usages of > UserCrossProcessFramesForGuests(). > > But still, I will try to figure out if we can replace all the browser side > occurrences with the new method which verifies it for > an individual WebContents. Yes, maybe we can have a renderer-side version for those two cases, and use this for all the browser-side cases (with nothing left in content/public/common)? https://codereview.chromium.org/2165523004/diff/290001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.h (right): https://codereview.chromium.org/2165523004/diff/290001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.h:456: // MimeHandlerView. Don't forget to update the comment. (Also, "tyoe" -> "type") https://codereview.chromium.org/2165523004/diff/290001/content/public/browser... File content/public/browser/browser_plugin_guest_delegate.h (right): https://codereview.chromium.org/2165523004/diff/290001/content/public/browser... content/public/browser/browser_plugin_guest_delegate.h:96: // TODO(ekaramad): A short workaround for to force some types of guests to use nit: Drop "for" https://codereview.chromium.org/2165523004/diff/290001/content/public/browser... File content/public/browser/guest_mode.h (right): https://codereview.chromium.org/2165523004/diff/290001/content/public/browser... content/public/browser/guest_mode.h:17: // TODO(ekaramad): This method is temporarily added to provide a workaround to If we do end up removing UseCrossProcessFramesForGuests for the browser process, we should add an explanatory comment like "Returns whether the given inner WebContents should be implemented using out-of-process iframes instead of BrowserPlugin" in addition to having the TODO.
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the reviews! Charlie: I think we should keep a method like BrowserPluginGuestMode::UseCrossProcessFramesForGuests() aroudn, either two copies one for content/public/browser and one for content/public/renderer, or just the one in content/common. I also ended up renaming the method and adding one more method which checks if RenderViewHost is associated with OOPIF. This was needed in RenderFrameHostManager where we cannot include WebContents stuff (other alternative would have been to add a method to RenderFrameHostManager::Delegate). PTAL. https://codereview.chromium.org/2165523004/diff/260001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2165523004/diff/260001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:1572: if (browser_plugin_guest_ && On 2016/09/15 20:56:10, Charlie Reis (slow) wrote: > On 2016/09/08 18:14:58, EhsanK wrote: > > On 2016/09/07 00:22:29, Charlie Reis wrote: > > > Why does this still have the browser_plugin_guest_ null check when the > > previous > > > one doesn't? > > > > The null check is inherent to the method call. So the call to > > guest_mode::IsCrossProcessFrameGuest(wc) returns true if > > 1) |wc| is a guest (has |browser_plugin_guest_| > > 2) |wc| is OOPIF, i.e., we have the switch AND |wc| is not a > > MimeHandlerViewGuest's web contents. > > > > So when the method returns false, the web contents is either not guest or it > is > > a guest but not OOPIF. > > That doesn't answer why line 1563 is missing the null check. If it's needed > here, I would imagine it's needed there as well. On line 1563, the call to guest_mode::IsCrossProcessFrameGuest(this) checks if |this->browser_plugin_guest_|). So it is not needed to check again. But here, when the method returns false we still don't know if it was because our guest was not OOPIF, or we did not have guest at all. So we check for |browser_plugin_guest_| to confirm that the former case is valid. https://codereview.chromium.org/2165523004/diff/260001/content/public/browser... File content/public/browser/guest_mode.h (right): https://codereview.chromium.org/2165523004/diff/260001/content/public/browser... content/public/browser/guest_mode.h:15: namespace guest_mode { On 2016/09/15 20:56:10, Charlie Reis (slow) wrote: > On 2016/09/08 18:14:58, EhsanK wrote: > > On 2016/09/07 00:22:29, Charlie Reis wrote: > > > Hmm, I'm not a fan of introducing a new namespace and file for this. Why > not > > > put it in BrowserPluginGuestMode instead? > > > > That one lives in content/public/common while this is in > content/public/browser. > > Should I introduce a new class? > > Yes, let's at least use the same pattern, even if we can't put it in > content/public/common. > > > Meanwhile, I am investigating the removal of UseCrossProcessFramesForGuest(). Sure. I am calling the class GuestMode now. https://codereview.chromium.org/2165523004/diff/260001/content/public/browser... content/public/browser/guest_mode.h:21: bool CONTENT_EXPORT IsCrossProcessFrameGuest(WebContents* web_contents); On 2016/09/15 20:56:10, Charlie Reis (slow) wrote: > On 2016/09/08 18:14:58, EhsanK wrote: > > On 2016/09/07 00:22:29, Charlie Reis wrote: > > > I'm finding this confusing, both in name and how it relates to > > > UseCrossProcessFramesForGuests. > > > > > > Is it ever safe to call UseCrossProcessFramesForGuests directly anymore? > > Maybe > > > we should just be replacing that. > > > > As lfg@ was suggesting offline, on the renderer side there are two usages of > > UserCrossProcessFramesForGuests(). > > > > But still, I will try to figure out if we can replace all the browser side > > occurrences with the new method which verifies it for > > an individual WebContents. > > Yes, maybe we can have a renderer-side version for those two cases, and use this > for all the browser-side cases (with nothing left in content/public/common)? On the renderer side it suffices to check for the existence of the flag. The usages are: 1- GuestViewIframeContainer, which cannot have a non-OOPIF implementation. 2- Dispatcher::GetJsResources(), which is static 3- Dispatcher::RequireGuestViewModules() which AFAICT is only for guests. But this is just two js bindings which are not necessary if the guest is not using <iframes>. For that reason, we might be better off with having BrowserPluginGuestMode::UseCrossProcessFramesForGuests() for those applications. As far as its existence in content/public/common goes, I think it is necessary because we use it in browser test. We could remove it, but instead, add the same method to both content/public/renderer and content/public/browser. WDYT? https://codereview.chromium.org/2165523004/diff/290001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.h (right): https://codereview.chromium.org/2165523004/diff/290001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.h:456: // MimeHandlerView. On 2016/09/15 20:56:10, Charlie Reis (slow) wrote: > Don't forget to update the comment. (Also, "tyoe" -> "type") Acknowledged. https://codereview.chromium.org/2165523004/diff/290001/content/public/browser... File content/public/browser/browser_plugin_guest_delegate.h (right): https://codereview.chromium.org/2165523004/diff/290001/content/public/browser... content/public/browser/browser_plugin_guest_delegate.h:96: // TODO(ekaramad): A short workaround for to force some types of guests to use On 2016/09/15 20:56:10, Charlie Reis (slow) wrote: > nit: Drop "for" Done. https://codereview.chromium.org/2165523004/diff/290001/content/public/browser... File content/public/browser/guest_mode.h (right): https://codereview.chromium.org/2165523004/diff/290001/content/public/browser... content/public/browser/guest_mode.h:17: // TODO(ekaramad): This method is temporarily added to provide a workaround to On 2016/09/15 20:56:10, Charlie Reis (slow) wrote: > If we do end up removing UseCrossProcessFramesForGuests for the browser process, > we should add an explanatory comment like "Returns whether the given inner > WebContents should be implemented using out-of-process iframes instead of > BrowserPlugin" in addition to having the TODO. Acknowledged.
ekaramad@chromium.org changed reviewers: + avallee@chromium.org
Adding avallee@ for focus related code paths in WebContentsImpl since he is working on those areas. Alex, could you please take a look?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks for following up! I think we can go a little further to make this cleaner and easier to reason about. On 2016/09/21 21:34:53, EhsanK wrote: > Thanks for the reviews! > > Charlie: I think we should keep a method like > BrowserPluginGuestMode::UseCrossProcessFramesForGuests() aroudn, either two > copies one for content/public/browser and one for content/public/renderer, or > just the one in content/common. Hmm, I disagree. I think we should remove it entirely from content/browser. (See my explanation below.) https://codereview.chromium.org/2165523004/diff/260001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2165523004/diff/260001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:1572: if (browser_plugin_guest_ && On 2016/09/21 21:34:53, EhsanK wrote: > On 2016/09/15 20:56:10, Charlie Reis (slow) wrote: > > On 2016/09/08 18:14:58, EhsanK wrote: > > > On 2016/09/07 00:22:29, Charlie Reis wrote: > > > > Why does this still have the browser_plugin_guest_ null check when the > > > previous > > > > one doesn't? > > > > > > The null check is inherent to the method call. So the call to > > > guest_mode::IsCrossProcessFrameGuest(wc) returns true if > > > 1) |wc| is a guest (has |browser_plugin_guest_| > > > 2) |wc| is OOPIF, i.e., we have the switch AND |wc| is not a > > > MimeHandlerViewGuest's web contents. > > > > > > So when the method returns false, the web contents is either not guest or it > > is > > > a guest but not OOPIF. > > > > That doesn't answer why line 1563 is missing the null check. If it's needed > > here, I would imagine it's needed there as well. > On line 1563, the call to guest_mode::IsCrossProcessFrameGuest(this) checks if > |this->browser_plugin_guest_|). So it is not needed to check again. > > But here, when the method returns false we still don't know if it was because > our guest was not OOPIF, or we did not have guest at all. So we check for > |browser_plugin_guest_| to confirm that the former case is valid. Oh, I see now. Thanks. https://codereview.chromium.org/2165523004/diff/260001/content/public/browser... File content/public/browser/guest_mode.h (right): https://codereview.chromium.org/2165523004/diff/260001/content/public/browser... content/public/browser/guest_mode.h:21: bool CONTENT_EXPORT IsCrossProcessFrameGuest(WebContents* web_contents); On 2016/09/21 21:34:53, EhsanK wrote: > On 2016/09/15 20:56:10, Charlie Reis (slow) wrote: > > On 2016/09/08 18:14:58, EhsanK wrote: > > > On 2016/09/07 00:22:29, Charlie Reis wrote: > > > > I'm finding this confusing, both in name and how it relates to > > > > UseCrossProcessFramesForGuests. > > > > > > > > Is it ever safe to call UseCrossProcessFramesForGuests directly anymore? > > > Maybe > > > > we should just be replacing that. > > > > > > As lfg@ was suggesting offline, on the renderer side there are two usages of > > > UserCrossProcessFramesForGuests(). > > > > > > But still, I will try to figure out if we can replace all the browser side > > > occurrences with the new method which verifies it for > > > an individual WebContents. > > > > Yes, maybe we can have a renderer-side version for those two cases, and use > this > > for all the browser-side cases (with nothing left in content/public/common)? > > On the renderer side it suffices to check for the existence of the flag. The > usages are: > 1- GuestViewIframeContainer, which cannot have a non-OOPIF implementation. > 2- Dispatcher::GetJsResources(), which is static > 3- Dispatcher::RequireGuestViewModules() which AFAICT is only for guests. But > this is just two js bindings which are not necessary if the guest is not using > <iframes>. The property we're looking for is that these places in the renderer won't break in the PDF case when UseCrossProcessFramesForGuests is on. Is that true? (I'm not sure from your description.) > For that reason, we might be better off with having > BrowserPluginGuestMode::UseCrossProcessFramesForGuests() for those applications. > As far as its existence in content/public/common goes, I think it is necessary > because we use it in browser test. Let's just check the flag directly in the browser tests, since they're just skipping the test if we're in that mode. We shouldn't keep the method in content/public/browser for that case. > > We could remove it, but instead, add the same method to both > content/public/renderer and content/public/browser. WDYT? Having it in content/public/browser would defeat the point. It seems like any code that calls UseCrossProcessFramesForGuests() will be wrong in the PDF case, so I'd like to remove it completely from the browser process so no one tries to call it (and be sure that the uses in the renderer are safe). I noticed there's a call in SiteIsolationPolicy::AreCrossProcessFramesPossible, but we can probably check the flag directly there. (Technically that method returns true on all desktop platforms today anyway, now that --isolate-extensions is enabled. I suppose it's still false on Android, though.) https://codereview.chromium.org/2165523004/diff/330001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.h (right): https://codereview.chromium.org/2165523004/diff/330001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.h:36: #include "content/public/browser/guest_mode.h" nit: Stale. https://codereview.chromium.org/2165523004/diff/330001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2165523004/diff/330001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1828: CHECK(GuestMode::ForInnerWebContentsUsingCrossProcessFrames(current_host())); This is just a sanity check, so I don't think we need the RVH version of the method just for that. (We're already doing the CHECK from the only callsite in AttachToOuterWebContentsFrame.) Let's remove the RVH version and just put a comment on the declaration of this method in render_frame_host_manager.h, saying that it should only be called for OOPIF-based inner WebContents. https://codereview.chromium.org/2165523004/diff/330001/content/public/browser... File content/public/browser/guest_mode.h (right): https://codereview.chromium.org/2165523004/diff/330001/content/public/browser... content/public/browser/guest_mode.h:24: static bool IsInnerWebContentsUsingCrossProcessFrames( Given that we're calling this class GuestMode (and other "guest" references exist in content/ names), I'm ok with the simpler IsCrossProcessFrameGuest method name that we had before. I think that's a bit easier to read. (The comment can keep the "inner WebContents" reference.) https://codereview.chromium.org/2165523004/diff/330001/content/public/browser... content/public/browser/guest_mode.h:29: static bool ForInnerWebContentsUsingCrossProcessFrames(RenderViewHost* rvh); IsCrossProcessFrameGuest(rvh) (Actually, we can remove this. See my comment in RFHM.)
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL. Thanks! https://codereview.chromium.org/2165523004/diff/260001/content/public/browser... File content/public/browser/guest_mode.h (right): https://codereview.chromium.org/2165523004/diff/260001/content/public/browser... content/public/browser/guest_mode.h:21: bool CONTENT_EXPORT IsCrossProcessFrameGuest(WebContents* web_contents); On 2016/09/21 22:18:21, Charlie Reis (slow) wrote: > On 2016/09/21 21:34:53, EhsanK wrote: > > On 2016/09/15 20:56:10, Charlie Reis (slow) wrote: > > > On 2016/09/08 18:14:58, EhsanK wrote: > > > > On 2016/09/07 00:22:29, Charlie Reis wrote: > > > > > I'm finding this confusing, both in name and how it relates to > > > > > UseCrossProcessFramesForGuests. > > > > > > > > > > Is it ever safe to call UseCrossProcessFramesForGuests directly anymore? > > > > > Maybe > > > > > we should just be replacing that. > > > > > > > > As lfg@ was suggesting offline, on the renderer side there are two usages > of > > > > UserCrossProcessFramesForGuests(). > > > > > > > > But still, I will try to figure out if we can replace all the browser side > > > > occurrences with the new method which verifies it for > > > > an individual WebContents. > > > > > > Yes, maybe we can have a renderer-side version for those two cases, and use > > this > > > for all the browser-side cases (with nothing left in content/public/common)? > > > > On the renderer side it suffices to check for the existence of the flag. The > > usages are: > > 1- GuestViewIframeContainer, which cannot have a non-OOPIF implementation. > > 2- Dispatcher::GetJsResources(), which is static > > 3- Dispatcher::RequireGuestViewModules() which AFAICT is only for guests. But > > this is just two js bindings which are not necessary if the guest is not using > > <iframes>. > > The property we're looking for is that these places in the renderer won't break > in the PDF case when UseCrossProcessFramesForGuests is on. Is that true? (I'm > not sure from your description.) > > > For that reason, we might be better off with having > > BrowserPluginGuestMode::UseCrossProcessFramesForGuests() for those > applications. > > As far as its existence in content/public/common goes, I think it is necessary > > because we use it in browser test. > > Let's just check the flag directly in the browser tests, since they're just > skipping the test if we're in that mode. We shouldn't keep the method in > content/public/browser for that case. > > > > > We could remove it, but instead, add the same method to both > > content/public/renderer and content/public/browser. WDYT? > > Having it in content/public/browser would defeat the point. It seems like any > code that calls UseCrossProcessFramesForGuests() will be wrong in the PDF case, > so I'd like to remove it completely from the browser process so no one tries to > call it (and be sure that the uses in the renderer are safe). > > I noticed there's a call in SiteIsolationPolicy::AreCrossProcessFramesPossible, > but we can probably check the flag directly there. (Technically that method > returns true on all desktop platforms today anyway, now that > --isolate-extensions is enabled. I suppose it's still false on Android, > though.) Thanks for clarification/suggestions. I removed the class from content/common. I have now added another class to renderer/ which will check the flag. I could instead check for the switch directly in those 3 usages. WDYT? https://codereview.chromium.org/2165523004/diff/330001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.h (right): https://codereview.chromium.org/2165523004/diff/330001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.h:36: #include "content/public/browser/guest_mode.h" On 2016/09/21 22:18:21, Charlie Reis (slow) wrote: > nit: Stale. Done. https://codereview.chromium.org/2165523004/diff/330001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2165523004/diff/330001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1828: CHECK(GuestMode::ForInnerWebContentsUsingCrossProcessFrames(current_host())); On 2016/09/21 22:18:21, Charlie Reis (slow) wrote: > This is just a sanity check, so I don't think we need the RVH version of the > method just for that. (We're already doing the CHECK from the only callsite in > AttachToOuterWebContentsFrame.) > > Let's remove the RVH version and just put a comment on the declaration of this > method in render_frame_host_manager.h, saying that it should only be called for > OOPIF-based inner WebContents. Acknowledged. https://codereview.chromium.org/2165523004/diff/330001/content/public/browser... File content/public/browser/guest_mode.h (right): https://codereview.chromium.org/2165523004/diff/330001/content/public/browser... content/public/browser/guest_mode.h:24: static bool IsInnerWebContentsUsingCrossProcessFrames( On 2016/09/21 22:18:21, Charlie Reis (slow) wrote: > Given that we're calling this class GuestMode (and other "guest" references > exist in content/ names), I'm ok with the simpler IsCrossProcessFrameGuest > method name that we had before. I think that's a bit easier to read. > > (The comment can keep the "inner WebContents" reference.) Acknowledged. https://codereview.chromium.org/2165523004/diff/330001/content/public/browser... content/public/browser/guest_mode.h:29: static bool ForInnerWebContentsUsingCrossProcessFrames(RenderViewHost* rvh); On 2016/09/21 22:18:21, Charlie Reis (slow) wrote: > IsCrossProcessFrameGuest(rvh) > > (Actually, we can remove this. See my comment in RFHM.) Acknowledged.
PTAL. Thanks! https://codereview.chromium.org/2165523004/diff/260001/content/public/browser... File content/public/browser/guest_mode.h (right): https://codereview.chromium.org/2165523004/diff/260001/content/public/browser... content/public/browser/guest_mode.h:21: bool CONTENT_EXPORT IsCrossProcessFrameGuest(WebContents* web_contents); On 2016/09/21 22:18:21, Charlie Reis (slow) wrote: > On 2016/09/21 21:34:53, EhsanK wrote: > > On 2016/09/15 20:56:10, Charlie Reis (slow) wrote: > > > On 2016/09/08 18:14:58, EhsanK wrote: > > > > On 2016/09/07 00:22:29, Charlie Reis wrote: > > > > > I'm finding this confusing, both in name and how it relates to > > > > > UseCrossProcessFramesForGuests. > > > > > > > > > > Is it ever safe to call UseCrossProcessFramesForGuests directly anymore? > > > > > Maybe > > > > > we should just be replacing that. > > > > > > > > As lfg@ was suggesting offline, on the renderer side there are two usages > of > > > > UserCrossProcessFramesForGuests(). > > > > > > > > But still, I will try to figure out if we can replace all the browser side > > > > occurrences with the new method which verifies it for > > > > an individual WebContents. > > > > > > Yes, maybe we can have a renderer-side version for those two cases, and use > > this > > > for all the browser-side cases (with nothing left in content/public/common)? > > > > On the renderer side it suffices to check for the existence of the flag. The > > usages are: > > 1- GuestViewIframeContainer, which cannot have a non-OOPIF implementation. > > 2- Dispatcher::GetJsResources(), which is static > > 3- Dispatcher::RequireGuestViewModules() which AFAICT is only for guests. But > > this is just two js bindings which are not necessary if the guest is not using > > <iframes>. > > The property we're looking for is that these places in the renderer won't break > in the PDF case when UseCrossProcessFramesForGuests is on. Is that true? (I'm > not sure from your description.) > > > For that reason, we might be better off with having > > BrowserPluginGuestMode::UseCrossProcessFramesForGuests() for those > applications. > > As far as its existence in content/public/common goes, I think it is necessary > > because we use it in browser test. > > Let's just check the flag directly in the browser tests, since they're just > skipping the test if we're in that mode. We shouldn't keep the method in > content/public/browser for that case. > > > > > We could remove it, but instead, add the same method to both > > content/public/renderer and content/public/browser. WDYT? > > Having it in content/public/browser would defeat the point. It seems like any > code that calls UseCrossProcessFramesForGuests() will be wrong in the PDF case, > so I'd like to remove it completely from the browser process so no one tries to > call it (and be sure that the uses in the renderer are safe). > > I noticed there's a call in SiteIsolationPolicy::AreCrossProcessFramesPossible, > but we can probably check the flag directly there. (Technically that method > returns true on all desktop platforms today anyway, now that > --isolate-extensions is enabled. I suppose it's still false on Android, > though.) Thanks for clarification/suggestions. I removed the class from content/common. I have now added another class to renderer/ which will check the flag. I could instead check for the switch directly in those 3 usages. WDYT? https://codereview.chromium.org/2165523004/diff/330001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.h (right): https://codereview.chromium.org/2165523004/diff/330001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.h:36: #include "content/public/browser/guest_mode.h" On 2016/09/21 22:18:21, Charlie Reis (slow) wrote: > nit: Stale. Done. https://codereview.chromium.org/2165523004/diff/330001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2165523004/diff/330001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1828: CHECK(GuestMode::ForInnerWebContentsUsingCrossProcessFrames(current_host())); On 2016/09/21 22:18:21, Charlie Reis (slow) wrote: > This is just a sanity check, so I don't think we need the RVH version of the > method just for that. (We're already doing the CHECK from the only callsite in > AttachToOuterWebContentsFrame.) > > Let's remove the RVH version and just put a comment on the declaration of this > method in render_frame_host_manager.h, saying that it should only be called for > OOPIF-based inner WebContents. Acknowledged. https://codereview.chromium.org/2165523004/diff/330001/content/public/browser... File content/public/browser/guest_mode.h (right): https://codereview.chromium.org/2165523004/diff/330001/content/public/browser... content/public/browser/guest_mode.h:24: static bool IsInnerWebContentsUsingCrossProcessFrames( On 2016/09/21 22:18:21, Charlie Reis (slow) wrote: > Given that we're calling this class GuestMode (and other "guest" references > exist in content/ names), I'm ok with the simpler IsCrossProcessFrameGuest > method name that we had before. I think that's a bit easier to read. > > (The comment can keep the "inner WebContents" reference.) Acknowledged. https://codereview.chromium.org/2165523004/diff/330001/content/public/browser... content/public/browser/guest_mode.h:29: static bool ForInnerWebContentsUsingCrossProcessFrames(RenderViewHost* rvh); On 2016/09/21 22:18:21, Charlie Reis (slow) wrote: > IsCrossProcessFrameGuest(rvh) > > (Actually, we can remove this. See my comment in RFHM.) Acknowledged.
Thanks for the updates! LGTM. https://codereview.chromium.org/2165523004/diff/390001/content/public/browser... File content/public/browser/guest_mode.h (right): https://codereview.chromium.org/2165523004/diff/390001/content/public/browser... content/public/browser/guest_mode.h:18: // https://crbug.com/642826 is resolved. I think we can leave out this TODO. Even when the PDF case supports OOPIFs, we'll still need a policy function to control the Finch trial, and we might as well keep this one (and the new one you've added in the renderer). https://codereview.chromium.org/2165523004/diff/390001/content/public/rendere... File content/public/renderer/guest_mode.h (right): https://codereview.chromium.org/2165523004/diff/390001/content/public/rendere... content/public/renderer/guest_mode.h:16: // https://crbug.com/642826 is resolved. Same: No need for this TODO, since we'll likely keep this for the Finch trial.
Thank you Charlie for the review! Istiaque, could you please take another look? Thanks! https://codereview.chromium.org/2165523004/diff/390001/content/public/browser... File content/public/browser/guest_mode.h (right): https://codereview.chromium.org/2165523004/diff/390001/content/public/browser... content/public/browser/guest_mode.h:18: // https://crbug.com/642826 is resolved. On 2016/09/23 05:21:26, Charlie Reis (slow) wrote: > I think we can leave out this TODO. Even when the PDF case supports OOPIFs, > we'll still need a policy function to control the Finch trial, and we might as > well keep this one (and the new one you've added in the renderer). Acknowledged. https://codereview.chromium.org/2165523004/diff/390001/content/public/rendere... File content/public/renderer/guest_mode.h (right): https://codereview.chromium.org/2165523004/diff/390001/content/public/rendere... content/public/renderer/guest_mode.h:16: // https://crbug.com/642826 is resolved. On 2016/09/23 05:21:26, Charlie Reis (slow) wrote: > Same: No need for this TODO, since we'll likely keep this for the Finch trial. Acknowledged.
still lgtm https://codereview.chromium.org/2165523004/diff/410001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2165523004/diff/410001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:43: #include "content/public/browser/guest_mode.h" I think this isn't used anymore.
extensions/* and render_view_context_menu* lgtm.
Thanks!
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by ekaramad@chromium.org
The CQ bit was checked by ekaramad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/2165523004/#ps410001 (title: "Addressing creis@'s comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Force MimeHandlerView to always use BrowserPlugin Currently, with the flag --use-cross-process-frames-for-guests switched on, MimeHandlerView still creates a BrowserPlugin while on the browser side we expect an OOPIF based implementation. This causes PDF viewer to crash. On the other hand, GuestViews will soon use OOPIF. Hence, temporarily, we need to make MimeHandlerView an exception and force it to use BrowserPlugin's codepath even when the switch above is turned on. BUG=563285 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Force MimeHandlerView to always use BrowserPlugin Currently, with the flag --use-cross-process-frames-for-guests switched on, MimeHandlerView still creates a BrowserPlugin while on the browser side we expect an OOPIF based implementation. This causes PDF viewer to crash. On the other hand, GuestViews will soon use OOPIF. Hence, temporarily, we need to make MimeHandlerView an exception and force it to use BrowserPlugin's codepath even when the switch above is turned on. BUG=563285 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #22 (id:410001)
Message was sent while issue was closed.
Description was changed from ========== Force MimeHandlerView to always use BrowserPlugin Currently, with the flag --use-cross-process-frames-for-guests switched on, MimeHandlerView still creates a BrowserPlugin while on the browser side we expect an OOPIF based implementation. This causes PDF viewer to crash. On the other hand, GuestViews will soon use OOPIF. Hence, temporarily, we need to make MimeHandlerView an exception and force it to use BrowserPlugin's codepath even when the switch above is turned on. BUG=563285 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Force MimeHandlerView to always use BrowserPlugin Currently, with the flag --use-cross-process-frames-for-guests switched on, MimeHandlerView still creates a BrowserPlugin while on the browser side we expect an OOPIF based implementation. This causes PDF viewer to crash. On the other hand, GuestViews will soon use OOPIF. Hence, temporarily, we need to make MimeHandlerView an exception and force it to use BrowserPlugin's codepath even when the switch above is turned on. BUG=563285 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/1beabeccb02b9c6c7e4706950dba275c95e18aa8 Cr-Commit-Position: refs/heads/master@{#420712} ==========
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/1beabeccb02b9c6c7e4706950dba275c95e18aa8 Cr-Commit-Position: refs/heads/master@{#420712}
Message was sent while issue was closed.
https://codereview.chromium.org/2165523004/diff/410001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2165523004/diff/410001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:43: #include "content/public/browser/guest_mode.h" On 2016/09/23 16:23:02, lfg wrote: > I think this isn't used anymore. Sorry I missed this one out. Submitted a patch to remove it. Thanks! |
