cc: Re-enable disabled tile manager unittests (plus fixes)

cc: Re-enable disabled tile manager unittests (plus fixes)

This patch creates a solid color tiling that pretends that it's not
solid, so that tiles can be created. This ensures that we don't try
to allocate resources/staging buffers for the tiles, and avoid a crash.

BUG=534911
R=danakj
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/a8d87627d155679cb83db95d39a1798c19c33c6f
Cr-Commit-Position: refs/heads/master@{#351181}

[vmpstr, 2015-09-26 00:36:06]

Please take a look.

[danakj, 2015-09-28 18:26:43]

https://codereview.chromium.org/1365793003/diff/1/cc/tiles/tile_manager_unitt...
File cc/tiles/tile_manager_unittest.cc (right):

https://codereview.chromium.org/1365793003/diff/1/cc/tiles/tile_manager_unitt...
cc/tiles/tile_manager_unittest.cc:87:
pending_raster_source->set_is_solid_color(false);
Please leave a comment explaining this solid color magic.

Did you figure out what's wrong with using non-solid color though? I'd like to
understand cuz my current understanding is that it should work.

[vmpstr, 2015-09-28 20:23:21]

Please take a look.

https://codereview.chromium.org/1365793003/diff/1/cc/tiles/tile_manager_unitt...
File cc/tiles/tile_manager_unittest.cc (right):

https://codereview.chromium.org/1365793003/diff/1/cc/tiles/tile_manager_unitt...
cc/tiles/tile_manager_unittest.cc:87:
pending_raster_source->set_is_solid_color(false);
On 2015/09/28 18:26:43, danakj wrote:
> Please leave a comment explaining this solid color magic.
> 

Done.

> Did you figure out what's wrong with using non-solid color though? I'd like to
> understand cuz my current understanding is that it should work.

One copy tile task runner tries to allocate a gpu memory buffer for the staging
buffer by calling
resource_provider_->gpu_memory_buffer_manager()->AllocateGpuMemoryBuffer(...).
However, FakeLayerTreeHostImpl always has nullptr as gpu_memory_buffer_manager,
so this is guaranteed to crash.

[danakj, 2015-09-28 21:00:10]

On Mon, Sep 28, 2015 at 1:23 PM, <vmpstr@chromium.org> wrote:

> Please take a look.
>
>
>
>
https://codereview.chromium.org/1365793003/diff/1/cc/tiles/tile_manager_unitt...
> File cc/tiles/tile_manager_unittest.cc (right):
>
>
>
https://codereview.chromium.org/1365793003/diff/1/cc/tiles/tile_manager_unitt...
> cc/tiles/tile_manager_unittest.cc:87:
> pending_raster_source->set_is_solid_color(false);
> On 2015/09/28 18:26:43, danakj wrote:
>
>> Please leave a comment explaining this solid color magic.
>>
>
>
> Done.
>
> Did you figure out what's wrong with using non-solid color though? I'd
>>
> like to
>
>> understand cuz my current understanding is that it should work.
>>
>
> One copy tile task runner tries to allocate a gpu memory buffer for the
> staging buffer by calling
>
>
resource_provider_->gpu_memory_buffer_manager()->AllocateGpuMemoryBuffer(...).
> However, FakeLayerTreeHostImpl always has nullptr as
> gpu_memory_buffer_manager, so this is guaranteed to crash.
>

You can pass a TestGpuMemoryBufferManager to the LTHI. Most tests do this,
but some still have null cuz they didn't crash so we didn't catch that yet.
https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/test_gpu_m...


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

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

[vmpstr, 2015-09-28 21:27:24]

PTAL

On 2015/09/28 21:00:10, danakj wrote:
> On Mon, Sep 28, 2015 at 1:23 PM, <mailto:vmpstr@chromium.org> wrote:
> 
> > Please take a look.
> >
> >
> >
> >
>
https://codereview.chromium.org/1365793003/diff/1/cc/tiles/tile_manager_unitt...
> > File cc/tiles/tile_manager_unittest.cc (right):
> >
> >
> >
>
https://codereview.chromium.org/1365793003/diff/1/cc/tiles/tile_manager_unitt...
> > cc/tiles/tile_manager_unittest.cc:87:
> > pending_raster_source->set_is_solid_color(false);
> > On 2015/09/28 18:26:43, danakj wrote:
> >
> >> Please leave a comment explaining this solid color magic.
> >>
> >
> >
> > Done.
> >
> > Did you figure out what's wrong with using non-solid color though? I'd
> >>
> > like to
> >
> >> understand cuz my current understanding is that it should work.
> >>
> >
> > One copy tile task runner tries to allocate a gpu memory buffer for the
> > staging buffer by calling
> >
> >
> resource_provider_->gpu_memory_buffer_manager()->AllocateGpuMemoryBuffer(...).
> > However, FakeLayerTreeHostImpl always has nullptr as
> > gpu_memory_buffer_manager, so this is guaranteed to crash.
> >
> 
> You can pass a TestGpuMemoryBufferManager to the LTHI. Most tests do this,
> but some still have null cuz they didn't crash so we didn't catch that yet.
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/test_gpu_m...
> 

Oh I wasn't aware of this, since I only looked at FakeLTHI ctors and it wasn't
there. I've added one and passed it to the tests, which now pass. Thanks!

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

[danakj, 2015-09-28 21:28:47]

LGTM

https://codereview.chromium.org/1365793003/diff/60001/cc/test/fake_layer_tree...
File cc/test/fake_layer_tree_host_impl.cc (left):

https://codereview.chromium.org/1365793003/diff/60001/cc/test/fake_layer_tree...
cc/test/fake_layer_tree_host_impl.cc:25: SetViewportSize(gfx::Size(100, 100));
Huh, no tests depended on this?

[vmpstr, 2015-09-28 21:36:44]

https://codereview.chromium.org/1365793003/diff/60001/cc/test/fake_layer_tree...
File cc/test/fake_layer_tree_host_impl.cc (left):

https://codereview.chromium.org/1365793003/diff/60001/cc/test/fake_layer_tree...
cc/test/fake_layer_tree_host_impl.cc:25: SetViewportSize(gfx::Size(100, 100));
On 2015/09/28 21:28:47, danakj wrote:
> Huh, no tests depended on this?

Oops, I missed that. I thought the code was all the same in each ctor. I readded
this, since some tests do depend on it.

[vmpstr, 2015-09-28 21:36:47]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2015-09-28 21:36:47]

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

[commit-bot: I haz the power, 2015-09-28 21:37:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1365793003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1365793003/80001

[commit-bot: I haz the power, 2015-09-28 22:43:25]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-09-28 22:44:33]

Patchset 5 (id:??) landed as
https://crrev.com/a8d87627d155679cb83db95d39a1798c19c33c6f
Cr-Commit-Position: refs/heads/master@{#351181}