Fix current/new password assignment in iOS PasswordController

Fix current/new password assignment in iOS PasswordController

Fixes assignment of current versus new password elements in
getPasswordForm to match LocateSpecificPasswords in
password_form_conversion_utils.cc. Previously, the password elements
had been added to the form in reverse order.

BUG=622253
Committed: https://crrev.com/b27161f6225ea23fc5e3c4d1b94f85cecca9d23e
Cr-Commit-Position: refs/heads/master@{#403889}

[Jackie Quinn, 2016-07-05 15:06:19]

jyquinn@chromium.org changed reviewers:
+ dvadym@chromium.org, vabr@chromium.org

[vabr (Chromium), 2016-07-05 15:36:39]

LGTM, thanks!

The controller still has tests downstream, so please do a two-sided patch trybot
run before landing, if you haven't already.

Cheers,
Vaclav

[dvadym, 2016-07-05 15:42:18]

Thanks, LGTM % small improvements

https://codereview.chromium.org/2125653002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/password_controller.mm (right):

https://codereview.chromium.org/2125653002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/password_controller.mm:694: if (values[0] ==
values[1]) {
Could you please add check if values are empty as in upstream
https://cs.chromium.org/chromium/src/components/autofill/content/renderer/pas...
?

https://codereview.chromium.org/2125653002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/password_controller.mm:710: // All three passwords
the same? This does not make sense, do not
Could you please add check if values are empty as in upstream
https://cs.chromium.org/chromium/src/components/autofill/content/renderer/pas...
?

[Jackie Quinn, 2016-07-06 07:39:28]

https://codereview.chromium.org/2125653002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/password_controller.mm (right):

https://codereview.chromium.org/2125653002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/password_controller.mm:694: if (values[0] ==
values[1]) {
On 2016/07/05 15:42:18, dvadym wrote:
> Could you please add check if values are empty as in upstream
>
https://cs.chromium.org/chromium/src/components/autofill/content/renderer/pas...
> ?

Done.

https://codereview.chromium.org/2125653002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/password_controller.mm:710: // All three passwords
the same? This does not make sense, do not
On 2016/07/05 15:42:18, dvadym wrote:
> Could you please add check if values are empty as in upstream
>
https://cs.chromium.org/chromium/src/components/autofill/content/renderer/pas...
> ?

Done.

[dvadym, 2016-07-06 11:27:23]

LGTM thanks

[vabr (Chromium), 2016-07-06 11:41:12]

Still LGTM, just a small nit. :)

Cheers,
Vaclav

https://codereview.chromium.org/2125653002/diff/20001/ios/chrome/browser/pass...
File ios/chrome/browser/passwords/password_controller.mm (right):

https://codereview.chromium.org/2125653002/diff/20001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/password_controller.mm:694: if (values[0].length()
&& values[0] == values[1]) {
nit: Please use !empty() instead of length(). It makes the code slightly easier
to read, and also allows optimisations on the side of the library (detecting
emptiness is an easier task than computing the length precisely).

Also below.

[Jackie Quinn, 2016-07-06 11:51:16]

https://codereview.chromium.org/2125653002/diff/20001/ios/chrome/browser/pass...
File ios/chrome/browser/passwords/password_controller.mm (right):

https://codereview.chromium.org/2125653002/diff/20001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/password_controller.mm:694: if (values[0].length()
&& values[0] == values[1]) {
On 2016/07/06 11:41:12, vabr (Chromium) wrote:
> nit: Please use !empty() instead of length(). It makes the code slightly
easier
> to read, and also allows optimisations on the side of the library (detecting
> emptiness is an easier task than computing the length precisely).
> 
> Also below.

Done.

[Jackie Quinn, 2016-07-06 12:07:04]

The CQ bit was checked by jyquinn@chromium.org

[Jackie Quinn, 2016-07-06 12:07:05]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
dvadym@chromium.org
Link to the patchset: https://codereview.chromium.org/2125653002/#ps10002
(title: "length->!empty")

[commit-bot: I haz the power, 2016-07-06 12:07:24]

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

[vabr (Chromium), 2016-07-06 12:18:26]

lgtm

[commit-bot: I haz the power, 2016-07-06 12:41:59]

Committed patchset #3 (id:10002)

[commit-bot: I haz the power, 2016-07-06 12:42:05]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-06 12:43:40]

Description was changed from

==========
Fix current/new password assignment in iOS PasswordController

Fixes assignment of current versus new password elements in
getPasswordForm to match LocateSpecificPasswords in
password_form_conversion_utils.cc. Previously, the password elements
had been added to the form in reverse order.

BUG=622253
==========

to

==========
Fix current/new password assignment in iOS PasswordController

Fixes assignment of current versus new password elements in
getPasswordForm to match LocateSpecificPasswords in
password_form_conversion_utils.cc. Previously, the password elements
had been added to the form in reverse order.

BUG=622253

Committed: https://crrev.com/b27161f6225ea23fc5e3c4d1b94f85cecca9d23e
Cr-Commit-Position: refs/heads/master@{#403889}
==========

[commit-bot: I haz the power, 2016-07-06 12:43:41]

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