Reland: Implement features in NativeViewHostAura for scroll end effect

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_|.

BUG=151356
BUG=282463
BUG=308075
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.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=231027

[rharrison, 2013-10-22 17:53:29]

PTAL, I have resolved the issues that led to this CL being reverted.

The initial version is what was originally landed for reference.

[sky, 2013-10-22 19:57:32]

https://codereview.chromium.org/30993004/diff/1/ui/views/controls/native/nati...
File ui/views/controls/native/native_view_host_aura.cc (right):

https://codereview.chromium.org/30993004/diff/1/ui/views/controls/native/nati...
ui/views/controls/native/native_view_host_aura.cc:21:
clipping_window_.SetTransparent(true);
What necessitated all these changes?

https://codereview.chromium.org/30993004/diff/40001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/find_bar_host.cc (right):

https://codereview.chromium.org/30993004/diff/40001/chrome/browser/ui/views/f...
chrome/browser/ui/views/find_bar_host.cc:43: host()->StackAtTop();
Why do we need a stack at top now? And this won't work on non-aura.

[xiyuan, 2013-10-22 20:12:46]

https://codereview.chromium.org/30993004/diff/1/ui/views/controls/native/nati...
File ui/views/controls/native/native_view_host_aura.cc (right):

https://codereview.chromium.org/30993004/diff/1/ui/views/controls/native/nati...
ui/views/controls/native/native_view_host_aura.cc:21:
clipping_window_.SetTransparent(true);
On 2013/10/22 19:57:32, sky wrote:
> What necessitated all these changes?

This is to fix the views_unittest memory leak. The leak happens when closing a
widget that has a NativeView. Because
clipping_window_.set_owned_by_parent(false), it is not destroyed when the
containing widget is closed. And when we RemoveClippingWindow(), we only do
clipping_window_.RemoveChild(host_->native_view()) because the containing widget
is destroyed (GetWidget()->GetNativeView() returns NULL). This leaves
host_->native_view() as an orphan and leaked.

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

https://codereview.chromium.org/30993004/diff/40001/ui/views/controls/native/...
ui/views/controls/native/native_view_host_aura.cc:185: native_view_bounds);
I missed this on the orignal review. We need to change the logic here because
CalculateNativeViewOrigin() should only be called on the fast resize path. This
needs to be moved out to ShowWidget() under the fast resize path.

I was trying to put up a patch https://codereview.chromium.org/27768003/. :)

[rharrison, 2013-10-22 20:34:41]

https://codereview.chromium.org/30993004/diff/40001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/find_bar_host.cc (right):

https://codereview.chromium.org/30993004/diff/40001/chrome/browser/ui/views/f...
chrome/browser/ui/views/find_bar_host.cc:43: host()->StackAtTop();
On 2013/10/22 19:57:32, sky wrote:
> Why do we need a stack at top now? And this won't work on non-aura.

The content/clipping layers are being painted on top of the find bar's widget
with my CL. Specifically the clipping/contents layer is appearing as a later
child of the browser frame layer now. I was thinking that since the find bar
cares about being on top that it should be enforcing its position explicitly. Do
you have any thoughts on how to better implement this or is the ordering of
these layers supposed to be enforced via a different mechanism and I should
investigate why it has changed?

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

https://codereview.chromium.org/30993004/diff/40001/ui/views/controls/native/...
ui/views/controls/native/native_view_host_aura.cc:185: native_view_bounds);
On 2013/10/22 20:12:46, xiyuan wrote:
> I missed this on the orignal review. We need to change the logic here because
> CalculateNativeViewOrigin() should only be called on the fast resize path.
This
> needs to be moved out to ShowWidget() under the fast resize path.
> 
> I was trying to put up a patch https://codereview.chromium.org/27768003/. :)

I have integrated your patch and tested that it works, thanks.
Done.

[xiyuan, 2013-10-22 20:43:25]

Awesome. Thanks.

LGTM from my side.

[sky, 2013-10-22 21:32:17]

https://codereview.chromium.org/30993004/diff/1/ui/views/controls/native/nati...
File ui/views/controls/native/native_view_host_aura.cc (right):

https://codereview.chromium.org/30993004/diff/1/ui/views/controls/native/nati...
ui/views/controls/native/native_view_host_aura.cc:21:
clipping_window_.SetTransparent(true);
On 2013/10/22 20:12:46, xiyuan wrote:
> On 2013/10/22 19:57:32, sky wrote:
> > What necessitated all these changes?
> 
> This is to fix the views_unittest memory leak. The leak happens when closing a
> widget that has a NativeView. Because
> clipping_window_.set_owned_by_parent(false), it is not destroyed when the
> containing widget is closed. And when we RemoveClippingWindow(), we only do
> clipping_window_.RemoveChild(host_->native_view()) because the containing
widget
> is destroyed (GetWidget()->GetNativeView() returns NULL). This leaves
> host_->native_view() as an orphan and leaked.

Can you add a unit test to verify we don't leak?

https://codereview.chromium.org/30993004/diff/40001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/find_bar_host.cc (right):

https://codereview.chromium.org/30993004/diff/40001/chrome/browser/ui/views/f...
chrome/browser/ui/views/find_bar_host.cc:43: host()->StackAtTop();
On 2013/10/22 20:34:41, rharrison wrote:
> On 2013/10/22 19:57:32, sky wrote:
> > Why do we need a stack at top now? And this won't work on non-aura.
> 
> The content/clipping layers are being painted on top of the find bar's widget
> with my CL. Specifically the clipping/contents layer is appearing as a later
> child of the browser frame layer now. I was thinking that since the find bar
> cares about being on top that it should be enforcing its position explicitly.
Do
> you have any thoughts on how to better implement this or is the ordering of
> these layers supposed to be enforced via a different mechanism and I should
> investigate why it has changed?

See kHostViewKey. I suspect you need to adjust stacking when reparenting.

[rharrison, 2013-10-24 14:40:15]

I have successfully reproduced the memory leak that caused the CL to be reverted
and confirmed this version of the CL corrects it.

https://codereview.chromium.org/30993004/diff/1/ui/views/controls/native/nati...
File ui/views/controls/native/native_view_host_aura.cc (right):

https://codereview.chromium.org/30993004/diff/1/ui/views/controls/native/nati...
ui/views/controls/native/native_view_host_aura.cc:21:
clipping_window_.SetTransparent(true);
On 2013/10/22 21:32:18, sky wrote:
> On 2013/10/22 20:12:46, xiyuan wrote:
> > On 2013/10/22 19:57:32, sky wrote:
> > > What necessitated all these changes?
> > 
> > This is to fix the views_unittest memory leak. The leak happens when closing
a
> > widget that has a NativeView. Because
> > clipping_window_.set_owned_by_parent(false), it is not destroyed when the
> > containing widget is closed. And when we RemoveClippingWindow(), we only do
> > clipping_window_.RemoveChild(host_->native_view()) because the containing
> widget
> > is destroyed (GetWidget()->GetNativeView() returns NULL). This leaves
> > host_->native_view() as an orphan and leaked.
> 
> Can you add a unit test to verify we don't leak?

Done.

https://codereview.chromium.org/30993004/diff/40001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/find_bar_host.cc (right):

https://codereview.chromium.org/30993004/diff/40001/chrome/browser/ui/views/f...
chrome/browser/ui/views/find_bar_host.cc:43: host()->StackAtTop();
On 2013/10/22 21:32:19, sky wrote:
> On 2013/10/22 20:34:41, rharrison wrote:
> > On 2013/10/22 19:57:32, sky wrote:
> > > Why do we need a stack at top now? And this won't work on non-aura.
> > 
> > The content/clipping layers are being painted on top of the find bar's
widget
> > with my CL. Specifically the clipping/contents layer is appearing as a later
> > child of the browser frame layer now. I was thinking that since the find bar
> > cares about being on top that it should be enforcing its position
explicitly.
> Do
> > you have any thoughts on how to better implement this or is the ordering of
> > these layers supposed to be enforced via a different mechanism and I should
> > investigate why it has changed?
> 
> See kHostViewKey. I suspect you need to adjust stacking when reparenting.

Done.

[sky, 2013-10-24 18:16:50]

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

https://codereview.chromium.org/30993004/diff/390001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:48: RemoveClippingWindow();
If destroyed is true, I Don't think we should be doing anything with
host_->native_view().

https://codereview.chromium.org/30993004/diff/390001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:165:
clipping_window_->AddObserver(this);
Having the same object be a listener for two different windows is typically
error prone (I have introduced bugs from this in the past). How about creating a
new object to be the listener for one of them so it is clear which observer does
what.

https://codereview.chromium.org/30993004/diff/390001/ui/views/controls/native...
File ui/views/controls/native/native_view_host_aura_unittest.cc (right):

https://codereview.chromium.org/30993004/diff/390001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura_unittest.cc:36: };
nit: DISALLOW_...

https://codereview.chromium.org/30993004/diff/390001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura_unittest.cc:57: };
nit: DISALLOW_...

[rharrison, 2013-10-24 19:28:17]

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

https://codereview.chromium.org/30993004/diff/390001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:48: RemoveClippingWindow();
On 2013/10/24 18:16:50, sky wrote:
> If destroyed is true, I Don't think we should be doing anything with
> host_->native_view().

Done.

https://codereview.chromium.org/30993004/diff/390001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:165:
clipping_window_->AddObserver(this);
On 2013/10/24 18:16:50, sky wrote:
> Having the same object be a listener for two different windows is typically
> error prone (I have introduced bugs from this in the past). How about creating
a
> new object to be the listener for one of them so it is clear which observer
does
> what.

Done.

https://codereview.chromium.org/30993004/diff/390001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura.cc:165:
clipping_window_->AddObserver(this);
On 2013/10/24 18:16:50, sky wrote:
> Having the same object be a listener for two different windows is typically
> error prone (I have introduced bugs from this in the past). How about creating
a
> new object to be the listener for one of them so it is clear which observer
does
> what.

Done.

https://codereview.chromium.org/30993004/diff/390001/ui/views/controls/native...
File ui/views/controls/native/native_view_host_aura_unittest.cc (right):

https://codereview.chromium.org/30993004/diff/390001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura_unittest.cc:36: };
On 2013/10/24 18:16:50, sky wrote:
> nit: DISALLOW_...

Done.

https://codereview.chromium.org/30993004/diff/390001/ui/views/controls/native...
ui/views/controls/native/native_view_host_aura_unittest.cc:57: };
On 2013/10/24 18:16:50, sky wrote:
> nit: DISALLOW_...

Done.

[sky, 2013-10-24 20:47:12]

LGTM

[commit-bot: I haz the power, 2013-10-24 22:06:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rharrison@chromium.org/30993004/440001

[commit-bot: I haz the power, 2013-10-24 22:23:49]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-10-25 13:21:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rharrison@chromium.org/30993004/700001

[commit-bot: I haz the power, 2013-10-25 15:59:45]

Change committed as 231027