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

chrome/browser/gpu_blacklist.h

Index: chrome/browser/gpu_blacklist.h
===================================================================
--- chrome/browser/gpu_blacklist.h	(revision 72173)
+++ chrome/browser/gpu_blacklist.h	(working copy)
@@ -9,6 +9,7 @@
 // Determines whether certain gpu-related features are blacklisted or not.
 // A valid gpu_blacklist.json file are in the format of

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

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.

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

Done.

 // {
+//   "version": "x.y",
 //   "entries": [
 //     { // entry 1
 //     },
@@ -25,16 +26,20 @@
 //    "version" is a VERSION structure (defined later).
 // 2. "vendor_id" has the value of a string.
 // 3. "device_id" has the value of a string.
-// 4. "driver_version" is a VERSION structure (defined later).
-// 5. "blacklist" is a list of gpu feature strings, valid values include
+// 4. "driver_vendor" is a STRING structure (defined later).

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

"defined later": Where? 

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

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:

+// 5. "driver_version" is a VERSION structure (defined later).
+// 6. "gl_renderer" is a STRING structure (defined later).

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

"defined later": Where?

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

See comment above.

On 2011/01/24 20:55:21, Vangelis Kokkevis wrote:

+// 7. "blacklist" is a list of gpu feature strings, valid values include
 //    "accelerated_2d_canvas", "accelerated_compositing", "webgl", and "all".
 //    Currently whatever feature is selected, the effect is the same as "all",
 //    i.e., it's not supported to turn off one GPU feature and not the others.
 // VERSION includes "op" "number", and "number2".  "op" can be any of the
-// following value: "=", "<", "<=", ">", ">=", "any", "between".  "number2" is
+// following values: "=", "<", "<=", ">", ">=", "any", "between".  "number2" is
 // only used if "op" is "between".  "number" is used for all "op" values except
 // "any". "number" and "number2" are in the format of x, x.x, x.x.x, ect.
 // Check out "gpu_blacklist_unittest.cc" for examples.
+// STRING includes "op" and "value".  "op" can be any of the following values:
+// "contains", "beginwith", "endwith", "=".  "value" is a string.
 
 #include <string>
 #include <vector>
@@ -74,8 +79,25 @@
   // current OS version.
   GpuFeatureFlags DetermineGpuFeatureFlags(OsType os,
                                            Version* os_version,
-                                           const GPUInfo& gpu_info) const;
+                                           const GPUInfo& gpu_info);
 
+  // Collects the entries that set the "feature" flag from the last
+  // DetermineGpuFeatureFlags() call.  This tells which entries are responsible
+  // for raising a certain flag, i.e, for blacklisting a certain feature.
+  // Examples of "feature":
+  //   kGpuFeatureAll - all supported features combined;

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

I guess in this case kGpuFeatureAll really means "any" of the features, right?  

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

Done.

+  //   kGpuFeatureWebgl - a single feature;
+  //   kGpuFeatureWebgl | kGpuFeatureAcceleratedCompositing - two features.
+  void GetGpuFeatureFlagEntries(GpuFeatureFlags::GpuFeatureType feature,
+                                std::vector<uint32>& entry_ids) const;
+
+  // Return the largest entry id.  This is used for histogram purpose.

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

nit: "used for histogram purpose" -> "used for histogramming"

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

Done.

+  uint32 max_entry_id() const;
+
+  // Collects the version of the current blacklist.  Return false and set major

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

Return -> Returns .  set -> sets

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

Done.

+  // and minor to 0 on failure.
+  bool GetVersion(uint16* major, uint16* monir) const;
+
  private:
   class VersionInfo {
    public:
@@ -134,6 +156,33 @@
     scoped_ptr<VersionInfo> version_info_;
   };
 
+  class StringInfo {
+   public:
+    StringInfo(const std::string& string_op, const std::string& string_value);
+    ~StringInfo();
+
+    // Determines if a given string is included in the StringInfo.
+    bool Contains(const std::string& value) const;
+
+    // Determines if the StringInfo contains valid information.
+    bool IsValid() const;
+
+   private:
+    enum Op {
+      kContains,  // contains

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

nit:  I don't think the inline comments add useful information here.

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

Removed.
On 2011/01/24 20:55:21, Vangelis Kokkevis wrote:

+      kBeginWith,  // beginwith
+      kEndWith,  // endwith
+      kEQ,  // =
+      kUnknown  // Indicates StringInfo data is invalid.
+    };
+
+    // Maps string to Op; returns kUnknown if it's not a valid Op.
+    static Op StringToOp(const std::string& string_op);
+
+    Op op_;
+    std::string value_;
+  };
+
   class GpuBlacklistEntry {
    public:
     // Constructs GpuBlacklistEntry from DictionaryValue loaded from json.
@@ -143,11 +192,16 @@
     // Determines if a given os/gc/driver is included in the Entry set.
     bool Contains(OsType os_type, const Version& os_version,
                   uint32 vendor_id, uint32 device_id,
-                  const Version& driver_version) const;
+                  const std::string& driver_vendor,
+                  const Version& driver_version,
+                  const std::string& gl_renderer) const;
 
     // Returns the OsType.
     OsType GetOsType() const;
 
+    // Returns the entry's unique id.  0 is preserved.

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

preserved -> reserved (?)

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

Done.

+    uint32 id() const;
+
     // Returns the GpuFeatureFlags.
     GpuFeatureFlags GetGpuFeatureFlags() const;
 
@@ -156,6 +210,8 @@
    private:
     GpuBlacklistEntry();
 
+    bool SetId(const std::string& id_string);
+
     bool SetOsInfo(const std::string& os,
                    const std::string& version_op,
                    const std::string& version_string,
@@ -165,17 +221,26 @@
 
     bool SetDeviceId(const std::string& device_id_string);
 
+    bool SetDriverVendorInfo(const std::string& vendor_op,
+                             const std::string& vendor_value);
+
     bool SetDriverVersionInfo(const std::string& version_op,
                               const std::string& version_string,
                               const std::string& version_string2);
 
+    bool SetGLRendererInfo(const std::string& renderer_op,
+                           const std::string& renderer_value);
+
     bool SetBlacklistedFeatures(
         const std::vector<std::string>& blacklisted_features);
 
+    uint32 id_;
     scoped_ptr<OsInfo> os_info_;
     uint32 vendor_id_;
     uint32 device_id_;
+    scoped_ptr<StringInfo> driver_vendor_info_;
     scoped_ptr<VersionInfo> driver_version_info_;
+    scoped_ptr<StringInfo> gl_renderer_info_;
     scoped_ptr<GpuFeatureFlags> feature_flags_;
   };
 
@@ -184,8 +249,17 @@
 
   void Clear();
 
+  scoped_ptr<Version> version_;
   std::vector<GpuBlacklistEntry*> blacklist_;
 
+  // These two vectors are updated everytime DetermineGpuFeatureFlags() is
+  // called.  They are used by GetGpuFeatureFlagEntries().
+  // Two vectors should always have the same number of elements.
+  std::vector<uint32> features_;

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

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]

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:

+  std::vector<uint32> entry_ids_;
+
+  uint32 max_entry_id_;
+
   DISALLOW_COPY_AND_ASSIGN(GpuBlacklist);
 };