WIP - Gutterless resize on Windows

This CL generalizes the WindowResizeHelper which was used
on mac to support windows (and other impls as well). This
class provides a nested run loop which can be used to
produce a frame while something is blocking our main
run loop (for instance, during WM_SIZE on windows).

I'll add a follow-up patch to handle linux, which
should be able to share most of this, but likely needs
some custom logic.

With this in place, our guttering is much much less.
There are two follow-ups which make guttering very
hard to notice:
- Change the contents web view background to match the
window background, so we don't draw jarring black bars
when the browser gets ahead of the renderer.
- Change the toolbar strip at the top to not indicate
that it is transparent. This currently causes us to clear
the background to black, and this black strip is visible
when we get the very small amount of guttering we're left
with.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

[ericrk, 2015-12-09 20:55:52]

Description was changed from

==========
wip window resize


wip window resize

BUG=
==========

to

==========
wip window resize


wip window resize

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[ericrk, 2015-12-10 23:08:04]

Description was changed from

==========
wip window resize


wip window resize

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
This CL generalizes the WindowResizeHelper which was used
on mac to support windows (and other impls as well). This
class provides a nested run loop which can be used to
produce a frame while something is blocking our main
run loop (for instance, during WM_SIZE on windows).

I'll add a follow-up patch to handle linux, which
should be able to share most of this, but likely needs
some custom logic.

With this in place, our guttering is much much less.
There are two follow-ups which make guttering very
hard to notice:
- Change the contents web view background to match the
window background, so we don't draw jarring black bars
when the browser gets ahead of the renderer.
- Change the toolbar strip at the top to not indicate
that it is transparent. This currently causes us to clear
the background to black, and this black strip is visible
when we get the very small amount of guttering we're left
with.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[ericrk, 2015-12-10 23:08:04]

ericrk@chromium.org changed reviewers:
+ piman@chromium.org

[ericrk, 2015-12-10 23:08:43]

Here's a first WIP for drastically reducing guttering on windows. Let me know
how this looks.

https://codereview.chromium.org/1513053002/diff/20001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1513053002/diff/20001/content/browser/browser...
content/browser/browser_main_loop.cc:754: #if (defined(OS_MACOSX) &&
!defined(OS_IOS)) || defined(USE_AURA)
should be OS_WIN not USE_AURA, will adjust in next patch.

https://codereview.chromium.org/1513053002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/1513053002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:81: #include
"ui/base/window_resize_helper.h"
The logic here doesn't seem to be necessary on Windows. Still trying to figure
out why the render_widget_host_impl as well as the actual bridged_native_widget
need to both have 50ms waits on mac. On windows these both cancel eachother out
(waiting in one place removes the need to wait in the other) - however, putting
the wait here introduced significant stuttering on windows.

[piman, 2015-12-11 07:53:12]

Feedback in a few places.
I think this CL would handle the UI resize, but not necessary the renderer
resize. I think it's ok to do it by steps, see the comments in
render_widget_host_impl.cc for where to look for the renderer. Happy to chat
when I come back for that part.

https://codereview.chromium.org/1513053002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/1513053002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:81: #include
"ui/base/window_resize_helper.h"
On 2015/12/10 23:08:43, ericrk wrote:
> The logic here doesn't seem to be necessary on Windows. Still trying to figure
> out why the render_widget_host_impl as well as the actual
bridged_native_widget
> need to both have 50ms waits on mac. On windows these both cancel eachother
out
> (waiting in one place removes the need to wait in the other) - however,
putting
> the wait here introduced significant stuttering on windows.



Well, if you want the UI *and* the renderer to stick to the edge, after a resize
you need to:
- resize the UI elements, including the container for the renderer (which will
send the resize to the renderer)
- wait for a frame from the renderer at the new size (WaitForSurface here)
- draw a UI frame with that renderer frame
- wait for the swap (the one in bridged_native_widget)

all of these without new resizes being serviced (i.e. blocking the message
loop).


The code here is to handle the renderer parts, on Mac, which is a little
different than Aura - the ui::Compositor only renders the renderer parts, not
the whole UI, and is resized by the renderer's frame, instead of the UI.
The 2 timeouts are meant to be nested within each other, so it could be why it
appears that they're redundant (but they may not be in all cases, e.g.
considering a slow renderer vs a slow GPU).

For Aura, the renderer parts would need to interact with the resize_lock logic
in DelegatedFrameHost/RenderWidgetHostViewAura (which was originally created for
Chrome OS, by throttling resizes rather than blocking the thread, because we
could. This code was disabled on linux/windows - see
RenderWidgetHostViewAura::DelegatedFrameCanCreateResizeLock).

https://codereview.chromium.org/1513053002/diff/20001/content/common/gpu/clie...
File content/common/gpu/client/gpu_channel_host.cc (right):

https://codereview.chromium.org/1513053002/diff/20001/content/common/gpu/clie...
content/common/gpu/client/gpu_channel_host.cc:328: thread_task_runner =
ui::WindowResizeHelper::Get()->task_runner();
This code (GpuChannelHost) is used by both the renderer and the browser, so it's
not appropriate to always use WindowResizeHelper here.
My suggestion is to have the factory (which is different between the 2 cases)
provide the TaskRunner.

https://codereview.chromium.org/1513053002/diff/20001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/1513053002/diff/20001/ui/compositor/composito...
ui/compositor/compositor.cc:446: has_swapped_frame_at_current_size_ = true;
It is possible that the main thread will start the next frame before it received
the SwapBuffers ack (e.g. if the GPU process is under load). If that happens, it
could be that SetScaleAndSize is called again before this is called, in which
case it wouldn't be appropriate to set this to true. We could also have multiple
frames in the pipeline.

You'd want to track the size as it goes through the compositor steps. After you
set the size you need all of these to happen:
- commit
- draw frame
- post swap
- complete swap

you only got a "correct" frame if SetScaleAndSize wasn't called between any of
these steps. You can either track that manually (e.g. have a state enum, that
gets advanced in each of the relevant callbacks, and reset in SetScaleAndSize),
or using a SwapPromise which will track the first 3 for you.

[ericrk, 2015-12-15 00:25:03]

Thanks for the feedback! I think it's best to save the renderer sync part for a
follow up.

https://codereview.chromium.org/1513053002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/1513053002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:81: #include
"ui/base/window_resize_helper.h"
On 2015/12/11 07:53:12, piman (in KST timezone) wrote:
> On 2015/12/10 23:08:43, ericrk wrote:
> > The logic here doesn't seem to be necessary on Windows. Still trying to
figure
> > out why the render_widget_host_impl as well as the actual
> bridged_native_widget
> > need to both have 50ms waits on mac. On windows these both cancel eachother
> out
> > (waiting in one place removes the need to wait in the other) - however,
> putting
> > the wait here introduced significant stuttering on windows.
> 
> 
> 
> Well, if you want the UI *and* the renderer to stick to the edge, after a
resize
> you need to:
> - resize the UI elements, including the container for the renderer (which will
> send the resize to the renderer)
> - wait for a frame from the renderer at the new size (WaitForSurface here)
> - draw a UI frame with that renderer frame
> - wait for the swap (the one in bridged_native_widget)
> 
> all of these without new resizes being serviced (i.e. blocking the message
> loop).
> 
> 
> The code here is to handle the renderer parts, on Mac, which is a little
> different than Aura - the ui::Compositor only renders the renderer parts, not
> the whole UI, and is resized by the renderer's frame, instead of the UI.
> The 2 timeouts are meant to be nested within each other, so it could be why it
> appears that they're redundant (but they may not be in all cases, e.g.
> considering a slow renderer vs a slow GPU).
> 
> For Aura, the renderer parts would need to interact with the resize_lock logic
> in DelegatedFrameHost/RenderWidgetHostViewAura (which was originally created
for
> Chrome OS, by throttling resizes rather than blocking the thread, because we
> could. This code was disabled on linux/windows - see
> RenderWidgetHostViewAura::DelegatedFrameCanCreateResizeLock).

Thanks - I think it's best to save this for a follow-up? We can discuss when
you're back.

https://codereview.chromium.org/1513053002/diff/20001/content/common/gpu/clie...
File content/common/gpu/client/gpu_channel_host.cc (right):

https://codereview.chromium.org/1513053002/diff/20001/content/common/gpu/clie...
content/common/gpu/client/gpu_channel_host.cc:328: thread_task_runner =
ui::WindowResizeHelper::Get()->task_runner();
On 2015/12/11 07:53:12, piman (in KST timezone) wrote:
> This code (GpuChannelHost) is used by both the renderer and the browser, so
it's
> not appropriate to always use WindowResizeHelper here.
> My suggestion is to have the factory (which is different between the 2 cases)
> provide the TaskRunner.

done - good point - was thinking that was what IsMainThread meant, but realize I
misread this.

https://codereview.chromium.org/1513053002/diff/20001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/1513053002/diff/20001/ui/compositor/composito...
ui/compositor/compositor.cc:446: has_swapped_frame_at_current_size_ = true;
On 2015/12/11 07:53:12, piman (in KST timezone) wrote:
> It is possible that the main thread will start the next frame before it
received
> the SwapBuffers ack (e.g. if the GPU process is under load). If that happens,
it
> could be that SetScaleAndSize is called again before this is called, in which
> case it wouldn't be appropriate to set this to true. We could also have
multiple
> frames in the pipeline.
> 
> You'd want to track the size as it goes through the compositor steps. After
you
> set the size you need all of these to happen:
> - commit
> - draw frame
> - post swap
> - complete swap
> 
> you only got a "correct" frame if SetScaleAndSize wasn't called between any of
> these steps. You can either track that manually (e.g. have a state enum, that
> gets advanced in each of the relevant callbacks, and reset in
SetScaleAndSize),
> or using a SwapPromise which will track the first 3 for you.

Good feedback - I've updated this to use a SwapPromise to track most of this.
Let me know how the new version looks.

[piman, 2015-12-15 07:15:36]

https://codereview.chromium.org/1513053002/diff/60001/content/browser/gpu/bro...
File content/browser/gpu/browser_gpu_channel_host_factory.h (right):

https://codereview.chromium.org/1513053002/diff/60001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_channel_host_factory.h:34:
scoped_refptr<base::SingleThreadTaskRunner> TaskRunner() override;
nit: GetMainThreadTaskRunner() ? To parallel GetIOThreadTaskRunner.

https://codereview.chromium.org/1513053002/diff/60001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/1513053002/diff/60001/ui/compositor/composito...
ui/compositor/compositor.cc:58: // thread.
We have only one thread on the browser side now, so no need to post the task.

Actually you probably don't want to post the task, otherwise you could have a
race where the swap buffer callback arrives before you got a chance to execute
this task.

https://codereview.chromium.org/1513053002/diff/60001/ui/compositor/composito...
ui/compositor/compositor.cc:484: weak_ptr_factory_.GetWeakPtr()));
You don't need to post the task any more (see above).

https://codereview.chromium.org/1513053002/diff/60001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right):

https://codereview.chromium.org/1513053002/diff/60001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:968: const int
kPaintMsgTimeoutMS = 55;
Any specific reason for 55 vs 50?
My gut feeling is that we should use the same value on all platforms, unless
there is a good reason not to.