Refactor chrome.bookmarks API to use JSON schema compiler.

chrome/browser/bookmarks/bookmark_extension_api.cc

Index: chrome/browser/bookmarks/bookmark_extension_api.cc
diff --git a/chrome/browser/bookmarks/bookmark_extension_api.cc b/chrome/browser/bookmarks/bookmark_extension_api.cc
index 96ac5b2ac226055532deeaa6eab3f74a1e75dd09..5a72783c6601fb209057024c1b3f6aec549a1ece 100644
--- a/chrome/browser/bookmarks/bookmark_extension_api.cc
+++ b/chrome/browser/bookmarks/bookmark_extension_api.cc
@@ -34,6 +34,7 @@
 #include "chrome/browser/ui/chrome_select_file_policy.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_paths.h"
+#include "chrome/common/extensions/api/bookmarks.h"
 #include "chrome/common/pref_names.h"
 #include "content/public/browser/notification_service.h"
 #include "grit/generated_resources.h"
@@ -41,9 +42,21 @@
 
 namespace keys = bookmark_extension_api_constants;
 
+namespace Create = extensions::api::bookmarks::Create;
+namespace Get = extensions::api::bookmarks::Get;
+namespace GetChildren = extensions::api::bookmarks::GetChildren;
+namespace GetRecent = extensions::api::bookmarks::GetRecent;
+namespace GetSubTree = extensions::api::bookmarks::GetSubTree;
+namespace GetTree = extensions::api::bookmarks::GetTree;
+namespace Move = extensions::api::bookmarks::Move;
+namespace Remove = extensions::api::bookmarks::Remove;
+namespace Search = extensions::api::bookmarks::Search;
+namespace Update = extensions::api::bookmarks::Update;
+
 using base::TimeDelta;
 using content::BrowserThread;
 using content::WebContents;
+using extensions::api::bookmarks::BookmarkTreeNode;
 
 typedef QuotaLimitHeuristic::Bucket Bucket;
 typedef QuotaLimitHeuristic::Config Config;
@@ -192,9 +205,9 @@ void BookmarkExtensionEventRouter::BookmarkNodeAdded(BookmarkModel* model,
   ListValue args;
   const BookmarkNode* node = parent->GetChild(index);
   args.Append(new StringValue(base::Int64ToString(node->id())));
-  DictionaryValue* obj =
-      bookmark_extension_helpers::GetNodeDictionary(node, false, false);
-  args.Append(obj);
+  BookmarkTreeNode* obj =
+      bookmark_extension_helpers::GetBookmarkTreeNode(node, false, false);
+  args.Append(obj->ToValue().get());
 
   std::string json_args;
   base::JSONWriter::Write(&args, &json_args);
@@ -288,55 +301,53 @@ void BookmarkExtensionEventRouter::ExtensiveBookmarkChangesEnded(
 }
 
 bool GetBookmarksFunction::RunImpl() {
+  scoped_ptr<Get::Params> params(Get::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
+
+  std::vector<linked_ptr<BookmarkTreeNode> > nodes;
   BookmarkModel* model = profile()->GetBookmarkModel();
-  scoped_ptr<ListValue> json(new ListValue());
-  Value* arg0;
-  EXTENSION_FUNCTION_VALIDATE(args_->Get(0, &arg0));
-  if (arg0->IsType(Value::TYPE_LIST)) {
-    const ListValue* ids = static_cast<const ListValue*>(arg0);
-    size_t count = ids->GetSize();
+  if (params->id_or_id_list_type == Get::Params::ID_OR_ID_LIST_ARRAY) {

[Aaron Boodman, 2012/07/23 23:37:57]

I see this pattern a lot. Can we teach the JSON Schema Compiler about it so we
don't have to handle it manually each time? Like:

choice: [
  {type: "string", serialize_to: "list_of_type"}
  {type: "array", items: {type: "string"}}
]

JSON Schema Compiler would treat this as if the type was just array-of-string.

[mitchellwrosen, 2012/07/24 17:13:39]

If I understand correctly: the end result would be an enum (ex. IdOrIdListType)
to differentiate between the choices, and one member variable (ex. IdOrIdList)
rather than one per choice.

[Matt Tytel, 2012/07/24 22:24:28]

Json schema compiler drive-by:
Yea this looks implementable by collapsing choices that have the same serialized
type.
An alternative that would be more concise, easier to implement, but maybe too
confusing is:
{type: "array", serialize_singles: true, items:{type: "string"}}

+    std::vector<std::string>* ids = params->id_or_id_list_array.get();
+    size_t count = ids->size();
     EXTENSION_FUNCTION_VALIDATE(count > 0);
     for (size_t i = 0; i < count; ++i) {
       int64 id;
-      std::string id_string;
-      EXTENSION_FUNCTION_VALIDATE(ids->GetString(i, &id_string));
-      if (!GetBookmarkIdAsInt64(id_string, &id))
+      if (!GetBookmarkIdAsInt64(ids->at(i), &id))
         return false;
       const BookmarkNode* node = model->GetNodeByID(id);
       if (!node) {
         error_ = keys::kNoNodeError;
         return false;
       } else {
-        bookmark_extension_helpers::AddNode(node, json.get(), false);
+        bookmark_extension_helpers::AddNode(node, &nodes, false);
       }
     }
   } else {
     int64 id;
-    std::string id_string;
-    EXTENSION_FUNCTION_VALIDATE(arg0->GetAsString(&id_string));
-    if (!GetBookmarkIdAsInt64(id_string, &id))
+    if (!GetBookmarkIdAsInt64(*params->id_or_id_list_string, &id))
       return false;
     const BookmarkNode* node = model->GetNodeByID(id);
     if (!node) {
       error_ = keys::kNoNodeError;
       return false;
     }
-    bookmark_extension_helpers::AddNode(node, json.get(), false);
+    bookmark_extension_helpers::AddNode(node, &nodes, false);
   }
 
-  result_.reset(json.release());
+  result_.reset(Get::Result::Create(nodes));
   return true;
 }
 
 bool GetBookmarkChildrenFunction::RunImpl() {
-  BookmarkModel* model = profile()->GetBookmarkModel();
+  scoped_ptr<GetChildren::Params> params(GetChildren::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
+
   int64 id;
-  std::string id_string;
-  EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &id_string));
-  if (!GetBookmarkIdAsInt64(id_string, &id))
+  if (!GetBookmarkIdAsInt64(params->id, &id))
     return false;
-  scoped_ptr<ListValue> json(new ListValue());
-  const BookmarkNode* node = model->GetNodeByID(id);
+
+  std::vector<linked_ptr<BookmarkTreeNode> > nodes;
+  const BookmarkNode* node = profile()->GetBookmarkModel()->GetNodeByID(id);
   if (!node) {
     error_ = keys::kNoNodeError;
     return false;
@@ -344,79 +355,81 @@ bool GetBookmarkChildrenFunction::RunImpl() {
   int child_count = node->child_count();
   for (int i = 0; i < child_count; ++i) {
     const BookmarkNode* child = node->GetChild(i);
-    bookmark_extension_helpers::AddNode(child, json.get(), false);
+    bookmark_extension_helpers::AddNode(child, &nodes, false);
   }
 
-  result_.reset(json.release());
+  result_.reset(GetChildren::Result::Create(nodes));
   return true;
 }
 
 bool GetBookmarkRecentFunction::RunImpl() {
-  int number_of_items;
-  EXTENSION_FUNCTION_VALIDATE(args_->GetInteger(0, &number_of_items));
-  if (number_of_items < 1)
+  scoped_ptr<GetRecent::Params> params(GetRecent::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
+  if (params->number_of_items < 1)
     return false;
 
-  BookmarkModel* model = profile()->GetBookmarkModel();
-  ListValue* json = new ListValue();
   std::vector<const BookmarkNode*> nodes;
-  bookmark_utils::GetMostRecentlyAddedEntries(model, number_of_items, &nodes);
+  bookmark_utils::GetMostRecentlyAddedEntries(profile()->GetBookmarkModel(),
+                                              params->number_of_items,
+                                              &nodes);
+
+  std::vector<linked_ptr<BookmarkTreeNode> > tree_nodes;
   std::vector<const BookmarkNode*>::iterator i = nodes.begin();
   for (; i != nodes.end(); ++i) {
     const BookmarkNode* node = *i;
-    bookmark_extension_helpers::AddNode(node, json, false);
+    bookmark_extension_helpers::AddNode(node, &tree_nodes, false);
   }
-  result_.reset(json);
+  result_.reset(GetRecent::Result::Create(tree_nodes));
   return true;
 }
 
 bool GetBookmarkTreeFunction::RunImpl() {
-  BookmarkModel* model = profile()->GetBookmarkModel();
-  scoped_ptr<ListValue> json(new ListValue());
-  const BookmarkNode* node = model->root_node();
-  bookmark_extension_helpers::AddNode(node, json.get(), true);
-  result_.reset(json.release());
+  std::vector<linked_ptr<BookmarkTreeNode> > nodes;
+  const BookmarkNode* node = profile()->GetBookmarkModel()->root_node();
+  bookmark_extension_helpers::AddNode(node, &nodes, true);
+  result_.reset(GetTree::Result::Create(nodes));
   return true;
 }
 
 bool GetBookmarkSubTreeFunction::RunImpl() {
-  BookmarkModel* model = profile()->GetBookmarkModel();
-  scoped_ptr<ListValue> json(new ListValue());
-  Value* arg0;
-  EXTENSION_FUNCTION_VALIDATE(args_->Get(0, &arg0));
+  scoped_ptr<GetSubTree::Params> params(GetSubTree::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
+
   int64 id;
-  std::string id_string;
-  EXTENSION_FUNCTION_VALIDATE(arg0->GetAsString(&id_string));
-  if (!GetBookmarkIdAsInt64(id_string, &id))
+  if (!GetBookmarkIdAsInt64(params->id, &id))
     return false;
-  const BookmarkNode* node = model->GetNodeByID(id);
+
+  const BookmarkNode* node = profile()->GetBookmarkModel()->GetNodeByID(id);
   if (!node) {
     error_ = keys::kNoNodeError;
     return false;
   }
-  bookmark_extension_helpers::AddNode(node, json.get(), true);
-  result_.reset(json.release());
+
+  std::vector<linked_ptr<BookmarkTreeNode> > nodes;
+  bookmark_extension_helpers::AddNode(node, &nodes, true);
+  result_.reset(GetSubTree::Result::Create(nodes));
   return true;
 }
 
 bool SearchBookmarksFunction::RunImpl() {
-  string16 query;
-  EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &query));
+  scoped_ptr<Search::Params> params(Search::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
 
-  BookmarkModel* model = profile()->GetBookmarkModel();
-  ListValue* json = new ListValue();
   std::string lang = profile()->GetPrefs()->GetString(prefs::kAcceptLanguages);
   std::vector<const BookmarkNode*> nodes;
-  bookmark_utils::GetBookmarksContainingText(model, query,
+  bookmark_utils::GetBookmarksContainingText(profile()->GetBookmarkModel(),
+                                             UTF8ToUTF16(params->query),
                                              std::numeric_limits<int>::max(),
-                                             lang, &nodes);
-  std::vector<const BookmarkNode*>::iterator i = nodes.begin();
-  for (; i != nodes.end(); ++i) {
-    const BookmarkNode* node = *i;
-    bookmark_extension_helpers::AddNode(node, json, false);
+                                             lang,
+                                             &nodes);
+
+  std::vector<linked_ptr<BookmarkTreeNode> > tree_nodes;
+  for (std::vector<const BookmarkNode*>::iterator node_iter = nodes.begin();
+       node_iter != nodes.end(); ++node_iter) {
+    bookmark_extension_helpers::AddNode(*node_iter, &tree_nodes, false);
   }
 
-  result_.reset(json);
+  result_.reset(Search::Result::Create(tree_nodes));
   return true;
 }
 
@@ -438,44 +451,42 @@ bool RemoveBookmarkFunction::ExtractIds(const ListValue* args,
 bool RemoveBookmarkFunction::RunImpl() {
   if (!EditBookmarksEnabled())
     return false;
-  std::list<int64> ids;
-  bool invalid_id = false;
-  EXTENSION_FUNCTION_VALIDATE(ExtractIds(args_.get(), &ids, &invalid_id));
-  if (invalid_id) {
+
+  scoped_ptr<Remove::Params> params(Remove::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
+
+  int64 id;
+  if (!base::StringToInt64(params->id, &id)) {

[Aaron Boodman, 2012/07/23 23:37:57]

Could add a feature to JSON Schema Compiler to know about this int64-as-string
convention, and to be able to handle it internally. That would eliminate a lot
of manual code.

Something like:

{type:"string", serialized_type:"int64"}

[mitchellwrosen, 2012/07/24 17:13:39]

Sounds good. Separate CL though right?

     error_ = keys::kInvalidIdError;
     return false;
   }
+
   bool recursive = false;
   if (name() == RemoveTreeBookmarkFunction::function_name())
     recursive = true;
 
   BookmarkModel* model = profile()->GetBookmarkModel();
-  size_t count = ids.size();
-  EXTENSION_FUNCTION_VALIDATE(count > 0);
-  for (std::list<int64>::iterator it = ids.begin(); it != ids.end(); ++it) {
-    if (!bookmark_extension_helpers::RemoveNode(model, *it, recursive, &error_))
-      return false;
-  }
+  if (!bookmark_extension_helpers::RemoveNode(model, id, recursive, &error_))
+    return false;
+
   return true;
 }
 
 bool CreateBookmarkFunction::RunImpl() {
   if (!EditBookmarksEnabled())
     return false;
-  DictionaryValue* json;
-  EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &json));
-  EXTENSION_FUNCTION_VALIDATE(json != NULL);
+
+  scoped_ptr<Create::Params> params(Create::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
 
   BookmarkModel* model = profile()->GetBookmarkModel();
   int64 parentId;
-  if (!json->HasKey(keys::kParentIdKey)) {
+
+  if (!params->bookmark.parent_id.get()) {
     // Optional, default to "other bookmarks".
     parentId = model->other_node()->id();
   } else {
-    std::string parentId_string;
-    EXTENSION_FUNCTION_VALIDATE(json->GetString(keys::kParentIdKey,
-                                                &parentId_string));
-    if (!GetBookmarkIdAsInt64(parentId_string, &parentId))
+    if (!GetBookmarkIdAsInt64(*params->bookmark.parent_id, &parentId))
       return false;
   }
   const BookmarkNode* parent = model->GetNodeByID(parentId);
@@ -489,20 +500,24 @@ bool CreateBookmarkFunction::RunImpl() {
   }
 
   int index;
-  if (!json->HasKey(keys::kIndexKey)) {  // Optional (defaults to end).
+  if (!params->bookmark.index.get()) {  // Optional (defaults to end).
     index = parent->child_count();
   } else {
-    EXTENSION_FUNCTION_VALIDATE(json->GetInteger(keys::kIndexKey, &index));
+    index = *params->bookmark.index;
     if (index > parent->child_count() || index < 0) {
       error_ = keys::kInvalidIndexError;
       return false;
     }
   }
 
-  string16 title;
-  json->GetString(keys::kTitleKey, &title);  // Optional.
-  std::string url_string;
-  json->GetString(keys::kUrlKey, &url_string);  // Optional.
+  string16 title;  // Optional.
+  if (params->bookmark.title.get())
+    title = UTF8ToUTF16(*params->bookmark.title.get());
+
+  std::string url_string;  // Optional.
+  if (params->bookmark.url.get())
+    url_string = *params->bookmark.url.get();
+
   GURL url(url_string);
   if (!url.is_empty() && !url.is_valid()) {
     error_ = keys::kInvalidUrlError;
@@ -520,9 +535,9 @@ bool CreateBookmarkFunction::RunImpl() {
     return false;
   }
 
-  DictionaryValue* ret =
-      bookmark_extension_helpers::GetNodeDictionary(node, false, false);
-  result_.reset(ret);
+  scoped_ptr<BookmarkTreeNode> ret(
+      bookmark_extension_helpers::GetBookmarkTreeNode(node, false, false));
+  result_.reset(Create::Result::Create(*ret));
 
   return true;
 }
@@ -538,20 +553,18 @@ bool MoveBookmarkFunction::ExtractIds(const ListValue* args,
 bool MoveBookmarkFunction::RunImpl() {
   if (!EditBookmarksEnabled())
     return false;
-  std::list<int64> ids;
-  bool invalid_id = false;
-  EXTENSION_FUNCTION_VALIDATE(ExtractIds(args_.get(), &ids, &invalid_id));
-  if (invalid_id) {
+
+  scoped_ptr<Move::Params> params(Move::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
+
+  int64 id;
+  if (!base::StringToInt64(params->id, &id)) {
     error_ = keys::kInvalidIdError;
     return false;
   }
-  EXTENSION_FUNCTION_VALIDATE(ids.size() == 1);
-
-  DictionaryValue* destination;
-  EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(1, &destination));
 
   BookmarkModel* model = profile()->GetBookmarkModel();
-  const BookmarkNode* node = model->GetNodeByID(ids.front());
+  const BookmarkNode* node = model->GetNodeByID(id);
   if (!node) {
     error_ = keys::kNoNodeError;
     return false;
@@ -562,15 +575,12 @@ bool MoveBookmarkFunction::RunImpl() {
   }
 
   const BookmarkNode* parent = NULL;
-  if (!destination->HasKey(keys::kParentIdKey)) {
+  if (!params->destination.parent_id.get()) {
     // Optional, defaults to current parent.
     parent = node->parent();
   } else {
-    std::string parentId_string;
-    EXTENSION_FUNCTION_VALIDATE(destination->GetString(keys::kParentIdKey,
-        &parentId_string));
     int64 parentId;
-    if (!GetBookmarkIdAsInt64(parentId_string, &parentId))
+    if (!GetBookmarkIdAsInt64(*params->destination.parent_id, &parentId))
       return false;
 
     parent = model->GetNodeByID(parentId);
@@ -586,9 +596,8 @@ bool MoveBookmarkFunction::RunImpl() {
   }
 
   int index;
-  if (destination->HasKey(keys::kIndexKey)) {  // Optional (defaults to end).
-    EXTENSION_FUNCTION_VALIDATE(destination->GetInteger(keys::kIndexKey,
-                                                        &index));
+  if (params->destination.index.get()) {  // Optional (defaults to end).
+    index = *params->destination.index;
     if (index > parent->child_count() || index < 0) {
       error_ = keys::kInvalidIndexError;
       return false;
@@ -599,9 +608,9 @@ bool MoveBookmarkFunction::RunImpl() {
 
   model->Move(node, parent, index);
 
-  DictionaryValue* ret =
-      bookmark_extension_helpers::GetNodeDictionary(node, false, false);
-  result_.reset(ret);
+  scoped_ptr<BookmarkTreeNode> tree_node(
+      bookmark_extension_helpers::GetBookmarkTreeNode(node, false, false));
+  result_.reset(Move::Result::Create(*tree_node));
 
   return true;
 }
@@ -617,33 +626,37 @@ bool UpdateBookmarkFunction::ExtractIds(const ListValue* args,
 bool UpdateBookmarkFunction::RunImpl() {
   if (!EditBookmarksEnabled())
     return false;
-  std::list<int64> ids;
-  bool invalid_id = false;
-  EXTENSION_FUNCTION_VALIDATE(ExtractIds(args_.get(), &ids, &invalid_id));
-  if (invalid_id) {
+
+  scoped_ptr<Update::Params> params(Update::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
+
+  int64 id;
+  if (!base::StringToInt64(params->id, &id)) {
     error_ = keys::kInvalidIdError;
     return false;
   }
-  EXTENSION_FUNCTION_VALIDATE(ids.size() == 1);
 
-  DictionaryValue* updates;
-  EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(1, &updates));
+  BookmarkModel* model = profile()->GetBookmarkModel();
 
   // Optional but we need to distinguish non present from an empty title.
   string16 title;
-  const bool has_title = updates->GetString(keys::kTitleKey, &title);
+  bool has_title = false;
+  if (params->changes.title.get()) {
+    title = UTF8ToUTF16(*params->changes.title);
+    has_title = true;
+  }
 
   // Optional.
   std::string url_string;
-  updates->GetString(keys::kUrlKey, &url_string);
+  if (params->changes.url.get())
+    url_string = *params->changes.url;
   GURL url(url_string);
   if (!url_string.empty() && !url.is_valid()) {
     error_ = keys::kInvalidUrlError;
     return false;
   }
 
-  BookmarkModel* model = profile()->GetBookmarkModel();
-  const BookmarkNode* node = model->GetNodeByID(ids.front());
+  const BookmarkNode* node = model->GetNodeByID(id);
   if (!node) {
     error_ = keys::kNoNodeError;
     return false;
@@ -657,9 +670,9 @@ bool UpdateBookmarkFunction::RunImpl() {
   if (!url.is_empty())
     model->SetURL(node, url);
 
-  DictionaryValue* ret =
-      bookmark_extension_helpers::GetNodeDictionary(node, false, false);
-  result_.reset(ret);
+  scoped_ptr<BookmarkTreeNode> tree_node(
+      bookmark_extension_helpers::GetBookmarkTreeNode(node, false, false));
+  result_.reset(Update::Result::Create(*tree_node));
 
   return true;
 }