Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12378004:
Android: Add a java version of ObserverList. (Closed)
|
Created:
7 years, 9 months ago by nilesh Modified:
7 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, erikwright+watch_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAndroid: Add a java version of ObserverList.
Provides a safe container to maintain observer/listener lists which can be modified during iteration.
This is heavily based on the C++ version.
TBR=jam@chromium.org
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=185595
Patch Set 1 #
Total comments: 31
Patch Set 2 : Addressed Tommy's comments. #
Total comments: 14
Patch Set 3 : Added testAddWhileIteration #Patch Set 4 : Added NotThreadSafe annotation and dep on jsr #Patch Set 5 : rebase and fix bug in test #Patch Set 6 : Added hasObserver call in the test. #
Messages
Total messages: 15 (0 generated)
https://codereview.chromium.org/12378004/diff/1/base/android/java/src/org/chr... File base/android/java/src/org/chromium/base/ObserverList.java (right): https://codereview.chromium.org/12378004/diff/1/base/android/java/src/org/chr... base/android/java/src/org/chromium/base/ObserverList.java:15: * Nit: Add <p/> here and below https://codereview.chromium.org/12378004/diff/1/base/android/java/src/org/chr... base/android/java/src/org/chromium/base/ObserverList.java:21: * C++ ObserverList. This class is not threadsafe. Annotate with @javax.annotation.concurrent.NotThreadSafe from jsr-305 instead. https://codereview.chromium.org/12378004/diff/1/base/android/java/src/org/chr... base/android/java/src/org/chromium/base/ObserverList.java:24: public final List<E> mObservers = new ArrayList<E>(); Nit: extra space after = https://codereview.chromium.org/12378004/diff/1/base/android/java/src/org/chr... base/android/java/src/org/chromium/base/ObserverList.java:31: * Nit: Add <p/> https://codereview.chromium.org/12378004/diff/1/base/android/java/src/org/chr... base/android/java/src/org/chromium/base/ObserverList.java:87: * Nit: Add <p/> https://codereview.chromium.org/12378004/diff/1/base/android/java/src/org/chr... base/android/java/src/org/chromium/base/ObserverList.java:94: while(it.hasNext()) { Nit: Add space after while https://codereview.chromium.org/12378004/diff/1/base/android/java/src/org/chr... base/android/java/src/org/chromium/base/ObserverList.java:102: mIterationDepth++; Prefix or suffix with ++/-- consistently within this class. I believe chromium code usually prefixes with ++/--. https://codereview.chromium.org/12378004/diff/1/base/android/java/src/org/chr... base/android/java/src/org/chromium/base/ObserverList.java:107: if (mIterationDepth == 0) assert mIterationDepth >= 0? https://codereview.chromium.org/12378004/diff/1/base/android/java/src/org/chr... base/android/java/src/org/chromium/base/ObserverList.java:120: private final int mMaxIndex; mMaxIndex kind of sounds like it points to the last index, but in fact it points to an index that does not exist (the size of the list). So either use <= or rename member field? https://codereview.chromium.org/12378004/diff/1/base/android/javatests/src/or... File base/android/javatests/src/org/chromium/base/ObserverListTest.java (right): https://codereview.chromium.org/12378004/diff/1/base/android/javatests/src/or... base/android/javatests/src/org/chromium/base/ObserverListTest.java:23: class Foo implements Observer { may be private static https://codereview.chromium.org/12378004/diff/1/base/android/javatests/src/or... base/android/javatests/src/org/chromium/base/ObserverListTest.java:25: public int mTotal = 0; private https://codereview.chromium.org/12378004/diff/1/base/android/javatests/src/or... base/android/javatests/src/org/chromium/base/ObserverListTest.java:41: class FooRemover implements Observer { may be private static https://codereview.chromium.org/12378004/diff/1/base/android/javatests/src/or... base/android/javatests/src/org/chromium/base/ObserverListTest.java:88: // evil removed c from the observerList. add something short like "before it got any callbacks" https://codereview.chromium.org/12378004/diff/1/base/android/javatests/src/or... base/android/javatests/src/org/chromium/base/ObserverListTest.java:114: it.remove(); Add fail(...) after removing. https://codereview.chromium.org/12378004/diff/1/base/android/javatests/src/or... base/android/javatests/src/org/chromium/base/ObserverListTest.java:116: removeExceptionThrown = true; How would you feel about adding a getSizeForTest() to the ObserverList, which makes it possible to assert the number of ObserverList entries here? Just to ensure that you don't delete an arbitrary element by calling it.remove(). Worst case, you could have a helper method that takes an Iterator<?> and counts the number of elements. I.e.: <T> int numElements(Iterable<T> iterable) { int num = 0; for (T t : iterable) { ++num; } return num; }
PTAL. https://codereview.chromium.org/12378004/diff/1/base/android/java/src/org/chr... File base/android/java/src/org/chromium/base/ObserverList.java (right): https://codereview.chromium.org/12378004/diff/1/base/android/java/src/org/chr... base/android/java/src/org/chromium/base/ObserverList.java:15: * On 2013/02/28 01:52:18, nyquist wrote: > Nit: Add <p/> here and below Done. https://codereview.chromium.org/12378004/diff/1/base/android/java/src/org/chr... base/android/java/src/org/chromium/base/ObserverList.java:21: * C++ ObserverList. This class is not threadsafe. On 2013/02/28 01:52:18, nyquist wrote: > Annotate with @javax.annotation.concurrent.NotThreadSafe from jsr-305 instead. I get an error trying to use this annotation: package javax.annotation.concurrent does not exist ? https://codereview.chromium.org/12378004/diff/1/base/android/java/src/org/chr... base/android/java/src/org/chromium/base/ObserverList.java:24: public final List<E> mObservers = new ArrayList<E>(); On 2013/02/28 01:52:18, nyquist wrote: > Nit: extra space after = Done. https://codereview.chromium.org/12378004/diff/1/base/android/java/src/org/chr... base/android/java/src/org/chromium/base/ObserverList.java:31: * On 2013/02/28 01:52:18, nyquist wrote: > Nit: Add <p/> Done. https://codereview.chromium.org/12378004/diff/1/base/android/java/src/org/chr... base/android/java/src/org/chromium/base/ObserverList.java:87: * On 2013/02/28 01:52:18, nyquist wrote: > Nit: Add <p/> Done. https://codereview.chromium.org/12378004/diff/1/base/android/java/src/org/chr... base/android/java/src/org/chromium/base/ObserverList.java:94: while(it.hasNext()) { On 2013/02/28 01:52:18, nyquist wrote: > Nit: Add space after while Done. https://codereview.chromium.org/12378004/diff/1/base/android/java/src/org/chr... base/android/java/src/org/chromium/base/ObserverList.java:102: mIterationDepth++; On 2013/02/28 01:52:18, nyquist wrote: > Prefix or suffix with ++/-- consistently within this class. I believe chromium > code usually prefixes with ++/--. Done. https://codereview.chromium.org/12378004/diff/1/base/android/java/src/org/chr... base/android/java/src/org/chromium/base/ObserverList.java:107: if (mIterationDepth == 0) On 2013/02/28 01:52:18, nyquist wrote: > assert mIterationDepth >= 0? Done. https://codereview.chromium.org/12378004/diff/1/base/android/java/src/org/chr... base/android/java/src/org/chromium/base/ObserverList.java:120: private final int mMaxIndex; Renamed to mListEndMarkeR https://codereview.chromium.org/12378004/diff/1/base/android/javatests/src/or... File base/android/javatests/src/org/chromium/base/ObserverListTest.java (right): https://codereview.chromium.org/12378004/diff/1/base/android/javatests/src/or... base/android/javatests/src/org/chromium/base/ObserverListTest.java:23: class Foo implements Observer { On 2013/02/28 01:52:18, nyquist wrote: > may be private static Done. https://codereview.chromium.org/12378004/diff/1/base/android/javatests/src/or... base/android/javatests/src/org/chromium/base/ObserverListTest.java:25: public int mTotal = 0; On 2013/02/28 01:52:18, nyquist wrote: > private Done. https://codereview.chromium.org/12378004/diff/1/base/android/javatests/src/or... base/android/javatests/src/org/chromium/base/ObserverListTest.java:41: class FooRemover implements Observer { On 2013/02/28 01:52:18, nyquist wrote: > may be private static Done. https://codereview.chromium.org/12378004/diff/1/base/android/javatests/src/or... base/android/javatests/src/org/chromium/base/ObserverListTest.java:88: // evil removed c from the observerList. On 2013/02/28 01:52:18, nyquist wrote: > add something short like "before it got any callbacks" Done. https://codereview.chromium.org/12378004/diff/1/base/android/javatests/src/or... base/android/javatests/src/org/chromium/base/ObserverListTest.java:114: it.remove(); On 2013/02/28 01:52:18, nyquist wrote: > Add fail(...) after removing. Done. https://codereview.chromium.org/12378004/diff/1/base/android/javatests/src/or... base/android/javatests/src/org/chromium/base/ObserverListTest.java:116: removeExceptionThrown = true; On 2013/02/28 01:52:18, nyquist wrote: > How would you feel about adding a getSizeForTest() to the ObserverList, which > makes it possible to assert the number of ObserverList entries here? Just to > ensure that you don't delete an arbitrary element by calling it.remove(). > > Worst case, you could have a helper method that takes an Iterator<?> and counts > the number of elements. I.e.: > > <T> int numElements(Iterable<T> iterable) { > int num = 0; > for (T t : iterable) { > ++num; > } > return num; > } Makes sense, added a helper method (in the test) to compute the size.
lgtm on my side, thanks! mostly documentation and some nits... feel free to go ahead once nyquist is happy with it! https://codereview.chromium.org/12378004/diff/9001/base/android/java/src/org/... File base/android/java/src/org/chromium/base/ObserverList.java (right): https://codereview.chromium.org/12378004/diff/9001/base/android/java/src/org/... base/android/java/src/org/chromium/base/ObserverList.java:20: * The implementation (and the interface) is heavily influenced by the C++ ObserverList. Notable differences: -- The iterator implements NOTIFY_EXISTING_ONLY -- The FOR_EACH_OBSERVER closure is left to the clients to implement in terms of iterator() https://codereview.chromium.org/12378004/diff/9001/base/android/java/src/org/... base/android/java/src/org/chromium/base/ObserverList.java:21: * This class is not threadsafe. maybe even more explicit: This class is NOT threadsafe: observers MUST be added, removed and will be notified on the same thread this is created. https://codereview.chromium.org/12378004/diff/9001/base/android/java/src/org/... base/android/java/src/org/chromium/base/ObserverList.java:53: if (index == -1) maybe assert false for consistency with 38 above? https://codereview.chromium.org/12378004/diff/9001/base/android/java/src/org/... base/android/java/src/org/chromium/base/ObserverList.java:91: // is iterating over the list. assert mIterationDepth == 0; https://codereview.chromium.org/12378004/diff/9001/base/android/java/src/org/... base/android/java/src/org/chromium/base/ObserverList.java:104: private void decrementIterationDepthAndCompact() { add "IfNeeded" https://codereview.chromium.org/12378004/diff/9001/base/android/javatests/src... File base/android/javatests/src/org/chromium/base/ObserverListTest.java (right): https://codereview.chromium.org/12378004/diff/9001/base/android/javatests/src... base/android/javatests/src/org/chromium/base/ObserverListTest.java:65: public void testRemoveWhileIteration() { it'd be nice to add a "testAddWhileIterating" and ensure that the new iterator is NOT called, wdyt? https://codereview.chromium.org/12378004/diff/9001/base/android/javatests/src... base/android/javatests/src/org/chromium/base/ObserverListTest.java:67: Foo a, b, c, d, e; nit: I don't think this is strictly forbidden, but just in case, it may be simpler to remove the multiple declaration and just repeat: Foo a = new Foo(1); Foo b = ...
Added testAddWhileIteration and addressed other comments. https://codereview.chromium.org/12378004/diff/9001/base/android/java/src/org/... File base/android/java/src/org/chromium/base/ObserverList.java (right): https://codereview.chromium.org/12378004/diff/9001/base/android/java/src/org/... base/android/java/src/org/chromium/base/ObserverList.java:20: * The implementation (and the interface) is heavily influenced by the C++ ObserverList. On 2013/02/28 12:39:12, bulach wrote: > Notable differences: > -- The iterator implements NOTIFY_EXISTING_ONLY > -- The FOR_EACH_OBSERVER closure is left to the clients to implement in terms of > iterator() Done. https://codereview.chromium.org/12378004/diff/9001/base/android/java/src/org/... base/android/java/src/org/chromium/base/ObserverList.java:21: * This class is not threadsafe. On 2013/02/28 12:39:12, bulach wrote: > maybe even more explicit: > This class is NOT threadsafe: observers MUST be added, removed and will be > notified on the same thread this is created. Done. https://codereview.chromium.org/12378004/diff/9001/base/android/java/src/org/... base/android/java/src/org/chromium/base/ObserverList.java:53: if (index == -1) On 2013/02/28 12:39:12, bulach wrote: > maybe assert false for consistency with 38 above? I am keeping this behavior consistent with the C++ version. https://codereview.chromium.org/12378004/diff/9001/base/android/java/src/org/... base/android/java/src/org/chromium/base/ObserverList.java:91: // is iterating over the list. On 2013/02/28 12:39:12, bulach wrote: > assert mIterationDepth == 0; Done. https://codereview.chromium.org/12378004/diff/9001/base/android/java/src/org/... base/android/java/src/org/chromium/base/ObserverList.java:104: private void decrementIterationDepthAndCompact() { On 2013/02/28 12:39:12, bulach wrote: > add "IfNeeded" Done. https://codereview.chromium.org/12378004/diff/9001/base/android/javatests/src... File base/android/javatests/src/org/chromium/base/ObserverListTest.java (right): https://codereview.chromium.org/12378004/diff/9001/base/android/javatests/src... base/android/javatests/src/org/chromium/base/ObserverListTest.java:65: public void testRemoveWhileIteration() { On 2013/02/28 12:39:12, bulach wrote: > it'd be nice to add a "testAddWhileIterating" and ensure that the new iterator > is NOT called, wdyt? Makes sense, add a test for addWhileIterating. https://codereview.chromium.org/12378004/diff/9001/base/android/javatests/src... base/android/javatests/src/org/chromium/base/ObserverListTest.java:67: Foo a, b, c, d, e; On 2013/02/28 12:39:12, bulach wrote: > nit: I don't think this is strictly forbidden, but just in case, it may be > simpler to remove the multiple declaration and just repeat: > Foo a = new Foo(1); > Foo b = ... Done.
https://codereview.chromium.org/12378004/diff/1/base/android/java/src/org/chr... File base/android/java/src/org/chromium/base/ObserverList.java (right): https://codereview.chromium.org/12378004/diff/1/base/android/java/src/org/chr... base/android/java/src/org/chromium/base/ObserverList.java:21: * C++ ObserverList. This class is not threadsafe. On 2013/02/28 04:57:28, nileshagrawal1 wrote: > On 2013/02/28 01:52:18, nyquist wrote: > > Annotate with @javax.annotation.concurrent.NotThreadSafe from jsr-305 instead. > I get an error trying to use this annotation: > package javax.annotation.concurrent does not exist ? Add a GYP dependency on ../third_party/jsr-305/jsr-305.gyp:jsr_305_javalib
On 2013/02/28 23:36:31, nyquist wrote: > https://codereview.chromium.org/12378004/diff/1/base/android/java/src/org/chr... > File base/android/java/src/org/chromium/base/ObserverList.java (right): > > https://codereview.chromium.org/12378004/diff/1/base/android/java/src/org/chr... > base/android/java/src/org/chromium/base/ObserverList.java:21: * C++ > ObserverList. This class is not threadsafe. > On 2013/02/28 04:57:28, nileshagrawal1 wrote: > > On 2013/02/28 01:52:18, nyquist wrote: > > > Annotate with @javax.annotation.concurrent.NotThreadSafe from jsr-305 > instead. > > I get an error trying to use this annotation: > > package javax.annotation.concurrent does not exist ? > > Add a GYP dependency on ../third_party/jsr-305/jsr-305.gyp:jsr_305_javalib Done.
Adding Mark for base/base.gyp
lgtm
LGTM
TBRing jam for content.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nileshagrawal@chromium.org/12378004/13001
The commit queue went berserk retrying too often for a seemingly flaky test. Builder is android_dbg, revision is HEAD
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nileshagrawal@chromium.org/12378004/10008
Message was sent while issue was closed.
Committed patchset #6 manually as r185595 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
