[Password Manager] Suppress UI when password generation is active

[Password Manager] Suppress UI when password generation is active

BUG=498545
Committed: https://crrev.com/cdd33e0d08225a3bf0ed54dd4ae3092fa91e161e
Cr-Commit-Position: refs/heads/master@{#334937}

[Garrett Casto, 2015-06-12 22:09:08]

gcasto@chromium.org changed reviewers:
+ rouslan@chromium.org

[Garrett Casto, 2015-06-12 22:09:08]

Rouslan: Since you originally added the focus code here, would you mind
reviewing this change?

[Garrett Casto, 2015-06-12 23:30:57]

On 2015/06/12 22:09:08, Garrett Casto wrote:
> Rouslan: Since you originally added the focus code here, would you mind
> reviewing this change?

Actually, this doesn't do what I want. I want generation to still trigger even
if the element wasn't clicked, especially since we are going to show the popup
once the user starts typing. Let me think about this some more.

[please use gerrit instead, 2015-06-15 15:47:47]

On 2015/06/12 23:30:57, Garrett Casto wrote:
> On 2015/06/12 22:09:08, Garrett Casto wrote:
> > Rouslan: Since you originally added the focus code here, would you mind
> > reviewing this change?
> 
> Actually, this doesn't do what I want. I want generation to still trigger even
> if the element wasn't clicked, especially since we are going to show the popup
> once the user starts typing. Let me think about this some more.

ok, ping me when you need a review.

[Garrett Casto, 2015-06-15 22:58:58]

gcasto@chromium.org changed reviewers:
+ isherman@chromium.org

[Garrett Casto, 2015-06-15 22:58:58]

+isherman because I want a second pair of eyes on this.

Here is take two. This is simpler but slightly less intuitive. It relies on the
fact that PageClickTracker::FocusChangeComplete() will be called after
AutofillAgent::FocusChangeComplete(). I called this out in a comment and made
sure that the test doesn't hard code the ordering, so on the off chance that
this changes it should break the test.

Long term I think that password autofill should also trigger on focus instead of
on click, but I want to merge this change and it seems more likely that there
would be unintended consequences from a change like that.

[please use gerrit instead, 2015-06-16 00:53:23]

Indeed it's quite tricky. Is there a website where this happens, so I can try it
out? It does not seem to trigger on the amazon change password page, the
screenshot of which is attached to the bug report.

https://codereview.chromium.org/1184913002/diff/40001/chrome/renderer/autofil...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/1184913002/diff/40001/chrome/renderer/autofil...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:21: #include
"content/public/renderer/render_view.h"
Is this include necessary?

https://codereview.chromium.org/1184913002/diff/40001/chrome/renderer/autofil...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1908:
SimulateElementClick("old_password");
Shouldn't password generation trigger on "new_password"?

https://codereview.chromium.org/1184913002/diff/40001/chrome/renderer/autofil...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1912:
EXPECT_FALSE(message);
EXPECT_EQ(nullptr,
render_thread_->sink().GetFirstMessageMatching(AutofillHostMsg_ShowPasswordSuggestions::ID));

https://codereview.chromium.org/1184913002/diff/40001/chrome/renderer/autofil...
File chrome/renderer/autofill/password_generation_test_utils.cc (right):

https://codereview.chromium.org/1184913002/diff/40001/chrome/renderer/autofil...
chrome/renderer/autofill/password_generation_test_utils.cc:13: #include
"third_party/WebKit/public/platform/WebString.h"
Why this include?

[Ilya Sherman, 2015-06-16 02:24:10]

Seems ok to me, though I agree that it would be better to find a less fragile
solution.  Do we really want one UI to categorically trump the other, or should
there be some sort of conflict resolution procedure in place when we'd try to
show both UIs?

https://codereview.chromium.org/1184913002/diff/40001/chrome/renderer/autofil...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/1184913002/diff/40001/chrome/renderer/autofil...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1914: true);
nit: No need to wrap here.  (Might want to run "git cl format".)

https://codereview.chromium.org/1184913002/diff/40001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/1184913002/diff/40001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:674: return;
This is a small bugfix that's otherwise unrelated to your change, right?

https://codereview.chromium.org/1184913002/diff/40001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.h (right):

https://codereview.chromium.org/1184913002/diff/40001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.h:272: // for the password
manager. TODO(gcasto): Have both UIs show on focus.
Long-term, should there be a combined UI?  Or are we sure that the generation
popup should always trump the password autofill popup?

[Garrett Casto, 2015-06-16 06:15:05]

Testing basically requires patching in
https://codereview.chromium.org/1161023008/ and then going to site which
triggers generation and clicking directly on the password field (if you click on
the username field first the manager UI won't show on the password field).

https://codereview.chromium.org/1184913002/diff/40001/chrome/renderer/autofil...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/1184913002/diff/40001/chrome/renderer/autofil...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:21: #include
"content/public/renderer/render_view.h"
On 2015/06/16 00:53:23, Rouslan wrote:
> Is this include necessary?

Leftover from debugging, removed.

https://codereview.chromium.org/1184913002/diff/40001/chrome/renderer/autofil...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1908:
SimulateElementClick("old_password");
On 2015/06/16 00:53:23, Rouslan wrote:
> Shouldn't password generation trigger on "new_password"?

In general yes. The test for this is slightly contrived at the moment because
for both generation and management to trigger at the same time basically
requires something to be wrong (either generation triggers on the wrong password
field or the manager fils into the wrong field). I changed this to be a signup
form where we fill, as this will be the compelling case going forward due to
https://codereview.chromium.org/1161023008/

https://codereview.chromium.org/1184913002/diff/40001/chrome/renderer/autofil...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1912:
EXPECT_FALSE(message);
On 2015/06/16 00:53:23, Rouslan wrote:
> EXPECT_EQ(nullptr,
>
render_thread_->sink().GetFirstMessageMatching(AutofillHostMsg_ShowPasswordSuggestions::ID));

Done.

https://codereview.chromium.org/1184913002/diff/40001/chrome/renderer/autofil...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1914: true);
On 2015/06/16 02:24:09, Ilya Sherman wrote:
> nit: No need to wrap here.  (Might want to run "git cl format".)

Done.

https://codereview.chromium.org/1184913002/diff/40001/chrome/renderer/autofil...
File chrome/renderer/autofill/password_generation_test_utils.cc (right):

https://codereview.chromium.org/1184913002/diff/40001/chrome/renderer/autofil...
chrome/renderer/autofill/password_generation_test_utils.cc:13: #include
"third_party/WebKit/public/platform/WebString.h"
On 2015/06/16 00:53:23, Rouslan wrote:
> Why this include?

Leftover from an earlier iteration. Removed.

https://codereview.chromium.org/1184913002/diff/40001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/1184913002/diff/40001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:674: return;
On 2015/06/16 02:24:09, Ilya Sherman wrote:
> This is a small bugfix that's otherwise unrelated to your change, right?

Yes. My original intent was to not introduce a new boolean and just use
|is_popup_possibly_visible_|. However, this has the side effect of not
re-showing Autofill UI if you dismiss it without changing focus (e.g. pressing
escape). I left this change in since it's slightly more efficient.

https://codereview.chromium.org/1184913002/diff/40001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.h (right):

https://codereview.chromium.org/1184913002/diff/40001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.h:272: // for the password
manager. TODO(gcasto): Have both UIs show on focus.
On 2015/06/16 02:24:09, Ilya Sherman wrote:
> Long-term, should there be a combined UI?  Or are we sure that the generation
> popup should always trump the password autofill popup?

Really only one of these should be active at any time. If the password field is
for a new password, then we should only show generation, if it's for a current
password we should only show the password management UI. At the moment there are
going to be cases where both trigger because heuristics aren't perfect, but it's
quite rare since we generally don't show the password manager UI when the
password field is focused. 

With increased support for change password forms this conflict may start
happening more often. It's possible that we will need some way to disambiguate, 
we'll just have to see how often it happens.

[please use gerrit instead, 2015-06-16 16:07:48]

lgtm

[Ilya Sherman, 2015-06-16 19:49:24]

https://codereview.chromium.org/1184913002/diff/40001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.h (right):

https://codereview.chromium.org/1184913002/diff/40001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.h:272: // for the password
manager. TODO(gcasto): Have both UIs show on focus.
On 2015/06/16 06:15:05, Garrett Casto wrote:
> On 2015/06/16 02:24:09, Ilya Sherman wrote:
> > Long-term, should there be a combined UI?  Or are we sure that the
generation
> > popup should always trump the password autofill popup?
> 
> Really only one of these should be active at any time. If the password field
is
> for a new password, then we should only show generation, if it's for a current
> password we should only show the password management UI. At the moment there
are
> going to be cases where both trigger because heuristics aren't perfect, but
it's
> quite rare since we generally don't show the password manager UI when the
> password field is focused. 
> 
> With increased support for change password forms this conflict may start
> happening more often. It's possible that we will need some way to
disambiguate, 
> we'll just have to see how often it happens.

Should we record some metrics to verify whether this is indeed common or rare?

[Garrett Casto, 2015-06-17 19:48:10]

https://codereview.chromium.org/1184913002/diff/40001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.h (right):

https://codereview.chromium.org/1184913002/diff/40001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.h:272: // for the password
manager. TODO(gcasto): Have both UIs show on focus.
On 2015/06/16 19:49:23, Ilya Sherman wrote:
> On 2015/06/16 06:15:05, Garrett Casto wrote:
> > On 2015/06/16 02:24:09, Ilya Sherman wrote:
> > > Long-term, should there be a combined UI?  Or are we sure that the
> generation
> > > popup should always trump the password autofill popup?
> > 
> > Really only one of these should be active at any time. If the password field
> is
> > for a new password, then we should only show generation, if it's for a
current
> > password we should only show the password management UI. At the moment there
> are
> > going to be cases where both trigger because heuristics aren't perfect, but
> it's
> > quite rare since we generally don't show the password manager UI when the
> > password field is focused. 
> > 
> > With increased support for change password forms this conflict may start
> > happening more often. It's possible that we will need some way to
> disambiguate, 
> > we'll just have to see how often it happens.
> 
> Should we record some metrics to verify whether this is indeed common or rare?

I added something in the latest patch. Even if this ends up showing a large
amount of conflicts, I'm not totally sure what we would do about it since we
don't know for certain which is correct, but in all cases I know of now
generation is the right UI if they are in conflict.

[Ilya Sherman, 2015-06-17 20:16:45]

LGTM, thanks.

https://codereview.chromium.org/1184913002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1184913002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:26972: +    enum="Boolean">
nit: Please use a custom boolean enum, with labels like "Shown" (or maybe "Not
suppressed"?) or "Suppressed"

https://codereview.chromium.org/1184913002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:26975: +    If the password manager ui
was suppressed because generation UI was already
nit: "ui" -> "UI"

[Garrett Casto, 2015-06-17 20:47:25]

https://codereview.chromium.org/1184913002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1184913002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:26972: +    enum="Boolean">
On 2015/06/17 20:16:45, Ilya Sherman wrote:
> nit: Please use a custom boolean enum, with labels like "Shown" (or maybe "Not
> suppressed"?) or "Suppressed"

Switch to BooleanSuppressed.

https://codereview.chromium.org/1184913002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:26975: +    If the password manager ui
was suppressed because generation UI was already
On 2015/06/17 20:16:45, Ilya Sherman wrote:
> nit: "ui" -> "UI"

Done.

[Garrett Casto, 2015-06-17 20:47:31]

The CQ bit was checked by gcasto@chromium.org

[Garrett Casto, 2015-06-17 20:47:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
rouslan@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1184913002/#ps100001
(title: "Comments")

[commit-bot: I haz the power, 2015-06-17 20:50:34]

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

[commit-bot: I haz the power, 2015-06-17 22:10:14]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-06-17 22:11:54]

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