[Android] Migrate javascript settings to a content setting from a pref.

[Android] Migrate javascript settings to a content setting from a pref.

This also adds a preferences migration function and tracking system to
handle future changes.

BUG=454063
Committed: https://crrev.com/c404e85f318501565ebd0c8b3541fbc2e260b158
Cr-Commit-Position: refs/heads/master@{#316023}

[Ted C, 2015-02-11 21:48:22]

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

[Ted C, 2015-02-11 21:48:23]

PTAL

This will keep Android consistent with how desktop supports javascript

[newt (away), 2015-02-12 00:48:59]

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

https://codereview.chromium.org/920583002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:172:
preferences.edit().putInt(MIGRATION_PREF_KEY, currentVersion).commit();
Why not put this at the end of the loop? Migrating prefs multiple times (in the
case that we crash during migration) should be safe, right? Then you can just
store MIGRATION_CURRENT_VERSION rather than currentVersion.

Also: how about this structure?

if (currentVersion == MIGRATION_CURRENT_VERSION) return;
if (currentVersion <= 0) {
  // migrate to 1
}
if (currentVersion <= 1) {
  // migrate to 2
}
...
preferences.edit().putInt(MIGRATION_PREF_KEY,
MIGRATION_CURRENT_VERSION).commit();

https://codereview.chromium.org/920583002/diff/1/chrome/browser/android/prefe...
File chrome/browser/android/preferences/pref_service_bridge.cc (right):

https://codereview.chromium.org/920583002/diff/1/chrome/browser/android/prefe...
chrome/browser/android/preferences/pref_service_bridge.cc:435: bool
javascript_enabled = GetPrefService()->GetBoolean(
Can't you do something like javascript_pref->GetBooleanValue()?

[Ted C, 2015-02-12 01:01:10]

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

https://codereview.chromium.org/920583002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:172:
preferences.edit().putInt(MIGRATION_PREF_KEY, currentVersion).commit();
On 2015/02/12 00:48:59, newt wrote:
> Why not put this at the end of the loop? Migrating prefs multiple times (in
the
> case that we crash during migration) should be safe, right? Then you can just
> store MIGRATION_CURRENT_VERSION rather than currentVersion.
> 
> Also: how about this structure?
> 
> if (currentVersion == MIGRATION_CURRENT_VERSION) return;
> if (currentVersion <= 0) {
>   // migrate to 1
> }
> if (currentVersion <= 1) {
>   // migrate to 2
> }
> ...
> preferences.edit().putInt(MIGRATION_PREF_KEY,
> MIGRATION_CURRENT_VERSION).commit();

Done...

I was very conflicted on the style, so I'm fine with this.

https://codereview.chromium.org/920583002/diff/1/chrome/browser/android/prefe...
File chrome/browser/android/preferences/pref_service_bridge.cc (right):

https://codereview.chromium.org/920583002/diff/1/chrome/browser/android/prefe...
chrome/browser/android/preferences/pref_service_bridge.cc:435: bool
javascript_enabled = GetPrefService()->GetBoolean(
On 2015/02/12 00:48:59, newt wrote:
> Can't you do something like javascript_pref->GetBooleanValue()?

Done.

[newt (away), 2015-02-12 01:25:02]

lgtm after debugging code is removed

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

https://codereview.chromium.org/920583002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:163:
if (currentVersion == MIGRATION_CURRENT_VERSION) return;
nit: I'd put this if check first, since it's the common/quick case

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

https://codereview.chromium.org/920583002/diff/20001/chrome/browser/android/p...
chrome/browser/android/preferences/pref_service_bridge.cc:436: bool retval =
javascript_pref->GetValue()->GetAsBoolean(&javascript_enabled);
Leftover debugging code?

[Ted C, 2015-02-12 17:18:20]

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

https://codereview.chromium.org/920583002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:163:
if (currentVersion == MIGRATION_CURRENT_VERSION) return;
On 2015/02/12 01:25:02, newt wrote:
> nit: I'd put this if check first, since it's the common/quick case

Done.

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

https://codereview.chromium.org/920583002/diff/20001/chrome/browser/android/p...
chrome/browser/android/preferences/pref_service_bridge.cc:436: bool retval =
javascript_pref->GetValue()->GetAsBoolean(&javascript_enabled);
On 2015/02/12 01:25:02, newt wrote:
> Leftover debugging code?

Actually no, the return value ensures that the pref is still stored as a boolean
(the GetAs<...> functions return false if the value type can not be converted to
what you are asking for).

This mimics GetPrefService()->GetBoolean()'s logic internally.

[Ted C, 2015-02-12 17:18:29]

New patchsets have been uploaded after l-g-t-m from newt@chromium.org

[newt (away), 2015-02-12 18:50:34]

On 2015/02/12 17:18:20, Ted C wrote:
>
https://codereview.chromium.org/920583002/diff/20001/chrome/android/java/src/...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java
> (right):
> 
>
https://codereview.chromium.org/920583002/diff/20001/chrome/android/java/src/...
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:163:
> if (currentVersion == MIGRATION_CURRENT_VERSION) return;
> On 2015/02/12 01:25:02, newt wrote:
> > nit: I'd put this if check first, since it's the common/quick case
> 
> Done.
> 
>
https://codereview.chromium.org/920583002/diff/20001/chrome/browser/android/p...
> File chrome/browser/android/preferences/pref_service_bridge.cc (right):
> 
>
https://codereview.chromium.org/920583002/diff/20001/chrome/browser/android/p...
> chrome/browser/android/preferences/pref_service_bridge.cc:436: bool retval =
> javascript_pref->GetValue()->GetAsBoolean(&javascript_enabled);
> On 2015/02/12 01:25:02, newt wrote:
> > Leftover debugging code?
> 
> Actually no, the return value ensures that the pref is still stored as a
boolean
> (the GetAs<...> functions return false if the value type can not be converted
to
> what you are asking for).
> 
> This mimics GetPrefService()->GetBoolean()'s logic internally.

lgtm

[Ted C, 2015-02-12 19:01:02]

The CQ bit was checked by tedchoc@chromium.org

[commit-bot: I haz the power, 2015-02-12 19:01:23]

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

[commit-bot: I haz the power, 2015-02-12 19:24:56]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-12 19:25:28]

Patchset 3 (id:??) landed as
https://crrev.com/c404e85f318501565ebd0c8b3541fbc2e260b158
Cr-Commit-Position: refs/heads/master@{#316023}

[Ted C, 2015-02-12 20:52:57]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/923573003/ by tedchoc@chromium.org.

The reason for reverting is: This breaks the
ContentSettings#getJavaScriptEnabled, so that dependency needs to be cleared
up..