[reading list] Remove feature flag and cleanup code

Bug: 1411914
Change-Id: Ic7480fa5dca7a79af34317620659b41c9e1f8d74
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4228280
Reviewed-by: Sky Malice <skym@chromium.org>
Reviewed-by: Justin DeWitt <dewittj@chromium.org>
Commit-Queue: Brandon Wylie <wylieb@chromium.org>
Reviewed-by: Olivier Robin <olivierrobin@chromium.org>
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1109205}

chrome/android/java/src/org/chromium/chrome/browser/app/flags/ChromeCachedFlags.java

@@ -107,7 +107,6 @@ public void cacheNativeFlags() {
                 ChromeFeatureList.sPaintPreviewDemo,
                 ChromeFeatureList.sQueryTiles,
                 ChromeFeatureList.sQueryTilesOnStart,
-                ChromeFeatureList.sReadLater,
                 ChromeFeatureList.sShouldIgnoreIntentSkipInternalCheck,
                 ChromeFeatureList.sStartSurfaceAndroid,
                 ChromeFeatureList.sStartSurfaceDisabledFeedImprovement,

chrome/android/java/src/org/chromium/chrome/browser/ntp/LegacyIncognitoDescriptionView.java

@@ -26,7 +26,6 @@
 import androidx.appcompat.widget.SwitchCompat;
 
 import org.chromium.chrome.R;
-import org.chromium.chrome.browser.flags.ChromeFeatureList;
 import org.chromium.components.content_settings.CookieControlsEnforcement;
 import org.chromium.ui.base.ViewUtils;
 import org.chromium.ui.text.NoUnderlineClickableSpan;
@@ -285,10 +284,8 @@ private void adjustIcon() {
 
     /** Adjust the "Learn More" link. */
     private void adjustLearnMore() {
-        boolean readLaterEnabled = ChromeFeatureList.sReadLater.isEnabled();
-        final String subtitleText = getContext().getResources().getString(readLaterEnabled
-                        ? R.string.new_tab_otr_subtitle_with_reading_list
-                        : R.string.new_tab_otr_subtitle);
+        final String subtitleText = getContext().getResources().getString(
+                R.string.new_tab_otr_subtitle_with_reading_list);
         boolean learnMoreInSubtitle = mWidthDp > WIDE_LAYOUT_THRESHOLD_DP;
 
         mLearnMore.setVisibility(learnMoreInSubtitle ? View.GONE : View.VISIBLE);

chrome/android/javatests/src/org/chromium/chrome/browser/app/bookmarks/ReadingListTest.java

@@ -40,7 +40,6 @@
 import org.mockito.junit.MockitoJUnit;
 import org.mockito.junit.MockitoRule;
 
-import org.chromium.base.FeatureList;
 import org.chromium.base.test.util.ApplicationTestUtils;
 import org.chromium.base.test.util.CommandLineFlags;
 import org.chromium.base.test.util.Criteria;
@@ -61,7 +60,6 @@
 import org.chromium.chrome.browser.bookmarks.BookmarkRow;
 import org.chromium.chrome.browser.bookmarks.BookmarkUIState;
 import org.chromium.chrome.browser.bookmarks.BookmarkUtils;
-import org.chromium.chrome.browser.flags.ChromeFeatureList;
 import org.chromium.chrome.browser.flags.ChromeSwitches;
 import org.chromium.chrome.browser.sync.SyncService;
 import org.chromium.chrome.browser.tab.Tab;
@@ -71,7 +69,6 @@
 import org.chromium.chrome.test.util.ActivityTestUtils;
 import org.chromium.chrome.test.util.BookmarkTestUtil;
 import org.chromium.chrome.test.util.MenuUtils;
-import org.chromium.chrome.test.util.browser.Features;
 import org.chromium.components.bookmarks.BookmarkId;
 import org.chromium.components.bookmarks.BookmarkType;
 import org.chromium.components.browser_ui.widget.RecyclerViewTestUtils;
@@ -91,7 +88,6 @@
 @RunWith(ChromeJUnit4ClassRunner.class)
 @CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE})
 @DoNotBatch(reason = "BookmarkTest has behaviours and thus can't be batched.")
-@Features.EnableFeatures({ChromeFeatureList.READ_LATER})
 public class ReadingListTest {
     @Rule
     public ChromeTabbedActivityTestRule mActivityTestRule = new ChromeTabbedActivityTestRule();
@@ -104,8 +100,6 @@ public class ReadingListTest {
     private static final String TEST_PAGE_URL_FOO = "/chrome/test/data/android/test.html";
     private static final int TEST_PORT = 12345;
 
-    FeatureList.TestValues mTestValues;
-
     private BookmarkManager mManager;
     private BookmarkModel mBookmarkModel;
     private RecyclerView mItemsContainer;
@@ -122,12 +116,6 @@ public class ReadingListTest {
 
     @Before
     public void setUp() {
-        mTestValues = new FeatureList.TestValues();
-        FeatureList.setTestValues(mTestValues);
-        mTestValues.addFeatureFlagOverride(ChromeFeatureList.READ_LATER, true);
-        mTestValues.addFeatureFlagOverride(ChromeFeatureList.SHOPPING_LIST, false);
-        setFieldTrialParamForReadLater("use_root_bookmark_as_default", "true");
-
         mActivityTestRule.startMainActivityOnBlankPage();
         TestThreadUtils.runOnUiThreadBlocking(() -> {
             mBookmarkModel = mActivityTestRule.getActivity().getBookmarkModelForTesting();
@@ -150,10 +138,6 @@ public void tearDown() throws Exception {
         if (mActionTester != null) mActionTester.tearDown();
     }
 
-    private void setFieldTrialParamForReadLater(String name, String value) {
-        mTestValues.addFieldTrialParamOverride(ChromeFeatureList.READ_LATER, name, value);
-    }
-
     private void openBookmarkManager() throws InterruptedException {
         BookmarkTestUtil.readPartnerBookmarks(mActivityTestRule);
         BookmarkTestUtil.waitForBookmarkModelLoaded();
@@ -227,8 +211,6 @@ private BookmarkManager getBookmarkManager() {
     @Restriction({UiRestriction.RESTRICTION_TYPE_PHONE})
     public void testOpenBookmarkManagerWhenDefaultToRootEnabled()
             throws InterruptedException, ExecutionException {
-        setFieldTrialParamForReadLater("use_root_bookmark_as_default", "true");
-
         openBookmarkManager();
         BookmarkDelegate delegate = getBookmarkManager();
         BookmarkActionBar toolbar = ((BookmarkManager) delegate).getToolbarForTests();

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

@@ -347,11 +347,8 @@ public void testGetUserBookmarkIdForTab() {
     @SmallTest
     @UiThreadTest
     @RequiresRestart
-    @Features.EnableFeatures({ChromeFeatureList.READ_LATER})
     @DisabledTest(message = "Broken on official bot, crbug.com/1165869")
     public void testAddToReadingList() {
-        Assert.assertTrue("Read later feature is not loaded properly.",
-                ChromeFeatureList.isEnabled(ChromeFeatureList.READ_LATER));
         Assert.assertNull("Should return null for non http/https URLs.",
                 mBookmarkBridge.addToReadingList("a", new GURL("chrome://flags")));
         BookmarkId readingListId =

chrome/android/junit/src/org/chromium/chrome/browser/read_later/ReadingListUtilsUnitTest.java

@@ -22,11 +22,8 @@
 import org.mockito.MockitoAnnotations;
 import org.robolectric.annotation.Config;
 
-import org.chromium.base.FeatureList;
-import org.chromium.base.FeatureList.TestValues;
 import org.chromium.base.test.BaseRobolectricTestRunner;
 import org.chromium.chrome.browser.bookmarks.BookmarkModel;
-import org.chromium.chrome.browser.flags.ChromeFeatureList;
 import org.chromium.chrome.browser.tab.Tab;
 import org.chromium.chrome.browser.ui.messages.snackbar.SnackbarManager;
 import org.chromium.components.bookmarks.BookmarkId;
@@ -122,11 +119,7 @@ public void isSwappableReadingListItem() {
         BookmarkId readingListId = new BookmarkId(1, BookmarkType.READING_LIST);
         BookmarkId regularId = new BookmarkId(1, BookmarkType.NORMAL);
 
-        // wrong type
-        setBookmarkTypeSwappingEnabled(true);
         Assert.assertFalse(ReadingListUtils.isSwappableReadingListItem(regularId));
-
-        setBookmarkTypeSwappingEnabled(true);
         Assert.assertTrue(ReadingListUtils.isSwappableReadingListItem(readingListId));
     }
 
@@ -136,21 +129,15 @@ public void maybeTypeSwapAndShowSaveFlow_EdgeCases() {
         BookmarkId bookmarkId = Mockito.mock(BookmarkId.class);
         doReturn(BookmarkType.NORMAL).when(bookmarkId).getType();
 
-        // bookmarkId null
-        setBookmarkTypeSwappingEnabled(true);
         Assert.assertFalse(ReadingListUtils.maybeTypeSwapAndShowSaveFlow(
                 Mockito.mock(Activity.class), Mockito.mock(BottomSheetController.class),
                 Mockito.mock(BookmarkModel.class), /*bookmarkId=*/null, BookmarkType.READING_LIST));
 
-        // bad original type
-        setBookmarkTypeSwappingEnabled(true);
         doReturn(BookmarkType.READING_LIST).when(bookmarkId).getType();
         Assert.assertFalse(ReadingListUtils.maybeTypeSwapAndShowSaveFlow(
                 Mockito.mock(Activity.class), Mockito.mock(BottomSheetController.class),
                 Mockito.mock(BookmarkModel.class), bookmarkId, BookmarkType.READING_LIST));
 
-        // bad desired type
-        setBookmarkTypeSwappingEnabled(true);
         doReturn(BookmarkType.NORMAL).when(bookmarkId).getType();
         Assert.assertFalse(ReadingListUtils.maybeTypeSwapAndShowSaveFlow(
                 Mockito.mock(Activity.class), Mockito.mock(BottomSheetController.class),
@@ -160,8 +147,6 @@ public void maybeTypeSwapAndShowSaveFlow_EdgeCases() {
     @Test
     @SmallTest
     public void testTypeSwapBookmarksIfNecessary_ToReadingList() {
-        setBookmarkTypeSwappingEnabled(true);
-
         BookmarkId parentId = new BookmarkId(0, BookmarkType.READING_LIST);
         BookmarkId existingBookmarkId = new BookmarkId(0, BookmarkType.NORMAL);
         BookmarkItem existingBookmark = new BookmarkItem(existingBookmarkId, "Test",
@@ -189,8 +174,6 @@ public void testTypeSwapBookmarksIfNecessary_ToReadingList() {
     @Test
     @SmallTest
     public void testTypeSwapBookmarksIfNecessary_ToBookmark() {
-        setBookmarkTypeSwappingEnabled(true);
-
         BookmarkId parentId = new BookmarkId(0, BookmarkType.NORMAL);
         BookmarkId existingBookmarkId = new BookmarkId(0, BookmarkType.READING_LIST);
         BookmarkItem existingBookmark = new BookmarkItem(existingBookmarkId, "Test",
@@ -218,8 +201,6 @@ public void testTypeSwapBookmarksIfNecessary_ToBookmark() {
     @Test
     @SmallTest
     public void testTypeSwapBookmarksIfNecessary_ToBookmark_Multiple() {
-        setBookmarkTypeSwappingEnabled(true);
-
         BookmarkId parentId = new BookmarkId(0, BookmarkType.NORMAL);
         BookmarkId existingBookmarkId1 = new BookmarkId(1, BookmarkType.READING_LIST);
         BookmarkItem existingBookmark1 = new BookmarkItem(existingBookmarkId1, "Test1",
@@ -262,8 +243,6 @@ public void testTypeSwapBookmarksIfNecessary_ToBookmark_Multiple() {
     @Test
     @SmallTest
     public void testTypeSwapBookmarksIfNecessary_TypeMatches() {
-        setBookmarkTypeSwappingEnabled(true);
-
         BookmarkId parentId = new BookmarkId(0, BookmarkType.NORMAL);
         BookmarkId existingBookmarkId = new BookmarkId(0, BookmarkType.NORMAL);
         BookmarkItem existingBookmark = new BookmarkItem(existingBookmarkId, "Test",
@@ -280,12 +259,4 @@ public void testTypeSwapBookmarksIfNecessary_TypeMatches() {
         Assert.assertEquals(0, typeSwappedBookmarks.size());
         Assert.assertEquals(existingBookmarkId, bookmarks.get(0));
     }
-
-    private void setBookmarkTypeSwappingEnabled(boolean enabled) {
-        TestValues readLaterTypeSwapping = new TestValues();
-        readLaterTypeSwapping.addFeatureFlagOverride(ChromeFeatureList.READ_LATER, true);
-        readLaterTypeSwapping.addFieldTrialParamOverride(ChromeFeatureList.READ_LATER,
-                "allow_bookmark_type_swapping", enabled ? "true" : "false");
-        FeatureList.setTestValues(readLaterTypeSwapping);
-    }
 }

chrome/android/junit/src/org/chromium/chrome/browser/tabbed_mode/TabbedAppMenuPropertiesDelegateUnitTest.java

@@ -85,8 +85,7 @@
  * Unit tests for {@link TabbedAppMenuPropertiesDelegate}.
  */
 @RunWith(BaseRobolectricTestRunner.class)
-@Features.EnableFeatures({ChromeFeatureList.WEB_FEED, ChromeFeatureList.READ_LATER,
-        ChromeFeatureList.BOOKMARKS_REFRESH})
+@Features.EnableFeatures({ChromeFeatureList.WEB_FEED, ChromeFeatureList.BOOKMARKS_REFRESH})
 @Features.DisableFeatures({ChromeFeatureList.SHOPPING_LIST, ChromeFeatureList.WEB_APK_UNIQUE_ID})
 public class TabbedAppMenuPropertiesDelegateUnitTest {
     // Costants defining flags that determines multi-window menu items visibility.

chrome/browser/about_flags.cc

@@ -2721,29 +2721,6 @@ constexpr char kBluetoothUseFlossInternalName[] = "bluetooth-use-floss";
 #endif  // BUILDFLAG(IS_CHROMEOS_ASH)
 
 #if BUILDFLAG(IS_ANDROID)
-const FeatureEntry::FeatureParam kReadLaterUseRootBookmarkAsDefault[] = {
-    {"use_root_bookmark_as_default", "true"}};
-const FeatureEntry::FeatureParam kReadLaterInAppMenu[] = {
-    {"use_root_bookmark_as_default", "true"},
-    {"reading_list_in_app_menu", "true"},
-    {"allow_bookmark_type_swapping", "true"}};
-const FeatureEntry::FeatureParam kReadLaterSemiIntegrated[] = {
-    {"use_root_bookmark_as_default", "true"},
-    {"allow_bookmark_type_swapping", "true"}};
-const FeatureEntry::FeatureParam kReadLaterNoCustomTab[] = {
-    {"use_root_bookmark_as_default", "true"},
-    {"use_cct", "false"}};
-
-const FeatureEntry::FeatureVariation kReadLaterVariations[] = {
-    {"(use root bookmark as default)", kReadLaterUseRootBookmarkAsDefault,
-     std::size(kReadLaterUseRootBookmarkAsDefault), nullptr},
-    {"(with app menu item)", kReadLaterInAppMenu,
-     std::size(kReadLaterInAppMenu), nullptr},
-    {"(bookmarks semi-integration)", kReadLaterSemiIntegrated,
-     std::size(kReadLaterSemiIntegrated), nullptr},
-    {"(no custom tab)", kReadLaterNoCustomTab, std::size(kReadLaterNoCustomTab),
-     nullptr}};
-
 const FeatureEntry::FeatureParam kBookmarksRefreshVisuals[] = {
     {"bookmark_visuals_enabled", "true"}};
 const FeatureEntry::FeatureParam kBookmarksRefreshCompactVisuals[] = {
@@ -5649,14 +5626,6 @@ const FeatureEntry kFeatureEntries[] = {
      FEATURE_VALUE_TYPE(features::kQuickSettingsPWANotifications)},
 #endif  // BUILDFLAG(IS_CHROMEOS_ASH)
 
-#if BUILDFLAG(IS_ANDROID)
-    {flag_descriptions::kReadLaterFlagId, flag_descriptions::kReadLaterName,
-     flag_descriptions::kReadLaterDescription, kOsAndroid,
-     FEATURE_WITH_PARAMS_VALUE_TYPE(reading_list::switches::kReadLater,
-                                    kReadLaterVariations,
-                                    "Collections")},
-#endif
-
 #if BUILDFLAG(IS_ANDROID)
     {"read-later-reminder-notification",
      flag_descriptions::kReadLaterReminderNotificationName,

chrome/browser/android/reading_list/reading_list_manager_factory.cc

@@ -6,7 +6,6 @@
 
 #include <memory>
 
-#include "chrome/browser/reading_list/android/empty_reading_list_manager.h"
 #include "chrome/browser/reading_list/android/reading_list_manager_impl.h"
 #include "chrome/browser/reading_list/reading_list_model_factory.h"
 #include "components/reading_list/features/reading_list_switches.h"
@@ -34,9 +33,6 @@ ReadingListManagerFactory::~ReadingListManagerFactory() = default;
 
 KeyedService* ReadingListManagerFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
-  if (!base::FeatureList::IsEnabled(reading_list::switches::kReadLater))
-    return new EmptyReadingListManager();
-
   auto* reading_list_model =
       ReadingListModelFactory::GetForBrowserContext(context);
   return new ReadingListManagerImpl(reading_list_model);

chrome/browser/flag-metadata.json

@@ -6028,11 +6028,6 @@
     "owners": [ "abigailbklein@google.com", "rhalavati@google.com", "//ui/accessibility/OWNERS" ],
     "expiry_milestone": 130
   },
-  {
-    "name": "read-later",
-    "owners": [ "chrome-desktop-ui-sea@google.com", "corising", "wylieb", "chrome-collections@google.com" ],
-    "expiry_milestone": 112
-  },
   {
     "name": "read-later-reminder-notification",
     "owners": [ "wylieb" ],

chrome/browser/flags/android/chrome_feature_list.cc

@@ -372,7 +372,6 @@ const base::Feature* const kFeaturesExposedToJava[] = {
     &query_tiles::features::kQueryTilesInNTP,
     &query_tiles::features::kQueryTilesOnStart,
     &query_tiles::features::kQueryTilesSegmentation,
-    &reading_list::switches::kReadLater,
     &segmentation_platform::features::kContextualPageActions,
     &segmentation_platform::features::kContextualPageActionPriceTracking,
     &segmentation_platform::features::kContextualPageActionReaderMode,

chrome/browser/flags/android/java/src/org/chromium/chrome/browser/flags/ChromeFeatureList.java

@@ -439,7 +439,6 @@ public static boolean getFieldTrialParamByFeatureAsBoolean(
     public static final String QUIET_NOTIFICATION_PROMPTS = "QuietNotificationPrompts";
     public static final String REACHED_CODE_PROFILER = "ReachedCodeProfiler";
     public static final String READER_MODE_IN_CCT = "ReaderModeInCCT";
-    public static final String READ_LATER = "ReadLater";
     public static final String RECORD_SUPPRESSION_METRICS = "RecordSuppressionMetrics";
     public static final String RECOVER_FROM_NEVER_SAVE_ANDROID = "RecoverFromNeverSaveAndroid";
     public static final String REENGAGEMENT_NOTIFICATION = "ReengagementNotification";
@@ -640,7 +639,6 @@ public static boolean getFieldTrialParamByFeatureAsBoolean(
             new CachedFlag(PREFETCH_NOTIFICATION_SCHEDULING_INTEGRATION, false);
     public static final CachedFlag sQueryTiles = new CachedFlag(QUERY_TILES, false);
     public static final CachedFlag sQueryTilesOnStart = new CachedFlag(QUERY_TILES_ON_START, false);
-    public static final CachedFlag sReadLater = new CachedFlag(READ_LATER, false);
     public static final CachedFlag sShouldIgnoreIntentSkipInternalCheck =
             new CachedFlag(SHOULD_IGNORE_INTENT_SKIP_INTERNAL_CHECK, true);
     public static final CachedFlag sStartSurfaceAndroid =

chrome/browser/reading_list/android/BUILD.gn

@@ -12,8 +12,6 @@ group("android") {
 
 source_set("reading_list") {
   sources = [
-    "empty_reading_list_manager.cc",
-    "empty_reading_list_manager.h",
     "reading_list_manager.h",
     "reading_list_manager_impl.cc",
     "reading_list_manager_impl.h",