Call CSD whitelist checking on UI thread and record UMA

Call CSD whitelist checking on UI thread and record UMA

This is the first part of CL to log non-whitelisted password-reuses
to UMA. This CL creates a function on UI thread to trigger the
checking of CSD whitelist (actual checking happens on IO thread),
such that password_reuse_detection_manager can call.

BUG=691103
Review-Url: https://codereview.chromium.org/2720643003
Cr-Commit-Position: refs/heads/master@{#454405}
Committed: https://chromium.googlesource.com/chromium/src/+/8b7d485807e80a6fd022fe96c474ff97fb75fc03

[Jialiu Lin, 2017-02-28 00:14:32]

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

[commit-bot: I haz the power, 2017-02-28 00:15:02]

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-02-28 00:30:39]

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

[commit-bot: I haz the power, 2017-02-28 00:30:40]

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

[Jialiu Lin, 2017-02-28 21:01:03]

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

[commit-bot: I haz the power, 2017-02-28 21:01:32]

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-02-28 21:11:59]

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

[commit-bot: I haz the power, 2017-02-28 21:12:00]

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-02-28 21:40:59]

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

[commit-bot: I haz the power, 2017-02-28 21:41:49]

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-02-28 21:49:19]

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

[commit-bot: I haz the power, 2017-02-28 21:49:20]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  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-02-28 21:55:26]

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

[commit-bot: I haz the power, 2017-02-28 21:56:19]

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-02-28 22:04:19]

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

[commit-bot: I haz the power, 2017-02-28 22:04:20]

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...)

[Jialiu Lin, 2017-02-28 22:25:14]

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

[commit-bot: I haz the power, 2017-02-28 22:26:10]

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-02-28 22:26:48]

Patchset #3 (id:40001) has been deleted

[Jialiu Lin, 2017-02-28 22:27:01]

Patchset #3 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-02-28 22:35:23]

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

[commit-bot: I haz the power, 2017-02-28 22:35:23]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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-02-28 22:42:19]

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

[commit-bot: I haz the power, 2017-02-28 22:43:04]

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-02-28 22:50:05]

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

[commit-bot: I haz the power, 2017-02-28 22:50:05]

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

[Jialiu Lin, 2017-02-28 22:53:33]

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

[commit-bot: I haz the power, 2017-02-28 22:54:07]

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-02-28 23:00:46]

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

[commit-bot: I haz the power, 2017-02-28 23:00:47]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Jialiu Lin, 2017-02-28 23:06:08]

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

[commit-bot: I haz the power, 2017-02-28 23:06:53]

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-02-28 23:16:08]

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

[commit-bot: I haz the power, 2017-02-28 23:16:09]

Dry run: Try jobs failed on following builders:
  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-03-01 00:22:07]

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

[commit-bot: I haz the power, 2017-03-01 00:22:47]

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-03-01 00:32:19]

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

[commit-bot: I haz the power, 2017-03-01 00:32:20]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  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-03-01 01:05:48]

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

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

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-03-01 01:13:29]

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

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

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

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

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

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

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

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

Dry run: Try jobs failed on following builders:
  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-03-01 02:34:11]

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

[commit-bot: I haz the power, 2017-03-01 02:34:57]

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

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

[commit-bot: I haz the power, 2017-03-01 02:44:04]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Jialiu Lin, 2017-03-01 02:51:04]

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

[commit-bot: I haz the power, 2017-03-01 02:52:04]

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-03-01 03:53:58]

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

[commit-bot: I haz the power, 2017-03-01 03:53:59]

Dry run: This issue passed the CQ dry run.

[Jialiu Lin, 2017-03-01 19:00:04]

Description was changed from

==========
Log non-whitelisted password-reuses to UMA

BUG=691103
==========

to

==========
Call CSD whitelist checking on UI thread and record UMA

This is the first part of CL to log non-whitelisted password-reuses
to UMA. This CL creates a function on UI thread to trigger the
checking of CSD whitelist (actual checking happens on IO thread),
such that password_reuse_detection_manager can call.

BUG=691103
==========

[Jialiu Lin, 2017-03-01 19:00:16]

Patchset #2 (id:20001) has been deleted

[Jialiu Lin, 2017-03-01 19:00:26]

Patchset #2 (id:80001) has been deleted

[Jialiu Lin, 2017-03-01 19:00:34]

Patchset #2 (id:100001) has been deleted

[Jialiu Lin, 2017-03-01 19:00:46]

Patchset #8 (id:230001) has been deleted

[Jialiu Lin, 2017-03-01 19:00:51]

Patchset #7 (id:210001) has been deleted

[Jialiu Lin, 2017-03-01 19:00:56]

Patchset #6 (id:190001) has been deleted

[Jialiu Lin, 2017-03-01 19:01:04]

Patchset #5 (id:100002) has been deleted

[Jialiu Lin, 2017-03-01 19:01:12]

Patchset #4 (id:160001) has been deleted

[Jialiu Lin, 2017-03-01 19:01:18]

Patchset #3 (id:140001) has been deleted

[Jialiu Lin, 2017-03-01 19:01:27]

Patchset #2 (id:120001) has been deleted

[Jialiu Lin, 2017-03-01 19:48:32]

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

[Jialiu Lin, 2017-03-01 19:48:33]

nparker@ and vakh@,
Please take a look. Thanks!

[Nathan Parker, 2017-03-01 22:59:22]

lgtm

https://codereview.chromium.org/2720643003/diff/270001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/2720643003/diff/270001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service.cc:335: new
PasswordProtectionService(database_manager());
nit: I think these and others around here should be std::make_unique<..>(..).
'tis the new style, so we dont' get used to using "new"

https://codereview.chromium.org/2720643003/diff/270001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/2720643003/diff/270001/components/BUILD.gn#ne...
components/BUILD.gn:208:
#"//components/safe_browsing/password_protection:password_protection_service_unittest",
Is this going to be included?

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
File components/safe_browsing/password_protection/password_protection_service.h
(right):

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
components/safe_browsing/password_protection/password_protection_service.h:26:
void RecordPasswordReuse(const GURL& main_frame_url);
Other args we may want, eventually:
* origins of the reused saved-password

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
File
components/safe_browsing/password_protection/password_protection_service_unittest.cc
(right):

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
components/safe_browsing/password_protection/password_protection_service_unittest.cc:54:
database_manager_ = nullptr;
Do we need to null these, or can we rely on the destructor de-ref'ing them?

[vakh (use Gerrit instead), 2017-03-01 23:25:20]

Nice work and I really liked that you added the tests for histograms!

https://codereview.chromium.org/2720643003/diff/270001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/2720643003/diff/270001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service.cc:335: new
PasswordProtectionService(database_manager());
On 2017/03/01 22:59:22, Nathan Parker wrote:
> nit: I think these and others around here should be std::make_unique<..>(..).

+1

> 'tis the new style, so we dont' get used to using "new"

I see what you did there.

https://codereview.chromium.org/2720643003/diff/270001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service.cc:458: return
password_protection_service_;
This can go in the header file.
https://google.github.io/styleguide/cppguide.html#Inline_Functions

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
File components/safe_browsing/password_protection/BUILD.gn (right):

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
components/safe_browsing/password_protection/BUILD.gn:5:
source_set("password_protection_service") {
This .cc/.h file combination is already under password_protection  directory so
the name could be shortened to, for instance, pp_service.*

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
File components/safe_browsing/password_protection/DEPS (right):

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
components/safe_browsing/password_protection/DEPS:2: "-content",
What does '-' signify?

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
File components/safe_browsing/password_protection/password_protection_service.cc
(right):

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
components/safe_browsing/password_protection/password_protection_service.cc:19:
: database_manager_(database_manager) {}
Can the constructor be called on any thread?

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
File components/safe_browsing/password_protection/password_protection_service.h
(right):

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
components/safe_browsing/password_protection/password_protection_service.h:26:
void RecordPasswordReuse(const GURL& main_frame_url);
Same for this method... please add a comment explaining what it does, when to
call it, etc.

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
components/safe_browsing/password_protection/password_protection_service.h:30:
// Called on UI thread.
This is a good place to describe when this function is called and what it is
expected to do.

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
File
components/safe_browsing/password_protection/password_protection_service_unittest.cc
(right):

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
components/safe_browsing/password_protection/password_protection_service_unittest.cc:59:
base::MessageLoopForUI message_loop_;
Not sure how message_loop_, ui_thread_, and io_thread_ are being used.

Are they?
If yes, could you please document how?

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
components/safe_browsing/password_protection/password_protection_service_unittest.cc:66:
TEST_F(PasswordProtectionServiceTest, TestPasswordReuseMatchWhitlistHistogram) {
Typo: whitelist

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
components/safe_browsing/password_protection/password_protection_service_unittest.cc:83:
password_protection_service_->RecordPasswordReuse(GURL(kWhitelistedUrl));
Use whitelisted_url instead

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
components/safe_browsing/password_protection/password_protection_service_unittest.cc:90:
password_protection_service_->RecordPasswordReuse(GURL(kNoneWhitelistedUrl));
Use not_whitelisted_url instead

[Jialiu Lin, 2017-03-02 00:44:15]

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

[commit-bot: I haz the power, 2017-03-02 00:45:03]

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-03-02 00:53:07]

Thanks!

https://codereview.chromium.org/2720643003/diff/270001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/2720643003/diff/270001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service.cc:335: new
PasswordProtectionService(database_manager());
On 2017/03/01 22:59:22, Nathan Parker wrote:
> nit: I think these and others around here should be std::make_unique<..>(..).
> 'tis the new style, so we dont' get used to using "new"

Done. Change it from scoped_refptr to unique_ptr instead.

https://codereview.chromium.org/2720643003/diff/270001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service.cc:335: new
PasswordProtectionService(database_manager());
On 2017/03/01 23:25:19, vakh (Varun Khaneja) wrote:
> On 2017/03/01 22:59:22, Nathan Parker wrote:
> > nit: I think these and others around here should be
std::make_unique<..>(..).
> 
> +1
> 
> > 'tis the new style, so we dont' get used to using "new"
> 
> I see what you did there.

Done.

https://codereview.chromium.org/2720643003/diff/270001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service.cc:458: return
password_protection_service_;
On 2017/03/01 23:25:19, vakh (Varun Khaneja) wrote:
> This can go in the header file.
> https://google.github.io/styleguide/cppguide.html#Inline_Functions

Acknowledged. Since all the other getters are defined in .cc file. It looks more
consistent to keep it here.

https://codereview.chromium.org/2720643003/diff/270001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/2720643003/diff/270001/components/BUILD.gn#ne...
components/BUILD.gn:208:
#"//components/safe_browsing/password_protection:password_protection_service_unittest",
On 2017/03/01 22:59:22, Nathan Parker wrote:
> Is this going to be included?
Oops, this should be included.

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
File components/safe_browsing/password_protection/BUILD.gn (right):

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
components/safe_browsing/password_protection/BUILD.gn:5:
source_set("password_protection_service") {
On 2017/03/01 23:25:19, vakh (Varun Khaneja) wrote:
> This .cc/.h file combination is already under password_protection  directory
so
> the name could be shortened to, for instance, pp_service.*

Done.

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
File components/safe_browsing/password_protection/password_protection_service.cc
(right):

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
components/safe_browsing/password_protection/password_protection_service.cc:19:
: database_manager_(database_manager) {}
On 2017/03/01 23:25:19, vakh (Varun Khaneja) wrote:
> Can the constructor be called on any thread?

In theory, it will only be created on UI thread during the initialization of
SafeBrowsingService. Add a DCHECK here.

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
File components/safe_browsing/password_protection/password_protection_service.h
(right):

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
components/safe_browsing/password_protection/password_protection_service.h:26:
void RecordPasswordReuse(const GURL& main_frame_url);
On 2017/03/01 22:59:22, Nathan Parker wrote:
> Other args we may want, eventually:
> * origins of the reused saved-password

ack. It seems password_reuse_detection_manager itself already logged some UMA
for saved_passwords.
Do you have any metrics in mind? It can be easily added in the future anyway.

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
components/safe_browsing/password_protection/password_protection_service.h:26:
void RecordPasswordReuse(const GURL& main_frame_url);
On 2017/03/01 23:25:19, vakh (Varun Khaneja) wrote:
> Same for this method... please add a comment explaining what it does, when to
> call it, etc.

Done.

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
components/safe_browsing/password_protection/password_protection_service.h:30:
// Called on UI thread.
On 2017/03/01 23:25:20, vakh (Varun Khaneja) wrote:
> This is a good place to describe when this function is called and what it is
> expected to do.

Done.

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
File
components/safe_browsing/password_protection/password_protection_service_unittest.cc
(right):

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
components/safe_browsing/password_protection/password_protection_service_unittest.cc:54:
database_manager_ = nullptr;
On 2017/03/01 22:59:22, Nathan Parker wrote:
> Do we need to null these, or can we rely on the destructor de-ref'ing them?

Right. it is not necessary.

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
components/safe_browsing/password_protection/password_protection_service_unittest.cc:59:
base::MessageLoopForUI message_loop_;
On 2017/03/01 23:25:20, vakh (Varun Khaneja) wrote:
> Not sure how message_loop_, ui_thread_, and io_thread_ are being used.
> 
> Are they?
> If yes, could you please document how?

Just realize ThreatBrowserThread is deprecated. Changed into
TestBrowserThreadBundle instead.

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
components/safe_browsing/password_protection/password_protection_service_unittest.cc:66:
TEST_F(PasswordProtectionServiceTest, TestPasswordReuseMatchWhitlistHistogram) {
On 2017/03/01 23:25:20, vakh (Varun Khaneja) wrote:
> Typo: whitelist

Good eye!

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
components/safe_browsing/password_protection/password_protection_service_unittest.cc:83:
password_protection_service_->RecordPasswordReuse(GURL(kWhitelistedUrl));
On 2017/03/01 23:25:20, vakh (Varun Khaneja) wrote:
> Use whitelisted_url instead

Done.

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
components/safe_browsing/password_protection/password_protection_service_unittest.cc:90:
password_protection_service_->RecordPasswordReuse(GURL(kNoneWhitelistedUrl));
On 2017/03/01 23:25:20, vakh (Varun Khaneja) wrote:
> Use not_whitelisted_url instead

Done.

[vakh (use Gerrit instead), 2017-03-02 01:01:01]

lgtm

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
File components/safe_browsing/password_protection/BUILD.gn (right):

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
components/safe_browsing/password_protection/BUILD.gn:5:
source_set("password_protection_service") {
On 2017/03/02 00:53:06, Jialiu Lin wrote:
> On 2017/03/01 23:25:19, vakh (Varun Khaneja) wrote:
> > This .cc/.h file combination is already under password_protection  directory
> so
> > the name could be shortened to, for instance, pp_service.*
> 
> Done.

I don't think this change got uploaded. Can you confirm?

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
File components/safe_browsing/password_protection/DEPS (right):

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
components/safe_browsing/password_protection/DEPS:2: "-content",
On 2017/03/01 23:25:19, vakh (Varun Khaneja) wrote:
> What does '-' signify?

^

https://codereview.chromium.org/2720643003/diff/310001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2720643003/diff/310001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:45988: +<histogram
name="PasswordManager.PasswordReuse.MainFrameMatchCsdWhitelist"
Nit: You should consider adding PasswordManager.PasswordReuse as a prefix, if
that's possible.

[Nathan Parker, 2017-03-02 01:02:42]

lgtm

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
File components/safe_browsing/password_protection/password_protection_service.h
(right):

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
components/safe_browsing/password_protection/password_protection_service.h:26:
void RecordPasswordReuse(const GURL& main_frame_url);
On 2017/03/02 00:53:06, Jialiu Lin wrote:
> On 2017/03/01 22:59:22, Nathan Parker wrote:
> > Other args we may want, eventually:
> > * origins of the reused saved-password
> 
> ack. It seems password_reuse_detection_manager itself already logged some UMA
> for saved_passwords.
> Do you have any metrics in mind? It can be easily added in the future anyway. 

nah, never mind -- we can add it later when we want this to trigger a ping
(since that value will go in the ping).

[Jialiu Lin, 2017-03-02 01:33:15]

Some remaining comments. Thanks!

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
File components/safe_browsing/password_protection/BUILD.gn (right):

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
components/safe_browsing/password_protection/BUILD.gn:5:
source_set("password_protection_service") {
On 2017/03/02 01:01:00, vakh (Varun Khaneja) wrote:
> On 2017/03/02 00:53:06, Jialiu Lin wrote:
> > On 2017/03/01 23:25:19, vakh (Varun Khaneja) wrote:
> > > This .cc/.h file combination is already under password_protection 
directory
> > so
> > > the name could be shortened to, for instance, pp_service.*
> > 
> > Done.
> 
> I don't think this change got uploaded. Can you confirm?

Oh, sorry, my bad. I thought you meant changing source_set name to pp_service,
which I did in patch #5.
I feel more comfortable spelling out password_protection instead of pp in file
name though, since it is not a common abbreviation like db for database. WDYT?

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
File components/safe_browsing/password_protection/DEPS (right):

https://codereview.chromium.org/2720643003/diff/270001/components/safe_browsi...
components/safe_browsing/password_protection/DEPS:2: "-content",
On 2017/03/02 01:01:01, vakh (Varun Khaneja) wrote:
> On 2017/03/01 23:25:19, vakh (Varun Khaneja) wrote:
> > What does '-' signify?
> 
> ^

Ah, sorry. Missed this one. This is to tighten the DEPS introduced by parent
folder. 
Basically it means removing deps to all content/, and adding only
content/public/browser and content/public/test.

https://codereview.chromium.org/2720643003/diff/310001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2720643003/diff/310001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:45988: +<histogram
name="PasswordManager.PasswordReuse.MainFrameMatchCsdWhitelist"
On 2017/03/02 01:01:01, vakh (Varun Khaneja) wrote:
> Nit: You should consider adding PasswordManager.PasswordReuse as a prefix, if
> that's possible.

Agree. I thought of that. I guess I'll defer this to dvadym@, since he is the
owner of all the other PasswordManager.PasswordReuse metrics.

[Jialiu Lin, 2017-03-02 03:38:40]

jialiul@chromium.org changed reviewers:
+ alexmos@chromium.org, holte@chromium.org

[Jialiu Lin, 2017-03-02 03:38:41]

+holte@ for histograms.xml
+alexmos@ because content/public/browser and content/public/test are added into
components/safe_browsing/password_protection/DEPS

Thanks!

[Jialiu Lin, 2017-03-02 03:40:24]

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

[Jialiu Lin, 2017-03-02 03:40:25]

+blundell@ for components/BUILD.gn

[blundell, 2017-03-02 15:54:02]

lgtm

https://codereview.chromium.org/2720643003/diff/310001/components/safe_browsi...
File components/safe_browsing/password_protection/BUILD.gn (right):

https://codereview.chromium.org/2720643003/diff/310001/components/safe_browsi...
components/safe_browsing/password_protection/BUILD.gn:5:
source_set("pp_service") {
nit: you should just call these targets password_protection and
password_protection_unittests respectively. You'll then be able to reference the
password_protection target in GN just as

//components/safe_browsing/password_protection

[blundell, 2017-03-02 15:54:03]

lgtm

[alexmos, 2017-03-02 17:40:46]

LGTM

[Jialiu Lin, 2017-03-02 18:58:23]

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

[commit-bot: I haz the power, 2017-03-02 18:58:54]

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

Patchset #6 (id:330001) has been deleted

[Jialiu Lin, 2017-03-02 19:01:45]

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

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

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-03-02 19:02:50]

Thanks!

https://codereview.chromium.org/2720643003/diff/310001/components/safe_browsi...
File components/safe_browsing/password_protection/BUILD.gn (right):

https://codereview.chromium.org/2720643003/diff/310001/components/safe_browsi...
components/safe_browsing/password_protection/BUILD.gn:5:
source_set("pp_service") {
On 2017/03/02 15:54:02, blundell wrote:
> nit: you should just call these targets password_protection and
> password_protection_unittests respectively. You'll then be able to reference
the
> password_protection target in GN just as
> 
> //components/safe_browsing/password_protection

Done.

[Steven Holte, 2017-03-02 21:00:23]

histograms lgtm

[commit-bot: I haz the power, 2017-03-02 21:04:52]

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

[commit-bot: I haz the power, 2017-03-02 21:04:53]

Dry run: This issue passed the CQ dry run.

[Jialiu Lin, 2017-03-02 22:09:03]

The CQ bit was checked by jialiul@chromium.org

[Jialiu Lin, 2017-03-02 22:09:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
nparker@chromium.org, vakh@chromium.org, alexmos@chromium.org,
blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/2720643003/#ps350001
(title: "change source_set name")

[commit-bot: I haz the power, 2017-03-02 22:09:41]

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-03-02 22:22:10]

CQ is committing da patch.

Bot data: {"patchset_id": 350001, "attempt_start_ts": 1488492543354290,
"parent_rev": "6621c36e3d747d014645903ebb93ff50b4975cbd", "commit_rev":
"8b7d485807e80a6fd022fe96c474ff97fb75fc03"}

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

Description was changed from

==========
Call CSD whitelist checking on UI thread and record UMA

This is the first part of CL to log non-whitelisted password-reuses
to UMA. This CL creates a function on UI thread to trigger the
checking of CSD whitelist (actual checking happens on IO thread),
such that password_reuse_detection_manager can call.

BUG=691103
==========

to

==========
Call CSD whitelist checking on UI thread and record UMA

This is the first part of CL to log non-whitelisted password-reuses
to UMA. This CL creates a function on UI thread to trigger the
checking of CSD whitelist (actual checking happens on IO thread),
such that password_reuse_detection_manager can call.

BUG=691103

Review-Url: https://codereview.chromium.org/2720643003
Cr-Commit-Position: refs/heads/master@{#454405}
Committed:
https://chromium.googlesource.com/chromium/src/+/8b7d485807e80a6fd022fe96c474...
==========

[commit-bot: I haz the power, 2017-03-02 22:23:04]

Committed patchset #6 (id:350001) as
https://chromium.googlesource.com/chromium/src/+/8b7d485807e80a6fd022fe96c474...

[Jialiu Lin, 2017-03-06 21:22:09]

Patchset #7 (id:370001) has been deleted