Observer Support for DistilledPagePrefs

components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java

Index: components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java
diff --git a/components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java b/components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java
index 3ec91baca8c91ad490fec48100bb72e9609e96e9..6ee7489fad7f0e80d78d3e3e6137dfbf13752b8c 100644
--- a/components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java
+++ b/components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java
@@ -6,6 +6,8 @@ package org.chromium.components.dom_distiller.core;
 
 import org.chromium.base.JNINamespace;
 
+import java.util.HashMap;
+
 /**
  * Wrapper for the dom_distiller::DistilledPagePrefs.
  */
@@ -13,25 +15,51 @@ import org.chromium.base.JNINamespace;
 public final class DistilledPagePrefs {
 
     private final long mDistilledPagePrefsAndroid;
+    private HashMap<Observer, DistilledPagePrefsObserverWrapper> observerMap;

[nyquist, 2014/07/22 22:51:36]

private final and also member names should start with m.

[sunangel, 2014/07/23 16:22:03]

Done.

+
+    /**
+     * Observer interface for observing DistilledPagePrefs changes.
+     */
+    public interface Observer {
+        void onChangeTheme(Theme theme);
+    }
 
     DistilledPagePrefs(long distilledPagePrefsPtr) {
         mDistilledPagePrefsAndroid = nativeInit(distilledPagePrefsPtr);
+        observerMap = new HashMap<Observer, DistilledPagePrefsObserverWrapper>();
+    }
+
+    public void addObserver(Observer obs) {
+        DistilledPagePrefsObserverWrapper wrappedObserver =
+                new DistilledPagePrefsObserverWrapper(obs);
+        nativeAddObserver(mDistilledPagePrefsAndroid, wrappedObserver.getNativePtr());
+        observerMap.put(obs, wrappedObserver);
+    }
+
+    public void removeObserver(Observer obs) {
+        long nativeObserverPtr = observerMap.get(obs).getNativePtr();

[nyquist, 2014/07/22 22:51:37]

What happens if I call this method with

prefs.removeObserver(new Observer() {...})
?

I think you might need to null-check the result.

Also, map.remove(key) returns the observer if it exists.

[sunangel, 2014/07/23 16:22:03]

Done.

+        observerMap.remove(obs);
+        nativeRemoveObserver(mDistilledPagePrefsAndroid, nativeObserverPtr);

[nyquist, 2014/07/22 22:51:36]

This seems to be leaking the native object.

[sunangel, 2014/07/23 16:22:03]

Done.

     }
 
-    // TODO(sunangel): Add observer support from this Java class to native
-    // counterpart so UI can be updated across tabs.
     public void setTheme(Theme theme) {
         nativeSetTheme(mDistilledPagePrefsAndroid, theme.asNativeEnum());
     }
 
     public Theme getTheme() {
         return Theme.getThemeForValue(
-            nativeGetTheme(mDistilledPagePrefsAndroid));
+                nativeGetTheme(mDistilledPagePrefsAndroid));

[nyquist, 2014/07/22 22:51:37]

Nit: I think this can be on one line. Remember 100 chars limit for Java.

[sunangel, 2014/07/23 16:22:03]

Done.

     }
 
     private native long nativeInit(long distilledPagePrefPtr);
-    private native void nativeSetTheme(long nativeDistilledPagePrefsAndroid,
-        int theme);
-    private native int nativeGetTheme(
-        long nativeDistilledPagePrefsAndroid);
+
+    private native void nativeSetTheme(long nativeDistilledPagePrefsAndroid, int theme);
+
+    private native int nativeGetTheme(long nativeDistilledPagePrefsAndroid);
+
+    private native void nativeAddObserver(long nativeDistilledPagePrefsAndroid,
+            long nativeObserverPtr);
+
+    private native void nativeRemoveObserver(long nativeDistilledPagePrefsAndroid,
+            long nativeObserverPtr);
 }