Trigger Password Protection ping on username/password field on focus

Trigger Password Protection ping on username/password field on focus.

Add CheckSafeBrowsingReputation() function to autofill_driver.mojom.
ContentPasswordDriver overrides this function to kick in
PasswordProtectionService.


BUG=698899
Review-Url: https://codereview.chromium.org/2833193002
Cr-Commit-Position: refs/heads/master@{#468716}
Committed: https://chromium.googlesource.com/chromium/src/+/1a0fcb3039c48ad810531e785637cbb9fe59d6c7

[Jialiu Lin, 2017-04-21 23:00:42]

The CQ bit was checked by jialiul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 23:01:23]

Dry run: 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, 2017-04-21 23:16:23]

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

[commit-bot: I haz the power, 2017-04-21 23:16:23]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Jialiu Lin, 2017-04-22 00:04:55]

The CQ bit was checked by jialiul@chromium.org to run a CQ dry run

[Jialiu Lin, 2017-04-22 00:04:56]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-04-22 00:05:37]

Dry run: 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, 2017-04-22 00:30:50]

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

[commit-bot: I haz the power, 2017-04-22 00:30:51]

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

[Jialiu Lin, 2017-04-22 00:41:05]

The CQ bit was checked by jialiul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-22 00:41:36]

Dry run: 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, 2017-04-22 01:27:42]

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

[commit-bot: I haz the power, 2017-04-22 01:27:44]

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

[Jialiu Lin, 2017-04-26 21:35:30]

The CQ bit was checked by jialiul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 21:36:10]

Dry run: 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, 2017-04-26 21:45:34]

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

[commit-bot: I haz the power, 2017-04-26 21:45:36]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Jialiu Lin, 2017-04-26 22:25:06]

The CQ bit was checked by jialiul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 22:27:14]

Dry run: 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, 2017-04-26 22:37:00]

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

[commit-bot: I haz the power, 2017-04-26 22:37:02]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Jialiu Lin, 2017-04-26 23:10:30]

The CQ bit was checked by jialiul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 23:11:15]

Dry run: 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, 2017-04-26 23:21:50]

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

[commit-bot: I haz the power, 2017-04-26 23:21:51]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Jialiu Lin, 2017-04-26 23:33:00]

The CQ bit was checked by jialiul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 23:34:27]

Dry run: CQ is trying da patch.

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

[Jialiu Lin, 2017-04-26 23:45:54]

Patchset #7 (id:140001) has been deleted

[Jialiu Lin, 2017-04-26 23:46:02]

Patchset #6 (id:120001) has been deleted

[Jialiu Lin, 2017-04-26 23:46:12]

Patchset #5 (id:100001) has been deleted

[Jialiu Lin, 2017-04-26 23:52:13]

Patchset #2 (id:40001) has been deleted

[Jialiu Lin, 2017-04-26 23:52:22]

Patchset #2 (id:60001) has been deleted

[Jialiu Lin, 2017-04-27 00:00:47]

jialiul@chromium.org changed reviewers:
+ dvadym@chromium.org

[Jialiu Lin, 2017-04-27 00:00:48]

Hi dvadym@, 
This is the CL to trigger Password Protection Ping when username/password field
is on focus based on our discussion in the doc. Let me know if this change makes
sense to you. 

And any suggestion on how to add some unit tests and browser tests for this
change? 

Thanks!

[Jialiu Lin, 2017-04-27 00:03:28]

Description was changed from

==========
Trigger Password Protection ping on username/password field on focus

BUG=698899
==========

to

==========
Trigger Password Protection ping on username/password field on focus.

Add CheckSafeBrowsingReputation() function to autofill_driver.mojom.
ContentPasswordDriver overrides this function to kick in
PasswordProtectionService.


BUG=698899
==========

[dvadym, 2017-04-27 10:46:30]

In general it looks good, I left some comments.

Tests:
1.password_autofill_agent_browsertest.cc here you can checked that driver is
called when a field is in focus (see  SetFocused() method in this test file).

2.content_password_manager_driver_unittest.cc you can check that driver calls a
client.

3.chrome_password_manager_client_unittest.cc here you can check that
SafeBrowsing is called (it would require mocking of PasswordProtectionService,
if it's too hard, I'm ok with skipping it, since a code in client is kind of
trivial).

https://codereview.chromium.org/2833193002/diff/180001/components/autofill/co...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/2833193002/diff/180001/components/autofill/co...
components/autofill/content/renderer/password_autofill_agent.cc:939: return
(password_form->username_element == element.NameForAutofill().Utf16());
Just keep in mind, that heuristics for detecting username element are not
reliable. But since true is returned when |element| is password, it covers all
passwords fields, so it looks ok.

https://codereview.chromium.org/2833193002/diff/180001/components/autofill/co...
components/autofill/content/renderer/password_autofill_agent.cc:957:
element.Form().IsNull()
Are you sure that you need form_action not frame url? Nowadays submissions are
often done with JavaScript and action means nothing. Or you need both.

https://codereview.chromium.org/2833193002/diff/180001/components/autofill/co...
components/autofill/content/renderer/password_autofill_agent.cc:958: ? GURL()
Probably in case when a form is absent, current url is better option. It's
completely valid when there is no <form> tag

[Jialiu Lin, 2017-04-28 00:09:09]

Thanks dvadym@!

I have added three tests based on your suggestion.

https://codereview.chromium.org/2833193002/diff/180001/components/autofill/co...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/2833193002/diff/180001/components/autofill/co...
components/autofill/content/renderer/password_autofill_agent.cc:939: return
(password_form->username_element == element.NameForAutofill().Utf16());
On 2017/04/27 10:46:30, dvadym wrote:
> Just keep in mind, that heuristics for detecting username element are not
> reliable. But since true is returned when |element| is password, it covers all
> passwords fields, so it looks ok.

Yes, I'm aware of that.

https://codereview.chromium.org/2833193002/diff/180001/components/autofill/co...
components/autofill/content/renderer/password_autofill_agent.cc:957:
element.Form().IsNull()
On 2017/04/27 10:46:30, dvadym wrote:
> Are you sure that you need form_action not frame url? Nowadays submissions are
> often done with JavaScript and action means nothing. Or you need both.

Good to know.  I'll add more fields to the CheckSafeBrowsingReputation call.

https://codereview.chromium.org/2833193002/diff/180001/components/autofill/co...
components/autofill/content/renderer/password_autofill_agent.cc:958: ? GURL()
On 2017/04/27 10:46:30, dvadym wrote:
> Probably in case when a form is absent, current url is better option. It's
> completely valid when there is no <form> tag
Changed to pass both the action url and current frame url to the
CheckSafeBrowsingReputation call.

[dvadym, 2017-04-28 12:14:40]

Password Manager LGTM % nit

https://codereview.chromium.org/2833193002/diff/200001/chrome/browser/passwor...
File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
(right):

https://codereview.chromium.org/2833193002/diff/200001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:87:
void MaybeStartLowReputationRequest(
Nit: Would it be possible to use gmock for mocking this method and using
EXPECT_CALL on it? We are trying to use gmock as much as possible.

[Jialiu Lin, 2017-04-28 18:38:46]

Thanks dvadym! I have addressed your comment.

https://codereview.chromium.org/2833193002/diff/200001/chrome/browser/passwor...
File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
(right):

https://codereview.chromium.org/2833193002/diff/200001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:87:
void MaybeStartLowReputationRequest(
On 2017/04/28 at 12:14:40, dvadym wrote:
> Nit: Would it be possible to use gmock for mocking this method and using
EXPECT_CALL on it? We are trying to use gmock as much as possible.

Changed to MOCK methods instead.

[Jialiu Lin, 2017-04-28 18:41:13]

The CQ bit was checked by jialiul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 18:42:27]

Dry run: CQ is trying da patch.

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

[Jialiu Lin, 2017-04-28 18:44:34]

jialiul@chromium.org changed reviewers:
+ mathp@chromium.org

[Jialiu Lin, 2017-04-28 18:44:35]

+mathp@chromium.org for owner review of changes in chrome/renderer/autofill/*
and components/autofill/*

Thanks!

[commit-bot: I haz the power, 2017-04-28 19:04:46]

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

[commit-bot: I haz the power, 2017-04-28 19:04:47]

Dry run: 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...)

[Jialiu Lin, 2017-04-28 19:26:51]

The CQ bit was checked by jialiul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 19:27:55]

Dry run: 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, 2017-04-28 21:03:23]

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

[commit-bot: I haz the power, 2017-04-28 21:03:24]

Dry run: This issue passed the CQ dry run.

[Mathieu, 2017-05-01 13:40:00]

Just nits, lgtm

https://codereview.chromium.org/2833193002/diff/240001/chrome/renderer/autofi...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/2833193002/diff/240001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:602:
WebInputElement dummy_element_;
Could you comment why this is needed? I don't see it referenced from the tests

https://codereview.chromium.org/2833193002/diff/240001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:2721: // Verify
CheckSafeBrowsingReputation() is called when user start filling
*starts

https://codereview.chromium.org/2833193002/diff/240001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:2722: //
username or password field, and this function is only called once.
*and that this function

https://codereview.chromium.org/2833193002/diff/240001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:2726: //
Simulate a click on password field to set is on focus,
*to set its focus

https://codereview.chromium.org/2833193002/diff/240001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:2747: 
nit: remove extra line

https://codereview.chromium.org/2833193002/diff/240001/components/autofill/co...
File components/autofill/content/renderer/password_autofill_agent.h (right):

https://codereview.chromium.org/2833193002/diff/240001/components/autofill/co...
components/autofill/content/renderer/password_autofill_agent.h:91: // If "Login
not secure" feature is enabled and if the form is non-secure.
// Returns whether a "Login not secure" warning should be shown on the input
field. This is true if the feature is enabled and if the form is non-secure,

https://codereview.chromium.org/2833193002/diff/240001/components/autofill/co...
components/autofill/content/renderer/password_autofill_agent.h:94: // If the
element is a username or password textfield.
*Returns whether the element is...

https://codereview.chromium.org/2833193002/diff/240001/components/password_ma...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/2833193002/diff/240001/components/password_ma...
components/password_manager/core/browser/password_manager_client.h:206: virtual
void CheckSafeBrowsingReputation(const GURL& form_action,
Add a function comment

[Jialiu Lin, 2017-05-01 18:59:10]

Thank you mathp@! All your comments have been addressed.

https://codereview.chromium.org/2833193002/diff/240001/chrome/renderer/autofi...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/2833193002/diff/240001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:602:
WebInputElement dummy_element_;
On 2017/05/01 at 13:40:00, Mathieu wrote:
> Could you comment why this is needed? I don't see it referenced from the tests

Oops, sorry. Forgot to remove this one.

https://codereview.chromium.org/2833193002/diff/240001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:2721: // Verify
CheckSafeBrowsingReputation() is called when user start filling
On 2017/05/01 at 13:40:00, Mathieu wrote:
> *starts

Done

https://codereview.chromium.org/2833193002/diff/240001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:2722: //
username or password field, and this function is only called once.
On 2017/05/01 at 13:40:00, Mathieu wrote:
> *and that this function

Done

https://codereview.chromium.org/2833193002/diff/240001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:2726: //
Simulate a click on password field to set is on focus,
On 2017/05/01 at 13:40:00, Mathieu wrote:
> *to set its focus

Done

https://codereview.chromium.org/2833193002/diff/240001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:2747: 
On 2017/05/01 at 13:40:00, Mathieu wrote:
> nit: remove extra line

Done.

https://codereview.chromium.org/2833193002/diff/240001/components/autofill/co...
File components/autofill/content/renderer/password_autofill_agent.h (right):

https://codereview.chromium.org/2833193002/diff/240001/components/autofill/co...
components/autofill/content/renderer/password_autofill_agent.h:91: // If "Login
not secure" feature is enabled and if the form is non-secure.
On 2017/05/01 at 13:40:00, Mathieu wrote:
> // Returns whether a "Login not secure" warning should be shown on the input
field. This is true if the feature is enabled and if the form is non-secure,

Done.

https://codereview.chromium.org/2833193002/diff/240001/components/autofill/co...
components/autofill/content/renderer/password_autofill_agent.h:94: // If the
element is a username or password textfield.
On 2017/05/01 at 13:40:00, Mathieu wrote:
> *Returns whether the element is...

Done.

https://codereview.chromium.org/2833193002/diff/240001/components/password_ma...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/2833193002/diff/240001/components/password_ma...
components/password_manager/core/browser/password_manager_client.h:206: virtual
void CheckSafeBrowsingReputation(const GURL& form_action,
On 2017/05/01 at 13:40:00, Mathieu wrote:
> Add a function comment

Done.

[Jialiu Lin, 2017-05-01 19:59:39]

The CQ bit was checked by jialiul@chromium.org

[Jialiu Lin, 2017-05-01 19:59:40]

The patchset sent to the CQ was uploaded after l-g-t-m from dvadym@chromium.org,
mathp@chromium.org
Link to the patchset: https://codereview.chromium.org/2833193002/#ps260001
(title: "Address comments from mathp")

[commit-bot: I haz the power, 2017-05-01 20:00:04]

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, 2017-05-01 20:22:04]

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

[commit-bot: I haz the power, 2017-05-01 20:22:06]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Jialiu Lin, 2017-05-01 20:35:10]

jialiul@chromium.org changed reviewers:
+ estark@chromium.org

[Jialiu Lin, 2017-05-01 20:35:12]

+estark@ 
estark@, could you take a look at
components/autofill/content/common/autofill_driver.mojom (because you're in
ipc/SECURITY_OWNERS)?

Thanks!

[estark, 2017-05-02 14:09:38]

mojom lgtm

https://codereview.chromium.org/2833193002/diff/260001/chrome/browser/passwor...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/2833193002/diff/260001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:420:
pps->MaybeStartLowReputationRequest(GetMainFrameURL(), form_action,
optional nit: I usually see curly braces around bodies that span >1 line

[Jialiu Lin, 2017-05-02 17:36:00]

Thanks estark@!

https://codereview.chromium.org/2833193002/diff/260001/chrome/browser/passwor...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/2833193002/diff/260001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:420:
pps->MaybeStartLowReputationRequest(GetMainFrameURL(), form_action,
On 2017/05/02 at 14:09:38, estark wrote:
> optional nit: I usually see curly braces around bodies that span >1 line

You're right. curly braces are needed here. Thanks for catching this.

[Jialiu Lin, 2017-05-02 17:36:10]

The CQ bit was checked by jialiul@chromium.org

[Jialiu Lin, 2017-05-02 17:36:10]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
dvadym@chromium.org, estark@chromium.org
Link to the patchset: https://codereview.chromium.org/2833193002/#ps280001
(title: "nit")

[commit-bot: I haz the power, 2017-05-02 17:36:25]

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, 2017-05-02 18:46:29]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1493746570271660,
"parent_rev": "7d30cf67b1bd1b78cc95ef9ad88f421bc9bc01f4", "commit_rev":
"1a0fcb3039c48ad810531e785637cbb9fe59d6c7"}

[commit-bot: I haz the power, 2017-05-02 18:46:58]

Description was changed from

==========
Trigger Password Protection ping on username/password field on focus.

Add CheckSafeBrowsingReputation() function to autofill_driver.mojom.
ContentPasswordDriver overrides this function to kick in
PasswordProtectionService.


BUG=698899
==========

to

==========
Trigger Password Protection ping on username/password field on focus.

Add CheckSafeBrowsingReputation() function to autofill_driver.mojom.
ContentPasswordDriver overrides this function to kick in
PasswordProtectionService.


BUG=698899

Review-Url: https://codereview.chromium.org/2833193002
Cr-Commit-Position: refs/heads/master@{#468716}
Committed:
https://chromium.googlesource.com/chromium/src/+/1a0fcb3039c48ad810531e785637...
==========

[commit-bot: I haz the power, 2017-05-02 18:47:00]

Committed patchset #9 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/1a0fcb3039c48ad810531e785637...