Relax software rendering list parsing.

content/browser/gpu/gpu_blacklist.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/gpu/gpu_blacklist.h"
 
 #include "base/json/json_reader.h"
 #include "base/logging.h"
 #include "base/string_number_conversions.h"
 #include "base/string_piece.h"
@@ skipping 188 matching lines @@
     return kEQ;
   else if (string_op == "contains")
     return kContains;
   else if (string_op == "beginwith")
     return kBeginWith;
   else if (string_op == "endwith")
     return kEndWith;
   return kUnknown;
 }
 
+// static
 GpuBlacklist::GpuBlacklistEntry*
 GpuBlacklist::GpuBlacklistEntry::GetGpuBlacklistEntryFromValue(
     DictionaryValue* value, bool top_level) {
   DCHECK(value);
-  scoped_ptr<GpuBlacklistEntry> entry(new GpuBlacklistEntry());
+  ScopedGpuBlacklistEntry entry(new GpuBlacklistEntry());
 
   size_t dictionary_entry_count = 0;
 
   if (top_level) {
     uint32 id;
     if (!value->GetInteger("id", reinterpret_cast<int*>(&id)) ||
         !entry->SetId(id)) {
       LOG(WARNING) << "Malformed id entry " << entry->id();
       return NULL;
     }
@@ skipping 162 matching lines @@
 
   if (top_level) {
     ListValue* exception_list_value = NULL;
     if (value->GetList("exceptions", &exception_list_value)) {
       for (size_t i = 0; i < exception_list_value->GetSize(); ++i) {
         DictionaryValue* exception_value = NULL;
         if (!exception_list_value->GetDictionary(i, &exception_value)) {
           LOG(WARNING) << "Malformed exceptions entry " << entry->id();
           return NULL;
         }
-        GpuBlacklistEntry* exception = GetGpuBlacklistEntryFromValue(
-            exception_value, false);
+        ScopedGpuBlacklistEntry exception(
+            GetGpuBlacklistEntryFromValue(exception_value, false));
         if (exception == NULL) {
           LOG(WARNING) << "Malformed exceptions entry " << entry->id();
           return NULL;
         }
-        entry->AddException(exception);
+        if (exception->contains_unknown_fields_) {
+          LOG(WARNING) << "Exception with unknown fields " << entry->id();
+          entry->contains_exceptions_with_unknown_fields_ = true;
+        } else {
+          entry->AddException(exception);
+        }
       }
       dictionary_entry_count++;
     }
 
     DictionaryValue* browser_version_value = NULL;
     // browser_version is processed in LoadGpuBlacklist().
     if (value->GetDictionary("browser_version", &browser_version_value))
       dictionary_entry_count++;
   }
 
   if (value->size() != dictionary_entry_count) {
-    LOG(WARNING) << "Malformed entry " << entry->id();
-    return NULL;
+    LOG(WARNING) << "Entry with unknown fields " << entry->id();
+    entry->contains_unknown_fields_ = true;
   }
   return entry.release();
 }
 
-GpuBlacklist::GpuBlacklistEntry::~GpuBlacklistEntry() {
-  for (size_t i = 0; i < exceptions_.size(); ++i)
-    delete exceptions_[i];
-}
-
 GpuBlacklist::GpuBlacklistEntry::GpuBlacklistEntry()
     : id_(0),
-      vendor_id_(0) {
+      vendor_id_(0),
+      contains_unknown_fields_(false),
+      contains_exceptions_with_unknown_fields_(false) {
 }
 
 bool GpuBlacklist::GpuBlacklistEntry::SetId(uint32 id) {
   if (id != 0) {
     id_ = id;
     return true;
   }
   return false;
 }
 
@@ skipping 69 matching lines @@
         GpuFeatureFlags::StringToGpuFeatureType(blacklisted_features[i]);
     switch (type) {
       case GpuFeatureFlags::kGpuFeatureAccelerated2dCanvas:
       case GpuFeatureFlags::kGpuFeatureAcceleratedCompositing:
       case GpuFeatureFlags::kGpuFeatureWebgl:
       case GpuFeatureFlags::kGpuFeatureMultisampling:
       case GpuFeatureFlags::kGpuFeatureAll:
         flags |= type;
         break;
       case GpuFeatureFlags::kGpuFeatureUnknown:
-        return false;
+        contains_unknown_fields_ = true;
+        break;
     }
   }
   feature_flags_.reset(new GpuFeatureFlags());
   feature_flags_->set_flags(flags);
   return true;
 }
 
 void GpuBlacklist::GpuBlacklistEntry::AddException(
     GpuBlacklistEntry* exception) {
   exceptions_.push_back(exception);
@@ skipping 51 matching lines @@
 
 uint32 GpuBlacklist::GpuBlacklistEntry::id() const {
   return id_;
 }
 
 GpuFeatureFlags GpuBlacklist::GpuBlacklistEntry::GetGpuFeatureFlags() const {
   return *feature_flags_;
 }
 
 GpuBlacklist::GpuBlacklist(const std::string& browser_version_string)
-    : max_entry_id_(0) {
+    : max_entry_id_(0),
+      contains_unknown_fields_(false) {
   browser_version_.reset(Version::GetVersionFromString(browser_version_string));
   DCHECK(browser_version_.get() != NULL);
 }
 
 GpuBlacklist::~GpuBlacklist() {
   Clear();
 }
 
-bool GpuBlacklist::LoadGpuBlacklist(const std::string& json_context,
-                                    bool current_os_only) {
+bool GpuBlacklist::LoadGpuBlacklist(
+    const std::string& json_context, GpuBlacklist::OsFilter os_filter) {
   scoped_ptr<Value> root;
   root.reset(base::JSONReader::Read(json_context, false));
   if (root.get() == NULL || !root->IsType(Value::TYPE_DICTIONARY))
     return false;
 
   DictionaryValue* root_dictionary = static_cast<DictionaryValue*>(root.get());
   DCHECK(root_dictionary);
-  return LoadGpuBlacklist(*root_dictionary, current_os_only);
+  return LoadGpuBlacklist(*root_dictionary, os_filter);
 }
 
-bool GpuBlacklist::LoadGpuBlacklist(const DictionaryValue& parsed_json,
-                                    bool current_os_only) {
-  std::vector<GpuBlacklistEntry*> entries;
+bool GpuBlacklist::LoadGpuBlacklist(
+    const DictionaryValue& parsed_json, GpuBlacklist::OsFilter os_filter) {
+  std::vector<ScopedGpuBlacklistEntry> entries;
 
   std::string version_string;
   parsed_json.GetString("version", &version_string);
   version_.reset(Version::GetVersionFromString(version_string));
   if (version_.get() == NULL)
     return false;
 
   ListValue* list = NULL;
   if (!parsed_json.GetList("entries", &list))
     return false;
 
   uint32 max_entry_id = 0;
-  size_t entry_count_expectation = list->GetSize();
+  bool error = false;

[vangelis, 2011/08/24 19:00:17]

Now that the code is so much simpler, you could eliminate the error variable and
simply return instead of doing "error=true;break"

[Zhenyao Mo, 2011/08/24 22:29:57]

Done.

+  bool contains_unknown_fields = false;
   for (size_t i = 0; i < list->GetSize(); ++i) {
     DictionaryValue* list_item = NULL;
     bool valid = list->GetDictionary(i, &list_item);
-    if (!valid)
+    if (!valid || list_item == NULL) {
+      error = true;
       break;
-    if (list_item == NULL)
-      break;
+    }
     // Check browser version compatibility: if the entry is not for the
     // current browser version, don't process it.
     BrowserVersionSupport browser_version_support =
         IsEntrySupportedByCurrentBrowserVersion(list_item);
-    if (browser_version_support == kMalformed)
+    if (browser_version_support == kMalformed) {
+      error = true;
       break;
-    if (browser_version_support == kUnsupported) {
-      entry_count_expectation--;
+    }
+    if (browser_version_support == kUnsupported)
+      continue;
+    DCHECK(browser_version_support == kSupported);
+    ScopedGpuBlacklistEntry entry(
+        GpuBlacklistEntry::GetGpuBlacklistEntryFromValue(list_item, true));
+    if (entry == NULL) {
+      error = true;
+      break;
+    }
+    if (entry->id() > max_entry_id)
+      max_entry_id = entry->id();
+    if (entry->contains_unknown_fields()) {
+      contains_unknown_fields = true;

[vangelis, 2011/08/24 19:00:17]

I forget what we decided but I thought that unknown fields on entries would just
be ignored. Wasn't it only the entries with unknown exceptions that would be
skipped?

For example, the presence of an unknown feature in the feature list should not
cause the whole entry to be skipped.


In any case, a small comment in the code that explains what the behavior is in
the presence of unknown fields would be really helpful.

[Zhenyao Mo, 2011/08/24 22:29:57]

I think we decided the following:
1) ignore unknown features in a feature list
2) ignore unknown field inside an exception (so we could add channels as
exceptions)
3) skip the entry if an unknown field exists outside an exception.  For example,
if we blacklist NVIDIA without an certain extension, and extension is a new
added field, then we don't wanna simply ignore the extension field, because this
will blacklist all NVIDIA.

And yes, the current code's behavior also skip with an unknown field.  So I am
adding a new flag of contains_unknown_features to differentiate from
contains_unknown_fields.
comments added.

       continue;
     }
-    DCHECK(browser_version_support == kSupported);
-    GpuBlacklistEntry* entry =
-        GpuBlacklistEntry::GetGpuBlacklistEntryFromValue(list_item, true);
-    if (entry == NULL)
-      break;
-    if (entry->id() > max_entry_id)
-      max_entry_id = entry->id();
+    if (entry->contains_exceptions_with_unknown_fields())
+      contains_unknown_fields = true;
     entries.push_back(entry);
   }
 
-  if (entries.size() != entry_count_expectation) {
-    for (size_t i = 0; i < entries.size(); ++i)
-      delete entries[i];
+  if (error)
     return false;
-  }
 
   Clear();
-  // Don't apply GPU blacklist for a non-registered OS.
-  OsType os_filter = GetOsType();
-  if (os_filter != kOsUnknown) {
-    for (size_t i = 0; i < entries.size(); ++i) {
-      OsType entry_os = entries[i]->GetOsType();
-      if (!current_os_only ||
-          entry_os == kOsAny || entry_os == os_filter)
-        blacklist_.push_back(entries[i]);
-      else
-        delete entries[i];
-    }
+  OsType my_os = GetOsType();
+  for (size_t i = 0; i < entries.size(); ++i) {
+    OsType entry_os = entries[i]->GetOsType();
+    if (os_filter == GpuBlacklist::kAllOs ||
+        entry_os == kOsAny || entry_os == my_os)
+      blacklist_.push_back(entries[i]);
   }
   max_entry_id_ = max_entry_id;
+  contains_unknown_fields_ = contains_unknown_fields;
   return true;
 }
 
 GpuFeatureFlags GpuBlacklist::DetermineGpuFeatureFlags(
     GpuBlacklist::OsType os,
     Version* os_version,
     const GPUInfo& gpu_info) {
   active_entries_.clear();
   GpuFeatureFlags flags;
 
@@ skipping 175 matching lines @@
     if (disable_multisampling) {
       DictionaryValue* problem = new DictionaryValue();
       problem->SetString("description",
           "Multisampling has been disabled, either via about:flags "
           "or command line");
       problem->Set("crBugs", new ListValue());
       problem->Set("webkitBugs", new ListValue());
       problem_list->Append(problem);
     }
     for (size_t i = 0; i < active_entries_.size(); ++i) {
-      GpuBlacklistEntry* entry = active_entries_[i];
+      ScopedGpuBlacklistEntry entry = active_entries_[i];
       DictionaryValue* problem = new DictionaryValue();
 
       problem->SetString("description", entry->description());
 
       ListValue* cr_bugs = new ListValue();
       for (size_t j = 0; j < entry->cr_bugs().size(); ++j)
         cr_bugs->Append(Value::CreateIntegerValue(
             entry->cr_bugs()[j]));
       problem->Set("crBugs", cr_bugs);
 
       ListValue* webkit_bugs = new ListValue();
       for (size_t j = 0; j < entry->webkit_bugs().size(); ++j)
         webkit_bugs->Append(Value::CreateIntegerValue(
             entry->webkit_bugs()[j]));
       problem->Set("webkitBugs", webkit_bugs);
 
       problem_list->Append(problem);
     }
     status->Set("problems", problem_list);
   }
   return status;
 }
 
+size_t GpuBlacklist::num_entries() const {
+  return blacklist_.size();
+}
+
 uint32 GpuBlacklist::max_entry_id() const {
   return max_entry_id_;
 }
 
 bool GpuBlacklist::GetVersion(uint16* major, uint16* minor) const {
   DCHECK(major && minor);
   *major = 0;
   *minor = 0;
   if (version_.get() == NULL)
     return false;
@@ skipping 32 matching lines @@
 #elif defined(OS_LINUX)
   return kOsLinux;
 #elif defined(OS_MACOSX)
   return kOsMacosx;
 #else
   return kOsUnknown;
 #endif
 }
 
 void GpuBlacklist::Clear() {
-  for (size_t i = 0; i < blacklist_.size(); ++i)
-    delete blacklist_[i];
   blacklist_.clear();
   active_entries_.clear();
+  max_entry_id_ = 0;
+  contains_unknown_fields_ = false;
 }
 
 GpuBlacklist::BrowserVersionSupport
 GpuBlacklist::IsEntrySupportedByCurrentBrowserVersion(
     DictionaryValue* value) {
   DCHECK(value);
   DictionaryValue* browser_version_value = NULL;
   if (value->GetDictionary("browser_version", &browser_version_value)) {
     std::string version_op = "any";
     std::string version_string;
     std::string version_string2;
     browser_version_value->GetString("op", &version_op);
     browser_version_value->GetString("number", &version_string);
     browser_version_value->GetString("number2", &version_string2);
     scoped_ptr<VersionInfo> browser_version_info;
     browser_version_info.reset(
       new VersionInfo(version_op, version_string, version_string2));
     if (!browser_version_info->IsValid())
       return kMalformed;
     if (browser_version_info->Contains(*browser_version_))
       return kSupported;
     return kUnsupported;
   }
   return kSupported;
 }