ObserverList add/remove methods should return a boolean.

BUG=347557

Review URL: https://codereview.chromium.org/182623003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@255902 0039d316-1c4b-4281-b951-d872f2087c98

base/android/java/src/org/chromium/base/ObserverList.java

@@ -53,27 +53,30 @@ public class ObserverList<E> implements Iterable<E> {
      * <p/>
      * An observer should not be added to the same list more than once. If an iteration is already
      * in progress, this observer will be not be visible during that iteration.
+     *
+     * @return true if the observer list changed as a result of the call.
      */
-    public void addObserver(E obs) {
+    public boolean addObserver(E obs) {
         // Avoid adding null elements to the list as they may be removed on a compaction.
         if (obs == null || mObservers.contains(obs)) {
-            assert false;
-            return;
+            return false;
         }
 
         // Structurally modifying the underlying list here. This means we
         // cannot use the underlying list's iterator to iterate over the list.
-        mObservers.add(obs);
+        return mObservers.add(obs);
     }
 
     /**
      * Remove an observer from the list if it is in the list.
+     *
+     * @return true if an element was removed as a result of this call.
      */
-    public void removeObserver(E obs) {
+    public boolean removeObserver(E obs) {
         int index = mObservers.indexOf(obs);
 
         if (index == -1)
-            return;
+            return false;
 
         if (mIterationDepth == 0) {
             // No one is iterating over the list.
@@ -81,6 +84,8 @@ public class ObserverList<E> implements Iterable<E> {
         } else {
             mObservers.set(index, null);
         }
+
+        return true;
     }
 
     public boolean hasObserver(E obs) {

base/android/javatests/src/org/chromium/base/ObserverListTest.java

@@ -215,4 +215,35 @@ public class ObserverListTest extends InstrumentationTestCase {
         assertTrue(it.hasNext());
         assertTrue(15 == it.next());
     }
+
+    @SmallTest
+    @Feature({"Android-AppBase"})
+    public void testAddObserverReturnValue() {
+        ObserverList<Object> observerList = new ObserverList<Object>();
+
+        Object a = new Object();
+        assertTrue(observerList.addObserver(a));
+        assertFalse(observerList.addObserver(a));
+
+        Object b = new Object();
+        assertTrue(observerList.addObserver(b));
+        assertFalse(observerList.addObserver(null));
+    }
+
+    @SmallTest
+    @Feature({"Android-AppBase"})
+    public void testRemoveObserverReturnValue() {
+        ObserverList<Object> observerList = new ObserverList<Object>();
+
+        Object a = new Object();
+        Object b = new Object();
+        observerList.addObserver(a);
+        observerList.addObserver(b);
+
+        assertTrue(observerList.removeObserver(a));
+        assertFalse(observerList.removeObserver(a));
+        assertFalse(observerList.removeObserver(new Object()));
+        assertTrue(observerList.removeObserver(b));
+        assertFalse(observerList.removeObserver(null));
+    }
 }

chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java

@@ -173,8 +173,8 @@ public class TemplateUrlService {
      */
     public void registerLoadListener(LoadListener listener) {
         ThreadUtils.assertOnUiThread();
-        assert !mLoadListeners.hasObserver(listener);
-        mLoadListeners.addObserver(listener);
+        boolean added = mLoadListeners.addObserver(listener);
+        assert added;
     }
 
     /**
@@ -183,8 +183,8 @@ public class TemplateUrlService {
      */
     public void unregisterLoadListener(LoadListener listener) {
         ThreadUtils.assertOnUiThread();
-        assert (mLoadListeners.hasObserver(listener));
-        mLoadListeners.removeObserver(listener);
+        boolean removed = mLoadListeners.removeObserver(listener);
+        assert removed;
     }
 
     /**

content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java

@@ -10,6 +10,7 @@ import android.content.res.Configuration;
 import android.hardware.display.DisplayManager;
 import android.hardware.display.DisplayManager.DisplayListener;
 import android.os.Build;
+import android.util.Log;
 import android.view.Surface;
 import android.view.WindowManager;
 
@@ -192,12 +193,10 @@ class ScreenOrientationListener {
         assert mAppContext == context.getApplicationContext();
         assert mAppContext != null;
 
-        // TODO(mlamouri): we should check if the observer was really added,
-        // http://crbug.com/347557
-        if (mObservers.hasObserver(observer)) {
+        if (!mObservers.addObserver(observer)) {
+            Log.w(TAG, "Adding an observer that is already present!");
             return;
         }
-        mObservers.addObserver(observer);
         mObserverCount++;
 
         // If we got our first observer, we should start listening.
@@ -224,12 +223,10 @@ class ScreenOrientationListener {
      * @param observer The observer that will no longer receive notification.
      */
     public void removeObserver(ScreenOrientationObserver observer) {
-        // TODO(mlamouri): we should check if the observer was really removed,
-        // http://crbug.com/347557
-        if (!mObservers.hasObserver(observer)) {
+        if (!mObservers.removeObserver(observer)) {
+            Log.w(TAG, "Removing an inexistent observer!");
             return;
         }
-        mObservers.removeObserver(observer);
         mObserverCount--;
 
         if (mObserverCount == 0) {

net/android/java/src/org/chromium/net/NetworkChangeNotifier.java

@@ -185,9 +185,7 @@ public class NetworkChangeNotifier {
     }
 
     private void addConnectionTypeObserverInternal(ConnectionTypeObserver observer) {
-        if (!mConnectionTypeObservers.hasObserver(observer)) {
-            mConnectionTypeObservers.addObserver(observer);
-        }
+        mConnectionTypeObservers.addObserver(observer);
     }
 
     /**