Prepare declarativeContent API for new script injection feature. Added Javascript types and functio…

chrome/browser/extensions/api/declarative_content/content_action.cc

Index: chrome/browser/extensions/api/declarative_content/content_action.cc
diff --git a/chrome/browser/extensions/api/declarative_content/content_action.cc b/chrome/browser/extensions/api/declarative_content/content_action.cc
index 751f8d53383c946c633b1ef23066a414675e8554..76c874377cbb518372ff4a227295ae67f120c4a6 100644
--- a/chrome/browser/extensions/api/declarative_content/content_action.cc
+++ b/chrome/browser/extensions/api/declarative_content/content_action.cc
@@ -29,6 +29,7 @@ const char kInvalidInstanceTypeError[] =
     "An action has an invalid instanceType: %s";
 const char kNoPageAction[] =
     "Can't use declarativeContent.ShowPageAction without a page action";
+const char kInvalidTypeOfParamter[] = "Attribute '%s' has an invalid type";

[Jeffrey Yasskin, 2014/07/15 17:12:59]

sp: "Paramter"

It's also more user-friendly to say what the type needs to be. This may mean you
need more strings.

[Mark Dittmer, 2014/07/15 18:32:42]

Done.

 
 #define INPUT_FORMAT_VALIDATE(test) do { \
     if (!(test)) { \
@@ -94,6 +95,114 @@ class ShowPageAction : public ContentAction {
   DISALLOW_COPY_AND_ASSIGN(ShowPageAction);
 };
 
+// Action that injects a content script.
+class RequestContentScript : public ContentAction {
+ public:
+  RequestContentScript() {}

[Jeffrey Yasskin, 2014/07/15 17:12:59]

Give this constructor parameters matching the values that need to be stored,
instead of side-loading them after constructing the object.

[Mark Dittmer, 2014/07/15 18:32:43]

Done.

+
+  static scoped_refptr<ContentAction> Create(const Extension* extension,
+                                             const base::DictionaryValue* dict,
+                                             std::string* error,
+                                             bool* bad_message);
+  friend scoped_refptr<ContentAction> RequestContentScript::Create(

[Jeffrey Yasskin, 2014/07/15 17:12:59]

You don't need to friend members of the same class.

[Mark Dittmer, 2014/07/15 18:32:43]

I hit a compiler error without this. I think the friendship is needed because
Create is static.

+      const Extension* extension,
+      const base::DictionaryValue* dict,
+      std::string* error,
+      bool* bad_message);
+
+  // Implementation of ContentAction:
+  virtual Type GetType() const OVERRIDE {
+    return ACTION_REQUEST_CONTENT_SCRIPT;
+  }
+
+  virtual void Apply(const std::string& extension_id,
+                     const base::Time& extension_install_time,
+                     ApplyInfo* apply_info) const OVERRIDE {
+    // TODO(markdittmer): Invoke UserScriptMaster declarative script loader:
+    // load new user script.
+  }
+
+  virtual void Revert(const std::string& extension_id,
+                      const base::Time& extension_install_time,
+                      ApplyInfo* apply_info) const OVERRIDE {
+    // TODO(markdittmer): Invoke UserScriptMaster declarative script loader:
+    // do not load user script in the future.

[Jeffrey Yasskin, 2014/07/15 17:13:00]

s/in the future/if Apply() runs again on the same page/

[Mark Dittmer, 2014/07/15 18:32:43]

Done.

+  }
+
+ private:
+  virtual ~RequestContentScript() {}
+  DISALLOW_COPY_AND_ASSIGN(RequestContentScript);

[Jeffrey Yasskin, 2014/07/15 17:13:00]

This goes last in the class:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

[Mark Dittmer, 2014/07/15 18:32:42]

Done.

+
+  std::vector<std::string> css_file_names_;
+  std::vector<std::string> js_file_names_;
+  bool all_frames_;
+  bool match_about_blank_;
+};
+
+// Helper for getting JS collections into C++.
+bool AppendJSStringsToCPPStrings(const base::ListValue& append_strings,

[Jeffrey Yasskin, 2014/07/15 17:12:59]

This should be static to be sure it doesn't collide with a helper in another .cc
file.

[Mark Dittmer, 2014/07/15 18:32:42]

Done. I'm curious, doesn't living in an anonymous namespace (which it does) also
achieve this?

[Jeffrey Yasskin, 2014/07/15 20:44:59]

Ah, yes it does. I'd lost the anonymous namespace around nearly everything in
the file.

+                                 std::vector<std::string>& append_to) {

[Jeffrey Yasskin, 2014/07/15 17:12:59]

Chrome doesn't use non-const reference arguments:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Refere...

[Mark Dittmer, 2014/07/15 18:32:42]

Done.

+  for (base::ListValue::const_iterator it = append_strings.begin();
+       it != append_strings.end(); ++it) {
+    std::string value;
+    if ((*it)->GetAsString(&value)) {
+      append_to.push_back(value);
+    } else {
+      return false;
+    }
+  }
+
+  return true;
+}
+
+// static
+scoped_refptr<ContentAction> RequestContentScript::Create(
+    const Extension* extension,
+    const base::DictionaryValue* dict,
+    std::string* error,
+    bool* bad_message) {
+  RequestContentScript* request_content_script = new RequestContentScript();
+
+  for (base::DictionaryValue::Iterator iter(*dict);
+       !iter.IsAtEnd(); iter.Advance()) {
+    const std::string& attribute_name = iter.key();
+    const base::Value& attribute_value = iter.value();
+    if (attribute_name == "css" || attribute_name == "js") {

[Jeffrey Yasskin, 2014/07/15 17:12:59]

I would probably look up the attributes I want instead of iterating all
attributes, but this isn't terrible.

[Mark Dittmer, 2014/07/15 18:32:42]

Done. Switched to looking up individual members. This didn't account for
instanceType anyway, and if we add more metadata in the future, we shouldn't
have to update a "skip-property list" in this loop.

+      std::vector<std::string>& string_list = (attribute_name == "css") ?
+          request_content_script->css_file_names_ :
+          request_content_script->js_file_names_;
+      const base::ListValue* list_value = NULL;
+      if (attribute_value.GetAsList(&list_value)) {
+        AppendJSStringsToCPPStrings(*list_value, string_list);
+      } else {
+        *error = base::StringPrintf(kInvalidTypeOfParamter,
+                                    attribute_name.c_str());
+        break;
+      }
+    } else if (attribute_name == "allFrames" ||
+               attribute_name == "matchAboutBlank") {
+      bool& bool_ref = (attribute_name == "allFrames") ?
+          request_content_script->all_frames_ :
+          request_content_script->match_about_blank_;
+      bool bool_read;
+      if (attribute_value.GetAsBoolean(&bool_read)) {
+        bool_ref = bool_read;
+      } else {
+        *error = base::StringPrintf(kInvalidTypeOfParamter,
+                                    attribute_name.c_str());
+        break;
+      }
+    }
+  }
+
+  if (error->empty()) {
+    delete request_content_script;

[Jeffrey Yasskin, 2014/07/15 17:12:59]

Try never to have to write "delete foo" at the end of a function: it makes the
function more fragile. Instead, capture the pointer into a scoped_refptr or
scoped_ptr as soon as you create it.

[Mark Dittmer, 2014/07/15 18:32:42]

Done.

+    return scoped_refptr<ContentAction>();
+  } else {
+    return scoped_refptr<ContentAction>(request_content_script);
+  }
+}
+
 struct ContentActionFactory {
   // Factory methods for ContentAction instances. |extension| is the extension
   // for which the action is being created. |dict| contains the json dictionary
@@ -113,6 +222,8 @@ struct ContentActionFactory {
   ContentActionFactory() {
     factory_methods[keys::kShowPageAction] =
         &ShowPageAction::Create;
+    factory_methods[keys::kRequestContentScript] =
+        &RequestContentScript::Create;
   }
 };