Add auto sign in slider in Clank settings.

Add auto sign in slider in Clank settings.

BUG=454815
Committed: https://crrev.com/a5b4ed96ea46f33bea7748904881e3461936b8f2
Cr-Commit-Position: refs/heads/master@{#321390}

[melandory, 2015-03-05 15:28:52]

Patchset #1 (id:1) has been deleted

[melandory, 2015-03-09 09:09:28]

Patchset #8 (id:160001) has been deleted

[melandory, 2015-03-09 09:09:38]

Patchset #7 (id:140001) has been deleted

[melandory, 2015-03-10 08:42:56]

Patchset #10 (id:240001) has been deleted

[melandory, 2015-03-10 08:43:03]

Patchset #9 (id:220001) has been deleted

[melandory, 2015-03-10 08:43:10]

Patchset #8 (id:200001) has been deleted

[melandory, 2015-03-10 08:43:20]

Patchset #7 (id:180001) has been deleted

[melandory, 2015-03-10 08:43:30]

Patchset #6 (id:120001) has been deleted

[melandory, 2015-03-10 08:43:39]

Patchset #5 (id:100001) has been deleted

[melandory, 2015-03-10 08:43:48]

Patchset #4 (id:80001) has been deleted

[melandory, 2015-03-10 12:07:58]

Patchset #3 (id:60001) has been deleted

[melandory, 2015-03-10 12:08:05]

Patchset #2 (id:40001) has been deleted

[melandory, 2015-03-10 12:08:14]

Patchset #1 (id:20001) has been deleted

[melandory, 2015-03-10 17:16:38]

Patchset #1 (id:260001) has been deleted

[melandory, 2015-03-10 17:16:54]

Patchset #1 (id:280001) has been deleted

[melandory, 2015-03-10 17:27:16]

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

[melandory, 2015-03-10 17:27:17]

tedchoc@chromium.org: Please review changes in 

content/public/android/java/src/org/chromium/content/common/ContentSwitches.java

newt@chromium.org: Please review changes in 

chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ManageSavedPasswordsPreferences.java		
chrome/android/java/strings/android_chrome_strings.grd	
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavedPasswordsPreferencesTest.java		
chrome/browser/android/preferences/pref_service_bridge.cc

[newt (away), 2015-03-11 17:41:40]

Looks generally good. One question:
 - Are there mocks for this? If not, has this at least been run past the
settings designers (rolfe/sgabriel)? Having two switches on the same settings
page seems questionable to me, so I'd like their input.

https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ManageSavedPasswordsPreferences.java
(right):

https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ManageSavedPasswordsPreferences.java:281:
private void createAutoSignInSwitch(boolean isEnabled) {
This should probably be a checkbox, not a switch. Normally, there's only one
switch per settings page and it enables/disables that entire feature and
disables or changes the effect of other settings on the page.

https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ManageSavedPasswordsPreferences.java:287:
mAutoSignInSwitch.setOrder(ORDER_SWITCH);
Don't use the same order value. Create a new ORDER_ constant for this switch.

https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ManageSavedPasswordsPreferences.java:297:
mAutoSignInSwitch.setTitle(R.string.passwords_auto_signin_title);
nit: the title is important and simple. move this up around the call to
setKey(). same for the save password title.

https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ManageSavedPasswordsPreferences.java:305:
mAutoSignInSwitch.setChecked(isEnabled);
I'd remove the isEnabled parameter and just call
"PrefServiceBridge.getInstance().isPasswordManagerAutoSigninEnabled()" right
here. You don't pass in other important values (like isManaged), so why pass in
isEnabled?

https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ManageSavedPasswordsPreferences.java:328:
mSavePasswordsSwitch.setDrawDivider(!shouldDisplayLink);
This logic needs updating. mSavePasswordsSwitch.setDrawDivider() should be
called with "true" if neither the manage account link nor the auto-sign-in
switch are present. If either of those is present, it should be called with
false.

https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:229: <message
name="IDS_PASSWORDS_AUTO_SIGNIN_TITLE" desc="Title for 'Auto sign-in' checkbox
in the password dialog">
"Title for checkbox to enable automatically signing the user in to websites."

https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:233: Automatically sign
in to websites using stored credential. When the feature is off, you'll be asked
for verification every time before signing in to a website.
s/credential/credentials ? Where did this string come from?

https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:235: <message
name="IDS_PREFS_SAVED_PASSWORDS" desc="Title for the Saved Passwords
preferences. [CHAR-LIMIT=32]">
nit: move "Save passwords" string above the new strings

https://codereview.chromium.org/978913004/diff/300001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavedPasswordsPreferencesTest.java
(right):

https://codereview.chromium.org/978913004/diff/300001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavedPasswordsPreferencesTest.java:86:
* disables
fix wrapping

https://codereview.chromium.org/978913004/diff/300001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavedPasswordsPreferencesTest.java:107:
ManageSavedPasswordsPreferences autoSignInPrefs =
I'd call this passwordPrefs

[melandory, 2015-03-11 22:04:30]

On 2015/03/11 17:41:40, newt wrote:
> Looks generally good. One question:
>  - Are there mocks for this? If not, has this at least been run past the
> settings designers (rolfe/sgabriel)? Having two switches on the same settings
> page seems questionable to me, so I'd like their input.
https://folio.googleplex.com/yolo/00_latest/Settings%20Android#%2Fyolo_settin...
"Passwords & connected accounts" goes to web page which doesn't exists yet and
when it'll be implemented
settings switches won't be there. But not only this we're thinking that having
native page is better solution, 
so since web page on mocks has both switches on same setting page I implemented
it in a same way here.

If you think that this argumentation isn't enough, I'll contact settings
designers and ask their opinion.

>
https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ManageSavedPasswordsPreferences.java
> (right):
> 
>
https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ManageSavedPasswordsPreferences.java:281:
> private void createAutoSignInSwitch(boolean isEnabled) {
> This should probably be a checkbox, not a switch. Normally, there's only one
> switch per settings page and it enables/disables that entire feature and
> disables or changes the effect of other settings on the page.
> 
>
https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ManageSavedPasswordsPreferences.java:287:
> mAutoSignInSwitch.setOrder(ORDER_SWITCH);
> Don't use the same order value. Create a new ORDER_ constant for this switch.
> 
>
https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ManageSavedPasswordsPreferences.java:297:
> mAutoSignInSwitch.setTitle(R.string.passwords_auto_signin_title);
> nit: the title is important and simple. move this up around the call to
> setKey(). same for the save password title.
> 
>
https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ManageSavedPasswordsPreferences.java:305:
> mAutoSignInSwitch.setChecked(isEnabled);
> I'd remove the isEnabled parameter and just call
> "PrefServiceBridge.getInstance().isPasswordManagerAutoSigninEnabled()" right
> here. You don't pass in other important values (like isManaged), so why pass
in
> isEnabled?
> 
>
https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ManageSavedPasswordsPreferences.java:328:
> mSavePasswordsSwitch.setDrawDivider(!shouldDisplayLink);
> This logic needs updating. mSavePasswordsSwitch.setDrawDivider() should be
> called with "true" if neither the manage account link nor the auto-sign-in
> switch are present. If either of those is present, it should be called with
> false.
> 
>
https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/str...
> File chrome/android/java/strings/android_chrome_strings.grd (right):
> 
>
https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/str...
> chrome/android/java/strings/android_chrome_strings.grd:229: <message
> name="IDS_PASSWORDS_AUTO_SIGNIN_TITLE" desc="Title for 'Auto sign-in' checkbox
> in the password dialog">
> "Title for checkbox to enable automatically signing the user in to websites."
> 
>
https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/str...
> chrome/android/java/strings/android_chrome_strings.grd:233: Automatically sign
> in to websites using stored credential. When the feature is off, you'll be
asked
> for verification every time before signing in to a website.
> s/credential/credentials ? Where did this string come from?
> 
>
https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/str...
> chrome/android/java/strings/android_chrome_strings.grd:235: <message
> name="IDS_PREFS_SAVED_PASSWORDS" desc="Title for the Saved Passwords
> preferences. [CHAR-LIMIT=32]">
> nit: move "Save passwords" string above the new strings
> 
>
https://codereview.chromium.org/978913004/diff/300001/chrome/android/javatest...
> File
>
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavedPasswordsPreferencesTest.java
> (right):
> 
>
https://codereview.chromium.org/978913004/diff/300001/chrome/android/javatest...
>
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavedPasswordsPreferencesTest.java:86:
> * disables
> fix wrapping
> 
>
https://codereview.chromium.org/978913004/diff/300001/chrome/android/javatest...
>
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavedPasswordsPreferencesTest.java:107:
> ManageSavedPasswordsPreferences autoSignInPrefs =
> I'd call this passwordPrefs

[newt (away), 2015-03-12 20:46:27]

On 2015/03/11 22:04:30, melandory wrote:
> On 2015/03/11 17:41:40, newt wrote:
> > Looks generally good. One question:
> >  - Are there mocks for this? If not, has this at least been run past the
> > settings designers (rolfe/sgabriel)? Having two switches on the same
settings
> > page seems questionable to me, so I'd like their input.
>
https://folio.googleplex.com/yolo/00_latest/Settings%20Android#%2Fyolo_settin...
> "Passwords & connected accounts" goes to web page which doesn't exists yet and
> when it'll be implemented
> settings switches won't be there. But not only this we're thinking that having
> native page is better solution, 
> so since web page on mocks has both switches on same setting page I
implemented
> it in a same way here.
> 
> If you think that this argumentation isn't enough, I'll contact settings
> designers and ask their opinion.

I think this is OK then.

[melandory, 2015-03-16 15:10:27]

https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ManageSavedPasswordsPreferences.java
(right):

https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ManageSavedPasswordsPreferences.java:281:
private void createAutoSignInSwitch(boolean isEnabled) {
On 2015/03/11 17:41:40, newt wrote:
> This should probably be a checkbox, not a switch. Normally, there's only one
> switch per settings page and it enables/disables that entire feature and
> disables or changes the effect of other settings on the page.

Done.

https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ManageSavedPasswordsPreferences.java:287:
mAutoSignInSwitch.setOrder(ORDER_SWITCH);
On 2015/03/11 17:41:40, newt wrote:
> Don't use the same order value. Create a new ORDER_ constant for this switch.

Done.

https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ManageSavedPasswordsPreferences.java:297:
mAutoSignInSwitch.setTitle(R.string.passwords_auto_signin_title);
On 2015/03/11 17:41:40, newt wrote:
> nit: the title is important and simple. move this up around the call to
> setKey(). same for the save password title.

Done.

https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ManageSavedPasswordsPreferences.java:305:
mAutoSignInSwitch.setChecked(isEnabled);
On 2015/03/11 at 17:41:40, newt wrote:
> I'd remove the isEnabled parameter and just call
"PrefServiceBridge.getInstance().isPasswordManagerAutoSigninEnabled()" right
here. You don't pass in other important values (like isManaged), so why pass in
isEnabled?

https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ManageSavedPasswordsPreferences.java:328:
mSavePasswordsSwitch.setDrawDivider(!shouldDisplayLink);
On 2015/03/11 17:41:40, newt wrote:
> This logic needs updating. mSavePasswordsSwitch.setDrawDivider() should be
> called with "true" if neither the manage account link nor the auto-sign-in
> switch are present. If either of those is present, it should be called with
> false.

Done.

https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:229: <message
name="IDS_PASSWORDS_AUTO_SIGNIN_TITLE" desc="Title for 'Auto sign-in' checkbox
in the password dialog">
On 2015/03/11 17:41:40, newt wrote:
> "Title for checkbox to enable automatically signing the user in to websites."

Done.

https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:233: Automatically sign
in to websites using stored credential. When the feature is off, you'll be asked
for verification every time before signing in to a website.
On 2015/03/11 17:41:40, newt wrote:
> s/credential/credentials ? Where did this string come from?

It's slightly modified string from mocks. Original string was saying: 
"Automatically sign in to apps and websites connected to your Account."

"Account" was removed because it restricts this feature only to sync users.
Desktop settings UI uses same string.

https://codereview.chromium.org/978913004/diff/300001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:235: <message
name="IDS_PREFS_SAVED_PASSWORDS" desc="Title for the Saved Passwords
preferences. [CHAR-LIMIT=32]">
On 2015/03/11 17:41:40, newt wrote:
> nit: move "Save passwords" string above the new strings

Done.

https://codereview.chromium.org/978913004/diff/300001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavedPasswordsPreferencesTest.java
(right):

https://codereview.chromium.org/978913004/diff/300001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavedPasswordsPreferencesTest.java:86:
* disables
On 2015/03/11 17:41:40, newt wrote:
> fix wrapping

Done.

https://codereview.chromium.org/978913004/diff/300001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavedPasswordsPreferencesTest.java:107:
ManageSavedPasswordsPreferences autoSignInPrefs =
On 2015/03/11 17:41:40, newt wrote:
> I'd call this passwordPrefs

Done.

[melandory, 2015-03-16 15:10:38]

Patchset #2 (id:320001) has been deleted

[newt (away), 2015-03-17 05:19:10]

lgtm after nit

https://codereview.chromium.org/978913004/diff/360001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ManageSavedPasswordsPreferences.java
(right):

https://codereview.chromium.org/978913004/diff/360001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ManageSavedPasswordsPreferences.java:61:
private static final int ORDER_CHECKBOX = 1;
nit: call this something more meaningful, e.g. ORDER_AUTO_SIGNIN_CHECKBOX

[melandory, 2015-03-17 09:11:16]

https://codereview.chromium.org/978913004/diff/360001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ManageSavedPasswordsPreferences.java
(right):

https://codereview.chromium.org/978913004/diff/360001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ManageSavedPasswordsPreferences.java:61:
private static final int ORDER_CHECKBOX = 1;
On 2015/03/17 05:19:10, newt wrote:
> nit: call this something more meaningful, e.g. ORDER_AUTO_SIGNIN_CHECKBOX

Done.

[melandory, 2015-03-17 09:32:53]

Patchset #4 (id:380001) has been deleted

[melandory, 2015-03-19 13:34:45]

On 2015/03/17 09:11:16, melandory wrote:
>
https://codereview.chromium.org/978913004/diff/360001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ManageSavedPasswordsPreferences.java
> (right):
> 
>
https://codereview.chromium.org/978913004/diff/360001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ManageSavedPasswordsPreferences.java:61:
> private static final int ORDER_CHECKBOX = 1;
> On 2015/03/17 05:19:10, newt wrote:
> > nit: call this something more meaningful, e.g. ORDER_AUTO_SIGNIN_CHECKBOX
> 
> Done.

tedchoc@ can you look at
content/public/android/java/src/org/chromium/content/common/ContentSwitches.java?


Thanks in advance!

[Ted C, 2015-03-19 15:53:34]

On 2015/03/19 13:34:45, melandory wrote:
> On 2015/03/17 09:11:16, melandory wrote:
> >
>
https://codereview.chromium.org/978913004/diff/360001/chrome/android/java/src...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ManageSavedPasswordsPreferences.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/978913004/diff/360001/chrome/android/java/src...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ManageSavedPasswordsPreferences.java:61:
> > private static final int ORDER_CHECKBOX = 1;
> > On 2015/03/17 05:19:10, newt wrote:
> > > nit: call this something more meaningful, e.g. ORDER_AUTO_SIGNIN_CHECKBOX
> > 
> > Done.
> 
> tedchoc@ can you look at
>
content/public/android/java/src/org/chromium/content/common/ContentSwitches.java?
> 
> 
> Thanks in advance!

ContentSwitches - lgtm

[melandory, 2015-03-19 17:14:51]

The CQ bit was checked by melandory@chromium.org

[melandory, 2015-03-19 17:14:51]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org
Link to the patchset: https://codereview.chromium.org/978913004/#ps400001
(title: " ")

[commit-bot: I haz the power, 2015-03-19 17:15:10]

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

[commit-bot: I haz the power, 2015-03-19 17:58:55]

Committed patchset #4 (id:400001)

[commit-bot: I haz the power, 2015-03-19 17:59:46]

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