Enable manual password autofilling if the username field is read-only.

Enable manual password autofilling if the username field is read-only.

If the username field was read-only, password manager allowed to choose saved
password but failed to insert it into the password field.

BUG=634906
R=vabr@chromium.org

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

[sense (YandexTeam), 2016-09-07 06:46:04]

We hit this problem in our browser setup process, when it offers you to login
with imported userdata from a different browser.

[vabr (Chromium), 2016-09-07 08:32:53]

Thanks for the patch!

Looks good, but before approving I left some comments in the code. Also:

(1) Could you please check what happens, if the user submits the filled password
and logs in successfully? Is the combination of the read-only username and the
filled password saved as a new credential in the password store? (Bonus points
if you make this a browser test, but I'm also happy with just a one-off manual
check.)

(2) In the future, please quickly announce on the bug that you are looking into
this, to avoid clashes with others (we had some in the past). I'll do this for
you this time.

Thanks!
Vaclav

https://codereview.chromium.org/2317983002/diff/1/chrome/renderer/autofill/pa...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/2317983002/diff/1/chrome/renderer/autofill/pa...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1094:
static_cast<std::string>(password_element_.suggestedValue().utf8()));
Does this really need the cast? utf8() should already return a std::string [1],
and it seems to be OK without the cast on line 1088. If dropping the cast does
not work, what about:
EXPECT_EQ(std::string(kAlicePassword),
password_element_.suggestedValue().utf8());

Also on line 1105.



[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebSt...

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

https://codereview.chromium.org/2317983002/diff/1/components/autofill/content...
components/autofill/content/renderer/password_autofill_agent.cc:714: } else if
(!username_element.isNull()) {
Should we only execute this branch if also
IsElementAutocompletable(username_element)?

[sense (YandexTeam), 2016-09-07 11:17:33]

On 2016/09/07 08:32:53, vabr (Chromium) wrote:
> Thanks for the patch!
> 
> Looks good, but before approving I left some comments in the code. Also:
> 
> (1) Could you please check what happens, if the user submits the filled
password
> and logs in successfully? Is the combination of the read-only username and the
> filled password saved as a new credential in the password store? (Bonus points
> if you make this a browser test, but I'm also happy with just a one-off manual
> check.)
> 
> (2) In the future, please quickly announce on the bug that you are looking
into
> this, to avoid clashes with others (we had some in the past). I'll do this for
> you this time.
> 
> Thanks!
> Vaclav
> 
>
https://codereview.chromium.org/2317983002/diff/1/chrome/renderer/autofill/pa...
> File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):
> 
>
https://codereview.chromium.org/2317983002/diff/1/chrome/renderer/autofill/pa...
> chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1094:
> static_cast<std::string>(password_element_.suggestedValue().utf8()));
> Does this really need the cast? utf8() should already return a std::string
[1],
> and it seems to be OK without the cast on line 1088. If dropping the cast does
> not work, what about:
> EXPECT_EQ(std::string(kAlicePassword),
> password_element_.suggestedValue().utf8());
> 
> Also on line 1105.
> 
> 
> 
> [1]
>
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebSt...
> 
>
https://codereview.chromium.org/2317983002/diff/1/components/autofill/content...
> File components/autofill/content/renderer/password_autofill_agent.cc (right):
> 
>
https://codereview.chromium.org/2317983002/diff/1/components/autofill/content...
> components/autofill/content/renderer/password_autofill_agent.cc:714: } else if
> (!username_element.isNull()) {
> Should we only execute this branch if also
> IsElementAutocompletable(username_element)?

Thanks for the review!

When a user logins with filled credentials, browser asks him to save it as a new
credential. I'll look if I can make a browser test for that.
About commenting on bug when work is in progress - sure, I'll do it next time.

[sense (YandexTeam), 2016-09-07 11:40:14]

https://codereview.chromium.org/2317983002/diff/1/chrome/renderer/autofill/pa...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/2317983002/diff/1/chrome/renderer/autofill/pa...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1094:
static_cast<std::string>(password_element_.suggestedValue().utf8()));
On 2016/09/07 08:32:53, vabr (Chromium) wrote:
> Does this really need the cast? utf8() should already return a std::string
[1],
> and it seems to be OK without the cast on line 1088. If dropping the cast does
> not work, what about:
> EXPECT_EQ(std::string(kAlicePassword),
> password_element_.suggestedValue().utf8());
> 
> Also on line 1105.
> 
> 
> 
> [1]
>
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebSt...

Yes, I also thought about this cast, but to be consistent with the existing
tests, I left it without changes.
I'll remove casts if it works without them.

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

https://codereview.chromium.org/2317983002/diff/1/components/autofill/content...
components/autofill/content/renderer/password_autofill_agent.cc:714: } else if
(!username_element.isNull()) {
On 2016/09/07 08:32:53, vabr (Chromium) wrote:
> Should we only execute this branch if also
> IsElementAutocompletable(username_element)?
I think it's OK to add this check to exclude possible incorrect behaviour (and
also add it in the PreviewSuggestion).
But I haven't found a form where this branch can be activated - you can't
preview an autofill data by clicking on read-only username field. I tried to
create password form with multiple fields, but suggestion popup activates only
on the username or password fields.

[vabr (Chromium), 2016-09-07 11:55:44]

> Thanks for the review!
> 
> When a user logins with filled credentials, browser asks him to save it as a
new
> credential. I'll look if I can make a browser test for that.
> About commenting on bug when work is in progress - sure, I'll do it next time.

Thanks for checking the behaviour.

I'd actually say: don't worry about writing a browser test for this. It should
be already covered by a combination of the existing tests. I was just a bit
paranoid. :)

Once you update the patch after the comments, please ping me for a re-review.

Cheers,
Vaclav

[sense (YandexTeam), 2016-09-08 04:46:51]

On 2016/09/07 11:55:44, vabr (Chromium) wrote:
> > Thanks for the review!
> > 
> > When a user logins with filled credentials, browser asks him to save it as a
> new
> > credential. I'll look if I can make a browser test for that.
> > About commenting on bug when work is in progress - sure, I'll do it next
time.
> 
> Thanks for checking the behaviour.
> 
> I'd actually say: don't worry about writing a browser test for this. It should
> be already covered by a combination of the existing tests. I was just a bit
> paranoid. :)
> 
> Once you update the patch after the comments, please ping me for a re-review.
> 
> Cheers,
> Vaclav

Done. Please take a look.
I've made small tests refactoring by removing all static_cast<std::string>().

[vabr (Chromium), 2016-09-08 08:48:58]

Thank you! LGTM.
Cheers,
Vaclav

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

https://codereview.chromium.org/2317983002/diff/1/components/autofill/content...
components/autofill/content/renderer/password_autofill_agent.cc:714: } else if
(!username_element.isNull()) {
On 2016/09/07 11:40:14, sense (YandexTeam) wrote:
> On 2016/09/07 08:32:53, vabr (Chromium) wrote:
> > Should we only execute this branch if also
> > IsElementAutocompletable(username_element)?
> I think it's OK to add this check to exclude possible incorrect behaviour (and
> also add it in the PreviewSuggestion).
> But I haven't found a form where this branch can be activated - you can't
> preview an autofill data by clicking on read-only username field. I tried to
> create password form with multiple fields, but suggestion popup activates only
> on the username or password fields.

Ah, you are right, the first if-branch is used instead.
Nevertheless, I think you did the correct think by updating the condition on
this line, because it might happen that the UI will change slightly in the
future.

[sense (YandexTeam), 2016-09-08 08:52:28]

The CQ bit was checked by sense@yandex-team.ru

[commit-bot: I haz the power, 2016-09-08 08:52:41]

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

[commit-bot: I haz the power, 2016-09-08 08:55:29]

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

[commit-bot: I haz the power, 2016-09-08 08:55:30]

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

[sense (YandexTeam), 2016-09-08 11:03:10]

The CQ bit was checked by sense@yandex-team.ru

[commit-bot: I haz the power, 2016-09-08 11:03:23]

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

[commit-bot: I haz the power, 2016-09-08 12:26:23]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-08 12:28:14]

Description was changed from

==========
Enable manual password autofilling if the username field is read-only.

If the username field was read-only, password manager allowed to choose saved
password but failed to insert it into the password field.

BUG=634906
R=vabr@chromium.org
==========

to

==========
Enable manual password autofilling if the username field is read-only.

If the username field was read-only, password manager allowed to choose saved
password but failed to insert it into the password field.

BUG=634906
R=vabr@chromium.org

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

[commit-bot: I haz the power, 2016-09-08 12:28:15]

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