Enable webgl conformance tests under content/test/gpu in content_browsertests

Enable webgl conformance tests under content/test/gpu in content_browsertests

It enables the webgl conformace test in content_browsertests basing on
chrome/test/gpu.

BUG=149892
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158742

[yongsheng, 2012-09-17 08:13:29]

It just moves webgl conformance tests to content/test/gpu and 
doesn't move other things under chrome/test/gpu in this step.
So the next:
1. Other tests under chrome/test/gpu(crash, feature, mapsgl_endurance&pixel)
need support from shell(need some time to discuss and implement): set/getbounds
for shell, snapshot, and tracking, etc.
2. Wondering about gpu_tests and soft_gpu_tests. All are merged into
content_browsertests?
3. About 'chromium_gpu_builder' and 'chromium_gpu_debug_builder', they depends
on gpu_tests, since gpu_tests is merged into content_browsertests, i'm not sure
it's suitable to make two targets depend on content_browsertests.

[Zhenyao Mo, 2012-09-17 16:56:08]

On 2012/09/17 08:13:29, yongsheng wrote:
> It just moves webgl conformance tests to content/test/gpu and 
> doesn't move other things under chrome/test/gpu in this step.
> So the next:
> 1. Other tests under chrome/test/gpu(crash, feature, mapsgl_endurance&pixel)
> need support from shell(need some time to discuss and implement):
set/getbounds
> for shell, snapshot, and tracking, etc.
> 2. Wondering about gpu_tests and soft_gpu_tests. All are merged into
> content_browsertests?
> 3. About 'chromium_gpu_builder' and 'chromium_gpu_debug_builder', they depends
> on gpu_tests, since gpu_tests is merged into content_browsertests, i'm not
sure
> it's suitable to make two targets depend on content_browsertests.

Thanks for working on this!

One major thing: gpu_tests is designed to run with real GPU (see
http://build.chromium.org/p/chromium.gpu/waterfall).  You should not merge
webgl_conformance_tests to content_browsertests.  It still needs to be part of
gpu_tests.

So it looks like you need to move all the tests (gpu_crash_browsertest,
gpu_feature_browsertest, gpu_pixel_browsertest, webgl_conformance_test) from
chrome browsertests to content browsertests, and they still stay in gpu_tests. 
Otherwise we have to create a new content_gpu_tests, and modify all the bot
configurations, and restart all the servers, which is a huge headache.

[Zhenyao Mo, 2012-09-17 16:56:55]

Add jam for browertests to content_browsertests migration.

[jam, 2012-09-17 19:47:52]

On 2012/09/17 16:56:08, Zhenyao Mo wrote:
> On 2012/09/17 08:13:29, yongsheng wrote:
> > It just moves webgl conformance tests to content/test/gpu and 
> > doesn't move other things under chrome/test/gpu in this step.
> > So the next:
> > 1. Other tests under chrome/test/gpu(crash, feature, mapsgl_endurance&pixel)
> > need support from shell(need some time to discuss and implement):
> set/getbounds
> > for shell, snapshot, and tracking, etc.
> > 2. Wondering about gpu_tests and soft_gpu_tests. All are merged into
> > content_browsertests?
> > 3. About 'chromium_gpu_builder' and 'chromium_gpu_debug_builder', they
depends
> > on gpu_tests, since gpu_tests is merged into content_browsertests, i'm not
> sure
> > it's suitable to make two targets depend on content_browsertests.
> 
> Thanks for working on this!
> 
> One major thing: gpu_tests is designed to run with real GPU (see
> http://build.chromium.org/p/chromium.gpu/waterfall).  You should not merge
> webgl_conformance_tests to content_browsertests.  It still needs to be part of
> gpu_tests.
> 
> So it looks like you need to move all the tests (gpu_crash_browsertest,
> gpu_feature_browsertest, gpu_pixel_browsertest, webgl_conformance_test) from
> chrome browsertests to content browsertests, and they still stay in gpu_tests.

> Otherwise we have to create a new content_gpu_tests, and modify all the bot
> configurations, and restart all the servers, which is a huge headache.

The new preferred way of doing this is to add tests to content_browsertests and
use a MANUAL_ prefix. That way they don't get run on all bots by default. On the
bots where you want them to run, use --run-manual
--gtest_filter=all_the_test_suites_that_you_want_to_run

The benefit of this is one less target to build and link, which saves a lot of
time in aggregate.

[Zhenyao Mo, 2012-09-17 20:15:17]

On 2012/09/17 19:47:52, John Abd-El-Malek wrote:
> On 2012/09/17 16:56:08, Zhenyao Mo wrote:
> > On 2012/09/17 08:13:29, yongsheng wrote:
> > > It just moves webgl conformance tests to content/test/gpu and 
> > > doesn't move other things under chrome/test/gpu in this step.
> > > So the next:
> > > 1. Other tests under chrome/test/gpu(crash, feature,
mapsgl_endurance&pixel)
> > > need support from shell(need some time to discuss and implement):
> > set/getbounds
> > > for shell, snapshot, and tracking, etc.
> > > 2. Wondering about gpu_tests and soft_gpu_tests. All are merged into
> > > content_browsertests?
> > > 3. About 'chromium_gpu_builder' and 'chromium_gpu_debug_builder', they
> depends
> > > on gpu_tests, since gpu_tests is merged into content_browsertests, i'm not
> > sure
> > > it's suitable to make two targets depend on content_browsertests.
> > 
> > Thanks for working on this!
> > 
> > One major thing: gpu_tests is designed to run with real GPU (see
> > http://build.chromium.org/p/chromium.gpu/waterfall).  You should not merge
> > webgl_conformance_tests to content_browsertests.  It still needs to be part
of
> > gpu_tests.
> > 
> > So it looks like you need to move all the tests (gpu_crash_browsertest,
> > gpu_feature_browsertest, gpu_pixel_browsertest, webgl_conformance_test) from
> > chrome browsertests to content browsertests, and they still stay in
gpu_tests.
> 
> > Otherwise we have to create a new content_gpu_tests, and modify all the bot
> > configurations, and restart all the servers, which is a huge headache.
> 
> The new preferred way of doing this is to add tests to content_browsertests
and
> use a MANUAL_ prefix. That way they don't get run on all bots by default. On
the
> bots where you want them to run, use --run-manual
> --gtest_filter=all_the_test_suites_that_you_want_to_run
> 
> The benefit of this is one less target to build and link, which saves a lot of
> time in aggregate.

If that's the case, what I would suggest is we don't delete the chrome side
tests for now, simply duplicate and add them to the content browsertests with
MANUAL prefix.   Once we migrate all the gpu_tests to content side, we switch
gpu_tests to content_browsertests with filters and delete the chrome side files.

This way, we can avoid creating a new test block on the GPU waterfalls.

[yongsheng, 2012-09-18 08:16:23]

On 2012/09/17 20:15:17, Zhenyao Mo wrote:
> On 2012/09/17 19:47:52, John Abd-El-Malek wrote:
> > On 2012/09/17 16:56:08, Zhenyao Mo wrote:
> > > On 2012/09/17 08:13:29, yongsheng wrote:
> > > > It just moves webgl conformance tests to content/test/gpu and 
> > > > doesn't move other things under chrome/test/gpu in this step.
> > > > So the next:
> > > > 1. Other tests under chrome/test/gpu(crash, feature,
> mapsgl_endurance&pixel)
> > > > need support from shell(need some time to discuss and implement):
> > > set/getbounds
> > > > for shell, snapshot, and tracking, etc.
> > > > 2. Wondering about gpu_tests and soft_gpu_tests. All are merged into
> > > > content_browsertests?
> > > > 3. About 'chromium_gpu_builder' and 'chromium_gpu_debug_builder', they
> > depends
> > > > on gpu_tests, since gpu_tests is merged into content_browsertests, i'm
not
> > > sure
> > > > it's suitable to make two targets depend on content_browsertests.
> > > 
> > > Thanks for working on this!
> > > 
> > > One major thing: gpu_tests is designed to run with real GPU (see
> > > http://build.chromium.org/p/chromium.gpu/waterfall).  You should not merge
> > > webgl_conformance_tests to content_browsertests.  It still needs to be
part
> of
> > > gpu_tests.
> > > 
> > > So it looks like you need to move all the tests (gpu_crash_browsertest,
> > > gpu_feature_browsertest, gpu_pixel_browsertest, webgl_conformance_test)
from
> > > chrome browsertests to content browsertests, and they still stay in
> gpu_tests.
> > 
> > > Otherwise we have to create a new content_gpu_tests, and modify all the
bot
> > > configurations, and restart all the servers, which is a huge headache.
> > 
> > The new preferred way of doing this is to add tests to content_browsertests
> and
> > use a MANUAL_ prefix. That way they don't get run on all bots by default. On
> the
> > bots where you want them to run, use --run-manual
> > --gtest_filter=all_the_test_suites_that_you_want_to_run
> > 
> > The benefit of this is one less target to build and link, which saves a lot
of
> > time in aggregate.
> 
> If that's the case, what I would suggest is we don't delete the chrome side
> tests for now, simply duplicate and add them to the content browsertests with
> MANUAL prefix.   Once we migrate all the gpu_tests to content side, we switch
> gpu_tests to content_browsertests with filters and delete the chrome side
files.
> 
> This way, we can avoid creating a new test block on the GPU waterfalls.
ok, I can do that. but what's 'MANUAL prefix'? 
> So it looks like you need to move all the tests (gpu_crash_browsertest,
gpu_feature_browsertest, gpu_pixel_browsertest, webgl_conformance_test) from
chrome browsertests to content browsertests,
I thought moving them one by one instead of one big patch.
If you think they're all in one patch, I'll do that.

Thanks you two.

[Zhenyao Mo, 2012-09-18 19:37:15]

On 2012/09/18 08:16:23, yongsheng wrote:
> On 2012/09/17 20:15:17, Zhenyao Mo wrote:
> > On 2012/09/17 19:47:52, John Abd-El-Malek wrote:
> > > On 2012/09/17 16:56:08, Zhenyao Mo wrote:
> > > > On 2012/09/17 08:13:29, yongsheng wrote:
> > > > > It just moves webgl conformance tests to content/test/gpu and 
> > > > > doesn't move other things under chrome/test/gpu in this step.
> > > > > So the next:
> > > > > 1. Other tests under chrome/test/gpu(crash, feature,
> > mapsgl_endurance&pixel)
> > > > > need support from shell(need some time to discuss and implement):
> > > > set/getbounds
> > > > > for shell, snapshot, and tracking, etc.
> > > > > 2. Wondering about gpu_tests and soft_gpu_tests. All are merged into
> > > > > content_browsertests?
> > > > > 3. About 'chromium_gpu_builder' and 'chromium_gpu_debug_builder', they
> > > depends
> > > > > on gpu_tests, since gpu_tests is merged into content_browsertests, i'm
> not
> > > > sure
> > > > > it's suitable to make two targets depend on content_browsertests.
> > > > 
> > > > Thanks for working on this!
> > > > 
> > > > One major thing: gpu_tests is designed to run with real GPU (see
> > > > http://build.chromium.org/p/chromium.gpu/waterfall).  You should not
merge
> > > > webgl_conformance_tests to content_browsertests.  It still needs to be
> part
> > of
> > > > gpu_tests.
> > > > 
> > > > So it looks like you need to move all the tests (gpu_crash_browsertest,
> > > > gpu_feature_browsertest, gpu_pixel_browsertest, webgl_conformance_test)
> from
> > > > chrome browsertests to content browsertests, and they still stay in
> > gpu_tests.
> > > 
> > > > Otherwise we have to create a new content_gpu_tests, and modify all the
> bot
> > > > configurations, and restart all the servers, which is a huge headache.
> > > 
> > > The new preferred way of doing this is to add tests to
content_browsertests
> > and
> > > use a MANUAL_ prefix. That way they don't get run on all bots by default.
On
> > the
> > > bots where you want them to run, use --run-manual
> > > --gtest_filter=all_the_test_suites_that_you_want_to_run
> > > 
> > > The benefit of this is one less target to build and link, which saves a
lot
> of
> > > time in aggregate.
> > 
> > If that's the case, what I would suggest is we don't delete the chrome side
> > tests for now, simply duplicate and add them to the content browsertests
with
> > MANUAL prefix.   Once we migrate all the gpu_tests to content side, we
switch
> > gpu_tests to content_browsertests with filters and delete the chrome side
> files.
> > 
> > This way, we can avoid creating a new test block on the GPU waterfalls.
> ok, I can do that. but what's 'MANUAL prefix'? 

MANUAL prefix makes sure it only runs when you specifically tells the harness to
do so.  So the content_browsertests by default won't run these tests, which is
what we want.  On GPU bots, we can specify the filters to make sure these tests
run.

> > So it looks like you need to move all the tests (gpu_crash_browsertest,
> gpu_feature_browsertest, gpu_pixel_browsertest, webgl_conformance_test) from
> chrome browsertests to content browsertests,
> I thought moving them one by one instead of one big patch.
> If you think they're all in one patch, I'll do that.

If you add them to the content_browsertests without deleting them on the chrome
side, then it is OK to do one per patch.  Once we have all the tests in
gpu_tests migrate to the content_browsertests, we can remove all of them from
the chrome side (and the gpu_tests target) and switch gpu_tests to
content_browsertests with the appropriate filters.

> 
> Thanks you two.

[yongsheng, 2012-09-19 13:33:42]

> MANUAL prefix makes sure it only runs when you specifically tells the harness
to
> do so.  So the content_browsertests by default won't run these tests, which is
> what we want.  On GPU bots, we can specify the filters to make sure these
tests
> run.
ok, thanks.
> > > So it looks like you need to move all the tests (gpu_crash_browsertest,
> > gpu_feature_browsertest, gpu_pixel_browsertest, webgl_conformance_test) from
> > chrome browsertests to content browsertests,
> > I thought moving them one by one instead of one big patch.
> > If you think they're all in one patch, I'll do that.
> 
> If you add them to the content_browsertests without deleting them on the
chrome
> side, then it is OK to do one per patch.  Once we have all the tests in
> gpu_tests migrate to the content_browsertests, we can remove all of them from
> the chrome side (and the gpu_tests target) and switch gpu_tests to
> content_browsertests with the appropriate filters.
yes, see the new patch.

[Zhenyao Mo, 2012-09-19 18:19:28]

On 2012/09/19 13:33:42, yongsheng wrote:
> > MANUAL prefix makes sure it only runs when you specifically tells the
harness
> to
> > do so.  So the content_browsertests by default won't run these tests, which
is
> > what we want.  On GPU bots, we can specify the filters to make sure these
> tests
> > run.
> ok, thanks.
> > > > So it looks like you need to move all the tests (gpu_crash_browsertest,
> > > gpu_feature_browsertest, gpu_pixel_browsertest, webgl_conformance_test)
from
> > > chrome browsertests to content browsertests,
> > > I thought moving them one by one instead of one big patch.
> > > If you think they're all in one patch, I'll do that.
> > 
> > If you add them to the content_browsertests without deleting them on the
> chrome
> > side, then it is OK to do one per patch.  Once we have all the tests in
> > gpu_tests migrate to the content_browsertests, we can remove all of them
from
> > the chrome side (and the gpu_tests target) and switch gpu_tests to
> > content_browsertests with the appropriate filters.
> yes, see the new patch.

LGTM, assuming you test the content_browsertests with the right filter locally.

[Zhenyao Mo, 2012-09-19 18:19:37]

http://codereview.chromium.org/10916334/diff/16001/content/test/gpu/gpu_test_...
File content/test/gpu/gpu_test_expectations_parser.cc (right):

http://codereview.chromium.org/10916334/diff/16001/content/test/gpu/gpu_test_...
content/test/gpu/gpu_test_expectations_parser.cc:504: *path =
path->Append(FILE_PATH_LITERAL("content"))
Since we switch to use the content side version of the test_expectations.txt,
please remove the chrome side file.

[jam, 2012-09-19 19:52:39]

https://codereview.chromium.org/10916334/diff/16001/content/test/gpu/gpu_test...
File content/test/gpu/gpu_test_expectations_parser.cc (right):

https://codereview.chromium.org/10916334/diff/16001/content/test/gpu/gpu_test...
content/test/gpu/gpu_test_expectations_parser.cc:506:
.Append(FILE_PATH_LITERAL("gpu"))
test data is normally put under content/test/data. any reason why this isn't
there? then you can use content::DIR_TEST_DATA

[yongsheng, 2012-09-20 01:55:06]

thanks for the review.

http://codereview.chromium.org/10916334/diff/16001/content/test/gpu/gpu_test_...
File content/test/gpu/gpu_test_expectations_parser.cc (right):

http://codereview.chromium.org/10916334/diff/16001/content/test/gpu/gpu_test_...
content/test/gpu/gpu_test_expectations_parser.cc:504: *path =
path->Append(FILE_PATH_LITERAL("content"))
On 2012/09/19 18:19:37, Zhenyao Mo wrote:
> Since we switch to use the content side version of the test_expectations.txt,
> please remove the chrome side file.
ok, i'll do it.

http://codereview.chromium.org/10916334/diff/16001/content/test/gpu/gpu_test_...
content/test/gpu/gpu_test_expectations_parser.cc:506:
.Append(FILE_PATH_LITERAL("gpu"))
On 2012/09/19 19:52:40, John Abd-El-Malek wrote:
> test data is normally put under content/test/data. any reason why this isn't
> there? then you can use content::DIR_TEST_DATA
yes, i agree with you. Zheyao, if you agree with this, I'll move it to test data
dir.

[yongsheng, 2012-09-20 06:03:28]

> LGTM, assuming you test the content_browsertests with the right filter
locally.
yes, i test it on Linux and results are the same as gpu_tests(webgl part).

[Zhenyao Mo, 2012-09-20 18:28:12]

On 2012/09/20 01:55:06, yongsheng wrote:
> thanks for the review.
> 
>
http://codereview.chromium.org/10916334/diff/16001/content/test/gpu/gpu_test_...
> File content/test/gpu/gpu_test_expectations_parser.cc (right):
> 
>
http://codereview.chromium.org/10916334/diff/16001/content/test/gpu/gpu_test_...
> content/test/gpu/gpu_test_expectations_parser.cc:504: *path =
> path->Append(FILE_PATH_LITERAL("content"))
> On 2012/09/19 18:19:37, Zhenyao Mo wrote:
> > Since we switch to use the content side version of the
test_expectations.txt,
> > please remove the chrome side file.
> ok, i'll do it.
> 
>
http://codereview.chromium.org/10916334/diff/16001/content/test/gpu/gpu_test_...
> content/test/gpu/gpu_test_expectations_parser.cc:506:
> .Append(FILE_PATH_LITERAL("gpu"))
> On 2012/09/19 19:52:40, John Abd-El-Malek wrote:
> > test data is normally put under content/test/data. any reason why this isn't
> > there? then you can use content::DIR_TEST_DATA
> yes, i agree with you. Zheyao, if you agree with this, I'll move it to test
data
> dir.

Sounds good to me.  You should also run the try bots (gcl try cl_name) for all
your CLs.

[yongsheng, 2012-09-21 01:18:59]

On 2012/09/20 18:28:12, Zhenyao Mo wrote:
> On 2012/09/20 01:55:06, yongsheng wrote:
> > thanks for the review.
> > 
> >
>
http://codereview.chromium.org/10916334/diff/16001/content/test/gpu/gpu_test_...
> > File content/test/gpu/gpu_test_expectations_parser.cc (right):
> > 
> >
>
http://codereview.chromium.org/10916334/diff/16001/content/test/gpu/gpu_test_...
> > content/test/gpu/gpu_test_expectations_parser.cc:504: *path =
> > path->Append(FILE_PATH_LITERAL("content"))
> > On 2012/09/19 18:19:37, Zhenyao Mo wrote:
> > > Since we switch to use the content side version of the
> test_expectations.txt,
> > > please remove the chrome side file.
> > ok, i'll do it.
> > 
> >
>
http://codereview.chromium.org/10916334/diff/16001/content/test/gpu/gpu_test_...
> > content/test/gpu/gpu_test_expectations_parser.cc:506:
> > .Append(FILE_PATH_LITERAL("gpu"))
> > On 2012/09/19 19:52:40, John Abd-El-Malek wrote:
> > > test data is normally put under content/test/data. any reason why this
isn't
> > > there? then you can use content::DIR_TEST_DATA
> > yes, i agree with you. Zheyao, if you agree with this, I'll move it to test
> data
> > dir.
> 
> Sounds good to me.  You should also run the try bots (gcl try cl_name) for all
> your CLs.
ok, I'll do it.

[yongsheng, 2012-09-21 12:58:50]

try server gets some problems.

[jam, 2012-09-21 20:39:38]

https://codereview.chromium.org/10916334/diff/36002/content/test/content_brow...
File content/test/content_browser_test_utils.h (right):

https://codereview.chromium.org/10916334/diff/36002/content/test/content_brow...
content/test/content_browser_test_utils.h:78: // Watches for responses from the
DOMAutomationController and keeps them in a
this is a duplicate of the same class in ui_test_utils.h. we don't duplicate
code.

move this class to content/public/test/browser_test_utils.h. see
http://www.chromium.org/developers/testing/browser-tests for more information
about the different test headers

https://codereview.chromium.org/10916334/diff/36002/content/test/gpu/webgl_co...
File content/test/gpu/webgl_conformance_tests.cc (right):

https://codereview.chromium.org/10916334/diff/36002/content/test/gpu/webgl_co...
content/test/gpu/webgl_conformance_tests.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
in general, tests under content are under browser/renderer/common etc.
content/test is for test harnesses and test related utilities. seems that this
test belongs in content/browser/gpu

[Zhenyao Mo, 2012-09-21 20:54:59]

On 2012/09/21 20:39:38, John Abd-El-Malek wrote:
>
https://codereview.chromium.org/10916334/diff/36002/content/test/content_brow...
> File content/test/content_browser_test_utils.h (right):
> 
>
https://codereview.chromium.org/10916334/diff/36002/content/test/content_brow...
> content/test/content_browser_test_utils.h:78: // Watches for responses from
the
> DOMAutomationController and keeps them in a
> this is a duplicate of the same class in ui_test_utils.h. we don't duplicate
> code.
> 
> move this class to content/public/test/browser_test_utils.h. see
> http://www.chromium.org/developers/testing/browser-tests for more information
> about the different test headers
> 
>
https://codereview.chromium.org/10916334/diff/36002/content/test/gpu/webgl_co...
> File content/test/gpu/webgl_conformance_tests.cc (right):
> 
>
https://codereview.chromium.org/10916334/diff/36002/content/test/gpu/webgl_co...
> content/test/gpu/webgl_conformance_tests.cc:1: // Copyright (c) 2012 The
> Chromium Authors. All rights reserved.
> in general, tests under content are under browser/renderer/common etc.
> content/test is for test harnesses and test related utilities. seems that this
> test belongs in content/browser/gpu

Maybe put it under content/common/gpu?  This test (and other gpu_tests) not only
tests browser process, but also renderer/gpu process side code.

[jam, 2012-09-21 21:02:06]

On 2012/09/21 20:54:59, Zhenyao Mo wrote:
> On 2012/09/21 20:39:38, John Abd-El-Malek wrote:
> >
>
https://codereview.chromium.org/10916334/diff/36002/content/test/content_brow...
> > File content/test/content_browser_test_utils.h (right):
> > 
> >
>
https://codereview.chromium.org/10916334/diff/36002/content/test/content_brow...
> > content/test/content_browser_test_utils.h:78: // Watches for responses from
> the
> > DOMAutomationController and keeps them in a
> > this is a duplicate of the same class in ui_test_utils.h. we don't duplicate
> > code.
> > 
> > move this class to content/public/test/browser_test_utils.h. see
> > http://www.chromium.org/developers/testing/browser-tests for more
information
> > about the different test headers
> > 
> >
>
https://codereview.chromium.org/10916334/diff/36002/content/test/gpu/webgl_co...
> > File content/test/gpu/webgl_conformance_tests.cc (right):
> > 
> >
>
https://codereview.chromium.org/10916334/diff/36002/content/test/gpu/webgl_co...
> > content/test/gpu/webgl_conformance_tests.cc:1: // Copyright (c) 2012 The
> > Chromium Authors. All rights reserved.
> > in general, tests under content are under browser/renderer/common etc.
> > content/test is for test harnesses and test related utilities. seems that
this
> > test belongs in content/browser/gpu
> 
> Maybe put it under content/common/gpu?

This file includes a file from content/browser, so it has to be under
content/browser (see the DEPS rules, they also apply for tests)

>  This test (and other gpu_tests) not only
> tests browser process, but also renderer/gpu process side code.

Note this is ok, many browser tests in the browser process test child process
code indirectly.

[yongsheng, 2012-09-23 05:34:18]

ok, I'll change them.

[yongsheng, 2012-09-24 08:51:49]

On 2012/09/23 05:34:18, yongsheng wrote:
> ok, I'll change them.
For those failures, it should not be due to this patch. I find many other
failures on try server. I'll try them later.
Owners, could  you please help review them?

[jam, 2012-09-24 16:45:37]

lgtm with nits

https://codereview.chromium.org/10916334/diff/34005/content/browser/gpu/webgl...
File content/browser/gpu/webgl_conformance_tests.cc (right):

https://codereview.chromium.org/10916334/diff/34005/content/browser/gpu/webgl...
content/browser/gpu/webgl_conformance_tests.cc:41:
PathService::Get(content::DIR_TEST_DATA, &test_path_);
nit: get rid of "content::" in this file

https://codereview.chromium.org/10916334/diff/34005/content/public/test/brows...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/10916334/diff/34005/content/public/test/brows...
content/public/test/browser_test_utils.cc:574: registrar_.Add(this,
content::NOTIFICATION_DOM_OPERATION_RESPONSE,
ditto

https://codereview.chromium.org/10916334/diff/34005/content/public/test/brows...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/10916334/diff/34005/content/public/test/brows...
content/public/test/browser_test_utils.h:218: class DOMMessageQueue : public
content::NotificationObserver {
nit: get rid of "content::" everywhere here since it's already in the content
namespace

[yongsheng, 2012-09-25 02:06:59]

On 2012/09/24 16:45:37, John Abd-El-Malek wrote:
> lgtm with nits
> 
thanks, change them.
+bulach for Android changes.

[bulach, 2012-09-25 08:47:42]

lgtm for android, thanks for the heads up!

[yongsheng, 2012-09-25 09:18:28]

On 2012/09/25 08:47:42, bulach wrote:
> lgtm for android, thanks for the heads up!
thanks.
I'll commit it tomorrow if no comments from zhenyao and john.

[commit-bot: I haz the power, 2012-09-26 01:25:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yongsheng.zhu@intel.com/10916334/38001

[commit-bot: I haz the power, 2012-09-26 03:50:25]

Change committed as 158742