Implement easy GPU feature status summary.

Implement easy GPU feature status summary.

The associated bug shows screenshots of about:gpu with this cl applied.

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

[Avi (use Gerrit), 2011-04-07 00:28:20]

http://codereview.chromium.org/6712048/diff/6001/content/common/gpu_feature_f...
File content/common/gpu_feature_flags.cc (right):

http://codereview.chromium.org/6712048/diff/6001/content/common/gpu_feature_f...
content/common/gpu_feature_flags.cc:68: matches.push_back("Unknown feature");
Gah. No, no, no. All user-visible text must go through internationalization and
strings.

http://codereview.chromium.org/6712048/diff/6001/content/common/gpu_feature_f...
File content/common/gpu_feature_flags.h (right):

http://codereview.chromium.org/6712048/diff/6001/content/common/gpu_feature_f...
content/common/gpu_feature_flags.h:52: // "Acclerated 2D Canvas"
typo: Accelerated

[nduca, 2011-04-07 10:12:22]

Wrt internationalization, we haven't internationalized the about:gpu system...
near as I can tell, the precident here is about:net-internals, which is also not
internationalized.

Thoughts?

[Avi (use Gerrit), 2011-04-07 12:28:48]

Grrr. Unfortunately, you're right; none of the about: pages are localized.
While I think that's a crock and a violation of our obligation to our users,
I can't hold it against you.

content LGTM with the fix of the typo that I noted.

sigh

[zmo, 2011-04-07 20:46:06]

LGTM (with a few nits)

http://codereview.chromium.org/6712048/diff/6001/content/browser/gpu_blacklis...
File content/browser/gpu_blacklist.cc (right):

http://codereview.chromium.org/6712048/diff/6001/content/browser/gpu_blacklis...
content/browser/gpu_blacklist.cc:706: flagged |= true;
should this be flagged = (flagged || true)?  | and || are different.

http://codereview.chromium.org/6712048/diff/6001/content/common/gpu_feature_f...
File content/common/gpu_feature_flags.cc (right):

http://codereview.chromium.org/6712048/diff/6001/content/common/gpu_feature_f...
content/common/gpu_feature_flags.cc:57: matches.push_back("All GPU features");
If you don't care about the difference between "accelerated_2d_canvas" and
"Accelerated 2D Canvas", maybe you can use the defined kGpuFeatureNameXXX here.

http://codereview.chromium.org/6712048/diff/6001/content/common/gpu_feature_f...
content/common/gpu_feature_flags.cc:66: matches.push_back("WebGL + Canvas
Multisampling");
I think only WebGL has Multisampling

http://codereview.chromium.org/6712048/diff/6001/content/common/gpu_feature_f...
content/common/gpu_feature_flags.cc:70: return JoinString(matches, ',');
There is no loop here, so why do you need a vector and JoinString?

http://codereview.chromium.org/6712048/diff/6001/content/common/gpu_feature_f...
File content/common/gpu_feature_flags_unittest.cc (right):

http://codereview.chromium.org/6712048/diff/6001/content/common/gpu_feature_f...
content/common/gpu_feature_flags_unittest.cc:54: // Test GpuFeatureTypeToString
for single-bit enums using the all enum
It would be better if this makes a separate test.

[zmo, 2011-04-11 23:10:25]

LGTM (a few style issues, please fix them before landing)

http://codereview.chromium.org/6712048/diff/9002/content/browser/gpu_blacklis...
File content/browser/gpu_blacklist.cc (right):

http://codereview.chromium.org/6712048/diff/9002/content/browser/gpu_blacklis...
content/browser/gpu_blacklist.cc:700: bool disableAcceleratedCompositing =
disable_accelerated_compositing

same for the other three following this.

http://codereview.chromium.org/6712048/diff/9002/content/browser/gpu_blacklis...
content/browser/gpu_blacklist.cc:733: // status is actually a function of
blacklisting + enable/disable flags + global GPU state
80 per line

http://codereview.chromium.org/6712048/diff/9002/content/browser/gpu_blacklis...
content/browser/gpu_blacklist.cc:739: case
GpuFeatureFlags::kGpuFeatureAccelerated2dCanvas:
switch style is incorrect.  two whitespaces before case.

http://codereview.chromium.org/6712048/diff/9002/content/browser/gpu_blacklis...
content/browser/gpu_blacklist.cc:762: status = "software";
Since we don't have a software renderer for WebGL yet, this might confuse users.

http://codereview.chromium.org/6712048/diff/9002/content/browser/gpu_blacklis...
content/browser/gpu_blacklist.cc:782: status = blacklisted ? "unavailable" :
"enabled";
shouldn't here be UNREACHABLE() or something like that?

http://codereview.chromium.org/6712048/diff/9002/content/browser/gpu_blacklis...
content/browser/gpu_blacklist.cc:798: problem->SetString("description", "GPU
process was unable to boot. Access to GPU disallowed.");
80 per line

http://codereview.chromium.org/6712048/diff/9002/content/browser/gpu_blacklist.h
File content/browser/gpu_blacklist.h (right):

http://codereview.chromium.org/6712048/diff/9002/content/browser/gpu_blacklis...
content/browser/gpu_blacklist.h:82: Value* GetFeatureStatus(bool
gpuAccessAllowed) const;
gpu_access_allowed

[jam, 2011-04-12 17:55:49]

content lgtm

http://codereview.chromium.org/6712048/diff/9002/content/common/gpu_feature_f...
File content/common/gpu_feature_flags.cc (right):

http://codereview.chromium.org/6712048/diff/9002/content/common/gpu_feature_f...
content/common/gpu_feature_flags.cc:9: #include <vector>
nit: this should go on line 7, and have a blank line after it