Show interstitial on a password on focus ping with PHISHING verdict.

Show interstitial on a password on focus ping with PHISHING verdict.

Plus a couple of minor bug fixing:
- Check is user turned on safe browsing before sending pings
- If verdict is already cached, don't attempt to cache again.

BUG=726794
Review-Url: https://codereview.chromium.org/2905343002
Cr-Commit-Position: refs/heads/master@{#476538}
Committed: https://chromium.googlesource.com/chromium/src/+/9e464a8bd5869da658e45ecf5be00305792b2e61

[Jialiu Lin, 2017-05-26 18:50:35]

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

[commit-bot: I haz the power, 2017-05-26 18:51:09]

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-05-26 19:59:27]

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

[commit-bot: I haz the power, 2017-05-26 19:59:28]

Dry run: This issue passed the CQ dry run.

[Jialiu Lin, 2017-05-26 20:02:00]

jialiul@chromium.org changed reviewers:
+ lpz@chromium.org, nparker@chromium.org

[Nathan Parker, 2017-05-30 18:26:58]

lgtm

https://codereview.chromium.org/2905343002/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/chrome_password_protection_service_unittest.cc
(right):

https://codereview.chromium.org/2905343002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_password_protection_service_unittest.cc:36:
// Note: this will delete the client object in the case of the CsdClient
Which is the client object?  The callback?

https://codereview.chromium.org/2905343002/diff/20001/components/safe_browsin...
File components/safe_browsing/base_ping_manager.cc (right):

https://codereview.chromium.org/2905343002/diff/20001/components/safe_browsin...
components/safe_browsing/base_ping_manager.cc:232: threat_list = "phishpphit";
Need to ensure backend knows of this string.

https://codereview.chromium.org/2905343002/diff/20001/components/safe_browsin...
File components/safe_browsing/password_protection/password_protection_request.h
(right):

https://codereview.chromium.org/2905343002/diff/20001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_request.h:70:
LoginReputationClientRequest* request_proto() const {
Could this be const*?  Otherwise the caller could modify it after it's filled
in.

[Jialiu Lin, 2017-05-30 19:12:47]

jialiul@chromium.org changed reviewers:
+ dvadym@chromium.org, estark@chromium.org, mathp@chromium.org

[Jialiu Lin, 2017-05-30 19:12:48]

Thanks nparker@!
+mathp@ for c/b/ui/autofill/* owner review
+dvadym@ for c/b/password_manager/ owner review
+estark@ for c/b/ssl/security_state_tab_helper.cc owner review

https://codereview.chromium.org/2905343002/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/chrome_password_protection_service_unittest.cc
(right):

https://codereview.chromium.org/2905343002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_password_protection_service_unittest.cc:36:
// Note: this will delete the client object in the case of the CsdClient
On 2017/05/30 18:26:57, Nathan Parker wrote:
> Which is the client object?  The callback?

Oops, this comment is not relevant here. Removed.

https://codereview.chromium.org/2905343002/diff/20001/components/safe_browsin...
File components/safe_browsing/base_ping_manager.cc (right):

https://codereview.chromium.org/2905343002/diff/20001/components/safe_browsin...
components/safe_browsing/base_ping_manager.cc:232: threat_list = "phishpphit";
On 2017/05/30 18:26:57, Nathan Parker wrote:
> Need to ensure backend knows of this string.

I've already confirmed with weiningy@, this will not break backend (these
reports will be ignored for now). I'll ask awoz@ to starts processing hit report
for phishguard once he comes back from vacation.

https://codereview.chromium.org/2905343002/diff/20001/components/safe_browsin...
File components/safe_browsing/password_protection/password_protection_request.h
(right):

https://codereview.chromium.org/2905343002/diff/20001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_request.h:70:
LoginReputationClientRequest* request_proto() const {
On 2017/05/30 18:26:57, Nathan Parker wrote:
> Could this be const*?  Otherwise the caller could modify it after it's filled
> in.

Done.

[estark, 2017-05-30 22:53:41]

c/b/ssl lgtm

[dvadym, 2017-05-31 08:45:55]

chrome_password_manager_client_unittest.cc LGTM

[Mathieu, 2017-05-31 17:35:44]

autofill lgtm

[Jialiu Lin, 2017-05-31 18:18:11]

jialiul@chromium.org changed reviewers:
+ vakh@chromium.org
- lpz@chromium.org

[Jialiu Lin, 2017-05-31 18:18:12]

-lpz@ since he is OOO for this week
+vakh@ as the second reviewer for safe_browsing namespace.

[Jialiu Lin, 2017-06-01 18:26:13]

vakh@, have time to take a look?

[vakh (use Gerrit instead), 2017-06-01 18:30:11]

lgtm

https://codereview.chromium.org/2905343002/diff/40001/components/safe_browsin...
File components/safe_browsing/csd.proto (right):

https://codereview.chromium.org/2905343002/diff/40001/components/safe_browsin...
components/safe_browsing/csd.proto:911: PASSWORD_PROTECTION_PHISHING_URL = 12;
URL_PHISHING_PASSWORD_PROTECTION (to keep it consistent with the rest of the
types)?

[vakh (use Gerrit instead), 2017-06-01 18:31:12]

On 2017/06/01 18:26:13, Jialiu Lin wrote:
> vakh@, have time to take a look?

Done.

[Jialiu Lin, 2017-06-01 18:53:33]

Thanks!

https://codereview.chromium.org/2905343002/diff/40001/components/safe_browsin...
File components/safe_browsing/csd.proto (right):

https://codereview.chromium.org/2905343002/diff/40001/components/safe_browsin...
components/safe_browsing/csd.proto:911: PASSWORD_PROTECTION_PHISHING_URL = 12;
On 2017/06/01 18:30:09, vakh (Varun Khaneja) wrote:
> URL_PHISHING_PASSWORD_PROTECTION (to keep it consistent with the rest of the
> types)?

Acknowledged
Good point! It seems we have some inconsistency here even without
PASSWORD_PROTECTION_PHISHING_URL.  Apparently CLIENT_SIDE_PHISHING/MALWARE_URL
put the _URL at the end. Since the server side proto has already been landed
with PHISHING_PASSWORD_PROTECTION_URL, let's keep it this way for now. 

Ideally, we probably should remove _URL suffix for Client side detection and
password protection. I filed crbug.com/728741 for this purpose.

[Jialiu Lin, 2017-06-01 18:54:00]

The CQ bit was checked by jialiul@chromium.org

[Jialiu Lin, 2017-06-01 18:54:00]

The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org
Link to the patchset: https://codereview.chromium.org/2905343002/#ps40001
(title: "address nparker's comments")

[commit-bot: I haz the power, 2017-06-01 18:54:53]

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-06-01 19:00:22]

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

[commit-bot: I haz the power, 2017-06-01 19:00:23]

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...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Jialiu Lin, 2017-06-01 21:09:12]

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

[commit-bot: I haz the power, 2017-06-01 21:10:02]

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-06-01 21:28:58]

The CQ bit was unchecked by jialiul@chromium.org

[Jialiu Lin, 2017-06-01 21:29:07]

The CQ bit was checked by jialiul@chromium.org

[Jialiu Lin, 2017-06-01 21:29:08]

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

[commit-bot: I haz the power, 2017-06-01 21:29:46]

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-06-02 03:01:01]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1496352547845800,
"parent_rev": "b71f6a8eed67fb326ac00d673c810d0a053ad048", "commit_rev":
"a0de4c7ad9c918aa0da7603ae4e2162a8ea73901"}

[commit-bot: I haz the power, 2017-06-02 03:01:13]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1496352547845800,
"parent_rev": "05681d3939d311c090824c901d922f5bdb4df78c", "commit_rev":
"9e464a8bd5869da658e45ecf5be00305792b2e61"}

[commit-bot: I haz the power, 2017-06-02 03:01:22]

Description was changed from

==========
Show interstitial on a password on focus ping with PHISHING verdict.

Plus a couple of minor bug fixing:
- Check is user turned on safe browsing before sending pings
- If verdict is already cached, don't attempt to cache again.

BUG=726794
==========

to

==========
Show interstitial on a password on focus ping with PHISHING verdict.

Plus a couple of minor bug fixing:
- Check is user turned on safe browsing before sending pings
- If verdict is already cached, don't attempt to cache again.

BUG=726794

Review-Url: https://codereview.chromium.org/2905343002
Cr-Commit-Position: refs/heads/master@{#476538}
Committed:
https://chromium.googlesource.com/chromium/src/+/9e464a8bd5869da658e45ecf5be0...
==========

[commit-bot: I haz the power, 2017-06-02 03:01:24]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/9e464a8bd5869da658e45ecf5be0...