Implement the statistics table for the passwords.

Implement the statistics table for the passwords.

It contains information of how the user closed the "Save password?" bubble in the past. Basing on this data two different algorithms are to be implemented so the bubble isn't so annoying for the users.

BUG=431739
Committed: https://crrev.com/0ca264b8e9dc7f0713cb78c7ff75099749b4806f
Cr-Commit-Position: refs/heads/master@{#326981}

[vasilii, 2015-04-24 16:16:42]

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

[vasilii, 2015-04-24 16:16:43]

Hi Vaclav,

please review.

[vabr (Chromium), 2015-04-24 17:36:53]

Thanks, Vasilii.

This LGTM, with some comments below.

Because I'll not be able to do another round until Tuesday, and because I trust
you more than I would myself with coding :), I'm fine with you landing this with
whatever way you chose to address my comments and to fix the failing tests you
are fixing right now. I'll have a look post-landing if you indicate you'd like
me to.

Cheers,
Vaclav

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
File components/password_manager/core/browser/password_store.h (right):

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
components/password_manager/core/browser/password_store.h:251: // Synchromous
implementation for manipulating with statistics.
typo: Synchronous

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
components/password_manager/core/browser/password_store.h:254: // Returns a raw
pointer so that InteractionsStats can be forward declared.
Does using scoped_ptr really require #including the referenced class?
And even if so, is avoiding the #include worth this comment and having the raw
pointer? The internal C++ guide (not applying to Chromium, though) actively
discourages forward declarations:
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
components/password_manager/core/browser/password_store.h:310: void
GetSiteStatsInternal(const GURL& origin_domain,
nit: Usually, methods called Get* return something, via return value or an
out-parameter. This one does not. Could you please explain in a comment, what is
the effect of this method, or adjust the name accordingly?

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
components/password_manager/core/browser/password_store.h:310: void
GetSiteStatsInternal(const GURL& origin_domain,
Also, there are now 3 methods with the GetSiteStats* prefix. Especially the
-Internal and -Impl versions might be hard to distinguish in the code.

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
File components/password_manager/core/browser/password_store_default.h (left):

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
components/password_manager/core/browser/password_store_default.h:63: protected:
Any reason to drop the protection? I could not find an added call to 
DeleteAndRecreateDatabaseFile in your CL.

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
File components/password_manager/core/browser/statistics_table.cc (right):

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
components/password_manager/core/browser/statistics_table.cc:11: namespace {
nit: Please add a blank line after the current lines 11 and 18.

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
components/password_manager/core/browser/statistics_table.cc:37: NOTREACHED();
Why is this a NOTREACHED? A NOTREACHED should signal a code path which should
not ever happen (like calling an unimplemented method, or getting past a switch
{...} where all branches return).

This looks like if the database has problems (something outside of the control
of this program), line 37 can be reached. If anything, a DVLOG might be
appropriate. Or just leave this line out completely.

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
File components/password_manager/core/browser/statistics_table.h (right):

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
components/password_manager/core/browser/statistics_table.h:50: // Returns the
statistics for |domain| if exists.
grammar: exists -> it exists

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
components/password_manager/core/browser/statistics_table.h:59: }  // namespace
password_manager
nit: Add a blank line after this line (like in password_store_default.h).

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
File components/password_manager/core/browser/statistics_table_unittest.cc
(right):

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
components/password_manager/core/browser/statistics_table_unittest.cc:16: void
CompareStats(const InteractionsStats& left,
nit: What about renaming to CheckStatsAreEqual. The current name does not say
whether equality is expected.

[vasilii, 2015-04-24 18:40:48]

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
File components/password_manager/core/browser/password_store.h (right):

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
components/password_manager/core/browser/password_store.h:251: // Synchromous
implementation for manipulating with statistics.
On 2015/04/24 17:36:53, vabr (in transit---no reviews) wrote:
> typo: Synchronous

Done.

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
components/password_manager/core/browser/password_store.h:254: // Returns a raw
pointer so that InteractionsStats can be forward declared.
On 2015/04/24 17:36:52, vabr (in transit---no reviews) wrote:
> Does using scoped_ptr really require #including the referenced class?
> And even if so, is avoiding the #include worth this comment and having the raw
> pointer? The internal C++ guide (not applying to Chromium, though) actively
> discourages forward declarations:
>
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

Done.

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
components/password_manager/core/browser/password_store.h:310: void
GetSiteStatsInternal(const GURL& origin_domain,
On 2015/04/24 17:36:52, vabr (in transit---no reviews) wrote:
> nit: Usually, methods called Get* return something, via return value or an
> out-parameter. This one does not. Could you please explain in a comment, what
is
> the effect of this method, or adjust the name accordingly?

Done.

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
components/password_manager/core/browser/password_store.h:310: void
GetSiteStatsInternal(const GURL& origin_domain,
On 2015/04/24 17:36:53, vabr (in transit---no reviews) wrote:
> Also, there are now 3 methods with the GetSiteStats* prefix. Especially the
> -Internal and -Impl versions might be hard to distinguish in the code.

Done.

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
File components/password_manager/core/browser/password_store_default.h (left):

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
components/password_manager/core/browser/password_store_default.h:63: protected:
On 2015/04/24 17:36:53, vabr (in transit---no reviews) wrote:
> Any reason to drop the protection? I could not find an added call to 
> DeleteAndRecreateDatabaseFile in your CL.

It was double protected :-)

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
File components/password_manager/core/browser/statistics_table.cc (right):

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
components/password_manager/core/browser/statistics_table.cc:11: namespace {
On 2015/04/24 17:36:53, vabr (in transit---no reviews) wrote:
> nit: Please add a blank line after the current lines 11 and 18.

Done.

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
components/password_manager/core/browser/statistics_table.cc:37: NOTREACHED();
On 2015/04/24 17:36:53, vabr (in transit---no reviews) wrote:
> Why is this a NOTREACHED? A NOTREACHED should signal a code path which should
> not ever happen (like calling an unimplemented method, or getting past a
switch
> {...} where all branches return).
> 
> This looks like if the database has problems (something outside of the control
> of this program), line 37 can be reached. If anything, a DVLOG might be
> appropriate. Or just leave this line out completely.

Done.

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
File components/password_manager/core/browser/statistics_table.h (right):

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
components/password_manager/core/browser/statistics_table.h:50: // Returns the
statistics for |domain| if exists.
On 2015/04/24 17:36:53, vabr (in transit---no reviews) wrote:
> grammar: exists -> it exists

Done.

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
components/password_manager/core/browser/statistics_table.h:59: }  // namespace
password_manager
On 2015/04/24 17:36:53, vabr (in transit---no reviews) wrote:
> nit: Add a blank line after this line (like in password_store_default.h).

Done.

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
File components/password_manager/core/browser/statistics_table_unittest.cc
(right):

https://codereview.chromium.org/1083293004/diff/1/components/password_manager...
components/password_manager/core/browser/statistics_table_unittest.cc:16: void
CompareStats(const InteractionsStats& left,
On 2015/04/24 17:36:53, vabr (in transit---no reviews) wrote:
> nit: What about renaming to CheckStatsAreEqual. The current name does not say
> whether equality is expected.

Done.

[vasilii, 2015-04-25 23:33:34]

The CQ bit was checked by vasilii@chromium.org

[vasilii, 2015-04-25 23:33:34]

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/1083293004/#ps40001
(title: "address the comments")

[commit-bot: I haz the power, 2015-04-25 23:33:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1083293004/40001

[commit-bot: I haz the power, 2015-04-26 00:48:11]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-26 00:49:09]

Patchset 3 (id:??) landed as
https://crrev.com/0ca264b8e9dc7f0713cb78c7ff75099749b4806f
Cr-Commit-Position: refs/heads/master@{#326981}