Refactoring PasswordAutofillAgent::FillUserNameAndPassword function.

Refactoring PasswordAutofillAgent::FillUserNameAndPassword for early return if password_element is not autocompleteable.

PasswordAutofillAgent::FillUserNameAndPassword function refactor done for early return as we chould check this at start only.

BUG=398436
Committed: https://crrev.com/2e88712e567b53efd941e87ef2b04fc60a8bce03
Cr-Commit-Position: refs/heads/master@{#295653}

[Deepak, 2014-09-09 08:39:05]

deepak.m1@samsung.com changed reviewers:
+ gcasto@chromium.org, isherman@chromium.org, vabr@chromium.org

[Deepak, 2014-09-09 08:39:06]

Refactoring changes done.
@PTAL

[Deepak, 2014-09-15 14:06:38]

On 2014/09/09 08:39:06, deepak.m1 wrote:
> Refactoring changes done.
> @PTAL

@vabr PTAL of my changes.

[vabr (Chromium), 2014-09-15 14:39:54]

Hi deepak.m1,

First, sorry for the delay, I was on holiday last week.

Second, there seems to be a misunderstanding. There is no problem in calling
IsElementsAutocompletable each time for each input field (it is, in fact, better
than having yet another function to do the job).

What I meant on the bug was to eliminate situations when
IsElementsAutocompletable is called, then the data is passed to somewhere else,
and IsElementsAutocompletable is called on that data at the destination again.

I'm no longer sure where I saw that, but it did happen somewhere.

Cheers,
Vaclav

[Deepak, 2014-09-15 15:00:32]

On 2014/09/15 14:39:54, vabr (Chromium) wrote:
> Hi deepak.m1,
> 
> First, sorry for the delay, I was on holiday last week.
> 
> Second, there seems to be a misunderstanding. There is no problem in calling
> IsElementsAutocompletable each time for each input field (it is, in fact,
better
> than having yet another function to do the job).
> 
> What I meant on the bug was to eliminate situations when
> IsElementsAutocompletable is called, then the data is passed to somewhere
else,
> and IsElementsAutocompletable is called on that data at the destination again.
> 
> I'm no longer sure where I saw that, but it did happen somewhere.
> 
> Cheers,
> Vaclav

so apart from  IsElementsAutocompletable(), changes in
PasswordAutofillAgent::FillUserNameAndPassword() is ok.
I found 2 times check in 
PasswordAutofillAgent::FillFormOnPasswordRecieved()
we are checking 

  // If we can't modify the password, don't try to set the username
  if (!IsElementAutocompletable(password_element))
    return;

and then we are checking password_element again in bool
PasswordAutofillAgent::FillUserNameAndPassword().
So we can remove the check from
PasswordAutofillAgent::FillFormOnPasswordRecieved().
I have not found duplication of IsElementsAutocompletable() at any other place.

Please let me know your opinion, so that I will remove
IsElementsAutocompletable() changes from earlier patch and remove check from
PasswordAutofillAgent::FillFormOnPasswordRecieved() function.

Thanks

[Deepak, 2014-09-15 15:09:23]

On 2014/09/15 15:00:32, deepak.m1 wrote:
> On 2014/09/15 14:39:54, vabr (Chromium) wrote:
> > Hi deepak.m1,
> > 
> > First, sorry for the delay, I was on holiday last week.
> > 
> > Second, there seems to be a misunderstanding. There is no problem in calling
> > IsElementsAutocompletable each time for each input field (it is, in fact,
> better
> > than having yet another function to do the job).
> > 
> > What I meant on the bug was to eliminate situations when
> > IsElementsAutocompletable is called, then the data is passed to somewhere
> else,
> > and IsElementsAutocompletable is called on that data at the destination
again.
> > 
> > I'm no longer sure where I saw that, but it did happen somewhere.
> > 
> > Cheers,
> > Vaclav
> 
> so apart from  IsElementsAutocompletable(), changes in
> PasswordAutofillAgent::FillUserNameAndPassword() is ok.
> I found 2 times check in 
> PasswordAutofillAgent::FillFormOnPasswordRecieved()
> we are checking 
> 
>   // If we can't modify the password, don't try to set the username
>   if (!IsElementAutocompletable(password_element))
>     return;
> 
> and then we are checking password_element again in bool
> PasswordAutofillAgent::FillUserNameAndPassword().
> So we can remove the check from
> PasswordAutofillAgent::FillFormOnPasswordRecieved().
> I have not found duplication of IsElementsAutocompletable() at any other
place.
> 
> Please let me know your opinion, so that I will remove
> IsElementsAutocompletable() changes from earlier patch and remove check from
> PasswordAutofillAgent::FillFormOnPasswordRecieved() function.
> 
> Thanks

One more thing I feel we should shift :

  // Don't fill username if password can't be set.
  if (!IsElementAutocompletable(*password_element)) {
    return false;
  }

code in bool PasswordAutofillAgent::FillUserNameAndPassword() to start of the
function that will help in early return.

[Deepak, 2014-09-18 04:17:08]

On 2014/09/15 15:09:23, deepak.m1 wrote:
> On 2014/09/15 15:00:32, deepak.m1 wrote:
> > On 2014/09/15 14:39:54, vabr (Chromium) wrote:
> > > Hi deepak.m1,
> > > 
> > > First, sorry for the delay, I was on holiday last week.
> > > 
> > > Second, there seems to be a misunderstanding. There is no problem in
calling
> > > IsElementsAutocompletable each time for each input field (it is, in fact,
> > better
> > > than having yet another function to do the job).
> > > 
> > > What I meant on the bug was to eliminate situations when
> > > IsElementsAutocompletable is called, then the data is passed to somewhere
> > else,
> > > and IsElementsAutocompletable is called on that data at the destination
> again.
> > > 
> > > I'm no longer sure where I saw that, but it did happen somewhere.
> > > 
> > > Cheers,
> > > Vaclav
> > 
> > so apart from  IsElementsAutocompletable(), changes in
> > PasswordAutofillAgent::FillUserNameAndPassword() is ok.
> > I found 2 times check in 
> > PasswordAutofillAgent::FillFormOnPasswordRecieved()
> > we are checking 
> > 
> >   // If we can't modify the password, don't try to set the username
> >   if (!IsElementAutocompletable(password_element))
> >     return;
> > 
> > and then we are checking password_element again in bool
> > PasswordAutofillAgent::FillUserNameAndPassword().
> > So we can remove the check from
> > PasswordAutofillAgent::FillFormOnPasswordRecieved().
> > I have not found duplication of IsElementsAutocompletable() at any other
> place.
> > 
> > Please let me know your opinion, so that I will remove
> > IsElementsAutocompletable() changes from earlier patch and remove check from
> > PasswordAutofillAgent::FillFormOnPasswordRecieved() function.
> > 
> > Thanks
> 
One more thing I feel we should shift :

   // Don't fill username if password can't be set.
   if (!IsElementAutocompletable(*password_element)) {
     return false;
   }
 
 code in bool PasswordAutofillAgent::FillUserNameAndPassword() to start of the
 function that will help in early return.

@vabr PTAL

[vabr (Chromium), 2014-09-18 09:14:24]

Checking IsElementAutocompletable(password_element) in
FillFormOnPasswordRecieved is necessary to avoid username element value just
below, so don't remove that.
Also, don't remove IsElementAutocompletable(*password_element) from
FillUserNameAndPassword, because FillUserNameAndPassword has more callsites than
just FillFormOnPasswordRecieved.

Cheers,
Vaclav

https://codereview.chromium.org/557703002/diff/1/components/autofill/content/...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/557703002/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_autofill_agent.cc:975: if
(!IsElementAutocompletable(*password_element))
Since you asked explicitly: yes, moving this check up seems OK.

https://codereview.chromium.org/557703002/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_autofill_agent.cc:1028: void
PasswordAutofillAgent::CheckLoginCollectionForMatch(
Why separating the code into the two functions here, when it is only called at
one place? I suggest reverting back to inlining in this case.

[Deepak, 2014-09-18 09:34:58]

On 2014/09/18 09:14:24, vabr (Chromium) wrote:
> Checking IsElementAutocompletable(password_element) in
> FillFormOnPasswordRecieved is necessary to avoid username element value just
> below, so don't remove that.
> Also, don't remove IsElementAutocompletable(*password_element) from
> FillUserNameAndPassword, because FillUserNameAndPassword has more callsites
than
> just FillFormOnPasswordRecieved.
> 
> Cheers,
> Vaclav
> 
>
https://codereview.chromium.org/557703002/diff/1/components/autofill/content/...
> File components/autofill/content/renderer/password_autofill_agent.cc (right):
> 
>
https://codereview.chromium.org/557703002/diff/1/components/autofill/content/...
> components/autofill/content/renderer/password_autofill_agent.cc:975: if
> (!IsElementAutocompletable(*password_element))
> Since you asked explicitly: yes, moving this check up seems OK.
> 
>
https://codereview.chromium.org/557703002/diff/1/components/autofill/content/...
> components/autofill/content/renderer/password_autofill_agent.cc:1028: void
> PasswordAutofillAgent::CheckLoginCollectionForMatch(
> Why separating the code into the two functions here, when it is only called at
> one place? I suggest reverting back to inlining in this case.

I understood. Just one more query , is it ok that we reduce 2 time calling of
the same function i.e IsElementAutocompletable() to IsElementsAutocompletable() 
as I am achieving same behavior ~A+~B equals ~(AB). and is less cluttered.

[Deepak, 2014-09-18 09:35:13]

https://codereview.chromium.org/557703002/diff/1/components/autofill/content/...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/557703002/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_autofill_agent.cc:975: if
(!IsElementAutocompletable(*password_element))
On 2014/09/18 09:14:24, vabr (Chromium) wrote:
> Since you asked explicitly: yes, moving this check up seems OK.

Thanks.

https://codereview.chromium.org/557703002/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_autofill_agent.cc:1028: void
PasswordAutofillAgent::CheckLoginCollectionForMatch(
On 2014/09/18 09:14:24, vabr (Chromium) wrote:
> Why separating the code into the two functions here, when it is only called at
> one place? I suggest reverting back to inlining in this case.

ok, I will make this again inline as earlier.

[Deepak, 2014-09-18 09:35:42]

On 2014/09/18 09:35:13, deepak.m1 wrote:
>
https://codereview.chromium.org/557703002/diff/1/components/autofill/content/...
> File components/autofill/content/renderer/password_autofill_agent.cc (right):
> 
>
https://codereview.chromium.org/557703002/diff/1/components/autofill/content/...
> components/autofill/content/renderer/password_autofill_agent.cc:975: if
> (!IsElementAutocompletable(*password_element))
> On 2014/09/18 09:14:24, vabr (Chromium) wrote:
> > Since you asked explicitly: yes, moving this check up seems OK.
> 
> Thanks.
> 
>
https://codereview.chromium.org/557703002/diff/1/components/autofill/content/...
> components/autofill/content/renderer/password_autofill_agent.cc:1028: void
> PasswordAutofillAgent::CheckLoginCollectionForMatch(
> On 2014/09/18 09:14:24, vabr (Chromium) wrote:
> > Why separating the code into the two functions here, when it is only called
at
> > one place? I suggest reverting back to inlining in this case.
> 
> ok, I will make this again inline as earlier.

I understood. Just one more query , is it ok that we reduce 2 time calling of
the same function i.e IsElementAutocompletable() to IsElementsAutocompletable() 
as I am achieving same behavior ~A+~B equals ~(AB). and is less cluttered.

[vabr (Chromium), 2014-09-18 10:04:17]

On 2014/09/18 09:35:42, deepak.m1 wrote:
> On 2014/09/18 09:35:13, deepak.m1 wrote:
> >
>
https://codereview.chromium.org/557703002/diff/1/components/autofill/content/...
> > File components/autofill/content/renderer/password_autofill_agent.cc
(right):
> > 
> >
>
https://codereview.chromium.org/557703002/diff/1/components/autofill/content/...
> > components/autofill/content/renderer/password_autofill_agent.cc:975: if
> > (!IsElementAutocompletable(*password_element))
> > On 2014/09/18 09:14:24, vabr (Chromium) wrote:
> > > Since you asked explicitly: yes, moving this check up seems OK.
> > 
> > Thanks.
> > 
> >
>
https://codereview.chromium.org/557703002/diff/1/components/autofill/content/...
> > components/autofill/content/renderer/password_autofill_agent.cc:1028: void
> > PasswordAutofillAgent::CheckLoginCollectionForMatch(
> > On 2014/09/18 09:14:24, vabr (Chromium) wrote:
> > > Why separating the code into the two functions here, when it is only
called
> at
> > > one place? I suggest reverting back to inlining in this case.
> > 
> > ok, I will make this again inline as earlier.
> 
> I understood. Just one more query , is it ok that we reduce 2 time calling of
> the same function i.e IsElementAutocompletable() to
IsElementsAutocompletable() 
> as I am achieving same behavior ~A+~B equals ~(AB). and is less cluttered.

Hi deepak.m1,

Actually, I prefer not to create the new IsElementAutocompletable:
You need the one-argument version of IsElementAutocompletable anyway, see, e.g.,
line 1004 in password_autofill_agent.cc of patch set 1, so having a version for
two arguments means adding a new function, as you do in patch set 1.
Now, currently a human reader of the code only has to read the single definition
of IsElementAutocompletable, and understands all the code.
With your patch set, there are two functions to read, so in my opinion there is
actually more clutter.
I don't understand what you mean by "~A+~B equals ~(AB)".

Thank you.
Vaclav

[Deepak, 2014-09-18 10:24:04]

On 2014/09/18 10:04:17, vabr (Chromium) wrote:
> On 2014/09/18 09:35:42, deepak.m1 wrote:
> > On 2014/09/18 09:35:13, deepak.m1 wrote:
> > >
> >
>
https://codereview.chromium.org/557703002/diff/1/components/autofill/content/...
> > > File components/autofill/content/renderer/password_autofill_agent.cc
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/557703002/diff/1/components/autofill/content/...
> > > components/autofill/content/renderer/password_autofill_agent.cc:975: if
> > > (!IsElementAutocompletable(*password_element))
> > > On 2014/09/18 09:14:24, vabr (Chromium) wrote:
> > > > Since you asked explicitly: yes, moving this check up seems OK.
> > > 
> > > Thanks.
> > > 
> > >
> >
>
https://codereview.chromium.org/557703002/diff/1/components/autofill/content/...
> > > components/autofill/content/renderer/password_autofill_agent.cc:1028: void
> > > PasswordAutofillAgent::CheckLoginCollectionForMatch(
> > > On 2014/09/18 09:14:24, vabr (Chromium) wrote:
> > > > Why separating the code into the two functions here, when it is only
> called
> > at
> > > > one place? I suggest reverting back to inlining in this case.
> > > 
> > > ok, I will make this again inline as earlier.
> > 
> > I understood. Just one more query , is it ok that we reduce 2 time calling
of
> > the same function i.e IsElementAutocompletable() to
> IsElementsAutocompletable() 
> > as I am achieving same behavior ~A+~B equals ~(AB). and is less cluttered.
> 
> Hi deepak.m1,
> 
> Actually, I prefer not to create the new IsElementAutocompletable:
> You need the one-argument version of IsElementAutocompletable anyway, see,
e.g.,
> line 1004 in password_autofill_agent.cc of patch set 1, so having a version
for
> two arguments means adding a new function, as you do in patch set 1.
> Now, currently a human reader of the code only has to read the single
definition
> of IsElementAutocompletable, and understands all the code.
> With your patch set, there are two functions to read, so in my opinion there
is
> actually more clutter.
> I don't understand what you mean by "~A+~B equals ~(AB)".
> 
> Thank you.
> Vaclav

ok. I agree.
IsElementAutocompletable() is for a single element.
IsElementsAutocompletable() is for 2 elements.

~A+~B equals ~(AB) means

!fun(X) || !fun(Y) == !(fun(X) && fun(Y)).

[Mike West, 2014-09-18 10:25:14]

mkwst@chromium.org changed reviewers:
+ mkwst@chromium.org

[Mike West, 2014-09-18 10:25:15]

One grammar nit, having read none of the code. :)

https://codereview.chromium.org/557703002/diff/1/components/autofill/content/...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/557703002/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_autofill_agent.cc:197: bool
IsElementsAutocompletable(const blink::WebInputElement& element1,
Nit: _Are_ElementsAutocompletable.

[vabr (Chromium), 2014-09-18 10:36:47]

On 2014/09/18 10:25:15, Mike West wrote:
> One grammar nit, having read none of the code. :)
> 
>
https://codereview.chromium.org/557703002/diff/1/components/autofill/content/...
> File components/autofill/content/renderer/password_autofill_agent.cc (right):
> 
>
https://codereview.chromium.org/557703002/diff/1/components/autofill/content/...
> components/autofill/content/renderer/password_autofill_agent.cc:197: bool
> IsElementsAutocompletable(const blink::WebInputElement& element1,
> Nit: _Are_ElementsAutocompletable.

Correct, although I hope that that function will be removed. :)

[vabr (Chromium), 2014-09-18 10:38:16]

On 2014/09/18 10:24:04, deepak.m1 wrote:
> On 2014/09/18 10:04:17, vabr (Chromium) wrote:
> > On 2014/09/18 09:35:42, deepak.m1 wrote:
> > > On 2014/09/18 09:35:13, deepak.m1 wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/557703002/diff/1/components/autofill/content/...
> > > > File components/autofill/content/renderer/password_autofill_agent.cc
> > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/557703002/diff/1/components/autofill/content/...
> > > > components/autofill/content/renderer/password_autofill_agent.cc:975: if
> > > > (!IsElementAutocompletable(*password_element))
> > > > On 2014/09/18 09:14:24, vabr (Chromium) wrote:
> > > > > Since you asked explicitly: yes, moving this check up seems OK.
> > > > 
> > > > Thanks.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/557703002/diff/1/components/autofill/content/...
> > > > components/autofill/content/renderer/password_autofill_agent.cc:1028:
void
> > > > PasswordAutofillAgent::CheckLoginCollectionForMatch(
> > > > On 2014/09/18 09:14:24, vabr (Chromium) wrote:
> > > > > Why separating the code into the two functions here, when it is only
> > called
> > > at
> > > > > one place? I suggest reverting back to inlining in this case.
> > > > 
> > > > ok, I will make this again inline as earlier.
> > > 
> > > I understood. Just one more query , is it ok that we reduce 2 time calling
> of
> > > the same function i.e IsElementAutocompletable() to
> > IsElementsAutocompletable() 
> > > as I am achieving same behavior ~A+~B equals ~(AB). and is less cluttered.
> > 
> > Hi deepak.m1,
> > 
> > Actually, I prefer not to create the new IsElementAutocompletable:
> > You need the one-argument version of IsElementAutocompletable anyway, see,
> e.g.,
> > line 1004 in password_autofill_agent.cc of patch set 1, so having a version
> for
> > two arguments means adding a new function, as you do in patch set 1.
> > Now, currently a human reader of the code only has to read the single
> definition
> > of IsElementAutocompletable, and understands all the code.
> > With your patch set, there are two functions to read, so in my opinion there
> is
> > actually more clutter.
> > I don't understand what you mean by "~A+~B equals ~(AB)".
> > 
> > Thank you.
> > Vaclav
> 
> ok. I agree.
> IsElementAutocompletable() is for a single element.
> IsElementsAutocompletable() is for 2 elements.
> 
> ~A+~B equals ~(AB) means
> 
> !fun(X) || !fun(Y) == !(fun(X) && fun(Y)).

Ah, thanks. Now I understand.
But I still prefer having just IsElementAutocompletable and calling it for every
element separately, as explained above.

Thanks you for your work on this!

Vaclav

[Deepak, 2014-09-18 12:11:32]

I understand that we will get more flexiblity by checking username and
password's seprately.
Thanks a lot for your time & guidance, Done changes as discussd with vabr.
PTAL.

[vabr (Chromium), 2014-09-18 12:15:02]

Thank you, LGTM!

But please before landing this CL, update the title and description.

Thank you,
Vaclav

[Garrett Casto, 2014-09-18 12:30:12]

The CQ bit was checked by gcasto@chromium.org

[Garrett Casto, 2014-09-18 12:30:13]

lgtm

[Deepak, 2014-09-18 12:30:25]

On 2014/09/18 12:15:02, vabr (Chromium) wrote:
> Thank you, LGTM!
> 
> But please before landing this CL, update the title and description.
> 
> Thank you,
> Vaclav

Thanks, Title and Desscription updated.

[commit-bot: I haz the power, 2014-09-18 12:31:29]

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

[commit-bot: I haz the power, 2014-09-18 14:32:08]

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

[commit-bot: I haz the power, 2014-09-18 14:32:10]

Exceeded time limit waiting for builds to trigger.

[Deepak, 2014-09-19 03:05:34]

The CQ bit was checked by deepak.m1@samsung.com

[commit-bot: I haz the power, 2014-09-19 03:05:52]

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

[commit-bot: I haz the power, 2014-09-19 03:41:14]

Committed patchset #2 (id:20001) as c210da5441fab3b87cd01dc138057f8d860f506e

[commit-bot: I haz the power, 2014-09-19 03:42:23]

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