cc: Remove DelegatingRendererLayerClient

cc: Remove DelegatingRendererLayerClient

RenderWidgetHostViewAndroid no longer subclasses DelegatingRendererLayerClient.
As it was the only use case for DelegatingRendererLayerClient, we can now remove
this class.

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

[powei, 2013-12-02 22:26:46]

PTAL.  thanks!

https://codereview.chromium.org/93463002/diff/40001/cc/layers/delegated_rende...
File cc/layers/delegated_renderer_layer.cc (left):

https://codereview.chromium.org/93463002/diff/40001/cc/layers/delegated_rende...
cc/layers/delegated_renderer_layer.cc:92: needs_push_properties_ = true;
@danakj. so I probably cannot remove this, but should I at least remove the
comment?

[danakj, 2013-12-02 22:41:29]

https://codereview.chromium.org/93463002/diff/40001/cc/layers/delegated_rende...
File cc/layers/delegated_renderer_layer.cc (left):

https://codereview.chromium.org/93463002/diff/40001/cc/layers/delegated_rende...
cc/layers/delegated_renderer_layer.cc:92: needs_push_properties_ = true;
On 2013/12/02 22:26:47, powei wrote:
> @danakj. so I probably cannot remove this, but should I at least remove the
> comment?

We should work on removing this, the TODO should remain as long as the bug is
open. If your change does not allow us to remove this, can you update the
comment to say why?

[piman, 2013-12-03 01:46:40]

LGTM overall, modulo the needs_push_properties_ thing to figure out.

https://codereview.chromium.org/93463002/diff/40001/cc/layers/delegated_rende...
File cc/layers/delegated_renderer_layer.cc (left):

https://codereview.chromium.org/93463002/diff/40001/cc/layers/delegated_rende...
cc/layers/delegated_renderer_layer.cc:92: needs_push_properties_ = true;
On 2013/12/02 22:41:30, danakj wrote:
> On 2013/12/02 22:26:47, powei wrote:
> > @danakj. so I probably cannot remove this, but should I at least remove the
> > comment?
> 
> We should work on removing this, the TODO should remain as long as the bug is
> open. If your change does not allow us to remove this, can you update the
> comment to say why?

I don't see a reason why a needs_push_properties is needed any more, with the
DFResourceCollection logic, I think it should go?
If there is a reason, make sure to update the TODO too, because
DidCommitFrameData doesn't exist any more.

[powei, 2013-12-03 02:11:11]

https://codereview.chromium.org/93463002/diff/40001/cc/layers/delegated_rende...
File cc/layers/delegated_renderer_layer.cc (left):

https://codereview.chromium.org/93463002/diff/40001/cc/layers/delegated_rende...
cc/layers/delegated_renderer_layer.cc:92: needs_push_properties_ = true;
On 2013/12/03 01:46:41, piman wrote:
> On 2013/12/02 22:41:30, danakj wrote:
> > On 2013/12/02 22:26:47, powei wrote:
> > > @danakj. so I probably cannot remove this, but should I at least remove
the
> > > comment?
> > 
> > We should work on removing this, the TODO should remain as long as the bug
is
> > open. If your change does not allow us to remove this, can you update the
> > comment to say why?
> 
> I don't see a reason why a needs_push_properties is needed any more, with the
> DFResourceCollection logic, I think it should go?
> If there is a reason, make sure to update the TODO too, because
> DidCommitFrameData doesn't exist any more.

crbug.com/259090 has been merged into crbug.com/263069, which is marked as
fixed.  However, when I tried to remove needs_push_properties_, the following
cc_unittests fail.  The last test crashes out on a failed dcheck.  Maybe these
tests are assuming needs_push_properties_ is set true?

LayerTreeHostDelegatedTestCreateChildId
LayerTreeHostDelegatedTestLayerUsesFrameDamage
LayerTreeHostDelegatedTestReturnUnusedResources
LayerTreeHostDelegatedTestReusedResources
LayerTreeHostDelegatedTestFrameBeforeAck
LayerTreeHostDelegatedTestFrameBeforeTakeResources
LayerTreeHostDelegatedTestBadFrame
LayerTreeHostDelegatedTestDontLeakResource
LayerTreeHostDelegatedTestCommitWithoutTake
DelegatedFrameIsActivatedDuringCommit
LayerTreeHostDelegatedTestTwoImplLayers
LayerTreeHostDelegatedTestRemoveAndAddToTree

[danakj, 2013-12-03 05:59:37]

Probably DRLayer is just not calling SetNeedsPushProperties when it has a
new frame to push. You can update the TODO to state this and we can do it
as a follow uo
On Dec 2, 2013 9:11 PM, <powei@chromium.org> wrote:

>
> https://codereview.chromium.org/93463002/diff/40001/cc/
> layers/delegated_renderer_layer.cc
> File cc/layers/delegated_renderer_layer.cc (left):
>
> https://codereview.chromium.org/93463002/diff/40001/cc/
> layers/delegated_renderer_layer.cc#oldcode92
> cc/layers/delegated_renderer_layer.cc:92: needs_push_properties_ = true;
> On 2013/12/03 01:46:41, piman wrote:
>
>> On 2013/12/02 22:41:30, danakj wrote:
>> > On 2013/12/02 22:26:47, powei wrote:
>> > > @danakj. so I probably cannot remove this, but should I at least
>>
> remove the
>
>> > > comment?
>> >
>> > We should work on removing this, the TODO should remain as long as
>>
> the bug is
>
>> > open. If your change does not allow us to remove this, can you
>>
> update the
>
>> > comment to say why?
>>
>
>  I don't see a reason why a needs_push_properties is needed any more,
>>
> with the
>
>> DFResourceCollection logic, I think it should go?
>> If there is a reason, make sure to update the TODO too, because
>> DidCommitFrameData doesn't exist any more.
>>
>
> crbug.com/259090 has been merged into crbug.com/263069, which is marked
> as fixed.  However, when I tried to remove needs_push_properties_, the
> following cc_unittests fail.  The last test crashes out on a failed
> dcheck.  Maybe these tests are assuming needs_push_properties_ is set
> true?
>
> LayerTreeHostDelegatedTestCreateChildId
> LayerTreeHostDelegatedTestLayerUsesFrameDamage
> LayerTreeHostDelegatedTestReturnUnusedResources
> LayerTreeHostDelegatedTestReusedResources
> LayerTreeHostDelegatedTestFrameBeforeAck
> LayerTreeHostDelegatedTestFrameBeforeTakeResources
> LayerTreeHostDelegatedTestBadFrame
> LayerTreeHostDelegatedTestDontLeakResource
> LayerTreeHostDelegatedTestCommitWithoutTake
> DelegatedFrameIsActivatedDuringCommit
> LayerTreeHostDelegatedTestTwoImplLayers
> LayerTreeHostDelegatedTestRemoveAndAddToTree
>
> https://codereview.chromium.org/93463002/
>

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

[danakj, 2013-12-03 18:28:27]

LGTM

https://codereview.chromium.org/93463002/diff/60001/cc/layers/delegated_rende...
File cc/layers/delegated_renderer_layer.cc (right):

https://codereview.chromium.org/93463002/diff/60001/cc/layers/delegated_rende...
cc/layers/delegated_renderer_layer.cc:81: // breaks unit tests.
Can you keep crbug.com/323049 in the comment?

[powei, 2013-12-03 18:29:33]

On 2013/12/03 05:59:37, danakj wrote:
> Probably DRLayer is just not calling SetNeedsPushProperties when it has a
> new frame to push. You can update the TODO to state this and we can do it
> as a follow uo
> On Dec 2, 2013 9:11 PM, <mailto:powei@chromium.org> wrote:
> 

Done

> >
> > https://codereview.chromium.org/93463002/diff/40001/cc/
> > layers/delegated_renderer_layer.cc
> > File cc/layers/delegated_renderer_layer.cc (left):
> >
> > https://codereview.chromium.org/93463002/diff/40001/cc/
> > layers/delegated_renderer_layer.cc#oldcode92
> > cc/layers/delegated_renderer_layer.cc:92: needs_push_properties_ = true;
> > On 2013/12/03 01:46:41, piman wrote:
> >
> >> On 2013/12/02 22:41:30, danakj wrote:
> >> > On 2013/12/02 22:26:47, powei wrote:
> >> > > @danakj. so I probably cannot remove this, but should I at least
> >>
> > remove the
> >
> >> > > comment?
> >> >
> >> > We should work on removing this, the TODO should remain as long as
> >>
> > the bug is
> >
> >> > open. If your change does not allow us to remove this, can you
> >>
> > update the
> >
> >> > comment to say why?
> >>
> >
> >  I don't see a reason why a needs_push_properties is needed any more,
> >>
> > with the
> >
> >> DFResourceCollection logic, I think it should go?
> >> If there is a reason, make sure to update the TODO too, because
> >> DidCommitFrameData doesn't exist any more.
> >>
> >
> > crbug.com/259090 has been merged into crbug.com/263069, which is marked
> > as fixed.  However, when I tried to remove needs_push_properties_, the
> > following cc_unittests fail.  The last test crashes out on a failed
> > dcheck.  Maybe these tests are assuming needs_push_properties_ is set
> > true?
> >
> > LayerTreeHostDelegatedTestCreateChildId
> > LayerTreeHostDelegatedTestLayerUsesFrameDamage
> > LayerTreeHostDelegatedTestReturnUnusedResources
> > LayerTreeHostDelegatedTestReusedResources
> > LayerTreeHostDelegatedTestFrameBeforeAck
> > LayerTreeHostDelegatedTestFrameBeforeTakeResources
> > LayerTreeHostDelegatedTestBadFrame
> > LayerTreeHostDelegatedTestDontLeakResource
> > LayerTreeHostDelegatedTestCommitWithoutTake
> > DelegatedFrameIsActivatedDuringCommit
> > LayerTreeHostDelegatedTestTwoImplLayers
> > LayerTreeHostDelegatedTestRemoveAndAddToTree
> >
> > https://codereview.chromium.org/93463002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[powei, 2013-12-03 18:38:52]

https://codereview.chromium.org/93463002/diff/60001/cc/layers/delegated_rende...
File cc/layers/delegated_renderer_layer.cc (right):

https://codereview.chromium.org/93463002/diff/60001/cc/layers/delegated_rende...
cc/layers/delegated_renderer_layer.cc:81: // breaks unit tests.
On 2013/12/03 18:28:27, danakj wrote:
> Can you keep crbug.com/323049 in the comment?

Done.  But do you really want crbug.com/323049 and not the original
crbug.com/259090?

[danakj, 2013-12-03 18:43:21]

On Tue, Dec 3, 2013 at 1:38 PM, <powei@chromium.org> wrote:

>
> https://codereview.chromium.org/93463002/diff/60001/cc/
> layers/delegated_renderer_layer.cc
> File cc/layers/delegated_renderer_layer.cc (right):
>
> https://codereview.chromium.org/93463002/diff/60001/cc/
> layers/delegated_renderer_layer.cc#newcode81
> cc/layers/delegated_renderer_layer.cc:81: // breaks unit tests.
> On 2013/12/03 18:28:27, danakj wrote:
>
>> Can you keep crbug.com/323049 in the comment?
>>
>
> Done.  But do you really want crbug.com/323049 and not the original
> crbug.com/259090?
>

Ohh.. too many bugs. I did mean 259090 sorry.


>
> https://codereview.chromium.org/93463002/
>

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

[powei, 2013-12-03 19:33:48]

On 2013/12/03 18:43:21, danakj wrote:
> On Tue, Dec 3, 2013 at 1:38 PM, <mailto:powei@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/93463002/diff/60001/cc/
> > layers/delegated_renderer_layer.cc
> > File cc/layers/delegated_renderer_layer.cc (right):
> >
> > https://codereview.chromium.org/93463002/diff/60001/cc/
> > layers/delegated_renderer_layer.cc#newcode81
> > cc/layers/delegated_renderer_layer.cc:81: // breaks unit tests.
> > On 2013/12/03 18:28:27, danakj wrote:
> >
> >> Can you keep crbug.com/323049 in the comment?
> >>
> >
> > Done.  But do you really want crbug.com/323049 and not the original
> > crbug.com/259090?
> >
> 
> Ohh.. too many bugs. I did mean 259090 sorry.
> 

Done.  will try to commit now.
> 
> >
> > https://codereview.chromium.org/93463002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2013-12-03 19:34:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/powei@chromium.org/93463002/100001

[commit-bot: I haz the power, 2013-12-03 23:46:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/powei@chromium.org/93463002/100001

[commit-bot: I haz the power, 2013-12-04 00:56:53]

Change committed as 238508