[Android] Make BookmarkModelObserver more convenient.

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

Index: chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java b/chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java
index 15a18f5ee2bbc377643ff60ec0eacb5a9ceb4e2c..2e1b490c37af073bd88a8bb30e76d6600d2d4345 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java
@@ -18,6 +18,7 @@ import java.util.List;
  */
 public class BookmarksBridge {
     private final Profile mProfile;
+    private boolean mExtensiveBookmarkChangesHappening;
     private long mNativeBookmarksBridge;
     private boolean mIsNativeBookmarkModelLoaded;
     private final List<DelayedBookmarkCallback> mDelayedBookmarkCallbacks =
@@ -48,71 +49,95 @@ public class BookmarksBridge {
     }
 
     /**
-     * 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();
+        public void bookmarkNodeChildrenReorderedNotDuringExtensiveChange(BookmarkItem node) {
+            bookmarkModelChangedNotDuringExtensiveChange();
+        }
 
         /**
-         * 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.
+         * Called when the native side of bookmark is loaded and now in usable state.
          */
-        void extensiveBookmarkChangesEnded();
+        public void bookmarkModelLoaded() {
+            bookmarkModelChangedNotDuringExtensiveChange();
+        }
 
         /**
          *  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.
+         *  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.
          */
-        void bookmarkModelChanged();
-
-        /**
-         * Called when the native side of bookmark is loaded and now in usable state.
-         */
-        void bookmarkModelLoaded();
+        public abstract void bookmarkModelChangedNotDuringExtensiveChange();
     }
 
     /**
@@ -372,57 +397,66 @@ public class BookmarksBridge {
     @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();
+            }
         }
     }