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 3dc1312a728ad3ea24333bb0cb1badbf897e4ff3..38a2b5705a2f9eb35c03c08c42b75ed8aaa75dde 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;

[brettw, 2012/07/26 18:03:15]

These namespaces seem really weird to me. It's confusing later on when you
write, for example, Get::Params. I would never expect "Get" to be a namespace.
Do you have any ideas for making this more clear?

[mitchellwrosen, 2012/07/26 18:05:14]

It's the only way of using the JSON schema compiler I've ever seen. I agree that
it looks weird.

[brettw, 2012/07/27 20:05:10]

It wouldn't look so weird if the users of this code wrote bookmarks::Create and
bookmarks::Get since then it falls under my expectations of what namespaces look
like.

So what do you think about doing "namespace bookmarks =
extensions::api::bookmarks" and then using that below?

[mitchellwrosen, 2012/07/27 22:53:21]

Done.

+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) {
+    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);
   }
 
-  SetResult(json.release());
+  results_ = Get::Results::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,82 @@ 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);
   }
 
-  SetResult(json.release());
+  results_ = GetChildren::Results::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);
   }
-  SetResult(json);
+
+  results_ = GetRecent::Results::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);
-  SetResult(json.release());
+  std::vector<linked_ptr<BookmarkTreeNode> > nodes;
+  const BookmarkNode* node = profile()->GetBookmarkModel()->root_node();
+  bookmark_extension_helpers::AddNode(node, &nodes, true);
+  results_ = GetTree::Results::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);
-  SetResult(json.release());
+
+  std::vector<linked_ptr<BookmarkTreeNode> > nodes;
+  bookmark_extension_helpers::AddNode(node, &nodes, true);
+  results_ = GetSubTree::Results::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);
   }
 
-  SetResult(json);
+  results_ = Search::Results::Create(tree_nodes);
   return true;
 }
 
@@ -438,44 +452,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)) {
     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 +501,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 +536,9 @@ bool CreateBookmarkFunction::RunImpl() {
     return false;
   }
 
-  DictionaryValue* ret =
-      bookmark_extension_helpers::GetNodeDictionary(node, false, false);
-  SetResult(ret);
+  scoped_ptr<BookmarkTreeNode> ret(
+      bookmark_extension_helpers::GetBookmarkTreeNode(node, false, false));
+  results_ = Create::Results::Create(*ret);
 
   return true;
 }
@@ -538,20 +554,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 +576,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 +597,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 +609,9 @@ bool MoveBookmarkFunction::RunImpl() {
 
   model->Move(node, parent, index);
 
-  DictionaryValue* ret =
-      bookmark_extension_helpers::GetNodeDictionary(node, false, false);
-  SetResult(ret);
+  scoped_ptr<BookmarkTreeNode> tree_node(
+      bookmark_extension_helpers::GetBookmarkTreeNode(node, false, false));
+  results_ = Move::Results::Create(*tree_node);
 
   return true;
 }
@@ -617,33 +627,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,10 +671,9 @@ bool UpdateBookmarkFunction::RunImpl() {
   if (!url.is_empty())
     model->SetURL(node, url);
 
-  DictionaryValue* ret =
-      bookmark_extension_helpers::GetNodeDictionary(node, false, false);
-  SetResult(ret);
-
+  scoped_ptr<BookmarkTreeNode> tree_node(
+      bookmark_extension_helpers::GetBookmarkTreeNode(node, false, false));
+  results_ = Update::Results::Create(*tree_node);
   return true;
 }