Reland "[Bookmarks] Switching the bookmark bridge ownership to native"

This is a reland of commit 2f25ad1 - This change requires a 3-way patch so the downstream code compiles correctly. - Patch 1: crrev.com/c/4017933 - Patch 2: (internal) 5083693 - Patch 3: (this patch) re-land of the original CL Original change's description: > [Bookmarks] Switching the bookmark bridge ownership to native > > This patch also removes references to BookmarkModel constructor and > effectively hides the BookmarkBridge, but we still need to combine the > two classes, as they don't make much sense separately. > > Bug: 1150129 > Change-Id: I7ae62043e10cd90e45aef2b3e8904c30210e34f3 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3969584 > Reviewed-by: Scott Violet <sky@chromium.org> > Commit-Queue: Filip Gorski <fgorski@chromium.org> > Reviewed-by: Matthew Jones <mdjones@chromium.org> > Reviewed-by: Brandon Wylie <wylieb@chromium.org> > Cr-Commit-Position: refs/heads/main@{#1067223} Bug: 1150129 Change-Id: I2ee5af9946d2ce65df827da9c7b7dc210d147a91 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4018044 Reviewed-by: Scott Violet <sky@chromium.org> Commit-Queue: Brandon Wylie <wylieb@chromium.org> Reviewed-by: Matthew Jones <mdjones@chromium.org> Cr-Commit-Position: refs/heads/main@{#1073607}
chromium · Nov 18, 2022 · 62d8d27 · 62d8d27
1 parent 9b77fd5
commit 62d8d27
Show file tree

Hide file tree

Showing 27 changed files with 184 additions and 217 deletions.
diff --git a/...urface/java/src/org/chromium/chrome/features/start_surface/ExploreSurfaceCoordinator.java b/...urface/java/src/org/chromium/chrome/features/start_surface/ExploreSurfaceCoordinator.java
@@ -83,7 +83,7 @@ public ExploreSurfaceCoordinator(Profile profile, Activity activity, boolean isI
                 SurfaceType.START_SURFACE, embeddingSurfaceConstructedTimeNs, swipeRefreshLayout,
                 /*overScrollDisabled=*/true, parentView,
                 new ExploreSurfaceActionDelegate(
-                        snackbarManager, new BookmarkModel(profile), crowButtonDelegate),
+                        snackbarManager, BookmarkModel.getForProfile(profile), crowButtonDelegate),
                 HelpAndFeedbackLauncherImpl.getInstance(), tabModelSelector);
 
         mFeedSurfaceCoordinator.getView().setId(R.id.start_surface_explore_view);

diff --git a/chrome/android/java/src/org/chromium/chrome/browser/app/ChromeActivity.java b/chrome/android/java/src/org/chromium/chrome/browser/app/ChromeActivity.java
@@ -290,7 +290,7 @@ public abstract class ChromeActivity<C extends ChromeActivityComponent>
 
     protected TabModelSelectorProfileSupplier mTabModelProfileSupplier =
             new TabModelSelectorProfileSupplier(mTabModelSelectorSupplier);
-    protected ObservableSupplierImpl<BookmarkModel> mBookmarkModelSupplier =
+    protected final ObservableSupplierImpl<BookmarkModel> mBookmarkModelSupplier =
             new ObservableSupplierImpl<>();
     protected ObservableSupplierImpl<TabBookmarker> mTabBookmarkerSupplier =
             new ObservableSupplierImpl<>();
@@ -474,9 +474,8 @@ public void performPreInflationStartup() {
         // There's no corresponding call to removeObserver() for this addObserver() because
         // mTabModelProfileSupplier has the same lifecycle as this activity.
         mTabModelProfileSupplier.addObserver((profile) -> {
-            BookmarkModel oldBridge = mBookmarkModelSupplier.get();
-            if (oldBridge != null) oldBridge.destroy();
-            mBookmarkModelSupplier.set(profile == null ? null : new BookmarkModel(profile));
+            mBookmarkModelSupplier.set(
+                    profile == null ? null : BookmarkModel.getForProfile(profile));
         });
 
         super.performPreInflationStartup();
@@ -1599,11 +1598,7 @@ protected final void onDestroy() {
 
         destroyTabModels();
 
-        if (mBookmarkModelSupplier != null) {
-            BookmarkModel bookmarkModel = mBookmarkModelSupplier.get();
-            if (bookmarkModel != null) bookmarkModel.destroy();
-            mBookmarkModelSupplier = null;
-        }
+        mBookmarkModelSupplier.set(null);
 
         if (mShareDelegateSupplier != null) {
             mShareDelegateSupplier.destroy();
@@ -2518,8 +2513,9 @@ public boolean onMenuOrKeyboardAction(int id, boolean fromMenu) {
         }
 
         if (id == R.id.delete_from_reading_list_menu_id) {
+            assert mBookmarkModelSupplier.hasValue();
             ReadingListUtils.deleteFromReadingList(
-                    new BookmarkModel(), mSnackbarManager, /*activity=*/this, currentTab);
+                    mBookmarkModelSupplier.get(), mSnackbarManager, /*activity=*/this, currentTab);
             RecordUserAction.record("MobileMenuDeleteFromReadingList");
             return true;
         }

diff --git a/...oid/java/src/org/chromium/chrome/browser/app/bookmarks/BookmarkAddEditFolderActivity.java b/...oid/java/src/org/chromium/chrome/browser/app/bookmarks/BookmarkAddEditFolderActivity.java
@@ -24,6 +24,7 @@
 import org.chromium.chrome.browser.bookmarks.BookmarkModelObserver;
 import org.chromium.chrome.browser.bookmarks.BookmarkTextInputLayout;
 import org.chromium.chrome.browser.bookmarks.BookmarkUtils;
+import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.components.bookmarks.BookmarkId;
 import org.chromium.components.bookmarks.BookmarkItem;
 import org.chromium.components.browser_ui.widget.TintedDrawable;
@@ -126,7 +127,7 @@ public static void startAddFolderActivity(
     @Override
     public void onCreate(Bundle savedInstanceState) {
         super.onCreate(savedInstanceState);
-        mModel = new BookmarkModel();
+        mModel = BookmarkModel.getForProfile(Profile.getLastUsedRegularProfile());
         mModel.addObserver(mBookmarkModelObserver);
         mIsAddMode = getIntent().getBooleanExtra(INTENT_IS_ADD_MODE, false);
         if (mIsAddMode) {
@@ -259,8 +260,6 @@ protected void onActivityResult(int requestCode, int resultCode, Intent data) {
     protected void onDestroy() {
         super.onDestroy();
         mModel.removeObserver(mBookmarkModelObserver);
-        mModel.destroy();
-        mModel = null;
     }
 
     private void updateParent(BookmarkId newParent) {

diff --git a/chrome/android/java/src/org/chromium/chrome/browser/app/bookmarks/BookmarkEditActivity.java b/chrome/android/java/src/org/chromium/chrome/browser/app/bookmarks/BookmarkEditActivity.java
@@ -21,6 +21,7 @@
 import org.chromium.chrome.browser.bookmarks.BookmarkModelObserver;
 import org.chromium.chrome.browser.bookmarks.BookmarkTextInputLayout;
 import org.chromium.chrome.browser.bookmarks.BookmarkUtils;
+import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.components.bookmarks.BookmarkId;
 import org.chromium.components.bookmarks.BookmarkItem;
 import org.chromium.components.browser_ui.widget.TintedDrawable;
@@ -64,7 +65,7 @@ public void bookmarkModelChanged() {
     protected void onCreate(Bundle savedInstanceState) {
         super.onCreate(savedInstanceState);
 
-        mModel = new BookmarkModel();
+        mModel = BookmarkModel.getForProfile(Profile.getLastUsedRegularProfile());
         mBookmarkId =
                 BookmarkId.getBookmarkIdFromString(getIntent().getStringExtra(INTENT_BOOKMARK_ID));
         mModel.addObserver(mBookmarkModelObserver);
@@ -179,8 +180,6 @@ protected void onStop() {
     @Override
     protected void onDestroy() {
         mModel.removeObserver(mBookmarkModelObserver);
-        mModel.destroy();
-        mModel = null;
         super.onDestroy();
     }
 

diff --git a/...roid/java/src/org/chromium/chrome/browser/app/bookmarks/BookmarkFolderSelectActivity.java b/...roid/java/src/org/chromium/chrome/browser/app/bookmarks/BookmarkFolderSelectActivity.java
@@ -32,6 +32,7 @@
 import org.chromium.chrome.browser.bookmarks.BookmarkModelObserver;
 import org.chromium.chrome.browser.bookmarks.BookmarkUtils;
 import org.chromium.chrome.browser.bookmarks.ReadingListFeatures;
+import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.chrome.browser.read_later.ReadingListUtils;
 import org.chromium.components.bookmarks.BookmarkId;
 import org.chromium.components.bookmarks.BookmarkItem;
@@ -137,7 +138,7 @@ public static void startNewFolderSelectActivity(
     @Override
     public void onCreate(Bundle savedInstanceState) {
         super.onCreate(savedInstanceState);
-        mModel = new BookmarkModel();
+        mModel = BookmarkModel.getForProfile(Profile.getLastUsedRegularProfile());
         List<String> stringList =
                 IntentUtils.safeGetStringArrayListExtra(getIntent(), INTENT_BOOKMARKS_TO_MOVE);
         mBookmarksToMove = new ArrayList<>(stringList.size());
@@ -247,8 +248,6 @@ public boolean onOptionsItemSelected(MenuItem item) {
     protected void onDestroy() {
         super.onDestroy();
         mModel.removeObserver(mBookmarkModelObserver);
-        mModel.destroy();
-        mModel = null;
     }
 
     /**

diff --git a/chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkBridge.java b/chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkBridge.java
@@ -48,29 +48,31 @@
  * bookmark model stored in native.
  */
 class BookmarkBridge {
-    private final Profile mProfile;
+    private long mNativeBookmarkBridge;
     private boolean mIsDestroyed;
     private boolean mIsDoingExtensiveChanges;
-    private long mNativeBookmarkBridge;
     private boolean mIsNativeBookmarkModelLoaded;
     private final ObserverList<BookmarkModelObserver> mObservers =
             new ObserverList<BookmarkModelObserver>();
     private SubscriptionsManager mSubscriptionManager;
     private SubscriptionsManager.SubscriptionObserver mSubscriptionsObserver;
 
-    public static BookmarkModel getForProfile(Profile profile) {
-        // TODO(crbug.com/1150129): Make this a singleton owned by native.
-        return new BookmarkModel(profile);
-    }
-
     /**
      * Handler to fetch the bookmarks, titles, urls and folder hierarchy.
      * @param profile Profile instance corresponding to the active profile.
      */
-    public BookmarkBridge(Profile profile) {
+    static BookmarkModel getForProfile(Profile profile) {
         ThreadUtils.assertOnUiThread();
-        mProfile = profile;
-        mNativeBookmarkBridge = BookmarkBridgeJni.get().init(BookmarkBridge.this, profile);
+        return BookmarkBridgeJni.get().getForProfile(profile);
+    }
+
+    @CalledByNative
+    static BookmarkModel createBookmarkModel(long nativeBookmarkBridge) {
+        return new BookmarkModel(nativeBookmarkBridge);
+    }
+
+    BookmarkBridge(long nativeBookmarkBridge) {
+        mNativeBookmarkBridge = nativeBookmarkBridge;
         mIsDoingExtensiveChanges = BookmarkBridgeJni.get().isDoingExtensiveChanges(
                 mNativeBookmarkBridge, BookmarkBridge.this);
         mSubscriptionsObserver = new SubscriptionsManager.SubscriptionObserver() {
@@ -98,7 +100,7 @@ SubscriptionsManager.SubscriptionObserver getSubscriptionObserver() {
     /**
      * Destroys this instance so no further calls can be executed.
      */
-    public void destroy() {
+    void destroy() {
         mIsDestroyed = true;
         if (mNativeBookmarkBridge != 0) {
             BookmarkBridgeJni.get().destroy(mNativeBookmarkBridge, BookmarkBridge.this);
@@ -113,7 +115,7 @@ public void destroy() {
     }
 
     /** Returns whether the bridge has been destroyed. */
-    public boolean isDestroyed() {
+    private boolean isDestroyed() {
         return mIsDestroyed;
     }
 
@@ -755,11 +757,6 @@ public boolean isEditBookmarksEnabled() {
         return BookmarkBridgeJni.get().isEditBookmarksEnabled(mNativeBookmarkBridge);
     }
 
-    /** Gets the profile. */
-    protected Profile getProfile() {
-        return mProfile;
-    }
-
     /**
      * Notifies the observer that bookmark model has been loaded.
      */
@@ -969,6 +966,7 @@ private static List<Pair<Integer, Integer>> createPairsList(int[] left, int[] ri
     @VisibleForTesting(otherwise = VisibleForTesting.PACKAGE_PRIVATE)
     @NativeMethods
     public interface Natives {
+        BookmarkModel getForProfile(Profile profile);
         BookmarkId getBookmarkIdForWebContents(long nativeBookmarkBridge, BookmarkBridge caller,
                 WebContents webContents, boolean onlyEditable);
         BookmarkItem getBookmarkByID(
@@ -1036,7 +1034,6 @@ void searchBookmarks(long nativeBookmarkBridge, BookmarkBridge caller,
                 int powerBookmarkType, int maxNumber);
         void getBookmarksOfType(long nativeBookmarkBridge, BookmarkBridge caller,
                 List<BookmarkId> bookmarkMatches, int powerBookmarkType);
-        long init(BookmarkBridge caller, Profile profile);
         boolean isDoingExtensiveChanges(long nativeBookmarkBridge, BookmarkBridge caller);
         void destroy(long nativeBookmarkBridge, BookmarkBridge caller);
         boolean isEditBookmarksEnabled(long nativeBookmarkBridge);

diff --git a/chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkManager.java b/chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkManager.java
@@ -203,7 +203,8 @@ public boolean toggleSelectionForItem(BookmarkId bookmark) {
 
         mDragStateDelegate = new BookmarkDragStateDelegate();
 
-        mBookmarkModel = new BookmarkModel();
+        Profile profile = Profile.getLastUsedRegularProfile();
+        mBookmarkModel = BookmarkModel.getForProfile(profile);
         mMainView = (ViewGroup) LayoutInflater.from(mContext).inflate(R.layout.bookmark_main, null);
 
         // TODO(1293885): Remove this validator once we have an API on the backend that sends
@@ -213,8 +214,7 @@ public boolean toggleSelectionForItem(BookmarkId bookmark) {
                     new CommerceSubscriptionsServiceFactory()
                             .getForLastUsedProfile()
                             .getSubscriptionsManager());
-            ShoppingServiceFactory.getForProfile(Profile.getLastUsedRegularProfile())
-                    .scheduleSavedProductUpdate();
+            ShoppingServiceFactory.getForProfile(profile).scheduleSavedProductUpdate();
         }
 
         @SuppressWarnings("unchecked")
@@ -266,7 +266,7 @@ public void onChanged() {
             mBookmarkModel.finishLoadingBookmarkModel(modelLoadedRunnable);
         }
 
-        mLargeIconBridge = new LargeIconBridge(Profile.getLastUsedRegularProfile());
+        mLargeIconBridge = new LargeIconBridge(profile);
         ActivityManager activityManager = ((ActivityManager) ContextUtils
                 .getApplicationContext().getSystemService(Context.ACTIVITY_SERVICE));
         int maxSize =
@@ -313,8 +313,6 @@ public void onDestroyed() {
             mUndoController = null;
         }
         mBookmarkModel.removeObserver(mBookmarkModelObserver);
-        mBookmarkModel.destroy();
-        mBookmarkModel = null;
         mLargeIconBridge.destroy();
         mLargeIconBridge = null;
         PartnerBookmarksReader.removeFaviconUpdateObserver(this);

diff --git a/chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java b/chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java
@@ -7,6 +7,7 @@
 import androidx.annotation.NonNull;
 
 import org.chromium.base.ObserverList;
+import org.chromium.base.ThreadUtils;
 import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.components.bookmarks.BookmarkId;
 import org.chromium.components.bookmarks.BookmarkItem;
@@ -38,19 +39,25 @@ public interface BookmarkDeleteObserver {
 
     private ObserverList<BookmarkDeleteObserver> mDeleteObservers = new ObserverList<>();
 
-    public static BookmarkModel getForProfile(Profile profile) {
+    /**
+     * Provides an instance of the bookmark model for the provided profile.
+     * @param profile A profile for which the bookmark model is provided.
+     * @return An instance of the bookmark model.
+     */
+    public static final BookmarkModel getForProfile(@NonNull Profile profile) {
+        assert profile != null;
+        ThreadUtils.assertOnUiThread();
         return BookmarkBridge.getForProfile(profile);
     }
 
-    /**
-     * Initialize bookmark model for last used non-incognito profile.
-     */
-    public BookmarkModel() {
-        this(Profile.getLastUsedRegularProfile());
+    BookmarkModel(long nativeBookmarkBridge) {
+        super(nativeBookmarkBridge);
     }
 
-    public BookmarkModel(Profile profile) {
-        super(profile);
+    @Override
+    // TODO(crbug.com/1150129): Remove this method once destroy calls are removed.
+    public void destroy() {
+        super.destroy();
     }
 
     /**

diff --git a/...e/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkSaveFlowCoordinator.java b/...e/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkSaveFlowCoordinator.java
@@ -66,7 +66,7 @@ public BookmarkSaveFlowCoordinator(@NonNull Context context,
         mContext = context;
         mBottomSheetController = bottomSheetController;
         mUserEducationHelper = userEducationHelper;
-        mBookmarkModel = new BookmarkModel();
+        mBookmarkModel = BookmarkModel.getForProfile(Profile.getLastUsedRegularProfile());
         mDestroyChecker = new DestroyChecker();
 
         mBookmarkSaveFlowView = LayoutInflater.from(mContext).inflate(
@@ -177,9 +177,6 @@ private void destroy() {
 
         mBookmarkSaveFlowView = null;
 
-        mBookmarkModel.destroy();
-        mBookmarkModel = null;
-
         mChangeProcessor.destroy();
     }
 

diff --git a/chrome/android/java/src/org/chromium/chrome/browser/bookmarks/PowerBookmarkUtils.java b/chrome/android/java/src/org/chromium/chrome/browser/bookmarks/PowerBookmarkUtils.java
@@ -271,7 +271,7 @@ public void bookmarkModelChanged() {}
     /** @see #validateBookmarkedCommerceSubscriptions(BookmarkModel, SubscriptionsManager) */
     private static void doBookmarkedSubscriptionValidation(BookmarkModel bookmarkModel,
             SubscriptionsManager subscriptionsManager, List<CommerceSubscription> subscriptions) {
-        if (bookmarkModel.isDestroyed() || subscriptions == null) return;
+        if (subscriptions == null) return;
 
         List<BookmarkId> products =
                 bookmarkModel.searchBookmarks("", null, PowerBookmarkType.SHOPPING, -1);

diff --git a/chrome/android/java/src/org/chromium/chrome/browser/bookmarks/TabBookmarker.java b/chrome/android/java/src/org/chromium/chrome/browser/bookmarks/TabBookmarker.java
@@ -96,19 +96,17 @@ private void addOrEditBookmark(
         }
 
         // Defense in depth against the UI being erroneously enabled.
-        BookmarkModel bridge = mBookmarkModelSupplier.get();
-        if (bridge == null || !bridge.isEditBookmarksEnabled()) {
+        final BookmarkModel bookmarkModel = mBookmarkModelSupplier.get();
+        if (bookmarkModel == null || !bookmarkModel.isEditBookmarksEnabled()) {
             assert false;
             return;
         }
 
-        final BookmarkModel bookmarkModel = new BookmarkModel();
         bookmarkModel.finishLoadingBookmarkModel(() -> {
             // Gives up the bookmarking if the tab is being destroyed.
             if (tabToBookmark.isClosing() || !tabToBookmark.isInitialized()
                     || mBottomSheetControllerSupplier.get() == null
                     || mSnackbarManagerSupplier.get() == null) {
-                bookmarkModel.destroy();
                 return;
             }
 
@@ -129,8 +127,6 @@ private void addOrEditBookmark(
     private void onBookmarkModelLoaded(final Tab tabToBookmark,
             @Nullable final BookmarkItem currentBookmarkItem, final BookmarkModel bookmarkModel,
             @BookmarkType int bookmarkType, boolean fromExplicitTrackUi) {
-        // The BookmarkModel will be destroyed by BookmarkUtils#addOrEditBookmark() when
-        // done.
         BookmarkUtils.addOrEditBookmark(currentBookmarkItem, bookmarkModel, tabToBookmark,
                 mSnackbarManagerSupplier.get(), mBottomSheetControllerSupplier.get(), mActivity,
                 mIsCustomTab, bookmarkType, (newBookmarkId) -> {
@@ -140,7 +136,6 @@ private void onBookmarkModelLoaded(final Tab tabToBookmark,
                     if (newBookmarkId != null && !newBookmarkId.equals(currentBookmarkId)) {
                         OfflinePageUtils.saveBookmarkOffline(newBookmarkId, tabToBookmark);
                     }
-                    bookmarkModel.destroy();
                 }, fromExplicitTrackUi);
     }
 }