Relax software rendering list parsing.

content/browser/gpu/gpu_blacklist.cc

Index: content/browser/gpu/gpu_blacklist.cc
===================================================================
--- content/browser/gpu/gpu_blacklist.cc	(revision 97470)
+++ content/browser/gpu/gpu_blacklist.cc	(working copy)
@@ -206,6 +206,7 @@
   return kUnknown;
 }
 
+// static
 GpuBlacklist::GpuBlacklistEntry*
 GpuBlacklist::GpuBlacklistEntry::GetGpuBlacklistEntryFromValue(
     DictionaryValue* value, bool top_level) {
@@ -399,7 +400,13 @@
           LOG(WARNING) << "Malformed exceptions entry " << entry->id();
           return NULL;
         }
-        entry->AddException(exception);
+        if (exception->contains_unknown_field_) {
+          LOG(WARNING) << "Exception with unknown fields " << entry->id();
+          delete exception;
+          entry->contains_exception_with_unknown_field_ = true;
+        } else {
+          entry->AddException(exception);
+        }
       }
       dictionary_entry_count++;
     }
@@ -411,8 +418,8 @@
   }
 
   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_field_ = true;
   }
   return entry.release();
 }
@@ -424,7 +431,9 @@
 
 GpuBlacklist::GpuBlacklistEntry::GpuBlacklistEntry()
     : id_(0),
-      vendor_id_(0) {
+      vendor_id_(0),
+      contains_unknown_field_(false),
+      contains_exception_with_unknown_field_(false) {
 }
 
 bool GpuBlacklist::GpuBlacklistEntry::SetId(uint32 id) {
@@ -514,7 +523,8 @@
         flags |= type;
         break;
       case GpuFeatureFlags::kGpuFeatureUnknown:
-        return false;
+        contains_unknown_field_ = true;
+        break;
     }
   }
   feature_flags_.reset(new GpuFeatureFlags());
@@ -595,8 +605,10 @@
   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,
+    GpuBlacklist::UnknownFieldOption unknown_field_option) {
   scoped_ptr<Value> root;
   root.reset(base::JSONReader::Read(json_context, false));
   if (root.get() == NULL || !root->IsType(Value::TYPE_DICTIONARY))
@@ -604,11 +616,13 @@
 
   DictionaryValue* root_dictionary = static_cast<DictionaryValue*>(root.get());
   DCHECK(root_dictionary);
-  return LoadGpuBlacklist(*root_dictionary, current_os_only);
+  return LoadGpuBlacklist(*root_dictionary, os_filter, unknown_field_option);
 }
 
-bool GpuBlacklist::LoadGpuBlacklist(const DictionaryValue& parsed_json,
-                                    bool current_os_only) {
+bool GpuBlacklist::LoadGpuBlacklist(
+    const DictionaryValue& parsed_json,
+    GpuBlacklist::OsFilter os_filter,
+    GpuBlacklist::UnknownFieldOption unknown_field_option) {
   std::vector<GpuBlacklistEntry*> entries;
 
   std::string version_string;
@@ -621,9 +635,10 @@
   if (!parsed_json.GetList("entries", &list))
     return false;
 
+  bool error = false;
   uint32 max_entry_id = 0;
-  size_t entry_count_expectation = list->GetSize();
   for (size_t i = 0; i < list->GetSize(); ++i) {
+    error = true;

[vangelis, 2011/08/19 22:50:07]

I find the use of the error variable is a bit confusing and fragile.  What if
you just set error = true only in situations where there is an error and you
break out of the loop?  That way you don't have to set its value twice in every
iteration.

[Zhenyao Mo, 2011/08/23 22:16:44]

Done.

     DictionaryValue* list_item = NULL;
     bool valid = list->GetDictionary(i, &list_item);
     if (!valid)
@@ -637,7 +652,7 @@
     if (browser_version_support == kMalformed)
       break;
     if (browser_version_support == kUnsupported) {
-      entry_count_expectation--;
+      error = false;
       continue;
     }
     DCHECK(browser_version_support == kSupported);
@@ -645,12 +660,29 @@
         GpuBlacklistEntry::GetGpuBlacklistEntryFromValue(list_item, true);
     if (entry == NULL)
       break;
+    if (entry->contains_unknown_field()) {
+      delete entry;
+      if (unknown_field_option == kFailOnUnknownField)

[vangelis, 2011/08/19 22:50:07]

I think kFailOnUnknownField adds a lot of unnecessary complication to the code. 
Is it useful to ever fail parsing the blacklist because we encountered an
unknown field or an unknown exception?  I think that if we ever encounter an
unknown field we should always discard the entry.  If we encounter an unknown
exception, ignore the exception and pretend it's not there. 

[Zhenyao Mo, 2011/08/23 22:16:44]

Done.

+        break;
+      error = false;
+      continue;
+    } else if (entry->contains_exception_with_unknown_field()) {

[vangelis, 2011/08/19 22:50:07]

Don't need an else if here.  Previous if clause either breaks out of the loop of
skips to the top again.

[Zhenyao Mo, 2011/08/23 22:16:44]

Done.

+      if (unknown_field_option == kFailOnUnknownField) {
+        delete entry;

[vangelis, 2011/08/19 22:50:07]

Don't need to delete the entry here.  It will be taken care of outside the loop
as error == true

[Zhenyao Mo, 2011/08/23 22:16:44]

Done.

+        break;
+      } else if (unknown_field_option == kIgnoreEntryWithUnknownField) {
+        delete entry;
+        error = false;
+        continue;
+      }
+    }
     if (entry->id() > max_entry_id)
       max_entry_id = entry->id();
     entries.push_back(entry);
+    error = false;
   }
 
-  if (entries.size() != entry_count_expectation) {
+  if (error) {
     for (size_t i = 0; i < entries.size(); ++i)
       delete entries[i];

[vangelis, 2011/08/19 22:50:07]

The management of entries is getting kind of hairy and error-prone.  Can't
entries be ref-counted so that you don't have to explicitly delete them? 
Entries that are actually used will be kept alive from a reference in blacklist_
and all other will automatically be deleted when the entries vector falls out of
scope.

[Zhenyao Mo, 2011/08/23 22:16:44]

Done.

     return false;
@@ -658,16 +690,19 @@
 
   Clear();
   // Don't apply GPU blacklist for a non-registered OS.
-  OsType os_filter = GetOsType();
-  if (os_filter != kOsUnknown) {
+  OsType my_os = GetOsType();
+  if (my_os != 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)
+      if (os_filter == GpuBlacklist::kAllOs ||
+          entry_os == kOsAny || entry_os == my_os)
         blacklist_.push_back(entries[i]);
       else
         delete entries[i];
     }
+  } else {
+    for (size_t i = 0; i < entries.size(); ++i)

[vangelis, 2011/08/19 22:50:07]

You could avoid adding another deletion loop here by getting the os version at
the very top of the function and bailing out early if it is kOsUnknown.

[Zhenyao Mo, 2011/08/23 22:16:44]

Turns out we don't need to deal with kOsUnknown at all because kCurrentOsOnly
already handles the filtering.

Deleted the loop here.

+      delete entries[i];
   }
   max_entry_id_ = max_entry_id;
   return true;
@@ -889,6 +924,10 @@
   return status;
 }
 
+size_t GpuBlacklist::num_entries() const {
+  return blacklist_.size();
+}
+
 uint32 GpuBlacklist::max_entry_id() const {
   return max_entry_id_;
 }