Re-land: ui: Add GLImage unit test framework.

Re-land: ui: Add GLImage unit test framework.

This makes it possible to test GLImage implementations without
requiring multi-process GpuMemoryBuffer support.

This initial version is limited to testing CopyTexSubImage with
the default buffer format but testing of more functionality and
formats will be added in follow up patches.

Also includes some minor cleanup needed to not have GLImage
implementations depend on GpuMemoryBuffer API.

BUG=538325
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/8aa0fb1d78963610ab62a349d1dff9bd2e065a3e
Cr-Commit-Position: refs/heads/master@{#353735}

[reveman, 2015-09-21 22:08:47]

reveman@chromium.org changed reviewers:
+ dcastagna@chromium.org

[reveman, 2015-09-22 02:43:26]

The CQ bit was checked by reveman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-22 02:43:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1354483004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1354483004/20001

[commit-bot: I haz the power, 2015-09-22 03:00:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-22 03:00:33]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)

[reveman, 2015-09-22 03:45:29]

The CQ bit was checked by reveman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-22 03:45:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1354483004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1354483004/40001

[commit-bot: I haz the power, 2015-09-22 04:17:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-22 04:17:46]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[reveman, 2015-09-22 04:36:13]

The CQ bit was checked by reveman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-22 04:36:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1354483004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1354483004/60001

[commit-bot: I haz the power, 2015-09-22 05:12:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-22 05:12:40]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[reveman, 2015-09-22 05:19:13]

The CQ bit was checked by reveman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-22 05:19:24]

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

[commit-bot: I haz the power, 2015-09-22 06:01:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-22 06:01:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Daniele Castagna, 2015-09-22 15:55:33]

https://codereview.chromium.org/1354483004/diff/80001/ui/gl/gl_image_unittest.cc
File ui/gl/gl_image_unittest.cc (right):

https://codereview.chromium.org/1354483004/diff/80001/ui/gl/gl_image_unittest...
ui/gl/gl_image_unittest.cc:44: enum class GLImageType {
Have you considered having one test fixture for each image type?
Something like:

class GLImageSharedMemory : public GLImageTest {
 blablabla

In this way you could move platform specific tests in their own files, build
them only on the platforms where it makes sense, and avoid the ifdef fest.

https://codereview.chromium.org/1354483004/diff/80001/ui/gl/gl_image_unittest...
ui/gl/gl_image_unittest.cc:484: TEST_P(GLImageTest, CopyTexSubImage) {
Can you spend few words here describing CopyTexSubImage expected behavior?
If someone is about to implement a new GLImage he's probably going to look at
these tests.
GLImage documentation seems pretty terse. It'd be nice to have some more details
here.

https://codereview.chromium.org/1354483004/diff/80001/ui/gl/gl_image_unittest...
ui/gl/gl_image_unittest.cc:490: if (!image)
When would this happen?

https://codereview.chromium.org/1354483004/diff/80001/ui/gl/gl_image_unittest...
ui/gl/gl_image_unittest.cc:507: if (image->CopyTexSubImage(target, Point(),
Rect(image_size))) {
Is it OK not to support CopyTexSubImage?
If that is OK, shouldn't we change the expectations accordingly and test that is
not supported for specific implementations?

[reveman, 2015-09-24 00:31:42]

ptal

https://codereview.chromium.org/1354483004/diff/80001/ui/gl/gl_image_unittest.cc
File ui/gl/gl_image_unittest.cc (right):

https://codereview.chromium.org/1354483004/diff/80001/ui/gl/gl_image_unittest...
ui/gl/gl_image_unittest.cc:44: enum class GLImageType {
On 2015/09/22 at 15:55:32, Daniele Castagna wrote:
> Have you considered having one test fixture for each image type?
> Something like:
> 
> class GLImageSharedMemory : public GLImageTest {
>  blablabla
> 
> In this way you could move platform specific tests in their own files, build
them only on the platforms where it makes sense, and avoid the ifdef fest.

Latest patch implements this using a test template as discussed.

https://codereview.chromium.org/1354483004/diff/80001/ui/gl/gl_image_unittest...
ui/gl/gl_image_unittest.cc:484: TEST_P(GLImageTest, CopyTexSubImage) {
On 2015/09/22 at 15:55:32, Daniele Castagna wrote:
> Can you spend few words here describing CopyTexSubImage expected behavior?
> If someone is about to implement a new GLImage he's probably going to look at
these tests.
> GLImage documentation seems pretty terse. It'd be nice to have some more
details here.

Added some more comments to latest patch but I'd rather add more documentation
to GLImage class if you think that's needed. Let's do that as part of future
refactor to that interface.

https://codereview.chromium.org/1354483004/diff/80001/ui/gl/gl_image_unittest...
ui/gl/gl_image_unittest.cc:490: if (!image)
On 2015/09/22 at 15:55:32, Daniele Castagna wrote:
> When would this happen?

Made it a requirement for this to be supported by all implementations in latest
patch. Should be fine with preferred format.

https://codereview.chromium.org/1354483004/diff/80001/ui/gl/gl_image_unittest...
ui/gl/gl_image_unittest.cc:507: if (image->CopyTexSubImage(target, Point(),
Rect(image_size))) {
On 2015/09/22 at 15:55:32, Daniele Castagna wrote:
> Is it OK not to support CopyTexSubImage?

Yes.

> If that is OK, shouldn't we change the expectations accordingly and test that
is not supported for specific implementations?

I'm checking that texture is not modified if this returns false in latest patch.

[Daniele Castagna, 2015-09-25 19:32:26]

On 2015/09/24 at 00:31:42, reveman wrote:
> ptal
> 
>
https://codereview.chromium.org/1354483004/diff/80001/ui/gl/gl_image_unittest.cc
> File ui/gl/gl_image_unittest.cc (right):
> 
>
https://codereview.chromium.org/1354483004/diff/80001/ui/gl/gl_image_unittest...
> ui/gl/gl_image_unittest.cc:44: enum class GLImageType {
> On 2015/09/22 at 15:55:32, Daniele Castagna wrote:
> > Have you considered having one test fixture for each image type?
> > Something like:
> > 
> > class GLImageSharedMemory : public GLImageTest {
> >  blablabla
> > 
> > In this way you could move platform specific tests in their own files, build
them only on the platforms where it makes sense, and avoid the ifdef fest.
> 
> Latest patch implements this using a test template as discussed.
> 

Thanks! It looks much more modular now.


>
https://codereview.chromium.org/1354483004/diff/80001/ui/gl/gl_image_unittest...
> ui/gl/gl_image_unittest.cc:484: TEST_P(GLImageTest, CopyTexSubImage) {
> On 2015/09/22 at 15:55:32, Daniele Castagna wrote:
> > Can you spend few words here describing CopyTexSubImage expected behavior?
> > If someone is about to implement a new GLImage he's probably going to look
at these tests.
> > GLImage documentation seems pretty terse. It'd be nice to have some more
details here.
> 
> Added some more comments to latest patch but I'd rather add more documentation
to GLImage class if you think that's needed. Let's do that as part of future
refactor to that interface.
> 
>
https://codereview.chromium.org/1354483004/diff/80001/ui/gl/gl_image_unittest...
> ui/gl/gl_image_unittest.cc:490: if (!image)
> On 2015/09/22 at 15:55:32, Daniele Castagna wrote:
> > When would this happen?
> 
> Made it a requirement for this to be supported by all implementations in
latest patch. Should be fine with preferred format.
> 
>
https://codereview.chromium.org/1354483004/diff/80001/ui/gl/gl_image_unittest...
> ui/gl/gl_image_unittest.cc:507: if (image->CopyTexSubImage(target, Point(),
Rect(image_size))) {
> On 2015/09/22 at 15:55:32, Daniele Castagna wrote:
> > Is it OK not to support CopyTexSubImage?
> 
> Yes.
> 
> > If that is OK, shouldn't we change the expectations accordingly and test
that is not supported for specific implementations?
> 
> I'm checking that texture is not modified if this returns false in latest
patch.

Can we split this patch?
I see GLTestHelper has been introduced but not all its methods are used.
Can we introduce this helper in another CL. Maybe we can also start using it in
other places like gl_gpu_memory_buffer_unittest.cc and
texture_upload_perftest.cc.

It'd be also nice to have the changes for correctly tearing down GL API in
another CL.

[reveman, 2015-09-30 10:13:56]

On 2015/09/25 at 19:32:26, dcastagna wrote:
> On 2015/09/24 at 00:31:42, reveman wrote:
> > ptal
> > 
> >
https://codereview.chromium.org/1354483004/diff/80001/ui/gl/gl_image_unittest.cc
> > File ui/gl/gl_image_unittest.cc (right):
> > 
> >
https://codereview.chromium.org/1354483004/diff/80001/ui/gl/gl_image_unittest...
> > ui/gl/gl_image_unittest.cc:44: enum class GLImageType {
> > On 2015/09/22 at 15:55:32, Daniele Castagna wrote:
> > > Have you considered having one test fixture for each image type?
> > > Something like:
> > > 
> > > class GLImageSharedMemory : public GLImageTest {
> > >  blablabla
> > > 
> > > In this way you could move platform specific tests in their own files,
build them only on the platforms where it makes sense, and avoid the ifdef fest.
> > 
> > Latest patch implements this using a test template as discussed.
> > 
> 
> Thanks! It looks much more modular now.
> 
> 
> >
https://codereview.chromium.org/1354483004/diff/80001/ui/gl/gl_image_unittest...
> > ui/gl/gl_image_unittest.cc:484: TEST_P(GLImageTest, CopyTexSubImage) {
> > On 2015/09/22 at 15:55:32, Daniele Castagna wrote:
> > > Can you spend few words here describing CopyTexSubImage expected behavior?
> > > If someone is about to implement a new GLImage he's probably going to look
at these tests.
> > > GLImage documentation seems pretty terse. It'd be nice to have some more
details here.
> > 
> > Added some more comments to latest patch but I'd rather add more
documentation to GLImage class if you think that's needed. Let's do that as part
of future refactor to that interface.
> > 
> >
https://codereview.chromium.org/1354483004/diff/80001/ui/gl/gl_image_unittest...
> > ui/gl/gl_image_unittest.cc:490: if (!image)
> > On 2015/09/22 at 15:55:32, Daniele Castagna wrote:
> > > When would this happen?
> > 
> > Made it a requirement for this to be supported by all implementations in
latest patch. Should be fine with preferred format.
> > 
> >
https://codereview.chromium.org/1354483004/diff/80001/ui/gl/gl_image_unittest...
> > ui/gl/gl_image_unittest.cc:507: if (image->CopyTexSubImage(target, Point(),
Rect(image_size))) {
> > On 2015/09/22 at 15:55:32, Daniele Castagna wrote:
> > > Is it OK not to support CopyTexSubImage?
> > 
> > Yes.
> > 
> > > If that is OK, shouldn't we change the expectations accordingly and test
that is not supported for specific implementations?
> > 
> > I'm checking that texture is not modified if this returns false in latest
patch.
> 
> Can we split this patch?
> I see GLTestHelper has been introduced but not all its methods are used.
> Can we introduce this helper in another CL. Maybe we can also start using it
in other places like gl_gpu_memory_buffer_unittest.cc and
texture_upload_perftest.cc.
> 
> It'd be also nice to have the changes for correctly tearing down GL API in
another CL.

Yes, I'll move that to a different patch before landing this but I'd like to
save the exercise of using gl_test_helper code in
gl_gpu_memory_buffer_unittest.cc and texture_upload_perftest.cc as a follow up
if you're ok with that? That's definitely the plan though. Just a bit tricky as
a result of how we include GL declarations.

[reveman, 2015-09-30 10:14:05]

The CQ bit was checked by reveman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-30 10:14:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1354483004/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1354483004/180001

[commit-bot: I haz the power, 2015-09-30 11:49:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-30 11:49:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Daniele Castagna, 2015-09-30 19:17:36]

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/gl_image_ref_cou...
File ui/gl/gl_image_ref_counted_memory_unittest.cc (right):

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/gl_image_ref_cou...
ui/gl/gl_image_ref_counted_memory_unittest.cc:21:
DCHECK_EQ(NumberOfPlanesForBufferFormat(format), 1u);
ASSERT_EQ?

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/gl_image_ref_cou...
ui/gl/gl_image_ref_counted_memory_unittest.cc:22: std::vector<uint8_t>
data(BufferSizeForBufferFormat(size, format), 0);
You can omit the second parameter. No harm in being explicit though.

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/gl_image_shared_...
File ui/gl/gl_image_shared_memory_unittest.cc (right):

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/gl_image_shared_...
ui/gl/gl_image_shared_memory_unittest.cc:23: DCHECK(rv);
Isn't it preferred to use ASSERT instead of DCHECKS in tests?

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_image_te...
File ui/gl/test/gl_image_test_support.cc (right):

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_image_te...
ui/gl/test/gl_image_test_support.cc:9: #include
"third_party/skia/include/core/SkTypes.h"
Does this need to depend on Skia just for SK_B32_SHIFT?

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_image_te...
File ui/gl/test/gl_image_test_support.h (right):

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_image_te...
ui/gl/test/gl_image_test_support.h:13: class GLImageTestSupport {
Can we have a namespace instead of a class used as a namespace?

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_image_te...
File ui/gl/test/gl_image_test_template.h (right):

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_image_te...
ui/gl/test/gl_image_test_template.h:5: // This file defines tests that
implementations of GLImage should
Nit: why did you break this line so early? It seems like pass in should fit here
at the end.

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_image_te...
ui/gl/test/gl_image_test_template.h:25: #define SHADER(Src) #Src
FYI: This is defined exactly like this already in 12 places. :)

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_image_te...
ui/gl/test/gl_image_test_template.h:77: scoped_ptr<uint8_t[]> pixels(new
uint8_t[BufferSizeForBufferFormat(
std::vector<uint8_t> ?

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_image_te...
ui/gl/test/gl_image_test_template.h:100: const char kVertexShader[] = SHADER(
It'd be nice if the following code to draw a quad could live in
gl_test_helper.cc
It's completely OK to leave it here in this patch.

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_test_hel...
File ui/gl/test/gl_test_helper.cc (right):

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_test_hel...
ui/gl/test/gl_test_helper.cc:32: // Compile the shader.
Nit: not sure this comment adds anything.

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_test_hel...
ui/gl/test/gl_test_helper.cc:113: scoped_ptr<uint8_t[]> pixels(new
uint8_t[size]);
Why not std::vector<uint8_t> pixels(size, kCheckClearValue)?

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_test_hel...
ui/gl/test/gl_test_helper.cc:127: // Exit early just so we don't spam the log
but we print enough to
FYI: if you EXPECT_EQ with two std::arrays as parameters, you get a similar
behavior.

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_test_hel...
File ui/gl/test/gl_test_helper.h (right):

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_test_hel...
ui/gl/test/gl_test_helper.h:15: // Creates a texture object.
You might want to specify it binds it too.

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_test_hel...
ui/gl/test/gl_test_helper.h:21: static GLuint CompileShader(GLenum type, const
char* src);
Can this be private?

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_test_hel...
ui/gl/test/gl_test_helper.h:29: static GLuint LinkProgram(GLuint vertex_shader,
GLuint fragment_shader);
Same here, can this be private?

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_test_hel...
ui/gl/test/gl_test_helper.h:37: static GLuint LoadProgram(const char*
vertex_shader_src,
Is this used anywhere?

[reveman, 2015-10-01 04:01:37]

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/gl_image_ref_cou...
File ui/gl/gl_image_ref_counted_memory_unittest.cc (right):

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/gl_image_ref_cou...
ui/gl/gl_image_ref_counted_memory_unittest.cc:21:
DCHECK_EQ(NumberOfPlanesForBufferFormat(format), 1u);
On 2015/09/30 at 19:17:35, Daniele Castagna wrote:
> ASSERT_EQ?

ASSERT_EQ expects to be able to return.

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/gl_image_ref_cou...
ui/gl/gl_image_ref_counted_memory_unittest.cc:22: std::vector<uint8_t>
data(BufferSizeForBufferFormat(size, format), 0);
On 2015/09/30 at 19:17:35, Daniele Castagna wrote:
> You can omit the second parameter. No harm in being explicit though.

Omitted.

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/gl_image_shared_...
File ui/gl/gl_image_shared_memory_unittest.cc (right):

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/gl_image_shared_...
ui/gl/gl_image_shared_memory_unittest.cc:23: DCHECK(rv);
On 2015/09/30 at 19:17:35, Daniele Castagna wrote:
> Isn't it preferred to use ASSERT instead of DCHECKS in tests?

Not sure how to do that outside the main body of the test.

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_image_te...
File ui/gl/test/gl_image_test_support.cc (right):

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_image_te...
ui/gl/test/gl_image_test_support.cc:9: #include
"third_party/skia/include/core/SkTypes.h"
On 2015/09/30 at 19:17:35, Daniele Castagna wrote:
> Does this need to depend on Skia just for SK_B32_SHIFT?

Correct.

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_image_te...
File ui/gl/test/gl_image_test_support.h (right):

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_image_te...
ui/gl/test/gl_image_test_support.h:13: class GLImageTestSupport {
On 2015/09/30 at 19:17:35, Daniele Castagna wrote:
> Can we have a namespace instead of a class used as a namespace?

This is more consistent with GLSurfaceTestSupport. Some future tests will likely
need private access to GLImage and this class serves as a mechanism for that.
ie. it can be made a friend of GLImage just like GLSurfaceTestSupport is a
friend of GLSurface.

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_image_te...
File ui/gl/test/gl_image_test_template.h (right):

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_image_te...
ui/gl/test/gl_image_test_template.h:5: // This file defines tests that
implementations of GLImage should
On 2015/09/30 at 19:17:35, Daniele Castagna wrote:
> Nit: why did you break this line so early? It seems like pass in should fit
here at the end.

Cut-and-paste from task_runner_test_template.h. Used cl format for line wrapping
in latest patch.

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_image_te...
ui/gl/test/gl_image_test_template.h:25: #define SHADER(Src) #Src
On 2015/09/30 at 19:17:36, Daniele Castagna wrote:
> FYI: This is defined exactly like this already in 12 places. :)

Instead using STRINGIZE from base/ in latest patch.

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_image_te...
ui/gl/test/gl_image_test_template.h:77: scoped_ptr<uint8_t[]> pixels(new
uint8_t[BufferSizeForBufferFormat(
On 2015/09/30 at 19:17:35, Daniele Castagna wrote:
> std::vector<uint8_t> ?

I prefer scoped_ptr<uint8_t[]> when std::vector doesn't provide functionality
that would make the code cleaner. e.g. initialize to a specific value.

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_image_te...
ui/gl/test/gl_image_test_template.h:100: const char kVertexShader[] = SHADER(
On 2015/09/30 at 19:17:35, Daniele Castagna wrote:
> It'd be nice if the following code to draw a quad could live in
gl_test_helper.cc
> It's completely OK to leave it here in this patch.

Yes, the plan is to move it as soon as we add another test. Didn't want to deal
with that in this initial patch.

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_test_hel...
File ui/gl/test/gl_test_helper.cc (right):

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_test_hel...
ui/gl/test/gl_test_helper.cc:32: // Compile the shader.
On 2015/09/30 at 19:17:36, Daniele Castagna wrote:
> Nit: not sure this comment adds anything.

I'll keep it the same as gl_test_utils.cc for now. We can fix this once this is
the only version left.

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_test_hel...
ui/gl/test/gl_test_helper.cc:113: scoped_ptr<uint8_t[]> pixels(new
uint8_t[size]);
On 2015/09/30 at 19:17:36, Daniele Castagna wrote:
> Why not std::vector<uint8_t> pixels(size, kCheckClearValue)?

Mostly because this comes from gpu/command_buffer/tests/gl_test_utils.cc but I
also typically prefer scoped_ptr<[]> instead of std::vector when dynamic size is
not needed.

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_test_hel...
File ui/gl/test/gl_test_helper.h (right):

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_test_hel...
ui/gl/test/gl_test_helper.h:15: // Creates a texture object.
On 2015/09/30 at 19:17:36, Daniele Castagna wrote:
> You might want to specify it binds it too.

I made sure that no code relies on that instead.

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_test_hel...
ui/gl/test/gl_test_helper.h:21: static GLuint CompileShader(GLenum type, const
char* src);
On 2015/09/30 at 19:17:36, Daniele Castagna wrote:
> Can this be private?

I'm keeping it like this so it's the same as
gpu/command_buffer/tests/gl_test_utils.h and that code can more easily be made
to use this implementation instead.

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_test_hel...
ui/gl/test/gl_test_helper.h:29: static GLuint LinkProgram(GLuint vertex_shader,
GLuint fragment_shader);
On 2015/09/30 at 19:17:36, Daniele Castagna wrote:
> Same here, can this be private?

Keeping it for the same reason.

https://codereview.chromium.org/1354483004/diff/200001/ui/gl/test/gl_test_hel...
ui/gl/test/gl_test_helper.h:37: static GLuint LoadProgram(const char*
vertex_shader_src,
On 2015/09/30 at 19:17:36, Daniele Castagna wrote:
> Is this used anywhere?

No, forgot to remove it from the header.

https://codereview.chromium.org/1354483004/diff/220001/build/sanitizers/lsan_...
File build/sanitizers/lsan_suppressions.cc (right):

https://codereview.chromium.org/1354483004/diff/220001/build/sanitizers/lsan_...
build/sanitizers/lsan_suppressions.cc:55: "leak:swrast_dri.so\n"
Needed on an asan bot that is using an older system provided swrast_dri.so.

[reveman, 2015-10-01 04:01:46]

The CQ bit was checked by reveman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-01 04:02:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1354483004/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1354483004/220001

[commit-bot: I haz the power, 2015-10-01 04:04:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-01 04:04:29]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[reveman, 2015-10-01 10:22:42]

The CQ bit was checked by reveman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-01 10:23:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1354483004/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1354483004/240001

[commit-bot: I haz the power, 2015-10-01 10:26:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-01 10:26:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[reveman, 2015-10-01 10:41:40]

The CQ bit was checked by reveman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-01 10:42:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1354483004/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1354483004/260001

[commit-bot: I haz the power, 2015-10-01 11:56:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-01 11:56:34]

Dry run: This issue passed the CQ dry run.

[reveman, 2015-10-01 13:28:14]

reveman@chromium.org changed reviewers:
+ piman@chromium.org

[reveman, 2015-10-01 13:28:16]

+piman

Fyi, I'll move GLContext/Surface changes needed to run real GL tests as part of
gl_unittests to a separate patch if you prefer? Just thought it'd be easier to
understand the motivation for these changes by looking at all this together
first.

https://codereview.chromium.org/1354483004/diff/260001/build/sanitizers/lsan_...
File build/sanitizers/lsan_suppressions.cc (right):

https://codereview.chromium.org/1354483004/diff/260001/build/sanitizers/lsan_...
build/sanitizers/lsan_suppressions.cc:55: "leak:swrast_dri.so\n"
Needed because some versions of this driver has a leak that is triggered by bots
running these new tests on Xvfb.

https://codereview.chromium.org/1354483004/diff/260001/components/mus/BUILD.gn
File components/mus/BUILD.gn (left):

https://codereview.chromium.org/1354483004/diff/260001/components/mus/BUILD.g...
components/mus/BUILD.gn:61: "//ui/gl:test_support",
we can't depend on this without testonly = true. luckily, we don't seem to use
it.

https://codereview.chromium.org/1354483004/diff/260001/components/mus/mus_app.cc
File components/mus/mus_app.cc (left):

https://codereview.chromium.org/1354483004/diff/260001/components/mus/mus_app...
components/mus/mus_app.cc:24: #include "ui/gl/test/gl_surface_test_support.h"
unused afaict and required to remove the //ui/gl:test_support dependency

https://codereview.chromium.org/1354483004/diff/260001/components/mus/vm/BUIL...
File components/mus/vm/BUILD.gn (left):

https://codereview.chromium.org/1354483004/diff/260001/components/mus/vm/BUIL...
components/mus/vm/BUILD.gn:71: "//ui/gl:test_support",
unused afaict

https://codereview.chromium.org/1354483004/diff/260001/ui/gl/gl_api_unittest.cc
File ui/gl/gl_api_unittest.cc (right):

https://codereview.chromium.org/1354483004/diff/260001/ui/gl/gl_api_unittest....
ui/gl/gl_api_unittest.cc:57: g_current_gl_context_tls = nullptr;
Needed to be able to use a real gl api after having run this test.

https://codereview.chromium.org/1354483004/diff/260001/ui/gl/gl_image_io_surf...
File ui/gl/gl_image_io_surface.h (left):

https://codereview.chromium.org/1354483004/diff/260001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.h:25: GLImageIOSurface(gfx::GenericSharedMemoryId
io_surface_id,
Changing this as the surface id is more appropriately passed with surface.

https://codereview.chromium.org/1354483004/diff/260001/ui/gl/gl_image_shared_...
File ui/gl/gl_image_shared_memory.h (left):

https://codereview.chromium.org/1354483004/diff/260001/ui/gl/gl_image_shared_...
ui/gl/gl_image_shared_memory.h:18: bool Initialize(const
gfx::GpuMemoryBufferHandle& handle,
GLImage implementations should preferably not depend on GpuMemoryBuffers.

[piman, 2015-10-06 01:23:56]

Couple of small things, but otherwise LGTM.

https://codereview.chromium.org/1354483004/diff/260001/ui/gl/test/gl_image_te...
File ui/gl/test/gl_image_test_template.h (right):

https://codereview.chromium.org/1354483004/diff/260001/ui/gl/test/gl_image_te...
ui/gl/test/gl_image_test_template.h:171: glDeleteTextures(1, &texture);
Also glDeleteFramebuffersEXT(1, &framebuffer);

https://codereview.chromium.org/1354483004/diff/260001/ui/gl/test/gl_test_hel...
File ui/gl/test/gl_test_helper.cc (right):

https://codereview.chromium.org/1354483004/diff/260001/ui/gl/test/gl_test_hel...
ui/gl/test/gl_test_helper.cc:88: GLuint color_buffer_texture =
CreateTexture(GL_TEXTURE_2D);
You're leaking this texture.
One way around it is to use the refcounting logic inside the driver: if you
delete the texture, it will only unbind it from the framebuffer if the
framebuffer is currently bound. So you could
glBindFramebuffersEXT(GL_FRAMEBUFFER, 0); glDeleteTextures(1,
&color_buffer_texture);

[reveman, 2015-10-06 13:53:46]

reveman@chromium.org changed reviewers:
+ glider@chromium.org, sky@chromium.org

[reveman, 2015-10-06 13:53:47]

+glider for build/sanitizers/
+sky for components/mus/

https://codereview.chromium.org/1354483004/diff/260001/ui/gl/test/gl_image_te...
File ui/gl/test/gl_image_test_template.h (right):

https://codereview.chromium.org/1354483004/diff/260001/ui/gl/test/gl_image_te...
ui/gl/test/gl_image_test_template.h:171: glDeleteTextures(1, &texture);
On 2015/10/06 at 01:23:56, piman (slow to review) wrote:
> Also glDeleteFramebuffersEXT(1, &framebuffer);

Done.

https://codereview.chromium.org/1354483004/diff/260001/ui/gl/test/gl_test_hel...
File ui/gl/test/gl_test_helper.cc (right):

https://codereview.chromium.org/1354483004/diff/260001/ui/gl/test/gl_test_hel...
ui/gl/test/gl_test_helper.cc:88: GLuint color_buffer_texture =
CreateTexture(GL_TEXTURE_2D);
On 2015/10/06 at 01:23:56, piman (slow to review) wrote:
> You're leaking this texture.
> One way around it is to use the refcounting logic inside the driver: if you
delete the texture, it will only unbind it from the framebuffer if the
framebuffer is currently bound. So you could
glBindFramebuffersEXT(GL_FRAMEBUFFER, 0); glDeleteTextures(1,
&color_buffer_texture);

Done.

[reveman, 2015-10-06 13:53:56]

The CQ bit was checked by reveman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-06 13:54:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1354483004/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1354483004/300001

[commit-bot: I haz the power, 2015-10-06 15:12:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-06 15:12:52]

Dry run: This issue passed the CQ dry run.

[Alexander Potapenko, 2015-10-06 15:55:53]

lsan_suppressions.cc - LGTM with a nit.

https://codereview.chromium.org/1354483004/diff/300001/build/sanitizers/lsan_...
File build/sanitizers/lsan_suppressions.cc (right):

https://codereview.chromium.org/1354483004/diff/300001/build/sanitizers/lsan_...
build/sanitizers/lsan_suppressions.cc:54: // Leaks in swrast_dri.so.
Can you please document these somewhere and add a link here?

[sky, 2015-10-06 16:59:36]

LGTM

[reveman, 2015-10-06 22:44:28]

https://codereview.chromium.org/1354483004/diff/300001/build/sanitizers/lsan_...
File build/sanitizers/lsan_suppressions.cc (right):

https://codereview.chromium.org/1354483004/diff/300001/build/sanitizers/lsan_...
build/sanitizers/lsan_suppressions.cc:54: // Leaks in swrast_dri.so.
On 2015/10/06 at 15:55:53, Alexander Potapenko wrote:
> Can you please document these somewhere and add a link here?

Done.

[reveman, 2015-10-06 22:44:41]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-10-06 22:44:42]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
glider@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1354483004/#ps320001
(title: "add link to bug for swrast.so leak")

[commit-bot: I haz the power, 2015-10-06 22:45:13]

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

[commit-bot: I haz the power, 2015-10-07 00:11:01]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2015-10-07 00:12:02]

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

[Kunihiko Sakamoto, 2015-10-07 01:47:12]

A revert of this CL (patchset #17 id:320001) has been created in
https://codereview.chromium.org/1390083002/ by ksakamoto@chromium.org.

The reason for reverting is: Those tests are failing on Linux GN bot.

failures:
GLImageRefCountedMemory/GLImageTest/0.CopyTexSubImage
GLImageSharedMemory/GLImageTest/0.CopyTexSubImage

http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32944
http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32945
http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32946
.

[reveman, 2015-10-07 02:45:43]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-10-07 02:45:44]

Added osmesa deps in a way consistent with cc/ to latest patch. I can only
assume that this is what caused this to fail but it's hard to be 100% sure when
the CQ doesn't catch this.

I will try to re-land this and hopefully it works this time.

[reveman, 2015-10-07 02:45:44]

The patchset sent to the CQ was uploaded after l-g-t-m from glider@chromium.org,
sky@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1354483004/#ps340001
(title: "add osmesa deps")

[commit-bot: I haz the power, 2015-10-07 02:47:17]

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

[commit-bot: I haz the power, 2015-10-07 04:12:55]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2015-10-07 04:13:59]

Patchset 18 (id:??) landed as
https://crrev.com/11b3655a4dfd5b46f7783421e2959a41ddbf3a00
Cr-Commit-Position: refs/heads/master@{#352767}

[Kunihiko Sakamoto, 2015-10-07 05:48:06]

A revert of this CL (patchset #18 id:340001) has been created in
https://codereview.chromium.org/1390143002/ by ksakamoto@chromium.org.

The reason for reverting is: libosmesa open failure has gone, but tests are
still failing.

http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32956
http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32957
http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32958
.

[reveman, 2015-10-07 14:31:42]

On 2015/10/07 at 05:48:06, ksakamoto wrote:
> A revert of this CL (patchset #18 id:340001) has been created in
https://codereview.chromium.org/1390143002/ by ksakamoto@chromium.org.
> 
> The reason for reverting is: libosmesa open failure has gone, but tests are
still failing.
> 
> http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32956
> http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32957
> http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32958
> .

Ok, this bot is missing an X display server so these tests will never be able to
run on it. I'm not sure how we typically avoid running tests when X is missing.
I'm guessing that's done by the isolate somehow. kbr@, do you know how this
works?

[Ken Russell (switch to Gerrit), 2015-10-07 15:33:51]

On 2015/10/07 14:31:42, reveman wrote:
> On 2015/10/07 at 05:48:06, ksakamoto wrote:
> > A revert of this CL (patchset #18 id:340001) has been created in
> https://codereview.chromium.org/1390143002/ by mailto:ksakamoto@chromium.org.
> > 
> > The reason for reverting is: libosmesa open failure has gone, but tests are
> still failing.
> > 
> > http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32956
> > http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32957
> > http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32958
> > .
> 
> Ok, this bot is missing an X display server so these tests will never be able
to
> run on it. I'm not sure how we typically avoid running tests when X is
missing.
> I'm guessing that's done by the isolate somehow. kbr@, do you know how this
> works?

The isolate is unrelated. Usually the tests that require a GPU are run in
targets that only run on the GPU bots.

In this case, it looks like gl_unittests only runs on the VMs that are on the
main waterfall. See any of the builds of
http://build.chromium.org/p/chromium.gpu/builders/Linux%20Release%20(NVIDIA) to
see which targets are run there.

I'd suggest covering the tests by --use-gpu-in-tests but that would mean they
won't run anywhere. I think you should move these tests to the gl_tests target.
Talk with dyen@, who I recall added that test target.

[David Yen, 2015-10-07 17:40:25]

On 2015/10/07 15:33:51, Ken Russell wrote:
> On 2015/10/07 14:31:42, reveman wrote:
> > On 2015/10/07 at 05:48:06, ksakamoto wrote:
> > > A revert of this CL (patchset #18 id:340001) has been created in
> > https://codereview.chromium.org/1390143002/ by
mailto:ksakamoto@chromium.org.
> > > 
> > > The reason for reverting is: libosmesa open failure has gone, but tests
are
> > still failing.
> > > 
> > >
http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32956
> > >
http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32957
> > >
http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32958
> > > .
> > 
> > Ok, this bot is missing an X display server so these tests will never be
able
> to
> > run on it. I'm not sure how we typically avoid running tests when X is
> missing.
> > I'm guessing that's done by the isolate somehow. kbr@, do you know how this
> > works?
> 
> The isolate is unrelated. Usually the tests that require a GPU are run in
> targets that only run on the GPU bots.
> 
> In this case, it looks like gl_unittests only runs on the VMs that are on the
> main waterfall. See any of the builds of
> http://build.chromium.org/p/chromium.gpu/builders/Linux%20Release%20(NVIDIA)
to
> see which targets are run there.
> 
> I'd suggest covering the tests by --use-gpu-in-tests but that would mean they
> won't run anywhere. I think you should move these tests to the gl_tests
target.
> Talk with dyen@, who I recall added that test target.

gl_unittests is actually the target I added. So the reason I added this
originally was there was no way to test things under ui/gl, and up until this
point we haven't needed to call into actual gl and were able to mock anything we
needed. The way we differentiated between gl_unittests is that these were tests
for components under ui/gl, while gl_tests was an end to end test from the
command buffer to ui/gl to gl. So in that sense these new tests is not really
appropriate for gl_tests because gl_tests is suppose to be for command buffer
end to end tests.

It seems to me that we now have yet another category which tests ui/gl and calls
into actual gl. Should this be another test suite? In that case we should
probably rename this new test suite to be gl_tests and call the command buffer
gl tests something else. Some of the tests currently in gpu/command_buffer/tests
can also be moved here.

Anyone have any opinions on this?

[reveman, 2015-10-07 17:53:38]

On 2015/10/07 at 17:40:25, dyen wrote:
> On 2015/10/07 15:33:51, Ken Russell wrote:
> > On 2015/10/07 14:31:42, reveman wrote:
> > > On 2015/10/07 at 05:48:06, ksakamoto wrote:
> > > > A revert of this CL (patchset #18 id:340001) has been created in
> > > https://codereview.chromium.org/1390143002/ by
mailto:ksakamoto@chromium.org.
> > > > 
> > > > The reason for reverting is: libosmesa open failure has gone, but tests
are
> > > still failing.
> > > > 
> > > >
http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32956
> > > >
http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32957
> > > >
http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32958
> > > > .
> > > 
> > > Ok, this bot is missing an X display server so these tests will never be
able
> > to
> > > run on it. I'm not sure how we typically avoid running tests when X is
> > missing.
> > > I'm guessing that's done by the isolate somehow. kbr@, do you know how
this
> > > works?
> > 
> > The isolate is unrelated. Usually the tests that require a GPU are run in
> > targets that only run on the GPU bots.
> > 
> > In this case, it looks like gl_unittests only runs on the VMs that are on
the
> > main waterfall. See any of the builds of
> > http://build.chromium.org/p/chromium.gpu/builders/Linux%20Release%20(NVIDIA)
to
> > see which targets are run there.
> > 
> > I'd suggest covering the tests by --use-gpu-in-tests but that would mean
they
> > won't run anywhere. I think you should move these tests to the gl_tests
target.
> > Talk with dyen@, who I recall added that test target.
> 
> gl_unittests is actually the target I added. So the reason I added this
originally was there was no way to test things under ui/gl, and up until this
point we haven't needed to call into actual gl and were able to mock anything we
needed. The way we differentiated between gl_unittests is that these were tests
for components under ui/gl, while gl_tests was an end to end test from the
command buffer to ui/gl to gl. So in that sense these new tests is not really
appropriate for gl_tests because gl_tests is suppose to be for command buffer
end to end tests.
> 
> It seems to me that we now have yet another category which tests ui/gl and
calls into actual gl. Should this be another test suite? In that case we should
probably rename this new test suite to be gl_tests and call the command buffer
gl tests something else. Some of the tests currently in gpu/command_buffer/tests
can also be moved here.
> 
> Anyone have any opinions on this?

Why don't we keep it simple and require actual gl for gl_unittests? How much
value do we get out of being able to run the existing gl_unittests on bots
without actual gl in addition to bots with actual gl?

[Ken Russell (switch to Gerrit), 2015-10-08 20:58:51]

On 2015/10/07 17:53:38, reveman wrote:
> On 2015/10/07 at 17:40:25, dyen wrote:
> > On 2015/10/07 15:33:51, Ken Russell wrote:
> > > On 2015/10/07 14:31:42, reveman wrote:
> > > > On 2015/10/07 at 05:48:06, ksakamoto wrote:
> > > > > A revert of this CL (patchset #18 id:340001) has been created in
> > > > https://codereview.chromium.org/1390143002/ by
> mailto:ksakamoto@chromium.org.
> > > > > 
> > > > > The reason for reverting is: libosmesa open failure has gone, but
tests
> are
> > > > still failing.
> > > > > 
> > > > >
> http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32956
> > > > >
> http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32957
> > > > >
> http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32958
> > > > > .
> > > > 
> > > > Ok, this bot is missing an X display server so these tests will never be
> able
> > > to
> > > > run on it. I'm not sure how we typically avoid running tests when X is
> > > missing.
> > > > I'm guessing that's done by the isolate somehow. kbr@, do you know how
> this
> > > > works?
> > > 
> > > The isolate is unrelated. Usually the tests that require a GPU are run in
> > > targets that only run on the GPU bots.
> > > 
> > > In this case, it looks like gl_unittests only runs on the VMs that are on
> the
> > > main waterfall. See any of the builds of
> > >
http://build.chromium.org/p/chromium.gpu/builders/Linux%20Release%20(NVIDIA)
> to
> > > see which targets are run there.
> > > 
> > > I'd suggest covering the tests by --use-gpu-in-tests but that would mean
> they
> > > won't run anywhere. I think you should move these tests to the gl_tests
> target.
> > > Talk with dyen@, who I recall added that test target.
> > 
> > gl_unittests is actually the target I added. So the reason I added this
> originally was there was no way to test things under ui/gl, and up until this
> point we haven't needed to call into actual gl and were able to mock anything
we
> needed. The way we differentiated between gl_unittests is that these were
tests
> for components under ui/gl, while gl_tests was an end to end test from the
> command buffer to ui/gl to gl. So in that sense these new tests is not really
> appropriate for gl_tests because gl_tests is suppose to be for command buffer
> end to end tests.
> > 
> > It seems to me that we now have yet another category which tests ui/gl and
> calls into actual gl. Should this be another test suite? In that case we
should
> probably rename this new test suite to be gl_tests and call the command buffer
> gl tests something else. Some of the tests currently in
gpu/command_buffer/tests
> can also be moved here.
> > 
> > Anyone have any opinions on this?
> 
> Why don't we keep it simple and require actual gl for gl_unittests? How much
> value do we get out of being able to run the existing gl_unittests on bots
> without actual gl in addition to bots with actual gl?

If gl_tests tests "too much" then we can move gl_unittests to run on the GPU
bots and tryservers. Doing this will require adding them to the GPU recipe per
https://www.chromium.org/developers/testing/gpu-recipe and removing them from
the Chromium-side json files (since the GPU bots don't use these yet). reveman@
would you be able to drive this change? I'd be happy to help.

[reveman, 2015-10-09 14:48:11]

On 2015/10/08 at 20:58:51, kbr wrote:
> On 2015/10/07 17:53:38, reveman wrote:
> > On 2015/10/07 at 17:40:25, dyen wrote:
> > > On 2015/10/07 15:33:51, Ken Russell wrote:
> > > > On 2015/10/07 14:31:42, reveman wrote:
> > > > > On 2015/10/07 at 05:48:06, ksakamoto wrote:
> > > > > > A revert of this CL (patchset #18 id:340001) has been created in
> > > > > https://codereview.chromium.org/1390143002/ by
> > mailto:ksakamoto@chromium.org.
> > > > > > 
> > > > > > The reason for reverting is: libosmesa open failure has gone, but
tests
> > are
> > > > > still failing.
> > > > > > 
> > > > > >
> > http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32956
> > > > > >
> > http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32957
> > > > > >
> > http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32958
> > > > > > .
> > > > > 
> > > > > Ok, this bot is missing an X display server so these tests will never
be
> > able
> > > > to
> > > > > run on it. I'm not sure how we typically avoid running tests when X is
> > > > missing.
> > > > > I'm guessing that's done by the isolate somehow. kbr@, do you know how
> > this
> > > > > works?
> > > > 
> > > > The isolate is unrelated. Usually the tests that require a GPU are run
in
> > > > targets that only run on the GPU bots.
> > > > 
> > > > In this case, it looks like gl_unittests only runs on the VMs that are
on
> > the
> > > > main waterfall. See any of the builds of
> > > >
http://build.chromium.org/p/chromium.gpu/builders/Linux%20Release%20(NVIDIA)
> > to
> > > > see which targets are run there.
> > > > 
> > > > I'd suggest covering the tests by --use-gpu-in-tests but that would mean
> > they
> > > > won't run anywhere. I think you should move these tests to the gl_tests
> > target.
> > > > Talk with dyen@, who I recall added that test target.
> > > 
> > > gl_unittests is actually the target I added. So the reason I added this
> > originally was there was no way to test things under ui/gl, and up until
this
> > point we haven't needed to call into actual gl and were able to mock
anything we
> > needed. The way we differentiated between gl_unittests is that these were
tests
> > for components under ui/gl, while gl_tests was an end to end test from the
> > command buffer to ui/gl to gl. So in that sense these new tests is not
really
> > appropriate for gl_tests because gl_tests is suppose to be for command
buffer
> > end to end tests.
> > > 
> > > It seems to me that we now have yet another category which tests ui/gl and
> > calls into actual gl. Should this be another test suite? In that case we
should
> > probably rename this new test suite to be gl_tests and call the command
buffer
> > gl tests something else. Some of the tests currently in
gpu/command_buffer/tests
> > can also be moved here.
> > > 
> > > Anyone have any opinions on this?
> > 
> > Why don't we keep it simple and require actual gl for gl_unittests? How much
> > value do we get out of being able to run the existing gl_unittests on bots
> > without actual gl in addition to bots with actual gl?
> 
> If gl_tests tests "too much" then we can move gl_unittests to run on the GPU
bots and tryservers. Doing this will require adding them to the GPU recipe per
https://www.chromium.org/developers/testing/gpu-recipe and removing them from
the Chromium-side json files (since the GPU bots don't use these yet). reveman@
would you be able to drive this change? I'd be happy to help.

I'm happy to do it but will probably need some hand holding as even after
reading through that page I'm still not sure exactly what I need to change.

[reveman, 2015-10-13 12:19:02]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-10-13 12:19:03]

fyi, re-landing this now that only GPU bots are running gl_unittests.

[reveman, 2015-10-13 12:19:03]

The patchset sent to the CQ was uploaded after l-g-t-m from glider@chromium.org,
sky@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1354483004/#ps360001
(title: "rebase")

[commit-bot: I haz the power, 2015-10-13 12:20:09]

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

[commit-bot: I haz the power, 2015-10-13 13:47:47]

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2015-10-13 13:49:25]

Patchset 19 (id:??) landed as
https://crrev.com/8aa0fb1d78963610ab62a349d1dff9bd2e065a3e
Cr-Commit-Position: refs/heads/master@{#353735}