Reland "Read later: Fix crash and check URL scheme when adding a reading list."

This reverts commit abd87a46.

Diff:

1. Use @RequiresRestart for test BookmarkBridgeTest.testAddToReadingList.

2. Add a logging in EmptyReadingListManager.Add.

Notices still can't repro the test failure with batch test script.

Bug: 1157808,1163225
Change-Id: I4774d7f3468a6aac07dcde167fd8649d7b1e8d26
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2622890Reviewed-by: David Trainor <dtrainor@chromium.org>
Commit-Queue: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842237}

chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkBridge.java

@@ -840,9 +840,10 @@ public class BookmarkBridge {
      * bookmark ID will be returned.
      * @param title The title to be used for the reading list item.
      * @param url The URL of the reading list item.
-     * @return The bookmark ID created after saving the article to the reading list.
+     * @return The bookmark ID created after saving the article to the reading list, or null on
+     *         error.
      */
-    public BookmarkId addToReadingList(String title, String url) {
+    public @Nullable BookmarkId addToReadingList(String title, String url) {
         ThreadUtils.assertOnUiThread();
         assert title != null;
         assert url != null;

chrome/android/javatests/src/org/chromium/chrome/browser/bookmarks/BookmarkBridgeTest.java

@@ -17,11 +17,15 @@ import org.chromium.base.test.BaseJUnit4ClassRunner;
 import org.chromium.base.test.UiThreadTest;
 import org.chromium.base.test.util.Batch;
 import org.chromium.base.test.util.Feature;
+import org.chromium.base.test.util.RequiresRestart;
 import org.chromium.chrome.browser.bookmarks.BookmarkBridge.BookmarkItem;
+import org.chromium.chrome.browser.flags.ChromeFeatureList;
 import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.chrome.test.ChromeBrowserTestRule;
 import org.chromium.chrome.test.util.BookmarkTestUtil;
+import org.chromium.chrome.test.util.browser.Features;
 import org.chromium.components.bookmarks.BookmarkId;
+import org.chromium.components.bookmarks.BookmarkType;
 import org.chromium.content_public.browser.test.util.TestThreadUtils;
 
 import java.util.ArrayList;
@@ -314,4 +318,22 @@ public class BookmarkBridgeTest {
                         + "over partner bookmarks",
                 expectedSearchResults, searchResults);
     }
+
+    @Test
+    @SmallTest
+    @UiThreadTest
+    @RequiresRestart("Reading list glue code uses the flag to load a keyed service.")
+    @Features.EnableFeatures({ChromeFeatureList.READ_LATER})
+    public void testAddToReadingList() {
+        Assert.assertNull("Should return null for non http/https URLs.",
+                mBookmarkBridge.addToReadingList("a", "chrome://flags"));
+        BookmarkId readingListId = mBookmarkBridge.addToReadingList("a", "https://www.google.com/");
+        Assert.assertNotNull(readingListId);
+        Assert.assertEquals(BookmarkType.READING_LIST, readingListId.getType());
+        BookmarkItem readingListItem =
+                mBookmarkBridge.getReadingListItem("https://www.google.com/");
+        Assert.assertNotNull(readingListItem);
+        Assert.assertEquals("https://www.google.com/", readingListItem.getUrl());
+        Assert.assertEquals("a", readingListItem.getTitle());
+    }
 }

chrome/browser/android/bookmarks/bookmark_bridge.cc

@@ -828,8 +828,9 @@ ScopedJavaLocalRef<jobject> BookmarkBridge::AddToReadingList(
   const BookmarkNode* node = reading_list_manager_->Add(
       GURL(base::android::ConvertJavaStringToUTF16(env, j_url)),
       base::android::ConvertJavaStringToUTF8(env, j_title));
-  DCHECK(node);
-  return JavaBookmarkIdCreateBookmarkId(env, node->id(), GetBookmarkType(node));
+  return node ? JavaBookmarkIdCreateBookmarkId(env, node->id(),
+                                               GetBookmarkType(node))
+              : ScopedJavaLocalRef<jobject>();
 }
 
 ScopedJavaLocalRef<jobject> BookmarkBridge::GetReadingListItem(

chrome/browser/reading_list/android/empty_reading_list_manager.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/reading_list/android/empty_reading_list_manager.h"
 
+#include "base/logging.h"
 #include "components/bookmarks/browser/bookmark_utils.h"
 
 EmptyReadingListManager::EmptyReadingListManager() = default;
@@ -17,6 +18,7 @@ void EmptyReadingListManager::RemoveObserver(Observer* observer) {}
 const bookmarks::BookmarkNode* EmptyReadingListManager::Add(
     const GURL& url,
     const std::string& title) {
+  LOG(ERROR) << "Try to add reading list with empty reading list backend.";
   return nullptr;
 }
 

chrome/browser/reading_list/android/reading_list_manager.h

@@ -47,7 +47,7 @@ class ReadingListManager : public KeyedService {
   // Adds a reading list article to the unread section, and return the bookmark
   // node representation. The bookmark node is owned by this class. If there is
   // a duplicate URL, a new bookmark node will be created, and the old bookmark
-  // node pointer will be invalidated.
+  // node pointer will be invalidated. May return nullptr on error.
   virtual const bookmarks::BookmarkNode* Add(const GURL& url,
                                              const std::string& title) = 0;
 

chrome/browser/reading_list/android/reading_list_manager_impl.cc

@@ -132,13 +132,13 @@ void ReadingListManagerImpl::RemoveObserver(Observer* observer) {
 const BookmarkNode* ReadingListManagerImpl::Add(const GURL& url,
                                                 const std::string& title) {
   DCHECK(reading_list_model_->loaded());
+  if (!reading_list_model_->IsUrlSupported(url))
+    return nullptr;
 
   // Add or swap the reading list entry.
   const auto& new_entry = reading_list_model_->AddEntry(
       url, title, reading_list::ADDED_VIA_CURRENT_APP);
   const auto* node = FindBookmarkByURL(new_entry.URL());
-  DCHECK(node)
-      << "Bookmark node should have been create in ReadingListDidAddEntry().";
   return node;
 }
 

chrome/browser/reading_list/android/reading_list_manager_impl_unittest.cc

@@ -32,6 +32,7 @@ constexpr char kTitle1[] = "boring title about dogs.";
 constexpr char kReadStatusKey[] = "read_status";
 constexpr char kReadStatusRead[] = "true";
 constexpr char kReadStatusUnread[] = "false";
+constexpr char kInvalidUTF8[] = "\xc3\x28";
 
 class MockObserver : public ReadingListManager::Observer {
  public:
@@ -228,6 +229,31 @@ TEST_F(ReadingListManagerImplTest, AddTwice) {
   EXPECT_EQ(url, new_node->url());
 }
 
+// If Add() with an invalid title, nullptr will be returned.
+TEST_F(ReadingListManagerImplTest, AddInvalidTitle) {
+  GURL url(kURL);
+
+  // Use an invalid UTF8 string.
+  base::string16 dummy;
+  EXPECT_FALSE(
+      base::UTF8ToUTF16(kInvalidUTF8, base::size(kInvalidUTF8), &dummy));
+  const auto* new_node = Add(url, std::string(kInvalidUTF8));
+  EXPECT_EQ(nullptr, new_node)
+      << "Should return nullptr when failed to parse the title.";
+}
+
+// If Add() with an invalid URL, nullptr will be returned.
+TEST_F(ReadingListManagerImplTest, AddInvalidURL) {
+  GURL invalid_url("chrome://flags");
+  EXPECT_FALSE(reading_list_model()->IsUrlSupported(invalid_url));
+
+  // Use an invalid URL, the observer method ReadingListDidAddEntry() won't be
+  // invoked.
+  const auto* new_node = manager()->Add(invalid_url, kTitle);
+  EXPECT_EQ(nullptr, new_node)
+      << "Should return nullptr when the URL scheme is not supported.";
+}
+
 // Verifes SetReadStatus()/GetReadStatus() API.
 TEST_F(ReadingListManagerImplTest, ReadStatus) {
   GURL url(kURL);