Enforce the 'requirements' field in manifests.

chrome/browser/extensions/requirements_checker.cc

Index: chrome/browser/extensions/requirements_checker.cc
diff --git a/chrome/browser/extensions/requirements_checker.cc b/chrome/browser/extensions/requirements_checker.cc
new file mode 100644
index 0000000000000000000000000000000000000000..5ea5b5003c6ce9bed346163deae7905176b145ca
--- /dev/null
+++ b/chrome/browser/extensions/requirements_checker.cc
@@ -0,0 +1,133 @@
+// Copyright (c) 2012 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 "chrome/browser/extensions/requirements_checker.h"
+
+#include "base/bind.h"
+#include "base/utf_string_conversions.h"
+#include "chrome/browser/gpu_feature_checker.h"
+#include "chrome/common/extensions/extension_manifest_constants.h"
+#include "chrome/common/extensions/extension.h"
+#include "chrome/common/extensions/manifest.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/common/gpu_feature_type.h"
+
+namespace keys = extension_manifest_keys;
+
+namespace {
+
+const char* kWebGlError = "WebGL is not supported";

[Aaron Boodman, 2012/08/01 03:58:54]

These need to be messages in generated_resources.grd so that they can be
localized.

[Yoyo Zhou, 2012/08/02 22:23:30]

Naming nit: kWebGLError

[eaugusti, 2012/08/03 01:06:26]

Done.

+const char* kCSS3dError = "CSS3d is not supported";
+#if defined(OS_CHROMEOS)
+const char* kPluginsError = "Plugins are not supported";
+#endif
+
+}  // namespace
+
+namespace extensions {
+
+RequirementsChecker::RequirementsChecker()
+    : async_requirement_checks_(0) {

[Aaron Boodman, 2012/08/01 03:58:54]

Nit: Maybe rename this to pending_requirements_checks_?

[eaugusti, 2012/08/03 01:06:26]

Done.

+}
+
+RequirementsChecker::~RequirementsChecker() {
+}
+
+void RequirementsChecker::Check(scoped_refptr<const Extension> extension,
+    base::Callback<void(std::vector<std::string>)> callback,
+    content::BrowserThread::ID callback_thread) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  callback_thread_ = callback_thread;
+  callback_ = callback;
+  DictionaryValue* requirements_value = NULL;
+  extension->manifest()->GetDictionary(keys::kRequirements,
+                                       &requirements_value);
+  if (!requirements_value) {
+    content::BrowserThread::PostTask(callback_thread_, FROM_HERE,
+                                     base::Bind(callback, errors_));
+    return;
+  }
+
+  for (DictionaryValue::key_iterator it = requirements_value->begin_keys();
+       it != requirements_value->end_keys(); ++it) {
+    DictionaryValue* requirement_value;

[Aaron Boodman, 2012/08/01 03:58:54]

= NULL;

[eaugusti, 2012/08/03 01:06:26]

Done.

+    if (!requirements_value->GetDictionaryWithoutPathExpansion(*it,

[Aaron Boodman, 2012/08/01 03:58:54]

first param on each line must align.

[eaugusti, 2012/08/03 01:06:26]

Done.

+        &requirement_value) || !requirement_value) {
+      continue;
+    }
+
+    if (*it == "plugins") {
+#if defined(OS_CHROMEOS)
+      errors_.push_back(kPluginsError);
+#endif
+    } else if (*it == "3D") {
+      ListValue* features;

[Aaron Boodman, 2012/08/01 03:58:54]

= NULL;

[eaugusti, 2012/08/03 01:06:26]

Done.

+      if (!requirement_value->GetListWithoutPathExpansion("features",
+                                                          &features) ||
+          !features) {
+        errors_.push_back("Improperly formatted requirement features for 3D");

[Yoyo Zhou, 2012/08/02 22:23:30]

Define literal strings at the top.

[eaugusti, 2012/08/03 01:06:26]

Done.

+        continue;

[Aaron Boodman, 2012/08/01 03:58:54]

This is a different kind of error (whee). This should be handled in extension.cc
with the other errors there. Perhaps it would make sense to parse requirements
into a struct which is a member of Extension so that they can be more easily
handled here.

[eaugusti, 2012/08/03 01:06:26]

Done, I like that struct.

+      }
+
+      std::string feature;

[Aaron Boodman, 2012/08/01 03:58:54]

define feature within for block

[eaugusti, 2012/08/03 01:06:26]

Done.

+      base::ListValue::iterator it;

[Aaron Boodman, 2012/08/01 03:58:54]

define it within for statement

[eaugusti, 2012/08/03 01:06:26]

Done.

+      for (it = features->begin(); it != features->end(); ++it) {
+        if ((*it)->GetAsString(&feature)) {
+          if (feature == "webgl") {
+              ++async_requirement_checks_;
+              webgl_checker_ = new GPUFeatureChecker(
+                  content::GPU_FEATURE_TYPE_WEBGL,
+                  base::Bind(&RequirementsChecker::IsWebGLAvailable,
+                             AsWeakPtr()));
+          } else if (feature == "css3d") {
+              ++async_requirement_checks_;
+              css3d_checker_ = new GPUFeatureChecker(
+                  content::GPU_FEATURE_TYPE_ACCELERATED_COMPOSITING,
+                  base::Bind(&RequirementsChecker::IsCSS3DAvailable,
+                             AsWeakPtr()));
+          }
+        }
+      }
+    }
+  }
+
+  if (!async_requirement_checks_) {

[Aaron Boodman, 2012/08/01 03:58:54]

Don't use boolean comparisons for integers.

[eaugusti, 2012/08/03 01:06:26]

:(

+    content::BrowserThread::PostTask(callback_thread_, FROM_HERE,
+                                     base::Bind(callback_, errors_));
+    return;
+  }
+  // Running the GPU checkers down here removes any race condition that arises
+  // from the use of async_requirement_checks_.

[Aaron Boodman, 2012/08/01 03:58:54]

Hm, I know that I spec'd RequirementsChecker::Check to maybe be re-entrant, but
now that I see the code, I think maybe that was a bad idea. If you instead make
RequirementsChecker guarantee to always callback asynchronously, I think it will
make it less error-prone and easier to maintain.

[eaugusti, 2012/08/03 01:06:26]

Done.

+  if (webgl_checker_.get())
+    webgl_checker_->CheckGPUFeatureAvailability();
+  if (css3d_checker_.get())
+    css3d_checker_->CheckGPUFeatureAvailability();
+}
+
+base::WeakPtr<RequirementsChecker> RequirementsChecker::AsWeakPtr() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  return base::SupportsWeakPtr<RequirementsChecker>::AsWeakPtr();
+}
+
+void RequirementsChecker::IsWebGLAvailable(bool available) {

[Aaron Boodman, 2012/08/01 03:58:54]

Traditionally a name like Foo::IsMonkey is asking Foo whether it is a monkey (it
would return a boolean). In this case you are essentially telling Foo about
monkeyness. So the method name should be: SetIsWebGLAvailable. Or maybe
SetWebGLAvailability.

[eaugusti, 2012/08/03 01:06:26]

Done.

+  if (!available)
+    errors_.push_back(kWebGlError);
+  MaybeRunCallback();
+}
+
+void RequirementsChecker::IsCSS3DAvailable(bool available) {
+  if (!available)
+    errors_.push_back(kCSS3dError);
+  MaybeRunCallback();
+}
+
+void RequirementsChecker::MaybeRunCallback() {
+  if (!--async_requirement_checks_) {

[Aaron Boodman, 2012/08/01 03:58:54]

Compare to zero instead.

[eaugusti, 2012/08/03 01:06:26]

Done.

+    content::BrowserThread::PostTask(callback_thread_, FROM_HERE,
+                                     base::Bind(callback_, errors_));
+  }
+}
+
+}  // namespace extensions