Observer Support for DistilledPagePrefs

chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java

Index: chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java
diff --git a/chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java b/chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java
index 60e87b154f6646cd6d780cdbaf3964edb601ef90..85ce6884bba4aa02a6b37f652da5469190de552a 100644
--- a/chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java
+++ b/chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java
@@ -7,36 +7,108 @@ package org.chromium.chrome.browser.dom_distiller;
 import android.test.suitebuilder.annotation.SmallTest;
 import android.test.UiThreadTest;
 
+import org.chromium.base.ThreadUtils;
 import org.chromium.base.test.util.Feature;
 import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.chrome.shell.ChromeShellTestBase;
 import org.chromium.components.dom_distiller.core.DistilledPagePrefs;
 import org.chromium.components.dom_distiller.core.DomDistillerService;
 import org.chromium.components.dom_distiller.core.Theme;
+import org.chromium.content.browser.test.util.UiUtils;
 
 /**
  * Test class for {@link DistilledPagePrefs}.
  */
 public class DistilledPagePrefsTest extends ChromeShellTestBase {
 
+    private DistilledPagePrefs mDistilledPagePrefs;
+
+    @Override
+    public void setUp() throws Exception {
+        super.setUp();
+        startChromeBrowserProcessSync(getInstrumentation().getTargetContext());
+        getDistilledPagePrefs();
+    }
+
+    private void getDistilledPagePrefs() {
+        ThreadUtils.runOnUiThreadBlocking(new Runnable() {
+            public void run() {
+                DomDistillerService domDistillerService = DomDistillerServiceFactory.
+                        getForProfile(Profile.getLastUsedProfile());
+                mDistilledPagePrefs = domDistillerService.getDistilledPagePrefs();
+            }
+        });
+    }
+
     @SmallTest
     @UiThreadTest
     @Feature({"DomDistiller"})
     public void testGetAndSetPrefs() throws InterruptedException {
-        startChromeBrowserProcessSync(getInstrumentation().getTargetContext());
-        DomDistillerService service = DomDistillerServiceFactory.
-            getForProfile(Profile.getLastUsedProfile());
-        assertNotNull(service);
-        DistilledPagePrefs distilledPagePrefs = service.getDistilledPagePrefs();
-        assertNotNull(distilledPagePrefs);
-        // Check default theme.
-        assertEquals(distilledPagePrefs.getTheme(), Theme.LIGHT);
+        // Check the default theme.
+        assertEquals(mDistilledPagePrefs.getTheme(), Theme.LIGHT);
         // Check that theme can be correctly set.
-        distilledPagePrefs.setTheme(Theme.DARK);
-        assertEquals(Theme.DARK, distilledPagePrefs.getTheme());
-        distilledPagePrefs.setTheme(Theme.LIGHT);
-        assertEquals(Theme.LIGHT, distilledPagePrefs.getTheme());
-        distilledPagePrefs.setTheme(Theme.SEPIA);
-        assertEquals(Theme.SEPIA, distilledPagePrefs.getTheme());
+        mDistilledPagePrefs.setTheme(Theme.DARK);
+        assertEquals(Theme.DARK, mDistilledPagePrefs.getTheme());
+        mDistilledPagePrefs.setTheme(Theme.LIGHT);
+        assertEquals(Theme.LIGHT, mDistilledPagePrefs.getTheme());
+        mDistilledPagePrefs.setTheme(Theme.SEPIA);
+        assertEquals(Theme.SEPIA, mDistilledPagePrefs.getTheme());
+    }
+
+    public void setTheme(final Theme theme) {

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

private. Also, move to the bottom of the class.

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

Done.

+        ThreadUtils.runOnUiThreadBlocking(new Runnable() {
+            public void run() {
+                mDistilledPagePrefs.setTheme(theme);
+            }
+        });
+    }
+
+    @SmallTest
+    @Feature({"DomDistiller"})
+    public void testSingleObserver() throws InterruptedException {
+        final TestingObserver testObserver = new TestingObserver(mDistilledPagePrefs);

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

Nit: Unnecessary final.

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

Done.

+        mDistilledPagePrefs.addObserver(testObserver);
+        setTheme(Theme.DARK);
+        UiUtils.settleDownUI(getInstrumentation());

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

If you want you could verify that testObserver.getTheme() returns null before
settling down the UI in this test. You don't have to do that in the other test
though. If you do it here, add a comment that you are assuming that the callback
doesn't happen immediately.

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

Done.

On 2014/07/22 22:51:36, nyquist wrote:

+        // Check that testObserver's theme has been updated,
+        assertEquals(Theme.DARK, testObserver.getTheme());
+        mDistilledPagePrefs.removeObserver(testObserver);
+    }
+
+    @SmallTest
+    @Feature({"DomDistiller"})
+    public void testMultipleObservers() throws InterruptedException {
+        final TestingObserver testObserverOne = new TestingObserver(mDistilledPagePrefs);

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

Nit: unnecessary final here and below.

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

Done.

+        mDistilledPagePrefs.addObserver(testObserverOne);
+        final TestingObserver testObserverTwo = new TestingObserver(mDistilledPagePrefs);
+        mDistilledPagePrefs.addObserver(testObserverTwo);
+        setTheme(Theme.SEPIA);
+        UiUtils.settleDownUI(getInstrumentation());
+        assertEquals(Theme.SEPIA, testObserverOne.getTheme());
+        assertEquals(Theme.SEPIA, testObserverTwo.getTheme());
+        mDistilledPagePrefs.removeObserver(testObserverOne);
+        setTheme(Theme.DARK);
+        UiUtils.settleDownUI(getInstrumentation());
+        // Check that testObserverOne's theme is not changed but testObserverTwo's

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

Nit: one line

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

Done.

+        // is.
+        assertEquals(Theme.SEPIA, testObserverOne.getTheme());
+        assertEquals(Theme.DARK, testObserverTwo.getTheme());
+        mDistilledPagePrefs.removeObserver(testObserverTwo);
+    }
+
+    private class TestingObserver implements DistilledPagePrefs.Observer {
+        private Theme mTheme;
+
+        public TestingObserver(DistilledPagePrefs distilledPagePrefs) {

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

I think you can remove the argument here. You don't use getTheme() until you
have called setTheme and waited for the UI to settle down. If anything, you
could check if getTheme() returns null.

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

Done.

+            mTheme = distilledPagePrefs.getTheme();
+        }
+
+        public Theme getTheme() {
+            return mTheme;
+        }
+
+        public void onChangeTheme(Theme theme) {
+            mTheme = theme;
+        }
     }
 }