Copy and view saved passwords.

Copy and view saved passwords.

Implements the ability to copy and view saved passwords if the user has
a lock. Before allow view or copy of passwords user is prompted with the
lock. The functionality is unavailable if there is no device level
lock.

The implementation in this CL has following shortcomings:
* "Nice viewer" is not available for the users without lock, still old version is used.
* There is no way to delete the credentials in the new view.
* In case password was viewed (eye icon clicked), the app switcher thumbnail contains the password in plain text.
* Wrong lock screen message.
* UI is not aligned with mocks.

Screenshots are attached to the bug.

The original CL: https://codereview.chromium.org/2067323004/

BUG=619868
Review-Url: https://codereview.chromium.org/2900173002
Cr-Commit-Position: refs/heads/master@{#478929}
Committed: https://chromium.googlesource.com/chromium/src/+/2a595fea49f5e6ed6ed9caae2b954d25798226ba

[melandory, 2017-05-23 20:38:39]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 20:39:19]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[melandory, 2017-05-23 21:00:11]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 21:00:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[melandory, 2017-05-23 21:01:08]

Description was changed from

==========
Copy and view saved passwords.

Implements the ability to copy and view saved passwords if the user has
a lock. Before allow view or copy of passwords user is prompted with the
lock. The functionality is anavailable if there is no device level
lock.

Screenshots are attached to the bug.

BUG=619868
==========

to

==========
Copy and view saved passwords.

Implements the ability to copy and view saved passwords if the user has
a lock. Before allow view or copy of passwords user is prompted with the
lock. The functionality is anavailable if there is no device level
lock.

Screenshots are attached to the bug.

The original CL: https://codereview.chromium.org/2067323004/

BUG=619868
==========

[melandory, 2017-05-23 21:03:58]

Description was changed from

==========
Copy and view saved passwords.

Implements the ability to copy and view saved passwords if the user has
a lock. Before allow view or copy of passwords user is prompted with the
lock. The functionality is anavailable if there is no device level
lock.

Screenshots are attached to the bug.

The original CL: https://codereview.chromium.org/2067323004/

BUG=619868
==========

to

==========
Copy and view saved passwords.

Implements the ability to copy and view saved passwords if the user has
a lock. Before allow view or copy of passwords user is prompted with the
lock. The functionality is anavailable if there is no device level
lock.

This CL doesn't implement:
* Nice viewer for users without lock, still old version is userd
* "Promo" for the lock

Screenshots are attached to the bug.

The original CL: https://codereview.chromium.org/2067323004/

BUG=619868
==========

[commit-bot: I haz the power, 2017-05-23 23:05:26]

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

[commit-bot: I haz the power, 2017-05-23 23:05:27]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[melandory, 2017-05-24 10:32:00]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 10:32:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[melandory, 2017-05-24 10:33:55]

Patchset #1 (id:1) has been deleted

[melandory, 2017-05-24 10:34:03]

Patchset #1 (id:20001) has been deleted

[melandory, 2017-05-24 11:08:35]

Description was changed from

==========
Copy and view saved passwords.

Implements the ability to copy and view saved passwords if the user has
a lock. Before allow view or copy of passwords user is prompted with the
lock. The functionality is anavailable if there is no device level
lock.

This CL doesn't implement:
* Nice viewer for users without lock, still old version is userd
* "Promo" for the lock

Screenshots are attached to the bug.

The original CL: https://codereview.chromium.org/2067323004/

BUG=619868
==========

to

==========
Copy and view saved passwords.

Implements the ability to copy and view saved passwords if the user has
a lock. Before allow view or copy of passwords user is prompted with the
lock. The functionality is unavailable if there is no device level
lock.

This CL doesn't implement:
* Nice viewer for users without lock, still old version is used
* "Promo" for the lock

Screenshots are attached to the bug.

The original CL: https://codereview.chromium.org/2067323004/

BUG=619868
==========

[commit-bot: I haz the power, 2017-05-24 12:29:13]

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

[commit-bot: I haz the power, 2017-05-24 12:29:14]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[melandory, 2017-05-24 13:54:15]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 13:54:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 13:57:05]

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

[commit-bot: I haz the power, 2017-05-24 13:57:06]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[melandory, 2017-05-26 09:52:07]

Description was changed from

==========
Copy and view saved passwords.

Implements the ability to copy and view saved passwords if the user has
a lock. Before allow view or copy of passwords user is prompted with the
lock. The functionality is unavailable if there is no device level
lock.

This CL doesn't implement:
* Nice viewer for users without lock, still old version is used
* "Promo" for the lock

Screenshots are attached to the bug.

The original CL: https://codereview.chromium.org/2067323004/

BUG=619868
==========

to

==========
Copy and view saved passwords.

Implements the ability to copy and view saved passwords if the user has
a lock. Before allow view or copy of passwords user is prompted with the
lock. The functionality is unavailable if there is no device level
lock.

This CL doesn't implement:
* Nice viewer for users without lock, still old version is used
* "Promo" for the lock
* There is no way to delete the credentials.

Screenshots are attached to the bug.

The original CL: https://codereview.chromium.org/2067323004/

BUG=619868
==========

[melandory, 2017-05-26 10:17:02]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 10:17:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 10:42:58]

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

[commit-bot: I haz the power, 2017-05-26 10:42:59]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[melandory, 2017-05-29 14:55:51]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-29 14:56:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[melandory, 2017-05-29 14:56:44]

Patchset #1 (id:40001) has been deleted

[melandory, 2017-05-29 14:56:53]

Patchset #1 (id:60001) has been deleted

[melandory, 2017-05-29 14:57:02]

Patchset #1 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-05-29 14:58:24]

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

[commit-bot: I haz the power, 2017-05-29 14:58:25]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[melandory, 2017-05-29 15:09:03]

Description was changed from

==========
Copy and view saved passwords.

Implements the ability to copy and view saved passwords if the user has
a lock. Before allow view or copy of passwords user is prompted with the
lock. The functionality is unavailable if there is no device level
lock.

This CL doesn't implement:
* Nice viewer for users without lock, still old version is used
* "Promo" for the lock
* There is no way to delete the credentials.

Screenshots are attached to the bug.

The original CL: https://codereview.chromium.org/2067323004/

BUG=619868
==========

to

==========
Copy and view saved passwords.

Implements the ability to copy and view saved passwords if the user has
a lock. Before allow view or copy of passwords user is prompted with the
lock. The functionality is unavailable if there is no device level
lock.

This CL doesn't implement:
* Nice viewer for users without lock, still old version is used
* "Promo" for the lock
* There is no way to delete the credentials.
* In case password was viewed, the app switcher thumbnail contains the password
in plain text.

Screenshots are attached to the bug.

The original CL: https://codereview.chromium.org/2067323004/

BUG=619868
==========

[melandory, 2017-05-29 15:09:21]

Description was changed from

==========
Copy and view saved passwords.

Implements the ability to copy and view saved passwords if the user has
a lock. Before allow view or copy of passwords user is prompted with the
lock. The functionality is unavailable if there is no device level
lock.

This CL doesn't implement:
* Nice viewer for users without lock, still old version is used
* "Promo" for the lock
* There is no way to delete the credentials.
* In case password was viewed, the app switcher thumbnail contains the password
in plain text.

Screenshots are attached to the bug.

The original CL: https://codereview.chromium.org/2067323004/

BUG=619868
==========

to

==========
Copy and view saved passwords.

Implements the ability to copy and view saved passwords if the user has
a lock. Before allow view or copy of passwords user is prompted with the
lock. The functionality is unavailable if there is no device level
lock.

This CL doesn't implement:
* Nice viewer for users without lock, still old version is used
* "Promo" for the lock
* There is no way to delete the credentials.
* In case password was viewed (eye icon clicked), the app switcher thumbnail
contains the password in plain text.

Screenshots are attached to the bug.

The original CL: https://codereview.chromium.org/2067323004/

BUG=619868
==========

[melandory, 2017-05-29 15:13:36]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-29 15:13:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-29 16:19:52]

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

[commit-bot: I haz the power, 2017-05-29 16:19:53]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[melandory, 2017-05-30 08:52:54]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-30 08:53:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-30 09:57:17]

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

[commit-bot: I haz the power, 2017-05-30 09:57:18]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[melandory, 2017-05-30 11:54:00]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-30 11:54:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-30 13:06:56]

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

[commit-bot: I haz the power, 2017-05-30 13:06:57]

Dry run: This issue passed the CQ dry run.

[melandory, 2017-05-30 13:08:00]

Patchset #3 (id:140001) has been deleted

[melandory, 2017-05-30 13:09:38]

Description was changed from

==========
Copy and view saved passwords.

Implements the ability to copy and view saved passwords if the user has
a lock. Before allow view or copy of passwords user is prompted with the
lock. The functionality is unavailable if there is no device level
lock.

This CL doesn't implement:
* Nice viewer for users without lock, still old version is used
* "Promo" for the lock
* There is no way to delete the credentials.
* In case password was viewed (eye icon clicked), the app switcher thumbnail
contains the password in plain text.

Screenshots are attached to the bug.

The original CL: https://codereview.chromium.org/2067323004/

BUG=619868
==========

to

==========
Copy and view saved passwords.

Implements the ability to copy and view saved passwords if the user has
a lock. Before allow view or copy of passwords user is prompted with the
lock. The functionality is unavailable if there is no device level
lock.

The implementation in this CL has following shortcomings:
* "Nice viewer" is not available for the users without lock, still old version
is used.
* "Promo" for the lock isn't shown.
* There is no way to delete the credentials in the new view.
* In case password was viewed (eye icon clicked), the app switcher thumbnail
contains the password in plain text.

Screenshots are attached to the bug.

The original CL: https://codereview.chromium.org/2067323004/

BUG=619868
==========

[melandory, 2017-05-30 13:09:50]

Patchset #1 (id:100001) has been deleted

[melandory, 2017-05-30 13:10:03]

Patchset #1 (id:120001) has been deleted

[melandory, 2017-05-30 14:49:21]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-30 14:49:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[melandory, 2017-05-30 16:08:21]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-30 16:08:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[melandory, 2017-05-30 16:10:15]

Patchset #2 (id:180001) has been deleted

[melandory, 2017-05-30 16:14:07]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-30 16:14:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[melandory, 2017-05-30 16:14:32]

Patchset #2 (id:200001) has been deleted

[melandory, 2017-05-30 16:17:43]

melandory@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[melandory, 2017-05-30 16:17:44]

PTAL, thanks!

[commit-bot: I haz the power, 2017-05-30 16:51:22]

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

[commit-bot: I haz the power, 2017-05-30 16:51:23]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[melandory, 2017-05-31 08:15:20]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 08:15:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 09:38:12]

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

[commit-bot: I haz the power, 2017-05-31 09:38:13]

Dry run: This issue passed the CQ dry run.

[melandory, 2017-05-31 09:45:41]

Patchset #2 (id:220001) has been deleted

[melandory, 2017-06-01 11:43:37]

On 2017/05/30 16:17:44, melandory wrote:
> PTAL, thanks!

Also, added same info to the relevant bug.

Mocks -> https://folio.googleplex.com/mocks-for-copy-and-view-passwords-on-clank

State implemented in CL https://codereview.chromium.org/2900173002/ ->
https://folio.googleplex.com/copy-and-view-passwords-on-clank

Current state of UI ->
https://folio.googleplex.com/current-state-manage-passwords

[gone, 2017-06-01 20:46:38]

The screenshots don't match the mocks exactly:
1) Your headings should probably use @style/RobotoMediumStyle

2) There should be more space between the heading and the entry

3) The fonts look like they should be bigger

Are you still iterating on that?  I'm cool with doing this as a follow-up.

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/re...
File chrome/android/java/res/layout/fragment_lock_screen.xml (right):

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/re...
chrome/android/java/res/layout/fragment_lock_screen.xml:1: <?xml version="1.0"
encoding="utf-8"?>
Couldn't comment on the images:

Did you already run tools/resources/optimize_png_files.sh on them?

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/re...
chrome/android/java/res/layout/fragment_lock_screen.xml:11:
android:title="@string/lockscreen_verification_title">
newline

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/re...
chrome/android/java/res/layout/fragment_lock_screen.xml:16:
android:text="@string/lockscreen_verification_text"/>
newline

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/re...
File chrome/android/java/res/layout/password_entry_editor_interactive.xml
(right):

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/re...
chrome/android/java/res/layout/password_entry_editor_interactive.xml:122:
android:background="@android:color/transparent"
Is this necessary?

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java
(right):

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:77:
// Extras are set on this intent in class SavePasswordsPreferences.
does {@link SavePasswordsPreferences} work here?

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:78:
mExtras = getArguments();
Why'd you remove the assert?

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:96:
Context.KEYGUARD_SERVICE);
what happens if the user doesn't have a lock screen?  does this ever return
null?

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:121:
}
if (authenticationStillValid()) {
    everything else?
}

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:231:
private void changeHowPasswordDisplayed(int visibilityIcon, int inputType) {
changeHowPasswordIsDisplayed

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragment.java
(right):

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragment.java:20:
public static final int CONFIRM_DEVICE_CREDENTIAL_REQUEST_CODE = 2;
does this have to be public?  can it just be package protected?

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragment.java:22:
private boolean mPreventLockDevice = false;
booleans are false by default

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragment.java:47:
* Prevent calling the {@link lockDevice} method in {@link onCreate}
1) {@link #lockDevice}

2) {@link #onCreate}

3) finish with .

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragment.java:54:
* Should only be called on Lollipop or above devices.
1) assert this, too
2) Does this have to be public?

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java
(right):

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java:72:
// PasswordReauthentication using System.currentTimeMillis() (milliseconds since
the UNIX epoch)
don't need to explain what currentTimeMillis() is

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java:73:
private static long sLastReauthTimeMillis = 0;
this defaults to 0

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java:75:
public static void setLastReauthTimeMillis(long value) {
javadocs for public methods

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:495: <message
name="IDS_ACTION_NEXT" desc='Text of a button/menu that moves on to the next
action.'>
You already have IDS_NEXT available.

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragmentTest.java
(right):

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragmentTest.java:32:
PasswordReauthenticationFragment sMockPasswordReauthentication =
s is only used for static variables

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragmentTest.java:37:
Fragment sMockPasswordEntryEditor = new Fragment();
s is only used for static variables

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragmentTest.java:39:
Activity mTestActivity = Robolectric.setupActivity(Activity.class);
m is only used for member variables

[melandory, 2017-06-02 12:06:17]

On 2017/06/01 20:46:38, dfalcantara wrote:
> The screenshots don't match the mocks exactly:
> 1) Your headings should probably use @style/RobotoMediumStyle
> 
> 2) There should be more space between the heading and the entry
> 
> 3) The fonts look like they should be bigger
> 
> Are you still iterating on that?  I'm cool with doing this as a follow-up.
Not even that. We have couples of CLs from last year intern in our team, I want
to land them with only nessary
modifications and finally launch the feature. With main motivation that
literally anything looks better than current
page for viewing passwords. Here is the proposal for the intermediate state,
which I'll run through UI review 

https://docs.google.com/a/google.com/document/d/1_fi50f5CMGMbw7ouNr362vFXmNm-...

[gone, 2017-06-02 17:04:51]

On 2017/06/02 12:06:17, melandory wrote:
> On 2017/06/01 20:46:38, dfalcantara wrote:
> > The screenshots don't match the mocks exactly:
> > 1) Your headings should probably use @style/RobotoMediumStyle
> > 
> > 2) There should be more space between the heading and the entry
> > 
> > 3) The fonts look like they should be bigger
> > 
> > Are you still iterating on that?  I'm cool with doing this as a follow-up.
> Not even that. We have couples of CLs from last year intern in our team, I
want
> to land them with only nessary
> modifications and finally launch the feature. With main motivation that
> literally anything looks better than current
> page for viewing passwords. Here is the proposal for the intermediate state,
> which I'll run through UI review 
> 
>
https://docs.google.com/a/google.com/document/d/1_fi50f5CMGMbw7ouNr362vFXmNm-...

It might look better than what you've got, but it's still inconsistent with the
previous
existing page that you're not changing (at the very least the headers are too
close).

If you're going to try to launch something that will likely be there forever,
you should
try to polish it instead of doing the bare minimum.  There's more than enough
divergence
between the different settings pages.

[gone, 2017-06-02 17:05:28]

On 2017/06/02 17:04:51, dfalcantara wrote:
> On 2017/06/02 12:06:17, melandory wrote:
> > On 2017/06/01 20:46:38, dfalcantara wrote:
> > > The screenshots don't match the mocks exactly:
> > > 1) Your headings should probably use @style/RobotoMediumStyle
> > > 
> > > 2) There should be more space between the heading and the entry
> > > 
> > > 3) The fonts look like they should be bigger
> > > 
> > > Are you still iterating on that?  I'm cool with doing this as a follow-up.
> > Not even that. We have couples of CLs from last year intern in our team, I
> want
> > to land them with only nessary
> > modifications and finally launch the feature. With main motivation that
> > literally anything looks better than current
> > page for viewing passwords. Here is the proposal for the intermediate state,
> > which I'll run through UI review 
> > 
> >
>
https://docs.google.com/a/google.com/document/d/1_fi50f5CMGMbw7ouNr362vFXmNm-...
> 
> It might look better than what you've got, but it's still inconsistent with
the
> previous
> existing page that you're not changing (at the very least the headers are too
> close).
> 
> If you're going to try to launch something that will likely be there forever,
> you should
> try to polish it instead of doing the bare minimum.  There's more than enough
> divergence
> between the different settings pages.

*There's already more than enough difference between the different settings
pages.
Don't want you introducing more.

[melandory, 2017-06-09 12:06:19]

Description was changed from

==========
Copy and view saved passwords.

Implements the ability to copy and view saved passwords if the user has
a lock. Before allow view or copy of passwords user is prompted with the
lock. The functionality is unavailable if there is no device level
lock.

The implementation in this CL has following shortcomings:
* "Nice viewer" is not available for the users without lock, still old version
is used.
* "Promo" for the lock isn't shown.
* There is no way to delete the credentials in the new view.
* In case password was viewed (eye icon clicked), the app switcher thumbnail
contains the password in plain text.

Screenshots are attached to the bug.

The original CL: https://codereview.chromium.org/2067323004/

BUG=619868
==========

to

==========
Copy and view saved passwords.

Implements the ability to copy and view saved passwords if the user has
a lock. Before allow view or copy of passwords user is prompted with the
lock. The functionality is unavailable if there is no device level
lock.

The implementation in this CL has following shortcomings:
* "Nice viewer" is not available for the users without lock, still old version
is used.
* There is no way to delete the credentials in the new view.
* In case password was viewed (eye icon clicked), the app switcher thumbnail
contains the password in plain text.

Screenshots are attached to the bug.

The original CL: https://codereview.chromium.org/2067323004/

BUG=619868
==========

[melandory, 2017-06-09 12:35:00]

Description was changed from

==========
Copy and view saved passwords.

Implements the ability to copy and view saved passwords if the user has
a lock. Before allow view or copy of passwords user is prompted with the
lock. The functionality is unavailable if there is no device level
lock.

The implementation in this CL has following shortcomings:
* "Nice viewer" is not available for the users without lock, still old version
is used.
* There is no way to delete the credentials in the new view.
* In case password was viewed (eye icon clicked), the app switcher thumbnail
contains the password in plain text.

Screenshots are attached to the bug.

The original CL: https://codereview.chromium.org/2067323004/

BUG=619868
==========

to

==========
Copy and view saved passwords.

Implements the ability to copy and view saved passwords if the user has
a lock. Before allow view or copy of passwords user is prompted with the
lock. The functionality is unavailable if there is no device level
lock.

The implementation in this CL has following shortcomings:
* "Nice viewer" is not available for the users without lock, still old version
is used.
* There is no way to delete the credentials in the new view.
* In case password was viewed (eye icon clicked), the app switcher thumbnail
contains the password in plain text.
* Wrong lock screen message.
* UI is not aligned with mocks.

Screenshots are attached to the bug.

The original CL: https://codereview.chromium.org/2067323004/

BUG=619868
==========

[melandory, 2017-06-09 13:32:22]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-09 13:32:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-09 14:06:01]

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

[commit-bot: I haz the power, 2017-06-09 14:06:02]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[melandory, 2017-06-09 15:09:17]

On 2017/06/02 17:05:28, dfalcantara wrote:
> On 2017/06/02 17:04:51, dfalcantara wrote:
> > On 2017/06/02 12:06:17, melandory wrote:
> > > On 2017/06/01 20:46:38, dfalcantara wrote:
> > > > The screenshots don't match the mocks exactly:
> > > > 1) Your headings should probably use @style/RobotoMediumStyle
> > > > 
> > > > 2) There should be more space between the heading and the entry
> > > > 
> > > > 3) The fonts look like they should be bigger
> > > > 
> > > > Are you still iterating on that?  I'm cool with doing this as a
follow-up.
> > > Not even that. We have couples of CLs from last year intern in our team, I
> > want
> > > to land them with only nessary
> > > modifications and finally launch the feature. With main motivation that
> > > literally anything looks better than current
> > > page for viewing passwords. Here is the proposal for the intermediate
state,
> > > which I'll run through UI review 
> > > 
> > >
> >
>
https://docs.google.com/a/google.com/document/d/1_fi50f5CMGMbw7ouNr362vFXmNm-...
> > 
> > It might look better than what you've got, but it's still inconsistent with
> the
> > previous
> > existing page that you're not changing (at the very least the headers are
too
> > close).
> > 
> > If you're going to try to launch something that will likely be there
forever,
> > you should
> > try to polish it instead of doing the bare minimum.  There's more than
enough
> > divergence
> > between the different settings pages.
> 
> *There's already more than enough difference between the different settings
> pages.
> Don't want you introducing more.

Fair enough :) Let's consider this Cl the iteration then and we'll can see what
can be done in terms of finding eng owner for the project.

[melandory, 2017-06-09 15:09:28]

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/re...
File chrome/android/java/res/layout/fragment_lock_screen.xml (right):

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/re...
chrome/android/java/res/layout/fragment_lock_screen.xml:1: <?xml version="1.0"
encoding="utf-8"?>
On 2017/06/01 20:46:37, dfalcantara wrote:
> Couldn't comment on the images:
> 
> Did you already run tools/resources/optimize_png_files.sh on them?

Done.

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/re...
chrome/android/java/res/layout/fragment_lock_screen.xml:11:
android:title="@string/lockscreen_verification_title">
On 2017/06/01 20:46:37, dfalcantara wrote:
> newline

Done.

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/re...
chrome/android/java/res/layout/fragment_lock_screen.xml:16:
android:text="@string/lockscreen_verification_text"/>
On 2017/06/01 20:46:37, dfalcantara wrote:
> newline

Done.

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/re...
File chrome/android/java/res/layout/password_entry_editor_interactive.xml
(right):

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/re...
chrome/android/java/res/layout/password_entry_editor_interactive.xml:122:
android:background="@android:color/transparent"
On 2017/06/01 20:46:37, dfalcantara wrote:
> Is this necessary?

Done.

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java
(right):

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:77:
// Extras are set on this intent in class SavePasswordsPreferences.
On 2017/06/01 20:46:37, dfalcantara wrote:
> does {@link SavePasswordsPreferences} work here?

Done.

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:78:
mExtras = getArguments();
On 2017/06/01 20:46:37, dfalcantara wrote:
> Why'd you remove the assert?

Accidentally, fixed

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:96:
Context.KEYGUARD_SERVICE);
On 2017/06/01 20:46:37, dfalcantara wrote:
> what happens if the user doesn't have a lock screen?  does this ever return
> null?

mKeyguardManager will not be null, but mKeyguardManager.isKeyguardSecure() will
return false. See PasswordEntryEditor.java:281

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:121:
}
On 2017/06/01 20:46:37, dfalcantara wrote:
> if (authenticationStillValid()) {
>     everything else?
> }

Done.

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:231:
private void changeHowPasswordDisplayed(int visibilityIcon, int inputType) {
On 2017/06/01 20:46:37, dfalcantara wrote:
> changeHowPasswordIsDisplayed

Done.

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragment.java
(right):

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragment.java:20:
public static final int CONFIRM_DEVICE_CREDENTIAL_REQUEST_CODE = 2;
On 2017/06/01 20:46:37, dfalcantara wrote:
> does this have to be public?  can it just be package protected?

Done.

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragment.java:22:
private boolean mPreventLockDevice = false;
On 2017/06/01 20:46:37, dfalcantara wrote:
> booleans are false by default

Done.

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragment.java:47:
* Prevent calling the {@link lockDevice} method in {@link onCreate}
On 2017/06/01 20:46:37, dfalcantara wrote:
> 1) {@link #lockDevice}
> 
> 2) {@link #onCreate}
> 
> 3) finish with .

Done.

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragment.java:54:
* Should only be called on Lollipop or above devices.
On 2017/06/01 20:46:37, dfalcantara wrote:
> 1) assert this, too
> 2) Does this have to be public?

Done.

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java
(right):

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java:72:
// PasswordReauthentication using System.currentTimeMillis() (milliseconds since
the UNIX epoch)
On 2017/06/01 20:46:38, dfalcantara wrote:
> don't need to explain what currentTimeMillis() is

Done.

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java:73:
private static long sLastReauthTimeMillis = 0;
On 2017/06/01 20:46:37, dfalcantara wrote:
> this defaults to 0

Done.

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java:75:
public static void setLastReauthTimeMillis(long value) {
On 2017/06/01 20:46:38, dfalcantara wrote:
> javadocs for public methods

Done.

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:495: <message
name="IDS_ACTION_NEXT" desc='Text of a button/menu that moves on to the next
action.'>
On 2017/06/01 20:46:38, dfalcantara wrote:
> You already have IDS_NEXT available.

Done.

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragmentTest.java
(right):

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragmentTest.java:32:
PasswordReauthenticationFragment sMockPasswordReauthentication =
On 2017/06/01 20:46:38, dfalcantara wrote:
> s is only used for static variables

Done.

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragmentTest.java:37:
Fragment sMockPasswordEntryEditor = new Fragment();
On 2017/06/01 20:46:38, dfalcantara wrote:
> s is only used for static variables

Done.

https://codereview.chromium.org/2900173002/diff/240001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragmentTest.java:39:
Activity mTestActivity = Robolectric.setupActivity(Activity.class);
On 2017/06/01 20:46:38, dfalcantara wrote:
> m is only used for member variables

Done.

[gone, 2017-06-09 17:12:01]

cool cool, lgtm

[melandory, 2017-06-12 07:36:01]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 07:36:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-12 08:03:52]

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

[commit-bot: I haz the power, 2017-06-12 08:03:53]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[melandory, 2017-06-12 13:07:16]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 13:07:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[melandory, 2017-06-12 14:45:59]

Patchset #3 (id:260001) has been deleted

[melandory, 2017-06-12 14:46:11]

Patchset #3 (id:280001) has been deleted

[commit-bot: I haz the power, 2017-06-12 15:30:17]

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

[commit-bot: I haz the power, 2017-06-12 15:30:18]

Dry run: This issue passed the CQ dry run.

[melandory, 2017-06-13 08:11:40]

The CQ bit was checked by melandory@chromium.org

[melandory, 2017-06-13 08:11:41]

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

[commit-bot: I haz the power, 2017-06-13 08:11:56]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-13 08:15:43]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1497341500504380,
"parent_rev": "a0c5fa3e6bb48c503df220d9c8ac946598f12215", "commit_rev":
"2a595fea49f5e6ed6ed9caae2b954d25798226ba"}

[commit-bot: I haz the power, 2017-06-13 08:15:55]

Description was changed from

==========
Copy and view saved passwords.

Implements the ability to copy and view saved passwords if the user has
a lock. Before allow view or copy of passwords user is prompted with the
lock. The functionality is unavailable if there is no device level
lock.

The implementation in this CL has following shortcomings:
* "Nice viewer" is not available for the users without lock, still old version
is used.
* There is no way to delete the credentials in the new view.
* In case password was viewed (eye icon clicked), the app switcher thumbnail
contains the password in plain text.
* Wrong lock screen message.
* UI is not aligned with mocks.

Screenshots are attached to the bug.

The original CL: https://codereview.chromium.org/2067323004/

BUG=619868
==========

to

==========
Copy and view saved passwords.

Implements the ability to copy and view saved passwords if the user has
a lock. Before allow view or copy of passwords user is prompted with the
lock. The functionality is unavailable if there is no device level
lock.

The implementation in this CL has following shortcomings:
* "Nice viewer" is not available for the users without lock, still old version
is used.
* There is no way to delete the credentials in the new view.
* In case password was viewed (eye icon clicked), the app switcher thumbnail
contains the password in plain text.
* Wrong lock screen message.
* UI is not aligned with mocks.

Screenshots are attached to the bug.

The original CL: https://codereview.chromium.org/2067323004/

BUG=619868

Review-Url: https://codereview.chromium.org/2900173002
Cr-Commit-Position: refs/heads/master@{#478929}
Committed:
https://chromium.googlesource.com/chromium/src/+/2a595fea49f5e6ed6ed9caae2b95...
==========

[commit-bot: I haz the power, 2017-06-13 08:15:57]

Committed patchset #3 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/2a595fea49f5e6ed6ed9caae2b95...