Unifies LayerTreeHost::SetNeedsUpdateLayers and SetNeedsAnimate -- V2

Unifies LayerTreeHost::SetNeedsUpdateLayers and SetNeedsAnimate -- V2

[2/2] Unifies LayerTreeHost::SetNeedsUpdateLayers and SetNeedsAnimate

They basically do the same thing except that SetNeedsAnimate makes the next
commit non-cancellable. However there is really no reason why SetNeedsAnimate
need to enforce a commit even if no tiles are updated and no layer properties
changed.

SetNeedsAnimate is thus merged into SetNeedsUpdateLayers. The proper use of
it is when there are potential layout/tile changes, we can use it to defer
calculation until the next frame. A commit will be scheduled but can be
cancelled if no updates are needed after calculation.

This part of the patch changes code behavior slightly.
SingleThreadProxy::SetNeedsUpdateLayers was originally implemented as
RenderWidget::ScheduleComposite but now it is RenderWidget::ScheduleAnimation.
ThreadProxy::SetNeedsAnimate was non-cancellable but is now cancellable.

[1/2] Cleanup RenderWidget::scheduleComposite/scheduleAnimation

scheduleComposite has been renamed to ScheduleComposite as it is no longer
a part of WebWidgetClient API.

scheduleAnimation has been renamed to ScheduleAnimation. The semantics is to
schedule a composite and also (potentially) animating WebWidget.

A new WebWidgetClient API scheduleUpdate has been added, to replace the old
scheduleAnimation. The semantics is to notify the embedder that something in
the WebWidget may change in 0 seconds. (i.e. it is allowed to be called
during a redraw, in such case another redraw will be scheduled after frame
delay.

This part of the patch should not change code behavior.

BUG=316929
R=danakj,jamesr,jochen

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

[trchen, 2014-01-11 00:47:52]

Hello reviewers,
This CL is a reincarnation of previous reverted change
https://codereview.chromium.org/68893031/

Most parts of the CL are identical to the previous one. The only difference is
to fix the layout test image failures.

The problem with the previous patch is that some internal code of RenderWidget
actually uses scheduleComposite/scheduleAnimation from its WebWidgetClient
interface. The two functions became protected internal functions. However the
WebTestRunner only hooks the WebWidgetClient interaface, thus some
RenderWidget-initiated composite were ignored by the WebTestRunner.

I feel a little bit uncomfortable that WebTestRunner needs to know too much
detail about the base class it wraps(RenderWidget). Should we un-template it and
inherit RenderWidget directly instead?

[trchen, 2014-01-11 00:48:54]

Sorry I meant WebTestProxy not WebTestRunner.

[jamesr, 2014-01-11 00:52:38]

On 2014/01/11 00:47:52, trchen wrote:
> I feel a little bit uncomfortable that WebTestRunner needs to know too much
> detail about the base class it wraps(RenderWidget). Should we un-template it
and
> inherit RenderWidget directly instead?

This is more a question for Jochen, but I believe that the WebTestRunner used to
be used to inherit from two different base classes (content::RenderWidget and
something in third_party/WebKit/.../DumpRenderTree) and used to live in the
blink repo instead of the chromium repo.  Now that it lives in the chromium repo
and always inherits from one base class I think it probably should be
de-templatized or maybe injected in another way but I don't know all of the
details.

[trchen, 2014-01-11 00:55:08]

On 2014/01/11 00:52:38, jamesr wrote:
> On 2014/01/11 00:47:52, trchen wrote:
> > I feel a little bit uncomfortable that WebTestRunner needs to know too much
> > detail about the base class it wraps(RenderWidget). Should we un-template it
> and
> > inherit RenderWidget directly instead?
> 
> This is more a question for Jochen, but I believe that the WebTestRunner used
to
> be used to inherit from two different base classes (content::RenderWidget and
> something in third_party/WebKit/.../DumpRenderTree) and used to live in the
> blink repo instead of the chromium repo.  Now that it lives in the chromium
repo
> and always inherits from one base class I think it probably should be
> de-templatized or maybe injected in another way but I don't know all of the
> details.

+cc jochen

[jamesr, 2014-01-11 02:52:15]

content/renderer/ lgtm

[jochen (gone - plz use gerrit), 2014-01-13 14:55:39]

test runner is still a content embedder and as such can't inherit from
RenderViewImpl

[danakj, 2014-01-13 16:43:07]

sounds like cc/ didn't change so LGTM

[trchen, 2014-01-13 23:37:11]

On 2014/01/13 14:55:39, jochen wrote:
> test runner is still a content embedder and as such can't inherit from
> RenderViewImpl

Sorry I meant WebTestProxy. Currently we only use WebTestProxy<RenderViewImpl,
RenderViewImplParams*>, so it always inherits RenderViewImpl, and it relies on
the inner workings of RenderViewImpl anyway. Should we just de-templatize it?

Do you think the change in
content/shell/renderer/test_runner/WebTestProxy(.cpp|.h) looks okay to you? If
not, what would you recommend me to do? Thanks!

[jochen (gone - plz use gerrit), 2014-01-14 06:14:28]

On 2014/01/13 23:37:11, trchen wrote:
> On 2014/01/13 14:55:39, jochen wrote:
> > test runner is still a content embedder and as such can't inherit from
> > RenderViewImpl
> 
> Sorry I meant WebTestProxy. Currently we only use WebTestProxy<RenderViewImpl,
> RenderViewImplParams*>, so it always inherits RenderViewImpl, and it relies on
> the inner workings of RenderViewImpl anyway. Should we just de-templatize it?
> 
> Do you think the change in
> content/shell/renderer/test_runner/WebTestProxy(.cpp|.h) looks okay to you? If
> not, what would you recommend me to do? Thanks!

Yes, WebTestProxy is part of the test runner library and it can't inherit from
RenderViewImpl directly.

This composite calls are actually the only case where there's a tight coupling.
For everything else, the WebTestProxy only depends on the WebViewClient /
WebFrameClient API from blink.

Anyway, the composite calls are a special case, so lgtm

[commit-bot: I haz the power, 2014-01-15 02:35:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/133263004/1

[commit-bot: I haz the power, 2014-01-15 17:37:14]

Retried try job too often on win for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2014-01-15 22:52:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/133263004/500002

[commit-bot: I haz the power, 2014-01-16 02:26:48]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-16 02:30:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/133263004/500002

[commit-bot: I haz the power, 2014-01-16 15:34:26]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-17 02:04:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/133263004/500002

[commit-bot: I haz the power, 2014-01-17 06:43:22]

Change committed as 245445

[danakj, 2014-02-24 18:47:32]

I guess this got reverted (https://codereview.chromium.org/141833002), and
skyostil@ is trying to do something similar with SetNeedsAnimate over here:
https://codereview.chromium.org/178123003/

Are you planning to reland this?

[trchen, 2014-02-24 22:00:47]

On 2014/02/24 18:47:32, danakj wrote:
> I guess this got reverted (https://codereview.chromium.org/141833002), and
> skyostil@ is trying to do something similar with SetNeedsAnimate over here:
> https://codereview.chromium.org/178123003/
> 
> Are you planning to reland this?

Yes when I have spare time to trace and fix the unit test issue, but I have
other bad bugs to fix for now. I will rebase after his change.