content/browser/renderer_host/DEPS - Issue 2417693002: Allow MimeHandlerViewGuest be embedded inside OOPIFs

Unified Diff: content/browser/renderer_host/DEPS

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

Patch Set: Fixed a compile error and added some comments Created 4 years, 1 month ago

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

Jump to:

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

« content/browser/browser_plugin/browser_plugin_guest.cc ('K') | « content/browser/frame_host/render_widget_host_view_guest.h ('k') | content/browser/renderer_host/render_widget_host_input_event_router.cc » ('j') | content/public/browser/browser_plugin_guest_delegate.h » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: content/browser/renderer_host/DEPS

diff --git a/content/browser/renderer_host/DEPS b/content/browser/renderer_host/DEPS

index 135250c17845d91546cc91ee0c212ab6f40f04f2..24c1d5af14a9155026c0d2257761a4dab7183be1 100644

--- a/content/browser/renderer_host/DEPS

+++ b/content/browser/renderer_host/DEPS

@@ -47,5 +47,6 @@ specific_include_rules = {

# to content/renderer_host, at which time this can be removed.

"render_widget_host_input_event_router.cc": [

"+content/browser/frame_host/render_widget_host_view_child_frame.h",

+ "+content/browser/frame_host/render_widget_host_view_guest.h",

Charlie Reis 2016/11/15 23:45:33 I'd really like to avoid making changes to DEPS if

EhsanK 2016/11/16 21:27:31 I actually explained this in patch 12 (copying ove

Charlie Reis 2016/11/16 22:25:26 Thanks, and sorry for missing the earlier explanat

Thanks, and sorry for missing the earlier explanation. I'd lean towards #2, but I'm ok doing that in a separate CL to avoid holding this one up further. (Out of curiosity, will RWHVGuest itself go away as we replace BrowserPlugin with OOPIFs, or will it always be necessary?)

EhsanK 2016/11/16 22:47:49 Sure. I will revert this to patch 12 in a separate

On 2016/11/16 22:25:26, Charlie Reis (slow) wrote: > On 2016/11/16 21:27:31, EhsanK wrote: > > On 2016/11/15 23:45:33, Charlie Reis (slow) wrote: > > > I'd really like to avoid making changes to DEPS if possible, especially in a > > > block that has a TODO to be removed. Why is it needed? Can we avoid it? > > > > > > (If it has to stay, please fix the indentation and update the comment.) > > > > I actually explained this in patch 12 (copying over): > > > > On 2016/11/02 20:47:06, Charlie Reis (OOO til 11-11) wrote: > > > > Why does this need to be here, if it's implemented as a NOTREACHED and > only > > > > callable on the subclass (RWHVG)? Would it be easier to cast to RWHVG at > > the > > > > only callsite (RenderWidgetHostInputEventRouter::RouteMouseEvent) and > leave > > it > > > > out of this class? > > > > > Yes, it would definitely be easier (lfg@ also suggested this > earlier). I > > avoided > > > doing this originally because this is a layering violation (RWHVG > lives > > inside > > > frame_host/ while RWHIER is inside renderer_host). > > > > > But as I understand this approached is being favored over adding > the > method > > to > > > RWHVCF esp. given that we already violate this for RWHVCF (there > is also a > > > comment about it inside the DEPS file for renderer_host/). > > > > > I made the change and modified the DEPS file. > > > > Also please note that in RenderWidgetHostInputEventRouter we need to know the > > target is a RWHVG so that the mouse event is sent to the embedder (not > directly > > routed). > > > > Right now I see the two options of 1) keeping the violation here, 2) making > > GetOwnerRenderWidgetHostView() a virtual method of RWHVCF. For 2) it is also > > worth mentioning that we already have a method > > RenderWidgetHostViewBase::IsRenderWidgetHostViewGuest which is a bit out of > > place but necessary, and for 1), well there is already a violation here. WDYT? > > Thanks, and sorry for missing the earlier explanation. > > I'd lean towards #2, but I'm ok doing that in a separate CL to avoid holding > this one up further. > > (Out of curiosity, will RWHVGuest itself go away as we replace BrowserPlugin > with OOPIFs, or will it always be necessary?)

Sure. I will revert this to patch 12 in a separate followup CL (with a new test added as you suggested).

}