Creating class for processing of keypress events.

Creating class for processing of keypress events.

This CL includes
1.Making ChromePasswordManagerClient to be InputEventObserver
2.ChromePasswordManagerClient subscribes itself on InputEvents on navigation of main frame.
3.Implementing class PasswordReuseDetectionManager, that will manage all password reuse relate stuff. Currently it saves only keystrokes in string.
4.Client propagates keystrokes to PasswordReuseDetectionManager

Calling PasswordStore for verification of password reuse will be done in later CLs.

BUG=657041
Committed: https://crrev.com/603e76750872611703d70a2baf397466f6e732b2
Cr-Commit-Position: refs/heads/master@{#440751}

[dvadym, 2016-12-23 14:59:37]

Description was changed from

==========
Handling keystrokes

BUG=None
==========

to

==========
Creating class for processing of keypress events.

This CL includes
1.Making ChromePasswordManagerClient to be InputEventObserver
2.ChromePasswordManagerClient subscribes itself on InputEvents on navigation of
main frame
3.Implementing class PasswordReuseDetectionManager, that will manage all 
password reuse relate stuff. Currently it saves only keystrokes in string.
4.Client propagates keystrokes to PasswordReuseDetectionManager

BUG=657041
==========

[dvadym, 2016-12-23 15:02:04]

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

[commit-bot: I haz the power, 2016-12-23 15:02:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dvadym, 2016-12-23 15:03:05]

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

[dvadym, 2016-12-23 15:03:53]

hi Vaclav,

 Could you please review this CL?

Regards,
Vadym

[dvadym, 2016-12-23 15:05:04]

Description was changed from

==========
Creating class for processing of keypress events.

This CL includes
1.Making ChromePasswordManagerClient to be InputEventObserver
2.ChromePasswordManagerClient subscribes itself on InputEvents on navigation of
main frame
3.Implementing class PasswordReuseDetectionManager, that will manage all 
password reuse relate stuff. Currently it saves only keystrokes in string.
4.Client propagates keystrokes to PasswordReuseDetectionManager

BUG=657041
==========

to

==========
Creating class for processing of keypress events.

This CL includes
1.Making ChromePasswordManagerClient to be InputEventObserver
2.ChromePasswordManagerClient subscribes itself on InputEvents on navigation of
main frame.
3.Implementing class PasswordReuseDetectionManager, that will manage all
password reuse relate stuff. Currently it saves only keystrokes in string.
4.Client propagates keystrokes to PasswordReuseDetectionManager

Calling PasswordStore for verification of password reuse will be done in later
CLs.

BUG=657041
==========

[commit-bot: I haz the power, 2016-12-23 15:09:29]

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

[commit-bot: I haz the power, 2016-12-23 15:09:30]

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

[dvadym, 2016-12-23 15:26:46]

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

[commit-bot: I haz the power, 2016-12-23 15:27:01]

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, 2016-12-23 16:16:37]

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

[commit-bot: I haz the power, 2016-12-23 16:16:38]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-12-23 17:14:11]

Hi Vadym,

I have some questions below.

I agree that ChromePasswordManagerClient::OnInputEvent and the
PasswordReuseDetectionManager are not testable now and that it will improve once
the detection manager does something detectable :), so it's OK to keep it
untested.

I don't understand what ChromePasswordManagerClient::DidNavigateMainFrame is
doing so far (see my comment below), so not sure whether to test it or not.

Cheers,
Vaclav

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

https://codereview.chromium.org/2472133004/diff/160001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:384:
web_contents()->GetRenderViewHost()->GetWidget()->RemoveInputEventObserver(
Why is it needed to remove and immediately add itself as an observer? (Please
also comment in the code.)

https://codereview.chromium.org/2472133004/diff/160001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:402:
password_manager::PasswordStore* ChromePasswordManagerClient::GetPasswordStore()
nit: Also here and on lines 408-409, please revert the unrelated changes, to
keep git history clear.

https://codereview.chromium.org/2472133004/diff/160001/chrome/browser/passwor...
File chrome/browser/password_manager/chrome_password_manager_client.h (right):

https://codereview.chromium.org/2472133004/diff/160001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.h:181:
base::WeakPtr<autofill::PasswordGenerationPopupControllerImpl>
nit: Unless the tools force you to do this change, could you please revert lines
181-182 to their original state? They do not seem relevant to the rest, so let's
make git blame on that code easier.

https://codereview.chromium.org/2472133004/diff/160001/components/password_ma...
File
components/password_manager/core/browser/password_reuse_detection_manager.cc
(right):

https://codereview.chromium.org/2472133004/diff/160001/components/password_ma...
components/password_manager/core/browser/password_reuse_detection_manager.cc:12:
const size_t kMaxNumberOfCharactersToStore = 30;
constexpr

https://codereview.chromium.org/2472133004/diff/160001/components/password_ma...
components/password_manager/core/browser/password_reuse_detection_manager.cc:30:
input_characters_.erase(
This is fairly inefficient -- with every key stroke (except the first 29), the
whole buffer gets shifted. Instead we should use a cyclic buffer (no shifting at
all), and modify PasswordReuseDetector::CheckReuse to work with that. With
https://crbug.com/668155 in mind, we could even populate the cyclic buffer here
in a reverse manner, to spare computing the reverse in
PasswordReuseDetector::CheckReuse.

I'm aware that your plan was to get a simple version going and apply
optimisations after that. But unlike in the last case
(https://crbug.com/668155), the code here will start processing in Chrome
already.

So either let's do the cyclic buffer, or don't save stuff at all until the
optimised solution is ready.

https://codereview.chromium.org/2472133004/diff/160001/components/password_ma...
File components/password_manager/core/browser/password_reuse_detection_manager.h
(right):

https://codereview.chromium.org/2472133004/diff/160001/components/password_ma...
components/password_manager/core/browser/password_reuse_detection_manager.h:1:
// Copyright (c) 2016 The Chromium Authors. All rights reserved.
nit: No (c).
Also in the other new files.

https://codereview.chromium.org/2472133004/diff/160001/components/password_ma...
components/password_manager/core/browser/password_reuse_detection_manager.h:16:
// saves typed string. TODO(dvadym): write other features of this class when
nit: Please use TODO(crbug.com/657041) instead of TODO(dvadym).

https://codereview.chromium.org/2472133004/diff/160001/components/password_ma...
File
components/password_manager/core/browser/password_reuse_detection_manager_unittest.cc
(right):

https://codereview.chromium.org/2472133004/diff/160001/components/password_ma...
components/password_manager/core/browser/password_reuse_detection_manager_unittest.cc:1:
// Copyright (c) 2016 The Chromium Authors. All rights reserved.
I don't think you are compiling this test.

https://codereview.chromium.org/2472133004/diff/160001/components/password_ma...
components/password_manager/core/browser/password_reuse_detection_manager_unittest.cc:7:
#include <memory>
all three unused?

https://codereview.chromium.org/2472133004/diff/160001/components/password_ma...
components/password_manager/core/browser/password_reuse_detection_manager_unittest.cc:11:
#include "base/strings/utf_string_conversions.h"
unused

https://codereview.chromium.org/2472133004/diff/160001/components/password_ma...
components/password_manager/core/browser/password_reuse_detection_manager_unittest.cc:12:
#include "components/autofill/core/common/password_form.h"
unused

https://codereview.chromium.org/2472133004/diff/160001/components/password_ma...
components/password_manager/core/browser/password_reuse_detection_manager_unittest.cc:17:
using autofill::PasswordForm;
unused

https://codereview.chromium.org/2472133004/diff/160001/components/password_ma...
components/password_manager/core/browser/password_reuse_detection_manager_unittest.cc:39:
EXPECT_CALL(client, AddInputEventObserver(&manager));
This method does not exist in PasswordManagerClient, does it?

[dvadym, 2016-12-23 17:45:20]

Thanks Vaclav for comments. I've addressed them. PTAL.

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

https://codereview.chromium.org/2472133004/diff/160001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:384:
web_contents()->GetRenderViewHost()->GetWidget()->RemoveInputEventObserver(
On 2016/12/23 17:14:10, vabr (Chromium) wrote:
> Why is it needed to remove and immediately add itself as an observer? (Please
> also comment in the code.)

I've added a comment with explanation.

https://codereview.chromium.org/2472133004/diff/160001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:402:
password_manager::PasswordStore* ChromePasswordManagerClient::GetPasswordStore()
On 2016/12/23 17:14:10, vabr (Chromium) wrote:
> nit: Also here and on lines 408-409, please revert the unrelated changes, to
> keep git history clear.

Done.

https://codereview.chromium.org/2472133004/diff/160001/chrome/browser/passwor...
File chrome/browser/password_manager/chrome_password_manager_client.h (right):

https://codereview.chromium.org/2472133004/diff/160001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.h:181:
base::WeakPtr<autofill::PasswordGenerationPopupControllerImpl>
On 2016/12/23 17:14:11, vabr (Chromium) wrote:
> nit: Unless the tools force you to do this change, could you please revert
lines
> 181-182 to their original state? They do not seem relevant to the rest, so
let's
> make git blame on that code easier.

Done.

https://codereview.chromium.org/2472133004/diff/160001/components/password_ma...
File
components/password_manager/core/browser/password_reuse_detection_manager.cc
(right):

https://codereview.chromium.org/2472133004/diff/160001/components/password_ma...
components/password_manager/core/browser/password_reuse_detection_manager.cc:12:
const size_t kMaxNumberOfCharactersToStore = 30;
On 2016/12/23 17:14:11, vabr (Chromium) wrote:
> constexpr

Done.

https://codereview.chromium.org/2472133004/diff/160001/components/password_ma...
components/password_manager/core/browser/password_reuse_detection_manager.cc:30:
input_characters_.erase(
On 2016/12/23 17:14:11, vabr (Chromium) wrote:
> This is fairly inefficient -- with every key stroke (except the first 29), the
> whole buffer gets shifted. Instead we should use a cyclic buffer (no shifting
at
> all), and modify PasswordReuseDetector::CheckReuse to work with that. With
> https://crbug.com/668155 in mind, we could even populate the cyclic buffer
here
> in a reverse manner, to spare computing the reverse in
> PasswordReuseDetector::CheckReuse.
> 
> I'm aware that your plan was to get a simple version going and apply
> optimisations after that. But unlike in the last case
> (https://crbug.com/668155), the code here will start processing in Chrome
> already.
> 
> So either let's do the cyclic buffer, or don't save stuff at all until the
> optimised solution is ready.

Good point. I think that cyclic buffer is the best solution here. I've removed
storing text for now and I'll implement cyclic buffer in next CL.

https://codereview.chromium.org/2472133004/diff/160001/components/password_ma...
File components/password_manager/core/browser/password_reuse_detection_manager.h
(right):

https://codereview.chromium.org/2472133004/diff/160001/components/password_ma...
components/password_manager/core/browser/password_reuse_detection_manager.h:1:
// Copyright (c) 2016 The Chromium Authors. All rights reserved.
On 2016/12/23 17:14:11, vabr (Chromium) wrote:
> nit: No (c).
> Also in the other new files.

Done.

https://codereview.chromium.org/2472133004/diff/160001/components/password_ma...
components/password_manager/core/browser/password_reuse_detection_manager.h:16:
// saves typed string. TODO(dvadym): write other features of this class when
On 2016/12/23 17:14:11, vabr (Chromium) wrote:
> nit: Please use TODO(crbug.com/657041) instead of TODO(dvadym).

Done.

https://codereview.chromium.org/2472133004/diff/160001/components/password_ma...
File
components/password_manager/core/browser/password_reuse_detection_manager_unittest.cc
(right):

https://codereview.chromium.org/2472133004/diff/160001/components/password_ma...
components/password_manager/core/browser/password_reuse_detection_manager_unittest.cc:1:
// Copyright (c) 2016 The Chromium Authors. All rights reserved.
On 2016/12/23 17:14:11, vabr (Chromium) wrote:
> I don't think you are compiling this test.

Sorry, this test should be removed for now. I didn't mentioned that git checkout
master <this file> failed, since it's not in master.

[vabr (Chromium), 2016-12-23 21:24:24]

LGTM with nits.

Thanks!
Vaclav

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

https://codereview.chromium.org/2472133004/diff/160001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:384:
web_contents()->GetRenderViewHost()->GetWidget()->RemoveInputEventObserver(
On 2016/12/23 17:45:20, dvadym wrote:
> On 2016/12/23 17:14:10, vabr (Chromium) wrote:
> > Why is it needed to remove and immediately add itself as an observer?
(Please
> > also comment in the code.)
> 
> I've added a comment with explanation.

Acknowledged.

https://codereview.chromium.org/2472133004/diff/220001/components/password_ma...
File
components/password_manager/core/browser/password_reuse_detection_manager.cc
(right):

https://codereview.chromium.org/2472133004/diff/220001/components/password_ma...
components/password_manager/core/browser/password_reuse_detection_manager.cc:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
2016 :)

https://codereview.chromium.org/2472133004/diff/220001/components/password_ma...
File components/password_manager/core/browser/password_reuse_detection_manager.h
(right):

https://codereview.chromium.org/2472133004/diff/220001/components/password_ma...
components/password_manager/core/browser/password_reuse_detection_manager.h:16:
// saves typed string. TODO(crbug.com/657041): write other features of this
nit: This comment is now out of date.

[dvadym, 2016-12-27 10:39:31]

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

[commit-bot: I haz the power, 2016-12-27 10:39:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dvadym, 2016-12-27 10:40:03]

Thanks Vaclav for review!

https://codereview.chromium.org/2472133004/diff/220001/components/password_ma...
File
components/password_manager/core/browser/password_reuse_detection_manager.cc
(right):

https://codereview.chromium.org/2472133004/diff/220001/components/password_ma...
components/password_manager/core/browser/password_reuse_detection_manager.cc:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
On 2016/12/23 21:24:24, vabr (Chromium) wrote:
> 2016 :)

Done.

https://codereview.chromium.org/2472133004/diff/220001/components/password_ma...
File components/password_manager/core/browser/password_reuse_detection_manager.h
(right):

https://codereview.chromium.org/2472133004/diff/220001/components/password_ma...
components/password_manager/core/browser/password_reuse_detection_manager.h:16:
// saves typed string. TODO(crbug.com/657041): write other features of this
On 2016/12/23 21:24:24, vabr (Chromium) wrote:
> nit: This comment is now out of date.

Done.

[commit-bot: I haz the power, 2016-12-27 10:45:53]

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

[commit-bot: I haz the power, 2016-12-27 10:45:54]

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, 2016-12-27 10:48:50]

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

[commit-bot: I haz the power, 2016-12-27 10:48:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2016-12-27 11:02:27]

LGTM!

https://codereview.chromium.org/2472133004/diff/260001/components/password_ma...
File components/password_manager/core/browser/password_reuse_detection_manager.h
(right):

https://codereview.chromium.org/2472133004/diff/260001/components/password_ma...
components/password_manager/core/browser/password_reuse_detection_manager.h:16:
// do nothing with them. TODO(crbug.com/657041): write other features of this
nit: do -> does

[dvadym, 2016-12-27 11:39:15]

https://codereview.chromium.org/2472133004/diff/260001/components/password_ma...
File components/password_manager/core/browser/password_reuse_detection_manager.h
(right):

https://codereview.chromium.org/2472133004/diff/260001/components/password_ma...
components/password_manager/core/browser/password_reuse_detection_manager.h:16:
// do nothing with them. TODO(crbug.com/657041): write other features of this
On 2016/12/27 11:02:27, vabr (Chromium) wrote:
> nit: do -> does

Done.

[dvadym, 2016-12-27 11:39:20]

The CQ bit was checked by dvadym@chromium.org

[dvadym, 2016-12-27 11:39:21]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/2472133004/#ps280001
(title: "comment fix")

[commit-bot: I haz the power, 2016-12-27 11:39:33]

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, 2016-12-27 12:43:40]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1482838760716110,
"parent_rev": "fd57d625f17820414ecd5d33b74087edadb8d67d", "commit_rev":
"45b68aff08678ff9f84a6572ffcef054e39ee0ba"}

[commit-bot: I haz the power, 2016-12-27 12:44:05]

Description was changed from

==========
Creating class for processing of keypress events.

This CL includes
1.Making ChromePasswordManagerClient to be InputEventObserver
2.ChromePasswordManagerClient subscribes itself on InputEvents on navigation of
main frame.
3.Implementing class PasswordReuseDetectionManager, that will manage all
password reuse relate stuff. Currently it saves only keystrokes in string.
4.Client propagates keystrokes to PasswordReuseDetectionManager

Calling PasswordStore for verification of password reuse will be done in later
CLs.

BUG=657041
==========

to

==========
Creating class for processing of keypress events.

This CL includes
1.Making ChromePasswordManagerClient to be InputEventObserver
2.ChromePasswordManagerClient subscribes itself on InputEvents on navigation of
main frame.
3.Implementing class PasswordReuseDetectionManager, that will manage all
password reuse relate stuff. Currently it saves only keystrokes in string.
4.Client propagates keystrokes to PasswordReuseDetectionManager

Calling PasswordStore for verification of password reuse will be done in later
CLs.

BUG=657041

Review-Url: https://codereview.chromium.org/2472133004
==========

[commit-bot: I haz the power, 2016-12-27 12:44:07]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2017-01-02 15:46:06]

Description was changed from

==========
Creating class for processing of keypress events.

This CL includes
1.Making ChromePasswordManagerClient to be InputEventObserver
2.ChromePasswordManagerClient subscribes itself on InputEvents on navigation of
main frame.
3.Implementing class PasswordReuseDetectionManager, that will manage all
password reuse relate stuff. Currently it saves only keystrokes in string.
4.Client propagates keystrokes to PasswordReuseDetectionManager

Calling PasswordStore for verification of password reuse will be done in later
CLs.

BUG=657041

Review-Url: https://codereview.chromium.org/2472133004
==========

to

==========
Creating class for processing of keypress events.

This CL includes
1.Making ChromePasswordManagerClient to be InputEventObserver
2.ChromePasswordManagerClient subscribes itself on InputEvents on navigation of
main frame.
3.Implementing class PasswordReuseDetectionManager, that will manage all
password reuse relate stuff. Currently it saves only keystrokes in string.
4.Client propagates keystrokes to PasswordReuseDetectionManager

Calling PasswordStore for verification of password reuse will be done in later
CLs.

BUG=657041

Committed: https://crrev.com/603e76750872611703d70a2baf397466f6e732b2
Cr-Commit-Position: refs/heads/master@{#440751}
==========

[commit-bot: I haz the power, 2017-01-02 15:46:08]

Patchset 15 (id:??) landed as
https://crrev.com/603e76750872611703d70a2baf397466f6e732b2
Cr-Commit-Position: refs/heads/master@{#440751}