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..48a3bea8df1506ade45072c99a9c0155a7029e4c 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,113 @@ 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}.
+ * Test class for {@link mDistilledPagePrefs}.

[robliao, 2014/07/21 21:05:09]

This should be DistilledPagePrefs.

[sunangel, 2014/07/21 22:43:55]

Done.

  */
 public class DistilledPagePrefsTest extends ChromeShellTestBase {
 
+    private volatile DistilledPagePrefs mDistilledPagePrefs;

[robliao, 2014/07/21 21:05:08]

Does this need to be volatile?

[sunangel, 2014/07/21 22:43:54]

I think it does because we switch between off and on the UiThread in testing
single and multiple observers.
On 2014/07/21 21:05:08, robliao wrote:

[robliao, 2014/07/21 23:08:36]

The value of this variable isn't being modified. volatile helps protect against
two or more threads writing to this variable. Since this is set during setup,
this modifier isn't necessary.

On 2014/07/21 22:43:54, sunangel wrote:

[sunangel, 2014/07/22 01:32:38]

Done.

+
+    @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());
+                assertNotNull(domDistillerService);

[robliao, 2014/07/21 21:05:08]

No need to assert not null here. You'll crash at the next line if it is.

[sunangel, 2014/07/21 22:43:55]

I also took out the assertNotNull for DistilledPagePrefs
because it is used after it is made for each test and
similarly, it will crash if not null. Let me know if that is
not okay.
On 2014/07/21 21:05:08, robliao wrote:

+                mDistilledPagePrefs = domDistillerService.getDistilledPagePrefs();
+                assertNotNull(mDistilledPagePrefs);
+            }
+        });
+    }
+
     @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) {
+        ThreadUtils.runOnUiThreadBlocking(new Runnable() {
+            public void run() {
+                mDistilledPagePrefs.setTheme(theme);
+            }
+        });
+    }
+
+    @SmallTest
+    @Feature({"DomDistiller"})
+    public void testSingleObserver() throws InterruptedException {
+        final TestingObserver testObserver = new TestingObserver(mDistilledPagePrefs);
+        assertNotNull(testObserver);

[robliao, 2014/07/21 21:05:09]

Java "new" should never return null, so it's not necessary to test for null.
You'll get an OutOfMemoryError instead.

[sunangel, 2014/07/21 22:43:55]

Done.

+        mDistilledPagePrefs.addObserver(testObserver);
+        setTheme(Theme.DARK);
+        UiUtils.settleDownUI(getInstrumentation());
+        // 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 testObserver = new TestingObserver(mDistilledPagePrefs);
+        assertNotNull(testObserver);
+        mDistilledPagePrefs.addObserver(testObserver);
+        final TestingObserver testObserverTwo = new TestingObserver(mDistilledPagePrefs);

[robliao, 2014/07/21 21:05:09]

testObserver2 is also okay. You can also call the first observer testObserver1
to make it easy to distinguish between the two.

[sunangel, 2014/07/21 22:43:54]

Done.

+        assertNotNull(testObserverTwo);
+        mDistilledPagePrefs.addObserver(testObserverTwo);
+        setTheme(Theme.SEPIA);
+        UiUtils.settleDownUI(getInstrumentation());
+        assertEquals(Theme.SEPIA, testObserver.getTheme());
+        assertEquals(Theme.SEPIA, testObserverTwo.getTheme());
+        mDistilledPagePrefs.removeObserver(testObserver);
+        setTheme(Theme.DARK);
+        UiUtils.settleDownUI(getInstrumentation());
+        // Check that testObserver's theme is not changed but testObserverTwo's
+        // is.
+        assertEquals(Theme.SEPIA, testObserver.getTheme());
+        assertEquals(Theme.DARK, testObserverTwo.getTheme());
+        mDistilledPagePrefs.removeObserver(testObserverTwo);
+    }
+
+    private class TestingObserver implements DistilledPagePrefs.Observer {
+        private volatile Theme mTheme;
+
+        public TestingObserver(DistilledPagePrefs distilledPagePrefs) {
+            mTheme = distilledPagePrefs.getTheme();
+        }
+
+        public Theme getTheme() {
+            return mTheme;
+        }
+
+        public void onChangeTheme(Theme theme) {
+            mTheme = theme;
+        }
     }
 }