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

This reverts commit 68ff3a4c.

Reason for revert: BookmarkBridgeTest is batched running on official
bot with @Batch annotation, suspect batched tests don't load feature
flag correctly, which make us to load EmptyReadingListManager in cc.

Original change's description:
> Read later: Fix crash and check URL scheme when adding a reading list.
>
> We should expect ReadingListManager::Add to return nullptr when the
> title can't be parsed as UTF strings, or the URL scheme is not http or
> https.
>
> Bug: 1157808,1163225
> Change-Id: I1d4e9ceb74f2821bda19907936d43919c8cc9dbd
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2611628
> Reviewed-by: David Trainor <dtrainor@chromium.org>
> Commit-Queue: Xing Liu <xingliu@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#840779}

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

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1157808
Bug: 1163225
Change-Id: I02e68dd57ccd1fa5d8edec4a0799c80c182b29b8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2616960Reviewed-by: Xing Liu <xingliu@chromium.org>
Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Commit-Queue: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841291}

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

@@ -18,13 +18,10 @@ import org.chromium.base.test.UiThreadTest;
 import org.chromium.base.test.util.Batch;
 import org.chromium.base.test.util.Feature;
 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;
@@ -317,21 +314,4 @@ public class BookmarkBridgeTest {
                         + "over partner bookmarks",
                 expectedSearchResults, searchResults);
     }
-
-    @Test
-    @SmallTest
-    @UiThreadTest
-    @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/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);