ObserverList add/remove methods should return a boolean.

ObserverList add/remove methods should return a boolean.

BUG=347557
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255902

[mlamouri (slow - plz ping), 2014-03-03 14:40:32]

ScreenOrientationListener.java is in a CL that has not landed yet, see
https://codereview.chromium.org/179783003/

[Yaron, 2014-03-04 04:45:39]

https://codereview.chromium.org/182623003/diff/1/base/android/java/src/org/ch...
File base/android/java/src/org/chromium/base/ObserverList.java (right):

https://codereview.chromium.org/182623003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ObserverList.java:83:
mObservers.remove(obs);
Hmm. Unrelated but I think it's more efficient to pass |index| instead of |obs|.

https://codereview.chromium.org/182623003/diff/1/base/android/javatests/src/o...
File base/android/javatests/src/org/chromium/base/ObserverListTest.java (right):

https://codereview.chromium.org/182623003/diff/1/base/android/javatests/src/o...
base/android/javatests/src/org/chromium/base/ObserverListTest.java:230: 
Nit: drop ws

https://codereview.chromium.org/182623003/diff/1/base/android/javatests/src/o...
base/android/javatests/src/org/chromium/base/ObserverListTest.java:233:
observerList.clear();
why include this?

https://codereview.chromium.org/182623003/diff/1/base/android/javatests/src/o...
base/android/javatests/src/org/chromium/base/ObserverListTest.java:252:
observerList.clear();
remove

[mlamouri (slow - plz ping), 2014-03-05 15:58:10]

https://codereview.chromium.org/182623003/diff/1/base/android/java/src/org/ch...
File base/android/java/src/org/chromium/base/ObserverList.java (right):

https://codereview.chromium.org/182623003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ObserverList.java:83:
mObservers.remove(obs);
On 2014/03/04 04:45:40, Yaron wrote:
> Hmm. Unrelated but I think it's more efficient to pass |index| instead of
|obs|.

I think this class tries to hide that information somehow given that you don't
have an object at every index. There could be another remove() function taking
an index if needed. Though, this is unrelated as you pointed ;)

https://codereview.chromium.org/182623003/diff/1/base/android/javatests/src/o...
File base/android/javatests/src/org/chromium/base/ObserverListTest.java (right):

https://codereview.chromium.org/182623003/diff/1/base/android/javatests/src/o...
base/android/javatests/src/org/chromium/base/ObserverListTest.java:230: 
On 2014/03/04 04:45:40, Yaron wrote:
> Nit: drop ws

Done.

https://codereview.chromium.org/182623003/diff/1/base/android/javatests/src/o...
base/android/javatests/src/org/chromium/base/ObserverListTest.java:233:
observerList.clear();
On 2014/03/04 04:45:40, Yaron wrote:
> why include this?

Done.

https://codereview.chromium.org/182623003/diff/1/base/android/javatests/src/o...
base/android/javatests/src/org/chromium/base/ObserverListTest.java:252:
observerList.clear();
On 2014/03/04 04:45:40, Yaron wrote:
> remove

Done.

[Yaron, 2014-03-05 19:29:00]

lgtm since change mentioend is orthogonal

https://codereview.chromium.org/182623003/diff/1/base/android/java/src/org/ch...
File base/android/java/src/org/chromium/base/ObserverList.java (right):

https://codereview.chromium.org/182623003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ObserverList.java:83:
mObservers.remove(obs);
On 2014/03/05 15:58:10, Mounir Lamouri wrote:
> On 2014/03/04 04:45:40, Yaron wrote:
> > Hmm. Unrelated but I think it's more efficient to pass |index| instead of
> |obs|.
> 
> I think this class tries to hide that information somehow given that you don't
> have an object at every index. There could be another remove() function taking
> an index if needed. Though, this is unrelated as you pointed ;)

Err, I don't think I explained myself clearly

I didn't mean to change the signature of ObserverList. I meant that its internal
usage of |mObservers| is slightly inefficient. Since we've already done the
indexOf based on |obs| on L76, this call to |remove| will do the same scan and
comparisons, rather than finding just operating on the same index

[mlamouri (slow - plz ping), 2014-03-09 16:56:57]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-03-09 16:57:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/182623003/60001

[commit-bot: I haz the power, 2014-03-09 16:57:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-09 16:57:15]

Failed to apply patch for
content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java
  Hunk #1 FAILED at 7.
  Hunk #2 FAILED at 185.
  Hunk #3 succeeded at 224 (offset 7 lines).
  2 out of 3 hunks FAILED -- saving rejects to file
content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java.rej

Patch:      
content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java
Index:
content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java
diff --git
a/content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java
b/content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java
index
5d19854e4a5b4990d85d93d074ce388d4038ea67..c67a2e8278ca3c1739d1662cd48301aabcea6735
100644
---
a/content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java
+++
b/content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java
@@ -7,11 +7,10 @@ package org.chromium.content.browser;
 import android.content.ComponentCallbacks;
 import android.content.Context;
 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;
 
@@ -185,12 +184,10 @@ class ScreenOrientationListener {
      * @param context The context associated with this observer.
      */
     public void addObserver(ScreenOrientationObserver observer) {
-        // 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.
@@ -217,12 +214,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) {

[mlamouri (slow - plz ping), 2014-03-09 17:16:00]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-03-09 17:16:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/182623003/80001

[commit-bot: I haz the power, 2014-03-10 07:23:56]

Change committed as 255902