Ensure Java DistilledPagePrefs observers can only be added once.

Ensure Java DistilledPagePrefs observers can only be added once.

The Java DistilledPagePrefs observers are wrapped in a
native observer, which is only deleted when the observer is
removed again. Since the reference to this native observer
wrapper is kept in a Java map with the real Java observer as
the key, the second time addObserver(foo) is called with the
same observer, the first reference is lost, and the native
object is leaked. 

For example, the following would leak a native object:
addObserver(foo);
addObserver(foo);
removeObserver(foo);
removeObserver(foo);

It is already safe to remove an observer if it is not
currently an observer, and this CL ensures that an observer
can only be added once, so the second calls to
addObserver(foo) and removeObserver(foo) will be no-ops.

addObserver and removeObserver will return a boolean based
on whether they have successfully completed the operation.

BUG=383630
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287911

[sunangel, 2014-07-30 21:56:48]

Should be a quick CL.

[sunangel, 2014-07-30 21:56:49]

Should be a quick CL.

[nyquist, 2014-07-30 22:08:13]

Remember to have a newline after the first line in the description.

Also, could you say something about ensuring that an observer can not be added
twice instead of mentioning the null-check?

How about something like:

###
Ensure Java DistilledPagePrefs observers can only be added once.

The Java DistilledPagePrefs observers are wrapped in a native observer,
which is only deleted when the observer is removed again. Since the
reference to this native observer wrapper is kept in a Java map with the
real Java observer as the key, the second time addObserver(foo) is
called with the same observer, the first reference is lost, and the
native object is leaked.

For example, the following would leak a native object:
addObserver(foo);
addObserver(foo);
removeObserver(foo);
removeObserver(foo);

It is already safe to remove an observer if it is not currently an observer, 
and this CL ensures that an observer can only be added once, so the second
calls to addObserver(foo) and removeObserver(foo) will be no-ops.
###

https://codereview.chromium.org/426153006/diff/1/components/dom_distiller/and...
File
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java
(right):

https://codereview.chromium.org/426153006/diff/1/components/dom_distiller/and...
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:69:
if (mObserverMap.get(obs) == null) {
Could you do an early return instead of indenting? Also, How about using
containsKey?

[sunangel, 2014-07-30 23:09:20]

https://codereview.chromium.org/426153006/diff/1/components/dom_distiller/and...
File
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java
(right):

https://codereview.chromium.org/426153006/diff/1/components/dom_distiller/and...
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:69:
if (mObserverMap.get(obs) == null) {
On 2014/07/30 22:08:12, nyquist wrote:
> Could you do an early return instead of indenting? Also, How about using
> containsKey?

Done.

[sunangel, 2014-07-31 00:42:02]

New patch for consistency of return for AddObserver and RemoveObserver

[robliao, 2014-07-31 00:51:37]

https://codereview.chromium.org/426153006/diff/40001/chrome/android/javatests...
File
chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java
(right):

https://codereview.chromium.org/426153006/diff/40001/chrome/android/javatests...
chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:105:
// Delete the observer the first time.
Fix tab alignment.

https://codereview.chromium.org/426153006/diff/40001/components/dom_distiller...
File
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java
(right):

https://codereview.chromium.org/426153006/diff/40001/components/dom_distiller...
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:73:
if (mObserverMap.containsKey(obs)) {
if (mObserverMap.containsKey(obs)) return false;

[sunangel, 2014-07-31 01:44:29]

https://codereview.chromium.org/426153006/diff/40001/chrome/android/javatests...
File
chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java
(right):

https://codereview.chromium.org/426153006/diff/40001/chrome/android/javatests...
chrome/android/javatests/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsTest.java:105:
// Delete the observer the first time.
On 2014/07/31 00:51:36, robliao wrote:
> Fix tab alignment.

Done.

https://codereview.chromium.org/426153006/diff/40001/components/dom_distiller...
File
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java
(right):

https://codereview.chromium.org/426153006/diff/40001/components/dom_distiller...
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:73:
if (mObserverMap.containsKey(obs)) {
On 2014/07/31 00:51:36, robliao wrote:
> if (mObserverMap.containsKey(obs)) return false;

Done.

[robliao, 2014-07-31 17:20:26]

lgtm

[nyquist, 2014-08-01 04:30:39]

mostly lg

https://codereview.chromium.org/426153006/diff/60001/components/dom_distiller...
File
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java
(right):

https://codereview.chromium.org/426153006/diff/60001/components/dom_distiller...
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:70:
* @return true if the observerMap was changed as a result of the call.
"true if" -> "whether". Same below.

https://codereview.chromium.org/426153006/diff/60001/components/dom_distiller...
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:87:
if (wrappedObserver != null) {
How about flipping this to remove the indent and make it similar to the one
above?

[sunangel, 2014-08-04 19:56:25]

https://codereview.chromium.org/426153006/diff/60001/components/dom_distiller...
File
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java
(right):

https://codereview.chromium.org/426153006/diff/60001/components/dom_distiller...
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:70:
* @return true if the observerMap was changed as a result of the call.
On 2014/08/01 04:30:38, nyquist wrote:
> "true if" -> "whether". Same below.

Done.

https://codereview.chromium.org/426153006/diff/60001/components/dom_distiller...
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DistilledPagePrefs.java:87:
if (wrappedObserver != null) {
On 2014/08/01 04:30:39, nyquist wrote:
> How about flipping this to remove the indent and make it similar to the one
> above?

Done.

[nyquist, 2014-08-06 21:51:49]

lgtm

Maybe you could add a sentence to the description about adding a return value to
the API?

[sunangel, 2014-08-06 22:08:09]

The CQ bit was checked by sunangel@chromium.org

[commit-bot: I haz the power, 2014-08-06 22:09:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sunangel@chromium.org/426153006/80001

[commit-bot: I haz the power, 2014-08-07 00:10:08]

Change committed as 287911