[Password Generation] Send a boolean flag of whether user changed generated password

[Password Generation] Send a boolean flag of whether user changed generated password

The flag helps to find the sites where passwords generated by Chrome doesn't meet site's requirements

BUG=699529
Review-Url: https://codereview.chromium.org/2937033002
Cr-Commit-Position: refs/heads/master@{#481210}
Committed: https://chromium.googlesource.com/chromium/src/+/5257dd237f1d169b88719b16c2e76682828b35c0

[kolos1, 2017-06-14 12:45:27]

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

[kolos1, 2017-06-14 12:45:27]

Hi Vadym,

Please review.

Regards,
Maxim

[kolos1, 2017-06-14 12:45:37]

Patchset #1 (id:1) has been deleted

[kolos1, 2017-06-14 12:46:05]

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

[commit-bot: I haz the power, 2017-06-14 12:46:20]

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-06-14 13:17:03]

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

[commit-bot: I haz the power, 2017-06-14 13:17:03]

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

[dvadym, 2017-06-14 13:38:50]

https://codereview.chromium.org/2937033002/diff/20001/components/password_man...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/2937033002/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager.cc:201:
form_manager->form_saver()->PresaveGeneratedPassword(form);
Why do you need to pass |generated_password_changed_| from renderer to
PasswordFormManager, when this information is anyway available in the browser?
Namely any presaving after the first is user update. PFM has a field
|has_generated_password_| for detecting whether this is the first or not the
first update.

[kolos1, 2017-06-14 15:32:51]

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

[commit-bot: I haz the power, 2017-06-14 15:33:12]

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-06-14 15:44:06]

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

[commit-bot: I haz the power, 2017-06-14 15:44:07]

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

[kolos1, 2017-06-14 16:01:30]

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

[commit-bot: I haz the power, 2017-06-14 16:01:54]

Dry run: CQ is trying da patch.

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

[kolos1, 2017-06-14 16:01:57]

https://codereview.chromium.org/2937033002/diff/20001/components/password_man...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/2937033002/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager.cc:201:
form_manager->form_saver()->PresaveGeneratedPassword(form);
On 2017/06/14 13:38:50, dvadym wrote:
> Why do you need to pass |generated_password_changed_| from renderer to
> PasswordFormManager, when this information is anyway available in the browser?
> Namely any presaving after the first is user update. PFM has a field
> |has_generated_password_| for detecting whether this is the first or not the
> first update.

Chenged design. PTAL.

[commit-bot: I haz the power, 2017-06-14 16:12:12]

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

[commit-bot: I haz the power, 2017-06-14 16:12:13]

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

[dvadym, 2017-06-16 09:15:24]

In general it looks good, some comments inline

https://codereview.chromium.org/2937033002/diff/60001/components/autofill/cor...
File components/autofill/core/browser/proto/server.proto (right):

https://codereview.chromium.org/2937033002/diff/60001/components/autofill/cor...
components/autofill/core/browser/proto/server.proto:103: // field is absent.
user -> the user

https://codereview.chromium.org/2937033002/diff/60001/components/password_man...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/2937033002/diff/60001/components/password_man...
components/password_manager/core/browser/password_manager.cc:204:
UMA_HISTOGRAM_BOOLEAN("PasswordManager.GeneratedFormHasNoFormManager", true);
This histogram is very misleading: true means how many times there are no PFM
when the password is generated, false how many times there are not PFM when
password is removed. I'd propose to move logging with false to line 200, in this
case the result will correspond to the name.

[kolos1, 2017-06-16 11:09:56]

Patchset #4 (id:80001) has been deleted

[kolos1, 2017-06-16 11:10:21]

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

[commit-bot: I haz the power, 2017-06-16 11:10:24]

Dry run: CQ is trying da patch.

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

[kolos1, 2017-06-16 11:10:28]

https://codereview.chromium.org/2937033002/diff/60001/components/autofill/cor...
File components/autofill/core/browser/proto/server.proto (right):

https://codereview.chromium.org/2937033002/diff/60001/components/autofill/cor...
components/autofill/core/browser/proto/server.proto:103: // field is absent.
On 2017/06/16 09:15:24, dvadym wrote:
> user -> the user

Done.

https://codereview.chromium.org/2937033002/diff/60001/components/password_man...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/2937033002/diff/60001/components/password_man...
components/password_manager/core/browser/password_manager.cc:204:
UMA_HISTOGRAM_BOOLEAN("PasswordManager.GeneratedFormHasNoFormManager", true);
On 2017/06/16 09:15:24, dvadym wrote:
> This histogram is very misleading: true means how many times there are no PFM
> when the password is generated, false how many times there are not PFM when
> password is removed. I'd propose to move logging with false to line 200, in
this
> case the result will correspond to the name.

Done.

[dvadym, 2017-06-16 11:53:31]

LGTM

[kolos1, 2017-06-16 12:05:48]

kolos@chromium.org changed reviewers:
+ vabr@chromium.org

[kolos1, 2017-06-16 12:05:48]

vabr@chromium.org: Please review changes in 
components/autofill/core/browser/*
ios/chrome/browser/passwords/password_generation_agent.mm

[vabr (Chromium), 2017-06-16 12:32:29]

Hi Maxim,

I encourage more tests and splitting independent parts off (see below). Other
than that, the changes in components/autofill/core/browser/* and
ios/chrome/browser/passwords/password_generation_agent.mm LGTM.

Cheers,
Vaclav

https://codereview.chromium.org/2937033002/diff/100001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.h
(left):

https://codereview.chromium.org/2937033002/diff/100001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.h:130:
password_manager::PasswordManager* password_manager_;
While I applaud to removing dead code, perhaps this could have been a separate
CL, because it is not really related to the main goal of the CL. If you could
extract this ("git cl split" might help), feel free to just TBR to me and land
the change in c/b/ui/autofill.

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

https://codereview.chromium.org/2937033002/diff/100001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:568: void
PasswordFormManager::PresaveGeneratedPassword(
Could we test the two newly added methods in the unittest?

https://codereview.chromium.org/2937033002/diff/100001/components/password_ma...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/2937033002/diff/100001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:201:
UMA_HISTOGRAM_BOOLEAN("PasswordManager.GeneratedFormHasNoFormManager",
Could you test setting the histograms in the unittest? (Here and line 206
below.)

https://codereview.chromium.org/2937033002/diff/100001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:214:
form_manager->PasswordNoLongerGenerated();
Is there a way to test that this forwarding to PasswordFormManager, and the one
on line 200, works?

[commit-bot: I haz the power, 2017-06-16 13:16:29]

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

[commit-bot: I haz the power, 2017-06-16 13:16:31]

Dry run: This issue passed the CQ dry run.

[kolos1, 2017-06-21 13:29:31]

Patchset #5 (id:120001) has been deleted

[kolos1, 2017-06-21 13:43:17]

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

[commit-bot: I haz the power, 2017-06-21 13:43:24]

Dry run: CQ is trying da patch.

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

[kolos1, 2017-06-21 13:46:12]

Thanks for comments Vaclav. PTAL

Regards,
Maxim

https://codereview.chromium.org/2937033002/diff/100001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.h
(left):

https://codereview.chromium.org/2937033002/diff/100001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.h:130:
password_manager::PasswordManager* password_manager_;
On 2017/06/16 12:32:29, vabr (Chromium) wrote:
> While I applaud to removing dead code, perhaps this could have been a separate
> CL, because it is not really related to the main goal of the CL. If you could
> extract this ("git cl split" might help), feel free to just TBR to me and land
> the change in c/b/ui/autofill.

This code became dead because of removing the call of SetHasGeneratedPassword in
password_generation_popup_controller_impl.cc. Removing of SetHas... is
nondetachable part of this CL.

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

https://codereview.chromium.org/2937033002/diff/100001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:568: void
PasswordFormManager::PresaveGeneratedPassword(
On 2017/06/16 12:32:29, vabr (Chromium) wrote:
> Could we test the two newly added methods in the unittest?

Done.

https://codereview.chromium.org/2937033002/diff/100001/components/password_ma...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/2937033002/diff/100001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:201:
UMA_HISTOGRAM_BOOLEAN("PasswordManager.GeneratedFormHasNoFormManager",
On 2017/06/16 12:32:29, vabr (Chromium) wrote:
> Could you test setting the histograms in the unittest? (Here and line 206
> below.)

Let me do it in a separate CL. I believe it is not good place for this metric
anymore.

https://codereview.chromium.org/2937033002/diff/100001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:214:
form_manager->PasswordNoLongerGenerated();
On 2017/06/16 12:32:29, vabr (Chromium) wrote:
> Is there a way to test that this forwarding to PasswordFormManager, and the
one
> on line 200, works?

I believe this forwarding is already tested indirectionally. Generation stuff
wouldn't work w/o that.

[kolos1, 2017-06-21 13:48:13]

https://codereview.chromium.org/2937033002/diff/100001/components/password_ma...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/2937033002/diff/100001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:201:
UMA_HISTOGRAM_BOOLEAN("PasswordManager.GeneratedFormHasNoFormManager",
On 2017/06/21 13:46:12, kolos1 wrote:
> On 2017/06/16 12:32:29, vabr (Chromium) wrote:
> > Could you test setting the histograms in the unittest? (Here and line 206
> > below.)
> 
> Let me do it in a separate CL. I believe it is not good place for this metric
> anymore.

ooops, I actually added tests for that in password_manager_unittest.cc

[commit-bot: I haz the power, 2017-06-21 13:51:35]

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

[commit-bot: I haz the power, 2017-06-21 13:51:36]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[kolos1, 2017-06-21 13:59:29]

Patchset #5 (id:140001) has been deleted

[kolos1, 2017-06-21 13:59:34]

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

[commit-bot: I haz the power, 2017-06-21 13:59:50]

Dry run: CQ is trying da patch.

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

[vabr (Chromium), 2017-06-21 14:18:33]

Thanks, Maxim, for the changes. This LGTM!
Vaclav

https://codereview.chromium.org/2937033002/diff/100001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.h
(left):

https://codereview.chromium.org/2937033002/diff/100001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.h:130:
password_manager::PasswordManager* password_manager_;
On 2017/06/21 13:46:12, kolos1 wrote:
> On 2017/06/16 12:32:29, vabr (Chromium) wrote:
> > While I applaud to removing dead code, perhaps this could have been a
separate
> > CL, because it is not really related to the main goal of the CL. If you
could
> > extract this ("git cl split" might help), feel free to just TBR to me and
land
> > the change in c/b/ui/autofill.
> 
> This code became dead because of removing the call of SetHasGeneratedPassword
in
> password_generation_popup_controller_impl.cc. Removing of SetHas... is
> nondetachable part of this CL.

You are right, sorry that I missed that.

https://codereview.chromium.org/2937033002/diff/100001/components/password_ma...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/2937033002/diff/100001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:214:
form_manager->PasswordNoLongerGenerated();
On 2017/06/21 13:46:12, kolos1 wrote:
> On 2017/06/16 12:32:29, vabr (Chromium) wrote:
> > Is there a way to test that this forwarding to PasswordFormManager, and the
> one
> > on line 200, works?
> 
> I believe this forwarding is already tested indirectionally. Generation stuff
> wouldn't work w/o that.  

"wouldn't work" assumes that our specification of what means "work" in the
current tests is complete, which is hard to prove.
However, there is unfortunately still not a good way to mock a PFM, so no easy
way to unittest this (I hope to fix it with the refactoring manual fallbacks for
saving). So I'm fine with not unit-testing it here.

[commit-bot: I haz the power, 2017-06-21 15:06:51]

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

[commit-bot: I haz the power, 2017-06-21 15:06:52]

Dry run: This issue passed the CQ dry run.

[kolos1, 2017-06-21 15:16:29]

The CQ bit was checked by kolos@chromium.org

[kolos1, 2017-06-21 15:16:29]

The patchset sent to the CQ was uploaded after l-g-t-m from dvadym@chromium.org
Link to the patchset: https://codereview.chromium.org/2937033002/#ps160001
(title: "Changes addressed to vabr@ comments")

[commit-bot: I haz the power, 2017-06-21 15:16: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-21 15:21:19]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1498058189430250,
"parent_rev": "ec8d42a55613160a897595d9fcece2620ef2c3b9", "commit_rev":
"5257dd237f1d169b88719b16c2e76682828b35c0"}

[commit-bot: I haz the power, 2017-06-21 15:21:32]

Description was changed from

==========
[Password Generation] Send a boolean flag of whether user changed generated
password

The flag helps to find the sites where passwords generated by Chrome doesn't
meet site's requirements

BUG=699529
==========

to

==========
[Password Generation] Send a boolean flag of whether user changed generated
password

The flag helps to find the sites where passwords generated by Chrome doesn't
meet site's requirements

BUG=699529

Review-Url: https://codereview.chromium.org/2937033002
Cr-Commit-Position: refs/heads/master@{#481210}
Committed:
https://chromium.googlesource.com/chromium/src/+/5257dd237f1d169b88719b16c2e7...
==========

[commit-bot: I haz the power, 2017-06-21 15:21:35]

Committed patchset #5 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/5257dd237f1d169b88719b16c2e7...