[Android] Upstream ChromeNativePreferences as PrefServiceBridge

[Android] Upstream ChromeNativePreferences as PrefServiceBridge

There's a few lingering bits that could be split out into separate
classes (ClearBrowsingDataObserver, VersionStrings) but can be done in a
separate change.

BUG=169106
NOTRY=true

Committed: https://crrev.com/433afcdd00085ccb0b9ab89143fe6410f9d0af80
Cr-Commit-Position: refs/heads/master@{#300562}

[Yaron, 2014-10-21 00:21:07]

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

[Yaron, 2014-10-21 00:21:08]

Differences from downstream:
1) The name
2) PopupException -> PopupExceptionInfo (otherwise it's a findbugs error)
3) I trimmed the includes of pref_service_bridge.cc

[Ted C, 2014-10-21 16:57:22]

lgtm -- sorry to give you several years of built of style nits, but someone's
got to do it :-)

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/NetworkPredictionOptions.java
(right):

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/NetworkPredictionOptions.java:6:

only one blank line here

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/NetworkPredictionOptions.java:14:
*
remove this empty line

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java
(right):

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:1:
// Copyright 2012 The Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:15:
* preferences should be grouped with their relevant functionality but this is a
grab-bag for other
:-) so honest

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:59:
return mApplicationVersion;
not that you want to change these as part of upstream, but for these simple data
objects where all members are final I think it is fine to just make the member
variables public (drop the m prefix) and get rid of these accessors.

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:91:
}
I would add blank spaces between these to be consistent with AboutVersionStrings
above.

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:131:
* Return the About Chrome value for profile path.
@return instead of Return

Looks like this file is split, so if you could just update all the ones that
start with Return or Returns with an @return

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:146:
* @return the mAcceptCookiesEnabled
that is an impressive comment.  I won't make you write all of these, but I think
in this case it is worse than having no comment (and the others like it).

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:250:
* Otherwise the change will be applied when {@link #sync} is called.
This comment seems no longer valid.

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:313:
* Sets the Contextual Search preference and sends a notification.
is this sends a notification still valid?

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:329:
*     user).
this should align with "whether" above

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:368:
* Return Network Predictions option. It is stored as an int and converted to an
enum.
The storage information seems to be very implementation specific and I think it
should be dropped here and below.

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:414:
public boolean isCrashReportManaged() {
should this go up with setCrashReporting?

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:462:
mClearBrowsingDataListener = listener;
Seems like we should be checking that mClearBrowsingDataListener is null here
otherwise if you had two callers it could never notify the previous one.

And to me it really seems like the listener should be associated with the
request and not saved as a member variable at all.

But I digress and this is outside of the scope of this change.

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:560:
* @return HashMap array of all the exceptions and their settings
s/HashMap array/List

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:562:
public ArrayList<PopupExceptionInfo> getPopupExceptions() {
This should be just returning a list instead of ArrayList

https://codereview.chromium.org/666063002/diff/40001/chrome/browser/android/p...
File chrome/browser/android/preferences/pref_service_bridge.cc (right):

https://codereview.chromium.org/666063002/diff/40001/chrome/browser/android/p...
chrome/browser/android/preferences/pref_service_bridge.cc:132: static void
EnsureConsistentGeolocationPreferences(Profile* profile) {
Should this go in the empty namespace above?  It's not a Native JNI method

https://codereview.chromium.org/666063002/diff/40001/chrome/browser/android/p...
chrome/browser/android/preferences/pref_service_bridge.cc:146: if
(defaultSetting == CONTENT_SETTING_ASK) return;
should be on the next line

https://codereview.chromium.org/666063002/diff/40001/chrome/browser/android/p...
chrome/browser/android/preferences/pref_service_bridge.cc:215: static void
EnsureConsistentProtectedMediaIdentifierPreferences(
same question as the other Ensure method

https://codereview.chromium.org/666063002/diff/40001/chrome/browser/android/p...
chrome/browser/android/preferences/pref_service_bridge.cc:229: if
(defaultSetting == CONTENT_SETTING_ASK) return;
return on the next line

https://codereview.chromium.org/666063002/diff/40001/chrome/browser/android/p...
chrome/browser/android/preferences/pref_service_bridge.cc:336:
browsing_data_remover->AddObserver(new ClearBrowsingDataObserver(env, obj));
Hmm...it even seems like the dataobserver could easily own the java object and
resolve my comment from the other file...in a different change of course.

https://codereview.chromium.org/666063002/diff/40001/chrome/browser/android/p...
chrome/browser/android/preferences/pref_service_bridge.cc:450: 
remove extra blank line

https://codereview.chromium.org/666063002/diff/40001/chrome/browser/android/p...
chrome/browser/android/preferences/pref_service_bridge.cc:588: void
PrependToAcceptLanguagesIfNecessary(std::string locale,
any reason this isn't in an empty namespace to avoid listing it in the header?

https://codereview.chromium.org/666063002/diff/40001/chrome/browser/android/p...
chrome/browser/android/preferences/pref_service_bridge.cc:653: }
add blank lines between all these methods for consistency with the rest of the
file

https://codereview.chromium.org/666063002/diff/40001/chrome/browser/android/p...
File chrome/browser/android/preferences/pref_service_bridge_unittest.cc (right):

https://codereview.chromium.org/666063002/diff/40001/chrome/browser/android/p...
chrome/browser/android/preferences/pref_service_bridge_unittest.cc:22:
TEST_F(PrefServiceBridgeTest, PrependToAcceptLanguagesAsNecessary) {
ah ha...for testing.  gotcha

[Yaron, 2014-10-21 17:36:33]

Patchset #4 (id:60001) has been deleted

[Yaron, 2014-10-21 17:40:14]

thanks for the review

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/NetworkPredictionOptions.java
(right):

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/NetworkPredictionOptions.java:6:

On 2014/10/21 16:57:20, Ted C wrote:
> only one blank line here

Done.

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/NetworkPredictionOptions.java:14:
*
On 2014/10/21 16:57:20, Ted C wrote:
> remove this empty line

Done.

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java
(right):

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:1:
// Copyright 2012 The Chromium Authors. All rights reserved.
On 2014/10/21 16:57:21, Ted C wrote:
> 2014

Done.

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:15:
* preferences should be grouped with their relevant functionality but this is a
grab-bag for other
On 2014/10/21 16:57:20, Ted C wrote:
> :-) so honest

Acknowledged.

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:59:
return mApplicationVersion;
On 2014/10/21 16:57:20, Ted C wrote:
> not that you want to change these as part of upstream, but for these simple
data
> objects where all members are final I think it is fine to just make the member
> variables public (drop the m prefix) and get rid of these accessors. 

Ya, that would probably be cleaner. Noted for future

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:91:
}
On 2014/10/21 16:57:21, Ted C wrote:
> I would add blank spaces between these to be consistent with
AboutVersionStrings
> above.

Done.

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:131:
* Return the About Chrome value for profile path.
On 2014/10/21 16:57:20, Ted C wrote:
> @return instead of Return
> 
> Looks like this file is split, so if you could just update all the ones that
> start with Return or Returns with an @return

Done.

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:146:
* @return the mAcceptCookiesEnabled
On 2014/10/21 16:57:21, Ted C wrote:
> that is an impressive comment.  I won't make you write all of these, but I
think
> in this case it is worse than having no comment (and the others like it).

Done.

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:250:
* Otherwise the change will be applied when {@link #sync} is called.
On 2014/10/21 16:57:21, Ted C wrote:
> This comment seems no longer valid.

Done.

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:313:
* Sets the Contextual Search preference and sends a notification.
On 2014/10/21 16:57:21, Ted C wrote:
> is this sends a notification still valid?

nah, I removed that junk prior to upstreaming :)

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:329:
*     user).
On 2014/10/21 16:57:21, Ted C wrote:
> this should align with "whether" above

Done.

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:368:
* Return Network Predictions option. It is stored as an int and converted to an
enum.
On 2014/10/21 16:57:20, Ted C wrote:
> The storage information seems to be very implementation specific and I think
it
> should be dropped here and below.

Done.

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:414:
public boolean isCrashReportManaged() {
On 2014/10/21 16:57:21, Ted C wrote:
> should this go up with setCrashReporting?

Done.

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:462:
mClearBrowsingDataListener = listener;
On 2014/10/21 16:57:21, Ted C wrote:
> Seems like we should be checking that mClearBrowsingDataListener is null here
> otherwise if you had two callers it could never notify the previous one.
> 
> And to me it really seems like the listener should be associated with the
> request and not saved as a member variable at all.
> 
> But I digress and this is outside of the scope of this change.

Agreed - added an assert. I even considered fixing it prior to upstreaming. Then
I decided that the perfect shouldn't be the enemy of the good :)

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:560:
* @return HashMap array of all the exceptions and their settings
On 2014/10/21 16:57:21, Ted C wrote:
> s/HashMap array/List

Done.

https://codereview.chromium.org/666063002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:562:
public ArrayList<PopupExceptionInfo> getPopupExceptions() {
On 2014/10/21 16:57:21, Ted C wrote:
> This should be just returning a list instead of ArrayList

Done.

https://codereview.chromium.org/666063002/diff/40001/chrome/browser/android/p...
File chrome/browser/android/preferences/pref_service_bridge.cc (right):

https://codereview.chromium.org/666063002/diff/40001/chrome/browser/android/p...
chrome/browser/android/preferences/pref_service_bridge.cc:132: static void
EnsureConsistentGeolocationPreferences(Profile* profile) {
On 2014/10/21 16:57:21, Ted C wrote:
> Should this go in the empty namespace above?  It's not a Native JNI method

Done.

https://codereview.chromium.org/666063002/diff/40001/chrome/browser/android/p...
chrome/browser/android/preferences/pref_service_bridge.cc:146: if
(defaultSetting == CONTENT_SETTING_ASK) return;
On 2014/10/21 16:57:21, Ted C wrote:
> should be on the next line

Done.

https://codereview.chromium.org/666063002/diff/40001/chrome/browser/android/p...
chrome/browser/android/preferences/pref_service_bridge.cc:215: static void
EnsureConsistentProtectedMediaIdentifierPreferences(
On 2014/10/21 16:57:21, Ted C wrote:
> same question as the other Ensure method

Done.

https://codereview.chromium.org/666063002/diff/40001/chrome/browser/android/p...
chrome/browser/android/preferences/pref_service_bridge.cc:229: if
(defaultSetting == CONTENT_SETTING_ASK) return;
On 2014/10/21 16:57:21, Ted C wrote:
> return on the next line

Done.

https://codereview.chromium.org/666063002/diff/40001/chrome/browser/android/p...
chrome/browser/android/preferences/pref_service_bridge.cc:336:
browsing_data_remover->AddObserver(new ClearBrowsingDataObserver(env, obj));
On 2014/10/21 16:57:21, Ted C wrote:
> Hmm...it even seems like the dataobserver could easily own the java object and
> resolve my comment from the other file...in a different change of course.

Acknowledged.

https://codereview.chromium.org/666063002/diff/40001/chrome/browser/android/p...
chrome/browser/android/preferences/pref_service_bridge.cc:450: 
On 2014/10/21 16:57:21, Ted C wrote:
> remove extra blank line

Done.

https://codereview.chromium.org/666063002/diff/40001/chrome/browser/android/p...
chrome/browser/android/preferences/pref_service_bridge.cc:588: void
PrependToAcceptLanguagesIfNecessary(std::string locale,
On 2014/10/21 16:57:21, Ted C wrote:
> any reason this isn't in an empty namespace to avoid listing it in the header?

Acknowledged.

https://codereview.chromium.org/666063002/diff/40001/chrome/browser/android/p...
chrome/browser/android/preferences/pref_service_bridge.cc:653: }
On 2014/10/21 16:57:21, Ted C wrote:
> add blank lines between all these methods for consistency with the rest of the
> file

Done.

https://codereview.chromium.org/666063002/diff/40001/chrome/browser/android/p...
File chrome/browser/android/preferences/pref_service_bridge_unittest.cc (right):

https://codereview.chromium.org/666063002/diff/40001/chrome/browser/android/p...
chrome/browser/android/preferences/pref_service_bridge_unittest.cc:22:
TEST_F(PrefServiceBridgeTest, PrependToAcceptLanguagesAsNecessary) {
On 2014/10/21 16:57:21, Ted C wrote:
> ah ha...for testing.  gotcha

Acknowledged.

[Yaron, 2014-10-21 17:40:25]

The CQ bit was checked by yfriedman@chromium.org

[commit-bot: I haz the power, 2014-10-21 17:41:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/666063002/90001

[commit-bot: I haz the power, 2014-10-21 19:06:13]

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

[commit-bot: I haz the power, 2014-10-21 19:06:14]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Yaron, 2014-10-21 19:49:14]

The CQ bit was checked by yfriedman@chromium.org

[commit-bot: I haz the power, 2014-10-21 19:50:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/666063002/90001

[commit-bot: I haz the power, 2014-10-21 20:42:16]

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

[commit-bot: I haz the power, 2014-10-21 20:42:17]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Yaron, 2014-10-21 20:45:46]

The CQ bit was checked by yfriedman@chromium.org

[commit-bot: I haz the power, 2014-10-21 20:47:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/666063002/90001

[Yaron, 2014-10-21 22:34:19]

The CQ bit was unchecked by yfriedman@chromium.org

[Yaron, 2014-10-21 22:34:22]

The CQ bit was checked by yfriedman@chromium.org

[commit-bot: I haz the power, 2014-10-21 22:35:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/666063002/90001

[commit-bot: I haz the power, 2014-10-21 22:38:48]

Committed patchset #5 (id:90001)

[commit-bot: I haz the power, 2014-10-21 22:39:55]

Patchset 5 (id:??) landed as
https://crrev.com/433afcdd00085ccb0b9ab89143fe6410f9d0af80
Cr-Commit-Position: refs/heads/master@{#300562}