Initialize UserGestureIndicator for IME commitText.

Initialize UserGestureIndicator for IME commitText.

Autofill code ignores non-user-gesture-based form updates since they are often
greyed-out filler text or suggestions.  KeyDown events as well as
IME setComposition were already correctly registered as user gestures, but
commitText was not.  This could cause autofill to save partially typed form
fields.

BUG=582401
Committed: https://crrev.com/6ad2101efa6830ca3f9d156d662b977707b8ceff
Cr-Commit-Position: refs/heads/master@{#374340}

[pals, 2016-01-29 05:55:05]

The CQ bit was checked by sanjoy.pal@samsung.com to run a CQ dry run

[pals, 2016-01-29 05:55:18]

The CQ bit was unchecked by sanjoy.pal@samsung.com

[pals, 2016-01-29 09:23:38]

The CQ bit was checked by sanjoy.pal@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-01-29 09:24:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1644993002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1644993002/1

[ashlin.j, 2016-01-29 11:03:01]

Description was changed from

==========
[SamsungPeerReview] fixup for https://codereview.chromium.org/1403453002

BUG=574653
==========

to

==========
Sending keyEvents instead of sending synthetic keyevents and calling commitText
explicitly for non-composited text.

BUG=582401
==========

[ashlin.j, 2016-01-29 11:03:01]

ashlin.j@samsung.com changed reviewers:
+ aelias@chromium.org, aurimas@chromium.org, vabr@chromium.org

[commit-bot: I haz the power, 2016-01-29 11:42:28]

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

[commit-bot: I haz the power, 2016-01-29 11:42:28]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[pals, 2016-02-01 12:30:17]

The CQ bit was checked by sanjoy.pal@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-02-01 12:32:25]

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

[commit-bot: I haz the power, 2016-02-01 12:32:26]

Dry run: CQ cannot get SignCLA result. Please try later.

[pals, 2016-02-02 03:35:29]

The CQ bit was checked by sanjoy.pal@samsung.com to run a CQ dry run

[pals, 2016-02-02 03:35:48]

The CQ bit was unchecked by sanjoy.pal@samsung.com

[pals, 2016-02-02 11:21:21]

The CQ bit was checked by sanjoy.pal@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-02-02 11:21:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1644993002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1644993002/60001

[commit-bot: I haz the power, 2016-02-02 12:18:27]

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

[commit-bot: I haz the power, 2016-02-02 12:18:28]

Dry run: This issue passed the CQ dry run.

[ashlin.j, 2016-02-02 12:52:19]

Please take a look

[vabr (Chromium), 2016-02-02 12:58:02]

Thanks for your fix.
I defer the review to the code OWNERS, though, because I am not familiar with
the changed code.

Looking into
content/public/android/java/src/org/chromium/content/browser/input/OWNERS, you
already have two of them (aelias@ and aurimas@) among your reviewers.

Cheers,
Vaclav

[vabr (Chromium), 2016-02-02 12:58:20]

vabr@chromium.org changed reviewers:
- vabr@chromium.org

[ashlin.j, 2016-02-02 13:02:17]

@vabr Will get it reviewed by the OWNERS of the file.

@aelias @aurimas PTAL .

[aelias_OOO_until_Jul13, 2016-02-02 22:52:11]

aelias@chromium.org changed reviewers:
+ changwan@chromium.org
- aurimas@chromium.org

[aelias_OOO_until_Jul13, 2016-02-02 23:00:00]

According to your investigation on http://crbug.com/582401, it seems like the
bug is that autofill doesn't understand IME compositions properly.  We should
change the autofill code to be aware of them instead of bringing back the
guessing hack in the browser.  For example, AutofillAgent::textFieldDidChange
could also check InputMethodController::hasComposition() (looks like that would
require a bit of new plumbing through public/web).

[ashlin.j, 2016-02-03 15:59:53]

On 2016/02/02 23:00:00, aelias wrote:
> According to your investigation on http://crbug.com/582401, it seems like the
> bug is that autofill doesn't understand IME compositions properly.  We should
> change the autofill code to be aware of them instead of bringing back the
> guessing hack in the browser.  For example, AutofillAgent::textFieldDidChange
> could also check InputMethodController::hasComposition() (looks like that
would
> require a bit of new plumbing through public/web).

I tried to use InputMethodController::hasComposition() but it returning as 0 in
all cases i.e adding/removing characters from input fields.
Trying different approach now. Will update if it works fine 
Thanks

[pals, 2016-02-05 10:09:15]

The CQ bit was checked by sanjoy.pal@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-02-05 10:09:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1644993002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1644993002/100001

[commit-bot: I haz the power, 2016-02-05 10:28:08]

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

[commit-bot: I haz the power, 2016-02-05 10:28:09]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[sohanjg, 2016-02-05 13:02:56]

The CQ bit was checked by sohan.jyoti@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-02-05 13:03:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1644993002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1644993002/120001

[commit-bot: I haz the power, 2016-02-05 14:11:57]

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

[commit-bot: I haz the power, 2016-02-05 14:11:58]

Dry run: This issue passed the CQ dry run.

[ashlin.j, 2016-02-05 15:02:44]

ashlin.j@samsung.com changed reviewers:
- sohan.jyoti@samsung.com

[ashlin.j, 2016-02-05 15:02:44]

@aelias
I have uploaded a new approach for solving the issue.
The issue was while editing a textfield, in ImeAdapeter.java
inside sendCompositionToNative() api nativeSetComposingText() is called where
userGestureIndicator is initialized . But when a password field is edited, in
ImeAdapeter.java
inside sendCompositionToNative() api nativeCommitText() is called and
userGestureIndicator is not initialized and thereby inside
AutofillAgent::textFieldDidChange() flow returns because of the check
(!IsUserGesture() && !render_frame()->IsPasting()). 
So initializing UserGestureIndicator while editing password field.

Please take a look at the change

[aelias_OOO_until_Jul13, 2016-02-05 20:15:00]

Looks like a good approach, thanks.  Could you also:

- Write a test checking the user gesture indicator is set properly (probably in
WebViewTest.cpp)
- Update your change description to explain what you're doing now and why.

https://codereview.chromium.org/1644993002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1644993002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:2401: bool
WebViewImpl::confirmComposition(const WebString& text,
ConfirmCompositionBehavior selectionBehavior)
Please move the UserGestureIndicator line here since all the
confirmComposition() calls wind up here and they all look like user-originating
events.

[ashlin.j, 2016-02-08 11:02:36]

ashlin.j@samsung.com changed reviewers:
- changwan@chromium.org, sanjoy.pal@samsung.com

[ashlin.j, 2016-02-08 11:02:36]

https://codereview.chromium.org/1644993002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1644993002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:2401: bool
WebViewImpl::confirmComposition(const WebString& text,
ConfirmCompositionBehavior selectionBehavior)
On 2016/02/05 20:15:00, aelias wrote:
> Please move the UserGestureIndicator line here since all the
> confirmComposition() calls wind up here and they all look like
user-originating
> events.

Not adding the initialization here because confirmComposition(const WebString&
text, ConfirmCompositionBehavior selectionBehavior) could be called even if the
password field is focused by javaScript with no text entered. Which needn't be
an user gesture.
Had added the initialization in this api (same as your comment) in patch set 6
and some of the tests had failed because of the above stated reasons.
Thus, updated the initialization to confirmComposition(const WebString& text)

[ashlin.j, 2016-02-08 11:08:40]

Description was changed from

==========
Sending keyEvents instead of sending synthetic keyevents and calling commitText
explicitly for non-composited text.

BUG=582401
==========

to

==========
Initializing userGestureIndicator() while editing password fields in a form.
Thereby, the value for the password that gets saved is updated for each
character added/removed by the user.

BUG=582401
==========

[ashlin.j, 2016-02-08 11:11:57]

@aelias
I have added the unit test to WebviewTest.cpp
and also updated the description as per the new approach.
Also check the inlne comment.
Is anything else need to be done ?

Please take a look
Thanks

[aelias_OOO_until_Jul13, 2016-02-08 22:06:04]

OK, lgtm modulo test comment below.

https://codereview.chromium.org/1644993002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/1644993002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:3219: #if OS(ANDROID)
As far as I can tell, this test should pass on every platform, so can you remove
#if OS(ANDROID)?

[aelias_OOO_until_Jul13, 2016-02-09 00:36:35]

Description was changed from

==========
Initializing userGestureIndicator() while editing password fields in a form.
Thereby, the value for the password that gets saved is updated for each
character added/removed by the user.

BUG=582401
==========

to

==========
Initialize userGestureIndicator() for IME commitText.

Autofill code ignores non-user-gesture-based form updates since they are often
greyed-out filler text or suggestions.  KeyDown events as well as
IME setComposition were already correctly registered as user gestures, but
commitText was not.  This could cause autofill to save partially
typed form fields.

BUG=582401
==========

[aelias_OOO_until_Jul13, 2016-02-09 00:36:47]

Description was changed from

==========
Initialize userGestureIndicator() for IME commitText.

Autofill code ignores non-user-gesture-based form updates since they are often
greyed-out filler text or suggestions.  KeyDown events as well as
IME setComposition were already correctly registered as user gestures, but
commitText was not.  This could cause autofill to save partially
typed form fields.

BUG=582401
==========

to

==========
Initialize userGestureIndicator() for IME commitText.

Autofill code ignores non-user-gesture-based form updates since they are often
greyed-out filler text or suggestions.  KeyDown events as well as
IME setComposition were already correctly registered as user gestures, but
commitText was not.  This could cause autofill to save partially typed form
fields.

BUG=582401
==========

[aelias_OOO_until_Jul13, 2016-02-09 00:37:34]

Description was changed from

==========
Initialize userGestureIndicator() for IME commitText.

Autofill code ignores non-user-gesture-based form updates since they are often
greyed-out filler text or suggestions.  KeyDown events as well as
IME setComposition were already correctly registered as user gestures, but
commitText was not.  This could cause autofill to save partially typed form
fields.

BUG=582401
==========

to

==========
Initialize UserGestureIndicator for IME commitText.

Autofill code ignores non-user-gesture-based form updates since they are often
greyed-out filler text or suggestions.  KeyDown events as well as
IME setComposition were already correctly registered as user gestures, but
commitText was not.  This could cause autofill to save partially typed form
fields.

BUG=582401
==========

[ashlin.j, 2016-02-09 07:43:22]

@alieas

I have updated WebViewTest.cpp as per your comments.
PTAL
Thanks for updating the commit message and for guiding me through the patch.

[aelias_OOO_until_Jul13, 2016-02-09 07:48:16]

Sure, thanks!  Still lgtm, you can press "Commit" when you want.

[ashlin.j, 2016-02-09 07:49:02]

The CQ bit was checked by ashlin.j@samsung.com

[commit-bot: I haz the power, 2016-02-09 07:49:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1644993002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1644993002/160001

[commit-bot: I haz the power, 2016-02-09 09:07:39]

Committed patchset #9 (id:160001)

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

Description was changed from

==========
Initialize UserGestureIndicator for IME commitText.

Autofill code ignores non-user-gesture-based form updates since they are often
greyed-out filler text or suggestions.  KeyDown events as well as
IME setComposition were already correctly registered as user gestures, but
commitText was not.  This could cause autofill to save partially typed form
fields.

BUG=582401
==========

to

==========
Initialize UserGestureIndicator for IME commitText.

Autofill code ignores non-user-gesture-based form updates since they are often
greyed-out filler text or suggestions.  KeyDown events as well as
IME setComposition were already correctly registered as user gestures, but
commitText was not.  This could cause autofill to save partially typed form
fields.

BUG=582401

Committed: https://crrev.com/6ad2101efa6830ca3f9d156d662b977707b8ceff
Cr-Commit-Position: refs/heads/master@{#374340}
==========

[commit-bot: I haz the power, 2016-02-09 09:08:54]

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