[Android] Make BookmarkModelObserver more convenient.

chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.chrome.browser;
 
 import org.chromium.base.CalledByNative;
 import org.chromium.base.ObserverList;
 import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.components.bookmarks.BookmarkId;
 
 import java.util.ArrayList;
 import java.util.List;
 
 /**
  * Provides the communication channel for Android to fetch and manipulate the
  * bookmark model stored in native.
  */
 public class BookmarksBridge {
     private final Profile mProfile;
+    private boolean mExtensiveBookmarkChangesHappening;
     private long mNativeBookmarksBridge;
     private boolean mIsNativeBookmarkModelLoaded;
     private final List<DelayedBookmarkCallback> mDelayedBookmarkCallbacks =
             new ArrayList<DelayedBookmarkCallback>();
     private final ObserverList<BookmarkModelObserver> mObservers =
             new ObserverList<BookmarkModelObserver>();
 
     /**
      * Interface for callback object for fetching bookmarks and folder hierarchy.
      */
@@ skipping 10 matching lines @@
          * Callback method for fetching the folder hierarchy.
          * @param folderId The folder id to which the bookmarks belong.
          * @param bookmarksList List holding the fetched folder details.
          */
         @CalledByNative("BookmarksCallback")
         void onBookmarksFolderHierarchyAvailable(BookmarkId folderId,
                 List<BookmarkItem> bookmarksList);
     }
 
     /**
-     * Interface that provides listeners to be notified of changes to the bookmark model.
+     * Base empty implementation observer class that provides listeners to be notified of changes
+     * to the bookmark model. It's a mandatory to implement one method,

[Ian Wen, 2014/09/10 21:48:51]

s It's a mandatory/It's mandatory

[Kibeom Kim (inactive), 2014/09/10 22:39:16]

Done.

+     * bookmarkModelChangedNotDuringExtensiveChange. Other methods are optional and if they aren't
+     * overridden, the default implementation of them will eventually call
+     * bookmarkModelChangedNotDuringExtensiveChange.
      */
-    public interface BookmarkModelObserver {
+    public abstract static class BookmarkModelObserver {
         /**
-         * Invoked when a node has moved.
+         * Invoked when a node has been removed, the item may still be starred though.
+         * @param parent The parent of the node that was removed.
+         * @param oldIndex The index of the removed node in the parent before it was removed.
+         * @param node The node that was removed.
+         * @param isExtensiveBookmarkChangesHappening whether extensive changes are happening.
+         */
+        public void bookmarkNodeRemoved(BookmarkItem parent, int oldIndex, BookmarkItem node,
+                boolean isExtensiveBookmarkChangesHappening) {
+            if (!isExtensiveBookmarkChangesHappening) {
+                bookmarkNodeRemovedNotDuringExtensiveChange(parent, oldIndex, node);
+            }
+        }
+
+        /**
+         * Invoked when a node has moved. But only when it's not during extensive change.

[Ted C, 2014/09/10 21:57:40]

I wouldn't include "But only when it's not during extensive change." for all
these methods.

I would just document the behavior in the class level description and just put a
extra documentation on bookmarkNodeRemoved.

[Kibeom Kim (inactive), 2014/09/10 22:39:16]

Done.

          * @param oldParent The parent before the move.
          * @param oldIndex The index of the node in the old parent.
          * @param newParent The parent after the move.
          * @param newIndex The index of the node in the new parent.
          */
-        void bookmarkNodeMoved(
-                BookmarkItem oldParent, int oldIndex, BookmarkItem newParent, int newIndex);
+        public void bookmarkNodeMovedNotDuringExtensiveChange(

[Ian Wen, 2014/09/10 21:48:51]

I wonder if we could rename these functions to be something simpler. Now when I
am reading these function names, all I get is NotDuringExtensiveChange and every
function looks the same. How about keeping original names, say
bookmarkNodeMoved, and write comments to warn people that these functions will
only be called not during extensive change period?

[Ted C, 2014/09/10 21:57:39]

I don't think we need these NotDuringExtensiveChange as the documentation should
be sufficient.

+                BookmarkItem oldParent, int oldIndex, BookmarkItem newParent, int newIndex) {
+            bookmarkModelChangedNotDuringExtensiveChange();
+        }
 
         /**
-         * Invoked when a node has been added.
+         * Invoked when a node has been added. But only when it's not during extensive change.
          * @param parent The parent of the node being added.
          * @param index The index of the added node.
          */
-        void bookmarkNodeAdded(BookmarkItem parent, int index);
+        public void bookmarkNodeAddedNotDuringExtensiveChange(BookmarkItem parent, int index) {
+            bookmarkModelChangedNotDuringExtensiveChange();
+        }
 
         /**
          * Invoked when a node has been removed, the item may still be starred though.
+         * But only when it's not during extensive change.
          * @param parent The parent of the node that was removed.
          * @param oldIndex The index of the removed node in the parent before it was removed.
          * @param node The node that was removed.
          */
-        void bookmarkNodeRemoved(BookmarkItem parent, int oldIndex, BookmarkItem node);
+        public void bookmarkNodeRemovedNotDuringExtensiveChange(BookmarkItem parent, int oldIndex,
+                BookmarkItem node) {
+            bookmarkModelChangedNotDuringExtensiveChange();
+        }
 
         /**
-         * Invoked when the title or url of a node changes.
+         * Invoked when the title or url of a node changes. But only when it's not during extensive
+         * change.
          * @param node The node being changed.
          */
-        void bookmarkNodeChanged(BookmarkItem node);
+        public void bookmarkNodeChangedNotDuringExtensiveChange(BookmarkItem node) {
+            bookmarkModelChangedNotDuringExtensiveChange();
+        }
 
         /**
          * Invoked when the children (just direct children, not descendants) of a node have been
-         * reordered in some way, such as sorted.
+         * reordered in some way, such as sorted. But only when it's not during extensive change.
          * @param node The node whose children are being reordered.
          */
-        void bookmarkNodeChildrenReordered(BookmarkItem node);
-
-        /**
-         * Invoked before an extensive set of model changes is about to begin.  This tells UI
-         * intensive observers to wait until the updates finish to update themselves. These methods
-         * should only be used for imports and sync. Observers should still respond to
-         * BookmarkNodeRemoved immediately, to avoid holding onto stale node references.
-         */
-        void extensiveBookmarkChangesBeginning();
-
-        /**
-         * Invoked after an extensive set of model changes has ended.  This tells observers to
-         * update themselves if they were waiting for the update to finish.
-         */
-        void extensiveBookmarkChangesEnded();
-
-        /**
-         *  Called when there are changes to the bookmark model that don't trigger any of the other
-         *  callback methods. For example, this is called when partner bookmarks change.
-         */
-        void bookmarkModelChanged();
+        public void bookmarkNodeChildrenReorderedNotDuringExtensiveChange(BookmarkItem node) {
+            bookmarkModelChangedNotDuringExtensiveChange();
+        }
 
         /**
          * Called when the native side of bookmark is loaded and now in usable state.
          */
-        void bookmarkModelLoaded();
+        public void bookmarkModelLoaded() {
+            bookmarkModelChangedNotDuringExtensiveChange();
+        }
+
+        /**
+         *  Called when there are changes to the bookmark model that don't trigger any of the other
+         *  callback methods or it wasn't handled by other callback methods. Also note that this

[Ted C, 2014/09/10 21:57:39]

This behavior is odd to me.  I fear that this will introduce multiple calls
because it requires people to specifically not call super.<whatever> because
that will end up calling this.

Do we really need this everything funnels into a single place behavior?  Is it
really that common of a pattern?  I think in most places people should only be
acting on a subset of these functions so this seems like unnecessary overkill. 
In general, you'd rather have updates be very targeted and not just bulk
updates.

[Kibeom Kim (inactive), 2014/09/10 22:39:16]

I think it should be a common pattern. First, we cannot avoid implementing very
general bookmarkModelChanged() event, because there are events not covered by
specific ones(e.g. partner update), and also now bulk-update. So I made it
abstract.

After that, all the other methods are optional to implement for more
optimization. But since bookmark changes won't happen that frequently, it's not
high-priority to optimize each specific path, and I've found myself funneling
all the events to one function repeatedly. (e.g., EnhancedBookmarkTitleBar)

One more potential advantage for funneling is when we add a new event. Say, if
we are adding bookmarkImageChanged() event. Then even if listeners don't handle
that particular case, it will function correctly by funneling.

Though, I agree that not preventing super.<whatever> is not good, and this is a
trade-off.

+         *  isn't called during extensive change.
+         *  Examples:
+         *  - On partner bookmarks change.
+         *  - On extensive change finished.
+         *  - Falling back from other methods that are not overridden in this class.
+         */
+        public abstract void bookmarkModelChangedNotDuringExtensiveChange();
     }
 
     /**
      * Handler to fetch the bookmarks, titles, urls and folder hierarchy.
      * @param profile Profile instance corresponding to the active profile.
      */
     public BookmarksBridge(Profile profile) {
         mProfile = profile;
         mNativeBookmarksBridge = nativeInit(profile);

[Ted C, 2014/09/10 21:57:39]

I think you should expose a private native call to
BookmarkModel::IsDoingExtensiveChanges and set
mExtensiveBookmarkChangesHappening equal to that to ensure your logic in the
rest of the file is sane.

[Kibeom Kim (inactive), 2014/09/10 22:39:16]

Done. (good point!)

     }
 
     /**
      * Destroys this instance so no further calls can be executed.
      */
     public void destroy() {
         if (mNativeBookmarksBridge != 0) {
             nativeDestroy(mNativeBookmarksBridge);
             mNativeBookmarksBridge = 0;
             mIsNativeBookmarkModelLoaded = false;
@@ skipping 230 matching lines @@
     }
 
     @CalledByNative
     private void bookmarkModelDeleted() {
         destroy();
     }
 
     @CalledByNative
     private void bookmarkNodeMoved(
             BookmarkItem oldParent, int oldIndex, BookmarkItem newParent, int newIndex) {
-        for (BookmarkModelObserver observer : mObservers) {
-            observer.bookmarkNodeMoved(oldParent, oldIndex, newParent, newIndex);
+        if (!mExtensiveBookmarkChangesHappening) {

[Ted C, 2014/09/10 21:57:40]

for all these i would early exit

if (mExtensiveBookmarkChangesHappening) return;

[Kibeom Kim (inactive), 2014/09/10 22:39:16]

Done.

+            for (BookmarkModelObserver observer : mObservers) {
+                observer.bookmarkNodeMovedNotDuringExtensiveChange(oldParent, oldIndex, newParent,
+                        newIndex);
+            }
         }
     }
 
     @CalledByNative
     private void bookmarkNodeAdded(BookmarkItem parent, int index) {
-        for (BookmarkModelObserver observer : mObservers) {
-            observer.bookmarkNodeAdded(parent, index);
+        if (!mExtensiveBookmarkChangesHappening) {
+            for (BookmarkModelObserver observer : mObservers) {
+                observer.bookmarkNodeAddedNotDuringExtensiveChange(parent, index);
+            }
         }
     }
 
     @CalledByNative
     private void bookmarkNodeRemoved(BookmarkItem parent, int oldIndex, BookmarkItem node) {
         for (BookmarkModelObserver observer : mObservers) {
-            observer.bookmarkNodeRemoved(parent, oldIndex, node);
+            observer.bookmarkNodeRemoved(parent, oldIndex, node,
+                    mExtensiveBookmarkChangesHappening);
         }
     }
 
     @CalledByNative
     private void bookmarkNodeChanged(BookmarkItem node) {
-        for (BookmarkModelObserver observer : mObservers) {
-            observer.bookmarkNodeChanged(node);
+        if (!mExtensiveBookmarkChangesHappening) {
+            for (BookmarkModelObserver observer : mObservers) {
+                observer.bookmarkNodeChangedNotDuringExtensiveChange(node);
+            }
         }
     }
 
     @CalledByNative
     private void bookmarkNodeChildrenReordered(BookmarkItem node) {
-        for (BookmarkModelObserver observer : mObservers) {
-            observer.bookmarkNodeChildrenReordered(node);
+        if (!mExtensiveBookmarkChangesHappening) {
+            for (BookmarkModelObserver observer : mObservers) {
+                observer.bookmarkNodeChildrenReorderedNotDuringExtensiveChange(node);
+            }
         }
     }
 
     @CalledByNative
     private void extensiveBookmarkChangesBeginning() {
-        for (BookmarkModelObserver observer : mObservers) {
-            observer.extensiveBookmarkChangesBeginning();
-        }
+        mExtensiveBookmarkChangesHappening = true;
     }
 
     @CalledByNative
     private void extensiveBookmarkChangesEnded() {
-        for (BookmarkModelObserver observer : mObservers) {
-            observer.extensiveBookmarkChangesEnded();
-        }
+        mExtensiveBookmarkChangesHappening = false;
+        bookmarkModelChanged();
     }
 
     @CalledByNative
     private void bookmarkModelChanged() {
-        for (BookmarkModelObserver observer : mObservers) {
-            observer.bookmarkModelChanged();
+        if (!mExtensiveBookmarkChangesHappening) {
+            for (BookmarkModelObserver observer : mObservers) {
+                observer.bookmarkModelChangedNotDuringExtensiveChange();
+            }
         }
     }
 
     @CalledByNative
     private static BookmarkItem createBookmarkItem(long id, int type, String title, String url,
             boolean isFolder, long parentId, int parentIdType, boolean isEditable,
             boolean isManaged) {
         return new BookmarkItem(new BookmarkId(id, type), title, url, isFolder,
                 new BookmarkId(parentId, parentIdType), isEditable, isManaged);
     }
@@ skipping 139 matching lines @@
                 case GET_CURRENT_FOLDER_HIERARCHY:
                     mHandler.getCurrentFolderHierarchy(mFolderId, mCallback);
                     break;
                 default:
                     assert false;
                     break;
             }
         }
     }
 }