Improve blacklist logic: use more fields (driver_vendor, gl_renderer, ect) fo...

Improve blacklist logic: use more fields (driver_vendor, gl_renderer, ect) for blacklist logic; add blacklist logic and blacklist entry id; also improves the histogram to record occurance of each entry blocking gpu.

BUG=none
TEST=unittest


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

[Zhenyao Mo, 2011-01-21 02:15:28]

Please review.  Don't worry about the trybots as they haven't picked up my
previous patch on gpu_info_collector yet.  I'll make sure trybots are green.

On 2011/01/21 02:13:42, Zhenyao Mo wrote:

[Ken Russell (switch to Gerrit), 2011-01-21 19:57:51]

http://codereview.chromium.org/6352011/diff/7001/chrome/browser/gpu_blacklist.cc
File chrome/browser/gpu_blacklist.cc (right):

http://codereview.chromium.org/6352011/diff/7001/chrome/browser/gpu_blacklist...
chrome/browser/gpu_blacklist.cc:153: return my_value.find_first_of(value_) == 0;
Probably faster to use std::string::compare(0, value_.length(), value_)

http://codereview.chromium.org/6352011/diff/7001/chrome/browser/gpu_blacklist...
chrome/browser/gpu_blacklist.cc:159: return pos + value_.length() ==
my_value.length();
Faster to use something like std::string::compare(my_value.length() -
value_.length(), value_.length(), value_, 0, value_.length()), with safeguards

http://codereview.chromium.org/6352011/diff/7001/chrome/browser/gpu_blacklist.h
File chrome/browser/gpu_blacklist.h (right):

http://codereview.chromium.org/6352011/diff/7001/chrome/browser/gpu_blacklist...
chrome/browser/gpu_blacklist.h:32: // 6. "blacklist" is a list of gpu feature
strings, valid values include
6 -> 7

http://codereview.chromium.org/6352011/diff/7001/chrome/browser/gpu_blacklist...
chrome/browser/gpu_blacklist.h:85: // DeterminGpuFeatureFlags() call.  This
tells which entries are responsible
Determin -> Determine

http://codereview.chromium.org/6352011/diff/7001/chrome/browser/gpu_blacklist...
chrome/browser/gpu_blacklist.h:87: // The "feature" could be a single feature or
a combination of multiple ones.
Could you improve this comment? It talks about features and flags, but these are
vague concepts. An example would help. Also, documenting what it means to have a
flag set would be helpful. It looks like having a feature flag set means that
it's blacklisted.

http://codereview.chromium.org/6352011/diff/7001/chrome/browser/gpu_blacklist...
chrome/browser/gpu_blacklist.h:228: bool SetGlRendererInfo(const std::string&
renderer_op,
Gl -> GL

[Ken Russell (switch to Gerrit), 2011-01-21 19:58:06]

LGTM otherwise.

[Zhenyao Mo, 2011-01-21 21:51:40]

http://codereview.chromium.org/6352011/diff/7001/chrome/browser/gpu_blacklist.cc
File chrome/browser/gpu_blacklist.cc (right):

http://codereview.chromium.org/6352011/diff/7001/chrome/browser/gpu_blacklist...
chrome/browser/gpu_blacklist.cc:153: return my_value.find_first_of(value_) == 0;
On 2011/01/21 19:57:52, kbr wrote:
> Probably faster to use std::string::compare(0, value_.length(), value_)

Done.

http://codereview.chromium.org/6352011/diff/7001/chrome/browser/gpu_blacklist...
chrome/browser/gpu_blacklist.cc:159: return pos + value_.length() ==
my_value.length();
On 2011/01/21 19:57:52, kbr wrote:
> Faster to use something like std::string::compare(my_value.length() -
> value_.length(), value_.length(), value_, 0, value_.length()), with safeguards

Done.

http://codereview.chromium.org/6352011/diff/7001/chrome/browser/gpu_blacklist.h
File chrome/browser/gpu_blacklist.h (right):

http://codereview.chromium.org/6352011/diff/7001/chrome/browser/gpu_blacklist...
chrome/browser/gpu_blacklist.h:32: // 6. "blacklist" is a list of gpu feature
strings, valid values include
On 2011/01/21 19:57:52, kbr wrote:
> 6 -> 7

Done.

http://codereview.chromium.org/6352011/diff/7001/chrome/browser/gpu_blacklist...
chrome/browser/gpu_blacklist.h:85: // DeterminGpuFeatureFlags() call.  This
tells which entries are responsible
On 2011/01/21 19:57:52, kbr wrote:
> Determin -> Determine

Done.

http://codereview.chromium.org/6352011/diff/7001/chrome/browser/gpu_blacklist...
chrome/browser/gpu_blacklist.h:87: // The "feature" could be a single feature or
a combination of multiple ones.
On 2011/01/21 19:57:52, kbr wrote:
> Could you improve this comment? It talks about features and flags, but these
are
> vague concepts. An example would help. Also, documenting what it means to have
a
> flag set would be helpful. It looks like having a feature flag set means that
> it's blacklisted.

Done.

http://codereview.chromium.org/6352011/diff/7001/chrome/browser/gpu_blacklist...
chrome/browser/gpu_blacklist.h:228: bool SetGlRendererInfo(const std::string&
renderer_op,
On 2011/01/21 19:57:52, kbr wrote:
> Gl -> GL

Done.

[Vangelis Kokkevis, 2011-01-24 20:55:21]

http://codereview.chromium.org/6352011/diff/28001/chrome/browser/gpu_blacklis...
File chrome/browser/gpu_blacklist.cc (right):

http://codereview.chromium.org/6352011/diff/28001/chrome/browser/gpu_blacklis...
chrome/browser/gpu_blacklist.cc:144: GpuBlacklist::StringInfo::~StringInfo() {
No point in defining a destructor if you're not doing anything special in it.

http://codereview.chromium.org/6352011/diff/28001/chrome/browser/gpu_blacklis...
chrome/browser/gpu_blacklist.cc:303: reinterpret_cast<int*>(&id_)) &&
reinterpret_cast 's are kind of scary and potentially unsafe.  Would static_cast
work in this case?  If not, could you split the conversion to an int to a
separate step and then convert to a uint32?

http://codereview.chromium.org/6352011/diff/28001/chrome/browser/gpu_blacklist.h
File chrome/browser/gpu_blacklist.h (right):

http://codereview.chromium.org/6352011/diff/28001/chrome/browser/gpu_blacklis...
chrome/browser/gpu_blacklist.h:10: // A valid gpu_blacklist.json file are in the
format of
As the blacklist is getting more and more full featured, I think this comment
block should move to the gpu_blacklist.json file instead. If you're editing the
blacklist entries, you probably won't know where to look for instructions.

http://codereview.chromium.org/6352011/diff/28001/chrome/browser/gpu_blacklis...
chrome/browser/gpu_blacklist.h:29: // 4. "driver_vendor" is a STRING structure
(defined later).
"defined later": Where?

http://codereview.chromium.org/6352011/diff/28001/chrome/browser/gpu_blacklis...
chrome/browser/gpu_blacklist.h:31: // 6. "gl_renderer" is a STRING structure
(defined later).
"defined later": Where?

http://codereview.chromium.org/6352011/diff/28001/chrome/browser/gpu_blacklis...
chrome/browser/gpu_blacklist.h:88: //   kGpuFeatureAll - all supported features
combined;
I guess in this case kGpuFeatureAll really means "any" of the features, right?

http://codereview.chromium.org/6352011/diff/28001/chrome/browser/gpu_blacklis...
chrome/browser/gpu_blacklist.h:94: // Return the largest entry id.  This is used
for histogram purpose.
nit: "used for histogram purpose" -> "used for histogramming"

http://codereview.chromium.org/6352011/diff/28001/chrome/browser/gpu_blacklis...
chrome/browser/gpu_blacklist.h:97: // Collects the version of the current
blacklist.  Return false and set major
Return -> Returns .  set -> sets

http://codereview.chromium.org/6352011/diff/28001/chrome/browser/gpu_blacklis...
chrome/browser/gpu_blacklist.h:172: kContains,  // contains
nit:  I don't think the inline comments add useful information here.

http://codereview.chromium.org/6352011/diff/28001/chrome/browser/gpu_blacklis...
chrome/browser/gpu_blacklist.h:202: // Returns the entry's unique id.  0 is
preserved.
preserved -> reserved (?)

http://codereview.chromium.org/6352011/diff/28001/chrome/browser/gpu_blacklis...
chrome/browser/gpu_blacklist.h:258: std::vector<uint32> features_;
Does it make sense to turn the feature/id pair into a struct to avoid having two
arrays that need to have matching entries?

[Zhenyao Mo, 2011-01-24 22:38:22]

http://codereview.chromium.org/6352011/diff/28001/chrome/browser/gpu_blacklis...
File chrome/browser/gpu_blacklist.cc (right):

http://codereview.chromium.org/6352011/diff/28001/chrome/browser/gpu_blacklis...
chrome/browser/gpu_blacklist.cc:144: GpuBlacklist::StringInfo::~StringInfo() {
Removed.

On 2011/01/24 20:55:21, Vangelis Kokkevis wrote:
> No point in defining a destructor if you're not doing anything special in it.

http://codereview.chromium.org/6352011/diff/28001/chrome/browser/gpu_blacklis...
chrome/browser/gpu_blacklist.cc:303: reinterpret_cast<int*>(&id_)) &&
On 2011/01/24 20:55:21, Vangelis Kokkevis wrote:
> reinterpret_cast 's are kind of scary and potentially unsafe.  Would
static_cast
> work in this case?  If not, could you split the conversion to an int to a
> separate step and then convert to a uint32?

Done.

http://codereview.chromium.org/6352011/diff/28001/chrome/browser/gpu_blacklist.h
File chrome/browser/gpu_blacklist.h (right):

http://codereview.chromium.org/6352011/diff/28001/chrome/browser/gpu_blacklis...
chrome/browser/gpu_blacklist.h:10: // A valid gpu_blacklist.json file are in the
format of
On 2011/01/24 20:55:21, Vangelis Kokkevis wrote:
> As the blacklist is getting more and more full featured, I think this comment
> block should move to the gpu_blacklist.json file instead. If you're editing
the
> blacklist entries, you probably won't know where to look for instructions.

Done.

http://codereview.chromium.org/6352011/diff/28001/chrome/browser/gpu_blacklis...
chrome/browser/gpu_blacklist.h:29: // 4. "driver_vendor" is a STRING structure
(defined later).
It's at the end of this comments.  I will change it to be "defined below" and
add an empty before the definition of STRING/VERSION so it's more apparent.

On 2011/01/24 20:55:21, Vangelis Kokkevis wrote:
> "defined later": Where?

http://codereview.chromium.org/6352011/diff/28001/chrome/browser/gpu_blacklis...
chrome/browser/gpu_blacklist.h:31: // 6. "gl_renderer" is a STRING structure
(defined later).
See comment above.

On 2011/01/24 20:55:21, Vangelis Kokkevis wrote:
> "defined later": Where?

http://codereview.chromium.org/6352011/diff/28001/chrome/browser/gpu_blacklis...
chrome/browser/gpu_blacklist.h:88: //   kGpuFeatureAll - all supported features
combined;
On 2011/01/24 20:55:21, Vangelis Kokkevis wrote:
> I guess in this case kGpuFeatureAll really means "any" of the features, right?
 

Done.

http://codereview.chromium.org/6352011/diff/28001/chrome/browser/gpu_blacklis...
chrome/browser/gpu_blacklist.h:94: // Return the largest entry id.  This is used
for histogram purpose.
On 2011/01/24 20:55:21, Vangelis Kokkevis wrote:
> nit: "used for histogram purpose" -> "used for histogramming"

Done.

http://codereview.chromium.org/6352011/diff/28001/chrome/browser/gpu_blacklis...
chrome/browser/gpu_blacklist.h:97: // Collects the version of the current
blacklist.  Return false and set major
On 2011/01/24 20:55:21, Vangelis Kokkevis wrote:
> Return -> Returns .  set -> sets

Done.

http://codereview.chromium.org/6352011/diff/28001/chrome/browser/gpu_blacklis...
chrome/browser/gpu_blacklist.h:172: kContains,  // contains
Removed.
On 2011/01/24 20:55:21, Vangelis Kokkevis wrote:
> nit:  I don't think the inline comments add useful information here.

http://codereview.chromium.org/6352011/diff/28001/chrome/browser/gpu_blacklis...
chrome/browser/gpu_blacklist.h:202: // Returns the entry's unique id.  0 is
preserved.
On 2011/01/24 20:55:21, Vangelis Kokkevis wrote:
> preserved -> reserved (?)

Done.

http://codereview.chromium.org/6352011/diff/28001/chrome/browser/gpu_blacklis...
chrome/browser/gpu_blacklist.h:258: std::vector<uint32> features_;
I switch to record GpuBlacklistEntry instead and later it's trivia to get
FeatureFlags and id from the entry.

On 2011/01/24 20:55:21, Vangelis Kokkevis wrote:
> Does it make sense to turn the feature/id pair into a struct to avoid having
two
> arrays that need to have matching entries?

[Vangelis Kokkevis, 2011-01-25 06:33:59]

Looks good!  Thanks!