Remove FontSizePrefs destroy() method as its a singleton class.

Remove FontSizePrefs destroy() method as its a singleton class.

As FontSizePrefs class is singleton and will be alive till the entire application lifespan
and there are no references for destroy(), hence destroy() implemention can be removed.

BUG=None.

Committed: https://crrev.com/42f2211d2f2b05665547a08a842adeca7a6ea779
Cr-Commit-Position: refs/heads/master@{#295269}

[wajahat, 2014-09-11 12:05:15]

wajahat.s@samsung.com changed reviewers:
+ sky@chromium.org, sunangel@chromium.org, yfriedman@chromium.org

[wajahat, 2014-09-11 12:05:15]

Please Review, Thanks!

[sky, 2014-09-11 16:03:52]

sky@chromium.org changed reviewers:
- sky@chromium.org

[sky, 2014-09-11 16:03:52]

I'm not a good reviewer for this.
-sky

[wajahat, 2014-09-15 06:09:30]

wajahat.s@samsung.com changed reviewers:
+ tedchoc@chromium.org

[wajahat, 2014-09-15 06:09:31]

+Ted Choc
Pl. review, Thanks!

[Ted C, 2014-09-15 18:42:36]

tedchoc@chromium.org changed reviewers:
+ nyquist@chromium.org

[Ted C, 2014-09-15 18:42:37]

Sorry to pass this along again, but I'm really unfamiliar with this code as
well.  nyquist@ did the initial and only review for this, so hopefully he has
some incite.

With that said, I have concerns about this methods existence in general, but
I'll leave it up to Tommy to comment further.

https://codereview.chromium.org/563743002/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java
(right):

https://codereview.chromium.org/563743002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:176:
mObserverMap.clear();
destroy seems like an odd API to have on a singleton.  The singleton would now
be in an invalid state and can never be reused.  That isn't a comment about this
change per say, but it's just general confusion about this.

Now for this, are the observers being cleaned up as part of the native destroy
call?  To me, it looks like this could end up leaking the native observer
references.  If that is the case, we should be iterating over the map and
calling destroy on them to make sure that doesn't happen.

Also, I can't find any references to this method and I suspect it is never
actually called (nor does it really need to as destroy would only come when
android is about to destroy our application and kill the process).

[nyquist, 2014-09-15 19:56:50]

https://codereview.chromium.org/563743002/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java
(right):

https://codereview.chromium.org/563743002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:176:
mObserverMap.clear();
On 2014/09/15 18:42:37, Ted C wrote:
> destroy seems like an odd API to have on a singleton.  The singleton would now
> be in an invalid state and can never be reused.  That isn't a comment about
this
> change per say, but it's just general confusion about this.
> 
> Now for this, are the observers being cleaned up as part of the native destroy
> call?  To me, it looks like this could end up leaking the native observer
> references.  If that is the case, we should be iterating over the map and
> calling destroy on them to make sure that doesn't happen.
> 
> Also, I can't find any references to this method and I suspect it is never
> actually called (nor does it really need to as destroy would only come when
> android is about to destroy our application and kill the process).

Yes, .clear() would be wrong. I think it would be better to just remove this
method alltogether.

[wajahat, 2014-09-16 09:30:36]

Changes incorporated as per suggestions
PTAL, Thanks!

https://codereview.chromium.org/563743002/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java
(right):

https://codereview.chromium.org/563743002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:176:
mObserverMap.clear();
On 2014/09/15 19:56:50, nyquist wrote:
> On 2014/09/15 18:42:37, Ted C wrote:
> > destroy seems like an odd API to have on a singleton.  The singleton would
now
> > be in an invalid state and can never be reused.  That isn't a comment about
> this
> > change per say, but it's just general confusion about this.
> > 
> > Now for this, are the observers being cleaned up as part of the native
destroy
> > call?  To me, it looks like this could end up leaking the native observer
> > references.  If that is the case, we should be iterating over the map and
> > calling destroy on them to make sure that doesn't happen.
> > 
> > Also, I can't find any references to this method and I suspect it is never
> > actually called (nor does it really need to as destroy would only come when
> > android is about to destroy our application and kill the process).
> 
> Yes, .clear() would be wrong. I think it would be better to just remove this
> method alltogether.

Done.

https://codereview.chromium.org/563743002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/accessibility/FontSizePrefs.java:176:
mObserverMap.clear();
As this is a singleton class and there are no references to destroy() hence
removing this method altogether.

On 2014/09/15 18:42:37, Ted C wrote:
> destroy seems like an odd API to have on a singleton.  The singleton would now
> be in an invalid state and can never be reused.  That isn't a comment about
this
> change per say, but it's just general confusion about this.
> 
> Now for this, are the observers being cleaned up as part of the native destroy
> call?  To me, it looks like this could end up leaking the native observer
> references.  If that is the case, we should be iterating over the map and
> calling destroy on them to make sure that doesn't happen.
> 
> Also, I can't find any references to this method and I suspect it is never
> actually called (nor does it really need to as destroy would only come when
> android is about to destroy our application and kill the process).

[nyquist, 2014-09-16 15:52:46]

lgtm. thanks!

[Ted C, 2014-09-16 18:42:09]

lgtm

[wajahat, 2014-09-17 05:30:16]

The CQ bit was checked by wajahat.s@samsung.com

[commit-bot: I haz the power, 2014-09-17 05:30:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/563743002/20001

[commit-bot: I haz the power, 2014-09-17 07:31:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-17 07:31:22]

Exceeded time limit waiting for builds to trigger.

[wajahat, 2014-09-17 13:33:53]

The CQ bit was checked by wajahat.s@samsung.com

[commit-bot: I haz the power, 2014-09-17 13:34:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/563743002/20001

[commit-bot: I haz the power, 2014-09-17 14:18:46]

Committed patchset #2 (id:20001) as 5a9879c71a76bb0fdebf3ee021346e66d1cc3dc4

[commit-bot: I haz the power, 2014-09-17 14:19:23]

Patchset 2 (id:??) landed as
https://crrev.com/42f2211d2f2b05665547a08a842adeca7a6ea779
Cr-Commit-Position: refs/heads/master@{#295269}

[Primiano Tucci (use gerrit), 2014-12-02 11:42:28]

On 2014/09/17 14:19:23, I haz the power (commit-bot) wrote:
> Patchset 2 (id:??) landed as
> https://crrev.com/42f2211d2f2b05665547a08a842adeca7a6ea779
> Cr-Commit-Position: refs/heads/master@{#295269}

There seem to be a clusterfuzz bug related to this CR: crbug.com/433807
Can anybody here check whether it is plausible that this CL is causing the bug?
If so can somebody own it?