Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
457913002:
Android WebView: flush input events during onVSync (Closed)
|
Created:
6 years, 4 months ago by hush (inactive) Modified:
6 years, 4 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, jam, penghuang+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, James Su, miu+watch_chromium.org, jdduke (slow) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAndroid WebView: flush input events during onVSync
BUG=394604
Patch Set 1 #
Total comments: 2
Patch Set 2 : flush but no new frame #Patch Set 3 : register observer only for synthetic gestures #
Total comments: 9
Patch Set 4 : subscriberTypes + postInvalidate during VSync #
Total comments: 18
Patch Set 5 : comments #
Total comments: 2
Patch Set 6 : jdduke@ comments #Patch Set 7 : minor fixups #
Total comments: 12
Patch Set 8 : revert to PS3 #
Total comments: 2
Patch Set 9 : Bo's comments #
Total comments: 2
Patch Set 10 : #
Total comments: 4
Messages
Total messages: 52 (0 generated)
Haven't looked into what happens if content_view_core_->GetWindowAndroid()->RequestVSyncUpdate() in Android WebView yet. But at least chrome.gpuBenchmarking works.
https://codereview.chromium.org/457913002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/457913002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_view_android.cc:261: if (content_view_core_) { btw, WasShown() is not called when you use Aw TestShell, nor ContentShellShell. Not sure when it is supposed to be called.
I would suggest implement everything separately in SynchronousCompositor first to avoid running any code webview doesn't want. Then try to reduce code duplication with render_widget_host_view_android https://codereview.chromium.org/457913002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/457913002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_view_android.cc:261: if (content_view_core_) { On 2014/08/09 03:28:12, hush wrote: > btw, WasShown() is not called when you use Aw TestShell, nor ContentShellShell. > Not sure when it is supposed to be called. Should be called in aw shell from here: https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja... Also this change is not ok. Now it's going to request vsyncs and start sending ipc like ViewMsg_BeginFrame. I don't even know what would happen in that case.
Do you mean making SynchronousCompositor inherit from ui::WindowAndroidObserver and overriding OnVSync? On 2014/08/11 15:57:48, boliu wrote: > I would suggest implement everything separately in SynchronousCompositor first > to avoid running any code webview doesn't want. Then try to reduce code > duplication with render_widget_host_view_android
On 2014/08/11 19:49:56, hush wrote: > Do you mean making SynchronousCompositor inherit from ui::WindowAndroidObserver > and overriding OnVSync? > Yeah start from there, then deduplicate as needed.
On 2014/08/11 19:57:04, boliu wrote: > On 2014/08/11 19:49:56, hush wrote: > > Do you mean making SynchronousCompositor inherit from > ui::WindowAndroidObserver > > and overriding OnVSync? > > > > Yeah start from there, then deduplicate as needed. chatted via IM. For the record, we don't need to do that. WebView and clank path will be separated as long as we early out in "OnVSync" in synchronous compositor mode.
(for the benefit of myself) things to investigate: 1. is the compositor_ in window_android null if using synchronous compositor? 2. are the other methods called in WindowAndroidObserver except OnVSync if using synchronous compositor?
On 2014/08/13 01:51:07, hush wrote: > (for the benefit of myself) things to investigate: > 1. is the compositor_ in window_android null if using synchronous compositor? > 2. are the other methods called in WindowAndroidObserver except OnVSync if using > synchronous compositor? 1. should be null 2. OnAttachCompositor, OnDetachCompositor, OnAnimate, OnCompositingDidCommit will only be called when you have a chrome browser process compositor. So they don't get called in Android Webview. All these are called in content/browser/renderer_host/compositor_impl_android.cc
cc +jdduke,sievers I think next step in this CL is to guarantee no behavior change to either webview or chrome if there are no synthetic events. On 2014/08/16 00:42:08, hush wrote: > On 2014/08/13 01:51:07, hush wrote: > > (for the benefit of myself) things to investigate: > > 1. is the compositor_ in window_android null if using synchronous compositor? > > 2. are the other methods called in WindowAndroidObserver except OnVSync if > using > > synchronous compositor? > > 1. should be null > 2. OnAttachCompositor, OnDetachCompositor, OnAnimate, OnCompositingDidCommit > will only be called when you have a chrome browser process compositor. So they > don't get called in Android Webview. All these are called in > content/browser/renderer_host/compositor_impl_android.cc We should separate out "request vsync" and "get callback from compositor" later. Daniel, does the compositor callbacks even need to go through ui/ code? Can ui just have something that handles vsyncs, and have compositor stuff all stay in content?
On 2014/08/16 00:52:16, boliu wrote: > cc +jdduke,sievers > > I think next step in this CL is to guarantee no behavior change to either > webview or chrome if there are no synthetic events. > > On 2014/08/16 00:42:08, hush wrote: > > On 2014/08/13 01:51:07, hush wrote: > > > (for the benefit of myself) things to investigate: > > > 1. is the compositor_ in window_android null if using synchronous > compositor? > > > 2. are the other methods called in WindowAndroidObserver except OnVSync if > > using > > > synchronous compositor? > > > > 1. should be null > > 2. OnAttachCompositor, OnDetachCompositor, OnAnimate, OnCompositingDidCommit > > will only be called when you have a chrome browser process compositor. So they > > don't get called in Android Webview. All these are called in > > content/browser/renderer_host/compositor_impl_android.cc > > We should separate out "request vsync" and "get callback from compositor" later. > Daniel, does the compositor callbacks even need to go through ui/ code? Can ui > just have something that handles vsyncs, and have compositor stuff all stay in > content? Shouldn't the synchronous compositor still trigger all these callbacks correctly instead?
On 2014/08/18 18:28:27, sievers_OOOuntil8_15 wrote: > On 2014/08/16 00:52:16, boliu wrote: > > cc +jdduke,sievers > > > > I think next step in this CL is to guarantee no behavior change to either > > webview or chrome if there are no synthetic events. > > > > On 2014/08/16 00:42:08, hush wrote: > > > On 2014/08/13 01:51:07, hush wrote: > > > > (for the benefit of myself) things to investigate: > > > > 1. is the compositor_ in window_android null if using synchronous > > compositor? > > > > 2. are the other methods called in WindowAndroidObserver except OnVSync if > > > using > > > > synchronous compositor? > > > > > > 1. should be null > > > 2. OnAttachCompositor, OnDetachCompositor, OnAnimate, OnCompositingDidCommit > > > will only be called when you have a chrome browser process compositor. So > they > > > don't get called in Android Webview. All these are called in > > > content/browser/renderer_host/compositor_impl_android.cc > > > > We should separate out "request vsync" and "get callback from compositor" > later. > > Daniel, does the compositor callbacks even need to go through ui/ code? Can ui > > just have something that handles vsyncs, and have compositor stuff all stay in > > content? > > Shouldn't the synchronous compositor still trigger all these callbacks correctly > instead? Sure, but synchronous compositor is in content too. It was a layering question, why have an interface (WindowAndroidCompositor) in ui/ that's implemented and used in content/ ?
On 2014/08/18 18:34:19, boliu wrote: > On 2014/08/18 18:28:27, sievers_OOOuntil8_15 wrote: > > On 2014/08/16 00:52:16, boliu wrote: > > > cc +jdduke,sievers > > > > > > I think next step in this CL is to guarantee no behavior change to either > > > webview or chrome if there are no synthetic events. > > > > > > On 2014/08/16 00:42:08, hush wrote: > > > > On 2014/08/13 01:51:07, hush wrote: > > > > > (for the benefit of myself) things to investigate: > > > > > 1. is the compositor_ in window_android null if using synchronous > > > compositor? > > > > > 2. are the other methods called in WindowAndroidObserver except OnVSync > if > > > > using > > > > > synchronous compositor? > > > > > > > > 1. should be null > > > > 2. OnAttachCompositor, OnDetachCompositor, OnAnimate, > OnCompositingDidCommit > > > > will only be called when you have a chrome browser process compositor. So > > they > > > > don't get called in Android Webview. All these are called in > > > > content/browser/renderer_host/compositor_impl_android.cc > > > > > > We should separate out "request vsync" and "get callback from compositor" > > later. > > > Daniel, does the compositor callbacks even need to go through ui/ code? Can > ui > > > just have something that handles vsyncs, and have compositor stuff all stay > in > > > content? > > > > Shouldn't the synchronous compositor still trigger all these callbacks > correctly > > instead? > > > Sure, but synchronous compositor is in content too. > > It was a layering question, why have an interface (WindowAndroidCompositor) in > ui/ that's implemented and used in content/ ? Looks like this needs a pretty sizable refactoring to bring the window android observer down to content/ layer.
On 2014/08/18 20:23:03, hush wrote: > On 2014/08/18 18:34:19, boliu wrote: > > On 2014/08/18 18:28:27, sievers_OOOuntil8_15 wrote: > > > On 2014/08/16 00:52:16, boliu wrote: > > > > cc +jdduke,sievers > > > > > > > > I think next step in this CL is to guarantee no behavior change to either > > > > webview or chrome if there are no synthetic events. > > > > > > > > On 2014/08/16 00:42:08, hush wrote: > > > > > On 2014/08/13 01:51:07, hush wrote: > > > > > > (for the benefit of myself) things to investigate: > > > > > > 1. is the compositor_ in window_android null if using synchronous > > > > compositor? > > > > > > 2. are the other methods called in WindowAndroidObserver except > OnVSync > > if > > > > > using > > > > > > synchronous compositor? > > > > > > > > > > 1. should be null > > > > > 2. OnAttachCompositor, OnDetachCompositor, OnAnimate, > > OnCompositingDidCommit > > > > > will only be called when you have a chrome browser process compositor. > So > > > they > > > > > don't get called in Android Webview. All these are called in > > > > > content/browser/renderer_host/compositor_impl_android.cc > > > > > > > > We should separate out "request vsync" and "get callback from compositor" > > > later. > > > > Daniel, does the compositor callbacks even need to go through ui/ code? > Can > > ui > > > > just have something that handles vsyncs, and have compositor stuff all > stay > > in > > > > content? > > > > > > Shouldn't the synchronous compositor still trigger all these callbacks > > correctly > > > instead? > > > > > > Sure, but synchronous compositor is in content too. > > > > It was a layering question, why have an interface (WindowAndroidCompositor) in > > ui/ that's implemented and used in content/ ? > > Looks like this needs a pretty sizable refactoring to bring the window android > observer down > to content/ layer. Sorry that was a bit off topic. For this CL: """ I think next step in this CL is to guarantee no behavior change to either webview or chrome if there are no synthetic events. """
> """ > I think next step in this CL is to guarantee no behavior change to either > webview or chrome if there are no synthetic events. > """ First of all, this CL does not change the Chrome path at all, because I am basically removing the "if (!using_synchronous_compositor)" guards, which is true for Chrome anyway. For Android webview, when there is no synthetic touch events, RenderWidgetHostViewAndroid::OnSetNeedsFlushInput is never called. As a result, even though webview registers a vsync observer, it does not nothing OnVSync because flush_input_requested_ stays false. Why RenderWidgetHostViewAndroid::OnSetNeedsFlushInput is never called when there is no synthetic touch events: RWHVA::OnSetNeedsFlushInput is called by RWHI::SetNeedsFlush. And the only place I find that calls RWHI::SetNeedsFlush is in SyntheticGestureTargetBase and MotionEventBuffer. I don't think Android WebView uses MotionEventBuffer, but I needs confirmation on this.
On 2014/08/19 02:09:27, hush wrote: > > """ > > I think next step in this CL is to guarantee no behavior change to either > > webview or chrome if there are no synthetic events. > > """ > > First of all, this CL does not change the Chrome path at all, because I am > basically removing the "if (!using_synchronous_compositor)" guards, which is > true for Chrome anyway. > > For Android webview, when there is no synthetic touch events, > RenderWidgetHostViewAndroid::OnSetNeedsFlushInput is never called. As a result, > even though webview registers a vsync observer, it does not nothing OnVSync > because > flush_input_requested_ stays false. RWHVA is now an observer of WindowAndroid in webview. It's hard to reason what behavior changes that might introduce, eg stuff like https://codereview.chromium.org/476233002/ > > Why RenderWidgetHostViewAndroid::OnSetNeedsFlushInput is never called when there > is > no synthetic touch events: > RWHVA::OnSetNeedsFlushInput is called by RWHI::SetNeedsFlush. And the only place > I find > that calls RWHI::SetNeedsFlush is in SyntheticGestureTargetBase and > MotionEventBuffer. > > I don't think Android WebView uses MotionEventBuffer, but I needs confirmation > on this.
Patch Set 3: Only add window android observer if CreateSyntheticGestureTarget is called, that is, when RWHVA is receiving synthetic events.
So how about WebView only registering the observer when there is synthetic events? > RWHVA is now an observer of WindowAndroid in webview. It's hard to reason what > behavior changes that might introduce, eg stuff like > https://codereview.chromium.org/476233002/
https://codereview.chromium.org/457913002/diff/60001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/457913002/diff/60001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:815: if (using_synchronous_compositor_) { && !observing_root_window_ https://codereview.chromium.org/457913002/diff/60001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1538: if (using_synchronous_compositor_) Can we also remove this as observer if there are no more synthetic gestures? Is there an easy way to detect that?
https://codereview.chromium.org/457913002/diff/60001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/457913002/diff/60001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:815: if (using_synchronous_compositor_) { On 2014/08/19 22:54:39, boliu wrote: > && !observing_root_window_ Done. https://codereview.chromium.org/457913002/diff/60001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1538: if (using_synchronous_compositor_) I'm not sure you can do that... You can't know if there are any more synthetic gestures, because they are coming from javascript console and could be typed by the user any time when the user connects to chrome DevTools. Also there's no need to remove observer when the synthetic gesture target is destructed because SyntheticGestureController does not change its GestureTarget once it is initialized and SyntheticGestureController only gets destructed when the render process exited (https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...) and webview is gone at that point so there is no need to remove observers. On 2014/08/19 22:54:39, boliu wrote: > Can we also remove this as observer if there are no more synthetic gestures? Is > there an easy way to detect that?
https://codereview.chromium.org/457913002/diff/60001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/457913002/diff/60001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1538: if (using_synchronous_compositor_) On 2014/08/19 22:54:39, boliu wrote: > Can we also remove this as observer if there are no more synthetic gestures? Is > there an easy way to detect that? |DidFlushInput| will be called when synthetic *events* have been fully flushed, however, the synthetic gesture may yet be active, and there's not really a great way of determining this from RWHVAndroid. Keep in mind that synthetic tests are designed to run in synthetic environments, so I'm not sure it's critical that we "clean up" after ourselves here. Rather than putting this early out here, why not have a simple |vsync_subscribers_| bit field that allows for input, transient BeginFrame and permanent BeginFrame subscriber types? That should handle the WebView case naturally, as WebView should never request BeginFrame under any circumstances, no? Perhaps we have a helper method |SubscribeToVsync(SubscriberType)| that requests the vsync and updates the flags as appropriate.
https://codereview.chromium.org/457913002/diff/60001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/457913002/diff/60001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1538: if (using_synchronous_compositor_) On 2014/08/19 23:30:38, jdduke wrote: > On 2014/08/19 22:54:39, boliu wrote: > > Can we also remove this as observer if there are no more synthetic gestures? > Is > > there an easy way to detect that? > > |DidFlushInput| will be called when synthetic *events* have been fully flushed, > however, the synthetic gesture may yet be active, and there's not really a great > way of determining this from RWHVAndroid. Never mind then. Maybe SyntheticGestureController should have a call to clean things up, but no need to do it in this CL. > Keep in mind that synthetic tests are > designed to run in synthetic environments, so I'm not sure it's critical that we > "clean up" after ourselves here. > > Rather than putting this early out here, why not have a simple > |vsync_subscribers_| bit field that allows for input, transient BeginFrame and > permanent BeginFrame subscriber types? That should handle the WebView case > naturally, as WebView should never request BeginFrame under any circumstances, > no? Right, webview should never request begin frame here. > Perhaps we have a helper method |SubscribeToVsync(SubscriberType)| that > requests the vsync and updates the flags as appropriate. This is outside my knowledge now, so I guess just do what jared says. What's transient BeginFrame??
https://codereview.chromium.org/457913002/diff/60001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/457913002/diff/60001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1538: if (using_synchronous_compositor_) On 2014/08/19 23:37:47, boliu wrote: > What's transient BeginFrame?? There are a couple one-off cases where we "kick start" BeginFrame without actually permanently subscribing to it, e.g., on touch start, on |WasShown()| and when we swap out the ContentViewCore. Hmm, why does every touch event request a vsync update? That smells a bit, I would think it's only the initial touchstart or touchmove..
https://codereview.chromium.org/457913002/diff/60001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/457913002/diff/60001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1538: if (using_synchronous_compositor_) So there are 3 VSync Subscriber Types: input: only flushes input OnVSync, never request beginFrame. transient BeginFrame: requests beginFrame once during the first OnVSync. Clear the corresponding bit in |vsync_subscribers_| after that. permanent BeginFrame: always requests beginFrame during each OnVSync. On 2014/08/19 23:41:33, jdduke wrote: > On 2014/08/19 23:37:47, boliu wrote: > > What's transient BeginFrame?? > > There are a couple one-off cases where we "kick start" BeginFrame without > actually permanently subscribing to it, e.g., on touch start, on |WasShown()| > and when we swap out the ContentViewCore. > > Hmm, why does every touch event request a vsync update? That smells a bit, I > would think it's only the initial touchstart or touchmove..
https://codereview.chromium.org/457913002/diff/60001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/457913002/diff/60001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1538: if (using_synchronous_compositor_) On 2014/08/20 00:16:14, hush wrote: > So there are 3 VSync Subscriber Types: > input: only flushes input OnVSync, never request beginFrame. > transient BeginFrame: requests beginFrame once during the first OnVSync. Clear > the corresponding bit in |vsync_subscribers_| after that. > permanent BeginFrame: always requests beginFrame during each OnVSync. Yup, that sounds about right. This way we can also enforce subscription validity with DCHECK (i.e., synchronous_compositor_ should never coincide with BeginFrame subscriptions).
PS4: Chatted with Jared. We will just have 2 VSyncsubscriber types for now: INPUT and BEGIN_FRAME. BEGIN_FRAME subscriber will be controlled by needs_begin_frame_. Also chatted with Bo. WebView should not postInvalidateOnAnimate during VSync, because VSync is hooked up to onAnimate in choreographer. Instead, let AwContents query the current state of WindowAndroid, and decide which function postInvalidate to call.
https://codereview.chromium.org/457913002/diff/80001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/457913002/diff/80001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:2047: if (SUPPORTS_ON_ANIMATION && mWindowAndroid != null && !mWindowAndroid.isInsideVSync()) { Is it possible for mWindowAndroid to be null here? https://codereview.chromium.org/457913002/diff/80001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:2050: mContainerView.postInvalidate(); we want invalidate here, not postInvalidate https://codereview.chromium.org/457913002/diff/80001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/457913002/diff/80001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:207: observing_root_window_(false) { vsync_subscribers_ can replace observing_root_window_ I think? https://codereview.chromium.org/457913002/diff/80001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1524: void RenderWidgetHostViewAndroid::SubscribeToVSync(int type) { Where is Unsubscribe? https://codereview.chromium.org/457913002/diff/80001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1532: DCHECK(using_synchronous_compositor_ && !(vsync_subscribers_ & BEGIN_FRAME)); This DCHECK can't be right. It fails if using_synchronous_compositor is false. I think you want DCHECK(!using_synchronous_compositor || !(vsync_subscribers_ & BEGIN_FRAME)); https://codereview.chromium.org/457913002/diff/80001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/457913002/diff/80001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.h:252: BEGIN_FRAME = 2 ehh, 1 << 0, 1 << 1, 1 << 2.... https://codereview.chromium.org/457913002/diff/80001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.h:391: int vsync_subscribers_; use uint32 or something, to specificy the number of bits probably also name this vsync_subscribers_bitmask_ https://codereview.chromium.org/457913002/diff/80001/ui/android/java/src/org/... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/457913002/diff/80001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:36: private boolean mInsideVSync = false; I think this logic should be moved to VSyncMonitor. But I'm no owner here. https://codereview.chromium.org/457913002/diff/80001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:59: mInsideVSync = false; you want to set it to false in finally, ie same pattern as c++ AutoReset Not an issue here, but in general, what if nativeOnVsync raises an exception that's caught by the caller of onVSync?
https://codereview.chromium.org/457913002/diff/80001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/457913002/diff/80001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:2047: if (SUPPORTS_ON_ANIMATION && mWindowAndroid != null && !mWindowAndroid.isInsideVSync()) { Not possible. postInvalidateOnAnimation must be called from AwContents native code. And at this point the java part must have been initialized. On 2014/08/20 02:54:16, boliu wrote: > Is it possible for mWindowAndroid to be null here? https://codereview.chromium.org/457913002/diff/80001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:2050: mContainerView.postInvalidate(); On 2014/08/20 02:54:16, boliu wrote: > we want invalidate here, not postInvalidate Done. https://codereview.chromium.org/457913002/diff/80001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/457913002/diff/80001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:207: observing_root_window_(false) { yes indeed. https://codereview.chromium.org/457913002/diff/80001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1524: void RenderWidgetHostViewAndroid::SubscribeToVSync(int type) { Will add it... On 2014/08/20 02:54:16, boliu wrote: > Where is Unsubscribe? https://codereview.chromium.org/457913002/diff/80001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.cc:1532: DCHECK(using_synchronous_compositor_ && !(vsync_subscribers_ & BEGIN_FRAME)); yeah.. you are right. Done On 2014/08/20 02:54:16, boliu wrote: > This DCHECK can't be right. It fails if using_synchronous_compositor is false. > > I think you want > > DCHECK(!using_synchronous_compositor || !(vsync_subscribers_ & BEGIN_FRAME)); https://codereview.chromium.org/457913002/diff/80001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/457913002/diff/80001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.h:252: BEGIN_FRAME = 2 On 2014/08/20 02:54:17, boliu wrote: > ehh, 1 << 0, 1 << 1, 1 << 2.... Done. https://codereview.chromium.org/457913002/diff/80001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_android.h:391: int vsync_subscribers_; On 2014/08/20 02:54:16, boliu wrote: > use uint32 or something, to specificy the number of bits > > probably also name this vsync_subscribers_bitmask_ Done. https://codereview.chromium.org/457913002/diff/80001/ui/android/java/src/org/... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/457913002/diff/80001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:36: private boolean mInsideVSync = false; Yes.. I thought about this too... I will change this logic in the next patch and see what the owner has to say. On 2014/08/20 02:54:17, boliu wrote: > I think this logic should be moved to VSyncMonitor. But I'm no owner here. https://codereview.chromium.org/457913002/diff/80001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:59: mInsideVSync = false; good point. Will do. On 2014/08/20 02:54:17, boliu wrote: > you want to set it to false in finally, ie same pattern as c++ AutoReset > > Not an issue here, but in general, what if nativeOnVsync raises an exception > that's caught by the caller of onVSync?
https://codereview.chromium.org/457913002/diff/100001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/457913002/diff/100001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:279: UnsubscribeToVSync(INPUT | BEGIN_FRAME); Hmm, this isn't quite what I had in mind. I was thinking/hoping the subscriber types would replace the various ad-hoc flags that track how a given vsync signal should be dispatched (flush_input_requested_, needs_begin_frame_), and the ad-hoc way in which a vsync signal is requested (via RequestVSyncupdate). Lazily adding |this| as an observer to the window is a nice fall-out from that approach, but not really the main motivation, if that makes sense.
https://codereview.chromium.org/457913002/diff/100001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/457913002/diff/100001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:279: UnsubscribeToVSync(INPUT | BEGIN_FRAME); I see. I will remove |flush_input_requested_| and |needs_begin_frame_|, and just use the bit mask. Also I will move almost every "requestVSyncUpdate" into SubscribeToVSync On 2014/08/20 04:11:53, jdduke wrote: > Hmm, this isn't quite what I had in mind. I was thinking/hoping the subscriber > types would replace the various ad-hoc flags that track how a given vsync signal > should be dispatched (flush_input_requested_, needs_begin_frame_), and the > ad-hoc way in which a vsync signal is requested (via RequestVSyncupdate). Lazily > adding |this| as an observer to the window is a nice fall-out from that > approach, but not really the main motivation, if that makes sense.
PTAL
I think subscribe/unsubscribe is kinda messy because it's missing a piece. You also want to keep whether a vsync has been already requested so not to request another one, and re-request things in vsync if needed. I'm imagining the missing piece is very similar to BrowserViewRenderer::EnsureContinuousInvalidate. The semantic of that is you are free to call it at any time any state that affects it changes, and it will ensure there is only one invalidate in flight etc...dunno what Jared had in mind. https://codereview.chromium.org/457913002/diff/140001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/457913002/diff/140001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:762: new WindowAndroid(mContext.getApplicationContext()); Move this into createAndInitializeContentViewCore. https://codereview.chromium.org/457913002/diff/140001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (left): https://codereview.chromium.org/457913002/diff/140001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1348: if (content_view_core_) { You need to rebase, I added something here to not request vsync in webview. https://codereview.chromium.org/457913002/diff/140001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/457913002/diff/140001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:547: if (enabled == IsSubscribedToType(BEGIN_FRAME) || !content_view_core_) No need to check for !content_view_core_ here. https://codereview.chromium.org/457913002/diff/140001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1303: TRACE_EVENT0("input", "RenderWidgetHostViewAndroid::OnSetNeedsFlushInput"); unrelated side note, this should be an instant event, requesting a vsync is fast https://codereview.chromium.org/457913002/diff/140001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1346: SubscribeToVSync(BEGIN_FRAME); This is not the same as simply calling RequestVSyncUpdate. Now any touch event is going to start producing begin frames forever. https://codereview.chromium.org/457913002/diff/140001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1440: UnsubscribeToVSync(INPUT | BEGIN_FRAME); I think we need an catch all enum value here. https://codereview.chromium.org/457913002/diff/140001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1467: SubscribeToVSync(INPUT | BEGIN_FRAME); Only subscribe to what was subscribed before. https://codereview.chromium.org/457913002/diff/140001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1530: bool RenderWidgetHostViewAndroid::IsSubscribedToType(uint32 type) { const https://codereview.chromium.org/457913002/diff/140001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1564: content_view_core_->GetWindowAndroid()->RequestVSyncUpdate(); This is still called outside of SubscribeToVSync. https://codereview.chromium.org/457913002/diff/140001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/457913002/diff/140001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.h:250: enum VSyncSubscriberType { INPUT = 1 << 0, BEGIN_FRAME = 1 << 1 }; nit: separate lines (unless this is what clang-format did..?) https://codereview.chromium.org/457913002/diff/140001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/VSyncMonitor.java (right): https://codereview.chromium.org/457913002/diff/140001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/VSyncMonitor.java:180: finally { nit: move finally one one above
PTAL https://codereview.chromium.org/457913002/diff/140001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/457913002/diff/140001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:762: new WindowAndroid(mContext.getApplicationContext()); createAndInitializeContentViewCore is a static method. And you will get warnings if you try to access mWindowAndroid in it. btw, why is createAndInitializeContentViewCore a static method? I think we can just make it non static. On 2014/08/20 20:30:00, boliu wrote: > Move this into createAndInitializeContentViewCore.
Daniel, can you look at ui/ part? Thanks!
On 2014/08/20 21:02:10, hush wrote: > Daniel, > can you look at ui/ part? Thanks! lgtm
https://codereview.chromium.org/457913002/diff/160001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/457913002/diff/160001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:815: if (using_synchronous_compositor_) { && !observing_root_window_ I think this should be done in OnSetNeedsFlushInput, where you don't even need the using_synchronous_compositor_ check
https://codereview.chromium.org/457913002/diff/160001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/457913002/diff/160001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:815: if (using_synchronous_compositor_) { I used to think doing this in OnSetNeedsFlushInput will cause webview to register observer unnecessarily. In fact, OnSetNeedsFlushInput is not called for webview unless the input comes from synthetic gestures. So yes. I will move this part to OnSetNeedsFlushInput. Thanks! On 2014/08/20 21:41:53, boliu wrote: > && !observing_root_window_ > > I think this should be done in OnSetNeedsFlushInput, where you don't even need > the using_synchronous_compositor_ check
lgtm assuming you tested it actually works :)
had a nit comment.. https://codereview.chromium.org/457913002/diff/180001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/457913002/diff/180001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:90: private WindowAndroid mWindowAndroid; nit: move this to where mContentViewCore is declared.
https://codereview.chromium.org/457913002/diff/180001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/457913002/diff/180001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:90: private WindowAndroid mWindowAndroid; On 2014/08/21 02:53:47, boliu wrote: > nit: move this to where mContentViewCore is declared. Done.
https://codereview.chromium.org/457913002/diff/180001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/457913002/diff/180001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:90: private WindowAndroid mWindowAndroid; On 2014/08/21 02:53:47, boliu wrote: > nit: move this to where mContentViewCore is declared. Done.
The CQ bit was checked by hush@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hush@chromium.org/457913002/200001
The CQ bit was unchecked by boliu@chromium.org
-cq No owner reviewed render_widget_host_view_android.cc yet
On 2014/08/21 17:45:59, boliu wrote:
> -cq
>
> No owner reviewed render_widget_host_view_android.cc yet
I did, but the problem is:
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
ui/android/java/src/org/chromium/ui/VSyncMonitor.java
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java
https://codereview.chromium.org/457913002/diff/200001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/457913002/diff/200001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1309: content_view_core_->GetWindowAndroid()->AddObserver(this); Actually I'm not sure if this is a good idea, since it potentially breaks the balancing of Add/Remove. I think Jared's patch takes care of this in a better way by remembering we need a 'vsync for flush' and requesting it when we become attached to a CVC or visible.
On 2014/08/21 17:58:41, sievers wrote: > On 2014/08/21 17:45:59, boliu wrote: > > -cq > > > > No owner reviewed render_widget_host_view_android.cc yet > > I did, but the problem is: > > ** Presubmit ERRORS ** > Missing LGTM from an OWNER for these files: > ui/android/java/src/org/chromium/ui/VSyncMonitor.java > ui/android/java/src/org/chromium/ui/base/WindowAndroid.java Meh, I thought you were owner of ui/, but only looked at that part.. render_widget_host_view_android.cc has changed since PS8 btw, might want to look again
https://codereview.chromium.org/457913002/diff/200001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/457913002/diff/200001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1309: content_view_core_->GetWindowAndroid()->AddObserver(this); On 2014/08/21 18:01:22, sievers wrote: > Actually I'm not sure if this is a good idea, since it potentially breaks the > balancing of Add/Remove. > > I think Jared's patch takes care of this in a better way by remembering we need > a 'vsync for flush' and requesting it when we become attached to a CVC or > visible. > Jared's patch came from this. And we should submit this first, and the clean up after, since we might want to get this into webvew I don't think there's a balancing problem here. It should only apply to webview where we *don't* add observer in attach/visible.
https://codereview.chromium.org/457913002/diff/200001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/457913002/diff/200001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1309: content_view_core_->GetWindowAndroid()->AddObserver(this); On 2014/08/21 18:04:56, boliu wrote: > On 2014/08/21 18:01:22, sievers wrote: > > Actually I'm not sure if this is a good idea, since it potentially breaks the > > balancing of Add/Remove. > > > > I think Jared's patch takes care of this in a better way by remembering we > need > > a 'vsync for flush' and requesting it when we become attached to a CVC or > > visible. > > > > Jared's patch came from this. And we should submit this first, and the clean up > after, since we might want to get this into webvew > > I don't think there's a balancing problem here. It should only apply to webview > where we *don't* add observer in attach/visible. What if OnSetNeedsFlushInput() gets called in Chrome when the view is hidden? I don't think this method should change the attachment state wrt to the window. Unfortunately dealing with lifetime issues between CVC, RWHV and WindowAndroid has previously cost us way too much time, so I'd be careful.
https://codereview.chromium.org/457913002/diff/200001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/457913002/diff/200001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1309: content_view_core_->GetWindowAndroid()->AddObserver(this); On 2014/08/21 18:11:00, sievers wrote: > What if OnSetNeedsFlushInput() gets called in Chrome when the view is hidden? I > don't think this method should change the attachment state wrt to the window. Hmm...webview should do what chrome does, which means not flush if it's hidden. But webview is never hidden. So instead of adding flush_input_requested_/using_synchronous_compositor_ checks in show/hide/attach/detach, we can just add " && using_synchronous_compositor_" here to avoid churn... Yeah still kinda sucks > > Unfortunately dealing with lifetime issues between CVC, RWHV and WindowAndroid > has previously cost us way too much time, so I'd be careful. >
On 2014/08/21 18:22:52, boliu wrote: > https://codereview.chromium.org/457913002/diff/200001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_view_android.cc (right): > > https://codereview.chromium.org/457913002/diff/200001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_view_android.cc:1309: > content_view_core_->GetWindowAndroid()->AddObserver(this); > On 2014/08/21 18:11:00, sievers wrote: > > What if OnSetNeedsFlushInput() gets called in Chrome when the view is hidden? > I > > don't think this method should change the attachment state wrt to the window. > > Hmm...webview should do what chrome does, which means not flush if it's hidden. > But webview is never hidden. > > So instead of adding flush_input_requested_/using_synchronous_compositor_ checks > in show/hide/attach/detach, we can just add " && using_synchronous_compositor_" > here to avoid churn... > > Yeah still kinda sucks > > > > > Unfortunately dealing with lifetime issues between CVC, RWHV and WindowAndroid > > has previously cost us way too much time, so I'd be careful. > > Why can't we just remove the sync. compositor checks around the other places where we normally add/remove rwhv as an observer?
abandon this CL in favor of https://codereview.chromium.org/492923002/ The "isInsideVSync" bit is pulled out separately here: https://codereview.chromium.org/470523006/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
