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

This reverts commit 3c5bfcbb.

Reason for revert:
Release build bot is still failing.
https://bugs.chromium.org/p/chromium/issues/detail?id=1165869
https://ci.chromium.org/p/chrome/builders/ci/android-arm-official-tests/1731

Original change's description:
> 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/+/2622890
> Reviewed-by: David Trainor <dtrainor@chromium.org>
> Commit-Queue: Xing Liu <xingliu@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#842237}

TBR=dtrainor@chromium.org,xingliu@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com

Change-Id: Ib282f1fd2aaaab2739c6cf96d25f257d35b511e3
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1157808
Bug: 1163225
Bug: 1165869
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2625276
Commit-Queue: Xing Liu <xingliu@chromium.org>
Reviewed-by: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842663}

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

@@ -840,10 +840,9 @@ 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, or null on
-     *         error.
+     * @return The bookmark ID created after saving the article to the reading list.
      */
-    public @Nullable BookmarkId addToReadingList(String title, String url) {
+    public 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,15 +17,11 @@ 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;
@@ -318,22 +314,4 @@ 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,9 +828,8 @@ ScopedJavaLocalRef<jobject> BookmarkBridge::AddToReadingList(
   const BookmarkNode* node = reading_list_manager_->Add(
       GURL(base::android::ConvertJavaStringToUTF16(env, j_url)),
       base::android::ConvertJavaStringToUTF8(env, j_title));
-  return node ? JavaBookmarkIdCreateBookmarkId(env, node->id(),
-                                               GetBookmarkType(node))
-              : ScopedJavaLocalRef<jobject>();
+  DCHECK(node);
+  return JavaBookmarkIdCreateBookmarkId(env, node->id(), GetBookmarkType(node));
 }
 
 ScopedJavaLocalRef<jobject> BookmarkBridge::GetReadingListItem(

chrome/browser/reading_list/android/empty_reading_list_manager.cc

@@ -4,7 +4,6 @@
 
 #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;
@@ -18,7 +17,6 @@ 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. May return nullptr on error.
+  // node pointer will be invalidated.
   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,7 +32,6 @@ 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:
@@ -229,31 +228,6 @@ 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);