Make RenderViewImpl::OnForceRedraw more robust

Make RenderViewImpl::OnForceRedraw more robust

Before this change, RenderWidgetHostImpl::GetSnapshotFromBrowser could
spuriously fail (and callback might never be fired). The nature of
unresponsiveness is in fact that scheduled draw and swap can
be aborted by cc::Scheduler and, as a result, we have active tree
deletion with all ui::LatencyInfo data sent by browser.

BUG=637066
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/751af1f9be588e97d6c37628499c421322797e18
Cr-Commit-Position: refs/heads/master@{#411892}

[svartmetal, 2016-07-28 15:23:13]

Description was changed from

==========
Make RenderViewImpl::OnForceRedraw more robust

Before this change, RenderWidgetHostImpl::GetSnapshotFromBrowser could
spuriously fail (and callback might never be fired). The nature of
unresponsiveness is in fact that scheduled draw and swap can
be aborted by cc::Scheduler and, as a result, we have active tree
deletion with all ui::LatencyInfo data sent by browser.

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

to

==========
Make RenderViewImpl::OnForceRedraw more robust

Before this change, RenderWidgetHostImpl::GetSnapshotFromBrowser could
spuriously fail (and callback might never be fired). The nature of
unresponsiveness is in fact that scheduled draw and swap can
be aborted by cc::Scheduler and, as a result, we have active tree
deletion with all ui::LatencyInfo data sent by browser.

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

[svartmetal, 2016-07-28 15:43:35]

Description was changed from

==========
Make RenderViewImpl::OnForceRedraw more robust

Before this change, RenderWidgetHostImpl::GetSnapshotFromBrowser could
spuriously fail (and callback might never be fired). The nature of
unresponsiveness is in fact that scheduled draw and swap can
be aborted by cc::Scheduler and, as a result, we have active tree
deletion with all ui::LatencyInfo data sent by browser.

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

to

==========
Make RenderViewImpl::OnForceRedraw more robust

Before this change, RenderWidgetHostImpl::GetSnapshotFromBrowser could
spuriously fail (and callback might never be fired). The nature of
unresponsiveness is in fact that scheduled draw and swap can
be aborted by cc::Scheduler and, as a result, we have active tree
deletion with all ui::LatencyInfo data sent by browser.

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

[svartmetal, 2016-07-28 15:43:35]

svartmetal@yandex-team.ru changed reviewers:
+ nasko@chromium.org, piman@chromium.org

[svartmetal, 2016-07-28 15:59:48]

PTAL

[nasko, 2016-07-28 16:11:58]

The IPC looks good. Will stamp once piman@ is happy with the CL and approves it.

[piman, 2016-07-28 20:54:08]

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

[piman, 2016-07-28 20:54:09]

+danakj for cc/
I'm not convinced that adding a completely separate path for "forced redraw"
promises is the most maintainable way. Maybe we could instead add an option to
the promises that they don't get broken for no damage / lost surface cases?
Maybe extending the DidNotSwapReason enum to properly distinguish those.

Alternatively, some other option could be to have DidNotSwap return a bool about
whether or not to keep the promise active, that the implementation could tune
depending on the reason.

https://codereview.chromium.org/2185823005/diff/1/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/2185823005/diff/1/cc/trees/layer_tree_host.cc...
cc/trees/layer_tree_host.cc:1314: swap_promise_list_.clear();
You also need to break the forced_redraw_swap_promises_ - they'll never happen
(this is called in LTH destructor).

https://codereview.chromium.org/2185823005/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2185823005/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:305: active_tree_ = nullptr;
You need to break the forced_redraw_swap_promises_ - they'll never happen.

https://codereview.chromium.org/2185823005/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:633:
forced_redraw_swap_promises_.push_back(std::move(swap_promise));
This would bypass the activation, meaning, you may end up calling DidSwap before
you actually drew the frame you forced.

https://codereview.chromium.org/2185823005/diff/1/cc/trees/swap_promise_monit...
File cc/trees/swap_promise_monitor.h (right):

https://codereview.chromium.org/2185823005/diff/1/cc/trees/swap_promise_monit...
cc/trees/swap_promise_monitor.h:35: virtual void OnSetNeedsForcedRedrawOnMain()
= 0;
Is this and the associated logic in LTH really needed? It looks like the same
logic (RenderViewImpl::OnForceRedraw) both creates the concrete swap promise
monitor and forces redraw. Couldn't it directly tell the monitor that it forces
redraw, instead of going through LTH to do that?

[svartmetal, 2016-08-02 16:46:33]

On 2016/07/28 20:54:09, piman - slow reviews until 8-8 wrote:
> +danakj for cc/
> I'm not convinced that adding a completely separate path for "forced redraw"
> promises is the most maintainable way. Maybe we could instead add an option to
> the promises that they don't get broken for no damage / lost surface cases?
> Maybe extending the DidNotSwapReason enum to properly distinguish those.
> 
> Alternatively, some other option could be to have DidNotSwap return a bool
about
> whether or not to keep the promise active, that the implementation could tune
> depending on the reason.
> 
> https://codereview.chromium.org/2185823005/diff/1/cc/trees/layer_tree_host.cc
> File cc/trees/layer_tree_host.cc (right):
> 
>
https://codereview.chromium.org/2185823005/diff/1/cc/trees/layer_tree_host.cc...
> cc/trees/layer_tree_host.cc:1314: swap_promise_list_.clear();
> You also need to break the forced_redraw_swap_promises_ - they'll never happen
> (this is called in LTH destructor).
> 
>
https://codereview.chromium.org/2185823005/diff/1/cc/trees/layer_tree_host_im...
> File cc/trees/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/2185823005/diff/1/cc/trees/layer_tree_host_im...
> cc/trees/layer_tree_host_impl.cc:305: active_tree_ = nullptr;
> You need to break the forced_redraw_swap_promises_ - they'll never happen.
> 
>
https://codereview.chromium.org/2185823005/diff/1/cc/trees/layer_tree_host_im...
> cc/trees/layer_tree_host_impl.cc:633:
> forced_redraw_swap_promises_.push_back(std::move(swap_promise));
> This would bypass the activation, meaning, you may end up calling DidSwap
before
> you actually drew the frame you forced.
> 
>
https://codereview.chromium.org/2185823005/diff/1/cc/trees/swap_promise_monit...
> File cc/trees/swap_promise_monitor.h (right):
> 
>
https://codereview.chromium.org/2185823005/diff/1/cc/trees/swap_promise_monit...
> cc/trees/swap_promise_monitor.h:35: virtual void
OnSetNeedsForcedRedrawOnMain()
> = 0;
> Is this and the associated logic in LTH really needed? It looks like the same
> logic (RenderViewImpl::OnForceRedraw) both creates the concrete swap promise
> monitor and forces redraw. Couldn't it directly tell the monitor that it
forces
> redraw, instead of going through LTH to do that?

Fixed your comments, PTAL.

[svartmetal, 2016-08-03 10:16:02]

The CQ bit was checked by svartmetal@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 10:16:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 10:16:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-03 10:16:16]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * master.tryserver.blink builder linux_blink_rel was renamed to
linux_precise_blink_rel (http://crbug.com/590036)

[piman, 2016-08-03 16:38:55]

https://codereview.chromium.org/2185823005/diff/20001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2185823005/diff/20001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:1511: if
(swap_promise->DidNotSwap(SwapPromise::SWAP_FAILS)) {
this should be |reason| instead of |SWAP_FAILS|

https://codereview.chromium.org/2185823005/diff/20001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:1519: swap_promise->DidNotSwap(reason);
Should the logic apply here too?

[svartmetal, 2016-08-04 14:58:49]

Thanks for response!

Fixed your comments. 
Also, i've introduced enum DidNotSwapAction (instead of bool) and added some
tests.

https://codereview.chromium.org/2185823005/diff/20001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2185823005/diff/20001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:1511: if
(swap_promise->DidNotSwap(SwapPromise::SWAP_FAILS)) {
On 2016/08/03 16:38:55, piman - slow reviews until 8-8 wrote:
> this should be |reason| instead of |SWAP_FAILS|
Fixed.

https://codereview.chromium.org/2185823005/diff/20001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:1519: swap_promise->DidNotSwap(reason);
On 2016/08/03 16:38:55, piman - slow reviews until 8-8 wrote:
> Should the logic apply here too?
I think, it should (to be consistent). But maybe we should reconsider the
concept of pinned swap promises as we have generalized way to create persistent
swap promises...

[svartmetal, 2016-08-04 15:02:58]

The CQ bit was checked by svartmetal@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2016-08-04 15:03:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 15:03:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-04 15:03:32]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * master.tryserver.blink builder linux_blink_rel was renamed to
linux_precise_blink_rel (http://crbug.com/590036)

[svartmetal, 2016-08-05 13:05:48]

piman, danakj ping.

[piman, 2016-08-08 19:35:56]

LGTM for content and overall approach (and thanks for the test!), but danakj
should review the cc part.

[danakj, 2016-08-09 22:40:02]

> BUG=

Can you file a bug and point this at it, if there isn't an existing bug?

[danakj, 2016-08-10 01:04:16]

There's no BUG to look for more info, can you explain this?

> scheduled draw and swap can be aborted by cc::Scheduler 

The draw/swap can be aborted by LayerTreeHostImpl if:
- There's no damage
- The LTHI::CanDraw is false (a number of reasons why)
- An animation is checkerboarding
- High res content is required but missing
- Context is lost
- "DRAW_ABORTED_DRAINING_PIPELINE" i am not sure what that is offhand.

What is happening in tests that is causing the draw fail exactly?

https://codereview.chromium.org/2185823005/diff/40001/cc/output/forced_redraw...
File cc/output/forced_redraw_swap_promise.h (right):

https://codereview.chromium.org/2185823005/diff/40001/cc/output/forced_redraw...
cc/output/forced_redraw_swap_promise.h:16: class CC_EXPORT
ForcedRedrawSwapPromise : public SwapPromise {
nit: i prefer AlwaysDrawSwapPromise. "Forced draws" are actually a term for a
specific unrelated thing in cc.

https://codereview.chromium.org/2185823005/diff/40001/cc/output/swap_promise.h
File cc/output/swap_promise.h (right):

https://codereview.chromium.org/2185823005/diff/40001/cc/output/swap_promise....
cc/output/swap_promise.h:54: DEFAULT_ACTION,
I'd like this name to actually talk about what it means. This is RETIRE or
BREAK_PROMISE or something.

https://codereview.chromium.org/2185823005/diff/40001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2185823005/diff/40001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:1490: // |swap_promise| must remain active, so place
it to |new_swap_promises|
"place it in"

https://codereview.chromium.org/2185823005/diff/40001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:1518:
swap_promise_list_.swap(persistent_swap_promises);
nit: swap_promise_list_ = std::move(persistent_swap_promises) cuz we don't
actually care what ends up in persistent_swap_promises.

https://codereview.chromium.org/2185823005/diff/40001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:1531:
pinned_swap_promise_list_.swap(persistent_swap_promises);
ditto

https://codereview.chromium.org/2185823005/diff/40001/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/2185823005/diff/40001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.h:361: // Takes the |new_swap_promises| and appends it
to |swap_promise_list_|.
"appends them" while youre fixing this

https://codereview.chromium.org/2185823005/diff/40001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl_unittest.cc (right):

https://codereview.chromium.org/2185823005/diff/40001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl_unittest.cc:2286: EXPECT_TRUE(persistent_promises[i])
<< "While checking case #" << i;
Instead of checking the destructor didn't run (what this does essentially) you
could instead check that breaking swap promise a 2nd time still reports
DidNotSwap, and that drawing eventually would report DidSwap?

https://codereview.chromium.org/2185823005/diff/40001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2185823005/diff/40001/content/renderer/render...
content/renderer/render_view_impl.cc:1471: new
cc::ForcedRedrawSwapPromise(latency_info));
Why does this SwapPromise impl live in cc::? There's only this one creation
site, it could live entirely in this .cc file?

https://codereview.chromium.org/2185823005/diff/40001/content/renderer/render...
content/renderer/render_view_impl.cc:1472:
rwc->QueueSwapPromise(std::move(swap_promise));
nit:
rwc->QueueSwapPromise(base::MakeUnique<ForcedRedrawSwapPromise>(latency_info));

(no need for the temp |swap_promise| var.)

[svartmetal, 2016-08-10 17:38:22]

Can't find existing bug. 
Also it's hard to reproduce the problem, because of spurious nature of draw
aborts. 
So, i can file a bug, but can't specify steps to reproduce.

For now, tests are not affected by this problem (there are no tests for
GetSnapshotFromBrowser). 
But it is a tricky source of flakiness and should be fixed.

https://codereview.chromium.org/2185823005/diff/40001/cc/output/forced_redraw...
File cc/output/forced_redraw_swap_promise.h (right):

https://codereview.chromium.org/2185823005/diff/40001/cc/output/forced_redraw...
cc/output/forced_redraw_swap_promise.h:16: class CC_EXPORT
ForcedRedrawSwapPromise : public SwapPromise {
On 2016/08/10 01:04:15, danakj wrote:
> nit: i prefer AlwaysDrawSwapPromise. "Forced draws" are actually a term for a
> specific unrelated thing in cc.
Fixed.

https://codereview.chromium.org/2185823005/diff/40001/cc/output/swap_promise.h
File cc/output/swap_promise.h (right):

https://codereview.chromium.org/2185823005/diff/40001/cc/output/swap_promise....
cc/output/swap_promise.h:54: DEFAULT_ACTION,
On 2016/08/10 01:04:15, danakj wrote:
> I'd like this name to actually talk about what it means. This is RETIRE or
> BREAK_PROMISE or something.
Done.

https://codereview.chromium.org/2185823005/diff/40001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2185823005/diff/40001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:1490: // |swap_promise| must remain active, so place
it to |new_swap_promises|
On 2016/08/10 01:04:15, danakj wrote:
> "place it in"
Done.

https://codereview.chromium.org/2185823005/diff/40001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:1518:
swap_promise_list_.swap(persistent_swap_promises);
On 2016/08/10 01:04:15, danakj wrote:
> nit: swap_promise_list_ = std::move(persistent_swap_promises) cuz we don't
> actually care what ends up in persistent_swap_promises.
Done.

https://codereview.chromium.org/2185823005/diff/40001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:1531:
pinned_swap_promise_list_.swap(persistent_swap_promises);
On 2016/08/10 01:04:15, danakj wrote:
> ditto
Done.

https://codereview.chromium.org/2185823005/diff/40001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl_unittest.cc (right):

https://codereview.chromium.org/2185823005/diff/40001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl_unittest.cc:2286: EXPECT_TRUE(persistent_promises[i])
<< "While checking case #" << i;
On 2016/08/10 01:04:15, danakj wrote:
> Instead of checking the destructor didn't run (what this does essentially) you
> could instead check that breaking swap promise a 2nd time still reports
> DidNotSwap, and that drawing eventually would report DidSwap?
Reasonable. Made EXPECT_CALL for DidSwap.

https://codereview.chromium.org/2185823005/diff/40001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2185823005/diff/40001/content/renderer/render...
content/renderer/render_view_impl.cc:1471: new
cc::ForcedRedrawSwapPromise(latency_info));
On 2016/08/10 01:04:15, danakj wrote:
> Why does this SwapPromise impl live in cc::? There's only this one creation
> site, it could live entirely in this .cc file?
Yes, for now it's true. 
But I think this file is already too big, that's why i'd like to keep
AlwaysDrawPromise in separate file...
However, i can fix it in the next patch set, if you want.

https://codereview.chromium.org/2185823005/diff/40001/content/renderer/render...
content/renderer/render_view_impl.cc:1472:
rwc->QueueSwapPromise(std::move(swap_promise));
On 2016/08/10 01:04:15, danakj wrote:
> nit:
>
rwc->QueueSwapPromise(base::MakeUnique<ForcedRedrawSwapPromise>(latency_info));
> 
> (no need for the temp |swap_promise| var.)
Done.

[danakj, 2016-08-11 18:08:51]

Oh, I thought this mechanism existed only for tests. I see it is used by
devtools.

Can you please file a bug and point to it from the patch description? That would
be helpful for following up in case of more problems or whatever. Thanks!

Also, I realize RenderViewImpl is a big file, but I'd rather you put it there
for now (or where you are thinking as a follow up?). If it's in cc/output/ it
becomes part of the cc external API, and that's already large enough.

[svartmetal, 2016-08-11 20:38:52]

Thanks for review!

Moved AlwaysDrawSwapPromise to render_view_iml.cc.

Also, i've created a bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=637066.

[danakj, 2016-08-11 21:57:18]

Description was changed from

==========
Make RenderViewImpl::OnForceRedraw more robust

Before this change, RenderWidgetHostImpl::GetSnapshotFromBrowser could
spuriously fail (and callback might never be fired). The nature of
unresponsiveness is in fact that scheduled draw and swap can
be aborted by cc::Scheduler and, as a result, we have active tree
deletion with all ui::LatencyInfo data sent by browser.

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

to

==========
Make RenderViewImpl::OnForceRedraw more robust

Before this change, RenderWidgetHostImpl::GetSnapshotFromBrowser could
spuriously fail (and callback might never be fired). The nature of
unresponsiveness is in fact that scheduled draw and swap can
be aborted by cc::Scheduler and, as a result, we have active tree
deletion with all ui::LatencyInfo data sent by browser.

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

[danakj, 2016-08-11 21:59:00]

Thanks! I added the bug number to the CL description for you. A few last nits
and LGTM

https://codereview.chromium.org/2185823005/diff/80001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2185823005/diff/80001/content/renderer/render...
content/renderer/render_view_impl.cc:639: class CC_EXPORT AlwaysDrawSwapPromise
: public cc::SwapPromise {
drop the CC_EXPORT

https://codereview.chromium.org/2185823005/diff/80001/content/renderer/render...
content/renderer/render_view_impl.cc:1500: base::WrapUnique(new
AlwaysDrawSwapPromise(latency_info)));
one last nit: prefer base::MakeUnique<AlwaysDrawSwapPromise>(...) over
base::WrapUnique(new AlwaysDrawSwapPromise(...))

[svartmetal, 2016-08-12 10:07:33]

Thanks a lot!

nasko@, PTAL.

https://codereview.chromium.org/2185823005/diff/80001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2185823005/diff/80001/content/renderer/render...
content/renderer/render_view_impl.cc:639: class CC_EXPORT AlwaysDrawSwapPromise
: public cc::SwapPromise {
On 2016/08/11 21:59:00, danakj wrote:
> drop the CC_EXPORT
Fixed.

https://codereview.chromium.org/2185823005/diff/80001/content/renderer/render...
content/renderer/render_view_impl.cc:1500: base::WrapUnique(new
AlwaysDrawSwapPromise(latency_info)));
On 2016/08/11 21:59:00, danakj wrote:
> one last nit: prefer base::MakeUnique<AlwaysDrawSwapPromise>(...) over
> base::WrapUnique(new AlwaysDrawSwapPromise(...))
Fixed.

[svartmetal, 2016-08-12 14:45:59]

The CQ bit was checked by svartmetal@yandex-team.ru

[svartmetal, 2016-08-12 14:46:00]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2185823005/#ps100001
(title: "Remove redundant CC_EXPORT")

[commit-bot: I haz the power, 2016-08-12 14:46:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 14:46:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-12 14:46:16]

Your CL was about to rely on recently removed CQ feature(s):
 * master.tryserver.blink builder linux_blink_rel was renamed to
linux_precise_blink_rel (http://crbug.com/590036)

[svartmetal, 2016-08-12 16:38:27]

Description was changed from

==========
Make RenderViewImpl::OnForceRedraw more robust

Before this change, RenderWidgetHostImpl::GetSnapshotFromBrowser could
spuriously fail (and callback might never be fired). The nature of
unresponsiveness is in fact that scheduled draw and swap can
be aborted by cc::Scheduler and, as a result, we have active tree
deletion with all ui::LatencyInfo data sent by browser.

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

to

==========
Make RenderViewImpl::OnForceRedraw more robust

Before this change, RenderWidgetHostImpl::GetSnapshotFromBrowser could
spuriously fail (and callback might never be fired). The nature of
unresponsiveness is in fact that scheduled draw and swap can
be aborted by cc::Scheduler and, as a result, we have active tree
deletion with all ui::LatencyInfo data sent by browser.

BUG=637066
==========

[svartmetal, 2016-08-12 16:38:36]

The CQ bit was checked by svartmetal@yandex-team.ru

[commit-bot: I haz the power, 2016-08-12 16:38:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 16:41:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-12 16:41:32]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[svartmetal, 2016-08-12 19:54:03]

Description was changed from

==========
Make RenderViewImpl::OnForceRedraw more robust

Before this change, RenderWidgetHostImpl::GetSnapshotFromBrowser could
spuriously fail (and callback might never be fired). The nature of
unresponsiveness is in fact that scheduled draw and swap can
be aborted by cc::Scheduler and, as a result, we have active tree
deletion with all ui::LatencyInfo data sent by browser.

BUG=637066
==========

to

==========
Make RenderViewImpl::OnForceRedraw more robust

Before this change, RenderWidgetHostImpl::GetSnapshotFromBrowser could
spuriously fail (and callback might never be fired). The nature of
unresponsiveness is in fact that scheduled draw and swap can
be aborted by cc::Scheduler and, as a result, we have active tree
deletion with all ui::LatencyInfo data sent by browser.

BUG=637066
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[nasko, 2016-08-12 20:59:36]

IPC LGTM

[svartmetal, 2016-08-12 21:11:19]

The CQ bit was checked by svartmetal@yandex-team.ru

[svartmetal, 2016-08-12 21:11:19]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2185823005/#ps120001
(title: "Rebased")

[commit-bot: I haz the power, 2016-08-12 21:11:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 21:22:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-12 21:22:14]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[svartmetal, 2016-08-13 09:03:34]

The CQ bit was checked by svartmetal@yandex-team.ru

[svartmetal, 2016-08-13 09:03:35]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
nasko@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2185823005/#ps140001
(title: "Fix compilation")

[commit-bot: I haz the power, 2016-08-13 09:03:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-13 09:41:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-13 09:41:07]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[svartmetal, 2016-08-13 10:40:35]

The CQ bit was checked by svartmetal@yandex-team.ru

[svartmetal, 2016-08-13 10:40:35]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
nasko@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2185823005/#ps160001
(title: "Fix compilation again")

[commit-bot: I haz the power, 2016-08-13 10:40:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-13 11:47:54]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-08-13 11:49:35]

Description was changed from

==========
Make RenderViewImpl::OnForceRedraw more robust

Before this change, RenderWidgetHostImpl::GetSnapshotFromBrowser could
spuriously fail (and callback might never be fired). The nature of
unresponsiveness is in fact that scheduled draw and swap can
be aborted by cc::Scheduler and, as a result, we have active tree
deletion with all ui::LatencyInfo data sent by browser.

BUG=637066
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Make RenderViewImpl::OnForceRedraw more robust

Before this change, RenderWidgetHostImpl::GetSnapshotFromBrowser could
spuriously fail (and callback might never be fired). The nature of
unresponsiveness is in fact that scheduled draw and swap can
be aborted by cc::Scheduler and, as a result, we have active tree
deletion with all ui::LatencyInfo data sent by browser.

BUG=637066
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/751af1f9be588e97d6c37628499c421322797e18
Cr-Commit-Position: refs/heads/master@{#411892}
==========

[commit-bot: I haz the power, 2016-08-13 11:49:37]

Patchset 9 (id:??) landed as
https://crrev.com/751af1f9be588e97d6c37628499c421322797e18
Cr-Commit-Position: refs/heads/master@{#411892}