Removed QuadSink and MockQuadCuller

Removed QuadSink and MockQuadCuller

Since QuadSink does not provide more functionatility than passing
through to RenderPass and OcclusionTracker, it can be torn down and
removed. Same is for MockQuadCuller.

This CL follows 302603006, 306683002, 309493002.

BUG=344962
TEST=cc_unittests

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

[enne (OOO), 2014-05-30 21:37:42]

lgtm

https://codereview.chromium.org/308193003/diff/20001/cc/layers/heads_up_displ...
File cc/layers/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/308193003/diff/20001/cc/layers/heads_up_displ...
cc/layers/heads_up_display_layer_impl.cc:598: SkPaint* paint,
Oops.  Thanks for this.

https://codereview.chromium.org/308193003/diff/20001/cc/test/mock_occlusion_t...
File cc/test/mock_occlusion_tracker.h (right):

https://codereview.chromium.org/308193003/diff/20001/cc/test/mock_occlusion_t...
cc/test/mock_occlusion_tracker.h:45: virtual gfx::Rect UnoccludedContentRect(
style nit: don't inline virtuals

Also, isn't this the same as the base version in OcclusionTracker at this point?
Can this be removed and unvirtualized?

https://codereview.chromium.org/308193003/diff/20001/cc/test/mock_occlusion_t...
cc/test/mock_occlusion_tracker.h:49: return this->UnoccludedContentRectHelper(
style nit: unnecessary this-> (here and elsewhere and in other patches)

[weiliangc, 2014-05-30 21:46:06]

https://codereview.chromium.org/308193003/diff/20001/cc/test/mock_occlusion_t...
File cc/test/mock_occlusion_tracker.h (right):

https://codereview.chromium.org/308193003/diff/20001/cc/test/mock_occlusion_t...
cc/test/mock_occlusion_tracker.h:45: virtual gfx::Rect UnoccludedContentRect(
On 2014/05/30 21:37:43, enne wrote:
> style nit: don't inline virtuals
> 
> Also, isn't this the same as the base version in OcclusionTracker at this
point?
> Can this be removed and unvirtualized?

In unittest the OcclusionTracker and LayerImpls aren't set up correctly so it
fails on the DCHECKs in OcclusionTracker.

I put all the actual calculation in a helper function. The normal
OcclusionTracker still DCHECKs, and MockOT just skips to calculation.

[weiliangc, 2014-05-30 22:04:56]

https://codereview.chromium.org/308193003/diff/20001/cc/test/mock_occlusion_t...
File cc/test/mock_occlusion_tracker.h (right):

https://codereview.chromium.org/308193003/diff/20001/cc/test/mock_occlusion_t...
cc/test/mock_occlusion_tracker.h:49: return this->UnoccludedContentRectHelper(
On 2014/05/30 21:37:43, enne wrote:
> style nit: unnecessary this-> (here and elsewhere and in other patches)

I think it is either this-> or OcclusionTracker<LayerType>::, because template
derived class is a pain.

[enne (OOO), 2014-05-30 22:05:58]

https://codereview.chromium.org/308193003/diff/20001/cc/test/mock_occlusion_t...
File cc/test/mock_occlusion_tracker.h (right):

https://codereview.chromium.org/308193003/diff/20001/cc/test/mock_occlusion_t...
cc/test/mock_occlusion_tracker.h:49: return this->UnoccludedContentRectHelper(
On 2014/05/30 22:04:57, weiliangc wrote:
> On 2014/05/30 21:37:43, enne wrote:
> > style nit: unnecessary this-> (here and elsewhere and in other patches)
> 
> I think it is either this-> or OcclusionTracker<LayerType>::, because template
> derived class is a pain.

Templates /o\

[enne (OOO), 2014-05-30 22:11:09]

https://codereview.chromium.org/308193003/diff/20001/cc/test/mock_occlusion_t...
File cc/test/mock_occlusion_tracker.h (right):

https://codereview.chromium.org/308193003/diff/20001/cc/test/mock_occlusion_t...
cc/test/mock_occlusion_tracker.h:45: virtual gfx::Rect UnoccludedContentRect(
On 2014/05/30 21:46:07, weiliangc wrote:
> On 2014/05/30 21:37:43, enne wrote:
> > style nit: don't inline virtuals
> > 
> > Also, isn't this the same as the base version in OcclusionTracker at this
> point?
> > Can this be removed and unvirtualized?
> 
> In unittest the OcclusionTracker and LayerImpls aren't set up correctly so it
> fails on the DCHECKs in OcclusionTracker.
> 
> I put all the actual calculation in a helper function. The normal
> OcclusionTracker still DCHECKs, and MockOT just skips to calculation.

Hmm, ok.  I'm always saddened by virtual-for-testing sorts of things from a
premature performance optimization standpoint.

How hard would it be to set up tests to not DCHECK?

[danakj, 2014-05-31 23:23:39]

On Sat, May 31, 2014 at 12:11 AM, <enne@chromium.org> wrote:

>
> https://codereview.chromium.org/308193003/diff/20001/cc/
> test/mock_occlusion_tracker.h
> File cc/test/mock_occlusion_tracker.h (right):
>
> https://codereview.chromium.org/308193003/diff/20001/cc/
> test/mock_occlusion_tracker.h#newcode45
> cc/test/mock_occlusion_tracker.h:45: virtual gfx::Rect
> UnoccludedContentRect(
> On 2014/05/30 21:46:07, weiliangc wrote:
>
>> On 2014/05/30 21:37:43, enne wrote:
>> > style nit: don't inline virtuals
>> >
>> > Also, isn't this the same as the base version in OcclusionTracker at
>>
> this
>
>> point?
>> > Can this be removed and unvirtualized?
>>
>
>  In unittest the OcclusionTracker and LayerImpls aren't set up
>>
> correctly so it
>
>> fails on the DCHECKs in OcclusionTracker.
>>
>
>  I put all the actual calculation in a helper function. The normal
>> OcclusionTracker still DCHECKs, and MockOT just skips to calculation.
>>
>
> Hmm, ok.  I'm always saddened by virtual-for-testing sorts of things
> from a premature performance optimization standpoint.
>
> How hard would it be to set up tests to not DCHECK?
>

Or perhaps to replace the DCHECKs with some unit tests? I feel like the
occlusion tracking code paths are a bit DCHECK-happy..

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

[weiliangc, 2014-06-02 17:11:21]

On 2014/05/31 23:23:39, danakj (OOO_back_june_6) wrote:
> On Sat, May 31, 2014 at 12:11 AM, <mailto:enne@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/308193003/diff/20001/cc/
> > test/mock_occlusion_tracker.h
> > File cc/test/mock_occlusion_tracker.h (right):
> >
> > https://codereview.chromium.org/308193003/diff/20001/cc/
> > test/mock_occlusion_tracker.h#newcode45
> > cc/test/mock_occlusion_tracker.h:45: virtual gfx::Rect
> > UnoccludedContentRect(
> > On 2014/05/30 21:46:07, weiliangc wrote:
> >
> >> On 2014/05/30 21:37:43, enne wrote:
> >> > style nit: don't inline virtuals
> >> >
> >> > Also, isn't this the same as the base version in OcclusionTracker at
> >>
> > this
> >
> >> point?
> >> > Can this be removed and unvirtualized?
> >>
> >
> >  In unittest the OcclusionTracker and LayerImpls aren't set up
> >>
> > correctly so it
> >
> >> fails on the DCHECKs in OcclusionTracker.
> >>
> >
> >  I put all the actual calculation in a helper function. The normal
> >> OcclusionTracker still DCHECKs, and MockOT just skips to calculation.
> >>
> >
> > Hmm, ok.  I'm always saddened by virtual-for-testing sorts of things
> > from a premature performance optimization standpoint.
> >
> > How hard would it be to set up tests to not DCHECK?
> >


Unit tests does not have a separate code path, it still goes through AppendQuads
in LayerImpl.

Unit tests used to call functions in MockQuadCuller that just does the
calculation without any checking.

OcclusionTracker functions used to check for NULL LayerImpl* and assume it's
testing when it is NULL. But now there is non NULL test case in layer test
common, so checking input is NULL is out.

Faking enough data to pass DCHECKs doesn't seem trivial. (Fails at LayerImpl's
render_target is not itself for now.) From what I can see to set up Occlusion
Tracker, EnterLayer and LeaverLayer function needs to be called, which only take
in LayerIterator from RSLL.

I can't think of an easy way to make sure it's testing, or faking the testing
data.

On a side note, I don't understand why virtual-for-testing is a premature
performance optimization?

> >
> 
> Or perhaps to replace the DCHECKs with some unit tests? I feel like the
> occlusion tracking code paths are a bit DCHECK-happy..

Do you mean to make unit test so it proves DCHECK won't be triggered in normal
use case?

Would the current occlusion tracker unit test suffice?

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

[danakj, 2014-06-02 17:16:00]

On Mon, Jun 2, 2014 at 7:11 PM, <weiliangc@chromium.org> wrote:

> On 2014/05/31 23:23:39, danakj (OOO_back_june_6) wrote:
>
>> On Sat, May 31, 2014 at 12:11 AM, <mailto:enne@chromium.org> wrote:
>>
>
>  >
>> > https://codereview.chromium.org/308193003/diff/20001/cc/
>> > test/mock_occlusion_tracker.h
>> > File cc/test/mock_occlusion_tracker.h (right):
>> >
>> > https://codereview.chromium.org/308193003/diff/20001/cc/
>>
>> > test/mock_occlusion_tracker.h#newcode45
>> > cc/test/mock_occlusion_tracker.h:45: virtual gfx::Rect
>> > UnoccludedContentRect(
>> > On 2014/05/30 21:46:07, weiliangc wrote:
>> >
>> >> On 2014/05/30 21:37:43, enne wrote:
>> >> > style nit: don't inline virtuals
>> >> >
>> >> > Also, isn't this the same as the base version in OcclusionTracker at
>> >>
>> > this
>> >
>> >> point?
>> >> > Can this be removed and unvirtualized?
>> >>
>> >
>> >  In unittest the OcclusionTracker and LayerImpls aren't set up
>> >>
>> > correctly so it
>> >
>> >> fails on the DCHECKs in OcclusionTracker.
>> >>
>> >
>> >  I put all the actual calculation in a helper function. The normal
>> >> OcclusionTracker still DCHECKs, and MockOT just skips to calculation.
>> >>
>> >
>> > Hmm, ok.  I'm always saddened by virtual-for-testing sorts of things
>> > from a premature performance optimization standpoint.
>> >
>> > How hard would it be to set up tests to not DCHECK?
>> >
>>
>
>
> Unit tests does not have a separate code path, it still goes through
> AppendQuads
> in LayerImpl.
>
> Unit tests used to call functions in MockQuadCuller that just does the
> calculation without any checking.
>
> OcclusionTracker functions used to check for NULL LayerImpl* and assume
> it's
> testing when it is NULL. But now there is non NULL test case in layer test
> common, so checking input is NULL is out.
>
> Faking enough data to pass DCHECKs doesn't seem trivial. (Fails at
> LayerImpl's
> render_target is not itself for now.) From what I can see to set up
> Occlusion
> Tracker, EnterLayer and LeaverLayer function needs to be called, which
> only take
> in LayerIterator from RSLL.
>
> I can't think of an easy way to make sure it's testing, or faking the
> testing
> data.
>
> On a side note, I don't understand why virtual-for-testing is a premature
> performance optimization?
>
>
>  >
>>
>
>  Or perhaps to replace the DCHECKs with some unit tests? I feel like the
>> occlusion tracking code paths are a bit DCHECK-happy..
>>
>
> Do you mean to make unit test so it proves DCHECK won't be triggered in
> normal
> use case?
>

Ya, I mean like, a unit test that shows we guard conditions well, or
produce an RSLL right, or whatnot, to never cause this DCHECK to trigger.

If there's no obvious guards/behaviours to check anywhere, we could
consider removing it. Just don't wanna leave ourselves exposed.


>
> Would the current occlusion tracker unit test suffice?
>

I don't know. Since they don't cause this DCHECK to fire, I'm not sure they
execute all possible paths/etc. I think this just needs some code
reading/reasoning.


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

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

[weiliangc, 2014-06-23 04:46:42]

The CQ bit was checked by weiliangc@chromium.org

[commit-bot: I haz the power, 2014-06-23 04:47:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/weiliangc@chromium.org/308193003/160001

[commit-bot: I haz the power, 2014-06-23 05:50:40]

Change committed as 279050