Allow only a user gesture to trigger autofill popup

[autofill] Allow only a user gesture to trigger autofill.

If a script inserts text into an input field without a user gesture,
then do not show the autofill popup.

TEST=AutofillRendererTest.IgnoreNonUserGestureTextFieldChanges
BUG=353001
Committed: https://crrev.com/6a3f8d9afcbb397f562e3a359ffbe39d59d278b9
Cr-Commit-Position: refs/heads/master@{#327204}

[please use gerrit instead, 2015-03-20 17:49:34]

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

[please use gerrit instead, 2015-03-20 17:49:34]

Evan, PTAL Patch Set 2.

[please use gerrit instead, 2015-03-20 21:19:46]

Please don't review yet. User gesture requirement breaks a bunch of automated
tests, which trigger autofill popup without a user gesture...

[please use gerrit instead, 2015-03-20 22:08:40]

rouslan@chromium.org changed reviewers:
- estade@chromium.org

[please use gerrit instead, 2015-03-23 21:50:51]

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

[please use gerrit instead, 2015-03-23 21:50:52]

Evan, PTAL Patch Set 3.

[Evan Stade, 2015-03-23 22:07:12]

test?

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

https://codereview.chromium.org/1026493002/diff/40001/chrome/renderer/autofil...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:887:
->DisableUserGestureCheckForTests();
This doesn't seem right to me. Instead of modifying production code execution
during tests (i.e. testing something that's not the real code), we should
simulate a user gesture. IOW, fix SimulateInputChangeForElement

[please use gerrit instead, 2015-03-25 00:17:09]

On 2015/03/23 22:07:12, Evan Stade wrote:
> test?
> 
>
https://codereview.chromium.org/1026493002/diff/40001/chrome/renderer/autofil...
> File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):
> 
>
https://codereview.chromium.org/1026493002/diff/40001/chrome/renderer/autofil...
> chrome/renderer/autofill/password_autofill_agent_browsertest.cc:887:
> ->DisableUserGestureCheckForTests();
> This doesn't seem right to me. Instead of modifying production code execution
> during tests (i.e. testing something that's not the real code), we should
> simulate a user gesture. IOW, fix SimulateInputChangeForElement

I can think of 2 ways to simulate a user gesture: (1) Send
InputMsg_HandleInputEvent to RenderViewImpl; (2) Tell Blink that it's
proccessing a user gesture.

Option (1) is already done in RenderViewImplTest::SendKeyEvent(), which is ~100
line method with platform-specific implementations. Using this method requires
moving it to RenderViewTest.

Option (2) requires enabling to construct a UserGestureIndicator from within
WebUserGestureIndicator.

Although option (2) will be a smaller patch, I feel that it has two
disadvantages: (a) A Blink change will take a while to roll into Chrome; (b)
Blink's public API will include a function used only in tests.

Therefore, I am going with option (1) for now. Let me know if you can think of
options 3+ that may be better.

[please use gerrit instead, 2015-04-13 23:24:52]

FYI, Android keyboard often uses composition to insert the text. Chrome does not
consider composition to be a user gesture. Applying this patch will prevent
showing autofill popups until keyboard finishes composition. This would cause a
regression when filling forms. I'm investigating how to make composition be a
user gesture on Android.

[please use gerrit instead, 2015-04-14 00:49:34]

Aurimas says IME composition on Android should be treated as a user gesture. So
this is the correct approach.

[please use gerrit instead, 2015-04-15 21:17:56]

blink-dev@ also says IME composition on Android can be treated as a user
gesture.

[Evan Stade, 2015-04-16 13:58:59]

didn't review tests yet, non-test code mostly lg

https://codereview.chromium.org/1026493002/diff/280001/components/autofill/co...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/1026493002/diff/280001/components/autofill/co...
components/autofill/content/renderer/autofill_agent.cc:398: if
(did_set_node_text_) {
there are a lot of early returns here. Can we make them a little more sane?

1) move DCHECK to top
2) get rid of did_set_node_text_ (re-use ignore_text_changes_)

https://codereview.chromium.org/1026493002/diff/280001/content/renderer/rende...
File content/renderer/render_widget.cc (left):

https://codereview.chromium.org/1026493002/diff/280001/content/renderer/rende...
content/renderer/render_widget.cc:1692: if
(!webwidget_->setUserGestureComposition(
why can't I find the old function anywhere?

[please use gerrit instead, 2015-04-16 16:08:35]

Thank you for the review, but the patch is not complete yet, sorry.

https://codereview.chromium.org/1026493002/diff/280001/content/renderer/rende...
File content/renderer/render_widget.cc (left):

https://codereview.chromium.org/1026493002/diff/280001/content/renderer/rende...
content/renderer/render_widget.cc:1692: if
(!webwidget_->setUserGestureComposition(
On 2015/04/16 13:58:59, Evan Stade wrote:
> why can't I find the old function anywhere?

That's a function that I was going to add, but decided to not add later on. This
patch is bad. I will re-upload a better one once I solve the 2 remaining issues:

1) Let autofill_agent know when compositing is going on. (hasCompoistion does
not do what we want yet.)

2) Simulate key presses on Mac.

[please use gerrit instead, 2015-04-17 20:54:36]

Evan, PTAL Patch Set 23.

Some android bots will continue to fail until the Blink change
http://crrev.com/1091573003 is rolled into Chrome.

[Evan Stade, 2015-04-17 20:58:58]

https://codereview.chromium.org/1026493002/diff/440001/components/autofill/co...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/1026493002/diff/440001/components/autofill/co...
components/autofill/content/renderer/autofill_agent.cc:416: // IME composition
on desktop is used for CJK, which will overlap with
hmm, wouldn't we need to hide any pre-existing popup?

[please use gerrit instead, 2015-04-17 22:25:46]

Evan, PTAL Patch Set 24.

AutofillRendererTest.DesktopCompositionDoesNotTriggerAutofill will be failing
until a Blink roll. It cannot verify that the popup hides on IME, because IME is
not a user gesture until the Blink roll.

https://codereview.chromium.org/1026493002/diff/440001/components/autofill/co...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/1026493002/diff/440001/components/autofill/co...
components/autofill/content/renderer/autofill_agent.cc:416: // IME composition
on desktop is used for CJK, which will overlap with
On 2015/04/17 20:58:58, Evan Stade wrote:
> hmm, wouldn't we need to hide any pre-existing popup?

Done.

[Evan Stade, 2015-04-20 22:40:31]

https://codereview.chromium.org/1026493002/diff/460001/components/autofill/co...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/1026493002/diff/460001/components/autofill/co...
components/autofill/content/renderer/autofill_agent.cc:412: HidePopup();
as per offline convo I think we should show autofill suggestion + ime
composition window at the same time

https://codereview.chromium.org/1026493002/diff/460001/components/autofill/co...
components/autofill/content/renderer/autofill_agent.cc:727: ignore_text_changes_
= false;
use base::AutoReset for this

[please use gerrit instead, 2015-04-20 23:51:27]

Evan, PTAL Patch Set 25.

https://codereview.chromium.org/1026493002/diff/460001/components/autofill/co...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/1026493002/diff/460001/components/autofill/co...
components/autofill/content/renderer/autofill_agent.cc:412: HidePopup();
On 2015/04/20 22:40:30, Evan Stade wrote:
> as per offline convo I think we should show autofill suggestion + ime
> composition window at the same time

Done.

https://codereview.chromium.org/1026493002/diff/460001/components/autofill/co...
components/autofill/content/renderer/autofill_agent.cc:727: ignore_text_changes_
= false;
On 2015/04/20 22:40:30, Evan Stade wrote:
> use base::AutoReset for this

Done.

[Evan Stade, 2015-04-21 00:54:30]

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

[Evan Stade, 2015-04-21 00:54:31]

+gcasto as it seems this affects behavior of passwords fields as well. Garrett
can you confirm this change is desirable?

for autofill lgtm

https://codereview.chromium.org/1026493002/diff/480001/components/autofill/co...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/1026493002/diff/480001/components/autofill/co...
components/autofill/content/renderer/autofill_agent.cc:716:
base::AutoReset<bool> auto_ignore_text_changes(&ignore_text_changes_, true);
nit: s/auto_long_and_redundant_variable_name/auto

[please use gerrit instead, 2015-04-21 17:02:47]

https://codereview.chromium.org/1026493002/diff/480001/components/autofill/co...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/1026493002/diff/480001/components/autofill/co...
components/autofill/content/renderer/autofill_agent.cc:716:
base::AutoReset<bool> auto_ignore_text_changes(&ignore_text_changes_, true);
On 2015/04/21 00:54:31, Evan Stade wrote:
> nit: s/auto_long_and_redundant_variable_name/auto

Renamed to auto_reset, because "auto" is a language keyword.

[Garrett Casto, 2015-04-21 21:07:15]

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

[Garrett Casto, 2015-04-21 21:07:16]

LGTM

+vabr@ FYI

I think this is fine. Since the password manager autofills on page load this
doesn't really change the attack surface much (harder to scrape multiple
passwords I suppose) but I don't think that it affects usability. If a site is
setup such that script is meant to affect the username or password it might
cause some UX issues (password and username might not match) but I think that's
unlikely. I know of no such sites, and even if they existed it's likely that the
scripts would run after a user gesture, so it wouldn't be affected by this
change.

[please use gerrit instead, 2015-04-21 21:32:06]

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

[please use gerrit instead, 2015-04-21 21:32:07]

Brett, OWNERS PTAL in Patch Set 26:

 content/public/test/render_view_test.h
 content/public/test/render_view_test.cc

[please use gerrit instead, 2015-04-21 21:35:40]

rouslan@chromium.org changed reviewers:
+ jcivelli@chromium.org
- brettw@chromium.org

[please use gerrit instead, 2015-04-21 21:35:41]

Actually, Jay would be a better OWNER for these particular files.

- Brett

+ Jay, OWNERS PTAL in Patch Set 26:

 content/public/test/render_view_test.h
 content/public/test/render_view_test.cc

[Jay Civelli, 2015-04-23 21:57:43]

https://codereview.chromium.org/1026493002/diff/500001/content/public/test/re...
File content/public/test/render_view_test.cc (right):

https://codereview.chromium.org/1026493002/diff/500001/content/public/test/re...
content/public/test/render_view_test.cc:448: void
RenderViewTest::ProcessInputForAutofill() {
I am confused on whether this actually does something that triggers processing
of autofill or if it makes us wait for a state when we know auotfill has been
applied.

https://codereview.chromium.org/1026493002/diff/500001/content/public/test/re...
content/public/test/render_view_test.cc:467:
input->document().frame()->view()->advanceFocus(false);
Wouldn't simulating a click on the input element achieve the same result in a
more straight-forward way?

https://codereview.chromium.org/1026493002/diff/500001/content/public/test/re...
content/public/test/render_view_test.cc:474:
ASSERT_TRUE(base::IsStringASCII(new_value));
Nit: this assert should probably be moved to the top of the method.

https://codereview.chromium.org/1026493002/diff/500001/content/public/test/re...
File content/public/test/render_view_test.h (right):

https://codereview.chromium.org/1026493002/diff/500001/content/public/test/re...
content/public/test/render_view_test.h:131: // Simulate typing the
|ascii_character| into the browser. Also accepts
"Simulates"
Also "into the browser" -> "into this render view"

[please use gerrit instead, 2015-04-27 21:20:34]

Jay, PTAL Patch Set 27 and comments inline.

https://codereview.chromium.org/1026493002/diff/500001/content/public/test/re...
File content/public/test/render_view_test.cc (right):

https://codereview.chromium.org/1026493002/diff/500001/content/public/test/re...
content/public/test/render_view_test.cc:448: void
RenderViewTest::ProcessInputForAutofill() {
On 2015/04/23 21:57:43, Jay Civelli wrote:
> I am confused on whether this actually does something that triggers processing
> of autofill or if it makes us wait for a state when we know auotfill has been
> applied.

Running the message loop will trigger autofill.

When a text changes in an input field,  AutofillAgent::textFieldDidChange will
post the function that will trigger the autofill field. The actual autofill
happens in AutofillAgent::TextFieldDidChangeImpl.

https://codereview.chromium.org/1026493002/diff/500001/content/public/test/re...
content/public/test/render_view_test.cc:467:
input->document().frame()->view()->advanceFocus(false);
I tried that and for some reason focus does not change in Blink. I'm not sure
what's at play here, but I'm leaning toward keeping that code as is, because it
remains after copying it from
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/au....
What do you think?

https://codereview.chromium.org/1026493002/diff/500001/content/public/test/re...
content/public/test/render_view_test.cc:474:
ASSERT_TRUE(base::IsStringASCII(new_value));
On 2015/04/23 21:57:43, Jay Civelli wrote:
> Nit: this assert should probably be moved to the top of the method.

Done.

https://codereview.chromium.org/1026493002/diff/500001/content/public/test/re...
File content/public/test/render_view_test.h (right):

https://codereview.chromium.org/1026493002/diff/500001/content/public/test/re...
content/public/test/render_view_test.h:131: // Simulate typing the
|ascii_character| into the browser. Also accepts
On 2015/04/23 21:57:43, Jay Civelli wrote:
> "Simulates"
> Also "into the browser" -> "into this render view"

Done.

[Jay Civelli, 2015-04-27 22:17:19]

https://codereview.chromium.org/1026493002/diff/500001/content/public/test/re...
File content/public/test/render_view_test.cc (right):

https://codereview.chromium.org/1026493002/diff/500001/content/public/test/re...
content/public/test/render_view_test.cc:448: void
RenderViewTest::ProcessInputForAutofill() {
On 2015/04/27 21:20:34, Rouslan Solomakhin wrote:
> On 2015/04/23 21:57:43, Jay Civelli wrote:
> > I am confused on whether this actually does something that triggers
processing
> > of autofill or if it makes us wait for a state when we know auotfill has
been
> > applied.
> 
> Running the message loop will trigger autofill.
> 
> When a text changes in an input field,  AutofillAgent::textFieldDidChange will
> post the function that will trigger the autofill field. The actual autofill
> happens in AutofillAgent::TextFieldDidChangeImpl.

OK, why do we need to re-layout though? Wouldn't the
AutofillAgent::textFieldDidChange be called without it?
I also wonder if we could merge this method with
SimulateUserTypingASCIICharacter.
It could take a bool parameter to specify whether to flush the message loop.
WDYT?

https://codereview.chromium.org/1026493002/diff/500001/content/public/test/re...
content/public/test/render_view_test.cc:467:
input->document().frame()->view()->advanceFocus(false);
On 2015/04/27 21:20:34, Rouslan Solomakhin wrote:
> I tried that and for some reason focus does not change in Blink. I'm not sure
> what's at play here, but I'm leaning toward keeping that code as is, because
it
> remains after copying it from
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/au....
> What do you think?

Sounds good.

[please use gerrit instead, 2015-04-27 23:49:55]

Jay, PTAL Patch Set 28.

https://codereview.chromium.org/1026493002/diff/500001/content/public/test/re...
File content/public/test/render_view_test.cc (right):

https://codereview.chromium.org/1026493002/diff/500001/content/public/test/re...
content/public/test/render_view_test.cc:448: void
RenderViewTest::ProcessInputForAutofill() {
On 2015/04/27 22:17:19, Jay Civelli wrote:
> On 2015/04/27 21:20:34, Rouslan Solomakhin wrote:
> > On 2015/04/23 21:57:43, Jay Civelli wrote:
> > > I am confused on whether this actually does something that triggers
> processing
> > > of autofill or if it makes us wait for a state when we know auotfill has
> been
> > > applied.
> > 
> > Running the message loop will trigger autofill.
> > 
> > When a text changes in an input field,  AutofillAgent::textFieldDidChange
will
> > post the function that will trigger the autofill field. The actual autofill
> > happens in AutofillAgent::TextFieldDidChangeImpl.
> 
> OK, why do we need to re-layout though? Wouldn't the
> AutofillAgent::textFieldDidChange be called without it?
> I also wonder if we could merge this method with
> SimulateUserTypingASCIICharacter.
> It could take a bool parameter to specify whether to flush the message loop.
> WDYT?

It seems we don't need re-layout. The original comment must
have been outdated. After removing re-layout from tests, all tests are still
passing.

I've added |flush_message_loop| parameter to SimulateUserTypingASCIICharacter
and removed ProcessInputForAutofill entirely.

[Jay Civelli, 2015-04-27 23:57:10]

Thanks for the changes!

LGTM

[please use gerrit instead, 2015-04-28 00:13:01]

The CQ bit was checked by rouslan@chromium.org

[please use gerrit instead, 2015-04-28 00:13:01]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org,
gcasto@chromium.org
Link to the patchset: https://codereview.chromium.org/1026493002/#ps540001
(title: "Do not layout frame.")

[commit-bot: I haz the power, 2015-04-28 00:13:39]

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

[commit-bot: I haz the power, 2015-04-28 01:00:13]

Committed patchset #28 (id:540001)

[commit-bot: I haz the power, 2015-04-28 01:01:13]

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

[Nico, 2015-05-10 21:42:56]

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

[Nico, 2015-05-10 21:42:57]

Looks like PasswordAutofillAgentTest.ClearPreviewWithInlineAutocompletedUsername
started failing consistently on DrMemory after this CL:

http://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Browser%20...

[please use gerrit instead, 2015-05-11 16:34:12]

On 2015/05/10 21:42:57, Nico wrote:
> Looks like
PasswordAutofillAgentTest.ClearPreviewWithInlineAutocompletedUsername
> started failing consistently on DrMemory after this CL:
> 
>
http://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Browser%20...

Yes, there's a bug for me here:
https://code.google.com/p/chromium/issues/detail?id=484910

Looks like a timeout. I will investigate.