Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
403323005:
Observer Support for DistilledPagePrefs (Closed)
|
Created:
6 years, 5 months ago by sunangel Modified:
6 years, 5 months ago CC:
chromium-reviews, smaslo Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionObserver Support for DistilledPagePrefs
These changes provide support for observers for DistilledPagePrefs
to listen to changes in DistilledPagePrefs.
BUG=383630
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285109
Patch Set 1 : Added removeObserver after test is done, styling #
Total comments: 22
Patch Set 2 : Style changes, private, order of declarations changed #
Total comments: 2
Patch Set 3 : remove volatile keyword, spacing #
Total comments: 4
Patch Set 4 : formatting #
Total comments: 44
Patch Set 5 : moved observerwrapper to inner class, renamed observer wrapper, styling, build.gn added #
Total comments: 21
Patch Set 6 : privacy, spacing, added final keyword, made TestingObserver static class to fix trybot fail #
Total comments: 2
Patch Set 7 : Changed observerMap type from HashMap to Map #
Messages
Total messages: 24 (0 generated)
https://codereview.chromium.org/403323005/diff/80001/chrome/android/javatests... File chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java (right): https://codereview.chromium.org/403323005/diff/80001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:20: * Test class for {@link mDistilledPagePrefs}. This should be DistilledPagePrefs. https://codereview.chromium.org/403323005/diff/80001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:24: private volatile DistilledPagePrefs mDistilledPagePrefs; Does this need to be volatile? https://codereview.chromium.org/403323005/diff/80001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:38: assertNotNull(domDistillerService); No need to assert not null here. You'll crash at the next line if it is. https://codereview.chromium.org/403323005/diff/80001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:72: assertNotNull(testObserver); Java "new" should never return null, so it's not necessary to test for null. You'll get an OutOfMemoryError instead. https://codereview.chromium.org/403323005/diff/80001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:87: final TestingObserver testObserverTwo = new TestingObserver(mDistilledPagePrefs); testObserver2 is also okay. You can also call the first observer testObserver1 to make it easy to distinguish between the two. https://codereview.chromium.org/403323005/diff/80001/components/dom_distiller... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java (right): https://codereview.chromium.org/403323005/diff/80001/components/dom_distiller... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:33: DistilledPagePrefsObserverWrapper newObs = new DistilledPagePrefsObserverWrapper(obs); Let's call this wrappedObserver. https://codereview.chromium.org/403323005/diff/80001/components/dom_distiller... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:39: nativeRemoveObserver(mDistilledPagePrefsAndroid, observerMap.get(obs).getNativePtr()); For reverse symmetry, the observerMap should be removed first and the the nativeRemoveObserver afterwards. This simplifies who had access to what at a given time. https://codereview.chromium.org/403323005/diff/80001/components/dom_distiller... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefsObserverWrapper.java (right): https://codereview.chromium.org/403323005/diff/80001/components/dom_distiller... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefsObserverWrapper.java:16: final DistilledPagePrefs.Observer mDistilledPagePrefsObserver; This should probably be private. https://codereview.chromium.org/403323005/diff/80001/components/dom_distiller... File components/dom_distiller/core/distilled_page_prefs_android.h (right): https://codereview.chromium.org/403323005/diff/80001/components/dom_distiller... components/dom_distiller/core/distilled_page_prefs_android.h:26: void AddObserver(JNIEnv* env, jobject obj, jlong obs); AddObserver was first in java land, so it should be first here for consistency. https://codereview.chromium.org/403323005/diff/80001/components/dom_distiller... File components/dom_distiller/core/distilled_page_prefs_observer_wrapper.h (right): https://codereview.chromium.org/403323005/diff/80001/components/dom_distiller... components/dom_distiller/core/distilled_page_prefs_observer_wrapper.h:21: virtual void OnChangeTheme(DistilledPagePrefs::Theme new_theme) OVERRIDE; It's customary to separate the override and add a comment above indicating the originating class for the override. Example: // DistilledPagePrefs::Observer virtual void OnChangeTheme(...
Please see new patch. Thanks! https://codereview.chromium.org/403323005/diff/80001/chrome/android/javatests... File chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java (right): https://codereview.chromium.org/403323005/diff/80001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:20: * Test class for {@link mDistilledPagePrefs}. On 2014/07/21 21:05:09, robliao wrote: > This should be DistilledPagePrefs. Done. https://codereview.chromium.org/403323005/diff/80001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:24: private volatile DistilledPagePrefs mDistilledPagePrefs; 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: > Does this need to be volatile? https://codereview.chromium.org/403323005/diff/80001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:38: assertNotNull(domDistillerService); 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: > No need to assert not null here. You'll crash at the next line if it is. https://codereview.chromium.org/403323005/diff/80001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:72: assertNotNull(testObserver); On 2014/07/21 21:05:09, robliao wrote: > Java "new" should never return null, so it's not necessary to test for null. > You'll get an OutOfMemoryError instead. Done. https://codereview.chromium.org/403323005/diff/80001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:87: final TestingObserver testObserverTwo = new TestingObserver(mDistilledPagePrefs); On 2014/07/21 21:05:09, robliao wrote: > testObserver2 is also okay. You can also call the first observer testObserver1 > to make it easy to distinguish between the two. Done. https://codereview.chromium.org/403323005/diff/80001/components/dom_distiller... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java (right): https://codereview.chromium.org/403323005/diff/80001/components/dom_distiller... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:33: DistilledPagePrefsObserverWrapper newObs = new DistilledPagePrefsObserverWrapper(obs); On 2014/07/21 21:05:09, robliao wrote: > Let's call this wrappedObserver. Done. https://codereview.chromium.org/403323005/diff/80001/components/dom_distiller... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:39: nativeRemoveObserver(mDistilledPagePrefsAndroid, observerMap.get(obs).getNativePtr()); On 2014/07/21 21:05:09, robliao wrote: > For reverse symmetry, the observerMap should be removed first and the the > nativeRemoveObserver afterwards. > This simplifies who had access to what at a given time. Done. https://codereview.chromium.org/403323005/diff/80001/components/dom_distiller... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefsObserverWrapper.java (right): https://codereview.chromium.org/403323005/diff/80001/components/dom_distiller... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefsObserverWrapper.java:16: final DistilledPagePrefs.Observer mDistilledPagePrefsObserver; On 2014/07/21 21:05:09, robliao wrote: > This should probably be private. Done. https://codereview.chromium.org/403323005/diff/80001/components/dom_distiller... File components/dom_distiller/core/distilled_page_prefs_android.h (right): https://codereview.chromium.org/403323005/diff/80001/components/dom_distiller... components/dom_distiller/core/distilled_page_prefs_android.h:26: void AddObserver(JNIEnv* env, jobject obj, jlong obs); On 2014/07/21 21:05:09, robliao wrote: > AddObserver was first in java land, so it should be first here for consistency. Done. https://codereview.chromium.org/403323005/diff/80001/components/dom_distiller... File components/dom_distiller/core/distilled_page_prefs_observer_wrapper.h (right): https://codereview.chromium.org/403323005/diff/80001/components/dom_distiller... components/dom_distiller/core/distilled_page_prefs_observer_wrapper.h:21: virtual void OnChangeTheme(DistilledPagePrefs::Theme new_theme) OVERRIDE; On 2014/07/21 21:05:09, robliao wrote: > It's customary to separate the override and add a comment above indicating the > originating class for the override. > > Example: > // DistilledPagePrefs::Observer > virtual void OnChangeTheme(... Done.
https://codereview.chromium.org/403323005/diff/80001/chrome/android/javatests... File chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java (right): https://codereview.chromium.org/403323005/diff/80001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:24: private volatile DistilledPagePrefs mDistilledPagePrefs; 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: > 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: > > Does this need to be volatile? > https://codereview.chromium.org/403323005/diff/120001/components/dom_distille... File components/dom_distiller/core/distilled_page_prefs_observer_wrapper.h (right): https://codereview.chromium.org/403323005/diff/120001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_observer_wrapper.h:21: // DistilledPagePrefs::Observer Add one linebreak above this line.
See new patch.
Sorry for the double spam, forgot to press done last time. https://codereview.chromium.org/403323005/diff/80001/chrome/android/javatests... File chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java (right): https://codereview.chromium.org/403323005/diff/80001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:24: private volatile DistilledPagePrefs mDistilledPagePrefs; On 2014/07/21 23:08:36, robliao wrote: > 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: > > 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: > > > Does this need to be volatile? > > > Done. https://codereview.chromium.org/403323005/diff/120001/components/dom_distille... File components/dom_distiller/core/distilled_page_prefs_observer_wrapper.h (right): https://codereview.chromium.org/403323005/diff/120001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_observer_wrapper.h:21: // DistilledPagePrefs::Observer On 2014/07/21 23:08:36, robliao wrote: > Add one linebreak above this line. Done.
A few more nits. Feel free to loop in nyquist@. https://codereview.chromium.org/403323005/diff/140001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java (right): https://codereview.chromium.org/403323005/diff/140001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:100: private volatile Theme mTheme; Since only one thread is ever active, this doesn't need to be volatile. https://codereview.chromium.org/403323005/diff/140001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java (right): https://codereview.chromium.org/403323005/diff/140001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:33: DistilledPagePrefsObserverWrapper wrappedObserver = new The new can go on the next line. (Java style: Break at higher semantic level https://google-styleguide.googlecode.com/svn/trunk/javaguide.html#s4.5.1-line...)
See new patch. https://codereview.chromium.org/403323005/diff/140001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java (right): https://codereview.chromium.org/403323005/diff/140001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:100: private volatile Theme mTheme; On 2014/07/22 17:43:54, robliao wrote: > Since only one thread is ever active, this doesn't need to be volatile. Done. https://codereview.chromium.org/403323005/diff/140001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java (right): https://codereview.chromium.org/403323005/diff/140001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:33: DistilledPagePrefsObserverWrapper wrappedObserver = new On 2014/07/22 17:43:54, robliao wrote: > The new can go on the next line. (Java style: Break at higher semantic level > https://google-styleguide.googlecode.com/svn/trunk/javaguide.html#s4.5.1-line...) Done.
lgtm
https://codereview.chromium.org/403323005/diff/160001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java (right): https://codereview.chromium.org/403323005/diff/160001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:58: public void setTheme(final Theme theme) { private. Also, move to the bottom of the class. https://codereview.chromium.org/403323005/diff/160001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:69: final TestingObserver testObserver = new TestingObserver(mDistilledPagePrefs); Nit: Unnecessary final. https://codereview.chromium.org/403323005/diff/160001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:72: UiUtils.settleDownUI(getInstrumentation()); 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. https://codereview.chromium.org/403323005/diff/160001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:81: final TestingObserver testObserverOne = new TestingObserver(mDistilledPagePrefs); Nit: unnecessary final here and below. https://codereview.chromium.org/403323005/diff/160001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:92: // Check that testObserverOne's theme is not changed but testObserverTwo's Nit: one line https://codereview.chromium.org/403323005/diff/160001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:102: public TestingObserver(DistilledPagePrefs distilledPagePrefs) { 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. https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... File components/dom_distiller.gypi (right): https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller.gypi:67: 'dom_distiller/core/distilled_page_prefs_observer_wrapper.cc', I think you need to also update dom_distiller/core/BUILD.gn https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java (right): https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:18: private HashMap<Observer, DistilledPagePrefsObserverWrapper> observerMap; private final and also member names should start with m. https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:40: long nativeObserverPtr = observerMap.get(obs).getNativePtr(); 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. https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:42: nativeRemoveObserver(mDistilledPagePrefsAndroid, nativeObserverPtr); This seems to be leaking the native object. https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:51: nativeGetTheme(mDistilledPagePrefsAndroid)); Nit: I think this can be on one line. Remember 100 chars limit for Java. https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefsObserverWrapper.java (right): https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefsObserverWrapper.java:13: @JNINamespace("dom_distiller") this namespace should probably match that of DistilledPagePrefs https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefsObserverWrapper.java:14: public class DistilledPagePrefsObserverWrapper { Could this be a static inner class of DistilledPagePrefs? At least make package protected. Also, I think in Java-land this could be called DistilledPagePrefsObserver if you rename the native calss to DistilledPagePrefsObserverAndroid. https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefsObserverWrapper.java:17: private long nativeDistilledPagePrefsObserverWrapperPtr; mNat... https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefsObserverWrapper.java:29: public long getNativePtr() { Could this be package protected? Seems like it's only used in DistilledPagePrefs. https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefsObserverWrapper.java:35: private native void nativeDestroy(long nativeDistilledPagePrefsObserverWrapper); This class should probably have a destroy-method which calls nativeDestroy. Otherwise you will be leaking the native object. I argue you could call destroy() on this object once it has been removed from the native list of observers. https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... File components/dom_distiller/core/distilled_page_prefs_android.cc (right): https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.cc:10: Nit: Remove empty newline. https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.cc:20: DistilledPagePrefs* distillerPagePrefsPtr) use _ instead of camelcase throughout this file. https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.cc:47: void DistilledPagePrefsAndroid::RemoveObserver(JNIEnv* env, Could you re-order these two functions to match the header? https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... File components/dom_distiller/core/distilled_page_prefs_android.h (right): https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.h:20: DistilledPagePrefs* distillerPagePrefsPtr); this should use _ instead of camelcase. https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... File components/dom_distiller/core/distilled_page_prefs_observer_wrapper.cc (right): https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_observer_wrapper.cc:34: DistilledPagePrefsObserverWrapper* native_observer_wrapper_ = the _ suffix is only for members. https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... File components/dom_distiller/core/distilled_page_prefs_observer_wrapper.h (right): https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_observer_wrapper.h:15: class DistilledPagePrefsObserverWrapper : public DistilledPagePrefs::Observer { How about DistilledPagePrefsObserverAndroid?
Please see new patch. https://codereview.chromium.org/403323005/diff/160001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java (right): https://codereview.chromium.org/403323005/diff/160001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:58: public void setTheme(final Theme theme) { On 2014/07/22 22:51:36, nyquist wrote: > private. Also, move to the bottom of the class. Done. https://codereview.chromium.org/403323005/diff/160001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:69: final TestingObserver testObserver = new TestingObserver(mDistilledPagePrefs); On 2014/07/22 22:51:36, nyquist wrote: > Nit: Unnecessary final. Done. https://codereview.chromium.org/403323005/diff/160001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:72: UiUtils.settleDownUI(getInstrumentation()); Done. On 2014/07/22 22:51:36, nyquist wrote: > 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. https://codereview.chromium.org/403323005/diff/160001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:81: final TestingObserver testObserverOne = new TestingObserver(mDistilledPagePrefs); On 2014/07/22 22:51:36, nyquist wrote: > Nit: unnecessary final here and below. Done. https://codereview.chromium.org/403323005/diff/160001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:92: // Check that testObserverOne's theme is not changed but testObserverTwo's On 2014/07/22 22:51:36, nyquist wrote: > Nit: one line Done. https://codereview.chromium.org/403323005/diff/160001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:102: public TestingObserver(DistilledPagePrefs distilledPagePrefs) { On 2014/07/22 22:51:36, nyquist wrote: > 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. Done. https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... File components/dom_distiller.gypi (right): https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller.gypi:67: 'dom_distiller/core/distilled_page_prefs_observer_wrapper.cc', Done. On 2014/07/22 22:51:36, nyquist wrote: > I think you need to also update dom_distiller/core/BUILD.gn https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java (right): https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:18: private HashMap<Observer, DistilledPagePrefsObserverWrapper> observerMap; On 2014/07/22 22:51:36, nyquist wrote: > private final and also member names should start with m. Done. https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:40: long nativeObserverPtr = observerMap.get(obs).getNativePtr(); On 2014/07/22 22:51:37, nyquist wrote: > 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. Done. https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:42: nativeRemoveObserver(mDistilledPagePrefsAndroid, nativeObserverPtr); On 2014/07/22 22:51:36, nyquist wrote: > This seems to be leaking the native object. Done. https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:51: nativeGetTheme(mDistilledPagePrefsAndroid)); On 2014/07/22 22:51:37, nyquist wrote: > Nit: I think this can be on one line. Remember 100 chars limit for Java. Done. https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefsObserverWrapper.java (right): https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefsObserverWrapper.java:13: @JNINamespace("dom_distiller") On 2014/07/22 22:51:37, nyquist wrote: > this namespace should probably match that of DistilledPagePrefs Done. https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefsObserverWrapper.java:14: public class DistilledPagePrefsObserverWrapper { Now static class. Inner class now called ObserverWrapper. Not sure if this is what you wanted? On 2014/07/22 22:51:37, nyquist wrote: > Could this be a static inner class of DistilledPagePrefs? At least make package > protected. > > Also, I think in Java-land this could be called DistilledPagePrefsObserver if > you rename the native calss to DistilledPagePrefsObserverAndroid. https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefsObserverWrapper.java:17: private long nativeDistilledPagePrefsObserverWrapperPtr; On 2014/07/22 22:51:37, nyquist wrote: > mNat... Done. https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefsObserverWrapper.java:29: public long getNativePtr() { On 2014/07/22 22:51:37, nyquist wrote: > Could this be package protected? Seems like it's only used in > DistilledPagePrefs. Done. https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefsObserverWrapper.java:35: private native void nativeDestroy(long nativeDistilledPagePrefsObserverWrapper); Done. On 2014/07/22 22:51:37, nyquist wrote: > This class should probably have a destroy-method which calls nativeDestroy. > Otherwise you will be leaking the native object. > > I argue you could call destroy() on this object once it has been removed from > the native list of observers. https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... File components/dom_distiller/core/distilled_page_prefs_android.cc (right): https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.cc:10: On 2014/07/22 22:51:37, nyquist wrote: > Nit: Remove empty newline. Done. https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.cc:20: DistilledPagePrefs* distillerPagePrefsPtr) On 2014/07/22 22:51:37, nyquist wrote: > use _ instead of camelcase throughout this file. Done. https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.cc:47: void DistilledPagePrefsAndroid::RemoveObserver(JNIEnv* env, On 2014/07/22 22:51:37, nyquist wrote: > Could you re-order these two functions to match the header? Done. https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... File components/dom_distiller/core/distilled_page_prefs_android.h (right): https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.h:20: DistilledPagePrefs* distillerPagePrefsPtr); On 2014/07/22 22:51:37, nyquist wrote: > this should use _ instead of camelcase. Done. https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... File components/dom_distiller/core/distilled_page_prefs_observer_wrapper.cc (right): https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_observer_wrapper.cc:34: DistilledPagePrefsObserverWrapper* native_observer_wrapper_ = On 2014/07/22 22:51:37, nyquist wrote: > the _ suffix is only for members. Done. https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... File components/dom_distiller/core/distilled_page_prefs_observer_wrapper.h (right): https://codereview.chromium.org/403323005/diff/160001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_observer_wrapper.h:15: class DistilledPagePrefsObserverWrapper : public DistilledPagePrefs::Observer { On 2014/07/22 22:51:37, nyquist wrote: > How about DistilledPagePrefsObserverAndroid? Done.
https://codereview.chromium.org/403323005/diff/300001/base/android/java/src/o... File base/android/java/src/org/chromium/base/NativeCall.java (right): https://codereview.chromium.org/403323005/diff/300001/base/android/java/src/o... base/android/java/src/org/chromium/base/NativeCall.java:14: * so a native function can be bound to a Java inner class. Nit: Mention that you can specify for which native class you want the JNI method to be generated. https://codereview.chromium.org/403323005/diff/300001/base/android/java/src/o... base/android/java/src/org/chromium/base/NativeCall.java:20: public String value() default ""; Nit: Could you add a comment about this? The value says which native class the method should map to. https://codereview.chromium.org/403323005/diff/300001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java (right): https://codereview.chromium.org/403323005/diff/300001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:63: setTheme(Theme.DARK); Nit: Empty line before this? https://codereview.chromium.org/403323005/diff/300001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:79: setTheme(Theme.SEPIA); Nit: Empty line before this? https://codereview.chromium.org/403323005/diff/300001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:84: setTheme(Theme.DARK); Nit: Empty line before this? https://codereview.chromium.org/403323005/diff/300001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java (right): https://codereview.chromium.org/403323005/diff/300001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:34: private long mNativeDistilledPagePrefsObserverAndroidPtr; final https://codereview.chromium.org/403323005/diff/300001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:50: long getNativePtr() { This should have the same visibility as the constructor and destroy. There are two ways of going about this, but I personally like what you did in the TestObserver. 1) Make them all private. 2) Since the class is already private, it can only be accessed by the outer class, so make the methods used by the outer class public to signify usage. So, public constructor, destroy and getNativePtr. Personally I prefer 2), but up to you. https://codereview.chromium.org/403323005/diff/300001/components/dom_distille... File components/dom_distiller/core/BUILD.gn (right): https://codereview.chromium.org/403323005/diff/300001/components/dom_distille... components/dom_distiller/core/BUILD.gn:20: "distilled_page_prefs.cc", Thanks for cleaning this up! https://codereview.chromium.org/403323005/diff/300001/components/dom_distille... File components/dom_distiller/core/distilled_page_prefs_android.cc (right): https://codereview.chromium.org/403323005/diff/300001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.cc:82: DistilledPagePrefsObserverAndroid* native_observer_wrapper = Nit: observer_android maybe? https://codereview.chromium.org/403323005/diff/300001/components/dom_distille... File components/dom_distiller/core/distilled_page_prefs_android.h (right): https://codereview.chromium.org/403323005/diff/300001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.h:34: class DistilledPagePrefsObserverAndroid : public DistilledPagePrefs::Observer { Could you file a bug and assign to me about making NativeCall supporting inner C++ classes? This class name is just super long now, but not your fault. You don't have to refer to the bug in your code. https://codereview.chromium.org/403323005/diff/300001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.h:39: // DistilledPagePrefs::Observer Nit: End comment with . Also, how about something on the form: "DistilledPagePrefs::Observer implementation."?
Please see new patch. https://codereview.chromium.org/403323005/diff/300001/base/android/java/src/o... File base/android/java/src/org/chromium/base/NativeCall.java (right): https://codereview.chromium.org/403323005/diff/300001/base/android/java/src/o... base/android/java/src/org/chromium/base/NativeCall.java:14: * so a native function can be bound to a Java inner class. On 2014/07/23 17:30:45, nyquist wrote: > Nit: Mention that you can specify for which native class you want the JNI method > to be generated. Done. https://codereview.chromium.org/403323005/diff/300001/base/android/java/src/o... base/android/java/src/org/chromium/base/NativeCall.java:20: public String value() default ""; On 2014/07/23 17:30:45, nyquist wrote: > Nit: Could you add a comment about this? The value says which native class the > method should map to. Done. https://codereview.chromium.org/403323005/diff/300001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java (right): https://codereview.chromium.org/403323005/diff/300001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:63: setTheme(Theme.DARK); On 2014/07/23 17:30:46, nyquist wrote: > Nit: Empty line before this? Done. https://codereview.chromium.org/403323005/diff/300001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:79: setTheme(Theme.SEPIA); On 2014/07/23 17:30:46, nyquist wrote: > Nit: Empty line before this? Done. https://codereview.chromium.org/403323005/diff/300001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:84: setTheme(Theme.DARK); On 2014/07/23 17:30:46, nyquist wrote: > Nit: Empty line before this? Done. https://codereview.chromium.org/403323005/diff/300001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java (right): https://codereview.chromium.org/403323005/diff/300001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:34: private long mNativeDistilledPagePrefsObserverAndroidPtr; On 2014/07/23 17:30:46, nyquist wrote: > final Done. https://codereview.chromium.org/403323005/diff/300001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:50: long getNativePtr() { On 2014/07/23 17:30:46, nyquist wrote: > This should have the same visibility as the constructor and destroy. > > There are two ways of going about this, but I personally like what you did in > the TestObserver. > > 1) Make them all private. > 2) Since the class is already private, it can only be accessed by the outer > class, so make the methods used by the outer class public to signify usage. So, > public constructor, destroy and getNativePtr. > > Personally I prefer 2), but up to you. Done. https://codereview.chromium.org/403323005/diff/300001/components/dom_distille... File components/dom_distiller/core/distilled_page_prefs_android.cc (right): https://codereview.chromium.org/403323005/diff/300001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.cc:82: DistilledPagePrefsObserverAndroid* native_observer_wrapper = On 2014/07/23 17:30:46, nyquist wrote: > Nit: observer_android maybe? Done. https://codereview.chromium.org/403323005/diff/300001/components/dom_distille... File components/dom_distiller/core/distilled_page_prefs_android.h (right): https://codereview.chromium.org/403323005/diff/300001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.h:34: class DistilledPagePrefsObserverAndroid : public DistilledPagePrefs::Observer { It is bug 396697. Let me know if I did not do it right. On 2014/07/23 17:30:46, nyquist wrote: > Could you file a bug and assign to me about making NativeCall supporting inner > C++ classes? This class name is just super long now, but not your fault. You > don't have to refer to the bug in your code. https://codereview.chromium.org/403323005/diff/300001/components/dom_distille... components/dom_distiller/core/distilled_page_prefs_android.h:39: // DistilledPagePrefs::Observer On 2014/07/23 17:30:46, nyquist wrote: > Nit: End comment with . > Also, how about something on the form: "DistilledPagePrefs::Observer > implementation."? Done.
lgtm https://codereview.chromium.org/403323005/diff/360001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java (right): https://codereview.chromium.org/403323005/diff/360001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:20: private HashMap<Observer, DistilledPagePrefsObserverWrapper> mObserverMap; Nit: Map<Observer, DPPOW> (but continue to use HashMap in constructor).
Please see new patch. https://codereview.chromium.org/403323005/diff/360001/components/dom_distille... File components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java (right): https://codereview.chromium.org/403323005/diff/360001/components/dom_distille... components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:20: private HashMap<Observer, DistilledPagePrefsObserverWrapper> mObserverMap; On 2014/07/23 18:14:28, nyquist wrote: > Nit: Map<Observer, DPPOW> (but continue to use HashMap in constructor). Done.
The CQ bit was checked by sunangel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sunangel@chromium.org/403323005/380001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was checked by sunangel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sunangel@chromium.org/403323005/380001
Message was sent while issue was closed.
Change committed as 285109 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
