Re-Reland: Implement features in NativeViewHostAura for scroll end effect

Re-Reland: Implement features in NativeViewHostAura for scroll end
effect

This CL enables the fast resize path on Aura by implementing
NativeViewHostAura::InstallClip. It also adds the concept of gravity to
the fast resize state to control the positioning of the clip relative to
the contents. Gravity is implemented for Aura only in this CL and
ignored in other implementations.

Refactored |clipping_window_| so that ownership is not in NVHA.  Added
test to make sure that the leak is resolved.  Fixed occlusion issue with
find bar by setting kHostViewKey for |clipping_window_|.

https://codereview.chromium.org/48113014/ fixes the issues with not
being able to drag apps.

https://codereview.chromium.org/54623007/ will resolve issues with drop
downs not locating correctly.

BUG=151356
BUG=282463
TEST=Confirmed the fast path and gravity works in conjunction with
     scroll end effect CL.
     Added and updated unit tests.
     Ran ash_unittests and views_unittests locally under headchecker.
     Ran ash_unittests and views_unittests locally under ASAN.

[rharrison, 2013-11-18 21:22:43]

This is a re-based version of the previous reverted CL. The bugs that caused the
revert have been resolved in other CLs.

[sadrul, 2013-11-19 05:51:29]

A couple of nits, a question. Otherwise, lgtm!

https://codereview.chromium.org/48113013/diff/20001/ui/views/controls/native/...
File ui/views/controls/native/native_view_host_aura.cc (right):

https://codereview.chromium.org/48113013/diff/20001/ui/views/controls/native/...
ui/views/controls/native/native_view_host_aura.cc:85: }
Add a DCHECK(!clipping_window_) here too.

Could this call RemoveClippingWindow() instead?

https://codereview.chromium.org/48113013/diff/20001/ui/views/controls/native/...
ui/views/controls/native/native_view_host_aura.cc:225: }
Add a DCHECK(!clipping_window_); here

[rharrison, 2013-11-19 16:51:21]

https://codereview.chromium.org/48113013/diff/20001/ui/views/controls/native/...
File ui/views/controls/native/native_view_host_aura.cc (right):

https://codereview.chromium.org/48113013/diff/20001/ui/views/controls/native/...
ui/views/controls/native/native_view_host_aura.cc:85: }
On 2013/11/19 05:51:30, sadrul wrote:
> Add a DCHECK(!clipping_window_) here too.
> 
> Could this call RemoveClippingWindow() instead?
Done.

I think it would be doing a bunch of unneeded operations, since we know the
NativeView is going/gone away, because |destroyed| is true. We really don't care
about re-parenting the NativeView or making sure the bounds are correct.

https://codereview.chromium.org/48113013/diff/20001/ui/views/controls/native/...
ui/views/controls/native/native_view_host_aura.cc:225: }
On 2013/11/19 05:51:30, sadrul wrote:
> Add a DCHECK(!clipping_window_); here

Done.

[sky, 2013-11-19 21:00:08]

https://codereview.chromium.org/48113013/diff/250001/ui/views/controls/native...
File ui/views/controls/native/native_view_host_aura.cc (left):

https://codereview.chromium.org/48113013/diff/250001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:39:
host_->native_view()->RemoveObserver(this);
nit: unless there is a compelling reason, keep the order as it was.

https://codereview.chromium.org/48113013/diff/250001/ui/views/controls/native...
File ui/views/controls/native/native_view_host_aura.cc (right):

https://codereview.chromium.org/48113013/diff/250001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:18: // NVHA's reference to the
clipping window.
Please add more description. In particular as NativeViewHostAura owns
clipping_window_ it isn't entirely obvious how the window can be destroyed out
from under NativeViewHostAura.

https://codereview.chromium.org/48113013/diff/250001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:21: explicit
ClippingWindowObserver(NativeViewHostAura* host) :
nit: ':' on next line.

https://codereview.chromium.org/48113013/diff/250001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:85: clipping_window_ = NULL;
Should this be a DCHECK?

https://codereview.chromium.org/48113013/diff/250001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:197:
clipping_window_->layer()->set_name("NativeViewHostAuraClip");
Isn't the name of the layer already updated for you in Window::SetName?

https://codereview.chromium.org/48113013/diff/250001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:208:
clipping_window_->SetProperty(views::kHostViewKey,
Shouldn't this only be set on the native_view()?

https://codereview.chromium.org/48113013/diff/250001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:214:
host_->native_view()->layer()->set_name("ClippedContent");
Same comment about name.

https://codereview.chromium.org/48113013/diff/250001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:228: clipping_window_ = NULL;
DCHECK?

https://codereview.chromium.org/48113013/diff/250001/ui/views/controls/native...
File ui/views/controls/native/native_view_host_aura.h (right):

https://codereview.chromium.org/48113013/diff/250001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.h:85:
scoped_ptr<ClippingWindowObserver> clipping_window_observer_;
AFAICT this is never NULL, as such why bother with the scoped_ptr? And note that
style guide says "Do not replace data members with pointers just to avoid an
#include."

[sky, 2013-11-19 21:00:26]

Also, did you add unit test coverage for the reasons for the revert?

[rharrison, 2013-11-20 15:50:12]

I have been able to reproduce the drop down bug that was one of the reasons that
this CL was reverted. I have reopened https://codereview.chromium.org/54623007/,
which should fix that issue. I will hold off on this CL until that is landed.

The issues that caused this CL to be reverted have been/will be resolved in
other CLs which should have related testing, so I am not sure if there is new
testing needed here.

https://codereview.chromium.org/48113013/diff/250001/ui/views/controls/native...
File ui/views/controls/native/native_view_host_aura.cc (left):

https://codereview.chromium.org/48113013/diff/250001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:39:
host_->native_view()->RemoveObserver(this);
On 2013/11/19 21:00:09, sky wrote:
> nit: unless there is a compelling reason, keep the order as it was.

Done.

https://codereview.chromium.org/48113013/diff/250001/ui/views/controls/native...
File ui/views/controls/native/native_view_host_aura.cc (right):

https://codereview.chromium.org/48113013/diff/250001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:18: // NVHA's reference to the
clipping window.
On 2013/11/19 21:00:09, sky wrote:
> Please add more description. In particular as NativeViewHostAura owns
> clipping_window_ it isn't entirely obvious how the window can be destroyed out
> from under NativeViewHostAura.

Done.

https://codereview.chromium.org/48113013/diff/250001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:21: explicit
ClippingWindowObserver(NativeViewHostAura* host) :
On 2013/11/19 21:00:09, sky wrote:
> nit: ':' on next line.

Done.

https://codereview.chromium.org/48113013/diff/250001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:85: clipping_window_ = NULL;
On 2013/11/19 21:00:09, sky wrote:
> Should this be a DCHECK?
Talked with sadrul about this, I now understand what you both were getting at. I
forgot about the observer.
Done

https://codereview.chromium.org/48113013/diff/250001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:197:
clipping_window_->layer()->set_name("NativeViewHostAuraClip");
On 2013/11/19 21:00:09, sky wrote:
> Isn't the name of the layer already updated for you in Window::SetName?

Done.

https://codereview.chromium.org/48113013/diff/250001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:208:
clipping_window_->SetProperty(views::kHostViewKey,
On 2013/11/19 21:00:09, sky wrote:
> Shouldn't this only be set on the native_view()?

Done.

https://codereview.chromium.org/48113013/diff/250001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:214:
host_->native_view()->layer()->set_name("ClippedContent");
On 2013/11/19 21:00:09, sky wrote:
> Same comment about name.

Done.

https://codereview.chromium.org/48113013/diff/250001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:228: clipping_window_ = NULL;
On 2013/11/19 21:00:09, sky wrote:
> DCHECK?

Done.

https://codereview.chromium.org/48113013/diff/250001/ui/views/controls/native...
File ui/views/controls/native/native_view_host_aura.h (right):

https://codereview.chromium.org/48113013/diff/250001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.h:85:
scoped_ptr<ClippingWindowObserver> clipping_window_observer_;
On 2013/11/19 21:00:09, sky wrote:
> AFAICT this is never NULL, as such why bother with the scoped_ptr? And note
that
> style guide says "Do not replace data members with pointers just to avoid an
> #include."

Done.

[sky, 2013-11-20 21:10:33]

https://codereview.chromium.org/48113013/diff/530001/ui/views/controls/native...
File ui/views/controls/native/native_view_host_aura.h (right):

https://codereview.chromium.org/48113013/diff/530001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.h:22: // the window hierarchy is
not owned by the NVHA, thus may be destroyed without
Actually, this begs the question why not make the clipping_window
set_owned_by_parent(false)?

[rharrison, 2013-11-21 18:38:00]

I will add jam@ for the content/ change once I have finished with the preceding
CL that has a bunch of content/ changes.

https://codereview.chromium.org/48113013/diff/530001/ui/views/controls/native...
File ui/views/controls/native/native_view_host_aura.h (right):

https://codereview.chromium.org/48113013/diff/530001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.h:22: // the window hierarchy is
not owned by the NVHA, thus may be destroyed without
On 2013/11/20 21:10:34, sky wrote:
> Actually, this begs the question why not make the clipping_window
> set_owned_by_parent(false)?

Done.

https://codereview.chromium.org/48113013/diff/580001/ui/views/controls/native...
File ui/views/controls/native/native_view_host_aura.cc (right):

https://codereview.chromium.org/48113013/diff/580001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:172:
clipping_window_->SetProperty(views::kHostViewKey,
Setting the kHostViewKey is needed to make sure that the clipping window does
not occlude the find bar.

[sky, 2013-11-21 23:15:13]

https://codereview.chromium.org/48113013/diff/580001/ui/views/controls/native...
File ui/views/controls/native/native_view_host_aura.cc (right):

https://codereview.chromium.org/48113013/diff/580001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:24: if (clipping_window_)
Don't you need to destroy clipping_window_ now?

https://codereview.chromium.org/48113013/diff/580001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:172:
clipping_window_->SetProperty(views::kHostViewKey,
On 2013/11/21 18:38:01, rharrison wrote:
> Setting the kHostViewKey is needed to make sure that the clipping window does
> not occlude the find bar.

Ah, right. In that case I don't think you need to set the property on the
native_view_ anymore, right?

[rharrison, 2013-11-25 20:46:17]

https://codereview.chromium.org/48113013/diff/580001/ui/views/controls/native...
File ui/views/controls/native/native_view_host_aura.cc (right):

https://codereview.chromium.org/48113013/diff/580001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:24: if (clipping_window_)
On 2013/11/21 23:15:14, sky wrote:
> Don't you need to destroy clipping_window_ now?

Done.

https://codereview.chromium.org/48113013/diff/580001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:172:
clipping_window_->SetProperty(views::kHostViewKey,
On 2013/11/21 23:15:14, sky wrote:
> On 2013/11/21 18:38:01, rharrison wrote:
> > Setting the kHostViewKey is needed to make sure that the clipping window
does
> > not occlude the find bar.
> 
> Ah, right. In that case I don't think you need to set the property on the
> native_view_ anymore, right?

Done.

[sky, 2013-11-25 21:47:51]

LGTM

[rharrison, 2013-11-29 15:54:54]

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.

[sky, 2013-12-02 21:35:22]

Again, -sky on this one.