[Password Manager] Refactor FindFormInputElements() in PasswordAutofillAgent.

[Password Manager] Refactor FindFormInputElements() in PasswordAutofillAgent.

It's a follow-up CL from cleanup issue #614023002.
  Ref: https://codereview.chromium.org/614023002/#msg24

BUG=none

Committed: https://crrev.com/f6ca6ce4da18f68ad2ef6f4ced7cd3187ffe0679
Cr-Commit-Position: refs/heads/master@{#303030}

[Pritam Nikam, 2014-11-06 06:26:24]

pritam.nikam@samsung.com changed reviewers:
+ isherman@chromium.org, vabr@chromium.org

[Pritam Nikam, 2014-11-06 06:26:24]

pritam.nikam@samsung.com changed required reviewers:
+ isherman@chromium.org, vabr@chromium.org

[Pritam Nikam, 2014-11-06 06:26:24]

Hi Ilya & Vaclav,

This cleanup CL addresses:
   - moving the code from "FindFormInputElements" to helper
"FindFormInputElement"
   - added enum for password/text-field field types.
   - remove the static qualifier from FindFormInputElements as it's already in
anonymous namespace.
   - renamed |fe| -> |form_element|.
   - removed for loop in FindFormInputElements, as there could be maximum two
input fields in PasswordFormFillData; one for username and another for password.

PTAL, Thanks!

[Pritam Nikam, 2014-11-06 10:00:25]

On 2014/11/06 06:26:24, Pritam Nikam wrote:
> Hi Ilya & Vaclav,
> 
> This cleanup CL addresses:
>    - moving the code from "FindFormInputElements" to helper
> "FindFormInputElement"
>    - added enum for password/text-field field types.
>    - remove the static qualifier from FindFormInputElements as it's already in
> anonymous namespace.
>    - renamed |fe| -> |form_element|.
>    - removed for loop in FindFormInputElements, as there could be maximum two
> input fields in PasswordFormFillData; one for username and another for
password.
> 
> PTAL, Thanks!

* below activities shall be taken care in #614023002 as a part of rest of the
cleanup:
  - added enum for password/text-field field types.
  - removed for loop in FindFormInputElements, as there could be maximum two
input fields in PasswordFormFillData; one for username and another for password.

to keep code movement lone here.

Thanks!

[vabr (Chromium), 2014-11-06 14:23:35]

Hi Pritam Nikam,
Patch set 3 LGTM.

I checked locally with meld that the code pulled out into FindFormInputElement
is the same as the for-loop body in the old code (up to substituting field for
data.fields[j]), and exiting FindFormInputElements on FindFormInputElement
returning false corresponds to the original code paths.

Feel free to land this.

Cheers,
Vaclav

https://codereview.chromium.org/700403002/diff/40001/components/autofill/cont...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/700403002/diff/40001/components/autofill/cont...
components/autofill/content/renderer/password_autofill_agent.cc:130: if
(!FindFormInputElement(form_element, data.fields[j], result)) {
optional nit: In Chromium code, one-line if-statements are usually written
without the {}. So for consistency, you can remove them.

On the other hand, keeping the {} does not seem to contradict anything in the
style guide, and it can be argued that it is safer to always have them, so feel
free to keep them.

[Pritam Nikam, 2014-11-06 14:32:52]

On 2014/11/06 14:23:35, vabr (Chromium) wrote:
> Hi Pritam Nikam,
> Patch set 3 LGTM.
> 
> I checked locally with meld that the code pulled out into FindFormInputElement
> is the same as the for-loop body in the old code (up to substituting field for
> data.fields[j]), and exiting FindFormInputElements on FindFormInputElement
> returning false corresponds to the original code paths.
> 
> Feel free to land this.
> 
> Cheers,
> Vaclav
> 
>
https://codereview.chromium.org/700403002/diff/40001/components/autofill/cont...
> File components/autofill/content/renderer/password_autofill_agent.cc (right):
> 
>
https://codereview.chromium.org/700403002/diff/40001/components/autofill/cont...
> components/autofill/content/renderer/password_autofill_agent.cc:130: if
> (!FindFormInputElement(form_element, data.fields[j], result)) {
> optional nit: In Chromium code, one-line if-statements are usually written
> without the {}. So for consistency, you can remove them.
> 
> On the other hand, keeping the {} does not seem to contradict anything in the
> style guide, and it can be argued that it is safer to always have them, so
feel
> free to keep them.

Sure, thanks Vaclav!
I'm going for commit.

Thanks!

[Pritam Nikam, 2014-11-06 14:33:00]

The CQ bit was checked by pritam.nikam@samsung.com

[commit-bot: I haz the power, 2014-11-06 14:33:45]

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

[commit-bot: I haz the power, 2014-11-06 14:33:47]

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

[commit-bot: I haz the power, 2014-11-06 14:33:49]

All required reviewers (with asterisk prefixes) have not yet approved this CL.

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Pritam Nikam, 2014-11-06 14:53:57]

pritam.nikam@samsung.com changed required reviewers:
- isherman@chromium.org, vabr@chromium.org

[Pritam Nikam, 2014-11-06 14:53:58]

Thanks Vaclav for review.

I've removed unnecessary curly braces in patch set #4, PTAL.
Removed asterisks as well so that I can hit commit bit :)

Thanks!

https://codereview.chromium.org/700403002/diff/40001/components/autofill/cont...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/700403002/diff/40001/components/autofill/cont...
components/autofill/content/renderer/password_autofill_agent.cc:130: if
(!FindFormInputElement(form_element, data.fields[j], result)) {
On 2014/11/06 14:23:35, vabr (Chromium) wrote:
> optional nit: In Chromium code, one-line if-statements are usually written
> without the {}. So for consistency, you can remove them.
> 
> On the other hand, keeping the {} does not seem to contradict anything in the
> style guide, and it can be argued that it is safer to always have them, so
feel
> free to keep them.

Done.

[Pritam Nikam, 2014-11-06 15:02:16]

The CQ bit was checked by pritam.nikam@samsung.com

[commit-bot: I haz the power, 2014-11-06 15:02:37]

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

[commit-bot: I haz the power, 2014-11-06 15:56:48]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-11-06 15:57:53]

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

[Ilya Sherman, 2014-11-06 21:36:41]

Thanks very much for splitting this off.  This should make the changes in
https://codereview.chromium.org/614023002/ much easier to review once it's
rebased -- please let me know when you've had a chance to rebase it :)