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());
 
   size_t dictionary_entry_count = 0;
 
   if (top_level) {
     uint32 id;
@@ skipping 173 matching lines @@
         if (!exception_list_value->GetDictionary(i, &exception_value)) {
           LOG(WARNING) << "Malformed exceptions entry " << entry->id();
           return NULL;
         }
         GpuBlacklistEntry* exception = GetGpuBlacklistEntryFromValue(
             exception_value, false);
         if (exception == NULL) {
           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++;
     }
 
     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_field_ = 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_field_(false),
+      contains_exception_with_unknown_field_(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_field_ = 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 60 matching lines @@
 GpuBlacklist::GpuBlacklist(const std::string& browser_version_string)
     : max_entry_id_(0) {
   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,
+    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))
     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, 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;
   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;
 
+  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)
       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)
       break;
     if (browser_version_support == kUnsupported) {
-      entry_count_expectation--;
+      error = false;
       continue;
     }
     DCHECK(browser_version_support == kSupported);
     GpuBlacklistEntry* entry =
         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;
   }
 
   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;
 }
 
 GpuFeatureFlags GpuBlacklist::DetermineGpuFeatureFlags(
     GpuBlacklist::OsType os,
     Version* os_version,
     const GPUInfo& gpu_info) {
   active_entries_.clear();
@@ skipping 201 matching lines @@
             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 61 matching lines @@
     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;
 }