Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22265009:
Implement initial version of scroll end effect
|
Created:
7 years, 4 months ago by rharrison Modified:
7 years ago CC:
chromium-reviews, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement initial version of scroll end effect
This is the first step of implementing the scroll end effect as shown in
the mocks. This version of the code depends primarily on bounds
manipulation to achieve the desired effect. It specifically depends on
the fast resize path existing in NativeViewHost, which has recently been
done for NativeViewHostAura. The intent of this CL is land an initial
version of the effect, which can be polished and launched while
determining what further work is needed.
This CL does not implement the effect in immersive/fullscreen mode and
does not implement the subtle shift of the tab contents. Those will be
implemented in follow on CLs.
BUG=151356
BUG=282463
TEST=Manually confirmed that effect behaves as expected with flag
enabled and does not occur with flag disabled.
Added new tests and confirmed they work.
Patch Set 1 #
Total comments: 10
Patch Set 2 : Added comment blob and clean up #Patch Set 3 : Fixed build issue on windows and updated description. #
Total comments: 8
Patch Set 4 : Fixed comment/style issues from sky@ #Patch Set 5 : Major rewrite to handle corner cases and clean up code #
Total comments: 4
Patch Set 6 : De-nitted #Patch Set 7 : Fixed stale paint in top container #Patch Set 8 : Added SlideAnimation to smooth out transitions #
Total comments: 16
Patch Set 9 : Cleaned up animation code #
Total comments: 13
Patch Set 10 : Code cleanup for sadrul@ #
Total comments: 37
Patch Set 11 : Responded to most of sky@'s comments #Patch Set 12 : Missed adding the delegate changes #Patch Set 13 : Missed adding the delegate changes #Patch Set 14 : Rewrote CL to use bounds manipulation instead of transforms. #Patch Set 15 : Moved clipping into NativeViewHostAura::InstallClip #Patch Set 16 : Moved the NVHA changes to their own CL #Patch Set 17 : Rebased, fixed bounds glitch, improved bypass for fullscreen #Patch Set 18 : Added tests for ScrollEndEffectControllerAsh #Patch Set 19 : Fixed build breakage #Patch Set 20 : Fixed another compile issue :-/ #
Total comments: 16
Patch Set 21 : Responded to outstanding comments from sadrul@ #Messages
Total messages: 44 (0 generated)
PTAL
On 2013/08/09 15:34:12, rharrison wrote: > PTAL ping? :-(
On 2013/08/19 20:40:00, rharrison wrote: > On 2013/08/09 15:34:12, rharrison wrote: > > PTAL > > ping? :-( Math in ApplyDelta lg2m.
This looks reasonable. Please send to owners in the next iteration. https://codereview.chromium.org/22265009/diff/1/chrome/browser/ui/views/frame... File chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc (right): https://codereview.chromium.org/22265009/diff/1/chrome/browser/ui/views/frame... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:85: CHECK(web_contents_layer_ != NULL); CHECK(web_contents_layer_), or CHECK_NE https://codereview.chromium.org/22265009/diff/1/chrome/browser/ui/views/frame... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:93: frame_clipping_bounds.set_origin(gfx::Point(0,0)); gfx::Point() https://codereview.chromium.org/22265009/diff/1/chrome/browser/ui/views/frame... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:118: web_clipping_layer_->Add(web_contents_layer_); I think this looks reasonable. Two notes: * Is it necessary to maintain the pointers to the layers? (i.e. instead of keeping |browser_frame_layer_| around, why not use |browser_view_->frame()->GetLayer()| in a local variable where necessary? etc. * Can you please explain the layer hierarchy before/during/after the overscroll? https://codereview.chromium.org/22265009/diff/1/chrome/browser/ui/views/frame... File chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.h (right): https://codereview.chromium.org/22265009/diff/1/chrome/browser/ui/views/frame... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.h:27: // Set up the layer topology for showing the effect. This involves making sure Sets up https://codereview.chromium.org/22265009/diff/1/chrome/browser/ui/views/frame... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.h:31: // Restore the layer topology and states to what they were before the effect Restores
Adding sky@ for OWNERS https://codereview.chromium.org/22265009/diff/1/chrome/browser/ui/views/frame... File chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc (right): https://codereview.chromium.org/22265009/diff/1/chrome/browser/ui/views/frame... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:85: CHECK(web_contents_layer_ != NULL); On 2013/08/20 16:14:59, sadrul wrote: > CHECK(web_contents_layer_), or CHECK_NE Done. https://codereview.chromium.org/22265009/diff/1/chrome/browser/ui/views/frame... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:93: frame_clipping_bounds.set_origin(gfx::Point(0,0)); On 2013/08/20 16:14:59, sadrul wrote: > gfx::Point() Done. https://codereview.chromium.org/22265009/diff/1/chrome/browser/ui/views/frame... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:118: web_clipping_layer_->Add(web_contents_layer_); On 2013/08/20 16:14:59, sadrul wrote: > I think this looks reasonable. Two notes: > * Is it necessary to maintain the pointers to the layers? (i.e. instead of > keeping |browser_frame_layer_| around, why not use > |browser_view_->frame()->GetLayer()| in a local variable where necessary? etc. > > * Can you please explain the layer hierarchy before/during/after the > overscroll? Done. https://codereview.chromium.org/22265009/diff/1/chrome/browser/ui/views/frame... File chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.h (right): https://codereview.chromium.org/22265009/diff/1/chrome/browser/ui/views/frame... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.h:27: // Set up the layer topology for showing the effect. This involves making sure On 2013/08/20 16:14:59, sadrul wrote: > Sets up Done. https://codereview.chromium.org/22265009/diff/1/chrome/browser/ui/views/frame... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.h:31: // Restore the layer topology and states to what they were before the effect On 2013/08/20 16:14:59, sadrul wrote: > Restores Done.
Is this description correct? I thought we weren't going with the obviously wrong approach?
On 2013/08/20 21:01:52, sky wrote: > Is this description correct? I thought we weren't going with the obviously wrong > approach? This CL is significantly different then what I proposed before. I have updated the description to hopefully make it clearer what I am doing. This CL is the first one in a series that will implement the full effect. This one focuses on using transforms to get the coarse level details correct; the window resizing, the content sliding around, and the elements of the window not being scaled. The web contents needs to be positioned using transforms since changing the bounds causes the page to change its layout. The shifting of the tab strip is no included in this CL since it will either require refactoring of the layout of the top container elements to allow a transform to work, or I will have to manipulate the bounds of elements. I am not sure which is the right way forward at the moment.
I'm a bit hesitant to look at the nitty gritty until you know how you'are going to move forward. Also, what happens if the webcontents changes during the animation, or say the bookmark bar comes/goes, or infobars or anything else that effects position. https://codereview.chromium.org/22265009/diff/31001/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.h (right): https://codereview.chromium.org/22265009/diff/31001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.h:27: // Sets up the layer topology for showing the effect. This involves making I don't think we've ever used topology for what this does. We typically use hierarchy. https://codereview.chromium.org/22265009/diff/31001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.h:36: void ApplyDelta(ui::Layer* frame, gfx::Rect bounds, int delta_y); const gfx::Rect& and add a description. https://codereview.chromium.org/22265009/diff/31001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.h:47: // |web_clipping_layer_| so that things like the controls show through.| nuke trailing | https://codereview.chromium.org/22265009/diff/31001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.h:53: // Saved when the effective is activated, so it can be restored when the effective?
I resolved the inline comments you had. I am going to look at writing a patch that implements the top container manipulations. I will send out another message when I am ready for this CL to be reviewed again. https://codereview.chromium.org/22265009/diff/31001/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.h (right): https://codereview.chromium.org/22265009/diff/31001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.h:27: // Sets up the layer topology for showing the effect. This involves making On 2013/08/21 21:01:01, sky wrote: > I don't think we've ever used topology for what this does. We typically use > hierarchy. Done. https://codereview.chromium.org/22265009/diff/31001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.h:36: void ApplyDelta(ui::Layer* frame, gfx::Rect bounds, int delta_y); On 2013/08/21 21:01:01, sky wrote: > const gfx::Rect& and add a description. Done. https://codereview.chromium.org/22265009/diff/31001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.h:47: // |web_clipping_layer_| so that things like the controls show through.| On 2013/08/21 21:01:01, sky wrote: > nuke trailing | Done. https://codereview.chromium.org/22265009/diff/31001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.h:53: // Saved when the effective is activated, so it can be restored when the On 2013/08/21 21:01:01, sky wrote: > effective? Done.
Also, make sure you deal with the following: webcontents size changing during the animation, or say the bookmark bar comes/goes, or infobars or anything else that effects position/size of webcontents.
On 2013/08/22 22:53:04, sky wrote: > Also, make sure you deal with the following: webcontents size changing during > the animation, or say the bookmark bar comes/goes, or infobars or anything else > that effects position/size of webcontents. sadrul, vollick: I have addressed sky's concerns, can you please re-review before I get him to review again, since I have pretty much rewritten the CL?
Math in ApplyDelta lg2m. https://codereview.chromium.org/22265009/diff/39001/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc (right): https://codereview.chromium.org/22265009/diff/39001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:72: // | web_contents_layer_ | Nice diagram! https://codereview.chromium.org/22265009/diff/39001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:97: return; nit: no braces.
https://codereview.chromium.org/22265009/diff/39001/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc (right): https://codereview.chromium.org/22265009/diff/39001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:72: // | web_contents_layer_ | On 2013/09/04 16:41:09, Ian Vollick wrote: > Nice diagram! thanks https://codereview.chromium.org/22265009/diff/39001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:97: return; On 2013/09/04 16:41:09, Ian Vollick wrote: > nit: no braces. Done.
vollick, can you review this yet again :-D Added in the animation code here instead of another CL, since I don't think it adds much complexity to the CL. After talking to vollick and avallee, for the short term this effect use a SlideAnimation and once the infrastructure exists, be transitioned to the code that they are working on.
Yeah, that animation code looks pretty reasonable and simple. https://codereview.chromium.org/22265009/diff/75001/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc (right): https://codereview.chromium.org/22265009/diff/75001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:137: if (animation != animation_.get()) Could there be another animation? https://codereview.chromium.org/22265009/diff/75001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:149: int current_delta = start_delta_y_ + I think you want to use ui::Tween::ValueBetween here. Check out tween.h
https://codereview.chromium.org/22265009/diff/75001/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc (right): https://codereview.chromium.org/22265009/diff/75001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:137: if (animation != animation_.get()) On 2013/09/05 02:33:48, Ian Vollick wrote: > Could there be another animation? No there shouldn't be. I was being overly defensive I think. https://codereview.chromium.org/22265009/diff/75001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:149: int current_delta = start_delta_y_ + On 2013/09/05 02:33:48, Ian Vollick wrote: > I think you want to use ui::Tween::ValueBetween here. Check out tween.h Done.
Some minor nits, a couple of questions below. Otherwise looks good! Please note that some of the following comments are in patchset8, which also applies for patchset9 (I started reviewing and left these comments before the next patchset was uploaded). https://codereview.chromium.org/22265009/diff/75001/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/22265009/diff/75001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/browser_view.cc:1862: return frame_->GetContentsView(); return frame_ ? frame_->.. : NULL https://codereview.chromium.org/22265009/diff/75001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/browser_view.cc:1865: views::View* BrowserView::GetDownloadView() { indent is off https://codereview.chromium.org/22265009/diff/75001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/browser_view.cc:1866: return static_cast<DownloadShelfView*>(GetDownloadShelf()); This can create the download-shelf if it didn't already exist, right? Is there a way to avoid this? https://codereview.chromium.org/22265009/diff/75001/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/browser_view.h (right): https://codereview.chromium.org/22265009/diff/75001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/browser_view.h:474: views::WebView* GetDevToolsWebViewForTest() { return devtools_container_; } Is this used? https://codereview.chromium.org/22265009/diff/75001/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.h (right): https://codereview.chromium.org/22265009/diff/75001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.h:45: ui::Layer* CreateClippingLayer(std::string name); const std::string& https://codereview.chromium.org/22265009/diff/75001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.h:77: ui::Layer* devtools_layer_; // non-owned Can you just leave a comment at the top of the block that only the scoped_ptr<>s are owned by the Controller, and all the rest of the layers are owned by the delegate? https://codereview.chromium.org/22265009/diff/81001/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc (right): https://codereview.chromium.org/22265009/diff/81001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:128: ApplyDelta(end_delta_y_); This seems a bit odd? I would've thought we would do ApplyDelta(capped_delta_y) here? https://codereview.chromium.org/22265009/diff/81001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:196: ui::Layer* ScrollEndEffectControllerAsh::CreateViewLayer(views::View* view) { CreateViewLayer and CreateClippingLayer could be separate functions in anonymous namespace here (instead of being part of ScrollEndEffectControllerAsh) https://codereview.chromium.org/22265009/diff/81001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:239: SetBoundsForEffect(); Hm. I assume you call this for each ApplyDelta() call, as opposed to just once in ActivateEffect(), is to make sure the views/layers are setup correctly if something (e.g. bookmark bar, download shelf) changes during the effect? https://codereview.chromium.org/22265009/diff/81001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:241: ui::Layer* frame = delegate_->GetBrowserFrameLayer(); Is there a reason GetBrowserFrameLayer() isn't cached in ActiveEffect like all the other layers/views? https://codereview.chromium.org/22265009/diff/81001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:249: if (delta_y > 0) { Can you set a 'bool scrolling_down = delta_y > 0;' above, and use 'if (scrolling_down) ...' and 'if (!scrolling_down)' in the rest of this function? That makes it a bit easier to follow. https://codereview.chromium.org/22265009/diff/81001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:328: web_contents_bounds.set_origin(gfx::Point(0, 0)); gfx::Point() like above https://codereview.chromium.org/22265009/diff/81001/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/scroll_end_effect_controller_delegate.h (right): https://codereview.chromium.org/22265009/diff/81001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_delegate.h:38: #endif // CHROME_BROWSER_UI_VIEWS_FRAME_SCROLL_END_EFFECT_CONTROLLER_DELEGATE_H_ Ha!
sky@, PTAL I have addressed all of sadrul's concerns https://codereview.chromium.org/22265009/diff/75001/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/22265009/diff/75001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/browser_view.cc:1862: return frame_->GetContentsView(); On 2013/09/05 16:09:02, sadrul wrote: > return frame_ ? frame_->.. : NULL Done. https://codereview.chromium.org/22265009/diff/75001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/browser_view.cc:1865: views::View* BrowserView::GetDownloadView() { On 2013/09/05 16:09:02, sadrul wrote: > indent is off Done. https://codereview.chromium.org/22265009/diff/75001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/browser_view.cc:1866: return static_cast<DownloadShelfView*>(GetDownloadShelf()); On 2013/09/05 16:09:02, sadrul wrote: > This can create the download-shelf if it didn't already exist, right? Is there a > way to avoid this? Done. https://codereview.chromium.org/22265009/diff/75001/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/browser_view.h (right): https://codereview.chromium.org/22265009/diff/75001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/browser_view.h:474: views::WebView* GetDevToolsWebViewForTest() { return devtools_container_; } On 2013/09/05 16:09:02, sadrul wrote: > Is this used? Nope https://codereview.chromium.org/22265009/diff/75001/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.h (right): https://codereview.chromium.org/22265009/diff/75001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.h:45: ui::Layer* CreateClippingLayer(std::string name); On 2013/09/05 16:09:02, sadrul wrote: > const std::string& Done. https://codereview.chromium.org/22265009/diff/75001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.h:77: ui::Layer* devtools_layer_; // non-owned On 2013/09/05 16:09:02, sadrul wrote: > Can you just leave a comment at the top of the block that only the scoped_ptr<>s > are owned by the Controller, and all the rest of the layers are owned by the > delegate? Done. https://codereview.chromium.org/22265009/diff/81001/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc (right): https://codereview.chromium.org/22265009/diff/81001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:128: ApplyDelta(end_delta_y_); On 2013/09/05 16:09:02, sadrul wrote: > This seems a bit odd? I would've thought we would do ApplyDelta(capped_delta_y) > here? Just applying the capped value would cause the effect skip between updates and bypass the animation. What we really want is to have the animation occur when the user stops overscrolling. That occurs when the delta is 0, so I have re-written the code to handle this. There is also the possibility that they have transitioned through 0 when changing directions, so I have added handling for this case. https://codereview.chromium.org/22265009/diff/81001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:196: ui::Layer* ScrollEndEffectControllerAsh::CreateViewLayer(views::View* view) { On 2013/09/05 16:09:02, sadrul wrote: > CreateViewLayer and CreateClippingLayer could be separate functions in anonymous > namespace here (instead of being part of ScrollEndEffectControllerAsh) Done. https://codereview.chromium.org/22265009/diff/81001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:239: SetBoundsForEffect(); On 2013/09/05 16:09:02, sadrul wrote: > Hm. I assume you call this for each ApplyDelta() call, as opposed to just once > in ActivateEffect(), is to make sure the views/layers are setup correctly if > something (e.g. bookmark bar, download shelf) changes during the effect? yup https://codereview.chromium.org/22265009/diff/81001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:241: ui::Layer* frame = delegate_->GetBrowserFrameLayer(); On 2013/09/05 16:09:02, sadrul wrote: > Is there a reason GetBrowserFrameLayer() isn't cached in ActiveEffect like all > the other layers/views? It is an artifact from how I refactored the code. Fixed https://codereview.chromium.org/22265009/diff/81001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:249: if (delta_y > 0) { On 2013/09/05 16:09:02, sadrul wrote: > Can you set a 'bool scrolling_down = delta_y > 0;' above, and use 'if > (scrolling_down) ...' and 'if (!scrolling_down)' in the rest of this function? > That makes it a bit easier to follow. Done. https://codereview.chromium.org/22265009/diff/81001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:328: web_contents_bounds.set_origin(gfx::Point(0, 0)); On 2013/09/05 16:09:02, sadrul wrote: > gfx::Point() like above Done.
How come no tests? https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/browser_view.cc:1845: return frame_ ? frame_->GetLayer() : NULL; Do you really need the NULL checks for frame_ here and 1858? https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/browser_view.cc:1866: views::View* devtools_view = contents_split_->child_at(1); Can't you use devtools_container_ directly? https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/browser_view.cc:1874: nit: only one newline. https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/scroll_end_effect_controller.h (right): https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller.h:10: class BrowserView; don't need BrowserView here. https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc (right): https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:38: view->parent()->SchedulePaint(); Why do you need the SchedulePaint here? https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:107: web_contents_parent_(NULL), Make sure you initialize everything (looks like you're missing some, including download_layer_...) https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:120: DeactivateEffect(); Did you verify you don't crash if you end up here? I ask as I suspect you do, but I'm not positive. In particular I think some of the views you're using have since been destroyed by the time you get here. https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:124: if (delegate_ == NULL) You never delegate_ to NULL, so why this check? https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:130: frame_layer_ = delegate_->GetBrowserFrameLayer(); Seems like you should only set frame_layer_ if you're going to activate. https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:186: CHECK(non_client_layer_); DCHECK on these https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:195: devtools_bounds_ = devtools_layer_->bounds(); How do you know these bounds are going to be right at the time you're done? https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:198: // Adjust the toplogy of the layer tree to add clipping layers toplogy->hierarchy https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:207: web_clipping_layer_->Add(web_contents_layer_); This all strikes me as very fragile. In particular BrowserView and related classes were to add/remove a view then all this is going to break. It feels wrong that you have to mess with the layer tree to this degree to get the effect you're after. Seems safer it things could be set up initially to match what you're after so you don't have to shuffle things around. Maybe with extra views in the hierarchy that you toggle layered painting on/off. https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:245: ui::Layer* frame = delegate_->GetBrowserFrameLayer(); Isn't this frame_layer_? https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:340: return !download_view_ ? 0 : nit: return download_view_ && download_view_->visible() ? download_view_->bounds().height() : 0 is a lot easier to read. https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:345: gfx::Transform ScrollEndEffectControllerAsh::CounterTransfromAboutPoint(int x, transform and const https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/scroll_end_effect_controller_delegate.h (right): https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_delegate.h:37: }; porotected virtual destructor. https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_delegate.h:38: #endif // CHROME_BROWSER_UI_VIEWS_FRAME_SCROLL_END_EFFECT_CONTROLLER_DELEGATE_H_ nit: newline between 37/38.
sorry about the plenitude of uploads, missed part of the changes and then it succeeded to upload but indicated it failed :-/ I have not yet added tests to this CL, since sky@ has concerns about the entire approach I am taking due brittleness/fragility. sky, I have responded to your comments on it being fragile. Please advise if you think I should continue pursuing this approach and write tests for it or it should be abandoned. https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/browser_view.cc:1845: return frame_ ? frame_->GetLayer() : NULL; On 2013/09/05 21:12:57, sky wrote: > Do you really need the NULL checks for frame_ here and 1858? Looking at the code again, I don't think so. I was likely concerned about the delayed setting of |frame_|, but it is as part of creating the browser window, so should never be NULL when this code is called. https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/browser_view.cc:1866: views::View* devtools_view = contents_split_->child_at(1); On 2013/09/05 21:12:57, sky wrote: > Can't you use devtools_container_ directly? Done. https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/browser_view.cc:1874: On 2013/09/05 21:12:57, sky wrote: > nit: only one newline. Done. https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/scroll_end_effect_controller.h (right): https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller.h:10: class BrowserView; On 2013/09/05 21:12:57, sky wrote: > don't need BrowserView here. Done. https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc (right): https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:38: view->parent()->SchedulePaint(); On 2013/09/05 21:12:57, sky wrote: > Why do you need the SchedulePaint here? Without it there is a stale version of the parent with the view still in it, since nothing else is scheduling a paint for it while the effect is running. When using a translucent frame this causes the what looks like a double paint at the top of the window, one version being scaled. https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:107: web_contents_parent_(NULL), On 2013/09/05 21:12:57, sky wrote: > Make sure you initialize everything (looks like you're missing some, including > download_layer_...) Done. https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:120: DeactivateEffect(); On 2013/09/05 21:12:57, sky wrote: > Did you verify you don't crash if you end up here? I ask as I suspect you do, > but I'm not positive. In particular I think some of the views you're using have > since been destroyed by the time you get here. It does crash, removed the code, since I don't think it is needed. https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:124: if (delegate_ == NULL) On 2013/09/05 21:12:57, sky wrote: > You never delegate_ to NULL, so why this check? Done. https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:130: frame_layer_ = delegate_->GetBrowserFrameLayer(); On 2013/09/05 21:12:57, sky wrote: > Seems like you should only set frame_layer_ if you're going to activate. Done. https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:186: CHECK(non_client_layer_); On 2013/09/05 21:12:57, sky wrote: > DCHECK on these Done. https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:195: devtools_bounds_ = devtools_layer_->bounds(); On 2013/09/05 21:12:57, sky wrote: > How do you know these bounds are going to be right at the time you're done The size of the devtools area should not change during the effect since anything like typing or moving another input device will cause overscroll to end and by association the effect. I am not aware of a mechanism to change the devtools size without user intervention. If you are thinking of a specific case please let me know. https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:198: // Adjust the toplogy of the layer tree to add clipping layers On 2013/09/05 21:12:57, sky wrote: > toplogy->hierarchy Done. https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:207: web_clipping_layer_->Add(web_contents_layer_); On 2013/09/05 21:12:57, sky wrote: > This all strikes me as very fragile. In particular BrowserView and related > classes were to add/remove a view then all this is going to break. > > It feels wrong that you have to mess with the layer tree to this degree to get > the effect you're after. Seems safer it things could be set up initially to > match what you're after so you don't have to shuffle things around. Maybe with > extra views in the hierarchy that you toggle layered painting on/off. I agree this is relatively brittle. If I recall correctly when we attempted something like that I encountered cases (creating new tabs comes to mind immediately) where other pieces of code made assumptions about the hierarchy and things broke/crashed. The fact that the user cannot effectively interact with the browser while overscrolling led to the idea of just making the changes that are needed while the effect is active. If you think this approach is too brittle, then I will abandon this CL. https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:245: ui::Layer* frame = delegate_->GetBrowserFrameLayer(); On 2013/09/05 21:12:57, sky wrote: > Isn't this frame_layer_? Done. https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:340: return !download_view_ ? 0 : On 2013/09/05 21:12:57, sky wrote: > nit: > > return download_view_ && download_view_->visible() ? > download_view_->bounds().height() : 0 > > is a lot easier to read. Done. https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:345: gfx::Transform ScrollEndEffectControllerAsh::CounterTransfromAboutPoint(int x, On 2013/09/05 21:12:57, sky wrote: > transform and const Done. https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/scroll_end_effect_controller_delegate.h (right): https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_delegate.h:37: }; On 2013/09/05 21:12:57, sky wrote: > porotected virtual destructor. Done. https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_delegate.h:38: #endif // CHROME_BROWSER_UI_VIEWS_FRAME_SCROLL_END_EFFECT_CONTROLLER_DELEGATE_H_ On 2013/09/05 21:12:57, sky wrote: > nit: newline between 37/38. Done.
https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc (right): https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:207: web_clipping_layer_->Add(web_contents_layer_); On 2013/09/09 20:01:17, rharrison wrote: > On 2013/09/05 21:12:57, sky wrote: > > This all strikes me as very fragile. In particular BrowserView and related > > classes were to add/remove a view then all this is going to break. > > > > It feels wrong that you have to mess with the layer tree to this degree to get > > the effect you're after. Seems safer it things could be set up initially to > > match what you're after so you don't have to shuffle things around. Maybe with > > extra views in the hierarchy that you toggle layered painting on/off. > > I agree this is relatively brittle. If I recall correctly when we attempted > something like that I encountered cases (creating new tabs comes to mind > immediately) where other pieces of code made assumptions about the hierarchy and > things broke/crashed. The fact that the user cannot effectively interact with > the browser while overscrolling led to the idea of just making the changes that > are needed while the effect is active. > > If you think this approach is too brittle, then I will abandon this CL. I don't think you need to abandon this CL entirely. I think you should set up the view hierarchy at construction of BrowserView time in such a way that you don't need to shuffle around the layer tree. In particular add extra views that are always there and exist solely so you can turn on clipping while doing this animation. The immersive mode mode went a similar way and it made for less brittle code. Alternatively perhaps you could adjust the clip on the actual view layers.
On 2013/09/11 00:13:39, sky wrote: > https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... > File chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc (right): > > https://codereview.chromium.org/22265009/diff/82008/chrome/browser/ui/views/f... > chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:207: > web_clipping_layer_->Add(web_contents_layer_); > On 2013/09/09 20:01:17, rharrison wrote: > > On 2013/09/05 21:12:57, sky wrote: > > > This all strikes me as very fragile. In particular BrowserView and related > > > classes were to add/remove a view then all this is going to break. > > > > > > It feels wrong that you have to mess with the layer tree to this degree to > get > > > the effect you're after. Seems safer it things could be set up initially to > > > match what you're after so you don't have to shuffle things around. Maybe > with > > > extra views in the hierarchy that you toggle layered painting on/off. > > > > I agree this is relatively brittle. If I recall correctly when we attempted > > something like that I encountered cases (creating new tabs comes to mind > > immediately) where other pieces of code made assumptions about the hierarchy > and > > things broke/crashed. The fact that the user cannot effectively interact with > > the browser while overscrolling led to the idea of just making the changes > that > > are needed while the effect is active. > > > > If you think this approach is too brittle, then I will abandon this CL. > > I don't think you need to abandon this CL entirely. I think you should set up > the view hierarchy at construction of BrowserView time in such a way that you > don't need to shuffle around the layer tree. In particular add extra views that > are always there and exist solely so you can turn on clipping while doing this > animation. The immersive mode mode went a similar way and it made for less > brittle code. > > Alternatively perhaps you could adjust the clip on the actual view layers. After pondering on your comments, I think I should be able to drop the top level clip, since all it actually is clipping is the web contents and I am doing another clip on it. I am going to look at rewritting this CL so it doesn't need to fiddle with the hierarchy. I will upload another version later today hopefully.
PTAL, major rewrite of the CL. Ended of having troubles implementing the suggested paths. Took a step back and looked at how to implement this CL on a high level. Decided to rewrite the CL to use borders manipulation instead of layer transforms. This produces much simpler code, fixes a few minor graphical glitches, and leaves a clear path forward to implementing the full effect. This doesn't completely eliminate the need for effect time manipulation, but now it is reduced to implementing just inserting a single clipping layer between the web contents and its parent. I cannot set up this relationship in the constructor since at that time the web contents is not hooked into the larger layer hierarchy. I attempted to just set it once and leave it, but that caused a crash when closing the browser, so added removing the layer at the end of effect.
Could you insert a View whose layer you turn on when necessary and enable the clip? -Scott On Mon, Sep 16, 2013 at 2:05 PM, <rharrison@chromium.org> wrote: > PTAL, major rewrite of the CL. > > Ended of having troubles implementing the suggested paths. Took a step back > and > looked at how to implement this CL on a high level. Decided to rewrite the > CL to > use borders manipulation instead of layer transforms. This produces much > simpler > code, fixes a few minor graphical glitches, and leaves a clear path forward > to > implementing the full effect. > > This doesn't completely eliminate the need for effect time manipulation, but > now > it is reduced to implementing just inserting a single clipping layer between > the > web contents and its parent. I cannot set up this relationship in the > constructor since at that time the web contents is not hooked into the > larger > layer hierarchy. I attempted to just set it once and leave it, but that > caused a > crash when closing the browser, so added removing the layer at the end of > effect. > > https://codereview.chromium.org/22265009/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
And the view would always be there. -Scott On Mon, Sep 16, 2013 at 4:24 PM, Scott Violet <sky@chromium.org> wrote: > Could you insert a View whose layer you turn on when necessary and > enable the clip? > > -Scott > > On Mon, Sep 16, 2013 at 2:05 PM, <rharrison@chromium.org> wrote: >> PTAL, major rewrite of the CL. >> >> Ended of having troubles implementing the suggested paths. Took a step back >> and >> looked at how to implement this CL on a high level. Decided to rewrite the >> CL to >> use borders manipulation instead of layer transforms. This produces much >> simpler >> code, fixes a few minor graphical glitches, and leaves a clear path forward >> to >> implementing the full effect. >> >> This doesn't completely eliminate the need for effect time manipulation, but >> now >> it is reduced to implementing just inserting a single clipping layer between >> the >> web contents and its parent. I cannot set up this relationship in the >> constructor since at that time the web contents is not hooked into the >> larger >> layer hierarchy. I attempted to just set it once and leave it, but that >> caused a >> crash when closing the browser, so added removing the layer at the end of >> effect. >> >> https://codereview.chromium.org/22265009/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/09/16 23:25:15, sky wrote: > And the view would always be there. > > -Scott > > On Mon, Sep 16, 2013 at 4:24 PM, Scott Violet <mailto:sky@chromium.org> wrote: > > Could you insert a View whose layer you turn on when necessary and > > enable the clip? > > > > -Scott > > > > On Mon, Sep 16, 2013 at 2:05 PM, <mailto:rharrison@chromium.org> wrote: > >> PTAL, major rewrite of the CL. > >> > >> Ended of having troubles implementing the suggested paths. Took a step back > >> and > >> looked at how to implement this CL on a high level. Decided to rewrite the > >> CL to > >> use borders manipulation instead of layer transforms. This produces much > >> simpler > >> code, fixes a few minor graphical glitches, and leaves a clear path forward > >> to > >> implementing the full effect. > >> > >> This doesn't completely eliminate the need for effect time manipulation, but > >> now > >> it is reduced to implementing just inserting a single clipping layer between > >> the > >> web contents and its parent. I cannot set up this relationship in the > >> constructor since at that time the web contents is not hooked into the > >> larger > >> layer hierarchy. I attempted to just set it once and leave it, but that > >> caused a > >> crash when closing the browser, so added removing the layer at the end of > >> effect. > >> > >> https://codereview.chromium.org/22265009/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. The animation code lgtm. My only concern is the performance impact of animating bounds since it invalidates every frame. Ryan showed me the patch in action on a pixel and it looked ok; maybe not 60FPS, but it was pretty nice. Not sure how this would feel on other devices. Scott, do we have other UI effects that animate bounds?
On 2013/09/17 12:40:45, Ian Vollick wrote: > On 2013/09/16 23:25:15, sky wrote: > > And the view would always be there. > > > > -Scott > > > > On Mon, Sep 16, 2013 at 4:24 PM, Scott Violet <mailto:sky@chromium.org> wrote: > > > Could you insert a View whose layer you turn on when necessary and > > > enable the clip? > > > > > > -Scott > > > > > > On Mon, Sep 16, 2013 at 2:05 PM, <mailto:rharrison@chromium.org> wrote: > > >> PTAL, major rewrite of the CL. > > >> > > >> Ended of having troubles implementing the suggested paths. Took a step back > > >> and > > >> looked at how to implement this CL on a high level. Decided to rewrite the > > >> CL to > > >> use borders manipulation instead of layer transforms. This produces much > > >> simpler > > >> code, fixes a few minor graphical glitches, and leaves a clear path forward > > >> to > > >> implementing the full effect. > > >> > > >> This doesn't completely eliminate the need for effect time manipulation, > but > > >> now > > >> it is reduced to implementing just inserting a single clipping layer > between > > >> the > > >> web contents and its parent. I cannot set up this relationship in the > > >> constructor since at that time the web contents is not hooked into the > > >> larger > > >> layer hierarchy. I attempted to just set it once and leave it, but that > > >> caused a > > >> crash when closing the browser, so added removing the layer at the end of > > >> effect. > > >> > > >> https://codereview.chromium.org/22265009/ > > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > The animation code lgtm. My only concern is the performance impact of animating > bounds since it invalidates every frame. Ryan showed me the patch in action on a > pixel and it looked ok; maybe not 60FPS, but it was pretty nice. Not sure how > this would feel on other devices. Scott, do we have other UI effects that > animate bounds? If I recall correctly, the shelf and the immersive mode top container are animated using bounds manipulation. fyi, I intend to write tests for this code, but I wanted people to get a chance to look at the new design while I work on them.
Generally we try to avoid animating the bounds of Windows that can be big. For example, the maximize animation is a cross fade between the start state and the end state so that the window itself is not animating. -Scott On Tue, Sep 17, 2013 at 5:40 AM, <vollick@chromium.org> wrote: > On 2013/09/16 23:25:15, sky wrote: >> >> And the view would always be there. > > >> -Scott > > >> On Mon, Sep 16, 2013 at 4:24 PM, Scott Violet <mailto:sky@chromium.org> >> wrote: >> > Could you insert a View whose layer you turn on when necessary and >> > enable the clip? >> > >> > -Scott >> > >> > On Mon, Sep 16, 2013 at 2:05 PM, <mailto:rharrison@chromium.org> wrote: >> >> PTAL, major rewrite of the CL. >> >> >> >> Ended of having troubles implementing the suggested paths. Took a step >> >> back >> >> and >> >> looked at how to implement this CL on a high level. Decided to rewrite >> >> the >> >> CL to >> >> use borders manipulation instead of layer transforms. This produces >> >> much >> >> simpler >> >> code, fixes a few minor graphical glitches, and leaves a clear path >> >> forward >> >> to >> >> implementing the full effect. >> >> >> >> This doesn't completely eliminate the need for effect time >> >> manipulation, > > but >> >> >> now >> >> it is reduced to implementing just inserting a single clipping layer > > between >> >> >> the >> >> web contents and its parent. I cannot set up this relationship in the >> >> constructor since at that time the web contents is not hooked into the >> >> larger >> >> layer hierarchy. I attempted to just set it once and leave it, but that >> >> caused a >> >> crash when closing the browser, so added removing the layer at the end >> >> of >> >> effect. >> >> >> >> https://codereview.chromium.org/22265009/ > > >> To unsubscribe from this group and stop receiving emails from it, send an > > email >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > The animation code lgtm. My only concern is the performance impact of > animating > bounds since it invalidates every frame. Ryan showed me the patch in action > on a > pixel and it looked ok; maybe not 60FPS, but it was pretty nice. Not sure > how > this would feel on other devices. Scott, do we have other UI effects that > animate bounds? > > https://codereview.chromium.org/22265009/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Looking at the maximize animation code, my understanding is that it is fading between a fixed start and a fixed end. For the overscroll effect, since it is supposed to be sticky to the user's scrolling/finger, the end isn't fixed, so I think we would have to do a series of cross fades. I am not sure if this gains us much, since currently I am just doing a re-layout per update, except for the end of the effect which animates back to the 0 state. Thus I think the number of re-layouts would be the same during the effect. There is a gain to be had for the end of the effect, but I am not sure how great it would be. An alternate would be to not have the effect be sticky, so once overscroll begins we can just go tot he end state. I really don't know if this would create an acceptable effect and would have to talk with the UX team about it, if we wanted to go that way. Do you think there is a way for the animating bounds version of the this CL could be made acceptable landed? Possibly using the existing code to tracking the user's finger and having the return to 0 animation done as a cross fade? -Ryan Harrison On 2013/09/17 16:00:18, sky wrote: > Generally we try to avoid animating the bounds of Windows that can be > big. For example, the maximize animation is a cross fade between the > start state and the end state so that the window itself is not > animating. > > -Scott > > On Tue, Sep 17, 2013 at 5:40 AM, <mailto:vollick@chromium.org> wrote: > > On 2013/09/16 23:25:15, sky wrote: > >> > >> And the view would always be there. > > > > > >> -Scott > > > > > >> On Mon, Sep 16, 2013 at 4:24 PM, Scott Violet <mailto:sky@chromium.org> > >> wrote: > >> > Could you insert a View whose layer you turn on when necessary and > >> > enable the clip? > >> > > >> > -Scott > >> > > >> > On Mon, Sep 16, 2013 at 2:05 PM, <mailto:rharrison@chromium.org> wrote: > >> >> PTAL, major rewrite of the CL. > >> >> > >> >> Ended of having troubles implementing the suggested paths. Took a step > >> >> back > >> >> and > >> >> looked at how to implement this CL on a high level. Decided to rewrite > >> >> the > >> >> CL to > >> >> use borders manipulation instead of layer transforms. This produces > >> >> much > >> >> simpler > >> >> code, fixes a few minor graphical glitches, and leaves a clear path > >> >> forward > >> >> to > >> >> implementing the full effect. > >> >> > >> >> This doesn't completely eliminate the need for effect time > >> >> manipulation, > > > > but > >> > >> >> now > >> >> it is reduced to implementing just inserting a single clipping layer > > > > between > >> > >> >> the > >> >> web contents and its parent. I cannot set up this relationship in the > >> >> constructor since at that time the web contents is not hooked into the > >> >> larger > >> >> layer hierarchy. I attempted to just set it once and leave it, but that > >> >> caused a > >> >> crash when closing the browser, so added removing the layer at the end > >> >> of > >> >> effect. > >> >> > >> >> https://codereview.chromium.org/22265009/ > > > > > >> To unsubscribe from this group and stop receiving emails from it, send an > > > > email > >> > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > The animation code lgtm. My only concern is the performance impact of > > animating > > bounds since it invalidates every frame. Ryan showed me the patch in action > > on a > > pixel and it looked ok; maybe not 60FPS, but it was pretty nice. Not sure > > how > > this would feel on other devices. Scott, do we have other UI effects that > > animate bounds? > > > > https://codereview.chromium.org/22265009/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On Wed, Sep 18, 2013 at 9:25 AM, <rharrison@chromium.org> wrote: > Looking at the maximize animation code, my understanding is that it is > fading > between a fixed start and a fixed end. I used the maximize animation as an example of a case where ideally we would layout at each step, but doing so is ridiculously slow. Slow enough that we were forced to animate between the start and end states. I wasn't implying you implement your animation as a cross fade, that doesn't make sense. For the animation you are after the key is to avoid resizing the content. Doing so should allow you to get a suitable frame rate. It turns out we have some code that is intended to allow for a resize of a NativeViewHost without resizing the content. See NativeViewHost::set_fast_resize. We use this when the visibility of the bookmark bar is toggled. Unfortunately it isn't implemented on the aura side. It shouldn't be that hard to implement though. > For the overscroll effect, since it > is > supposed to be sticky to the user's scrolling/finger, the end isn't fixed, > so I > think we would have to do a series of cross fades. I am not sure if this > gains > us much, since currently I am just doing a re-layout per update, except for > the > end of the effect which animates back to the 0 state. Thus I think the > number of > re-layouts would be the same during the effect. There is a gain to be had > for > the end of the effect, but I am not sure how great it would be. > > An alternate would be to not have the effect be sticky, so once overscroll > begins we can just go tot he end state. I really don't know if this would > create > an acceptable effect and would have to talk with the UX team about it, if we > wanted to go that way. > > Do you think there is a way for the animating bounds version of the this CL > could be made acceptable landed? Possibly using the existing code to > tracking > the user's finger and having the return to 0 animation done as a cross fade? What frame rate are you getting on slower machines? -Scott > -Ryan Harrison > > > On 2013/09/17 16:00:18, sky wrote: >> >> Generally we try to avoid animating the bounds of Windows that can be >> big. For example, the maximize animation is a cross fade between the >> start state and the end state so that the window itself is not >> animating. > > >> -Scott > > >> On Tue, Sep 17, 2013 at 5:40 AM, <mailto:vollick@chromium.org> wrote: >> > On 2013/09/16 23:25:15, sky wrote: >> >> >> >> And the view would always be there. >> > >> > >> >> -Scott >> > >> > >> >> On Mon, Sep 16, 2013 at 4:24 PM, Scott Violet <mailto:sky@chromium.org> >> >> wrote: >> >> > Could you insert a View whose layer you turn on when necessary and >> >> > enable the clip? >> >> > >> >> > -Scott >> >> > >> >> > On Mon, Sep 16, 2013 at 2:05 PM, <mailto:rharrison@chromium.org> >> >> > wrote: >> >> >> PTAL, major rewrite of the CL. >> >> >> >> >> >> Ended of having troubles implementing the suggested paths. Took a >> >> >> step >> >> >> back >> >> >> and >> >> >> looked at how to implement this CL on a high level. Decided to >> >> >> rewrite >> >> >> the >> >> >> CL to >> >> >> use borders manipulation instead of layer transforms. This produces >> >> >> much >> >> >> simpler >> >> >> code, fixes a few minor graphical glitches, and leaves a clear path >> >> >> forward >> >> >> to >> >> >> implementing the full effect. >> >> >> >> >> >> This doesn't completely eliminate the need for effect time >> >> >> manipulation, >> > >> > but >> >> >> >> >> now >> >> >> it is reduced to implementing just inserting a single clipping layer >> > >> > between >> >> >> >> >> the >> >> >> web contents and its parent. I cannot set up this relationship in >> >> >> the >> >> >> constructor since at that time the web contents is not hooked into >> >> >> the >> >> >> larger >> >> >> layer hierarchy. I attempted to just set it once and leave it, but >> >> >> that >> >> >> caused a >> >> >> crash when closing the browser, so added removing the layer at the >> >> >> end >> >> >> of >> >> >> effect. >> >> >> >> >> >> https://codereview.chromium.org/22265009/ >> > >> > >> >> To unsubscribe from this group and stop receiving emails from it, send >> >> an >> > >> > email >> >> >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > >> > >> > The animation code lgtm. My only concern is the performance impact of >> > animating >> > bounds since it invalidates every frame. Ryan showed me the patch in >> > action >> > on a >> > pixel and it looked ok; maybe not 60FPS, but it was pretty nice. Not >> > sure >> > how >> > this would feel on other devices. Scott, do we have other UI effects >> > that >> > animate bounds? >> > >> > https://codereview.chromium.org/22265009/ > > >> To unsubscribe from this group and stop receiving emails from it, send an > > email >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > https://codereview.chromium.org/22265009/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Currently my code doesn't resize the content, it just moves it to the origin, so that I can use a clipping layer to give the impression of the content sliding under other elements. I am not sure if this would be less of a performance hit, or if any change to the bounds will be the problem. Using a view permanently in the hierarchy for doing the clipping as you suggested earlier, I think I can remove moving the content. Alternatively I could use a translate transform on the layer to achieve the repositioning instead of touching the bounds. I am not sure about the performance on slower devices. I will test that this afternoon. -Ryan Harrison On 2013/09/18 17:24:05, sky wrote: > On Wed, Sep 18, 2013 at 9:25 AM, <mailto:rharrison@chromium.org> wrote: > > Looking at the maximize animation code, my understanding is that it is > > fading > > between a fixed start and a fixed end. > > I used the maximize animation as an example of a case where ideally we > would layout at each step, but doing so is ridiculously slow. Slow > enough that we were forced to animate between the start and end > states. I wasn't implying you implement your animation as a cross > fade, that doesn't make sense. > > For the animation you are after the key is to avoid resizing the > content. Doing so should allow you to get a suitable frame rate. It > turns out we have some code that is intended to allow for a resize of > a NativeViewHost without resizing the content. See > NativeViewHost::set_fast_resize. We use this when the visibility of > the bookmark bar is toggled. Unfortunately it isn't implemented on the > aura side. It shouldn't be that hard to implement though. > > > For the overscroll effect, since it > > is > > supposed to be sticky to the user's scrolling/finger, the end isn't fixed, > > so I > > think we would have to do a series of cross fades. I am not sure if this > > gains > > us much, since currently I am just doing a re-layout per update, except for > > the > > end of the effect which animates back to the 0 state. Thus I think the > > number of > > re-layouts would be the same during the effect. There is a gain to be had > > for > > the end of the effect, but I am not sure how great it would be. > > > > An alternate would be to not have the effect be sticky, so once overscroll > > begins we can just go tot he end state. I really don't know if this would > > create > > an acceptable effect and would have to talk with the UX team about it, if we > > wanted to go that way. > > > > Do you think there is a way for the animating bounds version of the this CL > > could be made acceptable landed? Possibly using the existing code to > > tracking > > the user's finger and having the return to 0 animation done as a cross fade? > > What frame rate are you getting on slower machines? > > -Scott > > > -Ryan Harrison > > > > > > On 2013/09/17 16:00:18, sky wrote: > >> > >> Generally we try to avoid animating the bounds of Windows that can be > >> big. For example, the maximize animation is a cross fade between the > >> start state and the end state so that the window itself is not > >> animating. > > > > > >> -Scott > > > > > >> On Tue, Sep 17, 2013 at 5:40 AM, <mailto:vollick@chromium.org> wrote: > >> > On 2013/09/16 23:25:15, sky wrote: > >> >> > >> >> And the view would always be there. > >> > > >> > > >> >> -Scott > >> > > >> > > >> >> On Mon, Sep 16, 2013 at 4:24 PM, Scott Violet <mailto:sky@chromium.org> > >> >> wrote: > >> >> > Could you insert a View whose layer you turn on when necessary and > >> >> > enable the clip? > >> >> > > >> >> > -Scott > >> >> > > >> >> > On Mon, Sep 16, 2013 at 2:05 PM, <mailto:rharrison@chromium.org> > >> >> > wrote: > >> >> >> PTAL, major rewrite of the CL. > >> >> >> > >> >> >> Ended of having troubles implementing the suggested paths. Took a > >> >> >> step > >> >> >> back > >> >> >> and > >> >> >> looked at how to implement this CL on a high level. Decided to > >> >> >> rewrite > >> >> >> the > >> >> >> CL to > >> >> >> use borders manipulation instead of layer transforms. This produces > >> >> >> much > >> >> >> simpler > >> >> >> code, fixes a few minor graphical glitches, and leaves a clear path > >> >> >> forward > >> >> >> to > >> >> >> implementing the full effect. > >> >> >> > >> >> >> This doesn't completely eliminate the need for effect time > >> >> >> manipulation, > >> > > >> > but > >> >> > >> >> >> now > >> >> >> it is reduced to implementing just inserting a single clipping layer > >> > > >> > between > >> >> > >> >> >> the > >> >> >> web contents and its parent. I cannot set up this relationship in > >> >> >> the > >> >> >> constructor since at that time the web contents is not hooked into > >> >> >> the > >> >> >> larger > >> >> >> layer hierarchy. I attempted to just set it once and leave it, but > >> >> >> that > >> >> >> caused a > >> >> >> crash when closing the browser, so added removing the layer at the > >> >> >> end > >> >> >> of > >> >> >> effect. > >> >> >> > >> >> >> https://codereview.chromium.org/22265009/ > >> > > >> > > >> >> To unsubscribe from this group and stop receiving emails from it, send > >> >> an > >> > > >> > email > >> >> > >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. > >> > > >> > > >> > The animation code lgtm. My only concern is the performance impact of > >> > animating > >> > bounds since it invalidates every frame. Ryan showed me the patch in > >> > action > >> > on a > >> > pixel and it looked ok; maybe not 60FPS, but it was pretty nice. Not > >> > sure > >> > how > >> > this would feel on other devices. Scott, do we have other UI effects > >> > that > >> > animate bounds? > >> > > >> > https://codereview.chromium.org/22265009/ > > > > > >> To unsubscribe from this group and stop receiving emails from it, send an > > > > email > >> > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > > > https://codereview.chromium.org/22265009/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Just built the CL and tested it on daisy. Showed it to vollick and sadrul, and the performance looks good. From talking with sadrul my understanding is that moving the content to the origin for the effect should not induce a layout on the content and associated performance hit, like changing the size would. -Ryan Harrison On 2013/09/18 18:03:23, rharrison wrote: > Currently my code doesn't resize the content, it just moves it to the origin, so > that I can use a clipping layer to give the impression of the content sliding > under other elements. I am not sure if this would be less of a performance hit, > or if any change to the bounds will be the problem. Using a view permanently in > the hierarchy for doing the clipping as you suggested earlier, I think I can > remove moving the content. Alternatively I could use a translate transform on > the layer to achieve the repositioning instead of touching the bounds. > > I am not sure about the performance on slower devices. I will test that this > afternoon. > -Ryan Harrison > > On 2013/09/18 17:24:05, sky wrote: > > On Wed, Sep 18, 2013 at 9:25 AM, <mailto:rharrison@chromium.org> wrote: > > > Looking at the maximize animation code, my understanding is that it is > > > fading > > > between a fixed start and a fixed end. > > > > I used the maximize animation as an example of a case where ideally we > > would layout at each step, but doing so is ridiculously slow. Slow > > enough that we were forced to animate between the start and end > > states. I wasn't implying you implement your animation as a cross > > fade, that doesn't make sense. > > > > For the animation you are after the key is to avoid resizing the > > content. Doing so should allow you to get a suitable frame rate. It > > turns out we have some code that is intended to allow for a resize of > > a NativeViewHost without resizing the content. See > > NativeViewHost::set_fast_resize. We use this when the visibility of > > the bookmark bar is toggled. Unfortunately it isn't implemented on the > > aura side. It shouldn't be that hard to implement though. > > > > > For the overscroll effect, since it > > > is > > > supposed to be sticky to the user's scrolling/finger, the end isn't fixed, > > > so I > > > think we would have to do a series of cross fades. I am not sure if this > > > gains > > > us much, since currently I am just doing a re-layout per update, except for > > > the > > > end of the effect which animates back to the 0 state. Thus I think the > > > number of > > > re-layouts would be the same during the effect. There is a gain to be had > > > for > > > the end of the effect, but I am not sure how great it would be. > > > > > > An alternate would be to not have the effect be sticky, so once overscroll > > > begins we can just go tot he end state. I really don't know if this would > > > create > > > an acceptable effect and would have to talk with the UX team about it, if we > > > wanted to go that way. > > > > > > Do you think there is a way for the animating bounds version of the this CL > > > could be made acceptable landed? Possibly using the existing code to > > > tracking > > > the user's finger and having the return to 0 animation done as a cross fade? > > > > What frame rate are you getting on slower machines? > > > > -Scott > > > > > -Ryan Harrison > > > > > > > > > On 2013/09/17 16:00:18, sky wrote: > > >> > > >> Generally we try to avoid animating the bounds of Windows that can be > > >> big. For example, the maximize animation is a cross fade between the > > >> start state and the end state so that the window itself is not > > >> animating. > > > > > > > > >> -Scott > > > > > > > > >> On Tue, Sep 17, 2013 at 5:40 AM, <mailto:vollick@chromium.org> wrote: > > >> > On 2013/09/16 23:25:15, sky wrote: > > >> >> > > >> >> And the view would always be there. > > >> > > > >> > > > >> >> -Scott > > >> > > > >> > > > >> >> On Mon, Sep 16, 2013 at 4:24 PM, Scott Violet <mailto:sky@chromium.org> > > >> >> wrote: > > >> >> > Could you insert a View whose layer you turn on when necessary and > > >> >> > enable the clip? > > >> >> > > > >> >> > -Scott > > >> >> > > > >> >> > On Mon, Sep 16, 2013 at 2:05 PM, <mailto:rharrison@chromium.org> > > >> >> > wrote: > > >> >> >> PTAL, major rewrite of the CL. > > >> >> >> > > >> >> >> Ended of having troubles implementing the suggested paths. Took a > > >> >> >> step > > >> >> >> back > > >> >> >> and > > >> >> >> looked at how to implement this CL on a high level. Decided to > > >> >> >> rewrite > > >> >> >> the > > >> >> >> CL to > > >> >> >> use borders manipulation instead of layer transforms. This produces > > >> >> >> much > > >> >> >> simpler > > >> >> >> code, fixes a few minor graphical glitches, and leaves a clear path > > >> >> >> forward > > >> >> >> to > > >> >> >> implementing the full effect. > > >> >> >> > > >> >> >> This doesn't completely eliminate the need for effect time > > >> >> >> manipulation, > > >> > > > >> > but > > >> >> > > >> >> >> now > > >> >> >> it is reduced to implementing just inserting a single clipping layer > > >> > > > >> > between > > >> >> > > >> >> >> the > > >> >> >> web contents and its parent. I cannot set up this relationship in > > >> >> >> the > > >> >> >> constructor since at that time the web contents is not hooked into > > >> >> >> the > > >> >> >> larger > > >> >> >> layer hierarchy. I attempted to just set it once and leave it, but > > >> >> >> that > > >> >> >> caused a > > >> >> >> crash when closing the browser, so added removing the layer at the > > >> >> >> end > > >> >> >> of > > >> >> >> effect. > > >> >> >> > > >> >> >> https://codereview.chromium.org/22265009/ > > >> > > > >> > > > >> >> To unsubscribe from this group and stop receiving emails from it, send > > >> >> an > > >> > > > >> > email > > >> >> > > >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > >> > > > >> > > > >> > The animation code lgtm. My only concern is the performance impact of > > >> > animating > > >> > bounds since it invalidates every frame. Ryan showed me the patch in > > >> > action > > >> > on a > > >> > pixel and it looked ok; maybe not 60FPS, but it was pretty nice. Not > > >> > sure > > >> > how > > >> > this would feel on other devices. Scott, do we have other UI effects > > >> > that > > >> > animate bounds? > > >> > > > >> > https://codereview.chromium.org/22265009/ > > > > > > > > >> To unsubscribe from this group and stop receiving emails from it, send an > > > > > > email > > >> > > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > > > > > > > https://codereview.chromium.org/22265009/ > > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org.
Can you use the fast_resize code path of NativeViewHost to avoid having to roll your own? -Scott On Wed, Sep 18, 2013 at 1:17 PM, <rharrison@chromium.org> wrote: > Just built the CL and tested it on daisy. Showed it to vollick and sadrul, > and > the performance looks good. > > From talking with sadrul my understanding is that moving the content to the > origin for the effect should not induce a layout on the content and > associated > performance hit, like changing the size would. > -Ryan Harrison > > > On 2013/09/18 18:03:23, rharrison wrote: >> >> Currently my code doesn't resize the content, it just moves it to the >> origin, > > so >> >> that I can use a clipping layer to give the impression of the content >> sliding >> under other elements. I am not sure if this would be less of a performance > > hit, >> >> or if any change to the bounds will be the problem. Using a view >> permanently > > in >> >> the hierarchy for doing the clipping as you suggested earlier, I think I >> can >> remove moving the content. Alternatively I could use a translate transform >> on >> the layer to achieve the repositioning instead of touching the bounds. > > >> I am not sure about the performance on slower devices. I will test that >> this >> afternoon. >> -Ryan Harrison > > >> On 2013/09/18 17:24:05, sky wrote: >> > On Wed, Sep 18, 2013 at 9:25 AM, <mailto:rharrison@chromium.org> wrote: >> > > Looking at the maximize animation code, my understanding is that it is >> > > fading >> > > between a fixed start and a fixed end. >> > >> > I used the maximize animation as an example of a case where ideally we >> > would layout at each step, but doing so is ridiculously slow. Slow >> > enough that we were forced to animate between the start and end >> > states. I wasn't implying you implement your animation as a cross >> > fade, that doesn't make sense. >> > >> > For the animation you are after the key is to avoid resizing the >> > content. Doing so should allow you to get a suitable frame rate. It >> > turns out we have some code that is intended to allow for a resize of >> > a NativeViewHost without resizing the content. See >> > NativeViewHost::set_fast_resize. We use this when the visibility of >> > the bookmark bar is toggled. Unfortunately it isn't implemented on the >> > aura side. It shouldn't be that hard to implement though. >> > >> > > For the overscroll effect, since it >> > > is >> > > supposed to be sticky to the user's scrolling/finger, the end isn't >> > > fixed, >> > > so I >> > > think we would have to do a series of cross fades. I am not sure if >> > > this >> > > gains >> > > us much, since currently I am just doing a re-layout per update, >> > > except > > for >> >> > > the >> > > end of the effect which animates back to the 0 state. Thus I think the >> > > number of >> > > re-layouts would be the same during the effect. There is a gain to be >> > > had >> > > for >> > > the end of the effect, but I am not sure how great it would be. >> > > >> > > An alternate would be to not have the effect be sticky, so once >> > > overscroll >> > > begins we can just go tot he end state. I really don't know if this >> > > would >> > > create >> > > an acceptable effect and would have to talk with the UX team about it, >> > > if > > we >> >> > > wanted to go that way. >> > > >> > > Do you think there is a way for the animating bounds version of the >> > > this > > CL >> >> > > could be made acceptable landed? Possibly using the existing code to >> > > tracking >> > > the user's finger and having the return to 0 animation done as a cross > > fade? >> >> > >> > What frame rate are you getting on slower machines? >> > >> > -Scott >> > >> > > -Ryan Harrison >> > > >> > > >> > > On 2013/09/17 16:00:18, sky wrote: >> > >> >> > >> Generally we try to avoid animating the bounds of Windows that can be >> > >> big. For example, the maximize animation is a cross fade between the >> > >> start state and the end state so that the window itself is not >> > >> animating. >> > > >> > > >> > >> -Scott >> > > >> > > >> > >> On Tue, Sep 17, 2013 at 5:40 AM, <mailto:vollick@chromium.org> >> > >> wrote: >> > >> > On 2013/09/16 23:25:15, sky wrote: >> > >> >> >> > >> >> And the view would always be there. >> > >> > >> > >> > >> > >> >> -Scott >> > >> > >> > >> > >> > >> >> On Mon, Sep 16, 2013 at 4:24 PM, Scott Violet > > <mailto:sky@chromium.org> >> >> > >> >> wrote: >> > >> >> > Could you insert a View whose layer you turn on when necessary >> > >> >> > and >> > >> >> > enable the clip? >> > >> >> > >> > >> >> > -Scott >> > >> >> > >> > >> >> > On Mon, Sep 16, 2013 at 2:05 PM, >> > >> >> > <mailto:rharrison@chromium.org> >> > >> >> > wrote: >> > >> >> >> PTAL, major rewrite of the CL. >> > >> >> >> >> > >> >> >> Ended of having troubles implementing the suggested paths. Took >> > >> >> >> a >> > >> >> >> step >> > >> >> >> back >> > >> >> >> and >> > >> >> >> looked at how to implement this CL on a high level. Decided to >> > >> >> >> rewrite >> > >> >> >> the >> > >> >> >> CL to >> > >> >> >> use borders manipulation instead of layer transforms. This >> > >> >> >> produces >> > >> >> >> much >> > >> >> >> simpler >> > >> >> >> code, fixes a few minor graphical glitches, and leaves a clear >> > >> >> >> path >> > >> >> >> forward >> > >> >> >> to >> > >> >> >> implementing the full effect. >> > >> >> >> >> > >> >> >> This doesn't completely eliminate the need for effect time >> > >> >> >> manipulation, >> > >> > >> > >> > but >> > >> >> >> > >> >> >> now >> > >> >> >> it is reduced to implementing just inserting a single clipping > > layer >> >> > >> > >> > >> > between >> > >> >> >> > >> >> >> the >> > >> >> >> web contents and its parent. I cannot set up this relationship >> > >> >> >> in >> > >> >> >> the >> > >> >> >> constructor since at that time the web contents is not hooked >> > >> >> >> into >> > >> >> >> the >> > >> >> >> larger >> > >> >> >> layer hierarchy. I attempted to just set it once and leave it, >> > >> >> >> but >> > >> >> >> that >> > >> >> >> caused a >> > >> >> >> crash when closing the browser, so added removing the layer at >> > >> >> >> the >> > >> >> >> end >> > >> >> >> of >> > >> >> >> effect. >> > >> >> >> >> > >> >> >> https://codereview.chromium.org/22265009/ >> > >> > >> > >> > >> > >> >> To unsubscribe from this group and stop receiving emails from it, >> > >> >> send >> > >> >> an >> > >> > >> > >> > email >> > >> >> >> > >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > >> > >> > >> > >> > >> > The animation code lgtm. My only concern is the performance impact >> > >> > of >> > >> > animating >> > >> > bounds since it invalidates every frame. Ryan showed me the patch >> > >> > in >> > >> > action >> > >> > on a >> > >> > pixel and it looked ok; maybe not 60FPS, but it was pretty nice. >> > >> > Not >> > >> > sure >> > >> > how >> > >> > this would feel on other devices. Scott, do we have other UI >> > >> > effects >> > >> > that >> > >> > animate bounds? >> > >> > >> > >> > https://codereview.chromium.org/22265009/ >> > > >> > > >> > >> To unsubscribe from this group and stop receiving emails from it, >> > >> send an >> > > >> > > email >> > >> >> > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > >> > > >> > > >> > > https://codereview.chromium.org/22265009/ >> > >> > To unsubscribe from this group and stop receiving emails from it, send >> > an >> email >> > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > https://codereview.chromium.org/22265009/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I have implemented a version of this that uses the NativeViewHost fast_resize code path to implement the clipping. I have implemented NativeViewHostAura::InstallClip and related functions following the basic design provided in the Win version. Once I have the entire effect working I intend to break out the InstallClip implementation into its own CL and land it separately, but for ease of reference I have included it here. Unfortunately, there is still one portion of the effect missing though. To achieve what we desire in the effect I need to be able to affect the gravity/alignment of the content relative to the clipping layer. Specifically the current behaviour is to have the clipping layer line up with the top left corner of the content layer, so that the right and bottom will be clipped. When scrolling up we want the top of the content to be clipped. Talking with sadrul there is a couple of ideas that might work here. For a very low road approach I could just have the ContentsContainer apply a translate transform to the content layer. This feels hackish and doesn't offer a general solution for other developers. Sadrul pointed out that x11 and gtk implement a concept of gravity in situations like this, which I think may be a good general solution, except I don't think we currently implement it any way. sky, do you have thoughts on how to resolve this? Do you think adding something like a gravity parameter that affects which side gets clipped on the fast_resize path and default it to the current behaviour is the best way forward or do you have an alternate suggestion?
On 2013/09/23 20:43:55, rharrison wrote: > I have implemented a version of this that uses the NativeViewHost fast_resize > code path to implement the clipping. I have implemented > NativeViewHostAura::InstallClip and related functions following the basic design > provided in the Win version. Once I have the entire effect working I intend to > break out the InstallClip implementation into its own CL and land it separately, > but for ease of reference I have included it here. Unfortunately, there is still > one portion of the effect missing though. > > To achieve what we desire in the effect I need to be able to affect the > gravity/alignment of the content relative to the clipping layer. Specifically > the current behaviour is to have the clipping layer line up with the top left > corner of the content layer, so that the right and bottom will be clipped. When > scrolling up we want the top of the content to be clipped. > > Talking with sadrul there is a couple of ideas that might work here. For a very > low road approach I could just have the ContentsContainer apply a translate > transform to the content layer. This feels hackish and doesn't offer a general > solution for other developers. Sadrul pointed out that x11 and gtk implement a > concept of gravity in situations like this, which I think may be a good general > solution, except I don't think we currently implement it any way. > > sky, do you have thoughts on how to resolve this? Do you think adding something > like a gravity parameter that affects which side gets clipped on the fast_resize > path and default it to the current behaviour is the best way forward or do you > have an alternate suggestion? Talking with sadrul again this morning, I think we have a good design for adding gravity, which I am going to implement. I will hopefully upload a new revision with everything in it later today, so people can see how all this hangs together.
Could you also separate out the NativeViewHostAura changes into its own patch and send that out now? Smaller patches generally go quicker and are easier on everyone. -Scott On Tue, Sep 24, 2013 at 8:36 AM, <rharrison@chromium.org> wrote: > On 2013/09/23 20:43:55, rharrison wrote: >> >> I have implemented a version of this that uses the NativeViewHost >> fast_resize >> code path to implement the clipping. I have implemented >> NativeViewHostAura::InstallClip and related functions following the basic > > design >> >> provided in the Win version. Once I have the entire effect working I >> intend to >> break out the InstallClip implementation into its own CL and land it > > separately, >> >> but for ease of reference I have included it here. Unfortunately, there is > > still >> >> one portion of the effect missing though. > > >> To achieve what we desire in the effect I need to be able to affect the >> gravity/alignment of the content relative to the clipping layer. >> Specifically >> the current behaviour is to have the clipping layer line up with the top >> left >> corner of the content layer, so that the right and bottom will be clipped. > > When >> >> scrolling up we want the top of the content to be clipped. > > >> Talking with sadrul there is a couple of ideas that might work here. For a > > very >> >> low road approach I could just have the ContentsContainer apply a >> translate >> transform to the content layer. This feels hackish and doesn't offer a >> general >> solution for other developers. Sadrul pointed out that x11 and gtk >> implement a >> concept of gravity in situations like this, which I think may be a good > > general >> >> solution, except I don't think we currently implement it any way. > > >> sky, do you have thoughts on how to resolve this? Do you think adding > > something >> >> like a gravity parameter that affects which side gets clipped on the > > fast_resize >> >> path and default it to the current behaviour is the best way forward or do >> you >> have an alternate suggestion? > > > Talking with sadrul again this morning, I think we have a good design for > adding > gravity, which > I am going to implement. I will hopefully upload a new revision with > everything > in > it later today, so people can see how all this hangs together. > > https://codereview.chromium.org/22265009/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Do you want the gravity and InstallClip changes in one CL, or should I break out gravity into its own CL?
On 2013/09/24 18:06:20, rharrison wrote: > Do you want the gravity and InstallClip changes in one CL, or should I break out > gravity into its own CL? I have moved everything related to modifying NVHA into https://codereview.chromium.org/24299004/.
PTAL, Rebased onto changes to NVHA and add tests
The prerequisite CL for this is in the CQ, so PTAL.
Looks pretty good. Some nits: https://codereview.chromium.org/22265009/diff/157001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/22265009/diff/157001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/browser_view.cc:2595: // related cases have been fixed in the effect. Reference the crbug here. https://codereview.chromium.org/22265009/diff/157001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/contents_container.h (right): https://codereview.chromium.org/22265009/diff/157001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/contents_container.h:37: // contents from being updated by layouts of this class. Don't mention the clipping layer here. Make a note here that fast-resize is used for |active_web_view_| while the scroll-end effect is active. https://codereview.chromium.org/22265009/diff/157001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/contents_container.h:43: virtual void UpdateScrollEndEffectHeightDelta(int height_delta, Don't really need the comment here. (the clipping window is now an internal detail of NativeViewHostAura, and this code should not need to worry about that). https://codereview.chromium.org/22265009/diff/157001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc (right): https://codereview.chromium.org/22265009/diff/157001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:53: ActivateEffect(); no {} https://codereview.chromium.org/22265009/diff/157001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:114: scale_factor = 1 - scale_factor; float scale_factor = 1 - std::abs(current_delta_y_) / bounds.height()? https://codereview.chromium.org/22265009/diff/157001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.h (right): https://codereview.chromium.org/22265009/diff/157001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.h:61: // The size of the frame at the size of the effect, so that the effect can 'at the beginning/start of the effect'? https://codereview.chromium.org/22265009/diff/157001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/scroll_end_effect_controller_ash_unittest.cc (right): https://codereview.chromium.org/22265009/diff/157001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash_unittest.cc:51: bool last_scrolling_direction_; DISALLOW_... https://codereview.chromium.org/22265009/diff/157001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash_unittest.cc:78: gfx::Rect frame_bounds_; ditto
PTAL https://codereview.chromium.org/22265009/diff/157001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/22265009/diff/157001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/browser_view.cc:2595: // related cases have been fixed in the effect. On 2013/10/25 18:28:25, sadrul wrote: > Reference the crbug here. Done. https://codereview.chromium.org/22265009/diff/157001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/contents_container.h (right): https://codereview.chromium.org/22265009/diff/157001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/contents_container.h:37: // contents from being updated by layouts of this class. On 2013/10/25 18:28:25, sadrul wrote: > Don't mention the clipping layer here. Make a note here that fast-resize is used > for |active_web_view_| while the scroll-end effect is active. Done. https://codereview.chromium.org/22265009/diff/157001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/contents_container.h:43: virtual void UpdateScrollEndEffectHeightDelta(int height_delta, On 2013/10/25 18:28:25, sadrul wrote: > Don't really need the comment here. (the clipping window is now an internal > detail of NativeViewHostAura, and this code should not need to worry about > that). Done. https://codereview.chromium.org/22265009/diff/157001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc (right): https://codereview.chromium.org/22265009/diff/157001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:53: ActivateEffect(); On 2013/10/25 18:28:25, sadrul wrote: > no {} Done. https://codereview.chromium.org/22265009/diff/157001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.cc:114: scale_factor = 1 - scale_factor; On 2013/10/25 18:28:25, sadrul wrote: > float scale_factor = 1 - std::abs(current_delta_y_) / bounds.height()? Done. https://codereview.chromium.org/22265009/diff/157001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.h (right): https://codereview.chromium.org/22265009/diff/157001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash.h:61: // The size of the frame at the size of the effect, so that the effect can On 2013/10/25 18:28:25, sadrul wrote: > 'at the beginning/start of the effect'? Done. https://codereview.chromium.org/22265009/diff/157001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/scroll_end_effect_controller_ash_unittest.cc (right): https://codereview.chromium.org/22265009/diff/157001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash_unittest.cc:51: bool last_scrolling_direction_; On 2013/10/25 18:28:25, sadrul wrote: > DISALLOW_... Done. https://codereview.chromium.org/22265009/diff/157001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/scroll_end_effect_controller_ash_unittest.cc:78: gfx::Rect frame_bounds_; On 2013/10/25 18:28:25, sadrul wrote: > ditto Done.
sky, sadrul, do you have any other comments on this CL?
On 2013/11/26 17:00:39, rharrison wrote: > sky, sadrul, do you have any other comments on this CL? I am going to be on leave/vacation for December while I work on my thesis and for the winter holidays. In the new year I am expecting to be leaving the Chrome team, so I won't be working on this CL anymore. There are people in WAT that are going to be taking over the scroll end effect work. I am leaving this CL up for them to reference, but there will be a new version from one of them in the future. |
