cc: Remove TiledLayer mask DCHECK for 0 tiles

cc: Remove TiledLayer mask DCHECK for 0 tiles

It seems that this is firing in practice, so don't DCHECK it.  Instead,
just return no resource at all since that mask layer can't draw.  This
will behave identically to the case where there is no mask at all.

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

[danakj, 2014-01-16 16:27:20]

LGTM with a simple test
On Jan 15, 2014 7:59 PM, <enne@chromium.org> wrote:

> Reviewers: danakj,
>
> Description:
> cc: Remove TiledLayer mask DCHECK for 0 tiles
>
> It seems that this is firing in practice, so don't DCHECK it.  Instead,
> just return no resource at all since that mask layer can't draw.  This
> will behave identically to the case where there is no mask at all.
>
> BUG=165723
>
> Please review this at https://codereview.chromium.org/136143004/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files (+6, -0 lines):
>   M cc/layers/tiled_layer_impl.cc
>
>
> Index: cc/layers/tiled_layer_impl.cc
> diff --git a/cc/layers/tiled_layer_impl.cc b/cc/layers/tiled_layer_impl.cc
> index aea456727e33e9c6a0fd417a78a4c11d42f0b885..
> d2767cf02eff991ada2d1520f78b7e31d63f9fad 100644
> --- a/cc/layers/tiled_layer_impl.cc
> +++ b/cc/layers/tiled_layer_impl.cc
> @@ -54,6 +54,12 @@ TiledLayerImpl::~TiledLayerImpl() {
>  ResourceProvider::ResourceId TiledLayerImpl::ContentsResourceId() const {
>    // This function is only valid for single texture layers, e.g. masks.
>    DCHECK(tiler_);
> +  // It's possible the mask layer is created but has no size or otherwise
> +  // can't draw.
> +  if (tiler_->num_tiles_x() == 0 || tiler_->num_tiles_y() == 0)
> +    return 0;
> +
> +  // Any other number of tiles other than 0 or 1 is incorrect for masks.
>    DCHECK_EQ(tiler_->num_tiles_x(), 1);
>    DCHECK_EQ(tiler_->num_tiles_y(), 1);
>
>
>
>

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

[enne (OOO), 2014-01-16 19:28:56]

What am I testing? "This doesn't DCHECK" is a really poor test.

[danakj, 2014-01-16 19:30:58]

On 2014/01/16 19:28:56, enne wrote:
> What am I testing? "This doesn't DCHECK" is a really poor test.

Just that the method returns a 0? DCHECK implies we couldn't have had a test for
this code path before.

[enne (OOO), 2014-01-16 19:41:24]

On 2014/01/16 19:30:58, danakj wrote:
> On 2014/01/16 19:28:56, enne wrote:
> > What am I testing? "This doesn't DCHECK" is a really poor test.
> 
> Just that the method returns a 0? DCHECK implies we couldn't have had a test
for
> this code path before.

I can write a test, but that won't test this patch.  That will just test that
the code used to work before this patch in a release build.

[enne (OOO), 2014-01-16 19:49:52]

Here's a test: https://codereview.chromium.org/137853021

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

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

[commit-bot: I haz the power, 2014-01-16 23:01:03]

Change committed as 245365