Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1412923009:
Route touch-events for WebViewGuest directly to guest renderer. (Closed)
DescriptionRoute touch-events for WebViewGuest directly to guest renderer.
This CL is the beginning of a refactor to route input events directly
to a WebViewGuest's renderer instead of taking a round-trip through
BrowserPlugin first.
Touch events were chosen as the first event type to re-route, since
guests with touch-handlers installed currently bypass our attempts to
force pinch-zoom to be handled in the embedder. Also, they require a
second gesture recognizer (owned in RenderWidgetHostViewGuest) to
create gestures based on the acked touch events.
This CL does away with the extra GestureRecognizer, and routes the
touch event acks back to the main GestureRecognizer instead, thus
simplifying the event handling logic, and providing more consistent
behaviour when touch handlers are present.
BUG=445319
Committed: https://crrev.com/e31234f7e0950fa7f062a42ed530b76ba3bd1bac
Cr-Commit-Position: refs/heads/master@{#365034}
Patch Set 1 #Patch Set 2 : Experiment: run on bots without DCHECK. #
Total comments: 23
Patch Set 3 : Address comments. #
Total comments: 2
Patch Set 4 : Suggestions, early registration for all but RWHVG. #Patch Set 5 : For WebView, check to make sure surface not already added. #
Total comments: 4
Patch Set 6 : Address suggestions. #
Total comments: 2
Patch Set 7 : Add DCHECK for touch events in BrowserPlugin::handleInputEvent(). #
Total comments: 1
Patch Set 8 : Fix attach/detach surface namespace id registration, embedder focuses on touch. #Patch Set 9 : Fix TouchFocusesEmbedder test. #Patch Set 10 : Fix null pointer deref when guest is terminated. #
Total comments: 7
Patch Set 11 : Update Mac code, rebase to master@{#361742}. #
Total comments: 15
Patch Set 12 : Address creis@'s suggestions. #
Total comments: 6
Patch Set 13 : Update comments, DCHECK touch events in RWHVM::ShouldRouteEvent(). #
Total comments: 4
Patch Set 14 : Update comments. #Patch Set 15 : Update comments (saving first this time). #Depends on Patchset: Messages
Total messages: 68 (23 generated)
Description was changed from ========== Route touch-events for WebViewGuest directly to guest renderer. This CL is the beginning of a refactor to route input events directly to a WebViewGuest's renderer instead of taking a round-trip through BrowserPlugin first. Touch events were chosen as the first event type to re-route, since guests with touch-handlers installed currently bypass our attempts to force pinch-zoom to be handled in the embedder. Also, they require a second gesture recognizer (owned in RenderWidgetHostViewGuwst) to create gestures based on the acked touch events. This CL does away with the extra GestureRecognizer, and routes the touch event acks back to the main GestureRecognizer instead, thus simplifying the event handling logic, and providing more consistent behaviour when touch handlers are present. BUG=445319 ========== to ========== Route touch-events for WebViewGuest directly to guest renderer. This CL is the beginning of a refactor to route input events directly to a WebViewGuest's renderer instead of taking a round-trip through BrowserPlugin first. Touch events were chosen as the first event type to re-route, since guests with touch-handlers installed currently bypass our attempts to force pinch-zoom to be handled in the embedder. Also, they require a second gesture recognizer (owned in RenderWidgetHostViewGuwst) to create gestures based on the acked touch events. This CL does away with the extra GestureRecognizer, and routes the touch event acks back to the main GestureRecognizer instead, thus simplifying the event handling logic, and providing more consistent behaviour when touch handlers are present. BUG=445319 ==========
wjmaclean@chromium.org changed reviewers: + kenrb@chromium.org, tdresser@chromium.org
tdresser@ and kenrb@ - could you please each do a first-pass (sanity-check review) on this? I still need to devise some DCHECKS() to catch if we're seeing events acked out of order between different renderer-concumers, but in its current form it seems pretty usable.
Looks reasonable to me at a high level. I gave a few low level comments - I'll take another pass whenever you're ready. https://codereview.chromium.org/1412923009/diff/20001/content/browser/browser... File content/browser/browser_plugin/browser_plugin_guest.h (right): https://codereview.chromium.org/1412923009/diff/20001/content/browser/browser... content/browser/browser_plugin/browser_plugin_guest.h:190: gfx::PointF GetScreenCoordinates(const gfx::PointF& relative_position) const; It's a shame that we need both of these - I assume it would be annoying to get rid of the one returning gfx::Point? https://codereview.chromium.org/1412923009/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/1412923009/diff/20001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_child_frame.cc:268: host_->delegate()->GetInputEventRouter()->AddSurfaceIdNamespaceOwner( It isn't clear to me why this was postponed - is it a performance optimization, or was there a correctness issue? https://codereview.chromium.org/1412923009/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/1412923009/diff/20001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_guest.cc:132: touch.latency); Indentation. https://codereview.chromium.org/1412923009/diff/20001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_guest.cc:133: // Convert coordinates to something suitable for the embedder. Do we ever actually use these co-ordinates? I seem to remember that I was to switch from passing a touch event to just passing the type and unique event id. It might be worth making that change - or at least double checking that this logic is necessary. https://codereview.chromium.org/1412923009/diff/20001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_guest.cc:149: const ui::LatencyInfo& latency) { Maybe GetOwnerRenderWidgetHostView()->ProcessTouchEvent(...) for symmetry with the above? https://codereview.chromium.org/1412923009/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_widget_host_view_guest.h (right): https://codereview.chromium.org/1412923009/diff/20001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_guest.h:43: public ui::GestureEventHelper { We don't need to be a GestureEventHelper anymore, do we? https://codereview.chromium.org/1412923009/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/1412923009/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_input_event_router.cc:91: case blink::WebInputEvent::TouchMove: Might be worth DCHECKing that we have a target in these cases. https://codereview.chromium.org/1412923009/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1412923009/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:2126: this, &mouse_wheel_event); Could we refactor this into ShouldRouteEvent and RouteEvent methods, or something like that? There's a bunch of duplication here, that makes this a bit harder to read.
https://codereview.chromium.org/1412923009/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/1412923009/diff/20001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_child_frame.cc:268: host_->delegate()->GetInputEventRouter()->AddSurfaceIdNamespaceOwner( On 2015/11/02 15:38:24, tdresser wrote: > It isn't clear to me why this was postponed - is it a performance optimization, > or was there a correctness issue? I don't really like this, but I understand why it is here and don't immedatiately see a better alternative. I think it is that the inner WebContents for the guestview isn't attached to the outer WebContents at the point that this is created, so it is not able to register the surfaceID namespace with the correct RenderWidgetHostInputEventRouter. The current code registers it as soon as the surfaceID is created, which is better, but I don't know how else to do this. https://codereview.chromium.org/1412923009/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1412923009/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:2122: // expensive? If so, can the result be stored in a bool? I don't think we can assume a non-null return from GetInputEventRouter() because interstitial pages don't create one and they do implement RenderWidgetHostDelegate.
On 2015/11/02 16:04:22, kenrb wrote: > https://codereview.chromium.org/1412923009/diff/20001/content/browser/frame_h... > File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): > > https://codereview.chromium.org/1412923009/diff/20001/content/browser/frame_h... > content/browser/frame_host/render_widget_host_view_child_frame.cc:268: > host_->delegate()->GetInputEventRouter()->AddSurfaceIdNamespaceOwner( > On 2015/11/02 15:38:24, tdresser wrote: > > It isn't clear to me why this was postponed - is it a performance > optimization, > > or was there a correctness issue? > > I don't really like this, but I understand why it is here and don't > immedatiately see a better alternative. I think it is that the inner WebContents > for the guestview isn't attached to the outer WebContents at the point that this > is created, so it is not able to register the surfaceID namespace with the > correct RenderWidgetHostInputEventRouter. > > The current code registers it as soon as the surfaceID is created, which is > better, but I don't know how else to do this. > > https://codereview.chromium.org/1412923009/diff/20001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_view_aura.cc (right): > > https://codereview.chromium.org/1412923009/diff/20001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_view_aura.cc:2122: // > expensive? If so, can the result be stored in a bool? > I don't think we can assume a non-null return from GetInputEventRouter() because > interstitial pages don't create one and they do implement > RenderWidgetHostDelegate. Ah, that makes sense. Let's add a comment indicating why this needs to be postponed.
I've gone through your initial comments ... I've responded inline with a few questions of my own ... PTAL? https://codereview.chromium.org/1412923009/diff/20001/content/browser/browser... File content/browser/browser_plugin/browser_plugin_guest.h (right): https://codereview.chromium.org/1412923009/diff/20001/content/browser/browser... content/browser/browser_plugin/browser_plugin_guest.h:190: gfx::PointF GetScreenCoordinates(const gfx::PointF& relative_position) const; On 2015/11/02 15:38:24, tdresser wrote: > It's a shame that we need both of these - I assume it would be annoying to get > rid of the one returning gfx::Point? In the context of this CL, yes. Though worth looking at in a follow-on CL. https://codereview.chromium.org/1412923009/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/1412923009/diff/20001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_child_frame.cc:268: host_->delegate()->GetInputEventRouter()->AddSurfaceIdNamespaceOwner( On 2015/11/02 16:04:22, kenrb wrote: > On 2015/11/02 15:38:24, tdresser wrote: > > It isn't clear to me why this was postponed - is it a performance > optimization, > > or was there a correctness issue? > > I don't really like this, but I understand why it is here and don't > immedatiately see a better alternative. I think it is that the inner WebContents > for the guestview isn't attached to the outer WebContents at the point that this > is created, so it is not able to register the surfaceID namespace with the > correct RenderWidgetHostInputEventRouter. > > The current code registers it as soon as the surfaceID is created, which is > better, but I don't know how else to do this. I suppose one alternative would be to leave this registration in the constructor, then have WebViewGuest *de-register* and then re-register at time of first swap ... I *think* that would work, though we'd have to consider it an experiment. I'm not sure how it would affect the RWHVA that's attached to the RWHVG. https://codereview.chromium.org/1412923009/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/1412923009/diff/20001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_guest.cc:132: touch.latency); On 2015/11/02 15:38:24, tdresser wrote: > Indentation. Done. https://codereview.chromium.org/1412923009/diff/20001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_guest.cc:133: // Convert coordinates to something suitable for the embedder. On 2015/11/02 15:38:24, tdresser wrote: > Do we ever actually use these co-ordinates? I seem to remember that I was to > switch from passing a touch event to just passing the type and unique event id. > > It might be worth making that change - or at least double checking that this > logic is necessary. Hmmm, it looks like we're *not* using the coords, just the IDs. I think we used to use the coords, but that was when they went to the local GR, and it would have been expecting everything in coords local to the guest. I'll remove this. https://codereview.chromium.org/1412923009/diff/20001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_guest.cc:149: const ui::LatencyInfo& latency) { On 2015/11/02 15:38:24, tdresser wrote: > Maybe > GetOwnerRenderWidgetHostView()->ProcessTouchEvent(...) > for symmetry with the above? This is more analogous to RenderWidgetHostViewChildFrame::ProcessMouseEvent() I think. In the code above, the events are flowing up (being acked), not down as they are here. https://codereview.chromium.org/1412923009/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_widget_host_view_guest.h (right): https://codereview.chromium.org/1412923009/diff/20001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_guest.h:43: public ui::GestureEventHelper { On 2015/11/02 15:38:25, tdresser wrote: > We don't need to be a GestureEventHelper anymore, do we? I missed this, so thanks for noticing! We can delete a bunch of code here ... https://codereview.chromium.org/1412923009/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/1412923009/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_input_event_router.cc:91: case blink::WebInputEvent::TouchMove: On 2015/11/02 15:38:25, tdresser wrote: > Might be worth DCHECKing that we have a target in these cases. Done. https://codereview.chromium.org/1412923009/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1412923009/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:2122: // expensive? If so, can the result be stored in a bool? On 2015/11/02 16:04:22, kenrb wrote: > I don't think we can assume a non-null return from GetInputEventRouter() because > interstitial pages don't create one and they do implement > RenderWidgetHostDelegate. Ok, good to know. https://codereview.chromium.org/1412923009/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:2126: this, &mouse_wheel_event); On 2015/11/02 15:38:25, tdresser wrote: > Could we refactor this into > ShouldRouteEvent and RouteEvent methods, or something like that? > > There's a bunch of duplication here, that makes this a bit harder to read. ShouldRouteEvent() was fairly easy, since we can key it off ui::Event*, but a single RouteEvent() method would be quite a bit messier ... on account of the different signatures, creating WebInputEvent types, etc. Is this version better?
https://codereview.chromium.org/1412923009/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/1412923009/diff/20001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_child_frame.cc:268: host_->delegate()->GetInputEventRouter()->AddSurfaceIdNamespaceOwner( On 2015/11/02 19:23:10, wjmaclean wrote: > On 2015/11/02 16:04:22, kenrb wrote: > > On 2015/11/02 15:38:24, tdresser wrote: > > > It isn't clear to me why this was postponed - is it a performance > > optimization, > > > or was there a correctness issue? > > > > I don't really like this, but I understand why it is here and don't > > immedatiately see a better alternative. I think it is that the inner > WebContents > > for the guestview isn't attached to the outer WebContents at the point that > this > > is created, so it is not able to register the surfaceID namespace with the > > correct RenderWidgetHostInputEventRouter. > > > > The current code registers it as soon as the surfaceID is created, which is > > better, but I don't know how else to do this. > > I suppose one alternative would be to leave this registration in the > constructor, then have WebViewGuest *de-register* and then re-register at time > of first swap ... I *think* that would work, though we'd have to consider it an > experiment. I'm not sure how it would affect the RWHVA that's attached to the > RWHVG. This seems reasonable to me. https://codereview.chromium.org/1412923009/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/1412923009/diff/20001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_guest.cc:133: // Convert coordinates to something suitable for the embedder. On 2015/11/02 19:23:10, wjmaclean wrote: > On 2015/11/02 15:38:24, tdresser wrote: > > Do we ever actually use these co-ordinates? I seem to remember that I was to > > switch from passing a touch event to just passing the type and unique event > id. > > > > It might be worth making that change - or at least double checking that this > > logic is necessary. > > Hmmm, it looks like we're *not* using the coords, just the IDs. I think we used > to use the coords, but that was when they went to the local GR, and it would > have been expecting everything in coords local to the guest. I'll remove this. Can you file a bug to stop passing the full event here, and reference it in a comment? Feel free to assign it to me. https://codereview.chromium.org/1412923009/diff/20001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_guest.cc:149: const ui::LatencyInfo& latency) { On 2015/11/02 19:23:10, wjmaclean wrote: > On 2015/11/02 15:38:24, tdresser wrote: > > Maybe > > GetOwnerRenderWidgetHostView()->ProcessTouchEvent(...) > > for symmetry with the above? > > This is more analogous to RenderWidgetHostViewChildFrame::ProcessMouseEvent() I > think. In the code above, the events are flowing up (being acked), not down as > they are here. SGTM. https://codereview.chromium.org/1412923009/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1412923009/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:2126: this, &mouse_wheel_event); On 2015/11/02 19:23:10, wjmaclean wrote: > On 2015/11/02 15:38:25, tdresser wrote: > > Could we refactor this into > > ShouldRouteEvent and RouteEvent methods, or something like that? > > > > There's a bunch of duplication here, that makes this a bit harder to read. > > ShouldRouteEvent() was fairly easy, since we can key it off ui::Event*, but a > single RouteEvent() method would be quite a bit messier ... on account of the > different signatures, creating WebInputEvent types, etc. > > Is this version better? This looks good to me. https://codereview.chromium.org/1412923009/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/1412923009/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_input_event_router.cc:96: case blink::WebInputEvent::TouchCancel: Check for target here too.
This patchset reverts to allowing RWHVCF and RWHVA to register their surface namespaces at construction, and just have RWHVG do a late registration. This patchset is primarily intended to run on the try bots, but if the bots are happy, then please take a look. https://codereview.chromium.org/1412923009/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/1412923009/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_input_event_router.cc:96: case blink::WebInputEvent::TouchCancel: On 2015/11/02 21:10:19, tdresser wrote: > Check for target here too. Done.
On 2015/11/03 14:54:25, wjmaclean wrote: > This patchset reverts to allowing RWHVCF and RWHVA to register their surface > namespaces at construction, and just have RWHVG do a late registration. > > This patchset is primarily intended to run on the try bots, but if the bots are > happy, then please take a look. > > https://codereview.chromium.org/1412923009/diff/40001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_input_event_router.cc > (right): > > https://codereview.chromium.org/1412923009/diff/40001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_input_event_router.cc:96: case > blink::WebInputEvent::TouchCancel: > On 2015/11/02 21:10:19, tdresser wrote: > > Check for target here too. > > Done. Bots are sad.
On 2015/11/04 14:27:49, tdresser wrote: > On 2015/11/03 14:54:25, wjmaclean wrote: > > This patchset reverts to allowing RWHVCF and RWHVA to register their surface > > namespaces at construction, and just have RWHVG do a late registration. > > > > This patchset is primarily intended to run on the try bots, but if the bots > are > > happy, then please take a look. > > Bots are sad. Yeah, saw that. It seems tests build their hierarchies differently ... I have a mod in mind, will upload this morning.
PTAL?
content/browser/renderer_host/ LGTM https://codereview.chromium.org/1412923009/diff/80001/content/browser/browser... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/1412923009/diff/80001/content/browser/browser... content/browser/browser_plugin/browser_plugin_guest.cc:499: gfx::Point BrowserPluginGuest::GetScreenCoordinates( Can this be rewritten in terms of the other GetScreenCoordinates method? https://codereview.chromium.org/1412923009/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/1412923009/diff/80001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_guest.cc:132: // Since all ProcessAckedTouchEvent() uses is the event id, don't pass the Let's call this TODO(tdresser):
kenrb@ - did you have any further suggestions on this one? https://codereview.chromium.org/1412923009/diff/80001/content/browser/browser... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/1412923009/diff/80001/content/browser/browser... content/browser/browser_plugin/browser_plugin_guest.cc:499: gfx::Point BrowserPluginGuest::GetScreenCoordinates( On 2015/11/04 19:24:17, tdresser wrote: > Can this be rewritten in terms of the other GetScreenCoordinates method? My bad ... the float version of this function should have disappeared in the last patch (when we removed the coord conversion from RWHVG::ProcessAckedTouchEvents()). Gone. https://codereview.chromium.org/1412923009/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/1412923009/diff/80001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_guest.cc:132: // Since all ProcessAckedTouchEvent() uses is the event id, don't pass the On 2015/11/04 19:24:17, tdresser wrote: > Let's call this > TODO(tdresser): Done.
https://codereview.chromium.org/1412923009/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1412923009/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:1597: return GetOuterWebContents()->GetInputEventRouter(); If we create an input event router before being attached to the outer WebContents, does that persist as an unused object containing stale SurfaceIdNamespace/RWHV entries? It might not be a problem, as long as we can guarantee that those entries never get referenced, but I would worry about the potential for bugs down the road if we are leaving around stale state like that.
https://codereview.chromium.org/1412923009/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1412923009/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:1597: return GetOuterWebContents()->GetInputEventRouter(); On 2015/11/05 17:05:26, kenrb wrote: > If we create an input event router before being attached to the outer > WebContents, does that persist as an unused object containing stale > SurfaceIdNamespace/RWHV entries? > > It might not be a problem, as long as we can guarantee that those entries never > get referenced, but I would worry about the potential for bugs down the road if > we are leaving around stale state like that. Yes, at present we do leave these around. I thought about deleting existing entries before adding the new ones, but we'd had to have a way of either remembering or retrieving the *old* InputEventROuter, which would make GetInputEventrouter() much more complicated (though we could have a separate function like GetLocalInputEventRouter() that wouldn't defer to the OuterWebContents if it exists, and use it to remove things?) At present, the input event router doesn't guarantee that its map is empty at destruction time, so it's conceivable we could have stale state around even outside of this CL. Let me know if you'd like me to go the removal route ...
fsamuel@chromium.org changed reviewers: + fsamuel@chromium.org
https://codereview.chromium.org/1412923009/diff/120001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/1412923009/diff/120001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_guest.cc:211: if (!has_started_rendering_) { drive-by: This doesn't seem right. A guest can be attached and detached multiple times during its lifetime. In fact, every time you set display:none, the guest is detached, and then display:block will reattach it to a new BrowserPlugin. This is not a good signal for attachment.
On 2015/11/05 18:09:11, Fady Samuel wrote: > https://codereview.chromium.org/1412923009/diff/120001/content/browser/frame_... > File content/browser/frame_host/render_widget_host_view_guest.cc (right): > > https://codereview.chromium.org/1412923009/diff/120001/content/browser/frame_... > content/browser/frame_host/render_widget_host_view_guest.cc:211: if > (!has_started_rendering_) { > drive-by: This doesn't seem right. A guest can be attached and detached multiple > times during its lifetime. In fact, every time you set display:none, the guest > is detached, and then display:block will reattach it to a new BrowserPlugin. > > This is not a good signal for attachment. We can do this (and undo it) in the attach/detach methods for BrowserPluginGuest (and relayed bay to RWHVG) ... is that reasonable?
On 2015/11/05 18:12:14, wjmaclean wrote: > On 2015/11/05 18:09:11, Fady Samuel wrote: > > > https://codereview.chromium.org/1412923009/diff/120001/content/browser/frame_... > > File content/browser/frame_host/render_widget_host_view_guest.cc (right): > > > > > https://codereview.chromium.org/1412923009/diff/120001/content/browser/frame_... > > content/browser/frame_host/render_widget_host_view_guest.cc:211: if > > (!has_started_rendering_) { > > drive-by: This doesn't seem right. A guest can be attached and detached > multiple > > times during its lifetime. In fact, every time you set display:none, the guest > > is detached, and then display:block will reattach it to a new BrowserPlugin. > > > > This is not a good signal for attachment. > > We can do this (and undo it) in the attach/detach methods for BrowserPluginGuest > (and relayed bay to RWHVG) ... is that reasonable? Yup. That seems reasonable! Thanks!
On 2015/11/05 18:13:13, Fady Samuel wrote: > > > drive-by: This doesn't seem right. A guest can be attached and detached multiple > > > times during its lifetime. In fact, every time you set display:none, the guest > > > is detached, and then display:block will reattach it to a new BrowserPlugin. I just did an experiment, and found that BrowserPluginGuest::OnDetach() doesn't seem to get called on display:none, though BrowserPluginGuest::OnWillAttachComplete() does get called (again) on removing display:none. Is this expected? BPG::OnDetach() does get called when a WebView element is deleted from the embedder document.
On 2015/11/17 20:43:57, wjmaclean wrote:
> On 2015/11/05 18:13:13, Fady Samuel wrote:
> > > > drive-by: This doesn't seem right. A guest can be attached and detached
> multiple
> > > > times during its lifetime. In fact, every time you set display:none, the
> guest
> > > > is detached, and then display:block will reattach it to a new
> BrowserPlugin.
>
> I just did an experiment, and found that BrowserPluginGuest::OnDetach()
doesn't
> seem to get called on display:none, though
> BrowserPluginGuest::OnWillAttachComplete() does get called (again) on removing
> display:none.
>
> Is this expected? BPG::OnDetach() does get called when a WebView element is
> deleted from the embedder document.
This looks like a bug. I think you want to send a detach IPC before destroying
the BrowserPlugin. Queuing of IPCs in BrowserPluginGuest is probably broken on
display:none. For example, if you start <webview
style="display:none;"></webview> and then
document.querySelector('webview').allowtransparency = true; then
document.querySelector('webview').style.display = 'block'; The ContentsOpaque
IPC may not get queued, and may get dropped (not 100% sure about this but I
suspect that's the case).
Fixed a couple of failing tests, added Detach() call in BrowserPlugin destructor. We now use attach/detach for registration with RenderWidgetHostInputEventRouter. kenrb@ - I think I've addressed your comments, ptal? fsamuel@ - does this look better?
lgtm + nit https://codereview.chromium.org/1412923009/diff/180001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/1412923009/diff/180001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_guest.cc:145: if (!embedder->GetView()->HasFocus()) This seems redundant. Is there anything wrong with always calling Focus()?
https://codereview.chromium.org/1412923009/diff/180001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/1412923009/diff/180001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:812: web_contents()->GetRenderWidgetHostView()); I don't exactly know how much this is a concern, but if we end up with a Guest that contains out-of-process iframes, then their RenderWidgetHostViews won't register/de-register, right? https://codereview.chromium.org/1412923009/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1412923009/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:868: result = result && SiteIsolationPolicy::AreCrossProcessFramesPossible(); I think we also need this change for RenderWidgetHostViewMac.
Patchset #11 (id:200001) has been deleted
Patchset #12 (id:240001) has been deleted
Patchset #12 (id:260001) has been deleted
Patchset #12 (id:280001) has been deleted
kenrb@ - I've update the mac code ... let me know what you think. https://codereview.chromium.org/1412923009/diff/180001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/1412923009/diff/180001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:812: web_contents()->GetRenderWidgetHostView()); On 2015/11/25 19:33:06, kenrb wrote: > I don't exactly know how much this is a concern, but if we end up with a Guest > that contains out-of-process iframes, then their RenderWidgetHostViews won't > register/de-register, right? I don't think so, but at least the touch events start at the guest's top-level frame, which is still better than going through the embedder. Do we treat iFrames/oopIfs with touch handlers differently than those without? https://codereview.chromium.org/1412923009/diff/180001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/1412923009/diff/180001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_guest.cc:145: if (!embedder->GetView()->HasFocus()) On 2015/11/25 18:31:32, Fady Samuel wrote: > This seems redundant. Is there anything wrong with always calling Focus()? I think the reason we did this in the original CL was to avoid the overhead that is involved in calling focus when the window is already focused. Calling HasFocus() has much lower overhead, and Focus() doesn't seem to have any early-out. https://codereview.chromium.org/1412923009/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1412923009/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:868: result = result && SiteIsolationPolicy::AreCrossProcessFramesPossible(); On 2015/11/25 19:33:06, kenrb wrote: > I think we also need this change for RenderWidgetHostViewMac. Done.
Patchset #11 (id:220001) has been deleted
https://codereview.chromium.org/1412923009/diff/180001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/1412923009/diff/180001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:812: web_contents()->GetRenderWidgetHostView()); On 2015/11/26 13:01:03, wjmaclean wrote: > On 2015/11/25 19:33:06, kenrb wrote: > > I don't exactly know how much this is a concern, but if we end up with a Guest > > that contains out-of-process iframes, then their RenderWidgetHostViews won't > > register/de-register, right? > > I don't think so, but at least the touch events start at the guest's top-level > frame, which is still better than going through the embedder. Do we treat > iFrames/oopIfs with touch handlers differently than those without? OOPIFs don't support touch events yet, but I don't think we want to treat them differently for having touch handlers. My concern here is that mouse event hit testing might be broken for webview under --site-per-process, because the OOPIF RWHVs won't have registered their Surface ID namespaces through the correct WebContents.
On 2015/11/26 16:19:14, kenrb wrote: > https://codereview.chromium.org/1412923009/diff/180001/content/browser/browse... > File content/browser/browser_plugin/browser_plugin_guest.cc (right): > > https://codereview.chromium.org/1412923009/diff/180001/content/browser/browse... > content/browser/browser_plugin/browser_plugin_guest.cc:812: > web_contents()->GetRenderWidgetHostView()); > On 2015/11/26 13:01:03, wjmaclean wrote: > > On 2015/11/25 19:33:06, kenrb wrote: > > > I don't exactly know how much this is a concern, but if we end up with a > Guest > > > that contains out-of-process iframes, then their RenderWidgetHostViews won't > > > register/de-register, right? > > > > I don't think so, but at least the touch events start at the guest's top-level > > frame, which is still better than going through the embedder. Do we treat > > iFrames/oopIfs with touch handlers differently than those without? > > OOPIFs don't support touch events yet, but I don't think we want to treat them > differently for having touch handlers. My concern here is that mouse event hit > testing might be broken for webview under --site-per-process, because the OOPIF > RWHVs won't have registered their Surface ID namespaces through the correct > WebContents. Currently --site-per-process doesn't work with WebView touch events, with or without this CL, so we're not ending up any worse off. With this CL touch events will still be sent to the top-level frame, since no sub-frame will register with the top-level InputEventRouter, but I don't think that's any different than what already happens. Definitely the next thing to do after this is to make WebView touch events work with --site-per-process, and in that same CL we can likely wire up the hit-testing for regular oop-iframes as well.
As discussed offline, this doesn't seem to break anything that isn't already broken, so lgtm.
wjmaclean@chromium.org changed reviewers: + creis@chromium.org
creis@ - this has been pre-reviewed by kenrb@ ... could you please take a look?
A few nits, but one larger concern that this seems to be changing how touch events are handled in all cases, not just BrowserPlugin (see my last comment). I'm not sure if that's intentional. https://codereview.chromium.org/1412923009/diff/300001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.h (right): https://codereview.chromium.org/1412923009/diff/300001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.h:190: gfx::PointF GetScreenCoordinates(const gfx::PointF& relative_position) const; This isn't defined anywhere. Stale? https://codereview.chromium.org/1412923009/diff/300001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/1412923009/diff/300001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_guest.cc:131: // TODO(TDRESSER): Since all ProcessAckedTouchEvent() uses is the event id, nit: s/TDRESSER/tdresser/ https://codereview.chromium.org/1412923009/diff/300001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_guest.cc:141: DCHECK(guest_); This is redundant with the next line. https://codereview.chromium.org/1412923009/diff/300001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1412923009/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:867: if (event->IsMouseEvent()) Please add a comment in this method about what policy you're trying to achieve. It's hard to understand what this means as is-- are we just in an intermediate state where we use the input event router for touch but not mouse events? Also, is this mean to affect non-BrowserPlugin code? It looks like it affects all views in default Chrome. https://codereview.chromium.org/1412923009/diff/300001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/1412923009/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_mac.mm:1620: result = result && SiteIsolationPolicy::AreCrossProcessFramesPossible(); Same concerns as in RWHVAura. https://codereview.chromium.org/1412923009/diff/300001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1412923009/diff/300001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:1611: rwh_input_event_router_.reset(new RenderWidgetHostInputEventRouter); Before this CL, we never created this in default Chrome, and now we always create it (during RWHVAura's constructor). There used to be a lot of code that checks for its existence (e.g., several within RWHVAura), and even with your addition of ShouldRouteEvent it looks like we'll be using the new path for touch events by default (not just in BrowserPlugin). Is this CL intended to affect all those cases?
Updated as per creis@'s comments, ptal? https://codereview.chromium.org/1412923009/diff/300001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.h (right): https://codereview.chromium.org/1412923009/diff/300001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.h:190: gfx::PointF GetScreenCoordinates(const gfx::PointF& relative_position) const; On 2015/11/30 18:09:11, Charlie Reis wrote: > This isn't defined anywhere. Stale? Yes, it's stale - removed. Done. https://codereview.chromium.org/1412923009/diff/300001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/1412923009/diff/300001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_guest.cc:131: // TODO(TDRESSER): Since all ProcessAckedTouchEvent() uses is the event id, On 2015/11/30 18:09:11, Charlie Reis wrote: > nit: s/TDRESSER/tdresser/ Done. https://codereview.chromium.org/1412923009/diff/300001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_guest.cc:141: DCHECK(guest_); On 2015/11/30 18:09:11, Charlie Reis wrote: > This is redundant with the next line. Done. https://codereview.chromium.org/1412923009/diff/300001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1412923009/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:867: if (event->IsMouseEvent()) On 2015/11/30 18:09:11, Charlie Reis wrote: > Please add a comment in this method about what policy you're trying to achieve. > It's hard to understand what this means as is-- are we just in an intermediate > state where we use the input event router for touch but not mouse events? Done. > Also, is this mean to affect non-BrowserPlugin code? It looks like it affects > all views in default Chrome. It doesn't really affect all views, as in the CL only RenderWidgetHostViewAura calls RenderWidgetHostViewBase::ProcessTouchEvent(). This means that RenderWidgetHostView{Mac|Android} each continue to do their own thing. At present touch forwarding is not supported in OOPIF, so this code doesn't break anything, though it does provide a way forward in a subsequent CL. https://codereview.chromium.org/1412923009/diff/300001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/1412923009/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_mac.mm:1620: result = result && SiteIsolationPolicy::AreCrossProcessFramesPossible(); On 2015/11/30 18:09:12, Charlie Reis wrote: > Same concerns as in RWHVAura. This will only (at present) be called for Mouse/Wheel events in RWHVM. If RWHVM implements a touch event mechanism, then we will need to also implement a RWHVM::ProcessTouchEvent() function. https://codereview.chromium.org/1412923009/diff/300001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1412923009/diff/300001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:1611: rwh_input_event_router_.reset(new RenderWidgetHostInputEventRouter); On 2015/11/30 18:09:12, Charlie Reis wrote: > Before this CL, we never created this in default Chrome, and now we always > create it (during RWHVAura's constructor). > > There used to be a lot of code that checks for its existence (e.g., several > within RWHVAura), and even with your addition of ShouldRouteEvent it looks like > we'll be using the new path for touch events by default (not just in > BrowserPlugin). Is this CL intended to affect all those cases? This CL is targeted to (in the short term) fixing touch events for BrowserPlugin. However, it can be implemented generally since checks for the existence of an IER are specific to (1) creation/destruction of RWHV{A|CF} or (2) processing mouse/wheel/touch events, only the latter of which will have effect without --site-per-process. Other view types, namely RWHVM and RWHVAndroid, either do not yet make use of the IER (RWHVAndroid) or don't use it for touch (RWHVM).
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412923009/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412923009/320001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hmm, I'm having trouble following your replies. Please see my questions below. https://codereview.chromium.org/1412923009/diff/300001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1412923009/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:867: if (event->IsMouseEvent()) On 2015/12/10 16:09:07, wjmaclean wrote: > On 2015/11/30 18:09:11, Charlie Reis wrote: > > Please add a comment in this method about what policy you're trying to > achieve. > > It's hard to understand what this means as is-- are we just in an intermediate > > state where we use the input event router for touch but not mouse events? > > Done. > > > Also, is this mean to affect non-BrowserPlugin code? It looks like it affects > > all views in default Chrome. > > It doesn't really affect all views, as in the CL only RenderWidgetHostViewAura > calls RenderWidgetHostViewBase::ProcessTouchEvent(). This means that > RenderWidgetHostView{Mac|Android} each continue to do their own thing. I think you misunderstood my question. By default Chrome, I meant platforms that use RenderWidgetHostViewAura (i.e., Windows, Linux, and ChromeOS). I'm concerned that this affects behavior for Windows/Linux/ChromeOS, even for users who don't have --site-per-process or BrowserPlugins. It's not clear to me whether that was intentional. > > At present touch forwarding is not supported in OOPIF, so this code doesn't > break anything, though it does provide a way forward in a subsequent CL. https://codereview.chromium.org/1412923009/diff/300001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/1412923009/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_mac.mm:1620: result = result && SiteIsolationPolicy::AreCrossProcessFramesPossible(); On 2015/12/10 16:09:07, wjmaclean wrote: > On 2015/11/30 18:09:12, Charlie Reis wrote: > > Same concerns as in RWHVAura. > > This will only (at present) be called for Mouse/Wheel events in RWHVM. If RWHVM > implements a touch event mechanism, then we will need to also implement a > RWHVM::ProcessTouchEvent() function. This still needs a comment. https://codereview.chromium.org/1412923009/diff/300001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1412923009/diff/300001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:1611: rwh_input_event_router_.reset(new RenderWidgetHostInputEventRouter); On 2015/12/10 16:09:07, wjmaclean wrote: > On 2015/11/30 18:09:12, Charlie Reis wrote: > > Before this CL, we never created this in default Chrome, and now we always > > create it (during RWHVAura's constructor). > > > > There used to be a lot of code that checks for its existence (e.g., several > > within RWHVAura), and even with your addition of ShouldRouteEvent it looks > like > > we'll be using the new path for touch events by default (not just in > > BrowserPlugin). Is this CL intended to affect all those cases? > > This CL is targeted to (in the short term) fixing touch events for > BrowserPlugin. However, it can be implemented generally since checks for the > existence of an IER are specific to (1) creation/destruction of RWHV{A|CF} or > (2) processing mouse/wheel/touch events, only the latter of which will have > effect without --site-per-process. Other view types, namely RWHVM and > RWHVAndroid, either do not yet make use of the IER (RWHVAndroid) or don't use > it for touch (RWHVM). This doesn't sound safe to me. If I read this correctly, for case (1) we'll create an InputEventRouter for all RenderWidgetHostViewAuras. That affects desktop builds on Windows, Linux, and ChromeOS, whether they have --site-per-process or guests or not. That seems like a behavior change to default Chrome on those platforms. For case (2), the presence of an IER without --site-per-process will mean that mouse/wheel/touch events will behave differently on Windows and Linux. My earlier question is whether this behavior change is intended-- do those events still work as well as before on Windows/Linux/ChromeOS without --site-per-process? https://codereview.chromium.org/1412923009/diff/320001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1412923009/diff/320001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:863: // 1) If cross-process frames are possible, then we route the event. This doesn't seem to be what the code below is doing. The comment makes it sound like "cross-process frames are possible" implies "route the event." The code below doesn't seem to take AreCrossProcessFramesPossible into account unless |result| is true, in which case *both* |result| and AreCrossProcessFramesPossible need to be true. https://codereview.chromium.org/1412923009/diff/320001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:868: bool result = host_->delegate() && host_->delegate()->GetInputEventRouter(); This looks like it will almost always be true (unless the tab is being destroyed), due to the change to WebContents. https://codereview.chromium.org/1412923009/diff/320001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:870: result = result && SiteIsolationPolicy::AreCrossProcessFramesPossible(); On a related note, we should confirm whether AreCrossProcessFramesPossible is the right thing to be checking here. In the near future, we'll likely be turning on --isolate-extensions by default, which means AreCrossProcessFramesPossible will always return true (since there will be OOPIFs for web iframes inside extensions). --site-per-process mode is still further out. Is that what you'd expect? Are there blocking issues here before we turn that on by default?
I've tried to clarify my answers based on your reply, please let me know if I'm doing a better job explaining the intent of the CL. > I think you misunderstood my question. By default > Chrome, I meant platforms that use > RenderWidgetHostViewAura (i.e., > Windows, Linux, and ChromeOS). I'm concerned that this > affects behavior for Windows/Linux/ChromeOS, even for > users who don't have --site-per-process or > BrowserPlugins. It's not clear to me whether that was > intentional. I don't think this changes anything for RWHVA users, unless they have a BrowserPlugin. Yes, we create an IER, but without --site-per-process (modulo the discussion below about AreCrossSiteFramesPossible) it shouldn't trigger surface hit testing on mouse/wheel events. And touch events, in the absence of a BrowserPlugin, should always hit test to the current RWHVA, in which case ProcessTouchEvent should still do the same thing as before. So I believe the changes here are structural, not behavioural, in the absence of a BrowserPlugin. Behavioural changes will only affect the BrowserPlugin events. > For case (2), the presence of an IER without --site-per- > process will mean that mouse/wheel/touch events will > behave differently on Windows and Linux. Ditto above - I don't think there will be any behavioural change in the absence of a BrowserPlugin. Re the comment in RWHVM, I'll add a comment. https://codereview.chromium.org/1412923009/diff/320001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1412923009/diff/320001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:863: // 1) If cross-process frames are possible, then we route the event. On 2015/12/10 21:58:14, Charlie Reis wrote: > This doesn't seem to be what the code below is doing. The comment makes it > sound like "cross-process frames are possible" implies "route the event." The > code below doesn't seem to take AreCrossProcessFramesPossible into account > unless |result| is true, in which case *both* |result| and > AreCrossProcessFramesPossible need to be true. Perhaps I've badly worded the comment. Prior to this CL we only checked if the IER exists, as that is gated on AreCrossProcessFramesPossible. It also only handled mouse/wheel events. This CL changes it so the IER is always created, but we still don't route mouse/wheel events if AreCrossProcessFramesPossible is false. But we do always route touch events. https://codereview.chromium.org/1412923009/diff/320001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:868: bool result = host_->delegate() && host_->delegate()->GetInputEventRouter(); On 2015/12/10 21:58:15, Charlie Reis wrote: > This looks like it will almost always be true (unless the tab is being > destroyed), due to the change to WebContents. Yes. But |result| can be set false if we are dealing with a mouse/wheel event, and --site-per-process isn't on. https://codereview.chromium.org/1412923009/diff/320001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:870: result = result && SiteIsolationPolicy::AreCrossProcessFramesPossible(); On 2015/12/10 21:58:15, Charlie Reis wrote: > On a related note, we should confirm whether AreCrossProcessFramesPossible is > the right thing to be checking here. OK. This code is based on the existing logic, so if it's wrong in this CL, then I suspect it's also wrong in the existing code. > In the near future, we'll likely be turning on --isolate-extensions by default, > which means AreCrossProcessFramesPossible will always return true (since there > will be OOPIFs for web iframes inside extensions). --site-per-process mode is > still further out. > > Is that what you'd expect? Are there blocking issues here before we turn that > on by default? I certainly wouldn't expect the IER to be enable for all RWHVA's with just --isolate-extensions, so perhaps AreCrossProcessFramesPossible is wrong, but then (as mentioned above) I think it's also wrong for the existing logic.
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412923009/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412923009/340001
On 2015/12/10 22:44:14, wjmaclean wrote: > I've tried to clarify my answers based on your reply, please let me know if I'm > doing a better job explaining the intent of the CL. > > > I think you misunderstood my question. By default > > Chrome, I meant platforms that use > > RenderWidgetHostViewAura (i.e., > > Windows, Linux, and ChromeOS). I'm concerned that this > > affects behavior for Windows/Linux/ChromeOS, even for > > users who don't have --site-per-process or > > BrowserPlugins. It's not clear to me whether that was > > intentional. > > I don't think this changes anything for RWHVA users, unless they have a > BrowserPlugin. Yes, we create an IER, but without --site-per-process (modulo the > discussion below about AreCrossSiteFramesPossible) it shouldn't trigger surface > hit testing on mouse/wheel events. And touch events, in the absence of a > BrowserPlugin, should always hit test to the current RWHVA, in which case > ProcessTouchEvent should still do the same thing as before. > > So I believe the changes here are structural, not behavioural, in the absence of > a BrowserPlugin. Behavioural changes will only affect the BrowserPlugin events. > > > For case (2), the presence of an IER without --site-per- > > process will mean that mouse/wheel/touch events will > > behave differently on Windows and Linux. > > Ditto above - I don't think there will be any behavioural change in the absence > of a BrowserPlugin. > > Re the comment in RWHVM, I'll add a comment. > > https://codereview.chromium.org/1412923009/diff/320001/content/browser/render... > File content/browser/renderer_host/render_widget_host_view_aura.cc (right): > > https://codereview.chromium.org/1412923009/diff/320001/content/browser/render... > content/browser/renderer_host/render_widget_host_view_aura.cc:863: // 1) If > cross-process frames are possible, then we route the event. > On 2015/12/10 21:58:14, Charlie Reis wrote: > > This doesn't seem to be what the code below is doing. The comment makes it > > sound like "cross-process frames are possible" implies "route the event." The > > code below doesn't seem to take AreCrossProcessFramesPossible into account > > unless |result| is true, in which case *both* |result| and > > AreCrossProcessFramesPossible need to be true. > > Perhaps I've badly worded the comment. Prior to this CL we only checked if the > IER exists, as that is gated on AreCrossProcessFramesPossible. It also only > handled mouse/wheel events. > > This CL changes it so the IER is always created, but we still don't route > mouse/wheel events if AreCrossProcessFramesPossible is false. But we do always > route touch events. > > https://codereview.chromium.org/1412923009/diff/320001/content/browser/render... > content/browser/renderer_host/render_widget_host_view_aura.cc:868: bool result = > host_->delegate() && host_->delegate()->GetInputEventRouter(); > On 2015/12/10 21:58:15, Charlie Reis wrote: > > This looks like it will almost always be true (unless the tab is being > > destroyed), due to the change to WebContents. > > Yes. But |result| can be set false if we are dealing with a mouse/wheel event, > and --site-per-process isn't on. > > https://codereview.chromium.org/1412923009/diff/320001/content/browser/render... > content/browser/renderer_host/render_widget_host_view_aura.cc:870: result = > result && SiteIsolationPolicy::AreCrossProcessFramesPossible(); > On 2015/12/10 21:58:15, Charlie Reis wrote: > > On a related note, we should confirm whether AreCrossProcessFramesPossible is > > the right thing to be checking here. > > OK. This code is based on the existing logic, so if it's wrong in this CL, then > I suspect it's also wrong in the existing code. > > > In the near future, we'll likely be turning on --isolate-extensions by > default, > > which means AreCrossProcessFramesPossible will always return true (since there > > will be OOPIFs for web iframes inside extensions). --site-per-process mode is > > still further out. > > > > Is that what you'd expect? Are there blocking issues here before we turn that > > on by default? > > I certainly wouldn't expect the IER to be enable for all RWHVA's with just > --isolate-extensions, so perhaps AreCrossProcessFramesPossible is wrong, but > then (as mentioned above) I think it's also wrong for the existing logic. So I had a quick chat with lfg@, and he confirmed that --isolate-extensions we will be creating IERs for each RWHVA, but since there is (in the absence of BrowserPlugins) just one RWHVA without --site-per-process, and since this CL handles the BrowserPlugin case, we should be ok.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I've added the requested comment to RWHVM::ShouldRouteEvent(), and I've revised that function to explicitly indicate that we don't expect it to receive any touch events. I've also revised the comment in RWHVA::ShouldRouteEvent() to hopefully be clearer. creis@ - PTAL?
Typo in CL description: RenderWidgetHostViewGuwst. On 2015/12/10 22:44:14, wjmaclean wrote: > I've tried to clarify my answers based on your reply, please let me know if I'm > doing a better job explaining the intent of the CL. > > > I think you misunderstood my question. By default > > Chrome, I meant platforms that use > > RenderWidgetHostViewAura (i.e., > > Windows, Linux, and ChromeOS). I'm concerned that this > > affects behavior for Windows/Linux/ChromeOS, even for > > users who don't have --site-per-process or > > BrowserPlugins. It's not clear to me whether that was > > intentional. > > I don't think this changes anything for RWHVA users, unless they have a > BrowserPlugin. Yes, we create an IER, but without --site-per-process (modulo the > discussion below about AreCrossSiteFramesPossible) it shouldn't trigger surface > hit testing on mouse/wheel events. And touch events, in the absence of a > BrowserPlugin, should always hit test to the current RWHVA, in which case > ProcessTouchEvent should still do the same thing as before. This part is what wasn't clear to me before-- that using the IER for ProcessTouchEvent will still do the right thing when there are no BrowserPlugins/OOPIFs involved. > > So I believe the changes here are structural, not behavioural, in the absence of > a BrowserPlugin. Behavioural changes will only affect the BrowserPlugin events. Thanks for confirming. LGTM with nits! On 2015/12/11 16:50:46, wjmaclean wrote: > So I had a quick chat with lfg@, and he confirmed that --isolate-extensions we > will be creating IERs for each RWHVA, but since there is (in the absence of > BrowserPlugins) just one RWHVA without --site-per-process, and since this CL > handles the BrowserPlugin case, we should be ok. Great. https://codereview.chromium.org/1412923009/diff/340001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1412923009/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:869: // frame. Thanks-- this comment is so much clearer! Are the statements about BrowserPlugins also true for OOPIFs? Maybe you can mention both. https://codereview.chromium.org/1412923009/diff/340001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/1412923009/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_mac.mm:1614: // OnTouchEvent(), to match what we are doing in RenderWidgetHostViewAura. nit: Start with // See also RenderWidgetHostViewAura::ShouldRouteEvent. (I think that's useful regardless of the TODO, since these are basically duplicated code. It might be nice to abstract them out at some point.) nit: Also capitalize Update.
Thanks! https://codereview.chromium.org/1412923009/diff/340001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1412923009/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:869: // frame. On 2015/12/11 21:32:52, Charlie Reis wrote: > Thanks-- this comment is so much clearer! > > Are the statements about BrowserPlugins also true for OOPIFs? Maybe you can > mention both. Done. https://codereview.chromium.org/1412923009/diff/340001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/1412923009/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_mac.mm:1614: // OnTouchEvent(), to match what we are doing in RenderWidgetHostViewAura. On 2015/12/11 21:32:52, Charlie Reis wrote: > nit: Start with // See also RenderWidgetHostViewAura::ShouldRouteEvent. > > (I think that's useful regardless of the TODO, since these are basically > duplicated code. It might be nice to abstract them out at some point.) > > nit: Also capitalize Update. Done.
The CQ bit was checked by wjmaclean@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org, fsamuel@chromium.org, kenrb@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/1412923009/#ps360001 (title: "Update comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412923009/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412923009/360001
Description was changed from ========== Route touch-events for WebViewGuest directly to guest renderer. This CL is the beginning of a refactor to route input events directly to a WebViewGuest's renderer instead of taking a round-trip through BrowserPlugin first. Touch events were chosen as the first event type to re-route, since guests with touch-handlers installed currently bypass our attempts to force pinch-zoom to be handled in the embedder. Also, they require a second gesture recognizer (owned in RenderWidgetHostViewGuwst) to create gestures based on the acked touch events. This CL does away with the extra GestureRecognizer, and routes the touch event acks back to the main GestureRecognizer instead, thus simplifying the event handling logic, and providing more consistent behaviour when touch handlers are present. BUG=445319 ========== to ========== Route touch-events for WebViewGuest directly to guest renderer. This CL is the beginning of a refactor to route input events directly to a WebViewGuest's renderer instead of taking a round-trip through BrowserPlugin first. Touch events were chosen as the first event type to re-route, since guests with touch-handlers installed currently bypass our attempts to force pinch-zoom to be handled in the embedder. Also, they require a second gesture recognizer (owned in RenderWidgetHostViewGuest) to create gestures based on the acked touch events. This CL does away with the extra GestureRecognizer, and routes the touch event acks back to the main GestureRecognizer instead, thus simplifying the event handling logic, and providing more consistent behaviour when touch handlers are present. BUG=445319 ==========
The CQ bit was unchecked by wjmaclean@chromium.org
The CQ bit was checked by wjmaclean@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org, fsamuel@chromium.org, creis@chromium.org, tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/1412923009/#ps380001 (title: "Update comments (saving first this time).")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412923009/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412923009/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by wjmaclean@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412923009/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412923009/380001
Message was sent while issue was closed.
Description was changed from ========== Route touch-events for WebViewGuest directly to guest renderer. This CL is the beginning of a refactor to route input events directly to a WebViewGuest's renderer instead of taking a round-trip through BrowserPlugin first. Touch events were chosen as the first event type to re-route, since guests with touch-handlers installed currently bypass our attempts to force pinch-zoom to be handled in the embedder. Also, they require a second gesture recognizer (owned in RenderWidgetHostViewGuest) to create gestures based on the acked touch events. This CL does away with the extra GestureRecognizer, and routes the touch event acks back to the main GestureRecognizer instead, thus simplifying the event handling logic, and providing more consistent behaviour when touch handlers are present. BUG=445319 ========== to ========== Route touch-events for WebViewGuest directly to guest renderer. This CL is the beginning of a refactor to route input events directly to a WebViewGuest's renderer instead of taking a round-trip through BrowserPlugin first. Touch events were chosen as the first event type to re-route, since guests with touch-handlers installed currently bypass our attempts to force pinch-zoom to be handled in the embedder. Also, they require a second gesture recognizer (owned in RenderWidgetHostViewGuest) to create gestures based on the acked touch events. This CL does away with the extra GestureRecognizer, and routes the touch event acks back to the main GestureRecognizer instead, thus simplifying the event handling logic, and providing more consistent behaviour when touch handlers are present. BUG=445319 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== Route touch-events for WebViewGuest directly to guest renderer. This CL is the beginning of a refactor to route input events directly to a WebViewGuest's renderer instead of taking a round-trip through BrowserPlugin first. Touch events were chosen as the first event type to re-route, since guests with touch-handlers installed currently bypass our attempts to force pinch-zoom to be handled in the embedder. Also, they require a second gesture recognizer (owned in RenderWidgetHostViewGuest) to create gestures based on the acked touch events. This CL does away with the extra GestureRecognizer, and routes the touch event acks back to the main GestureRecognizer instead, thus simplifying the event handling logic, and providing more consistent behaviour when touch handlers are present. BUG=445319 ========== to ========== Route touch-events for WebViewGuest directly to guest renderer. This CL is the beginning of a refactor to route input events directly to a WebViewGuest's renderer instead of taking a round-trip through BrowserPlugin first. Touch events were chosen as the first event type to re-route, since guests with touch-handlers installed currently bypass our attempts to force pinch-zoom to be handled in the embedder. Also, they require a second gesture recognizer (owned in RenderWidgetHostViewGuest) to create gestures based on the acked touch events. This CL does away with the extra GestureRecognizer, and routes the touch event acks back to the main GestureRecognizer instead, thus simplifying the event handling logic, and providing more consistent behaviour when touch handlers are present. BUG=445319 Committed: https://crrev.com/e31234f7e0950fa7f062a42ed530b76ba3bd1bac Cr-Commit-Position: refs/heads/master@{#365034} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/e31234f7e0950fa7f062a42ed530b76ba3bd1bac Cr-Commit-Position: refs/heads/master@{#365034} |
