Move gpu blacklist data file to content side.

Move gpu blacklist data file to content side.

This also fixes the bug that GPU histograms are missing.

BUG=151057
TEST=tree
R=jbauman
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=157851

[Zhenyao Mo, 2012-09-14 16:31:21]

jbauman: please review.

kbr, jam: owner approval.

[jbauman, 2012-09-14 16:35:41]

lgtm

[jbauman, 2012-09-14 16:38:46]

On 2012/09/14 16:35:41, jbauman wrote:
> lgtm

Actually, what about chrome/unit_tests.isolate?

[Zhenyao Mo, 2012-09-14 16:42:13]

On 2012/09/14 16:38:46, jbauman wrote:
> On 2012/09/14 16:35:41, jbauman wrote:
> > lgtm
> 
> Actually, what about chrome/unit_tests.isolate?

I just updated the CL.  Thanks for reminding that.

[jbauman, 2012-09-14 16:49:48]

http://codereview.chromium.org/10911315/diff/3003/chrome/unit_tests.isolate
File chrome/unit_tests.isolate (left):

http://codereview.chromium.org/10911315/diff/3003/chrome/unit_tests.isolate#o...
chrome/unit_tests.isolate:109: 'browser/resources/software_rendering_list.json',
Shouldn't this instead be changed to the new location, so the unit tests won't
fail on the bots?

[Zhenyao Mo, 2012-09-14 16:59:47]

I couldn't find .isolate files on the content side.  Let me add the expert and
maybe he can point out what's the appropriate thing here.

maruel: please advice on the .isolate related change.

[M-A Ruel, 2012-09-14 17:15:16]

On 2012/09/14 16:59:47, Zhenyao Mo wrote:
> I couldn't find .isolate files on the content side.  Let me add the expert and
> maybe he can point out what's the appropriate thing here.
> 
> maruel: please advice on the .isolate related change.

There's no .isolate for content_*tests.isolate yet, so nothing to do! Thanks for
updating unit_tests.isolate.

lgtm

[M-A Ruel, 2012-09-14 17:15:58]

On 2012/09/14 17:15:16, Marc-Antoine Ruel wrote:
> lgtm

Well, the code in patchset 2 currently fails to compile.

[Zhenyao Mo, 2012-09-14 21:25:36]

ilevy: any idea how I can fix the android_dbg_triggered_tests failure due to
moving the software_rendering_list.json file?

[Isaac (away), 2012-09-14 21:35:09]

git grep software_rendering_list.json

[Zhenyao Mo, 2012-09-14 21:37:02]

On 2012/09/14 21:35:09, Isaac wrote:
> git grep software_rendering_list.json

I don't use git, but from cs.chromium.org searching
software_rendering_list.json, nothing related to this bug shows up

[Isaac (away), 2012-09-14 21:58:29]

cs.chromium.org is stale.  The file to look at
is build/android/pylib/single_test_runner.py

Cheers,
Isaac

On Fri, Sep 14, 2012 at 2:37 PM, <zmo@chromium.org> wrote:

> On 2012/09/14 21:35:09, Isaac wrote:
>
>> git grep software_rendering_list.json
>>
>
> I don't use git, but from cs.chromium.org searching
> software_rendering_list.json, nothing related to this bug shows up
>
>
http://codereview.chromium.**org/10911315/<http://codereview.chromium.org/109...
>

[Zhenyao Mo, 2012-09-14 22:17:01]

On 2012/09/14 21:58:29, Isaac wrote:
> http://cs.chromium.org is stale.  The file to look at
> is build/android/pylib/single_test_runner.py
> 
> Cheers,
> Isaac
> 
> On Fri, Sep 14, 2012 at 2:37 PM, <mailto:zmo@chromium.org> wrote:
> 
> > On 2012/09/14 21:35:09, Isaac wrote:
> >
> >> git grep software_rendering_list.json
> >>
> >
> > I don't use git, but from http://cs.chromium.org searching
> > software_rendering_list.json, nothing related to this bug shows up
> >
> >
>
http://codereview.chromium.**org/10911315/%3Chttp://codereview.chromium.org/1...>
> >

Thanks.  Please have a look at the changes I made in that file.

[Isaac (away), 2012-09-14 22:25:07]

the android changes lgtm

[Zhenyao Mo, 2012-09-14 23:01:53]

Remove jam@, add joi@, since there is no added API.

joi: owner approval

[Ken Russell (switch to Gerrit), 2012-09-15 00:30:28]

chrome/test/gpu/ and content/browser/gpu/ LGTM

http://codereview.chromium.org/10911315/diff/26/content/content.gyp
File content/content.gyp (right):

http://codereview.chromium.org/10911315/diff/26/content/content.gyp#newcode90
content/content.gyp:90: 'content_common',
'content_resources.gyp:content_resources',
Is this change really desired? content_common already conditionally depends on
this resource.

[Zhenyao Mo, 2012-09-17 21:03:05]

http://codereview.chromium.org/10911315/diff/26/content/content.gyp
File content/content.gyp (right):

http://codereview.chromium.org/10911315/diff/26/content/content.gyp#newcode90
content/content.gyp:90: 'content_common',
'content_resources.gyp:content_resources',
On 2012/09/15 00:30:28, kbr wrote:
> Is this change really desired? content_common already conditionally depends on
> this resource.

We can either do this or remove the condition 'OS != "ios"' on content_common. 
I don't have a preference one way or the other.

[Jói, 2012-09-17 21:24:19]

LGTM for content/public.

[Zhenyao Mo, 2012-09-17 21:30:24]

asargent: chrome/browser/extensions owner.
piman: content/ owner.
thestig: chrome/ owner.

[piman, 2012-09-17 21:45:05]

What is driving this change?

[Lei Zhang, 2012-09-17 21:48:39]

chrome/ side LGTM

[Zhenyao Mo, 2012-09-17 21:52:02]

On 2012/09/17 21:45:05, piman wrote:
> What is driving this change?

The GPU blacklist code has already been moved to content side.  So by moving the
data file to content side, we can also enable blacklisting on content side.  One
example is http://code.google.com/p/chromium/issues/detail?id=147564.

[asargent_no_longer_on_chrome, 2012-09-17 22:36:48]

chrome/browser/extensions/ LGTM

[piman, 2012-09-17 23:06:39]

+jam since he originally left it in Chrome, I don't know the rationale.
Other than that the code LGTM

[Zhenyao Mo, 2012-09-17 23:08:21]

On 2012/09/17 23:06:39, piman wrote:
> +jam since he originally left it in Chrome, I don't know the rationale.
> Other than that the code LGTM

jam: can you have a look?  I try to get the file in the new place before the
branch cut, so any possible merging in the blacklist will be easy.

[Zhenyao Mo, 2012-09-19 18:11:49]

On 2012/09/17 23:08:21, Zhenyao Mo wrote:
> On 2012/09/17 23:06:39, piman wrote:
> > +jam since he originally left it in Chrome, I don't know the rationale.
> > Other than that the code LGTM
> 
> jam: can you have a look?  I try to get the file in the new place before the
> branch cut, so any possible merging in the blacklist will be easy.

jam: ping

[jam, 2012-09-19 20:09:57]

https://codereview.chromium.org/10911315/diff/26/chrome/browser/chrome_gpu_ut...
File chrome/browser/chrome_gpu_util.cc (right):

https://codereview.chromium.org/10911315/diff/26/chrome/browser/chrome_gpu_ut...
chrome/browser/chrome_gpu_util.cc:125: void InitializeGpuDataManager(const
CommandLine& command_line) {
now that the blacklist is in content, this should be there as well, so that not
every embedder has to do this.

https://codereview.chromium.org/10911315/diff/26/chrome/test/gpu/gpu_feature_...
File chrome/test/gpu/gpu_feature_browsertest.cc (right):

https://codereview.chromium.org/10911315/diff/26/chrome/test/gpu/gpu_feature_...
chrome/test/gpu/gpu_feature_browsertest.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
why is this file not in content now?

[jam, 2012-09-19 20:10:24]

On 2012/09/17 23:06:39, piman wrote:
> +jam since he originally left it in Chrome, I don't know the rationale.
> Other than that the code LGTM

originally we didn't have resources in content. but now that we do, it's
reasonable to move this to content.

[Zhenyao Mo, 2012-09-19 22:46:18]

If no other comments, I'd like to land this soon.  This CL needs to be merged
back to fix a histogram-related bug.

https://codereview.chromium.org/10911315/diff/26/chrome/browser/chrome_gpu_ut...
File chrome/browser/chrome_gpu_util.cc (right):

https://codereview.chromium.org/10911315/diff/26/chrome/browser/chrome_gpu_ut...
chrome/browser/chrome_gpu_util.cc:125: void InitializeGpuDataManager(const
CommandLine& command_line) {
On 2012/09/19 20:09:57, John Abd-El-Malek wrote:
> now that the blacklist is in content, this should be there as well, so that
not
> every embedder has to do this.

The chrome version has to be collected on the chrome side, so we still need the
code to be on this side.

https://codereview.chromium.org/10911315/diff/26/chrome/test/gpu/gpu_feature_...
File chrome/test/gpu/gpu_feature_browsertest.cc (right):

https://codereview.chromium.org/10911315/diff/26/chrome/test/gpu/gpu_feature_...
chrome/test/gpu/gpu_feature_browsertest.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/09/19 20:09:57, John Abd-El-Malek wrote:
> why is this file not in content now?

It's in progress. See http://code.google.com/p/chromium/issues/detail?id=111506

[jam, 2012-09-20 01:09:09]

https://codereview.chromium.org/10911315/diff/26/chrome/browser/chrome_gpu_ut...
File chrome/browser/chrome_gpu_util.cc (right):

https://codereview.chromium.org/10911315/diff/26/chrome/browser/chrome_gpu_ut...
chrome/browser/chrome_gpu_util.cc:125: void InitializeGpuDataManager(const
CommandLine& command_line) {
On 2012/09/19 22:46:19, Zhenyao Mo wrote:
> On 2012/09/19 20:09:57, John Abd-El-Malek wrote:
> > now that the blacklist is in content, this should be there as well, so that
> not
> > every embedder has to do this.
> 
> The chrome version has to be collected on the chrome side, so we still need
the
> code to be on this side.

the chrome version is already accessible through
content::GetContentClient()->GetProduct()

https://codereview.chromium.org/10911315/diff/26/chrome/test/gpu/gpu_feature_...
File chrome/test/gpu/gpu_feature_browsertest.cc (right):

https://codereview.chromium.org/10911315/diff/26/chrome/test/gpu/gpu_feature_...
chrome/test/gpu/gpu_feature_browsertest.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/09/19 22:46:19, Zhenyao Mo wrote:
> On 2012/09/19 20:09:57, John Abd-El-Malek wrote:
> > why is this file not in content now?
> 
> It's in progress. See
http://code.google.com/p/chromium/issues/detail?id=111506

that test should move with this, now that the data file lives in content. then
the public apis to load this can be removed.

[Zhenyao Mo, 2012-09-20 01:19:59]

On 2012/09/20 01:09:09, John Abd-El-Malek wrote:
>
https://codereview.chromium.org/10911315/diff/26/chrome/browser/chrome_gpu_ut...
> File chrome/browser/chrome_gpu_util.cc (right):
> 
>
https://codereview.chromium.org/10911315/diff/26/chrome/browser/chrome_gpu_ut...
> chrome/browser/chrome_gpu_util.cc:125: void InitializeGpuDataManager(const
> CommandLine& command_line) {
> On 2012/09/19 22:46:19, Zhenyao Mo wrote:
> > On 2012/09/19 20:09:57, John Abd-El-Malek wrote:
> > > now that the blacklist is in content, this should be there as well, so
that
> > not
> > > every embedder has to do this.
> > 
> > The chrome version has to be collected on the chrome side, so we still need
> the
> > code to be on this side.
> 
> the chrome version is already accessible through
> content::GetContentClient()->GetProduct()

If that's the case, sure, I'll move this function into
GpuDataManager::Initialize().

> 
>
https://codereview.chromium.org/10911315/diff/26/chrome/test/gpu/gpu_feature_...
> File chrome/test/gpu/gpu_feature_browsertest.cc (right):
> 
>
https://codereview.chromium.org/10911315/diff/26/chrome/test/gpu/gpu_feature_...
> chrome/test/gpu/gpu_feature_browsertest.cc:1: // Copyright (c) 2012 The
Chromium
> Authors. All rights reserved.
> On 2012/09/19 22:46:19, Zhenyao Mo wrote:
> > On 2012/09/19 20:09:57, John Abd-El-Malek wrote:
> > > why is this file not in content now?
> > 
> > It's in progress. See
> http://code.google.com/p/chromium/issues/detail?id=111506
> 
> that test should move with this, now that the data file lives in content. then
> the public apis to load this can be removed.

This test is part of gpu_tests as well as browsertests.  Migrating gpu_tests
requires much more care because it is a different target.  As we discussed in
http://codereview.chromium.org/10916334/, we are going to take multiple
steps/CLs to do this migration properly while reducing the pain to minimum.  It
has to be in other CLs rather than this.

[jam, 2012-09-20 01:29:58]

On Wed, Sep 19, 2012 at 6:19 PM, <zmo@chromium.org> wrote:

> On 2012/09/20 01:09:09, John Abd-El-Malek wrote:
>
> https://codereview.chromium.**org/10911315/diff/26/chrome/**
>
browser/chrome_gpu_util.cc<https://codereview.chromium.org/10911315/diff/26/chrome/browser/chrome_gpu_util.cc>
>
>> File chrome/browser/chrome_gpu_**util.cc (right):
>>
>
>
> https://codereview.chromium.**org/10911315/diff/26/chrome/**
>
browser/chrome_gpu_util.cc#**newcode125<https://codereview.chromium.org/10911315/diff/26/chrome/browser/chrome_gpu_util.cc#newcode125>
>
>> chrome/browser/chrome_gpu_**util.cc:125: void
>> InitializeGpuDataManager(const
>> CommandLine& command_line) {
>> On 2012/09/19 22:46:19, Zhenyao Mo wrote:
>> > On 2012/09/19 20:09:57, John Abd-El-Malek wrote:
>> > > now that the blacklist is in content, this should be there as well, so
>>
> that
>
>> > not
>> > > every embedder has to do this.
>> >
>> > The chrome version has to be collected on the chrome side, so we still
>> need
>> the
>> > code to be on this side.
>>
>
>  the chrome version is already accessible through
>> content::GetContentClient()->**GetProduct()
>>
>
> If that's the case, sure, I'll move this function into
> GpuDataManager::Initialize().


to be clear, what I'm saying is that InitializeGpuDataManager should be
removed completely. since content has the data file, it should be the one
responsible for initializing GpuDataManager (i.e. move the code in
chrome_browser_main.cc which calls gpu_util::InitializeGpuDataManager to
src/content/browser/browser_main_loop.cc beside the other GPU related
initializtion)

>
>
>
>
> https://codereview.chromium.**org/10911315/diff/26/chrome/**
>
test/gpu/gpu_feature_**browsertest.cc<https://codereview.chromium.org/10911315/diff/26/chrome/test/gpu/gpu_feature_browsertest.cc>
>
>> File chrome/test/gpu/gpu_feature_**browsertest.cc (right):
>>
>
>
> https://codereview.chromium.**org/10911315/diff/26/chrome/**
>
test/gpu/gpu_feature_**browsertest.cc#newcode1<https://codereview.chromium.org/10911315/diff/26/chrome/test/gpu/gpu_feature_browsertest.cc#newcode1>
>
>> chrome/test/gpu/gpu_feature_**browsertest.cc:1: // Copyright (c) 2012 The
>>
> Chromium
>
>> Authors. All rights reserved.
>> On 2012/09/19 22:46:19, Zhenyao Mo wrote:
>> > On 2012/09/19 20:09:57, John Abd-El-Malek wrote:
>> > > why is this file not in content now?
>> >
>> > It's in progress. See
>>
http://code.google.com/p/**chromium/issues/detail?id=**111506<http://code.goo...
>>
>
>  that test should move with this, now that the data file lives in content.
>> then
>> the public apis to load this can be removed.
>>
>
> This test is part of gpu_tests as well as browsertests.  Migrating
> gpu_tests
> requires much more care because it is a different target.  As we discussed
> in
>
http://codereview.chromium.**org/10916334/<http://codereview.chromium.org/109...,
> we are going to take multiple
> steps/CLs to do this migration properly while reducing the pain to
> minimum.  It
> has to be in other CLs rather than this.
>

Why does this test need to run in two different targets? i.e. what does it
catch in gpu_tests that it doesn't in browser_tests

Even if it needs to run on the gpu_tests target, it seems that it can be
converted now to run in content_browsertests and be taken out of gpu_tests
temporarily.

Moving the blacklist from chrome to content should entail moving the tests
alongside the code. Note this is why we have a separate browser_test target
for content, so that all its integration tests run against it, and we don't
have to add Content APi calls just for testing.

>
>
https://codereview.chromium.**org/10911315/<https://codereview.chromium.org/1...
>

[Zhenyao Mo, 2012-09-20 18:08:10]

On 2012/09/20 01:29:58, John Abd-El-Malek wrote:
> to be clear, what I'm saying is that InitializeGpuDataManager should be
> removed completely. since content has the data file, it should be the one
> responsible for initializing GpuDataManager (i.e. move the code in
> chrome_browser_main.cc which calls gpu_util::InitializeGpuDataManager to
> src/content/browser/browser_main_loop.cc beside the other GPU related
> initializtion)

I moved the version thing into GpuDataManager, so the chrome_gpu_util function
is removed.  However, after carefully looking at the code, moving
GpuDataManager::Initialize() from chrome side to content side is not trivia.  It
is before SetupSandbox now, but the content side GPU code is after it.  Also,
this moving around looks like will affect the startup time measurement.  With
all these complications, even it's simply one line of change (maybe more if we
need to move more stuff around), I'd like to do it in a separate CL.  This CL
needs to be merged back to M22, so the simpler, the less behavioral change, the
better.

> 
> 
> Why does this test need to run in two different targets? i.e. what does it
> catch in gpu_tests that it doesn't in browser_tests
> 
> Even if it needs to run on the gpu_tests target, it seems that it can be
> converted now to run in content_browsertests and be taken out of gpu_tests
> temporarily.
> 
> Moving the blacklist from chrome to content should entail moving the tests
> alongside the code. Note this is why we have a separate browser_test target
> for content, so that all its integration tests run against it, and we don't
> have to add Content APi calls just for testing.
>

The gpu_feature_browsertests need to run on top of real GPUs / real drivers. 
The vertualized GPU/software driver might behave differently, and does not cover
all the features we need to test (for example, multisampling).  So we can only
take them out of gpu_tests when we convert all the gpu_tests to content side and
then switch the GPU bots to run content_browsertests instead of gpu_tests. 
Before that, we can't remove the chrome side tests because we need them to run
on GPU bots.

[Zhenyao Mo, 2012-09-20 18:08:53]

Please have a look at the change I made.  I'd like to land this soon and request
a M23 merge.

[jam, 2012-09-20 18:35:32]

On Thu, Sep 20, 2012 at 11:08 AM, <zmo@chromium.org> wrote:

> On 2012/09/20 01:29:58, John Abd-El-Malek wrote:
>
>> to be clear, what I'm saying is that InitializeGpuDataManager should be
>> removed completely. since content has the data file, it should be the one
>> responsible for initializing GpuDataManager (i.e. move the code in
>> chrome_browser_main.cc which calls gpu_util::**InitializeGpuDataManager
>> to
>> src/content/browser/browser_**main_loop.cc beside the other GPU related
>> initializtion)
>>
>
> I moved the version thing into GpuDataManager, so the chrome_gpu_util
> function
> is removed.  However, after carefully looking at the code, moving
> GpuDataManager::Initialize() from chrome side to content side is not
> trivia.  It
> is before SetupSandbox now, but the content side GPU code is after it.



I'm not sure what you mean. Currently GpuDataManager::Initialize is called
in this callcstack:
  chrome.dll!ChromeBrowserMainParts::PreMainMessageLoopRunImpl()  Line 1340
C++
  chrome.dll!ChromeBrowserMainParts::PreMainMessageLoopRun()  Line 892 +
0x8 bytes C++
> chrome.dll!ChromeBrowserMainPartsWin::PreMainMessageLoopRun()  Line 213
C++
  content.dll!content::BrowserMainLoop::CreateThreads()  Line 448 + 0x26
bytes C++

right after PreMainMessageLoopRun returns in CreateThreads is the current
gpu code.

SetupSandbox() (in browser_main_loop.cc) happens before CreateThreads in
both cases.



>  Also,
> this moving around looks like will affect the startup time measurement.


how so?


>  With
> all these complications, even it's simply one line of change (maybe more
> if we
> need to move more stuff around), I'd like to do it in a separate CL.  This
> CL
> needs to be merged back to M22, so the simpler, the less behavioral
> change, the
> better.


See my question below at the bottom; I'm curious exactly what's the minimal
fix that you need for M23, and if that can be done separately.

>
>
>
>
>  Why does this test need to run in two different targets? i.e. what does it
>> catch in gpu_tests that it doesn't in browser_tests
>>
>
>  Even if it needs to run on the gpu_tests target, it seems that it can be
>> converted now to run in content_browsertests and be taken out of gpu_tests
>> temporarily.
>>
>
>  Moving the blacklist from chrome to content should entail moving the tests
>> alongside the code. Note this is why we have a separate browser_test
>> target
>> for content, so that all its integration tests run against it, and we
>> don't
>> have to add Content APi calls just for testing.
>>
>
>
> The gpu_feature_browsertests need to run on top of real GPUs / real
> drivers.
> The vertualized GPU/software driver might behave differently, and does not
> cover
> all the features we need to test (for example, multisampling).  So we can
> only
> take them out of gpu_tests when we convert all the gpu_tests to content
> side and
> then switch the GPU bots to run content_browsertests instead of gpu_tests.
> Before that, we can't remove the chrome side tests because we need them to
> run
> on GPU bots.
>

How many  regressions can happen on this code that can be caught on the gpu
bots from now until all the tests are converted?


>
>
http://codereview.chromium.**org/10911315/<http://codereview.chromium.org/109...
>


On Thu, Sep 20, 2012 at 11:08 AM, <zmo@chromium.org> wrote:

> Please have a look at the change I made.  I'd like to land this soon and
> request
> a M23 merge.


Does this whole change need to happen for the histogram fix? i..e can the
histogram be fixed in a separate, small change, and the filter be moved in
separate file?

[Zhenyao Mo, 2012-09-20 19:12:34]

On 2012/09/20 18:35:32, John Abd-El-Malek wrote:
> On Thu, Sep 20, 2012 at 11:08 AM, <mailto:zmo@chromium.org> wrote:
> 
> > On 2012/09/20 01:29:58, John Abd-El-Malek wrote:
> >
> >> to be clear, what I'm saying is that InitializeGpuDataManager should be
> >> removed completely. since content has the data file, it should be the one
> >> responsible for initializing GpuDataManager (i.e. move the code in
> >> chrome_browser_main.cc which calls gpu_util::**InitializeGpuDataManager
> >> to
> >> src/content/browser/browser_**main_loop.cc beside the other GPU related
> >> initializtion)
> >>
> >
> > I moved the version thing into GpuDataManager, so the chrome_gpu_util
> > function
> > is removed.  However, after carefully looking at the code, moving
> > GpuDataManager::Initialize() from chrome side to content side is not
> > trivia.  It
> > is before SetupSandbox now, but the content side GPU code is after it.
> 
> 
> 
> I'm not sure what you mean. Currently GpuDataManager::Initialize is called
> in this callcstack:
>   chrome.dll!ChromeBrowserMainParts::PreMainMessageLoopRunImpl()  Line 1340
> C++
>   chrome.dll!ChromeBrowserMainParts::PreMainMessageLoopRun()  Line 892 +
> 0x8 bytes C++
> > chrome.dll!ChromeBrowserMainPartsWin::PreMainMessageLoopRun()  Line 213
> C++
>   content.dll!content::BrowserMainLoop::CreateThreads()  Line 448 + 0x26
> bytes C++
> 
> right after PreMainMessageLoopRun returns in CreateThreads is the current
> gpu code.
> 
> SetupSandbox() (in browser_main_loop.cc) happens before CreateThreads in
> both cases.
> 

You are right.  I mis-read the code sequence.

> 
> 
> >  Also,
> > this moving around looks like will affect the startup time measurement.
> 
> 
> how so?

There are two places the GpuDataManager::GetInstance() is called in
browser_main_loop.cc.  The first place is earlier than the current Initialize(),
the second is later, and is after browser_process_->notification_ui_manager(),
which according to the comment above, is when measuring started.

So I can put the Initialize in the first place, but it won't stay in one block
with the other GPU related code as you recommended.

> 
> 
> >  With
> > all these complications, even it's simply one line of change (maybe more
> > if we
> > need to move more stuff around), I'd like to do it in a separate CL.  This
> > CL
> > needs to be merged back to M22, so the simpler, the less behavioral
> > change, the
> > better.
> 
> 
> See my question below at the bottom; I'm curious exactly what's the minimal
> fix that you need for M23, and if that can be done separately.

The minimal does not require moving the software_rendering_list.json, but I'd
like to merge back the relocation.  It will make our further updates of
software_rendering_list.json much easier through drover.  Otherwise we have to
do it manually every time.

> 
> >
> >
> >
> >
> >  Why does this test need to run in two different targets? i.e. what does it
> >> catch in gpu_tests that it doesn't in browser_tests
> >>
> >
> >  Even if it needs to run on the gpu_tests target, it seems that it can be
> >> converted now to run in content_browsertests and be taken out of gpu_tests
> >> temporarily.
> >>
> >
> >  Moving the blacklist from chrome to content should entail moving the tests
> >> alongside the code. Note this is why we have a separate browser_test
> >> target
> >> for content, so that all its integration tests run against it, and we
> >> don't
> >> have to add Content APi calls just for testing.
> >>
> >
> >
> > The gpu_feature_browsertests need to run on top of real GPUs / real
> > drivers.
> > The vertualized GPU/software driver might behave differently, and does not
> > cover
> > all the features we need to test (for example, multisampling).  So we can
> > only
> > take them out of gpu_tests when we convert all the gpu_tests to content
> > side and
> > then switch the GPU bots to run content_browsertests instead of gpu_tests.
> > Before that, we can't remove the chrome side tests because we need them to
> > run
> > on GPU bots.
> >
> 
> How many  regressions can happen on this code that can be caught on the gpu
> bots from now until all the tests are converted?
> 

These feature tests are smoke tests to make sure nothing insane happening to our
GPU features.  I'd rather not risk it.  Besides, I don't see why moving this
test has to be in this CL.  The function we expose for testing purpose is still
used by extension tests, so we won't be able to remove it even if we migrate
this feature_browsertests.

> 
> >
> >
>
http://codereview.chromium.**org/10911315/%3Chttp://codereview.chromium.org/1...>
> >
> 
> 
> On Thu, Sep 20, 2012 at 11:08 AM, <mailto:zmo@chromium.org> wrote:
> 
> > Please have a look at the change I made.  I'd like to land this soon and
> > request
> > a M23 merge.
> 
> 
> Does this whole change need to happen for the histogram fix? i..e can the
> histogram be fixed in a separate, small change, and the filter be moved in
> separate file?

[Zhenyao Mo, 2012-09-20 19:17:32]

GpuDataManager initialization moved to the content side.  Please review.

[jam, 2012-09-20 19:31:54]

lgtm with nits

re the test, do you have an estimate of when it'll be moved?

https://codereview.chromium.org/10911315/diff/30004/chrome/browser/chrome_gpu...
File chrome/browser/chrome_gpu_util.h (right):

https://codereview.chromium.org/10911315/diff/30004/chrome/browser/chrome_gpu...
chrome/browser/chrome_gpu_util.h:8: class CommandLine;
nit: get rid of this now

https://codereview.chromium.org/10911315/diff/30004/content/public/browser/gp...
File content/public/browser/gpu_data_manager.h (right):

https://codereview.chromium.org/10911315/diff/30004/content/public/browser/gp...
content/public/browser/gpu_data_manager.h:33: virtual void Initialize() = 0;
this can be taken out of the public API, since it's only called by content now.

https://codereview.chromium.org/10911315/diff/30004/content/public/browser/gp...
content/public/browser/gpu_data_manager.h:36: virtual void Initialize(const
std::string& gpu_blacklist_json,
nit: while you're changing the callsites, if this is only called by tests, it
should InitializeForTesting

[Zhenyao Mo, 2012-09-20 21:24:03]

yongsheng is doing the migration, you can keep an eye on the progress at
http://code.google.com/p/chromium/issues/detail?id=111506

https://codereview.chromium.org/10911315/diff/30004/chrome/browser/chrome_gpu...
File chrome/browser/chrome_gpu_util.h (right):

https://codereview.chromium.org/10911315/diff/30004/chrome/browser/chrome_gpu...
chrome/browser/chrome_gpu_util.h:8: class CommandLine;
On 2012/09/20 19:31:54, John Abd-El-Malek wrote:
> nit: get rid of this now

Done.

https://codereview.chromium.org/10911315/diff/30004/content/public/browser/gp...
File content/public/browser/gpu_data_manager.h (right):

https://codereview.chromium.org/10911315/diff/30004/content/public/browser/gp...
content/public/browser/gpu_data_manager.h:33: virtual void Initialize() = 0;
On 2012/09/20 19:31:54, John Abd-El-Malek wrote:
> this can be taken out of the public API, since it's only called by content
now.

Done.

https://codereview.chromium.org/10911315/diff/30004/content/public/browser/gp...
content/public/browser/gpu_data_manager.h:36: virtual void Initialize(const
std::string& gpu_blacklist_json,
On 2012/09/20 19:31:54, John Abd-El-Malek wrote:
> nit: while you're changing the callsites, if this is only called by tests, it
> should InitializeForTesting

Done.

[jam, 2012-09-20 21:52:18]

On Thu, Sep 20, 2012 at 2:24 PM, <zmo@chromium.org> wrote:

> yongsheng is doing the migration, you can keep an eye on the progress at
> http://code.google.com/p/**chromium/issues/detail?id=**111506
> <http://code.google.com/p/chromium/issues/detail?id=111506>


What is the timeline of that? This needs to be done soon, since the test
doesn't belong in chrome now.

[jam, 2012-11-15 16:43:43]

On 2012/09/20 21:52:18, John Abd-El-Malek wrote:
> On Thu, Sep 20, 2012 at 2:24 PM, <mailto:zmo@chromium.org> wrote:
> 
> > yongsheng is doing the migration, you can keep an eye on the progress at
> > http://code.google.com/p/**chromium/issues/detail?id=**111506
> > <http://code.google.com/p/chromium/issues/detail?id=111506>
> 
> 
> What is the timeline of that? This needs to be done soon, since the test
> doesn't belong in chrome now.

ping: it's been 7 weeks and this migration hasn't finished yet. This needs to be
prioritized.