Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2067323004:
Allow copying and viewing account credentials in PasswordEntryEditor (Closed)
DescriptionAllow copying and viewing account credentials in PasswordEntryEditor
This CL allows users to copy and view their credentials, and in the case of passwords, implements the reauthentication functionality
BUG=619868
Patch Set 1 #Patch Set 2 : Move reauthentication to separate object #Patch Set 3 : Implement listener in PasswordReauthentication #Patch Set 4 : Fix crash on reauthentication #
Total comments: 11
Patch Set 5 : Fix layout and remove pre-Lollipop implementation #Patch Set 6 : Remove possibility to view passwords if not able to check device lock #
Total comments: 14
Patch Set 7 : Rebase patch #Patch Set 8 : Fix rebase issues #Patch Set 9 : Address minor comments #Patch Set 10 : Make sStartTime private #Patch Set 11 : Remove reauthentication button from UI #Patch Set 12 : Rebase patch #Patch Set 13 : Patch to re-run trybots #Patch Set 14 : Move reauthentication functionality to PasswordEntryEditor, implement copying for link and username #
Total comments: 20
Patch Set 15 : Implement password viewing #Patch Set 16 : Insert timer functionality #
Total comments: 16
Patch Set 17 : Display hidden password view #Patch Set 18 : Give users possibility to hide passwords after viewing #Patch Set 19 : Move timestamp to SavePasswordsPreferences #
Total comments: 2
Patch Set 20 : Put reauthentication logic in separate class #Patch Set 21 : Change PasswordReauthentication to not implement listener #Patch Set 22 : Implement listener (throws RuntimeException in onAttach) #Patch Set 23 : Show/copy password upon reauthentication #Patch Set 24 : Remove unnecessary comments #Patch Set 25 : Move duplicated code into separate methods #Patch Set 26 : Comment removal #Patch Set 27 : Remove chromium.gyp_env #
Total comments: 24
Patch Set 28 : Solve comments #
Total comments: 23
Patch Set 29 : Require reauthentication after exiting from password list #
Total comments: 4
Patch Set 30 : Address comments #Patch Set 31 : Remove authentication time check when toggling password view to hidden #Patch Set 32 : Add junit PasswordReauthenticationTest #Patch Set 33 : Attempt to implement ShadowKeyguardManager #
Total comments: 1
Patch Set 34 : Tweaks to test #Patch Set 35 : Add junit test for reauthentication #
Total comments: 35
Patch Set 36 : Address comments #
Total comments: 10
Patch Set 37 : Add PasswordEntryEditorTest #Patch Set 38 : Address comments #
Total comments: 10
Patch Set 39 : Address comments #Messages
Total messages: 71 (19 generated)
Description was changed from ========== Allow reauthentication in PasswordEntryEditor This CL adds the deviceLockReauthentication() method which allows users to reauthenticate using their device lock. BUG=619868 ========== to ========== Allow reauthentication in PasswordEntryEditor This CL adds the deviceLockReauthentication() method which allows users to reauthenticate using their device lock. BUG=619868 ==========
dozsa@google.com changed reviewers: + vabr@chromium.org
On 2016/06/16 12:25:27, dozsa wrote: > mailto:dozsa@google.com changed reviewers: > + mailto:vabr@chromium.org Hi Vaclav, Could you please review this CL? Best, Paula
(+Tanja for some code question) Hi Paula, Thanks for the CL. Before we get to the details, let's clarify some more high-level points: (1) Is PasswordEntryEditor.java responsible for the "Edit saved name/password or exception" view for a single password, or is it showing the list of passwords ins the "Save passwords" view? If the former, then it might be better to bind the reauthentication timer to its parent, because otherwise the timer will be reset each time the user inspects a different password on the list. (2) On desktop Chrome and on passwords.google.com, if the user is asked for reauthentication, but then changes their mind and cancels the reauth, they can still carry on with using Chrome or the page. They just won't see the passwords. In this CL, however, the new method seems to just lock the device. So once triggered, the user has no other way back than to actually reauthenticate. That might be a bad experience in some situations (e.g., the user is suddenly in hurry, cannot type at the moment, does no longer want to type the password because someone else is looking etc.). It should be possible to just trigger the unlocking challenge, but allow the user to cancel it. (Tanja, could you confirm?) With that, I would suggest to change the API slightly, to match the desktop's PasswordManagerPresenter::IsUserAuthenticated: it should be a method returning a Boolean value: true meaning that the user succeeded at the lock challenge, and false meaning that they failed or cancelled. (3) Also, it would be good to make the authentication a separate object, which you can swap with a mock implementation for testing. That way, in tests for the view code we don't have to worry about the need to actually authenticate. (4) Speaking of tests, we should test at least the logic with the timer (note that the current CL does not use AUTHENTICATION_DURATION_SECONDS at all). The test would ideally be able to inject a mocked clock into the reauthentication class, increment it to different values above and below AUTHENTICATION_DURATION_SECONDS, and check whether it attempts a call to pop-up the lock challenge. In C++, this is achieved by injecting a base::Clock and using a fake one in tests (password_manager::FacetManager is an example). I'm not sure if there is a similar option for Java. Tanja, would you know? Cheers, Vaclav
On 2016/06/16 14:19:05, vabr (Chromium) wrote: > (+Tanja for some code question) > > Hi Paula, > > Thanks for the CL. > > Before we get to the details, let's clarify some more high-level points: > > (1) Is PasswordEntryEditor.java responsible for the "Edit saved name/password or > exception" view for a single password, or is it showing the list of passwords > ins the "Save passwords" view? If the former, then it might be better to bind > the reauthentication timer to its parent, because otherwise the timer will be > reset each time the user inspects a different password on the list. > > (2) > On desktop Chrome and on http://passwords.google.com, if the user is asked for > reauthentication, but then changes their mind and cancels the reauth, they can > still carry on with using Chrome or the page. They just won't see the passwords. > > In this CL, however, the new method seems to just lock the device. So once > triggered, the user has no other way back than to actually reauthenticate. That > might be a bad experience in some situations (e.g., the user is suddenly in > hurry, cannot type at the moment, does no longer want to type the password > because someone else is looking etc.). > > It should be possible to just trigger the unlocking challenge, but allow the > user to cancel it. (Tanja, could you confirm?) With that, I would suggest to > change the API slightly, to match the desktop's > PasswordManagerPresenter::IsUserAuthenticated: it should be a method returning a > Boolean value: true meaning that the user succeeded at the lock challenge, and > false meaning that they failed or cancelled. I've never worked with pattern/pin reauth api, so my thought on this not better than reading docs :) But as I understood the only feasible customization which is available to us is to setup a trusted interval, so let's say that we don't repeat the lock is user wants to perform same action within N seconds. Although, it might be the case that user can still press "back", can you check it? > (3) Also, it would be good to make the authentication a separate object, which > you can swap with a mock implementation for testing. That way, in tests for the > view code we don't have to worry about the need to actually authenticate. Yes I also think we should go with separate object. Since re-auth will be more complicated, than it is implemented now. Currently I see few caveats: 1. API you've used to trigger the lock is available only from lolipop* 2. Current code doesn't deal with fingerprint reauth. (*) You should use "@TargetApi(Build.VERSION_CODES.LOLLIPOP)" and "if (Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP))" in order to check what api is available to you now. > (4) Speaking of tests, we should test at least the logic with the timer (note > that the current CL does not use AUTHENTICATION_DURATION_SECONDS at all). The > test would ideally be able to inject a mocked clock into the reauthentication > class, increment it to different values above and below > AUTHENTICATION_DURATION_SECONDS, and check whether it attempts a call to pop-up > the lock challenge. In C++, this is achieved by injecting a base::Clock and > using a fake one in tests (password_manager::FacetManager is an example). I'm > not sure if there is a similar option for Java. Tanja, would you know? Don't know from the top of my head, will try to dig something. > Cheers, > Vaclav
On 2016/06/17 07:20:57, melandory wrote: > On 2016/06/16 14:19:05, vabr (Chromium) wrote: > > (+Tanja for some code question) > > > > Hi Paula, > > > > Thanks for the CL. > > > > Before we get to the details, let's clarify some more high-level points: > > > > (1) Is PasswordEntryEditor.java responsible for the "Edit saved name/password > or > > exception" view for a single password, or is it showing the list of passwords > > ins the "Save passwords" view? If the former, then it might be better to bind > > the reauthentication timer to its parent, because otherwise the timer will be > > reset each time the user inspects a different password on the list. PasswordEntryEditor is responsible for the Edit saved name/password or exception view, and then there’s SavePasswordsPreferences that is responsible for the list of passwords. So yes, I will be binding the timer to SavePasswordsPreference. > > (2) > > On desktop Chrome and on http://passwords.google.com, if the user is asked for > > reauthentication, but then changes their mind and cancels the reauth, they can > > still carry on with using Chrome or the page. They just won't see the > passwords. > > > > In this CL, however, the new method seems to just lock the device. So once > > triggered, the user has no other way back than to actually reauthenticate. > That > > might be a bad experience in some situations (e.g., the user is suddenly in > > hurry, cannot type at the moment, does no longer want to type the password > > because someone else is looking etc.). > > > > It should be possible to just trigger the unlocking challenge, but allow the > > user to cancel it. (Tanja, could you confirm?) With that, I would suggest to > > change the API slightly, to match the desktop's > > PasswordManagerPresenter::IsUserAuthenticated: it should be a method returning > a > > Boolean value: true meaning that the user succeeded at the lock challenge, and > > false meaning that they failed or cancelled. > I've never worked with pattern/pin reauth api, so my thought on this not better > than reading docs :) > But as I understood the only feasible customization which is available to us is > to setup a trusted interval, > so let's say that we don't repeat the lock is user wants to perform same action > within N seconds. > Although, it might be the case that user can still press "back", can you check > it? An option would be to use something similar to “Confirm your PIN”, which shows up in Settings when you want to change your PIN. I think LockPatternVerification.java is used to do something similar to that, so I will switch to using that instead. > > (3) Also, it would be good to make the authentication a separate object, which > > you can swap with a mock implementation for testing. That way, in tests for > the > > view code we don't have to worry about the need to actually authenticate. > Yes I also think we should go with separate object. Since re-auth will be more > complicated, than it is implemented now. Currently I see few caveats: > 1. API you've used to trigger the lock is available only from lolipop* > 2. Current code doesn't deal with fingerprint reauth. > > (*) You should use mailto:"@TargetApi(Build.VERSION_CODES.LOLLIPOP)" and "if > (Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP))" in order to check what > api is available to you now. Okay, will make separate object. As for API, LockPatternVerification should be available to all Android devices. > > (4) Speaking of tests, we should test at least the logic with the timer (note > > that the current CL does not use AUTHENTICATION_DURATION_SECONDS at all). The > > test would ideally be able to inject a mocked clock into the reauthentication > > class, increment it to different values above and below > > AUTHENTICATION_DURATION_SECONDS, and check whether it attempts a call to > pop-up > > the lock challenge. In C++, this is achieved by injecting a base::Clock and > > using a fake one in tests (password_manager::FacetManager is an example). I'm > > not sure if there is a similar option for Java. Tanja, would you know? > Don't know from the top of my head, will try to dig something. > > > Cheers, > > Vaclav
On 2016/06/17 07:57:29, dozsa wrote: > On 2016/06/17 07:20:57, melandory wrote: > > On 2016/06/16 14:19:05, vabr (Chromium) wrote: > > > (+Tanja for some code question) > > > > > > Hi Paula, > > > > > > Thanks for the CL. > > > > > > Before we get to the details, let's clarify some more high-level points: > > > > > > (1) Is PasswordEntryEditor.java responsible for the "Edit saved > name/password > > or > > > exception" view for a single password, or is it showing the list of > passwords > > > ins the "Save passwords" view? If the former, then it might be better to > bind > > > the reauthentication timer to its parent, because otherwise the timer will > be > > > reset each time the user inspects a different password on the list. > > PasswordEntryEditor is responsible for the Edit saved name/password or exception > view, and then there’s SavePasswordsPreferences that is responsible for the list > of passwords. So yes, I will be binding the timer to SavePasswordsPreference. > > > > (2) > > > On desktop Chrome and on http://passwords.google.com, if the user is asked > for > > > reauthentication, but then changes their mind and cancels the reauth, they > can > > > still carry on with using Chrome or the page. They just won't see the > > passwords. > > > > > > In this CL, however, the new method seems to just lock the device. So once > > > triggered, the user has no other way back than to actually reauthenticate. > > That > > > might be a bad experience in some situations (e.g., the user is suddenly in > > > hurry, cannot type at the moment, does no longer want to type the password > > > because someone else is looking etc.). > > > > > > It should be possible to just trigger the unlocking challenge, but allow the > > > user to cancel it. (Tanja, could you confirm?) With that, I would suggest to > > > change the API slightly, to match the desktop's > > > PasswordManagerPresenter::IsUserAuthenticated: it should be a method > returning > > a > > > Boolean value: true meaning that the user succeeded at the lock challenge, > and > > > false meaning that they failed or cancelled. > > I've never worked with pattern/pin reauth api, so my thought on this not > better > > than reading docs :) > > But as I understood the only feasible customization which is available to us > is > > to setup a trusted interval, > > so let's say that we don't repeat the lock is user wants to perform same > action > > within N seconds. > > Although, it might be the case that user can still press "back", can you check > > it? > > An option would be to use something similar to “Confirm your PIN”, which shows > up in Settings when you want to change your PIN. I think > LockPatternVerification.java is used to do something similar to that, so I will > switch to using that instead. > > > > (3) Also, it would be good to make the authentication a separate object, > which > > > you can swap with a mock implementation for testing. That way, in tests for > > the > > > view code we don't have to worry about the need to actually authenticate. > > Yes I also think we should go with separate object. Since re-auth will be more > > complicated, than it is implemented now. Currently I see few caveats: > > 1. API you've used to trigger the lock is available only from lolipop* > > 2. Current code doesn't deal with fingerprint reauth. > > > > (*) You should use mailto:"@TargetApi(Build.VERSION_CODES.LOLLIPOP)" and "if > > (Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP))" in order to check > what > > api is available to you now. > > Okay, will make separate object. As for API, LockPatternVerification should be > available to all Android devices. > > > > (4) Speaking of tests, we should test at least the logic with the timer > (note > > > that the current CL does not use AUTHENTICATION_DURATION_SECONDS at all). > The > > > test would ideally be able to inject a mocked clock into the > reauthentication > > > class, increment it to different values above and below > > > AUTHENTICATION_DURATION_SECONDS, and check whether it attempts a call to > > pop-up > > > the lock challenge. In C++, this is achieved by injecting a base::Clock and > > > using a fake one in tests (password_manager::FacetManager is an example). > I'm > > > not sure if there is a similar option for Java. Tanja, would you know? > > Don't know from the top of my head, will try to dig something. > > > > > Cheers, > > > Vaclav Hi Vaclav and Tanja, Could I please get your thoughts on this method of implementing reauthentication before I proceed with implementing the timer? Thank you! Paula
Hi Paula, Thanks for the updated patch. There is one question about pre-L below. The structure of the reauth API looks good: there is a way to trigger it from the detailed password entry, it returns "success" and "fail", and the timeout looks like being bound to the save passwords settings page. I'll be happy to continue with the further review once the code is nearer to the final state. Cheers, Vaclav https://codereview.chromium.org/2067323004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:145: onScreenUnlocked(); Would this enable password viewing on old devices without the screen locking API? https://codereview.chromium.org/2067323004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java (right): https://codereview.chromium.org/2067323004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:1: // Copyright 2014 The Chromium Authors. All rights reserved. nit: Once this is being cleaned-up, please don't forget to update the year to 2016. https://codereview.chromium.org/2067323004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:53: public final int WAKE_LOCK_RELEASE_TIMEOUT_MILLIS = 20 * 1000; Before landing, this will require some comment. https://codereview.chromium.org/2067323004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:149: // Auto release after 10 seconds to avoid leaving the screen on indefinitely. 10s in comment vs. 20s in the value of the constant https://codereview.chromium.org/2067323004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:216: private void lockDeviceOnPreLollipop() { There might have been some confusion about this, but I don't think we need to implement this for pre-Lollipop. The number of affected users will decrease with time, and we can add it later if there is a need for it. If dropping this would make the code simpler, I'd suggest focussing just on Lollipop+.
https://codereview.chromium.org/2067323004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:145: onScreenUnlocked(); On 2016/06/22 13:07:51, vabr (Chromium) wrote: > Would this enable password viewing on old devices without the screen locking > API? Yes, this would. Should I can it so that devices without the API cannot access passwords at all? https://codereview.chromium.org/2067323004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java (right): https://codereview.chromium.org/2067323004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/06/22 13:07:51, vabr (Chromium) wrote: > nit: Once this is being cleaned-up, please don't forget to update the year to > 2016. Done. https://codereview.chromium.org/2067323004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:53: public final int WAKE_LOCK_RELEASE_TIMEOUT_MILLIS = 20 * 1000; On 2016/06/22 13:07:51, vabr (Chromium) wrote: > Before landing, this will require some comment. Done. https://codereview.chromium.org/2067323004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:149: // Auto release after 10 seconds to avoid leaving the screen on indefinitely. On 2016/06/22 13:07:51, vabr (Chromium) wrote: > 10s in comment vs. 20s in the value of the constant Done. https://codereview.chromium.org/2067323004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:216: private void lockDeviceOnPreLollipop() { On 2016/06/22 13:07:51, vabr (Chromium) wrote: > There might have been some confusion about this, but I don't think we need to > implement this for pre-Lollipop. The number of affected users will decrease with > time, and we can add it later if there is a need for it. If dropping this would > make the code simpler, I'd suggest focussing just on Lollipop+. Done.
https://codereview.chromium.org/2067323004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:145: onScreenUnlocked(); On 2016/06/22 13:07:51, vabr (Chromium) wrote: > Would this enable password viewing on old devices without the screen locking > API? Done.
dozsa@google.com changed reviewers: + melandory@chromium.org
Hi Tanja, could you please look through this current implementation of reauthentication? Thank you!
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2067323004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dozsa@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2067323004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
I'm sending what I have for now, mostly nits. Let's run on bots, I will continue with review meanwhile. https://codereview.chromium.org/2067323004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java (right): https://codereview.chromium.org/2067323004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java:41: public static long startTime = 0; Why public? startTime -> mStartTime because it is member https://codereview.chromium.org/2067323004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. No need to change year for the existing file. Basically, this is the date, when the file was created for the first time. https://codereview.chromium.org/2067323004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:147: "Secure lock screen hasn't set up.\n" Add string to resource files chrome/android/java/strings/android_chrome_strings.grd https://codereview.chromium.org/2067323004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java (right): https://codereview.chromium.org/2067323004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/2067323004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:68: You don't need the space between members. https://codereview.chromium.org/2067323004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:197: getActivity().startActivityForResult(intent, So, back button still works here? https://codereview.chromium.org/2067323004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java (right): https://codereview.chromium.org/2067323004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java:77: public static long startTime = 0; why public? also since it's member rename it to mStartTime
https://codereview.chromium.org/2067323004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java (right): https://codereview.chromium.org/2067323004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java:41: public static long startTime = 0; On 2016/06/23 10:43:25, melandory wrote: > Why public? > startTime -> mStartTime because it is member I made it public so I can access it from PasswordEntryEditor (if device lock check is successful then the start time is changed to the current time so I can later check if 30 seconds have passed since the last time the user entered the device lock). https://codereview.chromium.org/2067323004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/06/23 10:43:26, melandory wrote: > No need to change year for the existing file. Basically, this is the date, when > the file was created for the first time. Done. https://codereview.chromium.org/2067323004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:147: "Secure lock screen hasn't set up.\n" On 2016/06/23 10:43:26, melandory wrote: > Add string to resource files > chrome/android/java/strings/android_chrome_strings.grd Done. https://codereview.chromium.org/2067323004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java (right): https://codereview.chromium.org/2067323004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/06/23 10:43:26, melandory wrote: > 2016 Done. https://codereview.chromium.org/2067323004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:68: On 2016/06/23 10:43:26, melandory wrote: > You don't need the space between members. Done. https://codereview.chromium.org/2067323004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:197: getActivity().startActivityForResult(intent, On 2016/06/23 10:43:26, melandory wrote: > So, back button still works here? Yes, it does, but it seems that onActivityResult() isn't being called again, as the changes I make there aren't being reflected. https://codereview.chromium.org/2067323004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java (right): https://codereview.chromium.org/2067323004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java:77: public static long startTime = 0; On 2016/06/23 10:43:26, melandory wrote: > why public? > also since it's member rename it to mStartTime Sorry, the variable was moved to MainPreferences, so I've removed it from this file.
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2067323004/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by dozsa@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by dozsa@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dozsa@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
Description was changed from ========== Allow reauthentication in PasswordEntryEditor This CL adds the deviceLockReauthentication() method which allows users to reauthenticate using their device lock. BUG=619868 ========== to ========== Allow copying and viewing account credentials in PasswordEntryEditor This CL allows users to copy and view their credentials, and in the case of passwords, implements the reauthentication functionality BUG=619868 ==========
Hi Vaclav and Tanja, Could you please take a look over this CL? I have yet to actually implement viewing the password, but through the latest patch it's possible to copy the link and username, and selecting copy/view for passwords triggers the reauthentication screen. Thank you! Paula
On 2016/07/26 12:29:17, dozsa wrote: > Hi Vaclav and Tanja, > > Could you please take a look over this CL? I have yet to actually implement > viewing the password, but through the latest patch it's possible to copy the > link and username, and selecting copy/view for passwords triggers the > reauthentication screen. > > Thank you! > Paula Hi Paula, Thanks for the CL. I'm sheriffing today and tomorrow, and Blink bots are as red as always, so I failed to find time for the review now. I'll do my best tomorrow, and definitely will have a look by Thursday. Sorry! Vaclav
Hi Paula, Some assorted comments from a quick pass are below. Also the change to chromium.gyp_env looks accidental. Why is the timer kept with the main preferences and not the password preferences (the direct parent view of PasswordEntryEditor? (I have a feeling that we discussed this once and you explained that to me, but I forgot. Sorry if I make you repeat yourself. :)) Also, if I understand correctly, another thing left for other CLs is actually checking the timeout, is that correct? That's fine, in general, but perhaps we should keep the timer completely for that future change, rather than keeping half of it (resetting on reauth) here and the rest elsewhere? Finally, do we need any tests? If we postpone the stuff with the timer, we are left with setting up the new UI. Perhaps Tanja knows what kinds of tests are appropriate in that case (e.g., should we check that the icons are present in the correct user syncing state)? Thanks, Vaclav https://codereview.chromium.org/2067323004/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java (right): https://codereview.chromium.org/2067323004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java:54: private static long sStartTime = 0; nit: What are the units? Also, "start time" implies a timestamp rather than a time duration. What about sTimeSinceReauthMillis or something similar (assuming that the units are milliseconds, which the current callsite of setStartTime indicates). https://codereview.chromium.org/2067323004/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:49: public final int REQUEST_CODE_CONFIRM_DEVICE_CREDENTIALS = 1; nit: Also static. https://codereview.chromium.org/2067323004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:249: nit: Please remove. https://codereview.chromium.org/2067323004/diff/260001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2067323004/diff/260001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:372: <message name="IDS_PASSWORD_ENTRY_EDITOR_CLIPBOARD_USERNAME" desc='Name used to identify copying username to clipboard'> This (and the "site" below) are a bit cryptic. Is this just an identifier not surfaced to the user? If so, does it need translating? Or if it is surfaced to the user, could you try to describe more where this is surfaced? I'm not sure what is "identify copying". https://codereview.chromium.org/2067323004/diff/260001/chrome/browser/android... File chrome/browser/android/chrome_feature_list.cc (right): https://codereview.chromium.org/2067323004/diff/260001/chrome/browser/android... chrome/browser/android/chrome_feature_list.cc:36: &password_manager::features::kViewPasswords, Please remove this duplicated line.
https://codereview.chromium.org/2067323004/diff/260001/chrome/android/java/re... File chrome/android/java/res/layout/password_entry_editor_interactive.xml (right): https://codereview.chromium.org/2067323004/diff/260001/chrome/android/java/re... chrome/android/java/res/layout/password_entry_editor_interactive.xml:147: android:background="#0000" You can define constant for the color: https://cs.chromium.org/chromium/src/chrome/android/java/res/values/colors.xm... https://codereview.chromium.org/2067323004/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:136: nit: remove line https://codereview.chromium.org/2067323004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:160: copyUsernameButton.setOnClickListener(new View.OnClickListener() { Shouldn't we use OnLongClickListener? I think it's more common to have copying on ling click, probably worth to ask Hwi. Same question to other copying actions. https://codereview.chromium.org/2067323004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:250: nit: remove
Thank you for the comments! Regarding your questions, Vaclav, the reason I'm keeping the timer with the main preferences is so that the sStartTime variable that I'm using to calculate the difference between the time the user asks to copy/view a password and the last time they reauthenticated is reset whenever a user exits the list of passwords. As for timeout, should I move it to a different CL? It is currently implemented in the latest patch, but I could move it to a different one. As for testing, Tanja, could you please suggest appropriate tests for this CL? Thank you for all of your help, Paula https://codereview.chromium.org/2067323004/diff/260001/chrome/android/java/re... File chrome/android/java/res/layout/password_entry_editor_interactive.xml (right): https://codereview.chromium.org/2067323004/diff/260001/chrome/android/java/re... chrome/android/java/res/layout/password_entry_editor_interactive.xml:147: android:background="#0000" On 2016/07/26 22:27:29, melandory wrote: > You can define constant for the color: > https://cs.chromium.org/chromium/src/chrome/android/java/res/values/colors.xm... Done. https://codereview.chromium.org/2067323004/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java (right): https://codereview.chromium.org/2067323004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java:54: private static long sStartTime = 0; On 2016/07/26 20:03:18, vabr (Chromium) wrote: > nit: What are the units? Also, "start time" implies a timestamp rather than a > time duration. What about sTimeSinceReauthMillis or something similar (assuming > that the units are milliseconds, which the current callsite of setStartTime > indicates). I'll be using sStartTime to register the time at which the user entered their device lock, and then whenever they request to view/copy a password, I'll calculate the difference between the current time and sStartTime. If it's greater than 30 seconds, they'll be requested to enter their password again. https://codereview.chromium.org/2067323004/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:49: public final int REQUEST_CODE_CONFIRM_DEVICE_CREDENTIALS = 1; On 2016/07/26 20:03:18, vabr (Chromium) wrote: > nit: Also static. Done. https://codereview.chromium.org/2067323004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:136: On 2016/07/26 22:27:29, melandory wrote: > nit: remove line Done. https://codereview.chromium.org/2067323004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:160: copyUsernameButton.setOnClickListener(new View.OnClickListener() { On 2016/07/26 22:27:29, melandory wrote: > Shouldn't we use OnLongClickListener? I think it's more common to have copying > on ling click, probably worth to ask Hwi. > Same question to other copying actions. I will ask Hwi! https://codereview.chromium.org/2067323004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:249: On 2016/07/26 20:03:18, vabr (Chromium) wrote: > nit: Please remove. Done. https://codereview.chromium.org/2067323004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:250: On 2016/07/26 22:27:29, melandory wrote: > nit: remove Done. https://codereview.chromium.org/2067323004/diff/260001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2067323004/diff/260001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:372: <message name="IDS_PASSWORD_ENTRY_EDITOR_CLIPBOARD_USERNAME" desc='Name used to identify copying username to clipboard'> On 2016/07/26 20:03:18, vabr (Chromium) wrote: > This (and the "site" below) are a bit cryptic. Is this just an identifier not > surfaced to the user? If so, does it need translating? Or if it is surfaced to > the user, could you try to describe more where this is surfaced? I'm not sure > what is "identify copying". Sorry, this is no longer necessary as it's never surfaced. https://codereview.chromium.org/2067323004/diff/260001/chrome/browser/android... File chrome/browser/android/chrome_feature_list.cc (right): https://codereview.chromium.org/2067323004/diff/260001/chrome/browser/android... chrome/browser/android/chrome_feature_list.cc:36: &password_manager::features::kViewPasswords, On 2016/07/26 20:03:18, vabr (Chromium) wrote: > Please remove this duplicated line. Done.
Hi Paula, Thanks for the update. I added some more comments. As for splitting the reauth from this CL: I do not deem it necessary, but you might find this CL growing substantially once you start working on tests, so it might be an advantage. Speaking of tests, you now have not only the UI interaction here, but also the reauth logic. That logic needs to be tested, and the integration of it in the view as well. The current inlining of the logic into the existing view classes will not be helpful. If you separate the reauth logic in a smaller object, you might be able to write small tests for the logic without involving the view, and also inject a fake version of the reauth logic into the view classes to simulate user authentication and the view's reaction without the trouble of actually doing it (might be impossible on the bots). Cheers, Vaclav https://codereview.chromium.org/2067323004/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java (right): https://codereview.chromium.org/2067323004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java:54: private static long sStartTime = 0; On 2016/07/27 19:04:44, dozsa wrote: > On 2016/07/26 20:03:18, vabr (Chromium) wrote: > > nit: What are the units? Also, "start time" implies a timestamp rather than a > > time duration. What about sTimeSinceReauthMillis or something similar > (assuming > > that the units are milliseconds, which the current callsite of setStartTime > > indicates). > > I'll be using sStartTime to register the time at which the user entered their > device lock, and then whenever they request to view/copy a password, I'll > calculate the difference between the current time and sStartTime. If it's > greater than 30 seconds, they'll be requested to enter their password again. Ah, good point about this being indeed a time stamp. The name is still not ideal, though. In the context of MainPreferences, "start time" does not give much clue that this is a timetampf of the last reauthentication. You should still capture the units, though. Your code in PEE.java indicates that the timestampt is in milliseconds, so the suffix should be Millis. Given the above, my suggestion would be something like: sLastReauthTimeMillis. https://codereview.chromium.org/2067323004/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java (right): https://codereview.chromium.org/2067323004/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java:53: // Used for verifying if 60 seconds have passed since last authenticating. So is it 60 or 30 seconds? The comment here and the design doc say 60, the code in the patch says 30. It looks like we should stick to what the design doc says (or change the design if there are good reasons for doing so). Note that the desktop implementation uses 60 seconds. https://cs.chromium.org/chromium/src/chrome/browser/ui/passwords/password_man... https://codereview.chromium.org/2067323004/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java:115: sStartTime = 0; If you made sStartTime a (non-static) data member of SavePasswordsPreferences, could you then use the constructor of SavePasswordsPreferences to reset it? That would allow you to keep MainPreferences out of this, which seems fair, given that the timeout is only relevant for passwords. Or is the problem that SavePasswordsPreferences is not destroyed on hiding? https://codereview.chromium.org/2067323004/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:66: if (!ChromeFeatureList.isEnabled(VIEW_PASSWORDS)) { Should this really be "if viewing passwords is NOT enabled"? https://codereview.chromium.org/2067323004/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:74: if (!ChromeFeatureList.isEnabled(VIEW_PASSWORDS)) { Also here: it seems that the setup in lines 75-80 should be done if viewing is enabled, no? https://codereview.chromium.org/2067323004/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:103: if (System.currentTimeMillis() - MainPreferences.getStartTime() > 30000) { Here you use a slightly different condition than below, for checking the timeout (you are no longer checking that the start time is not 0. They are both equivalent, assuming that start time 0 represents a date sufficiently back in the past, but this is one more reason to create a helper method (see my other comment) for this and unify the approach. https://codereview.chromium.org/2067323004/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:212: && System.currentTimeMillis() - MainPreferences.getStartTime() < 30000) { The 30000 should be a named constant, as long as you are using it on multiple places. Or even better, the condition of "is timestamp not 0 and is it not too old" could be wrapped in a helper method. https://codereview.chromium.org/2067323004/diff/300001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2067323004/diff/300001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:378: <message name="IDS_PASSWORD_ENTRY_EDITOR_PASSWORD_COPIED" desc='Text that announces user that the password of a saved account has been copied'> nit: announces user -> announces to the user https://codereview.chromium.org/2067323004/diff/300001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:378: <message name="IDS_PASSWORD_ENTRY_EDITOR_PASSWORD_COPIED" desc='Text that announces user that the password of a saved account has been copied'> Also: copied -> copied into clipboard (To avoid confusion with copying into, say, another password record.)
Thank you for the comments, Vaclav! I've solved almost all of them, I'm currently working on moving the reauthentication logic to a separate object. https://codereview.chromium.org/2067323004/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java (right): https://codereview.chromium.org/2067323004/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java:53: // Used for verifying if 60 seconds have passed since last authenticating. On 2016/07/28 09:44:33, vabr (Chromium) wrote: > So is it 60 or 30 seconds? The comment here and the design doc say 60, the code > in the patch says 30. It looks like we should stick to what the design doc says > (or change the design if there are good reasons for doing so). > > Note that the desktop implementation uses 60 seconds. > https://cs.chromium.org/chromium/src/chrome/browser/ui/passwords/password_man... Sorry, I think I changed it to 30 seconds for testing purposes. I've changed it back to 60. https://codereview.chromium.org/2067323004/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java:115: sStartTime = 0; On 2016/07/28 09:44:33, vabr (Chromium) wrote: > If you made sStartTime a (non-static) data member of SavePasswordsPreferences, > could you then use the constructor of SavePasswordsPreferences to reset it? That > would allow you to keep MainPreferences out of this, which seems fair, given > that the timeout is only relevant for passwords. > > Or is the problem that SavePasswordsPreferences is not destroyed on hiding? The latter isn't a problem, so I've moved sStartTime to SavePasswordsPreferences. https://codereview.chromium.org/2067323004/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:66: if (!ChromeFeatureList.isEnabled(VIEW_PASSWORDS)) { On 2016/07/28 09:44:33, vabr (Chromium) wrote: > Should this really be "if viewing passwords is NOT enabled"? Done. https://codereview.chromium.org/2067323004/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:74: if (!ChromeFeatureList.isEnabled(VIEW_PASSWORDS)) { On 2016/07/28 09:44:33, vabr (Chromium) wrote: > Also here: it seems that the setup in lines 75-80 should be done if viewing is > enabled, no? Done. https://codereview.chromium.org/2067323004/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:103: if (System.currentTimeMillis() - MainPreferences.getStartTime() > 30000) { On 2016/07/28 09:44:33, vabr (Chromium) wrote: > Here you use a slightly different condition than below, for checking the timeout > (you are no longer checking that the start time is not 0. They are both > equivalent, assuming that start time 0 represents a date sufficiently back in > the past, but this is one more reason to create a helper method (see my other > comment) for this and unify the approach. Done. https://codereview.chromium.org/2067323004/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:212: && System.currentTimeMillis() - MainPreferences.getStartTime() < 30000) { On 2016/07/28 09:44:33, vabr (Chromium) wrote: > The 30000 should be a named constant, as long as you are using it on multiple > places. > > Or even better, the condition of "is timestamp not 0 and is it not too old" > could be wrapped in a helper method. Done. https://codereview.chromium.org/2067323004/diff/300001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2067323004/diff/300001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:378: <message name="IDS_PASSWORD_ENTRY_EDITOR_PASSWORD_COPIED" desc='Text that announces user that the password of a saved account has been copied'> On 2016/07/28 09:44:33, vabr (Chromium) wrote: > Also: copied -> copied into clipboard > (To avoid confusion with copying into, say, another password record.) Done. https://codereview.chromium.org/2067323004/diff/300001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:378: <message name="IDS_PASSWORD_ENTRY_EDITOR_PASSWORD_COPIED" desc='Text that announces user that the password of a saved account has been copied'> On 2016/07/28 09:44:33, vabr (Chromium) wrote: > nit: announces user -> announces to the user Done.
Thanks, Paula, for the improvements. Looking forward to seeing the reauthentication class and tests. Cheers, Vaclav https://codereview.chromium.org/2067323004/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:118: * and the startTime is not equal to 0. nit: startTime -> last reauthentication time https://codereview.chromium.org/2067323004/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:121: int validReauthenticationTime = 60000; Because now this number is only used on one place, and it is in a small function, it is OK to just inline it. Alternatively, please consider at least making it a constant (static final) and "long" to match the type of the timestamp.
Hi Vaclav and Tanja, I'm having an issue with moving the reauth logic into a separate class. In order to launch the reauth screen, I need to call startActivityForResult. In order to be able to call it in a different class, I need to pass the activity from PasswordEntryEditor. However, when I do so, onActivityResult in PasswordEntryEditor is no longer called. Do you have any suggestions on how I can solve this issue? Paula
https://codereview.chromium.org/2067323004/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:160: copyUsernameButton.setOnClickListener(new View.OnClickListener() { On 2016/07/27 19:04:44, dozsa wrote: > On 2016/07/26 22:27:29, melandory wrote: > > Shouldn't we use OnLongClickListener? I think it's more common to have copying > > on ling click, probably worth to ask Hwi. > > Same question to other copying actions. > > I will ask Hwi! If Hwi responds, then great. If she does not, I propose not blocking this CL on that decision, it can be changed later. Long press is usual for texts, but I'm not sure about buttons. I thought long press is to get a description of a button, not to trigger its function. So I would suggest keeping the current state unless we hear differently from the UI team.
dozsa@google.com changed reviewers: + bauerb@chromium.org, tedchoc@chromium.org, twellington@chromium.org
Hello everyone, Could you please review the changes I've made in this CL? Tests will follow as soon as the current implementation of the device lock check is confirmed as correct. Best, Paula
On 2016/08/03 11:55:52, dozsa wrote: > mailto:dozsa@google.com changed reviewers: > + mailto:bauerb@chromium.org, mailto:tedchoc@chromium.org, mailto:twellington@chromium.org FTR, usually you would pick one reviewer (or a minimal spanning set of reviewers), to avoid conflicts / unnecessary work. https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/re... File chrome/android/java/res/values/colors.xml (right): https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/re... chrome/android/java/res/values/colors.xml:198: <color name="white_icon_background">#0000</color> Four hex digits is a bit strange. Is this meant to be white? In that case, it should be #000000 (six digits). https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:51: private View mView; Can you make these final? https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:84: assert mExtras != null; This is a bit pointless, BTW: If |mExtras| is in fact null, you'll crash on the next line in any case. https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:121: if (mViewButtonPressed) { Nit: It might help readability to inline this. https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:124: if (mCopyButtonPressed) { These flags are not reset after we have copied the password. If the fragment would get moved to the background and then brought forward, would we copy the password again? And just semantically, we don't care about whether a UI element has been pressed, we want to queue up an action, so maybe we should make it a single field with an enum value. Also, can you be sure that the fragment isn't destroyed while in the background? Should you persist that state? https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:134: int validReauthenticationTime = 60000; Extract this to a constant? Also, it's a time interval (or timeout), right? "Time" sounds more like an absolute timestamp. And lastly, add the unit to the name. https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:221: if (authenticationStillValid()) { You might want to return early if the authentication is not valid anymore. https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java (right): https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:52: throw new IllegalStateException("Must be called on L+ devices"); So we'll just crash on pre-L devices? o_O https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:62: } else { "Else" isn't necessary after a return statement. Then you could just fall through to the else branch in the outer if statement.
https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/re... File chrome/android/java/res/values/colors.xml (right): https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/re... chrome/android/java/res/values/colors.xml:198: <color name="white_icon_background">#0000</color> On 2016/08/03 12:54:42, Bernhard Bauer wrote: > Four hex digits is a bit strange. Is this meant to be white? In that case, it > should be #000000 (six digits). Turns out what I needed to do was set the background to @null, which #0000 did because it wasn't a valid color. https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:51: private View mView; On 2016/08/03 12:54:42, Bernhard Bauer wrote: > Can you make these final? I'm not able to assign values to them if I make them final. https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:84: assert mExtras != null; On 2016/08/03 12:54:42, Bernhard Bauer wrote: > This is a bit pointless, BTW: If |mExtras| is in fact null, you'll crash on the > next line in any case. Done. https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:121: if (mViewButtonPressed) { On 2016/08/03 12:54:42, Bernhard Bauer wrote: > Nit: It might help readability to inline this. Done. https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:124: if (mCopyButtonPressed) { On 2016/08/03 12:54:42, Bernhard Bauer wrote: > These flags are not reset after we have copied the password. If the fragment > would get moved to the background and then brought forward, would we copy the > password again? And just semantically, we don't care about whether a UI element > has been pressed, we want to queue up an action, so maybe we should make it a > single field with an enum value. > > Also, can you be sure that the fragment isn't destroyed while in the background? > Should you persist that state? The password isn't set to visible/copied if I access the fragment again after exiting it, I think the flags automatically reset once I go back to the list of passwords. And the reason for linking this logic to the UI element is that the view after reauthentication differs according to whether the user wanted to view or copy the password. As for your last question, it seems the fragment isn't destroyed while displaying the device lock check, or at least the values for mViewButtonPressed and mCopyButtonPressed aren't. https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:134: int validReauthenticationTime = 60000; On 2016/08/03 12:54:42, Bernhard Bauer wrote: > Extract this to a constant? Also, it's a time interval (or timeout), right? > "Time" sounds more like an absolute timestamp. And lastly, add the unit to the > name. Done. https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:221: if (authenticationStillValid()) { On 2016/08/03 12:54:42, Bernhard Bauer wrote: > You might want to return early if the authentication is not valid anymore. Done. https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java (right): https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:52: throw new IllegalStateException("Must be called on L+ devices"); On 2016/08/03 12:54:42, Bernhard Bauer wrote: > So we'll just crash on pre-L devices? o_O Sorry, I meant to replace this with a text announcing that users can't view/copy passwords if their devices are pre-Lollipop. One of Vaclav's earlier comments was "There might have been some confusion about this, but I don't think we need to implement this for pre-Lollipop. The number of affected users will decrease with time, and we can add it later if there is a need for it. If dropping this would make the code simpler, I'd suggest focussing just on Lollipop+." Should I look into implementing this on pre-Lollipop devices as well? https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:62: } else { On 2016/08/03 12:54:42, Bernhard Bauer wrote: > "Else" isn't necessary after a return statement. Then you could just fall > through to the else branch in the outer if statement. Done.
https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:124: if (mCopyButtonPressed) { On 2016/08/03 14:06:54, dozsa wrote: > On 2016/08/03 12:54:42, Bernhard Bauer wrote: > > These flags are not reset after we have copied the password. If the fragment > > would get moved to the background and then brought forward, would we copy the > > password again? And just semantically, we don't care about whether a UI > element > > has been pressed, we want to queue up an action, so maybe we should make it a > > single field with an enum value. > > > > Also, can you be sure that the fragment isn't destroyed while in the > background? > > Should you persist that state? > > The password isn't set to visible/copied if I access the fragment again after > exiting it, I think the flags automatically reset once I go back to the list of > passwords. I'm thinking of something like switching to a different app, then switching back. > And the reason for linking this logic to the UI element is that the > view after reauthentication differs according to whether the user wanted to view > or copy the password. As for your last question, it seems the fragment isn't > destroyed while displaying the device lock check, or at least the values for > mViewButtonPressed and mCopyButtonPressed aren't. Well, can you be sure the fragment isn't destroyed? I don't think the fragment lifecycle guarantees that (see https://developer.android.com/guide/components/fragments.html#Lifecycle), even if it worked when testing locally. https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:45: private final int mValidReauthenticationTimeIntervalMillis = 60000; This should be static, and then SHOUTING_STYLE. https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:68: if (!ChromeFeatureList.isEnabled(VIEW_PASSWORDS)) { Why is this check now flipped? https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:223: if (!authenticationStillValid()) { Inline the return? https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:290: && authenticationStillValid()) { Why do we require the authentication to still be valid to _hide_ the password? (Also, would we want to automatically hide it if authentication expires?) https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:295: passwordView.setText(mExtras.getString( You might want to extract this to a method you can share with displayPassword(). https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java (right): https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:52: if (Build.VERSION.SDK_INT <= Build.VERSION_CODES.LOLLIPOP) { Overall, showing the UI element to show passwords, only to then tell the user that it's not supported, does not seem like the best experience. Couldn't we just hide it in this case? https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:55: Toast.LENGTH_LONG).show(); I'd probably return here so you don't need the else on the next line. https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:59: if (keyguardManager != null) { Actually, when would this be null? In PasswordEntryEditor you don't check for null. https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:67: } else { You want to remove this `else` so you fall through if the intent is null, no? And actually, this shouldn't happen either, right? If there is no screen lock set up, we would already show a toast beforehand and not navigate to this fragment.
https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:124: if (mCopyButtonPressed) { On 2016/08/03 16:07:36, Bernhard Bauer wrote: > On 2016/08/03 14:06:54, dozsa wrote: > > On 2016/08/03 12:54:42, Bernhard Bauer wrote: > > > These flags are not reset after we have copied the password. If the fragment > > > would get moved to the background and then brought forward, would we copy > the > > > password again? And just semantically, we don't care about whether a UI > > element > > > has been pressed, we want to queue up an action, so maybe we should make it > a > > > single field with an enum value. > > > > > > Also, can you be sure that the fragment isn't destroyed while in the > > background? > > > Should you persist that state? > > > > The password isn't set to visible/copied if I access the fragment again after > > exiting it, I think the flags automatically reset once I go back to the list > of > > passwords. > > I'm thinking of something like switching to a different app, then switching > back. > > > And the reason for linking this logic to the UI element is that the > > view after reauthentication differs according to whether the user wanted to > view > > or copy the password. As for your last question, it seems the fragment isn't > > destroyed while displaying the device lock check, or at least the values for > > mViewButtonPressed and mCopyButtonPressed aren't. > > Well, can you be sure the fragment isn't destroyed? I don't think the fragment > lifecycle guarantees that (see > https://developer.android.com/guide/components/fragments.html#Lifecycle), even > if it worked when testing locally. Ah, I understand what you meant now. Indeed, when users left the app they didn't have to reauthenticate when requesting to view/copy a password as long as they were in the 60 second interval. In order to fix this I'm now setting sLastReauthTimeMillis in onDetach() in SavePasswordsPreferences. Given that the conditions including the copy and view flags also depend on sLastReauthTimeMillis, I don't think they have to be set back to false. https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:45: private final int mValidReauthenticationTimeIntervalMillis = 60000; On 2016/08/03 16:07:36, Bernhard Bauer wrote: > This should be static, and then SHOUTING_STYLE. Done. https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:68: if (!ChromeFeatureList.isEnabled(VIEW_PASSWORDS)) { On 2016/08/03 16:07:36, Bernhard Bauer wrote: > Why is this check now flipped? Sorry, for some reason I wasn't able to change the flag value from the commandline so I changed the flag conditions. Flipped back. https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:223: if (!authenticationStillValid()) { On 2016/08/03 16:07:36, Bernhard Bauer wrote: > Inline the return? Done. https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:290: && authenticationStillValid()) { On 2016/08/03 16:07:36, Bernhard Bauer wrote: > Why do we require the authentication to still be valid to _hide_ the password? > > (Also, would we want to automatically hide it if authentication expires?) I set this condition in the case in which a user views a password, but then returns to the list of password and accesses another password. In this case the user would still be able to view the password, but they would first have to press on the viewing icon, so I initially want the password to be hidden. https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:295: passwordView.setText(mExtras.getString( On 2016/08/03 16:07:36, Bernhard Bauer wrote: > You might want to extract this to a method you can share with displayPassword(). Done. https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java (right): https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:52: if (Build.VERSION.SDK_INT <= Build.VERSION_CODES.LOLLIPOP) { On 2016/08/03 16:07:36, Bernhard Bauer wrote: > Overall, showing the UI element to show passwords, only to then tell the user > that it's not supported, does not seem like the best experience. Couldn't we > just hide it in this case? I've moved the logic to PasswordEntryEditor so that Pre-L devices would display the old version of the password entry editor, so just the site and the username. Would this work? Or do you think I should be giving users the option to copy the site/username in that case as well? https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:55: Toast.LENGTH_LONG).show(); On 2016/08/03 16:07:36, Bernhard Bauer wrote: > I'd probably return here so you don't need the else on the next line. Done. https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:59: if (keyguardManager != null) { On 2016/08/03 16:07:36, Bernhard Bauer wrote: > Actually, when would this be null? In PasswordEntryEditor you don't check for > null. True, I've removed the condition. As you said in your comment bellow, the existence of a device lock is checked in PasswordEntryEditor. https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:67: } else { On 2016/08/03 16:07:36, Bernhard Bauer wrote: > You want to remove this `else` so you fall through if the intent is null, no? > > And actually, this shouldn't happen either, right? If there is no screen lock > set up, we would already show a toast beforehand and not navigate to this > fragment. Done.
https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:124: if (mCopyButtonPressed) { On 2016/08/04 10:40:51, dozsa wrote: > On 2016/08/03 16:07:36, Bernhard Bauer wrote: > > On 2016/08/03 14:06:54, dozsa wrote: > > > On 2016/08/03 12:54:42, Bernhard Bauer wrote: > > > > These flags are not reset after we have copied the password. If the > fragment > > > > would get moved to the background and then brought forward, would we copy > > the > > > > password again? And just semantically, we don't care about whether a UI > > > element > > > > has been pressed, we want to queue up an action, so maybe we should make > it > > a > > > > single field with an enum value. > > > > > > > > Also, can you be sure that the fragment isn't destroyed while in the > > > background? > > > > Should you persist that state? > > > > > > The password isn't set to visible/copied if I access the fragment again > after > > > exiting it, I think the flags automatically reset once I go back to the list > > of > > > passwords. > > > > I'm thinking of something like switching to a different app, then switching > > back. > > > > > And the reason for linking this logic to the UI element is that the > > > view after reauthentication differs according to whether the user wanted to > > view > > > or copy the password. As for your last question, it seems the fragment isn't > > > destroyed while displaying the device lock check, or at least the values for > > > mViewButtonPressed and mCopyButtonPressed aren't. > > > > Well, can you be sure the fragment isn't destroyed? I don't think the fragment > > lifecycle guarantees that (see > > https://developer.android.com/guide/components/fragments.html#Lifecycle), even > > if it worked when testing locally. > > Ah, I understand what you meant now. Indeed, when users left the app they didn't > have to reauthenticate when requesting to view/copy a password as long as they > were in the 60 second interval. In order to fix this I'm now setting > sLastReauthTimeMillis in onDetach() in SavePasswordsPreferences. Given that the > conditions including the copy and view flags also depend on > sLastReauthTimeMillis, I don't think they have to be set back to false. I guess that works, but I would still recommend you familiarize yourself with the Fragment lifecycle in Android (and the Activity lifecycle, which is similar), because this is still somewhat unidiomatic. https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:290: && authenticationStillValid()) { On 2016/08/04 10:40:51, dozsa wrote: > On 2016/08/03 16:07:36, Bernhard Bauer wrote: > > Why do we require the authentication to still be valid to _hide_ the password? > > > > (Also, would we want to automatically hide it if authentication expires?) > > I set this condition in the case in which a user views a password, but then > returns to the list of password and accesses another password. In this case the > user would still be able to view the password, but they would first have to > press on the viewing icon, so I initially want the password to be hidden. Sorry, I don't understand. If we navigate to this fragment, the password is initially going to be hidden anyway, no? And if the user wants to show the password, authenticationStillValid() will allow it if the user recently authenticated. https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java (right): https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:52: if (Build.VERSION.SDK_INT <= Build.VERSION_CODES.LOLLIPOP) { On 2016/08/04 10:40:51, dozsa wrote: > On 2016/08/03 16:07:36, Bernhard Bauer wrote: > > Overall, showing the UI element to show passwords, only to then tell the user > > that it's not supported, does not seem like the best experience. Couldn't we > > just hide it in this case? > > I've moved the logic to PasswordEntryEditor so that Pre-L devices would display > the old version of the password entry editor, so just the site and the username. > Would this work? Or do you think I should be giving users the option to copy the > site/username in that case as well? Dunno, I would defer to the PM on that. If we allow copying the site/username but not the password, I would just hide the button for the password, but this is okay with me. https://codereview.chromium.org/2067323004/diff/560001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:110: if (!displayInteractivePasswordEntryEditor()) { Invert the condition and put the other branch first? https://codereview.chromium.org/2067323004/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:122: && Build.VERSION.SDK_INT <= Build.VERSION_CODES.LOLLIPOP; We show the editor on L or *earlier*?
https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:124: if (mCopyButtonPressed) { On 2016/08/05 09:14:20, Bernhard Bauer wrote: > On 2016/08/04 10:40:51, dozsa wrote: > > On 2016/08/03 16:07:36, Bernhard Bauer wrote: > > > On 2016/08/03 14:06:54, dozsa wrote: > > > > On 2016/08/03 12:54:42, Bernhard Bauer wrote: > > > > > These flags are not reset after we have copied the password. If the > > fragment > > > > > would get moved to the background and then brought forward, would we > copy > > > the > > > > > password again? And just semantically, we don't care about whether a UI > > > > element > > > > > has been pressed, we want to queue up an action, so maybe we should make > > it > > > a > > > > > single field with an enum value. > > > > > > > > > > Also, can you be sure that the fragment isn't destroyed while in the > > > > background? > > > > > Should you persist that state? > > > > > > > > The password isn't set to visible/copied if I access the fragment again > > after > > > > exiting it, I think the flags automatically reset once I go back to the > list > > > of > > > > passwords. > > > > > > I'm thinking of something like switching to a different app, then switching > > > back. > > > > > > > And the reason for linking this logic to the UI element is that the > > > > view after reauthentication differs according to whether the user wanted > to > > > view > > > > or copy the password. As for your last question, it seems the fragment > isn't > > > > destroyed while displaying the device lock check, or at least the values > for > > > > mViewButtonPressed and mCopyButtonPressed aren't. > > > > > > Well, can you be sure the fragment isn't destroyed? I don't think the > fragment > > > lifecycle guarantees that (see > > > https://developer.android.com/guide/components/fragments.html#Lifecycle), > even > > > if it worked when testing locally. > > > > Ah, I understand what you meant now. Indeed, when users left the app they > didn't > > have to reauthenticate when requesting to view/copy a password as long as they > > were in the 60 second interval. In order to fix this I'm now setting > > sLastReauthTimeMillis in onDetach() in SavePasswordsPreferences. Given that > the > > conditions including the copy and view flags also depend on > > sLastReauthTimeMillis, I don't think they have to be set back to false. > > I guess that works, but I would still recommend you familiarize yourself with > the Fragment lifecycle in Android (and the Activity lifecycle, which is > similar), because this is still somewhat unidiomatic. I've spent some time looking into this and I can't find a better option to control when a user should be required to reauthenticate. Do you have any suggestions? https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:290: && authenticationStillValid()) { On 2016/08/05 09:14:20, Bernhard Bauer wrote: > On 2016/08/04 10:40:51, dozsa wrote: > > On 2016/08/03 16:07:36, Bernhard Bauer wrote: > > > Why do we require the authentication to still be valid to _hide_ the > password? > > > > > > (Also, would we want to automatically hide it if authentication expires?) > > > > I set this condition in the case in which a user views a password, but then > > returns to the list of password and accesses another password. In this case > the > > user would still be able to view the password, but they would first have to > > press on the viewing icon, so I initially want the password to be hidden. > > Sorry, I don't understand. If we navigate to this fragment, the password is > initially going to be hidden anyway, no? And if the user wants to show the > password, authenticationStillValid() will allow it if the user recently > authenticated. Sorry, I just realized this controls something else after removing it and testing the app's behavior. It displays the hidden password in the case in which a user requested to view the password, it was displayed, and now the user wants to hide it again. Basically it toggles the visibility of the password. https://codereview.chromium.org/2067323004/diff/560001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:110: if (!displayInteractivePasswordEntryEditor()) { On 2016/08/05 09:14:20, Bernhard Bauer wrote: > Invert the condition and put the other branch first? Done. https://codereview.chromium.org/2067323004/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:122: && Build.VERSION.SDK_INT <= Build.VERSION_CODES.LOLLIPOP; On 2016/08/05 09:14:20, Bernhard Bauer wrote: > We show the editor on L or *earlier*? Sorry, reversed the sign.
https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:124: if (mCopyButtonPressed) { On 2016/08/05 13:53:53, dozsa wrote: > On 2016/08/05 09:14:20, Bernhard Bauer wrote: > > On 2016/08/04 10:40:51, dozsa wrote: > > > On 2016/08/03 16:07:36, Bernhard Bauer wrote: > > > > On 2016/08/03 14:06:54, dozsa wrote: > > > > > On 2016/08/03 12:54:42, Bernhard Bauer wrote: > > > > > > These flags are not reset after we have copied the password. If the > > > fragment > > > > > > would get moved to the background and then brought forward, would we > > copy > > > > the > > > > > > password again? And just semantically, we don't care about whether a > UI > > > > > element > > > > > > has been pressed, we want to queue up an action, so maybe we should > make > > > it > > > > a > > > > > > single field with an enum value. > > > > > > > > > > > > Also, can you be sure that the fragment isn't destroyed while in the > > > > > background? > > > > > > Should you persist that state? > > > > > > > > > > The password isn't set to visible/copied if I access the fragment again > > > after > > > > > exiting it, I think the flags automatically reset once I go back to the > > list > > > > of > > > > > passwords. > > > > > > > > I'm thinking of something like switching to a different app, then > switching > > > > back. > > > > > > > > > And the reason for linking this logic to the UI element is that the > > > > > view after reauthentication differs according to whether the user wanted > > to > > > > view > > > > > or copy the password. As for your last question, it seems the fragment > > isn't > > > > > destroyed while displaying the device lock check, or at least the values > > for > > > > > mViewButtonPressed and mCopyButtonPressed aren't. > > > > > > > > Well, can you be sure the fragment isn't destroyed? I don't think the > > fragment > > > > lifecycle guarantees that (see > > > > https://developer.android.com/guide/components/fragments.html#Lifecycle), > > even > > > > if it worked when testing locally. > > > > > > Ah, I understand what you meant now. Indeed, when users left the app they > > didn't > > > have to reauthenticate when requesting to view/copy a password as long as > they > > > were in the 60 second interval. In order to fix this I'm now setting > > > sLastReauthTimeMillis in onDetach() in SavePasswordsPreferences. Given that > > the > > > conditions including the copy and view flags also depend on > > > sLastReauthTimeMillis, I don't think they have to be set back to false. > > > > I guess that works, but I would still recommend you familiarize yourself with > > the Fragment lifecycle in Android (and the Activity lifecycle, which is > > similar), because this is still somewhat unidiomatic. > > I've spent some time looking into this and I can't find a better option to > control when a user should be required to reauthenticate. Do you have any > suggestions? The idiomatic way would be the following: The Android framework will call you back at certain times and ask you to serialize your state to a Bundle (e.g. before the Fragment is paused). Then when the fragment is (re)created, you read the state out of there. That way the state will survive even if the Fragment is destroyed by the framework (see e.g. https://developer.android.com/reference/android/app/Fragment.html#onSaveInsta... ). Like I said, what you've implemented probably works, in particular if you actually want to drop state when going into the background, but I wanted you to at least be aware of the regular Fragment lifecycle when doing this. https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:290: && authenticationStillValid()) { On 2016/08/05 13:53:53, dozsa wrote: > On 2016/08/05 09:14:20, Bernhard Bauer wrote: > > On 2016/08/04 10:40:51, dozsa wrote: > > > On 2016/08/03 16:07:36, Bernhard Bauer wrote: > > > > Why do we require the authentication to still be valid to _hide_ the > > password? > > > > > > > > (Also, would we want to automatically hide it if authentication expires?) > > > > > > I set this condition in the case in which a user views a password, but then > > > returns to the list of password and accesses another password. In this case > > the > > > user would still be able to view the password, but they would first have to > > > press on the viewing icon, so I initially want the password to be hidden. > > > > Sorry, I don't understand. If we navigate to this fragment, the password is > > initially going to be hidden anyway, no? And if the user wants to show the > > password, authenticationStillValid() will allow it if the user recently > > authenticated. > > Sorry, I just realized this controls something else after removing it and > testing the app's behavior. It displays the hidden password in the case in which > a user requested to view the password, it was displayed, and now the user wants > to hide it again. Basically it toggles the visibility of the password. Yes. My question is: Why do we require authentication to *hide* the password?
https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/520001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:124: if (mCopyButtonPressed) { On 2016/08/05 14:29:38, Bernhard Bauer wrote: > On 2016/08/05 13:53:53, dozsa wrote: > > On 2016/08/05 09:14:20, Bernhard Bauer wrote: > > > On 2016/08/04 10:40:51, dozsa wrote: > > > > On 2016/08/03 16:07:36, Bernhard Bauer wrote: > > > > > On 2016/08/03 14:06:54, dozsa wrote: > > > > > > On 2016/08/03 12:54:42, Bernhard Bauer wrote: > > > > > > > These flags are not reset after we have copied the password. If the > > > > fragment > > > > > > > would get moved to the background and then brought forward, would we > > > copy > > > > > the > > > > > > > password again? And just semantically, we don't care about whether a > > UI > > > > > > element > > > > > > > has been pressed, we want to queue up an action, so maybe we should > > make > > > > it > > > > > a > > > > > > > single field with an enum value. > > > > > > > > > > > > > > Also, can you be sure that the fragment isn't destroyed while in the > > > > > > background? > > > > > > > Should you persist that state? > > > > > > > > > > > > The password isn't set to visible/copied if I access the fragment > again > > > > after > > > > > > exiting it, I think the flags automatically reset once I go back to > the > > > list > > > > > of > > > > > > passwords. > > > > > > > > > > I'm thinking of something like switching to a different app, then > > switching > > > > > back. > > > > > > > > > > > And the reason for linking this logic to the UI element is that the > > > > > > view after reauthentication differs according to whether the user > wanted > > > to > > > > > view > > > > > > or copy the password. As for your last question, it seems the fragment > > > isn't > > > > > > destroyed while displaying the device lock check, or at least the > values > > > for > > > > > > mViewButtonPressed and mCopyButtonPressed aren't. > > > > > > > > > > Well, can you be sure the fragment isn't destroyed? I don't think the > > > fragment > > > > > lifecycle guarantees that (see > > > > > > https://developer.android.com/guide/components/fragments.html#Lifecycle), > > > even > > > > > if it worked when testing locally. > > > > > > > > Ah, I understand what you meant now. Indeed, when users left the app they > > > didn't > > > > have to reauthenticate when requesting to view/copy a password as long as > > they > > > > were in the 60 second interval. In order to fix this I'm now setting > > > > sLastReauthTimeMillis in onDetach() in SavePasswordsPreferences. Given > that > > > the > > > > conditions including the copy and view flags also depend on > > > > sLastReauthTimeMillis, I don't think they have to be set back to false. > > > > > > I guess that works, but I would still recommend you familiarize yourself > with > > > the Fragment lifecycle in Android (and the Activity lifecycle, which is > > > similar), because this is still somewhat unidiomatic. > > > > I've spent some time looking into this and I can't find a better option to > > control when a user should be required to reauthenticate. Do you have any > > suggestions? > > The idiomatic way would be the following: The Android framework will call you > back at certain times and ask you to serialize your state to a Bundle (e.g. > before the Fragment is paused). Then when the fragment is (re)created, you read > the state out of there. That way the state will survive even if the Fragment is > destroyed by the framework (see e.g. > https://developer.android.com/reference/android/app/Fragment.html#onSaveInsta... > ). > > Like I said, what you've implemented probably works, in particular if you > actually want to drop state when going into the background, but I wanted you to > at least be aware of the regular Fragment lifecycle when doing this. Okay, got it. Thank you! https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:290: && authenticationStillValid()) { On 2016/08/05 14:29:38, Bernhard Bauer wrote: > On 2016/08/05 13:53:53, dozsa wrote: > > On 2016/08/05 09:14:20, Bernhard Bauer wrote: > > > On 2016/08/04 10:40:51, dozsa wrote: > > > > On 2016/08/03 16:07:36, Bernhard Bauer wrote: > > > > > Why do we require the authentication to still be valid to _hide_ the > > > password? > > > > > > > > > > (Also, would we want to automatically hide it if authentication > expires?) > > > > > > > > I set this condition in the case in which a user views a password, but > then > > > > returns to the list of password and accesses another password. In this > case > > > the > > > > user would still be able to view the password, but they would first have > to > > > > press on the viewing icon, so I initially want the password to be hidden. > > > > > > Sorry, I don't understand. If we navigate to this fragment, the password is > > > initially going to be hidden anyway, no? And if the user wants to show the > > > password, authenticationStillValid() will allow it if the user recently > > > authenticated. > > > > Sorry, I just realized this controls something else after removing it and > > testing the app's behavior. It displays the hidden password in the case in > which > > a user requested to view the password, it was displayed, and now the user > wants > > to hide it again. Basically it toggles the visibility of the password. > > Yes. My question is: Why do we require authentication to *hide* the password? Ah, sorry, that makes sense. I've removed the authentication check.
I've just added a test for PasswordReauthentication, and I need some help with part of its implementation. I am following https://cs.corp.google.com/piper///depot/google3/javatests/com/google/android... in order to do so. However, the current version of my test isn't functional, as I am not able to import org.robolectric.internal.ShadowExtractor.extract which is what I need to use in order to be able to shadow PasswordReauthentication. Would anyone know any alternative ways of doing so, or if it is somehow possible to use this?
On 2016/08/09 16:09:26, dozsa wrote: > I've just added a test for PasswordReauthentication, and I need some help with > part of its implementation. I am following > https://cs.corp.google.com/piper///depot/google3/javatests/com/google/android... > in order to do so. However, the current version of my test isn't functional, as > I am not able to import org.robolectric.internal.ShadowExtractor.extract which > is what I need to use in order to be able to shadow PasswordReauthentication. Indeed, you can't import a class used in a completely different, non-public code base, just as you shouldn't reference a non-public class in a publicly visible code review... > Would anyone know any alternative ways of doing so, or if it is somehow possible > to use this? Depending on where you want to draw the boundary to start mocking things, you could extract the code that deals with KeyguardManager to a class and inject a mock in the test, or try to directly set up a mock for KeyguardManager itself (you might be able to have the shadow application context return a custom object for KEYGUARD_SERVICE...)
I've implemented a functional PasswordReauthenticationTest. Could you please look over its implementation? Thank you!
https://codereview.chromium.org/2067323004/diff/640001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java (right): https://codereview.chromium.org/2067323004/diff/640001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java:141: } What's the change here? https://codereview.chromium.org/2067323004/diff/680001/build/android/test_run... File build/android/test_runner.py (right): https://codereview.chromium.org/2067323004/diff/680001/build/android/test_run... build/android/test_runner.py:61: '--debug', action='store_const', const='Default', dest='build_type', What's up with this change again? https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:116: if (displayInteractivePasswordEntryEditor()) { This condition is the same as in the if-statement right before. Just merge them? https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:126: private boolean displayInteractivePasswordEntryEditor() { A method name that starts with a verb usually indicates an imperative ("display the editor!"). Name it "shouldDisplay..."? https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java (right): https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:20: public class PasswordReauthentication extends Fragment { Maybe call this ...Fragment? (You could shorten "Reauthentication" to "Reauth" or something if the class name becomes unwieldy). https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:30: public boolean hello; https://boptopop.files.wordpress.com/2011/03/l9kph.jpg https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:53: public void setTesting(boolean testing) { Add a Javadoc comment. It would also make sense to rename this to be more descriptive in what the actual effect is, i.e. something like preventLockingForTesting(). https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:71: /*FragmentManager fragmentManager = getFragmentManager();*/ Remove this comment. https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:379: Secure lock screen hasn't been set up. In order to access passwords, set up a lock screen via your device's security settings. Use a curly apostrophe (’) rather than a straight single quote (') in user-facing strings. https://codereview.chromium.org/2067323004/diff/680001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java (right): https://codereview.chromium.org/2067323004/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java:141: } What's the change in this file? https://codereview.chromium.org/2067323004/diff/680001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationTest.java (right): https://codereview.chromium.org/2067323004/diff/680001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationTest.java:35: private static final int CONFIRM_DEVICE_CREDENTIAL_REQUEST_CODE = 2; Rather than duplicating this value, make it visible (for testing) in the fragment and use that? https://codereview.chromium.org/2067323004/diff/680001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationTest.java:52: mTestActivity = Robolectric.setupActivity(Activity.class); These two lines are duplicated in the setUp() method. If you remove them from there, you could get rid of the members (which are a bit inconsistent in whether they're static or non-static, right now).
Thanks Paula for the update! (And Bernhard, for the continuously helpful review!) I added some nits and comments we talked about in person. Cheers, Vaclav https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:84: mKeyguardManager = (KeyguardManager) getActivity() It should not be necessary to use KeyguardManager in here directly, given that PasswordReauthentication was written to encapsulate it. https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:217: private void hookupCopyUsernameButton() { The newly added copy and view functions should be tested. One potential issues when doing it might be mocking PasswordReauthentication correctly (if necessary, you could use a similar approach to bypass it in the test as you did for KeyguardManager in the PasswordReauthentication test). (The old code apparently did not have a test for deletion at all. While our team should add that asap, it is not something you need to worry about in this CL.) https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java (right): https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java:77: // Used for verifying if 60 seconds have passed since last authenticating. nit: Could you please also comment about what exactly the value is? I suppose it is the time since the Unix epoch in milliseconds, but not sure if that's correct. https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java:127: public void onDetach() { This is worth testing, e.g., by: (1) creating SavePasswordPreferences (2) setting the last reauth time to some non-zero value (3) destroying the SavePasswordPreferences (4) re-creating SavePasswordPreferences (5) checking that the last reauth time is zero https://codereview.chromium.org/2067323004/diff/680001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationTest.java (right): https://codereview.chromium.org/2067323004/diff/680001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationTest.java:1: // Copyright 2014 The Chromium Authors. All rights reserved. nit: 2016 https://codereview.chromium.org/2067323004/diff/680001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationTest.java:45: * Ensure that the on/off switch in "Save Passwords" settings actually enables and disables nit: This comment seems out of date.
https://codereview.chromium.org/2067323004/diff/680001/build/android/test_run... File build/android/test_runner.py (right): https://codereview.chromium.org/2067323004/diff/680001/build/android/test_run... build/android/test_runner.py:61: '--debug', action='store_const', const='Default', dest='build_type', On 2016/08/16 23:37:35, Bernhard Bauer wrote: > What's up with this change again? I need this change and the one below to be able to run tests on my machine, but I'll revert them back to the original test_runner.py before submitting the CL. https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:84: mKeyguardManager = (KeyguardManager) getActivity() On 2016/08/17 08:57:42, vabr (Chromium) wrote: > It should not be necessary to use KeyguardManager in here directly, given that > PasswordReauthentication was written to encapsulate it. It seems that I need to set up the Keyguard Manager in PasswordEntryEditor as well, as whether it's secure or not determines whether certain buttons are displayed, and in https://codereview.chromium.org/2232893002/ I use the keyguard manager to determine whether I should display the string telling users that they should set up a device lock. https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:116: if (displayInteractivePasswordEntryEditor()) { On 2016/08/16 23:37:35, Bernhard Bauer wrote: > This condition is the same as in the if-statement right before. Just merge them? Done. https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:126: private boolean displayInteractivePasswordEntryEditor() { On 2016/08/16 23:37:35, Bernhard Bauer wrote: > A method name that starts with a verb usually indicates an imperative ("display > the editor!"). Name it "shouldDisplay..."? Done. https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:217: private void hookupCopyUsernameButton() { On 2016/08/17 08:57:42, vabr (Chromium) wrote: > The newly added copy and view functions should be tested. > One potential issues when doing it might be mocking PasswordReauthentication > correctly (if necessary, you could use a similar approach to bypass it in the > test as you did for KeyguardManager in the PasswordReauthentication test). > > (The old code apparently did not have a test for deletion at all. While our team > should add that asap, it is not something you need to worry about in this CL.) Acknowledged. https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java (right): https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:20: public class PasswordReauthentication extends Fragment { On 2016/08/16 23:37:35, Bernhard Bauer wrote: > Maybe call this ...Fragment? (You could shorten "Reauthentication" to "Reauth" > or something if the class name becomes unwieldy). Done. https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:30: public boolean hello; On 2016/08/16 23:37:35, Bernhard Bauer wrote: > https://boptopop.files.wordpress.com/2011/03/l9kph.jpg hahaha, sorry, used that to test something and forgot to remove https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:53: public void setTesting(boolean testing) { On 2016/08/16 23:37:35, Bernhard Bauer wrote: > Add a Javadoc comment. It would also make sense to rename this to be more > descriptive in what the actual effect is, i.e. something like > preventLockingForTesting(). Done. https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthentication.java:71: /*FragmentManager fragmentManager = getFragmentManager();*/ On 2016/08/16 23:37:35, Bernhard Bauer wrote: > Remove this comment. Done. https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java (right): https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java:77: // Used for verifying if 60 seconds have passed since last authenticating. On 2016/08/17 08:57:42, vabr (Chromium) wrote: > nit: Could you please also comment about what exactly the value is? I suppose it > is the time since the Unix epoch in milliseconds, but not sure if that's > correct. Done. https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java:127: public void onDetach() { On 2016/08/17 08:57:42, vabr (Chromium) wrote: > This is worth testing, e.g., by: > (1) creating SavePasswordPreferences > (2) setting the last reauth time to some non-zero value > (3) destroying the SavePasswordPreferences > (4) re-creating SavePasswordPreferences > (5) checking that the last reauth time is zero Acknowledged. https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:379: Secure lock screen hasn't been set up. In order to access passwords, set up a lock screen via your device's security settings. On 2016/08/16 23:37:35, Bernhard Bauer wrote: > Use a curly apostrophe (’) rather than a straight single quote (') in > user-facing strings. Done. https://codereview.chromium.org/2067323004/diff/680001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java (right): https://codereview.chromium.org/2067323004/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java:141: } On 2016/08/16 23:37:35, Bernhard Bauer wrote: > What's the change in this file? Oops, it seems that I removed a line at the end. https://codereview.chromium.org/2067323004/diff/680001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationTest.java (right): https://codereview.chromium.org/2067323004/diff/680001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationTest.java:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/08/17 08:57:42, vabr (Chromium) wrote: > nit: 2016 Done. https://codereview.chromium.org/2067323004/diff/680001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationTest.java:35: private static final int CONFIRM_DEVICE_CREDENTIAL_REQUEST_CODE = 2; On 2016/08/16 23:37:35, Bernhard Bauer wrote: > Rather than duplicating this value, make it visible (for testing) in the > fragment and use that? Done. https://codereview.chromium.org/2067323004/diff/680001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationTest.java:45: * Ensure that the on/off switch in "Save Passwords" settings actually enables and disables On 2016/08/17 08:57:42, vabr (Chromium) wrote: > nit: This comment seems out of date. Done. https://codereview.chromium.org/2067323004/diff/680001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationTest.java:52: mTestActivity = Robolectric.setupActivity(Activity.class); On 2016/08/16 23:37:35, Bernhard Bauer wrote: > These two lines are duplicated in the setUp() method. If you remove them from > there, you could get rid of the members (which are a bit inconsistent in whether > they're static or non-static, right now). Done.
https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java (right): https://codereview.chromium.org/2067323004/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:84: mKeyguardManager = (KeyguardManager) getActivity() On 2016/08/17 10:40:26, dozsa wrote: > On 2016/08/17 08:57:42, vabr (Chromium) wrote: > > It should not be necessary to use KeyguardManager in here directly, given that > > PasswordReauthentication was written to encapsulate it. > > It seems that I need to set up the Keyguard Manager in PasswordEntryEditor as > well, as whether it's secure or not determines whether certain buttons are > displayed, and in https://codereview.chromium.org/2232893002/ I use the keyguard > manager to determine whether I should display the string telling users that they > should set up a device lock. As long as you don't run into issues because of that when testing this class, that's fine. An alternative would be to encapsulate all communication with the KeyguardManager inside PasswordReauthentication, i.e., exposing methods for all tasks you need KeyguardManager for presently.
https://codereview.chromium.org/2067323004/diff/700001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragment.java (right): https://codereview.chromium.org/2067323004/diff/700001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragment.java:30: public boolean hello; https://www.youtube.com/watch?v=YQHsXMglC9A https://codereview.chromium.org/2067323004/diff/700001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragment.java:54: * This method is used to set the variable mTesting to true in the case in which the class This is far too much detail for a Javadoc comment. You only want to explain the effect as far as a caller is concerned, not _how_ that is achieved (that's an implementation detail) nor _where_ this is used (which would couple this to its client). Just "Prevent calling the {@link lockDevice} method in {@link onCreate}" is fine. https://codereview.chromium.org/2067323004/diff/700001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragment.java:59: mTesting = testing; I would rename the member as well. https://codereview.chromium.org/2067323004/diff/700001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java (right): https://codereview.chromium.org/2067323004/diff/700001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java:78: // in PasswordReauthentication using System.currentTimeMillis() (milliseconds since the epoch) Nit: If you're explaining it in detail anyway, add that it's the _UNIX_ epoch :) https://codereview.chromium.org/2067323004/diff/700001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragmentTest.java (right): https://codereview.chromium.org/2067323004/diff/700001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragmentTest.java:18: Remove these empty lines? I would only have blocks for: * Android imports (android.*) * Our own code (org.chromium.*) * Third-party code (org.* here)
Hi Bernhard (and anyone else here who might be able to help), I've been running into a couple of issue while trying to test PasswordEntryEditor. I currently have a junit test implemented, as I need robolectric in order to be able to set up the activity. The first issue is that I can't access ChromeFeatureList in order to be able to set up the value of the flag. And even after removing the flag check in PasswordEntryEditor, I am not able to successfully add the sMockPasswordEntryEditor fragment to the fragmentManager because the view cannot be inflated (I get a "Resource not found" error on the line in PasswordEntryEditor where I inflated the view). Do you have any suggestions on how I could test the copy and view buttons in PasswordEntryEditor? Thank you! Paula
https://codereview.chromium.org/2067323004/diff/700001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragment.java (right): https://codereview.chromium.org/2067323004/diff/700001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragment.java:30: public boolean hello; On 2016/08/17 14:56:15, Bernhard Bauer wrote: > https://www.youtube.com/watch?v=YQHsXMglC9A https://www.youtube.com/watch?v=ZnbMHkiIGwk (but it's definitely removed now) https://codereview.chromium.org/2067323004/diff/700001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragment.java:54: * This method is used to set the variable mTesting to true in the case in which the class On 2016/08/17 14:56:15, Bernhard Bauer wrote: > This is far too much detail for a Javadoc comment. You only want to explain the > effect as far as a caller is concerned, not _how_ that is achieved (that's an > implementation detail) nor _where_ this is used (which would couple this to its > client). Just "Prevent calling the {@link lockDevice} method in {@link > onCreate}" is fine. Done. https://codereview.chromium.org/2067323004/diff/700001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragment.java:59: mTesting = testing; On 2016/08/17 14:56:15, Bernhard Bauer wrote: > I would rename the member as well. Done. https://codereview.chromium.org/2067323004/diff/700001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java (right): https://codereview.chromium.org/2067323004/diff/700001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java:78: // in PasswordReauthentication using System.currentTimeMillis() (milliseconds since the epoch) On 2016/08/17 14:56:15, Bernhard Bauer wrote: > Nit: If you're explaining it in detail anyway, add that it's the _UNIX_ epoch :) Done. https://codereview.chromium.org/2067323004/diff/700001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragmentTest.java (right): https://codereview.chromium.org/2067323004/diff/700001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragmentTest.java:18: On 2016/08/17 14:56:15, Bernhard Bauer wrote: > Remove these empty lines? I would only have blocks for: > * Android imports (android.*) > * Our own code (org.chromium.*) > * Third-party code (org.* here) Done.
https://codereview.chromium.org/2067323004/diff/740001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragment.java (right): https://codereview.chromium.org/2067323004/diff/740001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragment.java:24: private LinearLayout mLayout; Unused? https://codereview.chromium.org/2067323004/diff/740001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java (right): https://codereview.chromium.org/2067323004/diff/740001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java:79: // _UNIX_ epoch) I used the underscores for emphasis in my comment. You don't need them here. https://codereview.chromium.org/2067323004/diff/740001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2067323004/diff/740001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:378: <message name="IDS_PASSWORDS_ACTIVATE_DEVICE_LOCK" desc="Text displayed when user doesn't have device lock set"> Is this message used? https://codereview.chromium.org/2067323004/diff/740001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:381: <message name="IDS_PASSWORD_ENTRY_EDITOR_CLIPBOARD_USERNAME" desc='Name used to identify copying username to clipboard'> And this one? https://codereview.chromium.org/2067323004/diff/740001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditorTest.java (right): https://codereview.chromium.org/2067323004/diff/740001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditorTest.java:37: No empty line right before the end of the method.
Thank you for the comments, Bernhard! Would you also please be able to help with the PasswordEntryEditorTest issues that I mentioned a couple messages back (sorry, I should've included that message with the last batch of comments I sent out). https://codereview.chromium.org/2067323004/diff/740001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragment.java (right): https://codereview.chromium.org/2067323004/diff/740001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordReauthenticationFragment.java:24: private LinearLayout mLayout; On 2016/08/18 18:57:05, Bernhard Bauer wrote: > Unused? Removed. https://codereview.chromium.org/2067323004/diff/740001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java (right): https://codereview.chromium.org/2067323004/diff/740001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java:79: // _UNIX_ epoch) On 2016/08/18 18:57:05, Bernhard Bauer wrote: > I used the underscores for emphasis in my comment. You don't need them here. Ooh okay, sorry. Removed. https://codereview.chromium.org/2067323004/diff/740001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2067323004/diff/740001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:378: <message name="IDS_PASSWORDS_ACTIVATE_DEVICE_LOCK" desc="Text displayed when user doesn't have device lock set"> On 2016/08/18 18:57:05, Bernhard Bauer wrote: > Is this message used? Not anymore, removed. https://codereview.chromium.org/2067323004/diff/740001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:381: <message name="IDS_PASSWORD_ENTRY_EDITOR_CLIPBOARD_USERNAME" desc='Name used to identify copying username to clipboard'> On 2016/08/18 18:57:05, Bernhard Bauer wrote: > And this one? Removed as well. https://codereview.chromium.org/2067323004/diff/740001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditorTest.java (right): https://codereview.chromium.org/2067323004/diff/740001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditorTest.java:37: On 2016/08/18 18:57:05, Bernhard Bauer wrote: > No empty line right before the end of the method. Done.
On 2016/08/18 09:51:27, dozsa wrote: > Hi Bernhard (and anyone else here who might be able to help), > > I've been running into a couple of issue while trying to test > PasswordEntryEditor. I currently have a junit test implemented, as I need > robolectric in order to be able to set up the activity. > > The first issue is that I can't access ChromeFeatureList in order to be able to > set up the value of the flag. Yes, ChromeFeatureList calls into native code, which you can't do in a JUnit test. You could add a static Map of testing overrides to ChromeFeatureList to avoid the JNI call. > And even after removing the flag check in > PasswordEntryEditor, I am not able to successfully add the > sMockPasswordEntryEditor fragment to the fragmentManager because the view > cannot be inflated (I get a "Resource not found" error on the line in > PasswordEntryEditor where I inflated the view). > > Do you have any suggestions on how I could test the copy and view buttons in > PasswordEntryEditor? The resource is probably not available either in the JUnit test, so you might have to stub out loading it (for example). What exactly is the behavior you want to test? Where to stub out what probably depends on that. > Thank you! > Paula
Thanks, Bernhard, for the review so far! Paula has finished the internship, so I'll be taking over the work in progress. Because of upload restrictions I will need to do that in a separate CL. Once I get to it and address all outstanding comments and issues, I will ask you for a review on that. Thanks! Vaclav |
