[bookmarks] Adopt base::GUID for BookmarkModel::AddURL()

Pass in a base::GUID in lieu of a std::string GUID.

Bug: 1026195
Change-Id: Ie76c20a61bc8b42aeadbf41721b7d2666bef0f27
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2528632
Commit-Queue: Daniel Hosseinian <dhoss@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#828350}

chrome/browser/sync/test/integration/bookmarks_helper.cc

@@ -545,8 +545,7 @@ const BookmarkNode* AddURL(int profile,
     FindNodeInVerifier(model, parent, &v_parent);
     const BookmarkNode* v_node = GetVerifierBookmarkModel()->AddURL(
         v_parent, index, base::UTF8ToUTF16(title), url,
-        /*meta_info=*/nullptr, result->date_added(),
-        result->guid().AsLowercaseString());
+        /*meta_info=*/nullptr, result->date_added(), result->guid());
     if (!v_node) {
       LOG(ERROR) << "Could not add bookmark " << title << " to the verifier";
       return nullptr;

components/bookmarks/browser/bookmark_model.cc

@@ -607,7 +607,7 @@ const BookmarkNode* BookmarkModel::AddURL(
     const GURL& url,
     const BookmarkNode::MetaInfoMap* meta_info,
     base::Optional<base::Time> creation_time,
-    base::Optional<std::string> guid) {
+    base::Optional<base::GUID> guid) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(loaded_);
   DCHECK(url.is_valid());
@@ -615,10 +615,7 @@ const BookmarkNode* BookmarkModel::AddURL(
   DCHECK(parent->is_folder());
   DCHECK(!is_root_node(parent));
   DCHECK(IsValidIndex(parent, index, true));
-
-  base::GUID parsed_guid =
-      guid ? base::GUID::ParseLowercase(*guid) : base::GUID::GenerateRandomV4();
-  DCHECK(parsed_guid.is_valid());
+  DCHECK(!guid || guid->is_valid());
 
   if (!creation_time)
     creation_time = Time::Now();
@@ -627,8 +624,9 @@ const BookmarkNode* BookmarkModel::AddURL(
   if (*creation_time > parent->date_folder_modified())
     SetDateFolderModified(parent, *creation_time);
 
-  auto new_node =
-      std::make_unique<BookmarkNode>(generate_next_node_id(), parsed_guid, url);
+  auto new_node = std::make_unique<BookmarkNode>(
+      generate_next_node_id(), guid ? *guid : base::GUID::GenerateRandomV4(),
+      url);
   new_node->SetTitle(title);
   new_node->set_date_added(*creation_time);
   if (meta_info)

components/bookmarks/browser/bookmark_model.h

@@ -215,8 +215,8 @@ class BookmarkModel : public BookmarkUndoProvider,
       base::Optional<base::GUID> guid = base::nullopt);
 
   // Adds a url at the specified position with the given |creation_time|,
-  // |meta_info| and |guid|. If a GUID is provided, it must be a valid version 4
-  // GUID, otherwise a new one is generated to replace it.
+  // |meta_info| and |guid|. If no GUID is provided (i.e. nullopt), then a
+  // random one will be generated. If a GUID is provided, it must be valid.
   const BookmarkNode* AddURL(
       const BookmarkNode* parent,
       size_t index,
@@ -224,7 +224,7 @@ class BookmarkModel : public BookmarkUndoProvider,
       const GURL& url,
       const BookmarkNode::MetaInfoMap* meta_info = nullptr,
       base::Optional<base::Time> creation_time = base::nullopt,
-      base::Optional<std::string> guid = base::nullopt);
+      base::Optional<base::GUID> guid = base::nullopt);
 
   // Sorts the children of |parent|, notifying observers by way of the
   // BookmarkNodeChildrenReordered method.

components/bookmarks/browser/bookmark_model_unittest.cc

@@ -583,8 +583,7 @@ TEST_F(BookmarkModelTest, AddURLWithGUID) {
   const base::GUID guid = base::GUID::GenerateRandomV4();
 
   const BookmarkNode* new_node =
-      model_->AddURL(root, /*index=*/0, title, url, &meta_info, time,
-                     guid.AsLowercaseString());
+      model_->AddURL(root, /*index=*/0, title, url, &meta_info, time, guid);
 
   EXPECT_EQ(guid, new_node->guid());
 }

components/sync_bookmarks/bookmark_model_merger_unittest.cc

@@ -841,7 +841,7 @@ TEST(BookmarkModelMergerTest, ShouldMergeBookmarkByGUID) {
       bookmark_model->bookmark_bar_node();
   const bookmarks::BookmarkNode* bookmark = bookmark_model->AddURL(
       /*parent=*/bookmark_bar_node, /*index=*/0, base::UTF8ToUTF16(kLocalTitle),
-      GURL(kUrl), nullptr, base::Time::Now(), kGuid.AsLowercaseString());
+      GURL(kUrl), /*meta_info=*/nullptr, base::Time::Now(), kGuid);
   ASSERT_TRUE(bookmark);
   ASSERT_THAT(bookmark_bar_node->children(), ElementRawPointersAre(bookmark));
 
@@ -893,7 +893,7 @@ TEST(BookmarkModelMergerTest, ShouldMergeBookmarkByGUIDAndReparent) {
       base::UTF8ToUTF16("Folder Title"));
   const bookmarks::BookmarkNode* bookmark = bookmark_model->AddURL(
       /*parent=*/folder, /*index=*/0, base::UTF8ToUTF16(kLocalTitle),
-      GURL(kUrl), nullptr, base::Time::Now(), kGuid.AsLowercaseString());
+      GURL(kUrl), /*meta_info=*/nullptr, base::Time::Now(), kGuid);
   ASSERT_TRUE(folder);
   ASSERT_TRUE(bookmark);
   ASSERT_THAT(bookmark_bar_node->children(), ElementRawPointersAre(folder));
@@ -1078,8 +1078,7 @@ TEST(
       base::UTF8ToUTF16(kOriginalTitle), /*meta_info=*/nullptr, kGuid1);
   const bookmarks::BookmarkNode* bookmark = bookmark_model->AddURL(
       /*parent=*/folder, /*index=*/0, base::UTF8ToUTF16("Bookmark Title"),
-      GURL("http://foo.com/"), nullptr, base::Time::Now(),
-      base::GenerateGUID());
+      GURL("http://foo.com/"));
   ASSERT_TRUE(folder);
   ASSERT_TRUE(bookmark);
   ASSERT_THAT(bookmark_bar_node->children(), ElementRawPointersAre(folder));
@@ -1161,8 +1160,7 @@ TEST(BookmarkModelMergerTest,
       base::UTF8ToUTF16(kOriginalTitle), /*meta_info=*/nullptr, kGuid1);
   const bookmarks::BookmarkNode* bookmark = bookmark_model->AddURL(
       /*parent=*/folder, /*index=*/0, base::UTF8ToUTF16("Bookmark Title"),
-      GURL("http://foo.com/"), nullptr, base::Time::Now(),
-      base::GenerateGUID());
+      GURL("http://foo.com/"));
   ASSERT_TRUE(folder);
   ASSERT_TRUE(bookmark);
   ASSERT_THAT(bookmark_bar_node->children(), ElementRawPointersAre(folder));
@@ -1234,7 +1232,7 @@ TEST(BookmarkModelMergerTest, ShouldReplaceBookmarkGUIDWithConflictingURLs) {
       bookmark_model->bookmark_bar_node();
   const bookmarks::BookmarkNode* bookmark = bookmark_model->AddURL(
       /*parent=*/bookmark_bar_node, /*index=*/0, base::UTF8ToUTF16(kTitle),
-      GURL(kUrl1), nullptr, base::Time::Now(), kGuid.AsLowercaseString());
+      GURL(kUrl1), /*meta_info=*/nullptr, base::Time::Now(), kGuid);
   ASSERT_TRUE(bookmark);
   ASSERT_THAT(bookmark_bar_node->children(), ElementRawPointersAre(bookmark));
 
@@ -1282,8 +1280,8 @@ TEST(BookmarkModelMergerTest, ShouldReplaceBookmarkGUIDWithConflictingTypes) {
       bookmark_model->bookmark_bar_node();
   const bookmarks::BookmarkNode* bookmark = bookmark_model->AddURL(
       /*parent=*/bookmark_bar_node, /*index=*/0, base::UTF8ToUTF16(kTitle),
-      GURL("http://www.foo.com/"), nullptr, base::Time::Now(),
-      kGuid.AsLowercaseString());
+      GURL("http://www.foo.com/"), /*meta_info=*/nullptr, base::Time::Now(),
+      kGuid);
   ASSERT_TRUE(bookmark);
   ASSERT_THAT(bookmark_bar_node->children(), ElementRawPointersAre(bookmark));
 
@@ -1336,8 +1334,7 @@ TEST(BookmarkModelMergerTest,
       base::UTF8ToUTF16("Folder Title"), /*meta_info=*/nullptr, kGuid1);
   const bookmarks::BookmarkNode* bookmark = bookmark_model->AddURL(
       /*parent=*/folder, /*index=*/0, base::UTF8ToUTF16("Foo's title"),
-      GURL("http://foo.com"), nullptr, base::Time::Now(),
-      kGuid2.AsLowercaseString());
+      GURL("http://foo.com"), /*meta_info=*/nullptr, base::Time::Now(), kGuid2);
   ASSERT_TRUE(folder);
   ASSERT_TRUE(bookmark);
   ASSERT_THAT(bookmark_bar_node->children(), ElementRawPointersAre(folder));
@@ -1399,7 +1396,7 @@ TEST(BookmarkModelMergerTest, ShouldIgnoreRemoteGUIDIfOrphanNode) {
       bookmark_model->bookmark_bar_node();
   const bookmarks::BookmarkNode* bookmark = bookmark_model->AddURL(
       /*parent=*/bookmark_bar_node, /*index=*/0, base::UTF8ToUTF16(kTitle),
-      GURL(kUrl), nullptr, base::Time::Now(), kGuid.AsLowercaseString());
+      GURL(kUrl), /*meta_info=*/nullptr, base::Time::Now(), kGuid);
   ASSERT_TRUE(bookmark);
   ASSERT_THAT(bookmark_bar_node->children(), ElementRawPointersAre(bookmark));
 
@@ -1451,7 +1448,7 @@ TEST(BookmarkModelMergerTest, ShouldIgnoreRemoteGUIDIfInvalidSpecifics) {
       bookmark_model->bookmark_bar_node();
   const bookmarks::BookmarkNode* bookmark = bookmark_model->AddURL(
       /*parent=*/bookmark_bar_node, /*index=*/0, base::UTF8ToUTF16(kTitle),
-      GURL(kLocalUrl), nullptr, base::Time::Now(), kGuid.AsLowercaseString());
+      GURL(kLocalUrl), /*meta_info=*/nullptr, base::Time::Now(), kGuid);
   ASSERT_TRUE(bookmark);
   ASSERT_THAT(bookmark_bar_node->children(), ElementRawPointersAre(bookmark));
 
@@ -1502,7 +1499,7 @@ TEST(BookmarkModelMergerTest, ShouldIgnoreRemoteUpdateWithInvalidGUID) {
       bookmark_model->bookmark_bar_node();
   const bookmarks::BookmarkNode* bookmark = bookmark_model->AddURL(
       /*parent=*/bookmark_bar_node, /*index=*/0, base::UTF8ToUTF16(kLocalTitle),
-      GURL(kUrl), nullptr, base::Time::Now(), kGuid.AsLowercaseString());
+      GURL(kUrl), /*meta_info=*/nullptr, base::Time::Now(), kGuid);
   ASSERT_TRUE(bookmark);
   ASSERT_THAT(bookmark_bar_node->children(), ElementRawPointersAre(bookmark));
 

components/sync_bookmarks/bookmark_specifics_conversions.cc

@@ -284,8 +284,7 @@ const bookmarks::BookmarkNode* CreateBookmarkNodeFromSpecifics(
         // always used the Windows epoch.
         base::TimeDelta::FromMicroseconds(create_time_us));
     node = model->AddURL(parent, index, NodeTitleFromSpecifics(specifics),
-                         GURL(specifics.url()), &metainfo, create_time,
-                         guid.AsLowercaseString());
+                         GURL(specifics.url()), &metainfo, create_time, guid);
   }
   SetBookmarkFaviconFromSpecifics(specifics, node, favicon_service);
   return node;
@@ -333,10 +332,9 @@ const bookmarks::BookmarkNode* ReplaceBookmarkNodeGUID(
         model->AddFolder(node->parent(), node->parent()->GetIndexOf(node),
                          node->GetTitle(), node->GetMetaInfoMap(), guid);
   } else {
-    new_node =
-        model->AddURL(node->parent(), node->parent()->GetIndexOf(node),
-                      node->GetTitle(), node->url(), node->GetMetaInfoMap(),
-                      node->date_added(), guid.AsLowercaseString());
+    new_node = model->AddURL(node->parent(), node->parent()->GetIndexOf(node),
+                             node->GetTitle(), node->url(),
+                             node->GetMetaInfoMap(), node->date_added(), guid);
   }
   for (size_t i = node->children().size(); i > 0; --i) {
     model->Move(node->children()[i - 1].get(), new_node, 0);