predictors: ResourcePrefetchPredictorTables cleanup.

predictors: ResourcePrefetchPredictorTables cleanup.

Make resource score display in webui correctly.

ResourcePrefetchPredictorTables changes:
- Organize includes and usings.
- Get rid of mocking friend at the cost of moving ctor/dtor from private to
protected section.
- Make ComputeResourceScore public to use it in webui.

BUG=631966
Committed: https://crrev.com/e4bf55e82fa599637f01e3f6de08bc4d32db6450
Cr-Commit-Position: refs/heads/master@{#430245}

[alexilin, 2016-11-04 14:30:28]

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

[commit-bot: I haz the power, 2016-11-04 14:30:41]

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

[alexilin, 2016-11-04 14:33:20]

alexilin@chromium.org changed reviewers:
+ lizeb@chromium.org, pasko@chromium.org, pkasting@chromium.org

[alexilin, 2016-11-04 14:33:21]

pasko@, lizeb@: PTAL *

pkasting@: PTAL webui/
Tiny change to display resource score in webui correctly. (It wasn't displayed
at all before)

[Benoit L, 2016-11-04 15:03:10]

This CL does 3 things:
1. Surface the score in WebUI
2. Remove some un-necessary includes (using git cl lint?)
3. Move code around

(1) and (2) are good, but I'm not a fan of (3). The changes seem to be a matter
of taste, and I don't think this is a strong enough justification to change the
code, and make git log harder to understand.

https://codereview.chromium.org/2478823002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (left):

https://codereview.chromium.org/2478823002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:23: using
sql::Statement;
Why?

https://codereview.chromium.org/2478823002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_tables.h (left):

https://codereview.chromium.org/2478823002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.h:144: static bool
DropTablesIfOutdated(sql::Connection* db);
Why?

[pasko, 2016-11-04 15:16:57]

On 2016/11/04 15:03:10, Benoit L wrote:
> This CL does 3 things:
> 1. Surface the score in WebUI
> 2. Remove some un-necessary includes (using git cl lint?)
> 3. Move code around
> 
> (1) and (2) are good, but I'm not a fan of (3). The changes seem to be a
matter
> of taste, and I don't think this is a strong enough justification to change
the
> code, and make git log harder to understand.

I think lizeb@ means git blame ..

I am not a fan of moving code around either. If we want it, let's do it in a
separate move-only change and add an ignore for it in git hyper-blame (though I
am not sure it will work 100% correct).

[commit-bot: I haz the power, 2016-11-04 15:18:42]

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

[commit-bot: I haz the power, 2016-11-04 15:18:43]

Dry run: This issue passed the CQ dry run.

[alexilin, 2016-11-04 15:23:40]

Description was changed from

==========
predictors: ResourcePrefetchPredictorTables cleanup.

Make resource score display in webui correctly.

ResourcePrefetchPredictorTables changes:
- Organize includes and usings.
- Get rid of mocking friend at the cost of moving ctor/dtor from private to
protected section.
- Align definition order in .cc accroding to declaration order in .h.
- Make ComputeResourceScore public to use it in webui.
- Make functions that manipulate database version non-static.

BUG=631966
==========

to

==========
predictors: ResourcePrefetchPredictorTables cleanup.

Make resource score display in webui correctly.

ResourcePrefetchPredictorTables changes:
- Organize includes and usings.
- Get rid of mocking friend at the cost of moving ctor/dtor from private to
protected section.
- Make ComputeResourceScore public to use it in webui.
- Make functions that manipulate database version non-static.

BUG=631966
==========

[alexilin, 2016-11-04 15:29:47]

Undo moving code. 
As I can see there is no rule to preserve some definition order in .cc files,
right?

https://codereview.chromium.org/2478823002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (left):

https://codereview.chromium.org/2478823002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:23: using
sql::Statement;
On 2016/11/04 15:03:10, Benoit L wrote:
> Why?

sql::Statement is more readable than just Statement IMHO.
The namespace has short name so there is no problems with extra lines of code.
Compare with std:: namespace - we don't use usings with it.

https://codereview.chromium.org/2478823002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_tables.h (left):

https://codereview.chromium.org/2478823002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.h:144: static bool
DropTablesIfOutdated(sql::Connection* db);
On 2016/11/04 15:03:10, Benoit L wrote:
> Why?

I don't see any reason why they should be static.
It looks more consistent to have these methods as class methods than static one.
They are used only inside the class, they use table names that are database
specific. 
PredictorTableBase::DB() is passed as parameter value in ALL cases. There is no
sense to use other value.
Non-static will be just less surprising here.

[alexilin, 2016-11-04 15:32:37]

https://codereview.chromium.org/2478823002/diff/40001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right):

https://codereview.chromium.org/2478823002/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.h:84: static void
TrimResources(PrefetchData* data, size_t max_consecutive_misses);
BTW, I think it's worth to exctract these static methods outside the class
because they're not connected.
What place is more right for it?
WDYT?

[Benoit L, 2016-11-04 15:39:42]

Thanks for reverting some of the changes.

I still think that less code churn is preferable here.

https://codereview.chromium.org/2478823002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_tables.h (left):

https://codereview.chromium.org/2478823002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.h:144: static bool
DropTablesIfOutdated(sql::Connection* db);
On 2016/11/04 15:29:47, alexilin wrote:
> On 2016/11/04 15:03:10, Benoit L wrote:
> > Why?
> 
> I don't see any reason why they should be static.
> It looks more consistent to have these methods as class methods than static
one.
> They are used only inside the class, they use table names that are database
> specific. 
> PredictorTableBase::DB() is passed as parameter value in ALL cases. There is
no
> sense to use other value.
> Non-static will be just less surprising here.

That's a matter of preference here. I prefer to make whatever state a function
uses more explicit, thus making functions static as much as possible.

Generally speaking in cases like this, when correctness, performance or
likelihood of bugs are not affected, I tend to leave the code as it was written,
even if I would have written it differently. And yes, I may write it differently
now, even though I wrote the current (imperfect) version.

[alexilin, 2016-11-04 16:23:39]

Description was changed from

==========
predictors: ResourcePrefetchPredictorTables cleanup.

Make resource score display in webui correctly.

ResourcePrefetchPredictorTables changes:
- Organize includes and usings.
- Get rid of mocking friend at the cost of moving ctor/dtor from private to
protected section.
- Make ComputeResourceScore public to use it in webui.
- Make functions that manipulate database version non-static.

BUG=631966
==========

to

==========
predictors: ResourcePrefetchPredictorTables cleanup.

Make resource score display in webui correctly.

ResourcePrefetchPredictorTables changes:
- Organize includes and usings.
- Get rid of mocking friend at the cost of moving ctor/dtor from private to
protected section.
- Make ComputeResourceScore public to use it in webui.

BUG=631966
==========

[alexilin, 2016-11-04 16:25:59]

https://codereview.chromium.org/2478823002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_tables.h (left):

https://codereview.chromium.org/2478823002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.h:144: static bool
DropTablesIfOutdated(sql::Connection* db);
On 2016/11/04 15:39:42, Benoit L wrote:
> On 2016/11/04 15:29:47, alexilin wrote:
> > On 2016/11/04 15:03:10, Benoit L wrote:
> > > Why?
> > 
> > I don't see any reason why they should be static.
> > It looks more consistent to have these methods as class methods than static
> one.
> > They are used only inside the class, they use table names that are database
> > specific. 
> > PredictorTableBase::DB() is passed as parameter value in ALL cases. There is
> no
> > sense to use other value.
> > Non-static will be just less surprising here.
> 
> That's a matter of preference here. I prefer to make whatever state a function
> uses more explicit, thus making functions static as much as possible.
> 
> Generally speaking in cases like this, when correctness, performance or
> likelihood of bugs are not affected, I tend to leave the code as it was
written,
> even if I would have written it differently. And yes, I may write it
differently
> now, even though I wrote the current (imperfect) version.

Functional programmer detected. :)
Ok, I've got your message. Thanks.

[Benoit L, 2016-11-04 16:55:54]

lgtm, thanks.

[pasko, 2016-11-04 19:33:45]

lgtm

[Peter Kasting, 2016-11-04 20:09:15]

LGTM

[Peter Kasting, 2016-11-04 20:22:26]

On 2016/11/04 15:29:47, alexilin wrote:
> Undo moving code. 
> As I can see there is no rule to preserve some definition order in .cc files,
> right?

The only rule is that function definition order needs to be the same as
declaration order (see
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
).  If you're fixing places we violate that, please do continue to fix them.

Regarding a rule about preserving the existing order/structure of a file (for
the static vs. non-static question), different reviewers have different goals. 
Benoit's suggestions, for example, tend to reduce the chance of introducing
silly bugs while rewriting/cleaning up existing, working code, and mean folks
going through git blame will have fewer steps to take.

I tend to push very aggressively for cleanups and rewrites if they gain any
perceived readability/clarity, at the cost of some of those other things.

In this particular case, it's difficult to make a clear case that the static
versus nonstatic route is obviously better.  Therefore, I would go with the
OWNERS' call (as you've done).

In the future, though, if you want to make a change like this, you would likely
have more success by having your CL description justify the "why" of the change
and not just the "what"; think of it like a good code comment, which not only
summarizes but also motivates the changes.  For example, make an argument that
the DB parameter here is unnecessary abstraction that makes it less clear what
the practical purpose of the functions is, or that it introduces the possibility
of bugs (from passing the wrong argument), or doesn't contribute to testability,
or the resulting code is shorter.

HTH!

[alexilin, 2016-11-07 09:27:05]

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

[commit-bot: I haz the power, 2016-11-07 09:27:23]

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-11-07 10:01:36]

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

[commit-bot: I haz the power, 2016-11-07 10:01:37]

Dry run: This issue passed the CQ dry run.

[alexilin, 2016-11-07 10:12:44]

The CQ bit was checked by alexilin@chromium.org

[commit-bot: I haz the power, 2016-11-07 10:12:56]

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-11-07 10:17:39]

Description was changed from

==========
predictors: ResourcePrefetchPredictorTables cleanup.

Make resource score display in webui correctly.

ResourcePrefetchPredictorTables changes:
- Organize includes and usings.
- Get rid of mocking friend at the cost of moving ctor/dtor from private to
protected section.
- Make ComputeResourceScore public to use it in webui.

BUG=631966
==========

to

==========
predictors: ResourcePrefetchPredictorTables cleanup.

Make resource score display in webui correctly.

ResourcePrefetchPredictorTables changes:
- Organize includes and usings.
- Get rid of mocking friend at the cost of moving ctor/dtor from private to
protected section.
- Make ComputeResourceScore public to use it in webui.

BUG=631966
==========

[commit-bot: I haz the power, 2016-11-07 10:17:40]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-07 10:21:05]

Description was changed from

==========
predictors: ResourcePrefetchPredictorTables cleanup.

Make resource score display in webui correctly.

ResourcePrefetchPredictorTables changes:
- Organize includes and usings.
- Get rid of mocking friend at the cost of moving ctor/dtor from private to
protected section.
- Make ComputeResourceScore public to use it in webui.

BUG=631966
==========

to

==========
predictors: ResourcePrefetchPredictorTables cleanup.

Make resource score display in webui correctly.

ResourcePrefetchPredictorTables changes:
- Organize includes and usings.
- Get rid of mocking friend at the cost of moving ctor/dtor from private to
protected section.
- Make ComputeResourceScore public to use it in webui.

BUG=631966

Committed: https://crrev.com/e4bf55e82fa599637f01e3f6de08bc4d32db6450
Cr-Commit-Position: refs/heads/master@{#430245}
==========

[commit-bot: I haz the power, 2016-11-07 10:21:06]

Patchset 4 (id:??) landed as
https://crrev.com/e4bf55e82fa599637f01e3f6de08bc4d32db6450
Cr-Commit-Position: refs/heads/master@{#430245}