Set offline processing flag for new bookmarks

Sets the NEEDS_OFFLINE_PROCESSING flag for nodes that are created by non-enhanced bookmark aware code. Also fixes a potential issue if a bookmark node was removed while there was a pending operation on it.

BUG=411412

Review URL: https://codereview.chromium.org/626213002

Cr-Commit-Position: refs/heads/master@{#298935}

3 files changed, 134 insertions, 2 deletions

diff --git a/components/enhanced_bookmarks/enhanced_bookmark_model.cc b/components/enhanced_bookmarks/enhanced_bookmark_model.cc
index a3c135b..ba54759 100644
--- a/components/enhanced_bookmarks/enhanced_bookmark_model.cc
+++ b/components/enhanced_bookmarks/enhanced_bookmark_model.cc
@@ -11,6 +11,7 @@
 #include "base/logging.h"
 #include "base/message_loop/message_loop_proxy.h"
 #include "base/rand_util.h"
+#include "base/strings/string_number_conversions.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/browser/bookmark_node.h"
 #include "components/enhanced_bookmarks/enhanced_bookmark_model_observer.h"
@@ -21,6 +22,7 @@
 namespace {
 const char* kBookmarkBarId = "f_bookmarks_bar";
 
+const char* kFlagsKey = "stars.flags";
 const char* kIdKey = "stars.id";
 const char* kImageDataKey = "stars.imageData";
 const char* kNoteKey = "stars.note";
@@ -30,6 +32,11 @@ const char* kVersionKey = "stars.version";
 
 const char* kBookmarkPrefix = "ebc_";
 
+enum Flags {
+  // When set the server will attempt to fill in image and snippet information.
+  NEEDS_OFFLINE_PROCESSING = 0x1,
+};
+
 // Helper method for working with bookmark metainfo.
 std::string DataForMetaInfoField(const BookmarkNode* node,
                                  const std::string& field) {
@@ -283,8 +290,19 @@ void EnhancedBookmarkModel::BookmarkNodeAdded(BookmarkModel* model,
                                               const BookmarkNode* parent,
                                               int index) {
   const BookmarkNode* node = parent->GetChild(index);
-  AddToIdMap(node);
-  ScheduleResetDuplicateRemoteIds();
+  std::string remote_id;
+  if (node->GetMetaInfo(kIdKey, &remote_id)) {
+    AddToIdMap(node);
+    ScheduleResetDuplicateRemoteIds();
+  } else if (node->is_url()) {
+    set_needs_offline_processing_tasks_[node] =
+        make_linked_ptr(new base::CancelableClosure(
+            base::Bind(&EnhancedBookmarkModel::SetNeedsOfflineProcessing,
+                       weak_ptr_factory_.GetWeakPtr(),
+                       base::Unretained(node))));
+    base::MessageLoopProxy::current()->PostTask(
+        FROM_HERE, set_needs_offline_processing_tasks_[node]->callback());
+  }
   FOR_EACH_OBSERVER(
       EnhancedBookmarkModelObserver, observers_, EnhancedBookmarkAdded(node));
 }
@@ -297,6 +315,8 @@ void EnhancedBookmarkModel::BookmarkNodeRemoved(
     const std::set<GURL>& removed_urls) {
   std::string remote_id = GetRemoteId(node);
   id_map_.erase(remote_id);
+  nodes_to_reset_.erase(node);
+  set_needs_offline_processing_tasks_.erase(node);
   FOR_EACH_OBSERVER(
       EnhancedBookmarkModelObserver, observers_, EnhancedBookmarkRemoved(node));
 }
@@ -380,6 +400,19 @@ void EnhancedBookmarkModel::ResetDuplicateRemoteIds() {
   nodes_to_reset_.clear();
 }
 
+void EnhancedBookmarkModel::SetNeedsOfflineProcessing(
+    const BookmarkNode* node) {
+  set_needs_offline_processing_tasks_.erase(node);
+  int flags = 0;
+  std::string flags_str;
+  if (node->GetMetaInfo(kFlagsKey, &flags_str)) {
+    if (!base::StringToInt(flags_str, &flags))
+      flags = 0;
+  }
+  flags |= NEEDS_OFFLINE_PROCESSING;
+  SetMetaInfo(node, kFlagsKey, base::IntToString(flags));
+}
+
 void EnhancedBookmarkModel::SetMetaInfo(const BookmarkNode* node,
                                         const std::string& field,
                                         const std::string& value) {
diff --git a/components/enhanced_bookmarks/enhanced_bookmark_model.h b/components/enhanced_bookmarks/enhanced_bookmark_model.h
index 4fd1ba1..d46e7d9 100644
--- a/components/enhanced_bookmarks/enhanced_bookmark_model.h
+++ b/components/enhanced_bookmarks/enhanced_bookmark_model.h
@@ -8,6 +8,8 @@
 #include <map>
 #include <string>
 
+#include "base/cancelable_callback.h"
+#include "base/memory/linked_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "base/strings/string16.h"
@@ -170,6 +172,9 @@ class EnhancedBookmarkModel : public KeyedService,
   // Clears out any duplicate remote ids detected by AddToIdMap calls.
   void ResetDuplicateRemoteIds();
 
+  // Sets the NEEDS_OFFLINE_PROCESSING flag on the given node.
+  void SetNeedsOfflineProcessing(const BookmarkNode* node);
+
   // Helper method for setting a meta info field on a node. Also updates the
   // version field.
   void SetMetaInfo(const BookmarkNode* node,
@@ -190,6 +195,11 @@ class EnhancedBookmarkModel : public KeyedService,
   IdToNodeMap id_map_;
   NodeToIdMap nodes_to_reset_;
 
+  // Pending SetNeedsOfflineProcessing calls are stored here, as they may need
+  // to be cancelled if the node is removed.
+  std::map<const BookmarkNode*, linked_ptr<base::CancelableClosure>>
+      set_needs_offline_processing_tasks_;
+
   // Caches the remote id of a node before its meta info changes.
   std::string prev_remote_id_;
 
diff --git a/components/enhanced_bookmarks/enhanced_bookmark_model_unittest.cc b/components/enhanced_bookmarks/enhanced_bookmark_model_unittest.cc
index b2c0aae..8db03d0 100644
--- a/components/enhanced_bookmarks/enhanced_bookmark_model_unittest.cc
+++ b/components/enhanced_bookmarks/enhanced_bookmark_model_unittest.cc
@@ -9,6 +9,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/bookmarks/browser/bookmark_model.h"
@@ -649,3 +650,91 @@ TEST_F(EnhancedBookmarkModelTest, ShutDownWhileResetDuplicationScheduled) {
   model_.reset();
   base::RunLoop().RunUntilIdle();
 }
+
+TEST_F(EnhancedBookmarkModelTest, NodeRemovedWhileResetDuplicationScheduled) {
+  const BookmarkNode* node1 = AddBookmark();
+  const BookmarkNode* node2 = AddBookmark();
+  bookmark_model_->SetNodeMetaInfo(node1, "stars.id", "c_1");
+  bookmark_model_->SetNodeMetaInfo(node2, "stars.id", "c_1");
+  bookmark_model_->Remove(node1->parent(), node1->parent()->GetIndexOf(node1));
+  base::RunLoop().RunUntilIdle();
+}
+
+// Verifies that the NEEDS_OFFLINE_PROCESSING flag is set for nodes added
+// with no remote id.
+TEST_F(EnhancedBookmarkModelTest, BookmarkAddedSetsOfflineProcessingFlag) {
+  const BookmarkNode* node =
+      bookmark_model_->AddURL(bookmark_model_->other_node(),
+                              0,
+                              base::ASCIIToUTF16("Some title"),
+                              GURL(BOOKMARK_URL));
+  std::string flags_str;
+  EXPECT_FALSE(node->GetMetaInfo("stars.flags", &flags_str));
+  base::RunLoop().RunUntilIdle();
+  ASSERT_TRUE(node->GetMetaInfo("stars.flags", &flags_str));
+  int flags;
+  ASSERT_TRUE(base::StringToInt(flags_str, &flags));
+  EXPECT_EQ(1, (flags & 1));
+}
+
+// Verifies that the NEEDS_OFFLINE_PROCESSING_FLAG is not set for added folders.
+TEST_F(EnhancedBookmarkModelTest, FolderAddedDoesNotSetOfflineProcessingFlag) {
+  const BookmarkNode* node = AddFolder();
+  base::RunLoop().RunUntilIdle();
+
+  std::string flags_str;
+  if (node->GetMetaInfo("stars.flags", &flags_str)) {
+    int flags;
+    ASSERT_TRUE(base::StringToInt(flags_str, &flags));
+    EXPECT_EQ(0, (flags & 1));
+  }
+}
+
+// Verifies that when a bookmark is added that has a remote id, the status of
+// the NEEDS_OFFLINE_PROCESSING flag doesn't change.
+TEST_F(EnhancedBookmarkModelTest,
+       BookmarkAddedWithIdKeepsOfflineProcessingFlag) {
+  BookmarkNode::MetaInfoMap meta_info;
+  meta_info["stars.id"] = "some_id";
+  meta_info["stars.flags"] = "1";
+
+  const BookmarkNode* node1 =
+      bookmark_model_->AddURLWithCreationTimeAndMetaInfo(
+          bookmark_model_->other_node(),
+          0,
+          base::ASCIIToUTF16("Some title"),
+          GURL(BOOKMARK_URL),
+          base::Time::Now(),
+          &meta_info);
+  base::RunLoop().RunUntilIdle();
+  std::string flags_str;
+  ASSERT_TRUE(node1->GetMetaInfo("stars.flags", &flags_str));
+  int flags;
+  ASSERT_TRUE(base::StringToInt(flags_str, &flags));
+  EXPECT_EQ(1, (flags & 1));
+
+  meta_info["stars.flags"] = "0";
+  const BookmarkNode* node2 =
+      bookmark_model_->AddURLWithCreationTimeAndMetaInfo(
+          bookmark_model_->other_node(),
+          0,
+          base::ASCIIToUTF16("Some title"),
+          GURL(BOOKMARK_URL),
+          base::Time::Now(),
+          &meta_info);
+  base::RunLoop().RunUntilIdle();
+  ASSERT_TRUE(node2->GetMetaInfo("stars.flags", &flags_str));
+  ASSERT_TRUE(base::StringToInt(flags_str, &flags));
+  EXPECT_EQ(0, (flags & 1));
+}
+
+TEST_F(EnhancedBookmarkModelTest,
+       NodeRemovedWhileSetNeedsOfflineProcessingIsScheduled) {
+  const BookmarkNode* node =
+      bookmark_model_->AddURL(bookmark_model_->other_node(),
+                              0,
+                              base::ASCIIToUTF16("Some title"),
+                              GURL(BOOKMARK_URL));
+  bookmark_model_->Remove(node->parent(), node->parent()->GetIndexOf(node));
+  base::RunLoop().RunUntilIdle();
+}