Figure out whether we have enough information to make the final GPU blacklisting decision.

Figure out whether we have enough information to make the final GPU blacklisting decision.

If yes and all GPU features are blacklisted, we don't launch GPU process.

BUG=154130
TEST=content_unittests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=160923

[Zhenyao Mo, 2012-10-09 18:15:04]

Please take a look.

[Zhenyao Mo, 2012-10-09 18:17:41]

avi or joi: content/public owner approval

[Jói, 2012-10-09 18:29:42]

https://codereview.chromium.org/11047011/diff/11001/content/public/common/con...
File content/public/common/content_switches.h (left):

https://codereview.chromium.org/11047011/diff/11001/content/public/common/con...
content/public/common/content_switches.h:186: CONTENT_EXPORT extern const char
kSkipGpuDataLoading[];
I didn't see a change to a file outside of content/ that removes usage of this
flag; was it just unnecessarily a CONTENT_EXPORT?

[Zhenyao Mo, 2012-10-09 18:30:58]

I was used in chrome side before so we need the CONTENT_EXPORT then.  Later that
code moved to content side, but forgot to remove the CONTENT_EXPORT for this
switch.

[Jói, 2012-10-09 18:40:29]

OK, LGTM for content/public.

On Tue, Oct 9, 2012 at 6:30 PM,  <zmo@chromium.org> wrote:
> I was used in chrome side before so we need the CONTENT_EXPORT then.  Later
> that
> code moved to content side, but forgot to remove the CONTENT_EXPORT for this
> switch.
>
> https://codereview.chromium.org/11047011/

[Ken Russell (switch to Gerrit), 2012-10-09 18:52:55]

LGTM overall. Nice tests. Couple of comments.

https://codereview.chromium.org/11047011/diff/11001/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl.cc (right):

https://codereview.chromium.org/11047011/diff/11001/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl.cc:302: if ((gpu_feature_type_ & mask)
!= 0)
At some point we really should rename confusingly named variables like
"gpu_feature_type_". They make it difficult to read through code like this
quickly and figure out what it's doing.

https://codereview.chromium.org/11047011/diff/11001/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/11047011/diff/11001/content/gpu/gpu_main.cc#n...
content/gpu/gpu_main.cc:128: initialized_gl_context = true;
Now CollectGraphicsInfo might have failed above, so is it correct to always set
this to true?

[Zhenyao Mo, 2012-10-09 19:20:47]

Thanks for reviewing this CL.

https://codereview.chromium.org/11047011/diff/11001/content/browser/gpu/gpu_d...
File content/browser/gpu/gpu_data_manager_impl.cc (right):

https://codereview.chromium.org/11047011/diff/11001/content/browser/gpu/gpu_d...
content/browser/gpu/gpu_data_manager_impl.cc:302: if ((gpu_feature_type_ & mask)
!= 0)
On 2012/10/09 18:52:55, kbr wrote:
> At some point we really should rename confusingly named variables like
> "gpu_feature_type_". They make it difficult to read through code like this
> quickly and figure out what it's doing.

Will do in a follow-up CL.

https://codereview.chromium.org/11047011/diff/11001/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/11047011/diff/11001/content/gpu/gpu_main.cc#n...
content/gpu/gpu_main.cc:128: initialized_gl_context = true;
On 2012/10/09 18:52:55, kbr wrote:
> Now CollectGraphicsInfo might have failed above, so is it correct to always
set
> this to true?

For this we need to evaluate the effects of this variable
initialized_gl_context, however, this CL does not change the behavior (before
it's also possible to fail, only it's on Linux only; now it's for all
platforms).  I'll try to clean this up in a separate CL.

[Zhenyao Mo, 2012-10-09 19:36:19]

On 2012/10/09 19:20:47, Zhenyao Mo wrote:
> Thanks for reviewing this CL.
> 
>
https://codereview.chromium.org/11047011/diff/11001/content/browser/gpu/gpu_d...
> File content/browser/gpu/gpu_data_manager_impl.cc (right):
> 
>
https://codereview.chromium.org/11047011/diff/11001/content/browser/gpu/gpu_d...
> content/browser/gpu/gpu_data_manager_impl.cc:302: if ((gpu_feature_type_ &
mask)
> != 0)
> On 2012/10/09 18:52:55, kbr wrote:
> > At some point we really should rename confusingly named variables like
> > "gpu_feature_type_". They make it difficult to read through code like this
> > quickly and figure out what it's doing.
> 
> Will do in a follow-up CL.
> 
> https://codereview.chromium.org/11047011/diff/11001/content/gpu/gpu_main.cc
> File content/gpu/gpu_main.cc (right):
> 
>
https://codereview.chromium.org/11047011/diff/11001/content/gpu/gpu_main.cc#n...
> content/gpu/gpu_main.cc:128: initialized_gl_context = true;
> On 2012/10/09 18:52:55, kbr wrote:
> > Now CollectGraphicsInfo might have failed above, so is it correct to always
> set
> > this to true?
> 
> For this we need to evaluate the effects of this variable
> initialized_gl_context, however, this CL does not change the behavior (before
> it's also possible to fail, only it's on Linux only; now it's for all
> platforms).  I'll try to clean this up in a separate CL.

Actually it's OK.  First, this variable only takes effects on Linux, so this CL
does not change anything.  Second, even if we fail to create a context here, we
will fail later also, so setting this to true under such case does not hurt.