Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14268004:
Add overscroll edge effect animations for Android. (Closed)
|
Created:
7 years, 8 months ago by jdduke (slow) Modified:
7 years, 7 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, jam, penghuang+watch_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, James Su, trchen, David Trainor- moved to gerrit Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd overscroll edge effect animations for Android.
Android provides simple edge effect animations for scrollable widgets. However,
these are incompatible with performance constraints in the current compositing
architecture.
This patch implements a native version of the Android EdgeEffect and OverScroll
behaviors, with hooks for animation and addition to a given layer. The necessary
textures are also added as Android resources.
BUG=135975
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=200846
Patch Set 1 #
Total comments: 3
Patch Set 2 : Use system resources #Patch Set 3 : Cleanup and layer attachment fixes #
Total comments: 2
Patch Set 4 : Use vsync ticks to drive overscroll animation #
Total comments: 66
Patch Set 5 : Code review. Properly disable animation on loss of focus. #
Total comments: 11
Patch Set 6 : Fixes for new approach to vsync signalling #Patch Set 7 : Rebase and use ContentViewCore for animation #Patch Set 8 : Rebase #
Total comments: 6
Patch Set 9 : Rebase + CR. Avoid round-trip animation requests on animate(). #
Total comments: 25
Patch Set 10 : Code review. Simpler layer attachment. #Patch Set 11 : Enable by default #
Total comments: 2
Patch Set 12 : Rebase and switch string fix #
Total comments: 2
Patch Set 13 : Name refactor and make Java method private #Patch Set 14 : Rebase #Patch Set 15 : Use LazyInstance for overscroll resources #Messages
Total messages: 65 (0 generated)
This is a very rough first pass. Animation is driven by a somewhat crude callback timer, which I'd prefer to be driven by some kind of vsync-coupled mechanism. At the browser level, there are plenty of convenient hooks associated with compositor buffer swaps, but not necessarily hardware buffer swaps. Keep in mind, effort was made to avoid any substantive refactoring from the Android EdgeEffect and OverscrollGlow classes; this will (hopefully) minimize headache in maintaining behavioral parity in the future.
Can we get the resources from the Android system instead? I believe trchen did this when he last had a WIP patch for this, he should be able to explain how.
On 2013/04/18 00:11:28, aelias wrote:
> Can we get the resources from the Android system instead? I believe trchen
did
> this when he last had a WIP patch for this, he should be able to explain how.
As in, re-use the system's overscroll_{edge,glow}.png, rather than bundling them
explicitly? We could certainly do this, but I thought we wanted to use
optimized (i.e., smaller, power of 2-aligned) versions, like those included in
this patch.
On 2013/04/18 00:29:38, jdduke wrote:
> On 2013/04/18 00:11:28, aelias wrote:
> > Can we get the resources from the Android system instead? I believe trchen
> did
> > this when he last had a WIP patch for this, he should be able to explain
how.
>
> As in, re-use the system's overscroll_{edge,glow}.png, rather than bundling
them
> explicitly? We could certainly do this, but I thought we wanted to use
> optimized (i.e., smaller, power of 2-aligned) versions, like those included in
> this patch.
Regardless, I'll be replacing the current "push" resource code pipeline such
that resources are pulled directly from native.
I see, I had been thinking we would load the Android ones and shrink them dynamically. I'm a bit undecided on which way is cleaner or more optimized. How large are your new PNGs?
On 2013/04/18 01:27:27, aelias wrote: > I see, I had been thinking we would load the Android ones and shrink them > dynamically. I'm a bit undecided on which way is cleaner or more optimized. > How large are your new PNGs? You guys might already know this, but the system zygote loads these resources. In other words every Android process will already have the overscroll glow pngs loaded into memory (Marcus has more details on this so feel free to reach out to him). The obvious upside of using our own resources is that we won't have to do anything special if future versions of Android change the look of the glow effect.
https://codereview.chromium.org/14268004/diff/1/content/public/android/java/s... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/14268004/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:621: private void initOverscrollResources(Context context) { make this resilient to the resources not existing. When ContentViewCore is used in android_webview mode these resources will not be provided (their id's will be 0).
https://codereview.chromium.org/14268004/diff/1/content/public/android/java/s... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/14268004/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:621: private void initOverscrollResources(Context context) { On 2013/04/18 11:11:28, mkosiba wrote: > make this resilient to the resources not existing. When ContentViewCore is used > in android_webview mode these resources will not be provided (their id's will be > 0). This is all going away =/ Resource acquisition will be behind a flag, and pulled from native code.
On 2013/04/18 15:18:34, jdduke wrote: > https://codereview.chromium.org/14268004/diff/1/content/public/android/java/s... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (right): > > https://codereview.chromium.org/14268004/diff/1/content/public/android/java/s... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:621: > private void initOverscrollResources(Context context) { > On 2013/04/18 11:11:28, mkosiba wrote: > > make this resilient to the resources not existing. When ContentViewCore is > used > > in android_webview mode these resources will not be provided (their id's will > be > > 0). > > This is all going away =/ Resource acquisition will be behind a flag, and pulled > from native code. I'm fine with using the system resources for now. However, we should note that the overscroll resources are not currently public, so it's hard to make any assumptions on future existence.
The latest revision is much cleaner. System resources are used; if absent, the effect is disabled.
lgtm https://codereview.chromium.org/14268004/diff/21001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/14268004/diff/21001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:672: animation_timer_.Start(FROM_HERE, animationInterval, this, I'm not familiar with how animations work, so I apologize if it's a silly question. How is this synchronized with VSync? If the animation update tick isn't in sync with the how CC produces frames won't the animation be janky?
https://codereview.chromium.org/14268004/diff/21001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/14268004/diff/21001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:672: animation_timer_.Start(FROM_HERE, animationInterval, this, On 2013/04/22 16:24:53, mkosiba wrote: > I'm not familiar with how animations work, so I apologize if it's a silly > question. > How is this synchronized with VSync? If the animation update tick isn't in sync > with the how CC produces frames won't the animation be janky? It's not synchronized :) The timer hack was really only intended to get things working quickly. In practice, it works fairly well, but only because the effect lacks any kind of sharp contrast with the background. I've been waiting on https://chromiumcodereview.appspot.com/11959036 for vsync hooks, which just landed. That will be the next addition.
@aelias: Should anybody else take a look at these changes?
Various comments below. The main one requiring followup is that I don't like "CurrentAttachmentDepth". Whatever problem that's solving seems like a something we should resolve by improving CC. We may want to split off the fix for that one into a separate bug/patch and mark this one blocking on it. https://codereview.chromium.org/14268004/diff/1/content/public/common/content... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/14268004/diff/1/content/public/common/content... content/public/common/content_switches.cc:774: const char kEnableOverscrollEdgeEffect[] = No switch should be needed for this. https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... File content/browser/android/edge_effect.cc (right): https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... content/browser/android/edge_effect.cc:13: namespace { Please delete all "static" within this anonymous namespace block, they are redundant https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... content/browser/android/edge_effect.cc:176: if (delta_distance > 0 && pull_distance_ < 0) { No {} here https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... content/browser/android/edge_effect.cc:179: if (pull_distance_ == 0) { No {} here https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... content/browser/android/edge_effect.cc:197: if (state_ != STATE_PULL && state_ != STATE_PULL_DECAY) { No {} here https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... content/browser/android/edge_effect.cc:274: // After absorb, the glow and edge should fade to nothing. Indent https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... content/browser/android/edge_effect.cc:316: if (state_ == STATE_RECEDE && glow_scale_y_ <= 0 && edge_scale_y_ <= 0) { No {} https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... content/browser/android/edge_effect.cc:323: namespace { Delete statics here too https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... content/browser/android/edge_effect.cc:325: const gfx::SizeF& size, int height) { gfx::Size https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... content/browser/android/edge_effect.cc:358: if (IsFinished()) { No {} https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... content/browser/android/edge_effect.cc:362: // If additional levels have been added to the layer tree after the animation It seems that you're working around a bug in CC here. It would be better to fix the underlying issue rather than submit cruft working around it. https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... content/browser/android/edge_effect.cc:381: const int glow_bottom = (int)(std::min( static_cast<int> https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... content/browser/android/edge_effect.cc:394: const int edge_bottom = (int)(edge_height * edge_scale_y_ * dpi_scale); static_cast<int> https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... File content/browser/android/edge_effect.h (right): https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... content/browser/android/edge_effect.h:19: class Size; Delete these two forward declarations https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... content/browser/android/edge_effect.h:51: void Draw(cc::Layer* parent_layer, const gfx::SizeF& size, Edge edge); Nit: just gfx::SizeF https://codereview.chromium.org/14268004/diff/28001/content/browser/android/o... File content/browser/android/overscroll_glow.cc (right): https://codereview.chromium.org/14268004/diff/28001/content/browser/android/o... content/browser/android/overscroll_glow.cc:13: namespace { Delete all "static"s inside here https://codereview.chromium.org/14268004/diff/28001/content/browser/android/o... content/browser/android/overscroll_glow.cc:175: using std::max; Please delete these using declarations or move them to the top of the file. https://codereview.chromium.org/14268004/diff/28001/content/browser/android/o... content/browser/android/overscroll_glow.cc:191: for (unsigned i = 0; i < EDGE_COUNT; ++i) { Should be "size_t i" https://codereview.chromium.org/14268004/diff/28001/content/browser/android/o... content/browser/android/overscroll_glow.cc:207: using std::max; Delete these https://codereview.chromium.org/14268004/diff/28001/content/browser/android/o... content/browser/android/overscroll_glow.cc:221: for (unsigned i = 0; i < EDGE_COUNT; ++i) { Should be "size_t i" https://codereview.chromium.org/14268004/diff/28001/content/browser/android/o... content/browser/android/overscroll_glow.cc:225: EdgeEffect& edge_effect = *edge_effects_[i]; Please use pointers here, non-const references are discouraged https://codereview.chromium.org/14268004/diff/28001/content/browser/android/o... content/browser/android/overscroll_glow.cc:242: default: This "default" is weird. Please delete it entirely or put it in a separate clause that does "NOTREACHED" https://codereview.chromium.org/14268004/diff/28001/content/browser/android/o... File content/browser/android/overscroll_glow.h (right): https://codereview.chromium.org/14268004/diff/28001/content/browser/android/o... content/browser/android/overscroll_glow.h:42: void set_size(const gfx::SizeF& size); Just gfx::SizeF https://codereview.chromium.org/14268004/diff/28001/content/browser/android/o... content/browser/android/overscroll_glow.h:46: enum { EDGE_COUNT = EdgeEffect::EDGE_COUNT }; Seems unnecessary, please use EdgeEffect::EDGE_COUNT directly at use sites https://codereview.chromium.org/14268004/diff/28001/content/browser/android/o... content/browser/android/overscroll_glow.h:52: const gfx::Vector2dF& added_overscroll); Just gfx::Vector2dF all through here https://codereview.chromium.org/14268004/diff/28001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/14268004/diff/28001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:421: if (notifications_enabled != vsync_notifications_enabled_) { I think this vsync_notifications_enabled_ value isn't needed. Let's just always call into content_view_core_ regardless of whether it changed or not, there's no need for throttling. https://codereview.chromium.org/14268004/diff/28001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:459: overscroll_effect_.reset(); Delete this line, we can rely on the destructor. https://codereview.chromium.org/14268004/diff/28001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:594: metadata.page_scale_factor != metadata.min_page_scale_factor && Can this page scale factor comparison be removed? The viewport_size check should be sufficient. This extra check is also incorrect in the case where the viewport tag forces the minimum scale constraint to an unusual value. https://codereview.chromium.org/14268004/diff/28001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:597: overscroll_effect_->set_horizontal_overscroll_enabled( There should be an analogous check for the vertical effect. If I try to scroll a non-vertically scrollable Android system view (such as Settings->Display), no vertical overscroll effect is visible. https://codereview.chromium.org/14268004/diff/28001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/14268004/diff/28001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.h:151: virtual void OnOverscrolled(const gfx::Vector2dF& accumulated_overscroll, Just gfx::Vector2dF https://codereview.chromium.org/14268004/diff/28001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.h:251: bool vsync_notifications_enabled_; Please delete this field (see comment in .cc file) https://codereview.chromium.org/14268004/diff/28001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.h:252: bool client_vsync_notifications_enabled_; Please rename to "renderer_vsync_notifications_enabled". The render process is not usually referred to as a "client". https://codereview.chromium.org/14268004/diff/28001/content/public/common/con... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/14268004/diff/28001/content/public/common/con... content/public/common/content_switches.cc:791: const char kEnableOverscrollEdgeEffect[] = "enable-overscroll-edge-effect"; As part of this patch, please also append this flag in content/browser/android/content_startup_flags.cc https://codereview.chromium.org/14268004/diff/28001/content/public/common/con... File content/public/common/content_switches.h (right): https://codereview.chromium.org/14268004/diff/28001/content/public/common/con... content/public/common/content_switches.h:223: extern const char kEnableOverscrollEdgeEffect[]; Probably should be marked CONTENT_EXPORT as well, it may be of interest to users outside content/
On 2013/04/22 23:46:48, aelias wrote: > Various comments below. The main one requiring followup is that I don't like > "CurrentAttachmentDepth". Whatever problem that's solving seems like a > something we should resolve by improving CC. We may want to split off the fix > for that one into a separate bug/patch and mark this one blocking on it. Agreed. It's definitely a hack that I'd like to remove, and it's not something we'll want to count on (ever, though it works fine in practice for now). In short, there are 2 issues it addresses, that may or may not be related. 1) Layer animations (like those in the tab switcher/swipe) suffer marked slowdown when additional layers are attached to the root layer. Even with just one extra static, opaque layer, it's noticeable. 2) When both the edge effect and tab switcher animations are active, there's an odd flicker at the end of the edge effect animation. The edge effect's ImageLayer is rendered for a single frame after SetNeedsDisplay(false) is called. Now that animation is tied to vsync, I'd like to do some more investigating before we break them out into separate bugs.
> 1) Layer animations (like those in the tab switcher/swipe) suffer marked > slowdown when additional layers are attached to the root layer. Even with just > one extra static, opaque layer, it's noticeable. > 2) When both the edge effect and tab switcher animations are active, there's an > odd flicker at the end of the edge effect animation. The edge effect's > ImageLayer is rendered for a single frame after SetNeedsDisplay(false) is > called. A couple notes... 1) The slowdown is present on the Galaxy Nexus, but the Nexus 4 seems to handle it without any (apparent) problems. 2) Perhaps unrelated to the flicker, there appears to be a broader issue with how the swipe/tab animations handle transparency; ImageLayer opacity values in the edge effect appear to go ignored. I'm not sure if this is related to the transparent tab layer (with the title + X) also being applied at the time.
The main issue I have is with vsync notifications. See reply below. https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... File content/browser/android/edge_effect.cc (right): https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... content/browser/android/edge_effect.cc:13: namespace { On 2013/04/22 23:46:48, aelias wrote: > Please delete all "static" within this anonymous namespace block, they are > redundant Done. https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... content/browser/android/edge_effect.cc:176: if (delta_distance > 0 && pull_distance_ < 0) { On 2013/04/22 23:46:48, aelias wrote: > No {} here Done. https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... content/browser/android/edge_effect.cc:179: if (pull_distance_ == 0) { On 2013/04/22 23:46:48, aelias wrote: > No {} here Done. https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... content/browser/android/edge_effect.cc:197: if (state_ != STATE_PULL && state_ != STATE_PULL_DECAY) { On 2013/04/22 23:46:48, aelias wrote: > No {} here Done. https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... content/browser/android/edge_effect.cc:274: // After absorb, the glow and edge should fade to nothing. On 2013/04/22 23:46:48, aelias wrote: > Indent Done. https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... content/browser/android/edge_effect.cc:316: if (state_ == STATE_RECEDE && glow_scale_y_ <= 0 && edge_scale_y_ <= 0) { On 2013/04/22 23:46:48, aelias wrote: > No {} Done. https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... content/browser/android/edge_effect.cc:323: namespace { On 2013/04/22 23:46:48, aelias wrote: > Delete statics here too Done. https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... content/browser/android/edge_effect.cc:325: const gfx::SizeF& size, int height) { On 2013/04/22 23:46:48, aelias wrote: > gfx::Size Done. https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... content/browser/android/edge_effect.cc:358: if (IsFinished()) { On 2013/04/22 23:46:48, aelias wrote: > No {} Done. https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... content/browser/android/edge_effect.cc:362: // If additional levels have been added to the layer tree after the animation On 2013/04/22 23:46:48, aelias wrote: > It seems that you're working around a bug in CC here. It would be better to fix > the underlying issue rather than submit cruft working around it. Agreed. See my comments in this thread. https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... content/browser/android/edge_effect.cc:381: const int glow_bottom = (int)(std::min( On 2013/04/22 23:46:48, aelias wrote: > static_cast<int> Done. https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... content/browser/android/edge_effect.cc:394: const int edge_bottom = (int)(edge_height * edge_scale_y_ * dpi_scale); On 2013/04/22 23:46:48, aelias wrote: > static_cast<int> Done. https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... File content/browser/android/edge_effect.h (right): https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... content/browser/android/edge_effect.h:19: class Size; On 2013/04/22 23:46:48, aelias wrote: > Delete these two forward declarations Done. https://codereview.chromium.org/14268004/diff/28001/content/browser/android/e... content/browser/android/edge_effect.h:51: void Draw(cc::Layer* parent_layer, const gfx::SizeF& size, Edge edge); On 2013/04/22 23:46:48, aelias wrote: > Nit: just gfx::SizeF Done. https://codereview.chromium.org/14268004/diff/28001/content/browser/android/o... File content/browser/android/overscroll_glow.cc (right): https://codereview.chromium.org/14268004/diff/28001/content/browser/android/o... content/browser/android/overscroll_glow.cc:13: namespace { On 2013/04/22 23:46:48, aelias wrote: > Delete all "static"s inside here Done. https://codereview.chromium.org/14268004/diff/28001/content/browser/android/o... content/browser/android/overscroll_glow.cc:175: using std::max; On 2013/04/22 23:46:48, aelias wrote: > Please delete these using declarations or move them to the top of the file. Done. https://codereview.chromium.org/14268004/diff/28001/content/browser/android/o... content/browser/android/overscroll_glow.cc:191: for (unsigned i = 0; i < EDGE_COUNT; ++i) { On 2013/04/22 23:46:48, aelias wrote: > Should be "size_t i" Done. https://codereview.chromium.org/14268004/diff/28001/content/browser/android/o... content/browser/android/overscroll_glow.cc:207: using std::max; On 2013/04/22 23:46:48, aelias wrote: > Delete these Done. https://codereview.chromium.org/14268004/diff/28001/content/browser/android/o... content/browser/android/overscroll_glow.cc:221: for (unsigned i = 0; i < EDGE_COUNT; ++i) { On 2013/04/22 23:46:48, aelias wrote: > Should be "size_t i" Done. https://codereview.chromium.org/14268004/diff/28001/content/browser/android/o... content/browser/android/overscroll_glow.cc:225: EdgeEffect& edge_effect = *edge_effects_[i]; On 2013/04/22 23:46:48, aelias wrote: > Please use pointers here, non-const references are discouraged Done. https://codereview.chromium.org/14268004/diff/28001/content/browser/android/o... content/browser/android/overscroll_glow.cc:242: default: On 2013/04/22 23:46:48, aelias wrote: > This "default" is weird. Please delete it entirely or put it in a separate > clause that does "NOTREACHED" Done. https://codereview.chromium.org/14268004/diff/28001/content/browser/android/o... File content/browser/android/overscroll_glow.h (right): https://codereview.chromium.org/14268004/diff/28001/content/browser/android/o... content/browser/android/overscroll_glow.h:42: void set_size(const gfx::SizeF& size); On 2013/04/22 23:46:48, aelias wrote: > Just gfx::SizeF Done. https://codereview.chromium.org/14268004/diff/28001/content/browser/android/o... content/browser/android/overscroll_glow.h:46: enum { EDGE_COUNT = EdgeEffect::EDGE_COUNT }; On 2013/04/22 23:46:48, aelias wrote: > Seems unnecessary, please use EdgeEffect::EDGE_COUNT directly at use sites Done. https://codereview.chromium.org/14268004/diff/28001/content/browser/android/o... content/browser/android/overscroll_glow.h:52: const gfx::Vector2dF& added_overscroll); On 2013/04/22 23:46:48, aelias wrote: > Just gfx::Vector2dF all through here Done. https://codereview.chromium.org/14268004/diff/28001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/14268004/diff/28001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:421: if (notifications_enabled != vsync_notifications_enabled_) { On 2013/04/22 23:46:48, aelias wrote: > I think this vsync_notifications_enabled_ value isn't needed. Let's just always > call into content_view_core_ regardless of whether it changed or not, there's no > need for throttling. Are you sure? Every enable/disable call will affect the mVSyncSubscriberCount in ContentViewCore.java. If these calls are not balanced, we'll run into issues. Now, because it uses a subscriber count, all rwhva needs to track is animation_vsync_notifications_enabled_. The renderer and animation can subscribe independently via content_view_core_->SetVSyncNotificationEnabled. However, I'm somewhat hesitant to do that, as we're then coupled to implementation details we shouldn't have to be aware of... https://codereview.chromium.org/14268004/diff/28001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:459: overscroll_effect_.reset(); On 2013/04/22 23:46:48, aelias wrote: > Delete this line, we can rely on the destructor. Right, I had it here as I was unsure of the behavior behind timer callbacks, but we don't even use that anymore. https://codereview.chromium.org/14268004/diff/28001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:594: metadata.page_scale_factor != metadata.min_page_scale_factor && On 2013/04/22 23:46:48, aelias wrote: > Can this page scale factor comparison be removed? The viewport_size check > should be sufficient. This extra check is also incorrect in the case where the > viewport tag forces the minimum scale constraint to an unusual value. I added the page scale check when I noticed certain pages weren't satisfying the viewport dimension constraint. I'll do some more checking. https://codereview.chromium.org/14268004/diff/28001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:597: overscroll_effect_->set_horizontal_overscroll_enabled( On 2013/04/22 23:46:48, aelias wrote: > There should be an analogous check for the vertical effect. If I try to scroll > a non-vertically scrollable Android system view (such as Settings->Display), no > vertical overscroll effect is visible. Right, for whatever reason I thought it still showed up. Thanks. https://codereview.chromium.org/14268004/diff/28001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/14268004/diff/28001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.h:151: virtual void OnOverscrolled(const gfx::Vector2dF& accumulated_overscroll, On 2013/04/22 23:46:48, aelias wrote: > Just gfx::Vector2dF Done. https://codereview.chromium.org/14268004/diff/28001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.h:251: bool vsync_notifications_enabled_; On 2013/04/22 23:46:48, aelias wrote: > Please delete this field (see comment in .cc file) See reply in .cc file. https://codereview.chromium.org/14268004/diff/28001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.h:252: bool client_vsync_notifications_enabled_; On 2013/04/22 23:46:48, aelias wrote: > Please rename to "renderer_vsync_notifications_enabled". The render process is > not usually referred to as a "client". Done. https://codereview.chromium.org/14268004/diff/28001/content/public/common/con... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/14268004/diff/28001/content/public/common/con... content/public/common/content_switches.cc:791: const char kEnableOverscrollEdgeEffect[] = "enable-overscroll-edge-effect"; On 2013/04/22 23:46:48, aelias wrote: > As part of this patch, please also append this flag in > content/browser/android/content_startup_flags.cc Done. https://codereview.chromium.org/14268004/diff/28001/content/public/common/con... File content/public/common/content_switches.h (right): https://codereview.chromium.org/14268004/diff/28001/content/public/common/con... content/public/common/content_switches.h:223: extern const char kEnableOverscrollEdgeEffect[]; On 2013/04/22 23:46:48, aelias wrote: > Probably should be marked CONTENT_EXPORT as well, it may be of interest to users > outside content/ Done.
It sounds like it ends up falling back to a render surface when you have multiple layers. That can cause both performance and rendering changes; Android devices don't handle them very well so we try to avoid ever using them. [+dtrainor] David, any thoughts on this?
I've reworked the detachment logic for the edge effect; it no longer uses the kludge of detecting layer depth, instead using focus lost (blur) signals to trigger detachment. This works quite well. In the long run, we'll want to do some closer inspection of how/when layers should trigger RenderSurface creation, and why performance suffers in such an event.
[+skyostil] For the vsync notification, I think you should explicitly rely on the refcounting behavior and call it once per animation/renderer. I don't consider that an implementation detail. I rather think the ContentViewCore method is misnamed and we should call it something that expresses its refcount behavior: maybe split it into two methods ContentViewCore.registerVsyncSubscriber()/unregisterVsyncSubscriber()?
+sievers, +nduca I don't think we want to add yet another place for scheduling animations. That's going to bite us when we switch to the threaded browser compositing (crbug.com/229743). How about we hook this up to the tab switcher animation? Something along the lines what happens in WebViewImpl::animate that checks whether there's an active overscroll animation, ticks it and uses the return value to see if we need to schedule another frame. That isn't so great for the threaded compositor either, but at least then we need to fix just one place instead of N. Nat, any better ideas? https://codereview.chromium.org/14268004/diff/43001/content/browser/android/c... File content/browser/android/content_startup_flags.cc (right): https://codereview.chromium.org/14268004/diff/43001/content/browser/android/c... content/browser/android/content_startup_flags.cc:62: parsed_command_line->AppendSwitch(switches::kEnableOverscrollEdgeEffect); Does this need #ifdef OS_ANDROID? https://codereview.chromium.org/14268004/diff/43001/content/browser/android/e... File content/browser/android/edge_effect.cc (right): https://codereview.chromium.org/14268004/diff/43001/content/browser/android/e... content/browser/android/edge_effect.cc:24: const int RECEDE_TIME = 1000; nit: Constants like this should be of the form const int kRecedeTime = 1000; https://codereview.chromium.org/14268004/diff/43001/content/browser/android/e... content/browser/android/edge_effect.cc:341: } nit: "} // namespace" so it's clear what this brace is for. https://codereview.chromium.org/14268004/diff/43001/content/browser/android/o... File content/browser/android/overscroll_glow.cc (right): https://codereview.chromium.org/14268004/diff/43001/content/browser/android/o... content/browser/android/overscroll_glow.cc:18: const float EPSILON = 1e-3f; nit: Constant naming here too. https://codereview.chromium.org/14268004/diff/43001/content/browser/android/o... content/browser/android/overscroll_glow.cc:63: } Nit: "} // namespace" https://codereview.chromium.org/14268004/diff/43001/content/browser/android/o... content/browser/android/overscroll_glow.cc:76: , vertical_overscroll_enabled_(true) { Nit: this comma should go to the end of the previous line.
Also see crbug.com/235012. Nat, can you help sketch out how the animation system in the browser should come together (maybe in that bug)?
On 2013/04/24 00:28:10, aelias wrote: > [+skyostil] For the vsync notification, I think you should explicitly rely on > the refcounting behavior and call it once per animation/renderer. I don't > consider that an implementation detail. I rather think the ContentViewCore > method is misnamed and we should call it something that expresses its refcount > behavior: maybe split it into two methods > ContentViewCore.registerVsyncSubscriber()/unregisterVsyncSubscriber()? I'm only worried about getting out of sync (...) in terms of who is subscribed to vsync. Now, we still probably want a way to distinguish between who is subscribed, so we can properly route calls in rwhva::SendVSync. But it's not clear how this behaves, say, when a tab crashes. At that point, VSync notifications are disabled, but rhwva doesn't know that and may still think it's subscribed. This is all academic if we move the animation elsewhere.
https://codereview.chromium.org/14268004/diff/43001/content/browser/android/c... File content/browser/android/content_startup_flags.cc (right): https://codereview.chromium.org/14268004/diff/43001/content/browser/android/c... content/browser/android/content_startup_flags.cc:62: parsed_command_line->AppendSwitch(switches::kEnableOverscrollEdgeEffect); On 2013/04/24 13:15:55, Sami wrote: > Does this need #ifdef OS_ANDROID? Quite right. https://codereview.chromium.org/14268004/diff/43001/content/browser/android/e... File content/browser/android/edge_effect.cc (right): https://codereview.chromium.org/14268004/diff/43001/content/browser/android/e... content/browser/android/edge_effect.cc:24: const int RECEDE_TIME = 1000; On 2013/04/24 13:15:55, Sami wrote: > nit: Constants like this should be of the form const int kRecedeTime = 1000; Hmm. Typically, yes, though I'm somewhat hesitant to make that change; all of these constants come directly from constants in the Android EdgeEffect.java class. The idea was that sharing the names might (hopefully) ease maintenance burdens in the future. I'm happy to adapt them to Chromium style if you think that best. https://codereview.chromium.org/14268004/diff/43001/content/browser/android/e... content/browser/android/edge_effect.cc:341: } On 2013/04/24 13:15:55, Sami wrote: > nit: "} // namespace" so it's clear what this brace is for. Done. https://codereview.chromium.org/14268004/diff/43001/content/browser/android/o... File content/browser/android/overscroll_glow.cc (right): https://codereview.chromium.org/14268004/diff/43001/content/browser/android/o... content/browser/android/overscroll_glow.cc:18: const float EPSILON = 1e-3f; On 2013/04/24 13:15:55, Sami wrote: > nit: Constant naming here too. Done. https://codereview.chromium.org/14268004/diff/43001/content/browser/android/o... content/browser/android/overscroll_glow.cc:76: , vertical_overscroll_enabled_(true) { On 2013/04/24 13:15:55, Sami wrote: > Nit: this comma should go to the end of the previous line. Done.
On 2013/04/24 15:23:44, jdduke wrote: > On 2013/04/24 13:15:55, Sami wrote: > > nit: Constants like this should be of the form const int kRecedeTime = 1000; > Hmm. Typically, yes, though I'm somewhat hesitant to make that change; all of > these constants come directly from constants in the Android EdgeEffect.java > class. The idea was that sharing the names might (hopefully) ease maintenance > burdens in the future. I'm happy to adapt them to Chromium style if you think > that best. EdgeEffect.java should only change when the Android system style changes, which isn't all that often I'd imagine. And if does change, the new style is likely going to look completely different than the old one so the code needs to be rewritten anyway :) That's why I'd prefer we be consistent with the rest of Chromium instead.
> EdgeEffect.java should only change when the Android system style changes, which > isn't all that often I'd imagine. And if does change, the new style is likely > going to look completely different than the old one so the code needs to be > rewritten anyway :) That's why I'd prefer we be consistent with the rest of > Chromium instead. That's good enough for me, I'll make the change.
On 2013/04/24 13:15:54, Sami wrote: > +sievers, +nduca > > I don't think we want to add yet another place for scheduling animations. That's > going to bite us when we switch to the threaded browser compositing > (crbug.com/229743). > > How about we hook this up to the tab switcher animation? Something along the > lines what happens in WebViewImpl::animate that checks whether there's an active > overscroll animation, ticks it and uses the return value to see if we need to > schedule another frame. That isn't so great for the threaded compositor either, > but at least then we need to fix just one place instead of N. What about hooking in with CompositorImpl/Compositor::Client? Perhaps adding Compositor::Client::OnAnimate or some such? This would also prevent duplicating edge effect layers for every tab.
Keep in mind I don't think there's anything wrong with continuing to manage overscroll and tab-switcher animations on the browser main thread, even in the ThreadProxy world. They don't need to be synchronized with renderer frames, and because CC's animation system is not nearly flexible enough for touch-driven use cases, we can't manage them in the impl thread without adding custom logic to CC.
@skyostil: As it stands, most of the code that drives tab switcher animations is not yet public. Short of fixing that, it may be difficult to unify how we handle these animations. Also, as @aelias mentioned, we're not to the point where either overscroll or tab-switcher can rely on CC's animation system; the commit model will probably be necessary for some time.
Ok how about instead of hooking into vsync you add an Animate() callback (from ContentViewCore to RWHVA)? If you make it return a bool whether it needs another update scheduled you can save plumbing through a second function maybe. For now, CompositorView and ContentViewRenderView can just call this before they call Composite(). Later, we can hook it into the appropriate LayerTreeHostClient hook. Do we really need a cmdline for this? When do we want this enabled/disabled?
On 2013/04/25 07:38:49, Daniel Sievers wrote: > Ok how about instead of hooking into vsync you add an Animate() callback (from > ContentViewCore to RWHVA)? > If you make it return a bool whether it needs another update scheduled you can > save plumbing through a second function maybe. Oh you don't want callbacks on every frame when there's no overscroll, so maybe you do need the ScheduleAnimation() call up too. > For now, CompositorView and ContentViewRenderView can just call this before they > call Composite(). Later, we can hook it into the appropriate LayerTreeHostClient > hook. > > Do we really need a cmdline for this? When do we want this enabled/disabled?
On 2013/04/25 07:46:21, Daniel Sievers wrote: > On 2013/04/25 07:38:49, Daniel Sievers wrote: > > Ok how about instead of hooking into vsync you add an Animate() callback (from > > ContentViewCore to RWHVA)? > > If you make it return a bool whether it needs another update scheduled you can > > save plumbing through a second function maybe. > > Oh you don't want callbacks on every frame when there's no overscroll, so maybe > you do need the ScheduleAnimation() call up too. This sounds cleaner than intercepting the vsync message in mid-flight. I think continuing to manage tab switching and overscroll on the main thread seems fine too; I'm just looking for a way we can do it without tying it too closely to how vsync is managed today.
I've added a (short) NeedsAnimate/Animate pipeline between RHWVA and ContentViewCore that is somewhat decoupled from the explicit vsync/input signals that are sent. As we resolve the issues blocking threaded browser compositing, we can easily adjust this pipeline. Calls to animate() will then originate from LTH, making their way to RHWVA via LTHC and so forth, and animation requests can be served directly to the LTH from the layers we are animating in RHWVA.
On 2013/05/09 19:15:38, jdduke wrote: > I've added a (short) NeedsAnimate/Animate pipeline between RHWVA and > ContentViewCore that is somewhat decoupled from the explicit vsync/input signals > that are sent. > > As we resolve the issues blocking threaded browser compositing, we can easily > adjust this pipeline. Calls to animate() will then originate from LTH, making > their way to RHWVA via LTHC and so forth, and animation requests can be served > directly to the LTH from the layers we are animating in RHWVA. And by RHWVA I mean RWHVA, by which I mean RenderWidgetHostViewAndroid, by which I mean cursed be these names.
lgtm https://codereview.chromium.org/14268004/diff/67016/content/browser/android/c... File content/browser/android/content_startup_flags.cc (right): https://codereview.chromium.org/14268004/diff/67016/content/browser/android/c... content/browser/android/content_startup_flags.cc:79: #if defined(OS_ANDROID) No need for #if defined(), this is already in an android/ directory. https://codereview.chromium.org/14268004/diff/67016/content/browser/android/c... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/14268004/diff/67016/content/browser/android/c... content/browser/android/content_view_core_impl.cc:1302: Nit: accidental whitespace
Thanks, this animation system looks great. lgtm. https://codereview.chromium.org/14268004/diff/67016/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/14268004/diff/67016/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.h:275: // Used to render overscroll overlays Nit: end comments with a period.
Alright, thanks everybody for your feedback/review/patience. Looks like this just needs owner approval for the following: @sievers (or @tedchoc I suppose): content/browser/android @tedchoc: content/public/android @jam: content/public/common @danakj: ui/gfx Thanks! https://codereview.chromium.org/14268004/diff/67016/content/browser/android/c... File content/browser/android/content_startup_flags.cc (right): https://codereview.chromium.org/14268004/diff/67016/content/browser/android/c... content/browser/android/content_startup_flags.cc:79: #if defined(OS_ANDROID) On 2013/05/10 04:37:07, aelias wrote: > No need for #if defined(), this is already in an android/ directory. Done. https://codereview.chromium.org/14268004/diff/67016/content/browser/android/c... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/14268004/diff/67016/content/browser/android/c... content/browser/android/content_view_core_impl.cc:1302: On 2013/05/10 04:37:07, aelias wrote: > Nit: accidental whitespace Oops, fixed. https://codereview.chromium.org/14268004/diff/67016/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/14268004/diff/67016/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.h:275: // Used to render overscroll overlays On 2013/05/10 10:47:57, Sami wrote: > Nit: end comments with a period. Done.
danakj -> sky You may notice the comment by my ownership, which applies to geometry stuff only.
LGTM
Looks good. A couple for nits inline. Can you consider removing the command line? It looked like it was only used in one place in RWHVA and since you are force-adding it for all embedders in this patch it does not seem to add any value. (If you want the app to choose whether it uses the glow effect, you need to let the app append the command line.)
(Copying my previous comment, this time publishing inline comments) Looks good. A couple for nits inline. Can you consider removing the command line? It looked like it was only used in one place in RWHVA and since you are force-adding it for all embedders in this patch it does not seem to add any value. (If you want the app to choose whether it uses the glow effect, you need to let the app append the command line.) https://codereview.chromium.org/14268004/diff/80001/content/browser/android/c... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/14268004/diff/80001/content/browser/android/c... content/browser/android/content_view_core_impl.h:202: jboolean Animate(JNIEnv* env, jobject /* obj */, jlong frame_time_micros); Hmm, why 'frame_time_micros'? Maybe just 'monotonic_time' or something? https://codereview.chromium.org/14268004/diff/80001/content/browser/android/e... File content/browser/android/edge_effect.cc (right): https://codereview.chromium.org/14268004/diff/80001/content/browser/android/e... content/browser/android/edge_effect.cc:157: = std::min(kMaxAlpha, nit: move '=' to line above https://codereview.chromium.org/14268004/diff/80001/content/browser/android/e... content/browser/android/edge_effect.cc:169: = Clamp(glow_scale_y_ + glow_change * kPullDistanceGlowFactor, nit: move '=' to line above https://codereview.chromium.org/14268004/diff/80001/content/browser/android/e... content/browser/android/edge_effect.cc:228: = std::min(0.025f + (velocity * (velocity / 100) * 0.00015f), 1.75f); nit: move '=' to line above https://codereview.chromium.org/14268004/diff/80001/content/browser/android/e... content/browser/android/edge_effect.cc:231: = Clamp(glow_alpha_start_, nit: move '=' to line above https://codereview.chromium.org/14268004/diff/80001/content/browser/android/e... content/browser/android/edge_effect.cc:309: nit: maybe move this into the existing anonymous namespace above. https://codereview.chromium.org/14268004/diff/80001/content/browser/android/e... content/browser/android/edge_effect.cc:352: { nit: why the stack frames here? https://codereview.chromium.org/14268004/diff/80001/content/browser/android/e... File content/browser/android/edge_effect.h (right): https://codereview.chromium.org/14268004/diff/80001/content/browser/android/e... content/browser/android/edge_effect.h:46: void Draw(cc::Layer* parent_layer, gfx::SizeF size, Edge edge); Should this be called something other than Draw(), maybe Animate()? Or collapse with Update() since that all seems to be the same stage? https://codereview.chromium.org/14268004/diff/80001/content/browser/android/o... File content/browser/android/overscroll_glow.cc (right): https://codereview.chromium.org/14268004/diff/80001/content/browser/android/o... content/browser/android/overscroll_glow.cc:247: break; nit: indent https://codereview.chromium.org/14268004/diff/80001/content/browser/android/o... File content/browser/android/overscroll_glow.h (right): https://codereview.chromium.org/14268004/diff/80001/content/browser/android/o... content/browser/android/overscroll_glow.h:43: void set_horizontal_overscroll_enabled(bool enabled); nit: SetHorizontalOverscrollEnabled(), and same for other methods. https://codereview.chromium.org/14268004/diff/80001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/14268004/diff/80001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:711: overscroll_effect_->set_parent_layer(content_view_core_->GetLayer()); Is it hard to change it so that it actually attaches it to the tree here (and detaches below) rather than remembering a reference and attaching to the parent at draw (or rather animate) time (see EdgeEffect::Draw())?
dumb question: is overscroll done on the impl thread?
On 2013/05/13 18:39:09, nduca wrote: > dumb question: is overscroll done on the impl thread? Yes. The patch that plumbs overscroll from LayerTreeHostImpl is pending in crrev.com/14499002. I decided to wait until @jamesr lands the input handling overhaul patch before going ahead with that.
On 2013/05/13 18:41:44, jdduke wrote: > On 2013/05/13 18:39:09, nduca wrote: > > dumb question: is overscroll done on the impl thread? > > Yes. The patch that plumbs overscroll from LayerTreeHostImpl is pending in > crrev.com/14499002. I decided to wait until @jamesr lands the input handling > overhaul patch before going ahead with that. Am confused. What you are adding here seems to be a main-thread animation system. I thought the reason was that not all the property changes could be done impl-side.
On 2013/05/13 18:37:35, Daniel Sievers wrote: > Looks good. A couple for nits inline. > > Can you consider removing the command line? It looked like it was only used in > one place in RWHVA and since you are force-adding it for all embedders in this > patch it does not seem to add any value. (If you want the app to choose whether > it uses the glow effect, you need to let the app append the command line.) @aelias: Do you have any preference here? I think the general idea is that we want this for content shell and Chrome, but not for WebView. I'm definitely open for suggestions on how else to handle this. I suppose we could have a disable flag instead?
On 2013/05/13 18:45:00, Daniel Sievers wrote: > On 2013/05/13 18:41:44, jdduke wrote: > > On 2013/05/13 18:39:09, nduca wrote: > > > dumb question: is overscroll done on the impl thread? > > > > Yes. The patch that plumbs overscroll from LayerTreeHostImpl is pending in > > crrev.com/14499002. I decided to wait until @jamesr lands the input handling > > overhaul patch before going ahead with that. > > Am confused. What you are adding here seems to be a main-thread animation > system. I thought the reason was that not all the property changes could be done > impl-side. Hmm, maybe I misread the question. @nduca: Overscroll notifications originate from the impl thread. Animation is main-thread driven out of convenience, and as part of a general push for consistency with the rest of the browser UI animations.
Overscroll is done on the browser main thread, and there's no plan to ever move it to the browser impl thread. Notifications triggering it are sent from the renderer main thread. On 2013/05/13 18:49:01, jdduke wrote: > On 2013/05/13 18:37:35, Daniel Sievers wrote: > > Looks good. A couple for nits inline. > > > > Can you consider removing the command line? It looked like it was only used in > > one place in RWHVA and since you are force-adding it for all embedders in this > > patch it does not seem to add any value. (If you want the app to choose > whether > > it uses the glow effect, you need to let the app append the command line.) > > @aelias: Do you have any preference here? I think the general idea is that we > want this for content shell and Chrome, but not for WebView. I'm definitely > open for suggestions on how else to handle this. I suppose we could have a > disable flag instead? A disable flag sounds good.
Urgh sorry, s/renderer main thread/renderer impl thread/ in what I said above.
Responding to review, with a few questions. https://codereview.chromium.org/14268004/diff/80001/content/browser/android/c... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/14268004/diff/80001/content/browser/android/c... content/browser/android/content_view_core_impl.h:202: jboolean Animate(JNIEnv* env, jobject /* obj */, jlong frame_time_micros); On 2013/05/13 18:38:24, Daniel Sievers wrote: > Hmm, why 'frame_time_micros'? Maybe just 'monotonic_time' or something? This was primarily for consistency with the *VSync calls above; the time sources are the same. https://codereview.chromium.org/14268004/diff/80001/content/browser/android/e... File content/browser/android/edge_effect.cc (right): https://codereview.chromium.org/14268004/diff/80001/content/browser/android/e... content/browser/android/edge_effect.cc:157: = std::min(kMaxAlpha, On 2013/05/13 18:38:24, Daniel Sievers wrote: > nit: move '=' to line above Done. https://codereview.chromium.org/14268004/diff/80001/content/browser/android/e... content/browser/android/edge_effect.cc:169: = Clamp(glow_scale_y_ + glow_change * kPullDistanceGlowFactor, On 2013/05/13 18:38:24, Daniel Sievers wrote: > nit: move '=' to line above Done. https://codereview.chromium.org/14268004/diff/80001/content/browser/android/e... content/browser/android/edge_effect.cc:228: = std::min(0.025f + (velocity * (velocity / 100) * 0.00015f), 1.75f); On 2013/05/13 18:38:24, Daniel Sievers wrote: > nit: move '=' to line above Done. https://codereview.chromium.org/14268004/diff/80001/content/browser/android/e... content/browser/android/edge_effect.cc:231: = Clamp(glow_alpha_start_, On 2013/05/13 18:38:24, Daniel Sievers wrote: > nit: move '=' to line above Done. https://codereview.chromium.org/14268004/diff/80001/content/browser/android/e... content/browser/android/edge_effect.cc:309: On 2013/05/13 18:38:24, Daniel Sievers wrote: > nit: maybe move this into the existing anonymous namespace above. Done. https://codereview.chromium.org/14268004/diff/80001/content/browser/android/e... content/browser/android/edge_effect.cc:352: { On 2013/05/13 18:38:24, Daniel Sievers wrote: > nit: why the stack frames here? No good reason other than logical separation, removed. https://codereview.chromium.org/14268004/diff/80001/content/browser/android/e... File content/browser/android/edge_effect.h (right): https://codereview.chromium.org/14268004/diff/80001/content/browser/android/e... content/browser/android/edge_effect.h:46: void Draw(cc::Layer* parent_layer, gfx::SizeF size, Edge edge); On 2013/05/13 18:38:24, Daniel Sievers wrote: > Should this be called something other than Draw(), maybe Animate()? Or collapse > with Update() since that all seems to be the same stage? Yeah, I went back and forth on this, particularly when the layers were owned by the parent. I wouldn't mind keeping the logical updates separate from the layer updates, but "Draw" is probably a bad name here. What about "ApplyToLayers" or "UpdateLayers"? https://codereview.chromium.org/14268004/diff/80001/content/browser/android/o... File content/browser/android/overscroll_glow.cc (right): https://codereview.chromium.org/14268004/diff/80001/content/browser/android/o... content/browser/android/overscroll_glow.cc:247: break; On 2013/05/13 18:38:24, Daniel Sievers wrote: > nit: indent Done. https://codereview.chromium.org/14268004/diff/80001/content/browser/android/o... File content/browser/android/overscroll_glow.h (right): https://codereview.chromium.org/14268004/diff/80001/content/browser/android/o... content/browser/android/overscroll_glow.h:43: void set_horizontal_overscroll_enabled(bool enabled); On 2013/05/13 18:38:24, Daniel Sievers wrote: > nit: SetHorizontalOverscrollEnabled(), and same for other methods. Why? It's a trivial setter? https://codereview.chromium.org/14268004/diff/80001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/14268004/diff/80001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:711: overscroll_effect_->set_parent_layer(content_view_core_->GetLayer()); On 2013/05/13 18:38:24, Daniel Sievers wrote: > Is it hard to change it so that it actually attaches it to the tree here (and > detaches below) rather than remembering a reference and attaching to the parent > at draw (or rather animate) time (see EdgeEffect::Draw())? Yeah, I went back and forth on this as well. The reasoning for the current approach is that, as an EdgeEffect becomes dormant, it detaches its layers from the parent layer. ImageLayers (currently) can't clip themselves, so this prevents RenderSurface promotion down the pipeline, and generally avoids cluttering the layer tree. So, either the EdgeEffect needs to cache its parent for when it's re-activated, or OverscrollGlow needs to cache the parent and we just update it if necessary at every Draw call. The other thought I had was making an intermediate (but permanent) parent layer in OverscrollGlow, and let RWHVA set the parent of that just like it sets the parent of its owned layer_.
Enable-by-default with disable cmdline sounds good. I think WebView will deactivate it. https://codereview.chromium.org/14268004/diff/80001/content/browser/android/e... File content/browser/android/edge_effect.h (right): https://codereview.chromium.org/14268004/diff/80001/content/browser/android/e... content/browser/android/edge_effect.h:46: void Draw(cc::Layer* parent_layer, gfx::SizeF size, Edge edge); On 2013/05/13 20:44:24, jdduke wrote: > On 2013/05/13 18:38:24, Daniel Sievers wrote: > > Should this be called something other than Draw(), maybe Animate()? Or > collapse > > with Update() since that all seems to be the same stage? > > Yeah, I went back and forth on this, particularly when the layers were owned by > the parent. I wouldn't mind keeping the logical updates separate from the layer > updates, but "Draw" is probably a bad name here. What about "ApplyToLayers" or > "UpdateLayers"? I like ApplyToLayers(). https://codereview.chromium.org/14268004/diff/80001/content/browser/android/o... File content/browser/android/overscroll_glow.h (right): https://codereview.chromium.org/14268004/diff/80001/content/browser/android/o... content/browser/android/overscroll_glow.h:43: void set_horizontal_overscroll_enabled(bool enabled); On 2013/05/13 20:44:24, jdduke wrote: > On 2013/05/13 18:38:24, Daniel Sievers wrote: > > nit: SetHorizontalOverscrollEnabled(), and same for other methods. > > Why? It's a trivial setter? I think lower-case with underscores is normally used with inline functions only though. https://codereview.chromium.org/14268004/diff/80001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/14268004/diff/80001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_view_android.cc:711: overscroll_effect_->set_parent_layer(content_view_core_->GetLayer()); On 2013/05/13 20:44:24, jdduke wrote: > On 2013/05/13 18:38:24, Daniel Sievers wrote: > > Is it hard to change it so that it actually attaches it to the tree here (and > > detaches below) rather than remembering a reference and attaching to the > parent > > at draw (or rather animate) time (see EdgeEffect::Draw())? > > Yeah, I went back and forth on this as well. The reasoning for the current > approach is that, as an EdgeEffect becomes dormant, it detaches its layers from > the parent layer. ImageLayers (currently) can't clip themselves, so this > prevents RenderSurface promotion down the pipeline, and generally avoids > cluttering the layer tree. > And it doesn't work to just make it not drawable or so?
> https://codereview.chromium.org/14268004/diff/80001/content/browser/renderer_... > File content/browser/renderer_host/render_widget_host_view_android.cc (right): > > https://codereview.chromium.org/14268004/diff/80001/content/browser/renderer_... > content/browser/renderer_host/render_widget_host_view_android.cc:711: > overscroll_effect_->set_parent_layer(content_view_core_->GetLayer()); > On 2013/05/13 20:44:24, jdduke wrote: > > On 2013/05/13 18:38:24, Daniel Sievers wrote: > > > Is it hard to change it so that it actually attaches it to the tree here > (and > > > detaches below) rather than remembering a reference and attaching to the > > parent > > > at draw (or rather animate) time (see EdgeEffect::Draw())? > > > > Yeah, I went back and forth on this as well. The reasoning for the current > > approach is that, as an EdgeEffect becomes dormant, it detaches its layers > from > > the parent layer. ImageLayers (currently) can't clip themselves, so this > > prevents RenderSurface promotion down the pipeline, and generally avoids > > cluttering the layer tree. > > > > > And it doesn't work to just make it not drawable or so? Unfortunately, no. The rotation of the ImageLayer forces "descendants_can_clip_selves" to false when walking the tree, which in turn causes RenderSurface creation. It's a shame, really. The offending check is: if ((child_layer->DrawsContent() && !child_layer->CanClipSelf()) || !child_layer->draw_properties().descendants_can_clip_selves || sublayer_transform_prevents_clip || !child_layer->transform().IsPositiveScaleOrTranslation()) descendants_can_clip_selves = false; I would think that the "if" clause should exclude child layers that neither have children nor draw content, no?
On 2013/05/13 21:03:53, jdduke wrote: > > > https://codereview.chromium.org/14268004/diff/80001/content/browser/renderer_... > > File content/browser/renderer_host/render_widget_host_view_android.cc (right): > > > > > https://codereview.chromium.org/14268004/diff/80001/content/browser/renderer_... > > content/browser/renderer_host/render_widget_host_view_android.cc:711: > > overscroll_effect_->set_parent_layer(content_view_core_->GetLayer()); > > On 2013/05/13 20:44:24, jdduke wrote: > > > On 2013/05/13 18:38:24, Daniel Sievers wrote: > > > > Is it hard to change it so that it actually attaches it to the tree here > > (and > > > > detaches below) rather than remembering a reference and attaching to the > > > parent > > > > at draw (or rather animate) time (see EdgeEffect::Draw())? > > > > > > Yeah, I went back and forth on this as well. The reasoning for the current > > > approach is that, as an EdgeEffect becomes dormant, it detaches its layers > > from > > > the parent layer. ImageLayers (currently) can't clip themselves, so this > > > prevents RenderSurface promotion down the pipeline, and generally avoids > > > cluttering the layer tree. > > > > > > > > > And it doesn't work to just make it not drawable or so? > > Unfortunately, no. The rotation of the ImageLayer forces > "descendants_can_clip_selves" to false when walking the tree, which in turn > causes RenderSurface creation. It's a shame, really. The offending check is: > > if ((child_layer->DrawsContent() && !child_layer->CanClipSelf()) || > !child_layer->draw_properties().descendants_can_clip_selves || > sublayer_transform_prevents_clip || > !child_layer->transform().IsPositiveScaleOrTranslation()) > descendants_can_clip_selves = false; > > I would think that the "if" clause should exclude child layers that neither have > children nor draw content, no? Well, I think I may have answered my own question.. Let me try resetting the transforms in addition to making them non-drawable, and see if that doesn't get around the issue.
It turns out that we can avoid the attach/detach logic altogether by simply resetting the layer's transform, opacity, and making it non-drawable. It's also now enabled by default. I believe the remaining sign-offs are: @joth: content/public/android @jam: content/public/common Thanks all!
Thanks, that looks great. LGTM with cmdline string fixed. https://codereview.chromium.org/14268004/diff/115001/content/public/common/co... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/14268004/diff/115001/content/public/common/co... content/public/common/content_switches.cc:757: const char kDisableOverscrollEdgeEffect[] = "enable-overscroll-edge-effect"; = "disable-overscroll-edge-effect"
Hmm, it appears both @tedchoc and @joth are out. Yaron, would you mind taking a
look at content/public/android?
@jam: content/public/common/content_switches.{h,cc}
@yfriedman:
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
Thanks.
https://codereview.chromium.org/14268004/diff/115001/content/public/common/co...
File content/public/common/content_switches.cc (right):
https://codereview.chromium.org/14268004/diff/115001/content/public/common/co...
content/public/common/content_switches.cc:757: const char
kDisableOverscrollEdgeEffect[] = "enable-overscroll-edge-effect";
On 2013/05/14 18:12:15, Daniel Sievers wrote:
> = "disable-overscroll-edge-effect"
Good catch, fixed.
https://codereview.chromium.org/14268004/diff/118001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/14268004/diff/118001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2764: public boolean animate(long frameTimeMicros) { Is it just me that thinks "animate" is too vague? ContentViewCore is a huge class and this isn't descriptive enough. Also missing javadoc for a public member.
https://codereview.chromium.org/14268004/diff/118001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/14268004/diff/118001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2764: public boolean animate(long frameTimeMicros) { On 2013/05/16 17:38:40, Yaron wrote: > Is it just me that thinks "animate" is too vague? ContentViewCore is a huge > class and this isn't descriptive enough. Also missing javadoc for a public > member. Good point. It is vague, and in fact should not be public. Originally, I had the animate() call come from CompositorImpl by way of CompositorView{Holder}, all triggered by LayerTreeHostClient::Animate(frame_begin_time). In that context, it's a bit less vague, but we're not using that approach (yet). So, at the moment it's simply a call requested by setNeedsAnimate() and triggered by a vsync tick; ContentViewCore doesn't really know or need to know anything beyond that. onAnimate() might be slightly more clear in that it's simply a forwarding callback?
onAnimate() sounds fine to me given that it's a generic animation hook.
Ok. Definitely happier if it's not public. Part of me wonders if more of this should be in RWHVA, but it looks like there's some related code already so I won't block you on it. On Thu, May 16, 2013 at 11:35 AM, <aelias@chromium.org> wrote: > onAnimate() sounds fine to me given that it's a generic animation hook. > > https://codereview.chromium.**org/14268004/<https://codereview.chromium.org/1... >
Looks like @jam is out.
@joi: Could you review content/public/common/content_switches.{h,cc}? Thanks.
On 2013/05/16 18:41:25, Yaron wrote: > Ok. Definitely happier if it's not public. Part of me wonders if more of > this should be in RWHVA, but it looks like there's some related code > already so I won't block you on it. > Originally, it was. However, that relied on intercepting both vsync ticks and input events to trigger the animation call. As per review, it was decided that some explicit animation logic would be both cleaner and somewhat less fragile, especially as we start migrating all browser-side animations to a similar update path.
LGTM for //content/public/common.
On 2013/05/16 18:41:25, Yaron wrote: > Ok. Definitely happier if it's not public. Part of me wonders if more of > this should be in RWHVA, but it looks like there's some related code > already so I won't block you on it. > > This needs to be public (at least when we switch to threaded browser compositing - but we can talk about it more when that happens), because the app is involved in scheduling (the app is the compositor client). We're trying to keep it light but ContentViewCore needs to know a bit about layers and such. The other part in the public API is the compositor. It's then up to the app to put the two together. If this grows, we could also look into how to separate this logic out a bit - esp. considering that WebView with merged threads will not use the browser compositor anymore. (At the same time we don't want to make this an officially supported embedder mode - I think.)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jdduke@chromium.org/14268004/134016
Message was sent while issue was closed.
Change committed as 200846 |
