Update size of RWVH after bookmark bar is hidden

Update size of RWVH after bookmark bar is hidden

In the NTP, the bookmark bar is always shown detached. On navigating,
the navigation is committed (DidNavigateFrame -> CommitPending) which in
turn resizes the WebContentsView[Aura] and RenderWidgetHostView[Aura].
This causes a call to SnapToPhysicalPixelBoundary at the compositor
layer in non-integral DPI scales (which saves a fractional offset into
the layer to get it to align on a pixel boundary).

Later in navigation (DidNavigateMainFramePostCommit) we go back up to
chrome BrowserView, which now hides the bookmark bar (assuming it's not
shown by user preference) because it's not shown when not on the NTP.
This does a bunch of views/UI level layout but doesn't resize the
WebContentsView or RenderWidgetHostView. This means the saved 'snap' is
out of date.

In addition to not getting pixel-snapped (causing blurry fonts) this
also makes the "windows legacy hwnd" the wrong size because it also is
never resized (which would affect some mousewheel, etc. handling).

To resolve this, have the BrowserView resize the RenderWidgetHostView to
the size of the contents_container_ (ref:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/views/frame/browser_view.h&l=600
) when the bookmark bar visibility might have been modified.

r=sky@chromium.org
BUG=469857

[scottmg, 2015-03-25 06:11:30]

scottmg@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2015-03-25 20:45:36]

https://codereview.chromium.org/1009533005/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/1009533005/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:795:
content::RenderWidgetHostView* rwhv =
Won't Layout() end up in ContentsLayoutManager and do the right thing? If not,
where are things going wrong?

[scottmg, 2015-03-25 22:22:06]

https://codereview.chromium.org/1009533005/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/1009533005/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:795:
content::RenderWidgetHostView* rwhv =
On 2015/03/25 20:45:36, sky wrote:
> Won't Layout() end up in ContentsLayoutManager and do the right thing? If not,
> where are things going wrong?

Yeah, it does. On the initial navigate, the delegate_ on the aura::Window* ends
up going to

https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...

which does nothing.

**	views.dll!views::NativeViewHostAura::ClippingWindowDelegate::OnBoundsChanged(const
gfx::Rect & old_bounds, const gfx::Rect & new_bounds) Line 32	C++
 	aura.dll!aura::Window::OnWindowBoundsChanged(const gfx::Rect & old_bounds)
Line 1361	C++
 	aura.dll!base::internal::RunnableAdapter<void (__thiscall
aura::Window::*)(gfx::Rect const &)>::Run(aura::Window * object, const gfx::Rect
& <args_0>) Line 176	C++

	aura.dll!base::internal::InvokeHelper<0,void,base::internal::RunnableAdapter<void
(__thiscall aura::Window::*)(gfx::Rect const
&)>,base::internal::TypeList<aura::Window *,gfx::Rect const &>
>::MakeItSo(base::internal::RunnableAdapter<void (__thiscall
aura::Window::*)(gfx::Rect const &)> runnable, aura::Window * <args_0>, const
gfx::Rect & <args_1>) Line 294	C++

	aura.dll!base::internal::Invoker<IndexSequence<0,1>,base::internal::BindState<base::internal::RunnableAdapter<void
(__thiscall aura::Window::*)(gfx::Rect const &)>,void __cdecl(aura::Window
*,gfx::Rect const
&),base::internal::TypeList<base::internal::UnretainedWrapper<aura::Window>,gfx::Rect>
>,base::internal::TypeList<base::internal::UnwrapTraits<base::internal::UnretainedWrapper<aura::Window>
>,base::internal::UnwrapTraits<gfx::Rect>
>,base::internal::InvokeHelper<0,void,base::internal::RunnableAdapter<void
(__thiscall aura::Window::*)(gfx::Rect const
&)>,base::internal::TypeList<aura::Window *,gfx::Rect const &> >,void
__cdecl(void)>::Run(base::internal::BindStateBase * base) Line 346	C++
 	compositor.dll!base::Callback<void __cdecl(void)>::Run() Line 396	C++
 	compositor.dll!ui::Layer::SetBoundsFromAnimation(const gfx::Rect & bounds)
Line 872	C++
 	compositor.dll!ui::LayerAnimator::SetBounds(const gfx::Rect & value) Line
102	C++
 	compositor.dll!ui::Layer::SetBounds(const gfx::Rect & bounds) Line 246	C++
 	aura.dll!aura::Window::SetBoundsInternal(const gfx::Rect & new_bounds) Line
919	C++
 	aura.dll!aura::Window::SetBounds(const gfx::Rect & new_bounds) Line 455	C++
 	views.dll!views::NativeViewHostAura::ShowWidget(int x, int y, int w, int h)
Line 160	C++
 	views.dll!views::NativeViewHost::Layout() Line 97	C++
 	views.dll!views::View::BoundsChanged(const gfx::Rect & previous_bounds) Line
1941	C++
 	views.dll!views::View::SetBoundsRect(const gfx::Rect & bounds) Line 289	C++
 	webview.dll!views::WebView::OnBoundsChanged(const gfx::Rect & previous_bounds)
Line 163	C++
 	views.dll!views::View::BoundsChanged(const gfx::Rect & previous_bounds) Line
1937	C++
 	views.dll!views::View::SetBoundsRect(const gfx::Rect & bounds) Line 289	C++
 	chrome.dll!ContentsLayoutManager::Layout(views::View * contents_container)
Line 62	C++
 	views.dll!views::View::Layout() Line 536	C++
 	views.dll!views::View::SetBoundsRect(const gfx::Rect & bounds) Line 274	C++
 	chrome.dll!BrowserViewLayout::LayoutContentsContainerView(int top, int bottom)
Line 478	C++
 	chrome.dll!BrowserViewLayout::Layout(views::View * browser_view) Line 349	C++
 	views.dll!views::View::Layout() Line 536	C++
 	chrome.dll!BrowserView::Layout() Line 1904	C++
 	chrome.dll!BrowserView::BookmarkBarStateChanged(BookmarkBar::AnimateChangeType
change_type) Line 787	C++

	chrome.dll!Browser::UpdateBookmarkBarState(Browser::BookmarkBarStateChangeReason
reason) Line 2534	C++
 	chrome.dll!Browser::DidNavigateMainFramePostCommit(content::WebContents *
web_contents) Line 1662	C++


vs. when I resize the top level window it delegates back to
WebContentsViewAura::OnBoundsChanged:

 	compositor.dll!ui::SnapLayerToPhysicalPixelBoundary(ui::Layer * snapped_layer,
ui::Layer * layer_to_snap) Line 96	C++
 	content.dll!content::RenderWidgetHostViewAura::SnapToPhysicalPixelBoundary()
Line 2479	C++
 	content.dll!content::RenderWidgetHostViewAura::InternalSetBounds(const
gfx::Rect & rect) Line 2491	C++
 	content.dll!content::RenderWidgetHostViewAura::SetSize(const gfx::Size & size)
Line 657	C++
 	content.dll!content::WebContentsViewAura::SizeChangedCommon(const gfx::Size &
size) Line 776	C++
 	content.dll!content::WebContentsViewAura::OnBoundsChanged(const gfx::Rect &
old_bounds, const gfx::Rect & new_bounds) Line 1426	C++
**	aura.dll!aura::Window::OnWindowBoundsChanged(const gfx::Rect & old_bounds)
Line 1361	C++
 	aura.dll!base::internal::RunnableAdapter<void (__thiscall
aura::Window::*)(gfx::Rect const &)>::Run(aura::Window * object, const gfx::Rect
& <args_0>) Line 176	C++

	aura.dll!base::internal::InvokeHelper<0,void,base::internal::RunnableAdapter<void
(__thiscall aura::Window::*)(gfx::Rect const
&)>,base::internal::TypeList<aura::Window *,gfx::Rect const &>
>::MakeItSo(base::internal::RunnableAdapter<void (__thiscall
aura::Window::*)(gfx::Rect const &)> runnable, aura::Window * <args_0>, const
gfx::Rect & <args_1>) Line 294	C++

	aura.dll!base::internal::Invoker<IndexSequence<0,1>,base::internal::BindState<base::internal::RunnableAdapter<void
(__thiscall aura::Window::*)(gfx::Rect const &)>,void __cdecl(aura::Window
*,gfx::Rect const
&),base::internal::TypeList<base::internal::UnretainedWrapper<aura::Window>,gfx::Rect>
>,base::internal::TypeList<base::internal::UnwrapTraits<base::internal::UnretainedWrapper<aura::Window>
>,base::internal::UnwrapTraits<gfx::Rect>
>,base::internal::InvokeHelper<0,void,base::internal::RunnableAdapter<void
(__thiscall aura::Window::*)(gfx::Rect const
&)>,base::internal::TypeList<aura::Window *,gfx::Rect const &> >,void
__cdecl(void)>::Run(base::internal::BindStateBase * base) Line 346	C++
 	compositor.dll!base::Callback<void __cdecl(void)>::Run() Line 396	C++
 	compositor.dll!ui::Layer::SetBoundsFromAnimation(const gfx::Rect & bounds)
Line 872	C++
 	compositor.dll!ui::LayerAnimator::SetBounds(const gfx::Rect & value) Line
102	C++
 	compositor.dll!ui::Layer::SetBounds(const gfx::Rect & bounds) Line 246	C++
 	aura.dll!aura::Window::SetBoundsInternal(const gfx::Rect & new_bounds) Line
919	C++
 	aura.dll!aura::Window::SetBounds(const gfx::Rect & new_bounds) Line 455	C++
 	views.dll!views::NativeViewHostAura::ShowWidget(int x, int y, int w, int h)
Line 164	C++
 	views.dll!views::NativeViewHost::Layout() Line 97	C++
 	views.dll!views::View::BoundsChanged(const gfx::Rect & previous_bounds) Line
1941	C++
 	views.dll!views::View::SetBoundsRect(const gfx::Rect & bounds) Line 289	C++
 	webview.dll!views::WebView::OnBoundsChanged(const gfx::Rect & previous_bounds)
Line 163	C++
 	views.dll!views::View::BoundsChanged(const gfx::Rect & previous_bounds) Line
1937	C++
 	views.dll!views::View::SetBoundsRect(const gfx::Rect & bounds) Line 289	C++
 	chrome.dll!ContentsLayoutManager::Layout(views::View * contents_container)
Line 62	C++


I don't understand what that ClippingWindowDelegate is supposed to be doing. I
tried (in ps#1) to forward to its contained native_view_ and that fixed the
problem too, but I couldn't come up with a views_unittest that actually needed
that, so I went this way instead. Does any of that make sense?

[scottmg, 2015-03-25 22:36:02]

On 2015/03/25 22:22:06, scottmg wrote:
>
https://codereview.chromium.org/1009533005/diff/60001/chrome/browser/ui/views...
> File chrome/browser/ui/views/frame/browser_view.cc (right):
> 
>
https://codereview.chromium.org/1009533005/diff/60001/chrome/browser/ui/views...
> chrome/browser/ui/views/frame/browser_view.cc:795:
> content::RenderWidgetHostView* rwhv =
> On 2015/03/25 20:45:36, sky wrote:
> > Won't Layout() end up in ContentsLayoutManager and do the right thing? If
not,
> > where are things going wrong?
> 
> Yeah, it does. On the initial navigate, the delegate_ on the aura::Window*
ends
> up going to
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...
> 
> which does nothing.
> 
>
**	views.dll!views::NativeViewHostAura::ClippingWindowDelegate::OnBoundsChanged(const
> gfx::Rect & old_bounds, const gfx::Rect & new_bounds) Line 32	C++
>  	aura.dll!aura::Window::OnWindowBoundsChanged(const gfx::Rect & old_bounds)
> Line 1361	C++
>  	aura.dll!base::internal::RunnableAdapter<void (__thiscall
> aura::Window::*)(gfx::Rect const &)>::Run(aura::Window * object, const
gfx::Rect
> & <args_0>) Line 176	C++
> 
>
	aura.dll!base::internal::InvokeHelper<0,void,base::internal::RunnableAdapter<void
> (__thiscall aura::Window::*)(gfx::Rect const
> &)>,base::internal::TypeList<aura::Window *,gfx::Rect const &>
> >::MakeItSo(base::internal::RunnableAdapter<void (__thiscall
> aura::Window::*)(gfx::Rect const &)> runnable, aura::Window * <args_0>, const
> gfx::Rect & <args_1>) Line 294	C++
> 
>
	aura.dll!base::internal::Invoker<IndexSequence<0,1>,base::internal::BindState<base::internal::RunnableAdapter<void
> (__thiscall aura::Window::*)(gfx::Rect const &)>,void __cdecl(aura::Window
> *,gfx::Rect const
>
&),base::internal::TypeList<base::internal::UnretainedWrapper<aura::Window>,gfx::Rect>
>
>,base::internal::TypeList<base::internal::UnwrapTraits<base::internal::UnretainedWrapper<aura::Window>
> >,base::internal::UnwrapTraits<gfx::Rect>
> >,base::internal::InvokeHelper<0,void,base::internal::RunnableAdapter<void
> (__thiscall aura::Window::*)(gfx::Rect const
> &)>,base::internal::TypeList<aura::Window *,gfx::Rect const &> >,void
> __cdecl(void)>::Run(base::internal::BindStateBase * base) Line 346	C++
>  	compositor.dll!base::Callback<void __cdecl(void)>::Run() Line 396	C++
>  	compositor.dll!ui::Layer::SetBoundsFromAnimation(const gfx::Rect & bounds)
> Line 872	C++
>  	compositor.dll!ui::LayerAnimator::SetBounds(const gfx::Rect & value) Line
> 102	C++
>  	compositor.dll!ui::Layer::SetBounds(const gfx::Rect & bounds) Line 246	C++
>  	aura.dll!aura::Window::SetBoundsInternal(const gfx::Rect & new_bounds) Line
> 919	C++
>  	aura.dll!aura::Window::SetBounds(const gfx::Rect & new_bounds) Line 455	C++
>  	views.dll!views::NativeViewHostAura::ShowWidget(int x, int y, int w, int h)
> Line 160	C++
>  	views.dll!views::NativeViewHost::Layout() Line 97	C++
>  	views.dll!views::View::BoundsChanged(const gfx::Rect & previous_bounds) Line
> 1941	C++
>  	views.dll!views::View::SetBoundsRect(const gfx::Rect & bounds) Line 289	C++
>  	webview.dll!views::WebView::OnBoundsChanged(const gfx::Rect &
previous_bounds)
> Line 163	C++
>  	views.dll!views::View::BoundsChanged(const gfx::Rect & previous_bounds) Line
> 1937	C++
>  	views.dll!views::View::SetBoundsRect(const gfx::Rect & bounds) Line 289	C++
>  	chrome.dll!ContentsLayoutManager::Layout(views::View * contents_container)
> Line 62	C++
>  	views.dll!views::View::Layout() Line 536	C++
>  	views.dll!views::View::SetBoundsRect(const gfx::Rect & bounds) Line 274	C++
>  	chrome.dll!BrowserViewLayout::LayoutContentsContainerView(int top, int
bottom)
> Line 478	C++
>  	chrome.dll!BrowserViewLayout::Layout(views::View * browser_view) Line
349	C++
>  	views.dll!views::View::Layout() Line 536	C++
>  	chrome.dll!BrowserView::Layout() Line 1904	C++
> 
	chrome.dll!BrowserView::BookmarkBarStateChanged(BookmarkBar::AnimateChangeType
> change_type) Line 787	C++
> 
>
	chrome.dll!Browser::UpdateBookmarkBarState(Browser::BookmarkBarStateChangeReason
> reason) Line 2534	C++
>  	chrome.dll!Browser::DidNavigateMainFramePostCommit(content::WebContents *
> web_contents) Line 1662	C++
> 
> 
> vs. when I resize the top level window it delegates back to
> WebContentsViewAura::OnBoundsChanged:
> 
>  	compositor.dll!ui::SnapLayerToPhysicalPixelBoundary(ui::Layer *
snapped_layer,
> ui::Layer * layer_to_snap) Line 96	C++
>  	content.dll!content::RenderWidgetHostViewAura::SnapToPhysicalPixelBoundary()
> Line 2479	C++
>  	content.dll!content::RenderWidgetHostViewAura::InternalSetBounds(const
> gfx::Rect & rect) Line 2491	C++
>  	content.dll!content::RenderWidgetHostViewAura::SetSize(const gfx::Size &
size)
> Line 657	C++
>  	content.dll!content::WebContentsViewAura::SizeChangedCommon(const gfx::Size
&
> size) Line 776	C++
>  	content.dll!content::WebContentsViewAura::OnBoundsChanged(const gfx::Rect &
> old_bounds, const gfx::Rect & new_bounds) Line 1426	C++
> **	aura.dll!aura::Window::OnWindowBoundsChanged(const gfx::Rect & old_bounds)
> Line 1361	C++
>  	aura.dll!base::internal::RunnableAdapter<void (__thiscall
> aura::Window::*)(gfx::Rect const &)>::Run(aura::Window * object, const
gfx::Rect
> & <args_0>) Line 176	C++
> 
>
	aura.dll!base::internal::InvokeHelper<0,void,base::internal::RunnableAdapter<void
> (__thiscall aura::Window::*)(gfx::Rect const
> &)>,base::internal::TypeList<aura::Window *,gfx::Rect const &>
> >::MakeItSo(base::internal::RunnableAdapter<void (__thiscall
> aura::Window::*)(gfx::Rect const &)> runnable, aura::Window * <args_0>, const
> gfx::Rect & <args_1>) Line 294	C++
> 
>
	aura.dll!base::internal::Invoker<IndexSequence<0,1>,base::internal::BindState<base::internal::RunnableAdapter<void
> (__thiscall aura::Window::*)(gfx::Rect const &)>,void __cdecl(aura::Window
> *,gfx::Rect const
>
&),base::internal::TypeList<base::internal::UnretainedWrapper<aura::Window>,gfx::Rect>
>
>,base::internal::TypeList<base::internal::UnwrapTraits<base::internal::UnretainedWrapper<aura::Window>
> >,base::internal::UnwrapTraits<gfx::Rect>
> >,base::internal::InvokeHelper<0,void,base::internal::RunnableAdapter<void
> (__thiscall aura::Window::*)(gfx::Rect const
> &)>,base::internal::TypeList<aura::Window *,gfx::Rect const &> >,void
> __cdecl(void)>::Run(base::internal::BindStateBase * base) Line 346	C++
>  	compositor.dll!base::Callback<void __cdecl(void)>::Run() Line 396	C++
>  	compositor.dll!ui::Layer::SetBoundsFromAnimation(const gfx::Rect & bounds)
> Line 872	C++
>  	compositor.dll!ui::LayerAnimator::SetBounds(const gfx::Rect & value) Line
> 102	C++
>  	compositor.dll!ui::Layer::SetBounds(const gfx::Rect & bounds) Line 246	C++
>  	aura.dll!aura::Window::SetBoundsInternal(const gfx::Rect & new_bounds) Line
> 919	C++
>  	aura.dll!aura::Window::SetBounds(const gfx::Rect & new_bounds) Line 455	C++
>  	views.dll!views::NativeViewHostAura::ShowWidget(int x, int y, int w, int h)
> Line 164	C++
>  	views.dll!views::NativeViewHost::Layout() Line 97	C++
>  	views.dll!views::View::BoundsChanged(const gfx::Rect & previous_bounds) Line
> 1941	C++
>  	views.dll!views::View::SetBoundsRect(const gfx::Rect & bounds) Line 289	C++
>  	webview.dll!views::WebView::OnBoundsChanged(const gfx::Rect &
previous_bounds)
> Line 163	C++
>  	views.dll!views::View::BoundsChanged(const gfx::Rect & previous_bounds) Line
> 1937	C++
>  	views.dll!views::View::SetBoundsRect(const gfx::Rect & bounds) Line 289	C++
>  	chrome.dll!ContentsLayoutManager::Layout(views::View * contents_container)
> Line 62	C++
> 
> 
> I don't understand what that ClippingWindowDelegate is supposed to be doing. I
> tried (in ps#1) to forward to its contained native_view_ and that fixed the
> problem too, but I couldn't come up with a views_unittest that actually needed
> that, so I went this way instead. Does any of that make sense?

er, to its native_view_->delegate(). I had trouble figuring out what the
structure was, and what was contained and delegated to there. Maybe that's a
better way. I tried adding a test similar to
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...
but I didn't come up with anything required
ClippingWindowDelegate::OnBoundsChanged(...) to call
native_view_->delegate()->OnBoundsChanged(...).

[sky, 2015-03-25 23:28:57]

The ClippingWindow is there for fast resize (see it in action when you
toggle the bookmark bar between always visible and not always
visible).

The NativeViewHostAura::ClippingWindowDelegate::OnBoundsChanged()
shouldn't matter. This is the important line:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...
.

On Wed, Mar 25, 2015 at 3:22 PM,  <scottmg@chromium.org> wrote:
>
>
https://codereview.chromium.org/1009533005/diff/60001/chrome/browser/ui/views...
> File chrome/browser/ui/views/frame/browser_view.cc (right):
>
>
https://codereview.chromium.org/1009533005/diff/60001/chrome/browser/ui/views...
> chrome/browser/ui/views/frame/browser_view.cc:795:
> content::RenderWidgetHostView* rwhv =
> On 2015/03/25 20:45:36, sky wrote:
>>
>> Won't Layout() end up in ContentsLayoutManager and do the right thing?
>
> If not,
>>
>> where are things going wrong?
>
>
> Yeah, it does. On the initial navigate, the delegate_ on the
> aura::Window* ends up going to
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...
>
> which does nothing.
>
> **
>
views.dll!views::NativeViewHostAura::ClippingWindowDelegate::OnBoundsChanged(const
> gfx::Rect & old_bounds, const gfx::Rect & new_bounds) Line 32   C++
>         aura.dll!aura::Window::OnWindowBoundsChanged(const gfx::Rect &
> old_bounds) Line 1361   C++
>         aura.dll!base::internal::RunnableAdapter<void (__thiscall
> aura::Window::*)(gfx::Rect const &)>::Run(aura::Window * object, const
> gfx::Rect & <args_0>) Line 176  C++
>
>
>
aura.dll!base::internal::InvokeHelper<0,void,base::internal::RunnableAdapter<void
> (__thiscall aura::Window::*)(gfx::Rect const
> &)>,base::internal::TypeList<aura::Window *,gfx::Rect const &>
>>
>> ::MakeItSo(base::internal::RunnableAdapter<void (__thiscall
>
> aura::Window::*)(gfx::Rect const &)> runnable, aura::Window * <args_0>,
> const gfx::Rect & <args_1>) Line 294    C++
>
>
>
aura.dll!base::internal::Invoker<IndexSequence<0,1>,base::internal::BindState<base::internal::RunnableAdapter<void
> (__thiscall aura::Window::*)(gfx::Rect const &)>,void
> __cdecl(aura::Window *,gfx::Rect const
>
&),base::internal::TypeList<base::internal::UnretainedWrapper<aura::Window>,gfx::Rect>
>>
>>
>>
,base::internal::TypeList<base::internal::UnwrapTraits<base::internal::UnretainedWrapper<aura::Window>
>> ,base::internal::UnwrapTraits<gfx::Rect>
>> ,base::internal::InvokeHelper<0,void,base::internal::RunnableAdapter<void
>
> (__thiscall aura::Window::*)(gfx::Rect const
> &)>,base::internal::TypeList<aura::Window *,gfx::Rect const &> >,void
> __cdecl(void)>::Run(base::internal::BindStateBase * base) Line 346      C++
>         compositor.dll!base::Callback<void __cdecl(void)>::Run() Line 396
> C++
>         compositor.dll!ui::Layer::SetBoundsFromAnimation(const gfx::Rect &
> bounds) Line 872        C++
>         compositor.dll!ui::LayerAnimator::SetBounds(const gfx::Rect & value)
> Line 102        C++
>         compositor.dll!ui::Layer::SetBounds(const gfx::Rect & bounds) Line
> 246     C++
>         aura.dll!aura::Window::SetBoundsInternal(const gfx::Rect &
> new_bounds)
> Line 919        C++
>         aura.dll!aura::Window::SetBounds(const gfx::Rect & new_bounds) Line
> 455     C++
>         views.dll!views::NativeViewHostAura::ShowWidget(int x, int y, int w,
> int h) Line 160 C++
>         views.dll!views::NativeViewHost::Layout() Line 97       C++
>         views.dll!views::View::BoundsChanged(const gfx::Rect &
> previous_bounds) Line 1941      C++
>         views.dll!views::View::SetBoundsRect(const gfx::Rect & bounds) Line
> 289     C++
>         webview.dll!views::WebView::OnBoundsChanged(const gfx::Rect &
> previous_bounds) Line 163       C++
>         views.dll!views::View::BoundsChanged(const gfx::Rect &
> previous_bounds) Line 1937      C++
>         views.dll!views::View::SetBoundsRect(const gfx::Rect & bounds) Line
> 289     C++
>         chrome.dll!ContentsLayoutManager::Layout(views::View *
> contents_container) Line 62     C++
>         views.dll!views::View::Layout() Line 536        C++
>         views.dll!views::View::SetBoundsRect(const gfx::Rect & bounds) Line
> 274     C++
>         chrome.dll!BrowserViewLayout::LayoutContentsContainerView(int top,
> int
> bottom) Line 478        C++
>         chrome.dll!BrowserViewLayout::Layout(views::View * browser_view)
> Line
> 349     C++
>         views.dll!views::View::Layout() Line 536        C++
>         chrome.dll!BrowserView::Layout() Line 1904      C++
>
>
>
chrome.dll!BrowserView::BookmarkBarStateChanged(BookmarkBar::AnimateChangeType
> change_type) Line 787   C++
>
>
>
chrome.dll!Browser::UpdateBookmarkBarState(Browser::BookmarkBarStateChangeReason
> reason) Line 2534       C++
>
>
> chrome.dll!Browser::DidNavigateMainFramePostCommit(content::WebContents
> * web_contents) Line 1662       C++
>
>
> vs. when I resize the top level window it delegates back to
> WebContentsViewAura::OnBoundsChanged:
>
>         compositor.dll!ui::SnapLayerToPhysicalPixelBoundary(ui::Layer *
> snapped_layer, ui::Layer * layer_to_snap) Line 96       C++
>
>
> content.dll!content::RenderWidgetHostViewAura::SnapToPhysicalPixelBoundary()
> Line 2479       C++
>
> content.dll!content::RenderWidgetHostViewAura::InternalSetBounds(const
> gfx::Rect & rect) Line 2491     C++
>         content.dll!content::RenderWidgetHostViewAura::SetSize(const
> gfx::Size
> & size) Line 657        C++
>         content.dll!content::WebContentsViewAura::SizeChangedCommon(const
> gfx::Size & size) Line 776      C++
>         content.dll!content::WebContentsViewAura::OnBoundsChanged(const
> gfx::Rect & old_bounds, const gfx::Rect & new_bounds) Line 1426 C++
> **      aura.dll!aura::Window::OnWindowBoundsChanged(const gfx::Rect &
> old_bounds) Line 1361   C++
>         aura.dll!base::internal::RunnableAdapter<void (__thiscall
> aura::Window::*)(gfx::Rect const &)>::Run(aura::Window * object, const
> gfx::Rect & <args_0>) Line 176  C++
>
>
>
aura.dll!base::internal::InvokeHelper<0,void,base::internal::RunnableAdapter<void
> (__thiscall aura::Window::*)(gfx::Rect const
> &)>,base::internal::TypeList<aura::Window *,gfx::Rect const &>
>>
>> ::MakeItSo(base::internal::RunnableAdapter<void (__thiscall
>
> aura::Window::*)(gfx::Rect const &)> runnable, aura::Window * <args_0>,
> const gfx::Rect & <args_1>) Line 294    C++
>
>
>
aura.dll!base::internal::Invoker<IndexSequence<0,1>,base::internal::BindState<base::internal::RunnableAdapter<void
> (__thiscall aura::Window::*)(gfx::Rect const &)>,void
> __cdecl(aura::Window *,gfx::Rect const
>
&),base::internal::TypeList<base::internal::UnretainedWrapper<aura::Window>,gfx::Rect>
>>
>>
>>
,base::internal::TypeList<base::internal::UnwrapTraits<base::internal::UnretainedWrapper<aura::Window>
>> ,base::internal::UnwrapTraits<gfx::Rect>
>> ,base::internal::InvokeHelper<0,void,base::internal::RunnableAdapter<void
>
> (__thiscall aura::Window::*)(gfx::Rect const
> &)>,base::internal::TypeList<aura::Window *,gfx::Rect const &> >,void
> __cdecl(void)>::Run(base::internal::BindStateBase * base) Line 346      C++
>         compositor.dll!base::Callback<void __cdecl(void)>::Run() Line 396
> C++
>         compositor.dll!ui::Layer::SetBoundsFromAnimation(const gfx::Rect &
> bounds) Line 872        C++
>         compositor.dll!ui::LayerAnimator::SetBounds(const gfx::Rect & value)
> Line 102        C++
>         compositor.dll!ui::Layer::SetBounds(const gfx::Rect & bounds) Line
> 246     C++
>         aura.dll!aura::Window::SetBoundsInternal(const gfx::Rect &
> new_bounds)
> Line 919        C++
>         aura.dll!aura::Window::SetBounds(const gfx::Rect & new_bounds) Line
> 455     C++
>         views.dll!views::NativeViewHostAura::ShowWidget(int x, int y, int w,
> int h) Line 164 C++
>         views.dll!views::NativeViewHost::Layout() Line 97       C++
>         views.dll!views::View::BoundsChanged(const gfx::Rect &
> previous_bounds) Line 1941      C++
>         views.dll!views::View::SetBoundsRect(const gfx::Rect & bounds) Line
> 289     C++
>         webview.dll!views::WebView::OnBoundsChanged(const gfx::Rect &
> previous_bounds) Line 163       C++
>         views.dll!views::View::BoundsChanged(const gfx::Rect &
> previous_bounds) Line 1937      C++
>         views.dll!views::View::SetBoundsRect(const gfx::Rect & bounds) Line
> 289     C++
>         chrome.dll!ContentsLayoutManager::Layout(views::View *
> contents_container) Line 62     C++
>
>
> I don't understand what that ClippingWindowDelegate is supposed to be
> doing. I tried (in ps#1) to forward to its contained native_view_ and
> that fixed the problem too, but I couldn't come up with a views_unittest
> that actually needed that, so I went this way instead. Does any of that
> make sense?
>
> https://codereview.chromium.org/1009533005/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[scottmg, 2015-03-26 01:51:56]

On 2015/03/25 23:28:57, sky wrote:
> The ClippingWindow is there for fast resize (see it in action when you
> toggle the bookmark bar between always visible and not always
> visible).
> 
> The NativeViewHostAura::ClippingWindowDelegate::OnBoundsChanged()
> shouldn't matter. This is the important line:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...
> .


That goes through almost the right place:

>	aura.dll!aura::Window::SetBoundsInternal(const gfx::Rect & new_bounds) Line
927	C++
 	aura.dll!aura::Window::SetBounds(const gfx::Rect & new_bounds) Line 455	C++
 	views.dll!views::NativeViewHostAura::ShowWidget(int x, int y, int w, int h)
Line 164	C++
 	views.dll!views::NativeViewHost::Layout() Line 97	C++
 	views.dll!views::View::BoundsChanged(const gfx::Rect & previous_bounds) Line
1941	C++
 	views.dll!views::View::SetBoundsRect(const gfx::Rect & bounds) Line 289	C++
 	webview.dll!views::WebView::OnBoundsChanged(const gfx::Rect & previous_bounds)
Line 163	C++
 	views.dll!views::View::BoundsChanged(const gfx::Rect & previous_bounds) Line
1937	C++
 	views.dll!views::View::SetBoundsRect(const gfx::Rect & bounds) Line 289	C++
 	chrome.dll!ContentsLayoutManager::Layout(views::View * contents_container)
Line 62	C++

But
https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window.cc&...
makes it not call OnWindowBoundsChanged(). Making it unconditional does make the
resize happen and fixes the problem.

There's no animation on removing the bookmark bar, so I guess related to that?

[sky, 2015-03-26 13:47:42]

On Wed, Mar 25, 2015 at 6:51 PM,  <scottmg@chromium.org> wrote:
> On 2015/03/25 23:28:57, sky wrote:
>>
>> The ClippingWindow is there for fast resize (see it in action when you
>> toggle the bookmark bar between always visible and not always
>> visible).
>
>
>> The NativeViewHostAura::ClippingWindowDelegate::OnBoundsChanged()
>> shouldn't matter. This is the important line:
>
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...
>>
>> .
>
>
>
> That goes through almost the right place:
>
>>         aura.dll!aura::Window::SetBoundsInternal(const gfx::Rect &
>> new_bounds) Line
>
> 927     C++
>         aura.dll!aura::Window::SetBounds(const gfx::Rect & new_bounds) Line
> 455 C++
>         views.dll!views::NativeViewHostAura::ShowWidget(int x, int y, int w,
> int h)
> Line 164        C++
>         views.dll!views::NativeViewHost::Layout() Line 97       C++
>         views.dll!views::View::BoundsChanged(const gfx::Rect &
> previous_bounds) Line
> 1941    C++
>         views.dll!views::View::SetBoundsRect(const gfx::Rect & bounds) Line
> 289 C++
>         webview.dll!views::WebView::OnBoundsChanged(const gfx::Rect &
> previous_bounds)
> Line 163        C++
>         views.dll!views::View::BoundsChanged(const gfx::Rect &
> previous_bounds) Line
> 1937    C++
>         views.dll!views::View::SetBoundsRect(const gfx::Rect & bounds) Line
> 289 C++
>         chrome.dll!ContentsLayoutManager::Layout(views::View *
> contents_container)
> Line 62 C++
>
> But
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window.cc&...
> makes it not call OnWindowBoundsChanged(). Making it unconditional does make
> the
> resize happen and fixes the problem.

So, if 925 isn't being hit it means the Window has a Layer and the
Window is the Layer's delegate. The reason 925 is a no-op in this case
is because it's expected:

    layer()->SetBounds(actual_new_bounds);

a couple of lines up is going to end up calling
OnWindowBoundsChanged() when the bounds change. Where does that code
end up going?

  -Scott

> There's no animation on removing the bookmark bar, so I guess related to
> that?
>
>
> https://codereview.chromium.org/1009533005/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2015-03-26 17:08:03]

On Thu, Mar 26, 2015 at 6:47 AM, Scott Violet <sky@chromium.org> wrote:

> On Wed, Mar 25, 2015 at 6:51 PM,  <scottmg@chromium.org> wrote:
> > On 2015/03/25 23:28:57, sky wrote:
> >>
> >> The ClippingWindow is there for fast resize (see it in action when you
> >> toggle the bookmark bar between always visible and not always
> >> visible).
> >
> >
> >> The NativeViewHostAura::ClippingWindowDelegate::OnBoundsChanged()
> >> shouldn't matter. This is the important line:
> >
> >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...
> >>
> >> .
> >
> >
> >
> > That goes through almost the right place:
> >
> >>         aura.dll!aura::Window::SetBoundsInternal(const gfx::Rect &
> >> new_bounds) Line
> >
> > 927     C++
> >         aura.dll!aura::Window::SetBounds(const gfx::Rect & new_bounds)
> Line
> > 455 C++
> >         views.dll!views::NativeViewHostAura::ShowWidget(int x, int y,
> int w,
> > int h)
> > Line 164        C++
> >         views.dll!views::NativeViewHost::Layout() Line 97       C++
> >         views.dll!views::View::BoundsChanged(const gfx::Rect &
> > previous_bounds) Line
> > 1941    C++
> >         views.dll!views::View::SetBoundsRect(const gfx::Rect & bounds)
> Line
> > 289 C++
> >         webview.dll!views::WebView::OnBoundsChanged(const gfx::Rect &
> > previous_bounds)
> > Line 163        C++
> >         views.dll!views::View::BoundsChanged(const gfx::Rect &
> > previous_bounds) Line
> > 1937    C++
> >         views.dll!views::View::SetBoundsRect(const gfx::Rect & bounds)
> Line
> > 289 C++
> >         chrome.dll!ContentsLayoutManager::Layout(views::View *
> > contents_container)
> > Line 62 C++
> >
> > But
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window.cc&...
> > makes it not call OnWindowBoundsChanged(). Making it unconditional does
> make
> > the
> > resize happen and fixes the problem.
>
> So, if 925 isn't being hit it means the Window has a Layer and the
> Window is the Layer's delegate. The reason 925 is a no-op in this case
> is because it's expected:
>
>     layer()->SetBounds(actual_new_bounds);
>
> a couple of lines up is going to end up calling
> OnWindowBoundsChanged() when the bounds change. Where does that code
> end up going?
>
>
During creating a new tab, the bar gets shown in DETACHED and there's a
resize to

{origin_={x_=3 y_=119 } size_={width_=1030 height_=672 } } 0x32257130

after navigate and BookmarkBarView::Layout when the bar is HIDDEN.

{origin_={x_=3 y_=80 } size_={width_=1030 height_=711 } } 0x32257130

So, I think that's the one that should be resnapping (but I'm not
positive). But the layer()->SetBounds(...) in Window::SetBoundsInternal()
for that size goes to the ClippingWindowDelegate::OnBoundsChanged which
does nothing.

So confusing... The log's here:
https://gist.github.com/sgraham/6a56455a7629b094708b




>   -Scott
>
> > There's no animation on removing the bookmark bar, so I guess related to
> > that?
> >
> >
> > https://codereview.chromium.org/1009533005/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2015-03-26 19:08:16]

Ah, I think I know what it is. Before the bookmark bar is actually hidden
this happens:

  compositor.dll!ui::Layer::SetBoundsFromAnimation(const gfx::Rect &
bounds) Line 857 C++
  compositor.dll!ui::LayerAnimator::SetBounds(const gfx::Rect & value) Line
102 C++
  compositor.dll!ui::Layer::SetBounds(const gfx::Rect & bounds) Line 246 C++
  aura.dll!aura::Window::SetBoundsInternal(const gfx::Rect & new_bounds)
Line 919 C++
  aura.dll!aura::Window::SetBounds(const gfx::Rect & new_bounds) Line 455
C++
  content.dll!content::WebContentsViewAura::SizeContents(const gfx::Size &
size) Line 1022 C++
> content.dll!content::WebContentsImpl::UpdateRenderViewSizeForRenderManager()
Line 4267 C++
  content.dll!content::RenderFrameHostManager::CommitPending() Line 1706 C++
 
content.dll!content::RenderFrameHostManager::CommitPendingIfNecessary(content::RenderFrameHostImpl
* render_frame_host, bool was_caused_by_user_gesture) Line 488 C++
 
content.dll!content::RenderFrameHostManager::DidNavigateFrame(content::RenderFrameHostImpl
* render_frame_host, bool was_caused_by_user_gesture) Line 452 C++
  content.dll!content::NavigatorImpl::DidNavigate(content::RenderFrameHostImpl
* render_frame_host, const FrameHostMsg_DidCommitProvisionalLoad_Params &
input_params) Line 408 C++

this sets the size to the larger size (as if the bookmark bar was already
gone). This happens because of what looks like a bit of hack in
Browser::GetSizeForNewRenderView. This causes the WebContentsViewAura to be
sized to that larger size, but that happens before the bookmark bar is
actually hidden in views layout code. So the screen offset is still spaced
down and unaligned, so a snap happens to a fraction.

Then I think what happens, is that the bookmark bar hides and does Layout.
But the resizes that it causes that would normally get to
WebContentsViewAura layer are ignored because SetBoundsFromAnimation does

    void Layer::SetBoundsFromAnimation(const gfx::Rect& bounds) {
      if (bounds == bounds_)
        return;
    ...

so it never gets another chance to snap.

Does that seem plausible? I'm not sure what the right fix is now.


On Thu, Mar 26, 2015 at 10:08 AM, Scott Graham <scottmg@google.com> wrote:

>
>
> On Thu, Mar 26, 2015 at 6:47 AM, Scott Violet <sky@chromium.org> wrote:
>
>> On Wed, Mar 25, 2015 at 6:51 PM,  <scottmg@chromium.org> wrote:
>> > On 2015/03/25 23:28:57, sky wrote:
>> >>
>> >> The ClippingWindow is there for fast resize (see it in action when you
>> >> toggle the bookmark bar between always visible and not always
>> >> visible).
>> >
>> >
>> >> The NativeViewHostAura::ClippingWindowDelegate::OnBoundsChanged()
>> >> shouldn't matter. This is the important line:
>> >
>> >
>> >
>>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...
>> >>
>> >> .
>> >
>> >
>> >
>> > That goes through almost the right place:
>> >
>> >>         aura.dll!aura::Window::SetBoundsInternal(const gfx::Rect &
>> >> new_bounds) Line
>> >
>> > 927     C++
>> >         aura.dll!aura::Window::SetBounds(const gfx::Rect & new_bounds)
>> Line
>> > 455 C++
>> >         views.dll!views::NativeViewHostAura::ShowWidget(int x, int y,
>> int w,
>> > int h)
>> > Line 164        C++
>> >         views.dll!views::NativeViewHost::Layout() Line 97       C++
>> >         views.dll!views::View::BoundsChanged(const gfx::Rect &
>> > previous_bounds) Line
>> > 1941    C++
>> >         views.dll!views::View::SetBoundsRect(const gfx::Rect & bounds)
>> Line
>> > 289 C++
>> >         webview.dll!views::WebView::OnBoundsChanged(const gfx::Rect &
>> > previous_bounds)
>> > Line 163        C++
>> >         views.dll!views::View::BoundsChanged(const gfx::Rect &
>> > previous_bounds) Line
>> > 1937    C++
>> >         views.dll!views::View::SetBoundsRect(const gfx::Rect & bounds)
>> Line
>> > 289 C++
>> >         chrome.dll!ContentsLayoutManager::Layout(views::View *
>> > contents_container)
>> > Line 62 C++
>> >
>> > But
>> >
>>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window.cc&...
>> > makes it not call OnWindowBoundsChanged(). Making it unconditional does
>> make
>> > the
>> > resize happen and fixes the problem.
>>
>> So, if 925 isn't being hit it means the Window has a Layer and the
>> Window is the Layer's delegate. The reason 925 is a no-op in this case
>> is because it's expected:
>>
>>     layer()->SetBounds(actual_new_bounds);
>>
>> a couple of lines up is going to end up calling
>> OnWindowBoundsChanged() when the bounds change. Where does that code
>> end up going?
>>
>>
> During creating a new tab, the bar gets shown in DETACHED and there's a
> resize to
>
> {origin_={x_=3 y_=119 } size_={width_=1030 height_=672 } } 0x32257130
>
> after navigate and BookmarkBarView::Layout when the bar is HIDDEN.
>
> {origin_={x_=3 y_=80 } size_={width_=1030 height_=711 } } 0x32257130
>
> So, I think that's the one that should be resnapping (but I'm not
> positive). But the layer()->SetBounds(...) in Window::SetBoundsInternal()
> for that size goes to the ClippingWindowDelegate::OnBoundsChanged which
> does nothing.
>
> So confusing... The log's here:
> https://gist.github.com/sgraham/6a56455a7629b094708b
>
>
>
>
>>   -Scott
>>
>> > There's no animation on removing the bookmark bar, so I guess related to
>> > that?
>> >
>> >
>> > https://codereview.chromium.org/1009533005/
>>
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to chromium-reviews+unsubscribe@chromium.org.
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[scottmg, 2015-04-01 22:10:27]

Hi, do you think my last explanation makes some sense?

I kind of like the last patch set because in that it should only affect this
particular case. But you have any other suggestions I'd be happy to try some
other way.

[scottmg, 2015-04-01 22:10:52]

+ananta who's looking at a bug related to the legacy hwnd

[sky, 2015-04-01 22:57:38]

If we're saying RWHVA needs to resnap any times it's bounds relative
to the root change then RWHVA should listen to ancestors location
changes and resnap.

  -Scott

On Wed, Apr 1, 2015 at 3:10 PM,  <scottmg@chromium.org> wrote:
> Hi, do you think my last explanation makes some sense?
>
> I kind of like the last patch set because in that it should only affect this
> particular case. But you have any other suggestions I'd be happy to try some
> other way.
>
> https://codereview.chromium.org/1009533005/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.