Move fallback tick handling to compositor thread

Move fallback tick software draw.

Ideally we want to remove the fallback ticking of webview entirely, but
in the interim we should move it to the compositor thread in order to
avoid the sync IPC for the software draw as well as the shared memory
allocation.

BUG=431166
Committed: https://crrev.com/3fd0c95168d270bd4c8bdc52caf05d24ae64d32e
Cr-Commit-Position: refs/heads/master@{#381688}

[Tobias Sargeant, 2016-03-10 15:28:16]

Description was changed from

==========
Remove fallback tick, reimplement on compositor thread

[WIP]

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

to

==========
Move fallback tick software draw.

Ideally we want to remove the fallback ticking of webview entirely, but
in the interim we should move it to the compositor thread in order to
avoid the sync IPC for the software draw as well as the shared memory
allocation.

BUG=431166
==========

[Tobias Sargeant, 2016-03-10 15:28:16]

tobiasjs@chromium.org changed reviewers:
+ boliu@chromium.org

[boliu, 2016-03-11 17:43:09]

https://codereview.chromium.org/1782093002/diff/1/android_webview/browser/bro...
File android_webview/browser/browser_view_renderer.cc (left):

https://codereview.chromium.org/1782093002/diff/1/android_webview/browser/bro...
android_webview/browser/browser_view_renderer.cc:718: (is_paused_ &&
!clear_view_) || (attached_to_window_ && !window_visible_);
So, according to IsClientVisible, visible = !is_paused_ && (!attached_to_window_
|| window_visible_);

And compositor will commit or animate etc when invisible.

So, the first point is bogus after we enabled page visibility, and second point
is superceded by visibility. Cool :)

https://codereview.chromium.org/1782093002/diff/1/content/renderer/android/sy...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/1782093002/diff/1/content/renderer/android/sy...
content/renderer/android/synchronous_compositor_output_surface.cc:137: void
SynchronousCompositorOutputSurface::FallbackTickFired() {
DCHECK(CalledOnValidThread());

https://codereview.chromium.org/1782093002/diff/1/content/renderer/android/sy...
content/renderer/android/synchronous_compositor_output_surface.cc:138:
TRACE_EVENT0("android_webview",
change category to "renderer"

https://codereview.chromium.org/1782093002/diff/1/content/renderer/android/sy...
content/renderer/android/synchronous_compositor_output_surface.cc:152:
fallback_tick_.Reset(
Need to check if fallback tick is already pending, so this doesn't end up in a
live lock where compositor calls invalidate more frequently than ticks, so it
gets pushed back indefinitely.

[Tobias Sargeant, 2016-03-14 13:11:03]

https://codereview.chromium.org/1782093002/diff/1/content/renderer/android/sy...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/1782093002/diff/1/content/renderer/android/sy...
content/renderer/android/synchronous_compositor_output_surface.cc:137: void
SynchronousCompositorOutputSurface::FallbackTickFired() {
On 2016/03/11 17:43:09, boliu wrote:
> DCHECK(CalledOnValidThread());

Done.

https://codereview.chromium.org/1782093002/diff/1/content/renderer/android/sy...
content/renderer/android/synchronous_compositor_output_surface.cc:138:
TRACE_EVENT0("android_webview",
On 2016/03/11 17:43:09, boliu wrote:
> change category to "renderer"

Done.

https://codereview.chromium.org/1782093002/diff/1/content/renderer/android/sy...
content/renderer/android/synchronous_compositor_output_surface.cc:152:
fallback_tick_.Reset(
On 2016/03/11 17:43:09, boliu wrote:
> Need to check if fallback tick is already pending, so this doesn't end up in a
> live lock where compositor calls invalidate more frequently than ticks, so it
> gets pushed back indefinitely.

Done.

[boliu, 2016-03-14 15:33:55]

https://codereview.chromium.org/1782093002/diff/20001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/1782093002/diff/20001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.cc:80:
frame_swap_message_queue_(frame_swap_message_queue) {
initialize fallback_tick_pending_

[Tobias Sargeant, 2016-03-14 15:40:48]

https://codereview.chromium.org/1782093002/diff/20001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/1782093002/diff/20001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.cc:80:
frame_swap_message_queue_(frame_swap_message_queue) {
On 2016/03/14 15:33:55, boliu wrote:
> initialize fallback_tick_pending_

Done.

[boliu, 2016-03-14 15:41:29]

lgtm

[Tobias Sargeant, 2016-03-14 15:41:42]

The CQ bit was checked by tobiasjs@chromium.org

[commit-bot: I haz the power, 2016-03-14 15:41:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1782093002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1782093002/40001

[boliu, 2016-03-14 17:42:57]

The CQ bit was unchecked by boliu@chromium.org

[boliu, 2016-03-14 17:45:10]

https://codereview.chromium.org/1782093002/diff/40001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/1782093002/diff/40001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.cc:133:
sync_client_->SwapBuffers(frame);
Tests failing DCHECK:
[FATAL:synchronous_compositor_proxy.cc(370)] Check failed: hardware_draw_reply_
|| software_draw_reply_.

Forgot that we are not supposed to swap these frames for |sync_client_|.

I guess need to keep more state. Maybe a inside_fallback_tick_ with an autoreset

[boliu, 2016-03-15 15:10:54]

https://codereview.chromium.org/1782093002/diff/60001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/1782093002/diff/60001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.cc:134: if
(!fallback_tick_running_) sync_client_->SwapBuffers(frame);
c++ style says this is should be on two lines

https://codereview.chromium.org/1782093002/diff/60001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.cc:149:
fallback_tick_pending_ = false;
subtle: but should do this before DemandDrawSw, in case DemandDrawSw calls
invalidate internally.

[Tobias Sargeant, 2016-03-16 17:29:28]

https://codereview.chromium.org/1782093002/diff/60001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/1782093002/diff/60001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.cc:134: if
(!fallback_tick_running_) sync_client_->SwapBuffers(frame);
On 2016/03/15 15:10:54, boliu wrote:
> c++ style says this is should be on two lines

Huh. I thought so, but clang-format didn't reindent it, so I assumed that it
wasn't gospel. But then git cl format did. I'll check my clang-format config.

[in fact clang-format'ing the line actively puts it back like this]

https://codereview.chromium.org/1782093002/diff/60001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.cc:149:
fallback_tick_pending_ = false;
On 2016/03/15 15:10:54, boliu wrote:
> subtle: but should do this before DemandDrawSw, in case DemandDrawSw calls
> invalidate internally.

Done.

[boliu, 2016-03-16 19:05:57]

https://codereview.chromium.org/1782093002/diff/80001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/1782093002/diff/80001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.cc:123:
fallback_tick_pending_ = false;
This Cancel + pending_ = false pattern is repeated 3 times now. Factor it out
into a private method.

[boliu, 2016-03-16 19:06:10]

On 2016/03/16 19:05:57, boliu wrote:
>
https://codereview.chromium.org/1782093002/diff/80001/content/renderer/androi...
> File content/renderer/android/synchronous_compositor_output_surface.cc
(right):
> 
>
https://codereview.chromium.org/1782093002/diff/80001/content/renderer/androi...
> content/renderer/android/synchronous_compositor_output_surface.cc:123:
> fallback_tick_pending_ = false;
> This Cancel + pending_ = false pattern is repeated 3 times now. Factor it out
> into a private method.

lgtm after that

[Tobias Sargeant, 2016-03-17 09:45:22]

The CQ bit was checked by tobiasjs@chromium.org

[Tobias Sargeant, 2016-03-17 09:45:23]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/1782093002/#ps100001
(title: "Refactor fallback tick cancellation")

[commit-bot: I haz the power, 2016-03-17 09:45:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1782093002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1782093002/100001

[commit-bot: I haz the power, 2016-03-17 10:51:48]

Description was changed from

==========
Move fallback tick software draw.

Ideally we want to remove the fallback ticking of webview entirely, but
in the interim we should move it to the compositor thread in order to
avoid the sync IPC for the software draw as well as the shared memory
allocation.

BUG=431166
==========

to

==========
Move fallback tick software draw.

Ideally we want to remove the fallback ticking of webview entirely, but
in the interim we should move it to the compositor thread in order to
avoid the sync IPC for the software draw as well as the shared memory
allocation.

BUG=431166
==========

[commit-bot: I haz the power, 2016-03-17 10:51:49]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-03-17 10:52:51]

Description was changed from

==========
Move fallback tick software draw.

Ideally we want to remove the fallback ticking of webview entirely, but
in the interim we should move it to the compositor thread in order to
avoid the sync IPC for the software draw as well as the shared memory
allocation.

BUG=431166
==========

to

==========
Move fallback tick software draw.

Ideally we want to remove the fallback ticking of webview entirely, but
in the interim we should move it to the compositor thread in order to
avoid the sync IPC for the software draw as well as the shared memory
allocation.

BUG=431166

Committed: https://crrev.com/3fd0c95168d270bd4c8bdc52caf05d24ae64d32e
Cr-Commit-Position: refs/heads/master@{#381688}
==========

[commit-bot: I haz the power, 2016-03-17 10:52:52]

Patchset 6 (id:??) landed as
https://crrev.com/3fd0c95168d270bd4c8bdc52caf05d24ae64d32e
Cr-Commit-Position: refs/heads/master@{#381688}