Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15002007:
Delegate root layer scroll offset to android_webview. (Closed)
|
Created:
7 years, 7 months ago by mkosiba (inactive) Modified:
7 years, 6 months ago CC:
chromium-reviews, jam, apatrick_chromium, joi+watch-content_chromium.org, darin-cc_chromium.org, cc-bugs_chromium.org, android-webview-reviews_chromium.org, jdduke (slow) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDelegate root layer scroll offset to android_webview.
This connects the LayerScrollOffsetDeleate to the android_webview
layer via the SynchronousCompositor.
BUG=b/6029133
TESTS=None
R=joth@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=205591
Patch Set 1 #
Total comments: 9
Patch Set 2 : rebase #
Total comments: 12
Patch Set 3 : register the handler in the InputHandlerManager #Patch Set 4 : remove stale file #Patch Set 5 : rebase on top of crrev.com/15875009 #
Total comments: 7
Patch Set 6 : #
Total comments: 1
Patch Set 7 : rebase #Patch Set 8 : rebase #Patch Set 9 : remove spurious edits #
Total comments: 8
Patch Set 10 : rebase on top of Jared's CL #
Total comments: 6
Patch Set 11 : joth@'s feedback #Patch Set 12 : rebase #
Total comments: 2
Patch Set 13 : remove use of WeakPtr #Patch Set 14 : rebase #Patch Set 15 : fix unittest compile #Messages
Total messages: 39 (0 generated)
PTAL
I only looked at the structure of the changes, which generally LG 2 me. I didn't think much on the correctness of plumbing the scroll in this way... will defer to jamesr on that. (I'm curious if there's a way we can avoid "wrapping" input handler client... but no idea offhand if there is a nicer arrangement of those pieces) Thx https://chromiumcodereview.appspot.com/15002007/diff/1/content/renderer/andro... File content/renderer/android/synchronous_compositor_impl.cc (right): https://chromiumcodereview.appspot.com/15002007/diff/1/content/renderer/andro... content/renderer/android/synchronous_compositor_impl.cc:34: .PassAs<cc::InputHandlerClient>(); awkward to read. Maybe easier to just allocate a scoped_ptr<cc::InputHandlerClient> on stack and initialize with the new SyncBlahBlahWrapper(..)) https://chromiumcodereview.appspot.com/15002007/diff/1/content/renderer/andro... File content/renderer/android/synchronous_compositor_impl.h (right): https://chromiumcodereview.appspot.com/15002007/diff/1/content/renderer/andro... content/renderer/android/synchronous_compositor_impl.h:22: // Specialization of the output surface that adapts it to implement the comment doesn't apply here -- needs rewriting include a bit about ownership model of this class. (cc owns the OutputSurface, does it also own this class? etc) https://chromiumcodereview.appspot.com/15002007/diff/1/content/renderer/andro... File content/renderer/android/synchronous_compositor_input_handler_client_wrapper.h (right): https://chromiumcodereview.appspot.com/15002007/diff/1/content/renderer/andro... content/renderer/android/synchronous_compositor_input_handler_client_wrapper.h:20: commentette? https://chromiumcodereview.appspot.com/15002007/diff/1/content/renderer/andro... File content/renderer/android/synchronous_compositor_output_surface.h (right): https://chromiumcodereview.appspot.com/15002007/diff/1/content/renderer/andro... content/renderer/android/synchronous_compositor_output_surface.h:31: class Delegate { the emerging preference is not to do inner classes - I don't have strong opinion on that though https://chromiumcodereview.appspot.com/15002007/diff/1/content/renderer/andro... content/renderer/android/synchronous_compositor_output_surface.h:35: virtual void DidDestroyCompositor() = 0; the extra layer of delegate delegation is unfortunate extra complexity, but I don't have any immediate better idea. https://chromiumcodereview.appspot.com/15002007/diff/1/content/renderer/rende... File content/renderer/render_widget.cc (right): https://chromiumcodereview.appspot.com/15002007/diff/1/content/renderer/rende... content/renderer/render_widget.cc:647: return NULL; awkward - this is unreachable as this function is never called on non-ANDROID platform. Maybe remove all the #if ANDROID from call sites and just have this one in here? https://chromiumcodereview.appspot.com/15002007/diff/1/content/renderer/rende... File content/renderer/render_widget.h (right): https://chromiumcodereview.appspot.com/15002007/diff/1/content/renderer/rende... content/renderer/render_widget.h:515: scoped_ptr<SynchronousCompositorImpl> synchronous_compositor_; could be #if ANDROID .. ?
@James - I rebased this CL on top of your https://codereview.chromium.org/13844021/. I'm not entirely sure how my code is supposed to fit in after that change though.. What would be the best way for the SynchronousCompositorImpl to get a hold of the InputHandler? Should we write a webview-specific InputHandlerWrapper and replace InputHandlerManager with a simpler lookup-table based version or am I looking at this the wrong way?
On 2013/05/21 17:45:49, mkosiba wrote: > @James - I rebased this CL on top of your > https://codereview.chromium.org/13844021/. I'm not entirely sure how my code is > supposed to fit in after that change though.. > > What would be the best way for the SynchronousCompositorImpl to get a hold of > the InputHandler? Should we write a webview-specific InputHandlerWrapper and > replace InputHandlerManager with a simpler lookup-table based version or am I > looking at this the wrong way? Regarding the second question, this work should largely replace/augment earlier work on the browser-side SyncInputEventFilter. content::InputEventFilter and content::InputHandlerWrapper have their analogs in sync_input_event_filter.{h,cc}. I was going to adapt them to the latest changes, in which case they would still need access in some way to an InputHandlerProxy... One alternative is adding a method to the SynchronousCompositor interface, e.g., InputEventAckState HandleInputEvent(const WebKit::WebInputEvent&) This is accessible by the browser, and would allow us to delegate the filtering to an InputHandlerProxy hooked up to the InputHandler provided via SynchronousCompositor::didCreateInputHandler?
On 21 May 2013 15:56, <jdduke@chromium.org> wrote: > On 2013/05/21 17:45:49, mkosiba wrote: > >> @James - I rebased this CL on top of your >> https://codereview.chromium.**org/13844021/<https://codereview.chromium.org/1.... >> I'm not entirely sure how my code >> > is > >> supposed to fit in after that change though.. >> > > What would be the best way for the SynchronousCompositorImpl to get a >> hold of >> the InputHandler? Should we write a webview-specific InputHandlerWrapper >> and >> replace InputHandlerManager with a simpler lookup-table based version or >> am I >> looking at this the wrong way? >> > > Regarding the second question, this work should largely replace/augment > earlier > work on the browser-side SyncInputEventFilter. > > content::InputEventFilter and content::InputHandlerWrapper have their > analogs in > sync_input_event_filter.{h,cc}**. I was going to adapt them to the latest > changes, in which case they would still need access in some way to an > InputHandlerProxy... > > One alternative is adding a method to the SynchronousCompositor interface, > e.g., > > InputEventAckState HandleInputEvent(const WebKit::WebInputEvent&) > > This is accessible by the browser, and would allow us to delegate the > filtering > to an InputHandlerProxy hooked up to the InputHandler provided via > SynchronousCompositor::**didCreateInputHandler? > > SynchronousCompositor::HandleInputEvent() makes sense to me for the aw/browser -> content/renderer hop, but I'm not sure how the SynchronousCompositor::**didCreateInputHandler() step would work. Who would call that, when, and on what thread? https://codereview.chromium.**org/15002007/<https://codereview.chromium.org/1... >
On 2013/05/22 00:04:46, joth wrote: > On 21 May 2013 15:56, <mailto:jdduke@chromium.org> wrote: > > > On 2013/05/21 17:45:49, mkosiba wrote: > > > >> @James - I rebased this CL on top of your > >> > https://codereview.chromium.**org/13844021/%3Chttps://codereview.chromium.org...>. > >> I'm not entirely sure how my code > >> > > is > > > >> supposed to fit in after that change though.. > >> > > > > What would be the best way for the SynchronousCompositorImpl to get a > >> hold of > >> the InputHandler? Should we write a webview-specific InputHandlerWrapper > >> and > >> replace InputHandlerManager with a simpler lookup-table based version or > >> am I > >> looking at this the wrong way? > >> > > > > Regarding the second question, this work should largely replace/augment > > earlier > > work on the browser-side SyncInputEventFilter. > > > > content::InputEventFilter and content::InputHandlerWrapper have their > > analogs in > > sync_input_event_filter.{h,cc}**. I was going to adapt them to the latest > > changes, in which case they would still need access in some way to an > > InputHandlerProxy... > > > > One alternative is adding a method to the SynchronousCompositor interface, > > e.g., > > > > InputEventAckState HandleInputEvent(const WebKit::WebInputEvent&) > > > > This is accessible by the browser, and would allow us to delegate the > > filtering > > to an InputHandlerProxy hooked up to the InputHandler provided via > > SynchronousCompositor::**didCreateInputHandler? > > > > > SynchronousCompositor::HandleInputEvent() makes sense to me for the > aw/browser -> content/renderer hop, SGTM2 > but I'm not sure how the > SynchronousCompositor::**didCreateInputHandler() step would work. Who would > call that, when, and on what thread? What hookup requires this call? Once the SynchronousCompositor is hooked up, you can simply provide input events as you need them.
https://codereview.chromium.org/15002007/diff/9001/cc/input/layer_scroll_offs... File cc/input/layer_scroll_offset_delegate.h (right): https://codereview.chromium.org/15002007/diff/9001/cc/input/layer_scroll_offs... cc/input/layer_scroll_offset_delegate.h:11: class Vector2dF; the functions here accept/return a gfx::Vector2dF by value, so i don't think having a forward declaration of gfx::Vector2dF can help anyone. adding an #include of ui/gfx/vector2d_f.h could be helpful, of course https://codereview.chromium.org/15002007/diff/9001/content/public/renderer/an... File content/public/renderer/android/synchronous_compositor.h (right): https://codereview.chromium.org/15002007/diff/9001/content/public/renderer/an... content/public/renderer/android/synchronous_compositor.h:6: #define CONTENT_PUBLIC_RENDERER_ANDROID_SYNCHRONOUS_COMPOSTIOR_ it looks like you're fixing it to a typo - this should be ..._COMPOSITOR_H_, not _COMPOSTIOR_ https://codereview.chromium.org/15002007/diff/9001/content/public/renderer/an... File content/public/renderer/android/synchronous_compositor_client.h (right): https://codereview.chromium.org/15002007/diff/9001/content/public/renderer/an... content/public/renderer/android/synchronous_compositor_client.h:6: #define CONTENT_PUBLIC_RENDERER_ANDROID_SYNCRHONOUS_COMPOSITOR_CLIENT_H_ same typo here - s/COMPOSTIOR/COMPOSITOR/ https://codereview.chromium.org/15002007/diff/9001/content/public/renderer/an... content/public/renderer/android/synchronous_compositor_client.h:11: class Vector2dF; forward decl can't help when the type is passed by value. https://codereview.chromium.org/15002007/diff/9001/content/renderer/android/s... File content/renderer/android/synchronous_compositor_impl.cc (right): https://codereview.chromium.org/15002007/diff/9001/content/renderer/android/s... content/renderer/android/synchronous_compositor_impl.cc:34: GetContentClient()->renderer()->OverrideCompositorMessageLoop()->PostTask( this looks needlessly indirect. you're in the content implementation here, you can get to the compositor thread, and you know this view's routing_id. content::InputHandlerManager maintains a map of routing_ids to InputHandler instances. This class should coordinate with the InputHandlerManager to do whatever work needs doing https://codereview.chromium.org/15002007/diff/9001/content/renderer/android/s... File content/renderer/android/synchronous_compositor_impl.h (right): https://codereview.chromium.org/15002007/diff/9001/content/renderer/android/s... content/renderer/android/synchronous_compositor_impl.h:33: void didCreateInputHandler(base::WeakPtr<cc::InputHandler> input_handler); why is this function named in WebKit style?
I wonder if we can get at this by slightly abstracting InputHandlerManager. As it stands, WebView does not want to use the existing InputHandlerManager or InputEventFilter, as the input filtering is done browser-side. Currently, the InputHandler is hooked up to the InputHandlerManager in RenderViewImpl::didActivateCompositor, and the InputHandlerManager is created in RenderThreadImpl::EnsureWebKitInitialized. What if we delegate InputHandlerManager creation to the ContentRendererClient? AwContentRendererClient would create an InputHandlerManager implementation that hooks up the InputHandler (from didActivateCompositor) to its SynchronousCompositor, while the default ContentRendererClient will create the current version of InputHandlerManager. This would replace the rather ugly ContentRendererClient::ShouldCreateCompositorInputHandler call, which is simply wrong at this point. So, InputHandlerManager would be an interface with AddInputHandler/RemoveInputHandler stubs, and InputHandlerManager would be renamed... InputHandlerManagerImpl?
On 2013/05/22 15:18:14, jdduke wrote: > I wonder if we can get at this by slightly abstracting InputHandlerManager. As > it stands, WebView does not want to use the existing InputHandlerManager or > InputEventFilter, as the input filtering is done browser-side. > > Currently, the InputHandler is hooked up to the InputHandlerManager in > RenderViewImpl::didActivateCompositor, and the InputHandlerManager is created in > RenderThreadImpl::EnsureWebKitInitialized. What if we delegate > InputHandlerManager creation to the ContentRendererClient? > AwContentRendererClient would create an InputHandlerManager implementation that > hooks up the InputHandler (from didActivateCompositor) to its > SynchronousCompositor, while the default ContentRendererClient will create the > current version of InputHandlerManager. This would replace the rather ugly > ContentRendererClient::ShouldCreateCompositorInputHandler call, which is simply > wrong at this point. > > So, InputHandlerManager would be an interface with > AddInputHandler/RemoveInputHandler stubs, and InputHandlerManager would be > renamed... InputHandlerManagerImpl? Hmm, I guess we're not in cc-Land so InputHandlerManagerImpl is probably a bad name, and it's not really important at this point... @mkosiba, I was assigned the bug of fixing up input with the latest changes anyhow, so I'd be happy to tackle that and hook things up after you feel good about this patch?
On 2013/05/22 15:18:14, jdduke wrote: > I wonder if we can get at this by slightly abstracting InputHandlerManager. As > it stands, WebView does not want to use the existing InputHandlerManager or > InputEventFilter, as the input filtering is done browser-side. > > Currently, the InputHandler is hooked up to the InputHandlerManager in > RenderViewImpl::didActivateCompositor, and the InputHandlerManager is created in > RenderThreadImpl::EnsureWebKitInitialized. What if we delegate > InputHandlerManager creation to the ContentRendererClient? > AwContentRendererClient would create an InputHandlerManager implementation that > hooks up the InputHandler (from didActivateCompositor) to its > SynchronousCompositor, while the default ContentRendererClient will create the > current version of InputHandlerManager. This would replace the rather ugly > ContentRendererClient::ShouldCreateCompositorInputHandler call, which is simply > wrong at this point. > > So, InputHandlerManager would be an interface with > AddInputHandler/RemoveInputHandler stubs, and InputHandlerManager would be > renamed... InputHandlerManagerImpl? Does it even make sense for the android_webview to use these? We could send the events directly from the AwContents (I mean not via IPC) to the cc::InputHandler. Maybe it would make more sense for the SynchronousCompositor to have a HandleInputEvent method?
https://codereview.chromium.org/15002007/diff/9001/cc/input/layer_scroll_offs... File cc/input/layer_scroll_offset_delegate.h (right): https://codereview.chromium.org/15002007/diff/9001/cc/input/layer_scroll_offs... cc/input/layer_scroll_offset_delegate.h:11: class Vector2dF; On 2013/05/22 01:28:13, jamesr wrote: > the functions here accept/return a gfx::Vector2dF by value, so i don't think > having a forward declaration of gfx::Vector2dF can help anyone. adding an > #include of ui/gfx/vector2d_f.h could be helpful, of course yikes! I added the fwd-declare out of habit.. https://codereview.chromium.org/15002007/diff/9001/content/public/renderer/an... File content/public/renderer/android/synchronous_compositor.h (right): https://codereview.chromium.org/15002007/diff/9001/content/public/renderer/an... content/public/renderer/android/synchronous_compositor.h:6: #define CONTENT_PUBLIC_RENDERER_ANDROID_SYNCHRONOUS_COMPOSTIOR_ On 2013/05/22 01:28:13, jamesr wrote: > it looks like you're fixing it to a typo - this should be ..._COMPOSITOR_H_, not > _COMPOSTIOR_ Done. https://codereview.chromium.org/15002007/diff/9001/content/public/renderer/an... File content/public/renderer/android/synchronous_compositor_client.h (right): https://codereview.chromium.org/15002007/diff/9001/content/public/renderer/an... content/public/renderer/android/synchronous_compositor_client.h:6: #define CONTENT_PUBLIC_RENDERER_ANDROID_SYNCRHONOUS_COMPOSITOR_CLIENT_H_ On 2013/05/22 01:28:13, jamesr wrote: > same typo here - s/COMPOSTIOR/COMPOSITOR/ haha! no, the typo in this file is in syncrhonous :P https://codereview.chromium.org/15002007/diff/9001/content/public/renderer/an... content/public/renderer/android/synchronous_compositor_client.h:11: class Vector2dF; On 2013/05/22 01:28:13, jamesr wrote: > forward decl can't help when the type is passed by value. Done. https://codereview.chromium.org/15002007/diff/9001/content/renderer/android/s... File content/renderer/android/synchronous_compositor_impl.cc (right): https://codereview.chromium.org/15002007/diff/9001/content/renderer/android/s... content/renderer/android/synchronous_compositor_impl.cc:34: GetContentClient()->renderer()->OverrideCompositorMessageLoop()->PostTask( On 2013/05/22 01:28:13, jamesr wrote: > this looks needlessly indirect. you're in the content implementation here, you > can get to the compositor thread, and you know this view's routing_id. > content::InputHandlerManager maintains a map of routing_ids to InputHandler > instances. This class should coordinate with the InputHandlerManager to do > whatever work needs doing Done. https://codereview.chromium.org/15002007/diff/9001/content/renderer/android/s... File content/renderer/android/synchronous_compositor_impl.h (right): https://codereview.chromium.org/15002007/diff/9001/content/renderer/android/s... content/renderer/android/synchronous_compositor_impl.h:33: void didCreateInputHandler(base::WeakPtr<cc::InputHandler> input_handler); On 2013/05/22 01:28:13, jamesr wrote: > why is this function named in WebKit style? no idea. probably because most of the didDoSomething functions I've seen in the last couple of hours where in WebKit. fixing.
On 22 May 2013 08:33, <mkosiba@chromium.org> wrote: > On 2013/05/22 15:18:14, jdduke wrote: > >> I wonder if we can get at this by slightly abstracting >> InputHandlerManager. >> > As > >> it stands, WebView does not want to use the existing InputHandlerManager >> or >> InputEventFilter, as the input filtering is done browser-side. >> > > Currently, the InputHandler is hooked up to the InputHandlerManager in >> RenderViewImpl::**didActivateCompositor, and the InputHandlerManager is >> created >> > in > >> RenderThreadImpl::**EnsureWebKitInitialized. What if we delegate >> InputHandlerManager creation to the ContentRendererClient? >> AwContentRendererClient would create an InputHandlerManager implementation >> > that > >> hooks up the InputHandler (from didActivateCompositor) to its >> SynchronousCompositor, while the default ContentRendererClient will >> create the >> current version of InputHandlerManager. This would replace the rather >> ugly >> ContentRendererClient::**ShouldCreateCompositorInputHan**dler call, >> which is >> > simply > >> wrong at this point. >> > > So, InputHandlerManager would be an interface with >> AddInputHandler/**RemoveInputHandler stubs, and InputHandlerManager >> would be >> renamed... InputHandlerManagerImpl? >> > > Does it even make sense for the android_webview to use these? We could > send the > events directly from the AwContents (I mean not via IPC) to the > cc::InputHandler. Maybe it would make more sense for the > SynchronousCompositor > to have a HandleInputEvent method? > > I was looking at this over in https://codereview.chromium.org/15484013/ at it gets caught in speed bumps along the way: AwContents.java currently forwards the _java_ events straight to ContentViewCore and it's in the RenderWidgetHostViewAndroid that we first see it in WebKit::WebInputEvent format and where the current 'sync handle event hook' is installed. The snag there is we then don't have the browser -> renderer connection so we either need to hook it back up to aw/ layer to push down to sync compositor, or we need to introduce the DEPS hole into content too (which is not out the question -- content/browser/renderer_host already knows about content/renderer specifically for single-process.) > https://codereview.chromium.**org/15002007/<https://codereview.chromium.org/1... >
@joth, @jdduke - what are your thoughts on this https://codereview.chromium.org/15002007/diff/30001/content/renderer/android/... File content/renderer/android/synchronous_compositor_impl.cc (right): https://codereview.chromium.org/15002007/diff/30001/content/renderer/android/... content/renderer/android/synchronous_compositor_impl.cc:77: compositor_client_->SetTotalRootLayerScrollOffset(new_value); actually it looks like this ends up causing a use-after-free problem if the WebContents is being deleted (in which case the android_webview layer un-sets the compositor_client_ *before* the SynchronousCompositorImpl gets destroyed). Ideally we'd tie the existence of a RootLayerScrollOffsetDelegate with the existence of the compositor_client_ (set the RLSCOD_ to NULL if the compositor_client_ is NULL) but for that I'd need to change the code Jared is currently adding as part of https://codereview.chromium.org/15920002/ to use the SynchronousCompositorImpl. Thoughts?
https://codereview.chromium.org/15002007/diff/1/content/public/renderer/andro... File content/public/renderer/android/synchronous_compositor.h (right): https://codereview.chromium.org/15002007/diff/1/content/public/renderer/andro... content/public/renderer/android/synchronous_compositor.h:6: #define CONTENT_PUBLIC_RENDERER_ANDROID_SYNCHRONOUS_COMPOSTIOR_ you fixed the wrong line here :) (PRESUBMIT used to catch this?) https://codereview.chromium.org/15002007/diff/30001/content/renderer/android/... File content/renderer/android/synchronous_compositor_impl.cc (right): https://codereview.chromium.org/15002007/diff/30001/content/renderer/android/... content/renderer/android/synchronous_compositor_impl.cc:77: compositor_client_->SetTotalRootLayerScrollOffset(new_value); On 2013/05/28 17:29:50, mkosiba wrote: > actually it looks like this ends up causing a use-after-free problem if the > WebContents is being deleted (in which case the android_webview layer un-sets > the compositor_client_ *before* the SynchronousCompositorImpl gets destroyed). > > Ideally we'd tie the existence of a RootLayerScrollOffsetDelegate with the > existence of the compositor_client_ (set the RLSCOD_ to NULL if the > compositor_client_ is NULL) but for that I'd need to change the code Jared is > currently adding as part of https://codereview.chromium.org/15920002/ to use the > SynchronousCompositorImpl. > Thoughts? Hmm.. I think the bug is in your prior refactor patch. The current ToT code always does a null-check on compositor_client_ before calling through it. Can we just use that same policy in the new synchronous_compositor_impl.cc arrangement? https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/a...
https://codereview.chromium.org/15002007/diff/30001/content/renderer/android/... File content/renderer/android/synchronous_compositor_impl.h (right): https://codereview.chromium.org/15002007/diff/30001/content/renderer/android/... content/renderer/android/synchronous_compositor_impl.h:50: // LayerScrollOffsetDelegate I don't see LayerScrollOffsetDelegate in the class declaration up on line 26? https://codereview.chromium.org/15002007/diff/30001/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/15002007/diff/30001/content/renderer/render_v... content/renderer/render_view_impl.cc:2631: routing_id_, GetSynchronousCompositor()); do you mean GetSynchronousCompositor() here, or something like GetSynchronousCompositor()->GetInputHandlerScrollDelegate() ?
https://codereview.chromium.org/15002007/diff/1/content/public/renderer/andro... File content/public/renderer/android/synchronous_compositor.h (right): https://codereview.chromium.org/15002007/diff/1/content/public/renderer/andro... content/public/renderer/android/synchronous_compositor.h:6: #define CONTENT_PUBLIC_RENDERER_ANDROID_SYNCHRONOUS_COMPOSTIOR_ On 2013/05/28 17:36:24, joth wrote: > you fixed the wrong line here :) > > (PRESUBMIT used to catch this?) Done. https://codereview.chromium.org/15002007/diff/30001/content/renderer/android/... File content/renderer/android/synchronous_compositor_impl.cc (right): https://codereview.chromium.org/15002007/diff/30001/content/renderer/android/... content/renderer/android/synchronous_compositor_impl.cc:77: compositor_client_->SetTotalRootLayerScrollOffset(new_value); On 2013/05/28 17:36:25, joth wrote: > On 2013/05/28 17:29:50, mkosiba wrote: > > actually it looks like this ends up causing a use-after-free problem if the > > WebContents is being deleted (in which case the android_webview layer un-sets > > the compositor_client_ *before* the SynchronousCompositorImpl gets destroyed). > > > > Ideally we'd tie the existence of a RootLayerScrollOffsetDelegate with the > > existence of the compositor_client_ (set the RLSCOD_ to NULL if the > > compositor_client_ is NULL) but for that I'd need to change the code Jared is > > currently adding as part of https://codereview.chromium.org/15920002/ to use > the > > SynchronousCompositorImpl. > > Thoughts? > > Hmm.. I think the bug is in your prior refactor patch. The current ToT code > always does a null-check on compositor_client_ before calling through it. Can we > just use that same policy in the new synchronous_compositor_impl.cc arrangement? > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/a... ah, I see. Yes, I can do that. Do you know if the compositor_client_ will be null in cases other than setup/teardown? https://codereview.chromium.org/15002007/diff/30001/content/renderer/android/... File content/renderer/android/synchronous_compositor_impl.h (right): https://codereview.chromium.org/15002007/diff/30001/content/renderer/android/... content/renderer/android/synchronous_compositor_impl.h:50: // LayerScrollOffsetDelegate On 2013/05/29 00:46:25, joth wrote: > I don't see LayerScrollOffsetDelegate in the class declaration up on line 26? Done. https://codereview.chromium.org/15002007/diff/30001/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/15002007/diff/30001/content/renderer/render_v... content/renderer/render_view_impl.cc:2631: routing_id_, GetSynchronousCompositor()); On 2013/05/29 00:46:25, joth wrote: > do you mean GetSynchronousCompositor() here, or something like > GetSynchronousCompositor()->GetInputHandlerScrollDelegate() ? This is fine. I just forgot to make SynchronousCompositorImpl inherit from RootLayerScrollDelegate. https://codereview.chromium.org/15002007/diff/39001/content/renderer/android/... File content/renderer/android/synchronous_compositor_impl.cc (right): https://codereview.chromium.org/15002007/diff/39001/content/renderer/android/... content/renderer/android/synchronous_compositor_impl.cc:89: return gfx::Vector2dF(); this is the only downside of checking for compositor_client_ (instead of removing the scroll offset delegate ). If the compositor_client_ can only be NULL during setup/teardown then this is fine, but otherwise this could lead to the page scroll jumping to the top at seemingly arbitrary times.
@joth - the InputHandlerManager hookup that I'm now able to do in the SynchronousCompositorImpl is really convinient, however it requires a significant broadening of DEPS. Is that OK? Should I change those to be per-file rules?
https://codereview.chromium.org/15002007/diff/53001/content/browser/android/i... File content/browser/android/in_process/DEPS (right): https://codereview.chromium.org/15002007/diff/53001/content/browser/android/i... content/browser/android/in_process/DEPS:4: "+content/renderer", jared probably needs this too, so IMHO we should just do this rather than make extra hurdles. For consistency we may as well broaden content/public/renderer above too
+jdduke has picture of how InputHandlerManager will be evolving in future https://codereview.chromium.org/15002007/diff/53001/content/browser/android/i... File content/browser/android/in_process/synchronous_compositor_impl.cc (right): https://codereview.chromium.org/15002007/diff/53001/content/browser/android/i... content/browser/android/in_process/synchronous_compositor_impl.cc:37: class SynchronousCompositorFactoryImpl: public SynchronousCompositorFactory { undo edit https://codereview.chromium.org/15002007/diff/53001/content/browser/android/i... content/browser/android/in_process/synchronous_compositor_impl.cc:81: DCHECK(RenderThread::Get()->GetMessageLoop() == base::MessageLoop::current()); I can't work out why this would be. There's only one call to create this class, SynchronousCompositorImpl::CreateForWebContents below, and it can only happen on UI thread (it takes a WebContents* which would be very hard to ever get hold of on render main thread) https://codereview.chromium.org/15002007/diff/53001/content/renderer/gpu/inpu... File content/renderer/gpu/input_handler_manager.h (right): https://codereview.chromium.org/15002007/diff/53001/content/renderer/gpu/inpu... content/renderer/gpu/input_handler_manager.h:58: void SetRootLayerScrollDelegate( @jdduke - any comments on duplicating this cc::InputHandler method on InputHandlerManager? Alternatives could be: 1 - a getter here: cc::InputHandler* GetHandler(int routing_id) 2 - pushing it up the otherway: a) InputHandlerManagerClient::DidCreate/DestoryInputHandler(int routing_id, cc::InputHandler*) b) cc::LayerScrollOffsetDelegate* InputHandlerManagerClient::GetScrollOffsetDelegate(int routing_id) c) InputHandlerManagerClient::Get/SetTotalRootLayerScrollOffset() #2 has threading and lifetime questions. might need a new LayerScrollOffsetDelegate::DidDestroyInputHandler() call or something. 2(c) ends up duplicating the cc::LayerScrollOffsetDelegate::Xxx methods onto three different interfaces... hmmm even after this patch, we also need to connect the call from SynchronousCompositorImpl down to InputHandler::OnRootLayerDelegatedScrollOffsetChanged() so, with ref to https://codereview.chromium.org/15920002/diff/53001/content/renderer/gpu/inpu... it seems like 3/ the 'Handler' typedef could be expanded to a full interface and host the three methods (HandleEvent, SetRootScrollDelegate, OnRootLayerDelegatedScrollOffsetChanged) we'd be good. OR 4/ can we somehow skip past InputHandlerManager and InputHandlerWrapper and somehow get straight to the appropriate InputHandler instance from SynchronousCompositorImpl? Then we'd be golden for all three method calls.
ok, since Jared's CL is landing I guess we can start reviewing this one now? I rebased it and went with Jared's suggestion of passing the InputHandler weakptr via DidAddInputHandler. FWIW the (draft, but working) Java side part of this CL is here: https://codereview.chromium.org/16255010/ https://codereview.chromium.org/15002007/diff/53001/content/browser/android/i... File content/browser/android/in_process/synchronous_compositor_impl.cc (right): https://codereview.chromium.org/15002007/diff/53001/content/browser/android/i... content/browser/android/in_process/synchronous_compositor_impl.cc:37: class SynchronousCompositorFactoryImpl: public SynchronousCompositorFactory { On 2013/06/01 02:11:35, joth wrote: > undo edit why? was the space in front of the colon intentional? https://codereview.chromium.org/15002007/diff/53001/content/browser/android/i... content/browser/android/in_process/synchronous_compositor_impl.cc:81: DCHECK(RenderThread::Get()->GetMessageLoop() == base::MessageLoop::current()); On 2013/06/01 02:11:35, joth wrote: > I can't work out why this would be. There's only one call to create this class, > SynchronousCompositorImpl::CreateForWebContents below, and it can only happen on > UI thread (it takes a WebContents* which would be very hard to ever get hold of > on render main thread) I think you misunderstood me about which main thread I was asking when I asked you over IM whether this class is created on the main thread. I was thinking of the main renderer thread, you were thinking of the main UI thread :)
the structure of this works fine for me. Cheers https://codereview.chromium.org/15002007/diff/53001/content/browser/android/i... File content/browser/android/in_process/synchronous_compositor_impl.cc (right): https://codereview.chromium.org/15002007/diff/53001/content/browser/android/i... content/browser/android/in_process/synchronous_compositor_impl.cc:37: class SynchronousCompositorFactoryImpl: public SynchronousCompositorFactory { On 2013/06/06 17:55:10, mkosiba wrote: > On 2013/06/01 02:11:35, joth wrote: > > undo edit > > why? was the space in front of the colon intentional? Of course! We always have a space either side of that colon. $ git grep " : public" | wc -l 15034 $ git grep "\w: public" | wc -l 106 https://codereview.chromium.org/15002007/diff/53001/content/browser/android/i... content/browser/android/in_process/synchronous_compositor_impl.cc:81: DCHECK(RenderThread::Get()->GetMessageLoop() == base::MessageLoop::current()); On 2013/06/06 17:55:10, mkosiba wrote: > On 2013/06/01 02:11:35, joth wrote: > > I can't work out why this would be. There's only one call to create this > class, > > SynchronousCompositorImpl::CreateForWebContents below, and it can only happen > on > > UI thread (it takes a WebContents* which would be very hard to ever get hold > of > > on render main thread) > > I think you misunderstood me about which main thread I was asking when I asked > you over IM whether this class is created on the main thread. I was thinking of > the main renderer thread, you were thinking of the main UI thread :) Opps! sorry for confusion. (I've learnt the new meaning of 'main' thread in verbal conversation, but obviously I still get tripped up in IM chatting..) https://codereview.chromium.org/15002007/diff/61001/android_webview/browser/i... File android_webview/browser/in_process_view_renderer.h (right): https://codereview.chromium.org/15002007/diff/61001/android_webview/browser/i... android_webview/browser/in_process_view_renderer.h:97: gfx::Vector2dF root_layer_scroll_offset_; I'm tempted to name this next_frame_scroll_offet_ c.f. ContentViewCore always carries info about the last frame. We'd like this guy to be copied down java->native at the start of the frame, and remain our authoritative source until the frame is complete (when we copy it back to java if it changed). the dance is avoid doing JNI in the middle of the draw functor, which is apparently Not Cool. If you want to mention this is root-layer scroll offset you can do that in a comment, but at this level in the stack it's basically the only scroll we care about so I don't think the differentiation is needed in the name (The TODO is fine in just the .cc, don't think it needs repeating here.) https://codereview.chromium.org/15002007/diff/61001/content/browser/android/i... File content/browser/android/in_process/synchronous_compositor_impl.cc (right): https://codereview.chromium.org/15002007/diff/61001/content/browser/android/i... content/browser/android/in_process/synchronous_compositor_impl.cc:39: class SynchronousCompositorFactoryImpl: public SynchronousCompositorFactory { reminder: revert me https://codereview.chromium.org/15002007/diff/61001/content/browser/android/i... content/browser/android/in_process/synchronous_compositor_impl.cc:195: return gfx::Vector2dF(); use { } (or nix the else caluse)
https://codereview.chromium.org/15002007/diff/61001/android_webview/browser/i... File android_webview/browser/in_process_view_renderer.h (right): https://codereview.chromium.org/15002007/diff/61001/android_webview/browser/i... android_webview/browser/in_process_view_renderer.h:97: gfx::Vector2dF root_layer_scroll_offset_; On 2013/06/06 20:19:32, joth wrote: > I'm tempted to name this next_frame_scroll_offet_ > > c.f. ContentViewCore always carries info about the last frame. We'd like this > guy to be copied down java->native at the start of the frame, and remain our > authoritative source until the frame is complete (when we copy it back to java > if it changed). > the dance is avoid doing JNI in the middle of the draw functor, which is > apparently Not Cool. Calling it next_frame_scroll_offset_ would be a bit misleading (at least to me) because it might imply that once the value is set it doesn't change till the next frame. AS far as not making any JNI in the draw functor - I think we should 1) add DCHECKS in the JNI layers to enforce this and, 2) see if it actually happens. Also, in SW mode, what are you going to do about the guy who thought it's a good idea to change the scroll offset from his onDraw implementation? > If you want to mention this is root-layer scroll offset you can do that in a > comment, but at this level in the stack it's basically the only scroll we care > about so I don't think the differentiation is needed in the name Ok. > (The TODO is fine in just the .cc, don't think it needs repeating here.) Ok. https://codereview.chromium.org/15002007/diff/61001/content/browser/android/i... File content/browser/android/in_process/synchronous_compositor_impl.cc (right): https://codereview.chromium.org/15002007/diff/61001/content/browser/android/i... content/browser/android/in_process/synchronous_compositor_impl.cc:39: class SynchronousCompositorFactoryImpl: public SynchronousCompositorFactory { On 2013/06/06 20:19:32, joth wrote: > reminder: revert me Thanks! Done. https://codereview.chromium.org/15002007/diff/61001/content/browser/android/i... content/browser/android/in_process/synchronous_compositor_impl.cc:195: return gfx::Vector2dF(); On 2013/06/06 20:19:32, joth wrote: > use { } (or nix the else caluse) Done.
On 7 June 2013 09:18, <mkosiba@chromium.org> wrote: > > https://codereview.chromium.**org/15002007/diff/61001/** > android_webview/browser/in_**process_view_renderer.h<https://codereview.chromium.org/15002007/diff/61001/android_webview/browser/in_process_view_renderer.h> > File android_webview/browser/in_**process_view_renderer.h (right): > > https://codereview.chromium.**org/15002007/diff/61001/** > android_webview/browser/in_**process_view_renderer.h#**newcode97<https://codereview.chromium.org/15002007/diff/61001/android_webview/browser/in_process_view_renderer.h#newcode97> > android_webview/browser/in_**process_view_renderer.h:97: gfx::Vector2dF > root_layer_scroll_offset_; > On 2013/06/06 20:19:32, joth wrote: > >> I'm tempted to name this next_frame_scroll_offet_ >> > > c.f. ContentViewCore always carries info about the last frame. We'd >> > like this > >> guy to be copied down java->native at the start of the frame, and >> > remain our > >> authoritative source until the frame is complete (when we copy it back >> > to java > >> if it changed). >> the dance is avoid doing JNI in the middle of the draw functor, which >> > is > >> apparently Not Cool. >> > > Calling it next_frame_scroll_offset_ would be a bit misleading (at least > to me) because it might imply that once the value is set it doesn't > change till the next frame. > > Agree. Naming's hard :) 'current frame' is equally fuzzy. Something like shadow_scroll_offset_ or cached_scroll_offset_ might at least express the real authority is on the java side, and this is our closest copy of it native side. > AS far as not making any JNI in the draw functor - I think we should 1) > add DCHECKS in the JNI layers to enforce this and, 2) see if it actually > happens. Also, in SW mode, what are you going to do about the guy who > thought it's a good idea to change the scroll offset from his onDraw > implementation? > > Agree we need DCHECKs... but we know they will happen for sure if we don't cache the scroll offset in native, as we synchronously call into the cc/ layer to generate the frame from the functor, and that is bound to read the root scroll layer from within that call. About scroll changes in the middle of the tree traversal... I don't think it matters. They can't change scroll *while* the webview onDraw is executing, so long as we copy to native at the start of onDraw then that will include any changes made by already drawn views, but not changes made by views not yet drawn (cus that's impossible) so I think that's as good as we can do anyway. And by doing the native side copy in onDraw and using that in subsequent functor call it ensures the visual result is consistent across both HW and SW mode. (i.e. any change made to scroll after webview's onDraw but before the functor call will not be visible until the subsequent frame, in keeping with SW mode). > > If you want to mention this is root-layer scroll offset you can do >> > that in a > >> comment, but at this level in the stack it's basically the only scroll >> > we care > >> about so I don't think the differentiation is needed in the name >> > > Ok. > > > (The TODO is fine in just the .cc, don't think it needs repeating >> > here.) > > Ok. > > > https://codereview.chromium.**org/15002007/diff/61001/** > content/browser/android/in_**process/synchronous_**compositor_impl.cc<https://codereview.chromium.org/15002007/diff/61001/content/browser/android/in_process/synchronous_compositor_impl.cc> > File content/browser/android/in_**process/synchronous_**compositor_impl.cc > (right): > > https://codereview.chromium.**org/15002007/diff/61001/** > content/browser/android/in_**process/synchronous_** > compositor_impl.cc#newcode39<https://codereview.chromium.org/15002007/diff/61001/content/browser/android/in_process/synchronous_compositor_impl.cc#newcode39> > content/browser/android/in_**process/synchronous_**compositor_impl.cc:39: > class SynchronousCompositorFactoryIm**pl: public > SynchronousCompositorFactory { > On 2013/06/06 20:19:32, joth wrote: > >> reminder: revert me >> > > Thanks! Done. > > > https://codereview.chromium.**org/15002007/diff/61001/** > content/browser/android/in_**process/synchronous_** > compositor_impl.cc#newcode195<https://codereview.chromium.org/15002007/diff/61001/content/browser/android/in_process/synchronous_compositor_impl.cc#newcode195> > content/browser/android/in_**process/synchronous_**compositor_impl.cc:195: > return gfx::Vector2dF(); > On 2013/06/06 20:19:32, joth wrote: > >> use { } (or nix the else caluse) >> > > Done. > > https://codereview.chromium.**org/15002007/<https://codereview.chromium.org/1... >
On 2013/06/07 18:20:57, joth wrote:
> On 7 June 2013 09:18, <mailto:mkosiba@chromium.org> wrote:
> > AS far as not making any JNI in the draw functor - I think we should 1)
> > add DCHECKS in the JNI layers to enforce this and, 2) see if it actually
> > happens. Also, in SW mode, what are you going to do about the guy who
> > thought it's a good idea to change the scroll offset from his onDraw
> > implementation?
> >
> > Agree we need DCHECKs... but we know they will happen for sure if we don't
> cache the scroll offset in native, as we synchronously call into the cc/
> layer to generate the frame from the functor, and that is bound to read the
> root scroll layer from within that call.
reads coming from the CC won't cause JNI (see the Java patch, maybe?), only
writes from CC and updates initiated from Java[1] will cause JNI.
Writes from CC will currently happen on VSync because I haven't moved the fling
processing from CC to Java, but after that's done CC will have no reason to
update scroll offset during vsync/draw (I'm assuming doing a vsync/draw
doesn't 'pump' the CC state machine and cause it to assimilate any pending
commits, we'd have to double-check that one just to be safe).
[1] I'll have to update my patch to double-check that the 'last sent to native'
and 'current' scroll offsets are the same when onDraw is called, but that's
only to handle the case of 3rd party code not calling super.onScrollChanged
from an override.
>
> About scroll changes in the middle of the tree traversal... I don't think
> it matters. They can't change scroll *while* the webview onDraw is
> executing, so long as we copy to native at the start of onDraw then that
exactly, since 3rd party code can't run while our code is running the only
source of JNI would be CC scroll offset *writes*. As long as those don't
happen, we're fine.
If they do happen, they're suspicious, a DCHECK should fire and we should
see why the write occured in the first place. From my understanding
of the CC code the root layer scroll offset should not be updated during
vsync/draw (% fling) so any such cases suggest a bug.
> will include any changes made by already drawn views, but not changes made
> by views not yet drawn (cus that's impossible) so I think that's as good as
> we can do anyway. And by doing the native side copy in onDraw and using
> that in subsequent functor call it ensures the visual result is consistent
> across both HW and SW mode. (i.e. any change made to scroll after webview's
> onDraw but before the functor call will not be visible until the subsequent
> frame, in keeping with SW mode).
so when do we actually get the functor callback?
I always assumed it would pretty much happen immediately after the onDraw call
(immediately in the sense that no 3rd party code had the chance to run between
onDraw and the callback). If that isn't the case then we gotta maintain two
scroll offsets - the current one and the shadow copy for the last frame.
If we have no control over what runs between onDraw and the callback then the
following could happen:
1) onDraw, we stop synchronizing native and Java scroll offsets
2) 3rd party Java code does a scrollTo(10, 10);
3) CC does a commit which contains a JS-initiated scrollTo(50, 50)
-> what do we do with this, drop it? we need to run it through Java to
ensure correctness!
4) functor callback, we use the scroll offset from 1)
5) ok, draw done, but how do we reconcile the changes from 2) and 3)?
Having a shadow copy would make it possible to draw with a stale scroll offset,
however depending on how much can happen between onDraw and the callback it
might be possible for JavaScript to observe the future value of the scroll
offset
(you'd get the inverse of lagging).
Sooo.. big question is - when does the functor callback run?
quick answers - > Sooo.. big question is - when does the functor callback run? after the *entire* tree traversal, but before any other Looper gets a chance to run. So significantly, a webview's child views need to see in their onDraw the offset that webview will (nominally, did) draw it, and any change they make to it (if this is even a reasonable/legal thing to do inside a draw traversal, I'm not convinced it is) should only be seen in the subsequent frame. > (I'm assuming doing a vsync/draw > doesn't 'pump' the CC state machine and cause it to assimilate any pending > commits, we'd have to double-check that one just to be safe). calling OutputSurfaceClient::BeginFrame today can certainly push it through numerous states. I don't know if these will fire scroll changes or not though.
I moved my further discussion on the java synchronization over to the java hookup patch (16255010). That doesn't impact any of this content/cc hookup. PS11 lgtm.
James,
Joth likes this, what say you?
The diff's broken for me - all I see is "old chunk mismatch". Try uploading again?
On 2013/06/10 17:48:23, jamesr wrote: > The diff's broken for me - all I see is "old chunk mismatch". Try uploading > again? Deleted and re-uploaded PS11. Try now?
https://codereview.chromium.org/15002007/diff/97001/content/browser/android/i... File content/browser/android/in_process/synchronous_input_event_filter.cc (right): https://codereview.chromium.org/15002007/diff/97001/content/browser/android/i... content/browser/android/in_process/synchronous_input_event_filter.cc:55: ->SetInputHandler(base::WeakPtr<cc::InputHandler>()); if you have an explicit shutdown path, why are you using base::WeakPtr<>? WeakPtr is useful when there isn't an explicit notification path for shutdown
https://codereview.chromium.org/15002007/diff/97001/content/browser/android/i... File content/browser/android/in_process/synchronous_input_event_filter.cc (right): https://codereview.chromium.org/15002007/diff/97001/content/browser/android/i... content/browser/android/in_process/synchronous_input_event_filter.cc:55: ->SetInputHandler(base::WeakPtr<cc::InputHandler>()); On 2013/06/10 21:19:44, jamesr wrote: > if you have an explicit shutdown path, why are you using base::WeakPtr<>? > WeakPtr is useful when there isn't an explicit notification path for shutdown I assumed that there was some reason behind the fact that InputHandlerManager holds on to the InputHandlers as WeakPtrs, not raw pointers. I looked around a bit more and it seems like the only reason is that the InputHandler gets passed from the impl -> main -> impl thread. Anyways, I'll change this to raw ptrs.
On 2013/06/10 21:38:32, mkosiba wrote: > https://codereview.chromium.org/15002007/diff/97001/content/browser/android/i... > File content/browser/android/in_process/synchronous_input_event_filter.cc > (right): > > https://codereview.chromium.org/15002007/diff/97001/content/browser/android/i... > content/browser/android/in_process/synchronous_input_event_filter.cc:55: > ->SetInputHandler(base::WeakPtr<cc::InputHandler>()); > On 2013/06/10 21:19:44, jamesr wrote: > > if you have an explicit shutdown path, why are you using base::WeakPtr<>? > > WeakPtr is useful when there isn't an explicit notification path for shutdown > > I assumed that there was some reason behind the fact that InputHandlerManager > holds on to the InputHandlers as WeakPtrs, not raw pointers. > I looked around a bit more and it seems like the only reason is that the > InputHandler > gets passed from the impl -> main -> impl thread. Anyways, I'll change this to > raw ptrs. Right, it uses WeakPtr<> because in initialization we need a way to make sure on the impl thread that the pointer is valid. After reaching the AddInput...() on the impl thread, we check if the ptr is valid and if so we know the lifetime of the cc::InputHandler from then on. The android code should only be invoked if we complete the startup sequence sucessfully, so it doesn't need to know anything about the WeakPtr<>-ness.
I removed the use of WeakPtr in DidAddInputHandler. PTAL.
also, would the person who marked this review as private please stand up? If not I'm going to un-private it tomorrow morning London time.
lgtm
+bulach for content/public/browser/android rubber stamp.
lgtm, thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mkosiba@chromium.org/15002007/114001
Message was sent while issue was closed.
Change committed as 205591 |
