Measure how often PSL and same-organization name credentials are suppressed.

Measure how often PSL and same-organization name credentials are suppressed.

To this end, the SuppressedHTTPSFormFetcher is extended into the
SuppressedFormFetcher, which now fetches the two new kinds of suppressed
credentials as well; and makes them available through the FormFetcher interface
to PasswordFormManager, which ultimately records the histograms.

BUG=720599
Review-Url: https://codereview.chromium.org/2912783002
Cr-Commit-Position: refs/heads/master@{#475561}
Committed: https://chromium.googlesource.com/chromium/src/+/e373a531ac62344a5fe43c81e9511bc0df495c3d

[engedy, 2017-05-29 14:00:55]

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

[commit-bot: I haz the power, 2017-05-29 14:01:03]

Dry run: CQ is trying da patch.

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

[engedy, 2017-05-29 14:39:30]

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

[engedy, 2017-05-29 14:39:32]

Description was changed from

==========
Measure how often PSL and same-organization name credentials are suppressed.

BUG=720599
==========

to

==========
Measure how often PSL and same-organization name credentials are suppressed.

BUG=720599
==========

[engedy, 2017-05-29 14:39:33]

engedy@chromium.org changed reviewers:
+ dvadym@chromium.org, kolos@chromium.org

[commit-bot: I haz the power, 2017-05-29 14:39:48]

Dry run: CQ is trying da patch.

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

[engedy, 2017-05-29 14:40:38]

The CQ bit was unchecked by engedy@chromium.org

[engedy, 2017-05-29 14:41:39]

Patchset #3 (id:40001) has been deleted

[engedy, 2017-05-29 14:41:49]

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

[commit-bot: I haz the power, 2017-05-29 14:42:02]

Dry run: CQ is trying da patch.

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

[engedy, 2017-05-29 14:42:06]

Maxim, Vadym, this is the last piece. Please take a look.

[engedy, 2017-05-29 14:45:15]

On 2017/05/29 14:42:06, engedy wrote:
> Maxim, Vadym, this is the last piece. Please take a look.

I created a third category, suppressed PSL matches. -- This might sound a bit
weird at first. The rationale is this: today, unless the user actually selects a
PSL match, we do not know if one was available. It would be good to know how not
indicating the a PSL match can be filled affects sign-in rates. Maybe we need
more indication.

[commit-bot: I haz the power, 2017-05-29 14:51:27]

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

[commit-bot: I haz the power, 2017-05-29 14:51:29]

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

[engedy, 2017-05-29 14:59:50]

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

[commit-bot: I haz the power, 2017-05-29 15:02:10]

Dry run: CQ is trying da patch.

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

[engedy, 2017-05-29 16:04:56]

engedy@chromium.org changed reviewers:
+ rkaplow@chromium.org

[engedy, 2017-05-29 16:05:18]

+Robert: PTAL at histograms.xml.

[commit-bot: I haz the power, 2017-05-29 17:29:39]

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

[commit-bot: I haz the power, 2017-05-29 17:29:40]

Dry run: This issue passed the CQ dry run.

[rkaplow, 2017-05-29 19:19:50]

lgtm

[kolos1, 2017-05-30 07:02:29]

LGTM with comments. Thanks Balazs. 

Please also mention how this CL is aligned with previous CLs (e.g. "it extends
the SuppressedHTTPSFormFetcher...")

https://codereview.chromium.org/2912783002/diff/80001/components/password_man...
File components/password_manager/core/browser/form_fetcher_impl.cc (right):

https://codereview.chromium.org/2912783002/diff/80001/components/password_man...
components/password_manager/core/browser/form_fetcher_impl.cc:52:
std::vector<std::unique_ptr<PasswordForm>> suppressed_forms,
Should we pass |suppressed_forms| by value, by not by point (since we change the
vector)?

https://codereview.chromium.org/2912783002/diff/80001/components/password_man...
components/password_manager/core/browser/form_fetcher_impl.cc:55:
std::vector<std::unique_ptr<PasswordForm>>* same_organization_name_forms) {
Optional: add DCHECKs for vectors here

https://codereview.chromium.org/2912783002/diff/80001/components/password_man...
components/password_manager/core/browser/form_fetcher_impl.cc:185: // If this is
a non-secure Web origin (i.e. HTTP), kick off the discovery of
I believe we should change the comment.

https://codereview.chromium.org/2912783002/diff/80001/components/password_man...
components/password_manager/core/browser/form_fetcher_impl.cc:276: form_digest_,
client_, false, should_query_suppressed_forms_);
Shall we create a copy with SuppressedFormsFetcher? Or should
|should_query_suppressed_forms| always be false?

https://codereview.chromium.org/2912783002/diff/80001/components/password_man...
File components/password_manager/core/browser/form_fetcher_impl.h (right):

https://codereview.chromium.org/2912783002/diff/80001/components/password_man...
components/password_manager/core/browser/form_fetcher_impl.h:130: // Indicates
whether to query suppressed forms on HTTP/HTTPS origins.
Do we need to mention "HTTP/HTTPS origins"? Sounds like all origins, doesn't it?

[engedy, 2017-05-30 08:44:57]

Description was changed from

==========
Measure how often PSL and same-organization name credentials are suppressed.

BUG=720599
==========

to

==========
Measure how often PSL and same-organization name credentials are suppressed.

To this end, the SuppressedHTTPSFormFetcher is extended into the
SuppressedFormFetcher, which now fetches the two new kinds of suppressed
credentials as well.

BUG=720599
==========

[engedy, 2017-05-30 08:46:30]

Description was changed from

==========
Measure how often PSL and same-organization name credentials are suppressed.

To this end, the SuppressedHTTPSFormFetcher is extended into the
SuppressedFormFetcher, which now fetches the two new kinds of suppressed
credentials as well.

BUG=720599
==========

to

==========
Measure how often PSL and same-organization name credentials are suppressed.

To this end, the SuppressedHTTPSFormFetcher is extended into the
SuppressedFormFetcher, which now fetches the two new kinds of suppressed
credentials as well; and makes them available through the FormFetcher interface
to PasswordFormManager, which ultimately records the histograms.

BUG=720599
==========

[engedy, 2017-05-30 08:47:00]

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

[commit-bot: I haz the power, 2017-05-30 08:47:23]

Dry run: CQ is trying da patch.

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

[engedy, 2017-05-30 08:47:44]

All done, updated CL description too.

@Vadym, PTAL.

https://codereview.chromium.org/2912783002/diff/80001/components/password_man...
File components/password_manager/core/browser/form_fetcher_impl.cc (right):

https://codereview.chromium.org/2912783002/diff/80001/components/password_man...
components/password_manager/core/browser/form_fetcher_impl.cc:52:
std::vector<std::unique_ptr<PasswordForm>> suppressed_forms,
On 2017/05/30 07:02:29, kolos1 wrote:
> Should we pass |suppressed_forms| by value, by not by point (since we change
the
> vector)?

We do pass it by value.

https://codereview.chromium.org/2912783002/diff/80001/components/password_man...
components/password_manager/core/browser/form_fetcher_impl.cc:55:
std::vector<std::unique_ptr<PasswordForm>>* same_organization_name_forms) {
On 2017/05/30 07:02:29, kolos1 wrote:
> Optional: add DCHECKs for vectors here 

Done.

https://codereview.chromium.org/2912783002/diff/80001/components/password_man...
components/password_manager/core/browser/form_fetcher_impl.cc:185: // If this is
a non-secure Web origin (i.e. HTTP), kick off the discovery of
On 2017/05/30 07:02:29, kolos1 wrote:
> I believe we should change the comment.

Oh yes. Done.

https://codereview.chromium.org/2912783002/diff/80001/components/password_man...
components/password_manager/core/browser/form_fetcher_impl.cc:276: form_digest_,
client_, false, should_query_suppressed_forms_);
On 2017/05/30 07:02:29, kolos1 wrote:
> Shall we create a copy with SuppressedFormsFetcher? Or should
> |should_query_suppressed_forms| always be false?

The SuppressedFormFetcher has no state, so I guess implementing a Clone()
operation on it would be over-engineering it. :-) It is probably fine to have it
created on the next call to Start() in the clone.

Note that if you clone |this| right after construction, without ever having
called Start(), |suppressed*_forms_| will all be empty, so I'd be uneasy about
setting |should_query_suppressed_forms| to false in the clone.

https://codereview.chromium.org/2912783002/diff/80001/components/password_man...
File components/password_manager/core/browser/form_fetcher_impl.h (right):

https://codereview.chromium.org/2912783002/diff/80001/components/password_man...
components/password_manager/core/browser/form_fetcher_impl.h:130: // Indicates
whether to query suppressed forms on HTTP/HTTPS origins.
On 2017/05/30 07:02:29, kolos1 wrote:
> Do we need to mention "HTTP/HTTPS origins"? Sounds like all origins, doesn't
it?

Right. Done.

[kolos1, 2017-05-30 09:16:36]

https://codereview.chromium.org/2912783002/diff/80001/components/password_man...
File components/password_manager/core/browser/form_fetcher_impl.cc (right):

https://codereview.chromium.org/2912783002/diff/80001/components/password_man...
components/password_manager/core/browser/form_fetcher_impl.cc:52:
std::vector<std::unique_ptr<PasswordForm>> suppressed_forms,
On 2017/05/30 08:47:43, engedy wrote:
> On 2017/05/30 07:02:29, kolos1 wrote:
> > Should we pass |suppressed_forms| by value, by not by point (since we change
> the
> > vector)?
> 
> We do pass it by value.

oops, actually I suggested the opposite. I believe we can pass it by point.

https://codereview.chromium.org/2912783002/diff/80001/components/password_man...
components/password_manager/core/browser/form_fetcher_impl.cc:276: form_digest_,
client_, false, should_query_suppressed_forms_);
On 2017/05/30 08:47:43, engedy wrote:
> On 2017/05/30 07:02:29, kolos1 wrote:
> > Shall we create a copy with SuppressedFormsFetcher? Or should
> > |should_query_suppressed_forms| always be false?
> 
> The SuppressedFormFetcher has no state, so I guess implementing a Clone()
> operation on it would be over-engineering it. :-) It is probably fine to have
it
> created on the next call to Start() in the clone.
> 
> Note that if you clone |this| right after construction, without ever having
> called Start(), |suppressed*_forms_| will all be empty, so I'd be uneasy about
> setting |should_query_suppressed_forms| to false in the clone.

Sorry for confusing comment again :(

HTTP migration is suppressed in the copy of FormFetcher (look at |false| and
comment before). I believe we should do the same for SuppressedFormFetcher.

The comment to FormFetcher::Clone says that it should not cause additional
fetches. The same comment in tests
https://cs.chromium.org/chromium/src/components/password_manager/core/browser...

My suggestion:
* replace |should_query_suppressed_forms_| with |false|
* update FormFetcherImplTest, Clone_* tests

[commit-bot: I haz the power, 2017-05-30 10:15:48]

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

[commit-bot: I haz the power, 2017-05-30 10:15:49]

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

[engedy, 2017-05-30 13:51:26]

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

[commit-bot: I haz the power, 2017-05-30 13:51:34]

Dry run: CQ is trying da patch.

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

[engedy, 2017-05-30 13:51:35]

https://codereview.chromium.org/2912783002/diff/80001/components/password_man...
File components/password_manager/core/browser/form_fetcher_impl.cc (right):

https://codereview.chromium.org/2912783002/diff/80001/components/password_man...
components/password_manager/core/browser/form_fetcher_impl.cc:52:
std::vector<std::unique_ptr<PasswordForm>> suppressed_forms,
On 2017/05/30 09:16:36, kolos1 wrote:
> On 2017/05/30 08:47:43, engedy wrote:
> > On 2017/05/30 07:02:29, kolos1 wrote:
> > > Should we pass |suppressed_forms| by value, by not by point (since we
change
> > the
> > > vector)?
> > 
> > We do pass it by value.
> 
> oops, actually I suggested the opposite. I believe we can pass it by point.

I dunno -- passing it by value allows for std::move at the call site, which
expresses nicely that it is consumed. WDYT?

https://codereview.chromium.org/2912783002/diff/80001/components/password_man...
components/password_manager/core/browser/form_fetcher_impl.cc:276: form_digest_,
client_, false, should_query_suppressed_forms_);
On 2017/05/30 09:16:36, kolos1 wrote:
> On 2017/05/30 08:47:43, engedy wrote:
> > On 2017/05/30 07:02:29, kolos1 wrote:
> > > Shall we create a copy with SuppressedFormsFetcher? Or should
> > > |should_query_suppressed_forms| always be false?
> > 
> > The SuppressedFormFetcher has no state, so I guess implementing a Clone()
> > operation on it would be over-engineering it. :-) It is probably fine to
have
> it
> > created on the next call to Start() in the clone.
> > 
> > Note that if you clone |this| right after construction, without ever having
> > called Start(), |suppressed*_forms_| will all be empty, so I'd be uneasy
about
> > setting |should_query_suppressed_forms| to false in the clone.
> 
> Sorry for confusing comment again :(
> 
> HTTP migration is suppressed in the copy of FormFetcher (look at |false| and
> comment before). I believe we should do the same for SuppressedFormFetcher.
> 
> The comment to FormFetcher::Clone says that it should not cause additional
> fetches. The same comment in tests
>
https://cs.chromium.org/chromium/src/components/password_manager/core/browser...
> 
> My suggestion:
> * replace |should_query_suppressed_forms_| with |false|
> * update FormFetcherImplTest, Clone_* tests

Added a unittest to enforce that no fetch is made directly in response to the
Clone call.

[dvadym, 2017-05-30 14:17:00]

It looks good, I just have a question.

https://codereview.chromium.org/2912783002/diff/120001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/2912783002/diff/120001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:357:
"PasswordManager.SuppressedAccount.Generated.PSLMatching",
What're the other reasons except http vs https when PSL matches are suppressed?

[engedy, 2017-05-30 14:21:25]

https://codereview.chromium.org/2912783002/diff/120001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/2912783002/diff/120001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:357:
"PasswordManager.SuppressedAccount.Generated.PSLMatching",
On 2017/05/30 14:17:00, dvadym wrote:
> What're the other reasons except http vs https when PSL matches are
suppressed?

That's a bit tricky. In these histograms, PSL matches are always considered
`suppressed`, and they have to be the same scheme.

[dvadym, 2017-05-30 14:30:53]

On 2017/05/30 14:21:25, engedy wrote:
>
https://codereview.chromium.org/2912783002/diff/120001/components/password_ma...
> File components/password_manager/core/browser/password_form_manager.cc
(right):
> 
>
https://codereview.chromium.org/2912783002/diff/120001/components/password_ma...
> components/password_manager/core/browser/password_form_manager.cc:357:
> "PasswordManager.SuppressedAccount.Generated.PSLMatching",
> On 2017/05/30 14:17:00, dvadym wrote:
> > What're the other reasons except http vs https when PSL matches are
> suppressed?
> 
> That's a bit tricky. In these histograms, PSL matches are always considered
> `suppressed`, and they have to be the same scheme.

Ok, it makes sense. LGTM

[commit-bot: I haz the power, 2017-05-30 15:15:13]

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

[commit-bot: I haz the power, 2017-05-30 15:15:14]

Dry run: This issue passed the CQ dry run.

[engedy, 2017-05-30 16:43:53]

The CQ bit was checked by engedy@chromium.org

[engedy, 2017-05-30 16:43:54]

The patchset sent to the CQ was uploaded after l-g-t-m from kolos@chromium.org,
rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2912783002/#ps120001
(title: "Addressed comments from kolos@.")

[engedy, 2017-05-30 16:43:58]

The CQ bit was checked by engedy@chromium.org

[commit-bot: I haz the power, 2017-05-30 16:44:26]

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-30 16:49:59]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496162638178550,
"parent_rev": "00797c765a5272808c6cafb3e165f401017f7cc9", "commit_rev":
"e373a531ac62344a5fe43c81e9511bc0df495c3d"}

[commit-bot: I haz the power, 2017-05-30 16:50:07]

Description was changed from

==========
Measure how often PSL and same-organization name credentials are suppressed.

To this end, the SuppressedHTTPSFormFetcher is extended into the
SuppressedFormFetcher, which now fetches the two new kinds of suppressed
credentials as well; and makes them available through the FormFetcher interface
to PasswordFormManager, which ultimately records the histograms.

BUG=720599
==========

to

==========
Measure how often PSL and same-organization name credentials are suppressed.

To this end, the SuppressedHTTPSFormFetcher is extended into the
SuppressedFormFetcher, which now fetches the two new kinds of suppressed
credentials as well; and makes them available through the FormFetcher interface
to PasswordFormManager, which ultimately records the histograms.

BUG=720599

Review-Url: https://codereview.chromium.org/2912783002
Cr-Commit-Position: refs/heads/master@{#475561}
Committed:
https://chromium.googlesource.com/chromium/src/+/e373a531ac62344a5fe43c81e951...
==========

[commit-bot: I haz the power, 2017-05-30 16:50:08]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/e373a531ac62344a5fe43c81e951...