Refactor LoginDatabase migration

Refactor LoginDatabase migration

The migration code had some unnecessarily complicated parts which made
https://codereview.chromium.org/2127533003/ harder than needed.

This CL pulls common database tasks related to table scheme migration
and creation into a separate class, SQLTableBuilder. During that the code
is also cleaned up and made safer by using transactions.

The CL also fixes two typos in the test files.

BUG=413020
Committed: https://crrev.com/8ff2f69c0743ca2b95fd9eac2d9b250e599eeedb
Cr-Commit-Position: refs/heads/master@{#405116}

[vabr (Chromium), 2016-07-06 20:07:05]

Description was changed from

==========
Refactor LoginDatabase migration

The migration code had some unnecessarily complicated parts which made
https://codereview.chromium.org/2127533003/ harder than needed.

This CL attempts to pull renaming and reducing of columns in a table out into
easier-to-understand subroutines, and makes some other minor fixes to the
code. It also fixes two typos in the test files.

BUG=413020
==========

to

==========
Refactor LoginDatabase migration

The migration code had some unnecessarily complicated parts which made
https://codereview.chromium.org/2127533003/ harder than needed.

This CL attempts to pull renaming and reducing of columns in a table out into
easier-to-understand subroutines, and makes some other minor fixes to the
code, e.g., reducing the number of StringPrintf calls, restructuring helper
methods into functions in anonymous namespace with less parameters. The CL
also changes the code so that it does not create the current database scheme
in the middle of the migration sequence, making the result simpler and easier
to extend. It also fixes two typos in the test files.

BUG=413020
==========

[vabr (Chromium), 2016-07-06 20:26:51]

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

[commit-bot: I haz the power, 2016-07-06 20:27:53]

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-07-06 20:55:56]

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

[vabr (Chromium), 2016-07-06 20:55:56]

Hi Vadym,

Could you please take a look?

Thanks!
Vaclav

[commit-bot: I haz the power, 2016-07-07 00:01:14]

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

[commit-bot: I haz the power, 2016-07-07 00:01:15]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-07-07 08:32:05]

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

[commit-bot: I haz the power, 2016-07-07 08:32:18]

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-07-07 08:33:03]

Hi Vadym,

I just added a single comment in patch set 3, all the rest is the same as in the
previous patch set. Sorry about moving things under your review!

Vaclav

[commit-bot: I haz the power, 2016-07-07 09:20:33]

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

[commit-bot: I haz the power, 2016-07-07 09:20:34]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-07-07 20:00:23]

Description was changed from

==========
Refactor LoginDatabase migration

The migration code had some unnecessarily complicated parts which made
https://codereview.chromium.org/2127533003/ harder than needed.

This CL attempts to pull renaming and reducing of columns in a table out into
easier-to-understand subroutines, and makes some other minor fixes to the
code, e.g., reducing the number of StringPrintf calls, restructuring helper
methods into functions in anonymous namespace with less parameters. The CL
also changes the code so that it does not create the current database scheme
in the middle of the migration sequence, making the result simpler and easier
to extend. It also fixes two typos in the test files.

BUG=413020
==========

to

==========
Refactor LoginDatabase migration

The migration code had some unnecessarily complicated parts which made
https://codereview.chromium.org/2127533003/ harder than needed.

This CL pulls common database tasks related to table scheme migration
and creation into a separate class, SQLTableBuilder. During that the code
is also cleaned up and made safer by using transactions.

The CL also fixes two typos in the test files.

BUG=413020
==========

[vabr (Chromium), 2016-07-08 15:57:00]

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

[commit-bot: I haz the power, 2016-07-08 15:57: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-07-08 18:34:38]

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

[commit-bot: I haz the power, 2016-07-08 18:34:39]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-07-08 19:41:42]

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

[commit-bot: I haz the power, 2016-07-08 19:42:07]

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-07-08 19:42:10]

Hello Vadym,

Please take a look, I added the builder as you suggested yesterday.

Cheers,
Vaclav

[vabr (Chromium), 2016-07-08 19:53:28]

The CQ bit was unchecked by vabr@chromium.org

[vabr (Chromium), 2016-07-08 19:54:14]

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

[commit-bot: I haz the power, 2016-07-08 19:55:06]

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-07-08 20:30:18]

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

[commit-bot: I haz the power, 2016-07-08 20:30:19]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[vabr (Chromium), 2016-07-08 21:05:57]

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

[commit-bot: I haz the power, 2016-07-08 21:06:31]

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-07-09 00:33:03]

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

[commit-bot: I haz the power, 2016-07-09 00:33:04]

Dry run: This issue passed the CQ dry run.

[dvadym, 2016-07-12 14:48:59]

Thanks Vaclav, LGTM. Code and comments look great. I've made some comments,
mostly nits. Feel free to skip some nits if you consider them not useful

https://codereview.chromium.org/2126713006/diff/200001/components/password_ma...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/2126713006/diff/200001/components/password_ma...
components/password_manager/core/browser/login_database.cc:554: // Initialize
the cached strings.
Nit: it looks like all next strings can be moved inside if.

https://codereview.chromium.org/2126713006/diff/200001/components/password_ma...
components/password_manager/core/browser/login_database.cc:566: if
(add_statement_.empty()) {
Nit: I'd prefer to extract initialization *statement to separate method, since
Init() method is too big. But it's up to you.

https://codereview.chromium.org/2126713006/diff/200001/components/password_ma...
File components/password_manager/core/browser/sql_table_builder.cc (right):

https://codereview.chromium.org/2126713006/diff/200001/components/password_ma...
components/password_manager/core/browser/sql_table_builder.cc:143:
std::for_each(columns_.begin(), columns_.end(),
Nit: Isn't it easier to use range based for? 
for (auto& column: columns_) {

instead of 

std::for_each(columns_.begin(), columns_.end(),
[&names, this](const Column& column) {

And the same for all others for_each usages.

https://codereview.chromium.org/2126713006/diff/200001/components/password_ma...
components/password_manager/core/browser/sql_table_builder.cc:150: if
(!transaction.Begin() ||
Nit: it's possible to write
  return transaction.Begin() && db->Execute( ...
No negations, no if, only one return.

https://codereview.chromium.org/2126713006/diff/200001/components/password_ma...
components/password_manager/core/browser/sql_table_builder.cc:190: if
(result.empty())
Nit: we can avoid duplication by something like:
if (!result.empty())
  result += " AND ";
result += column.name + "=?";

https://codereview.chromium.org/2126713006/diff/200001/components/password_ma...
components/password_manager/core/browser/sql_table_builder.cc:311: [this](const
Column& column) {
Nit: better to limit scope, and to add to scope only sealed_version_

https://codereview.chromium.org/2126713006/diff/200001/components/password_ma...
File components/password_manager/core/browser/sql_table_builder.h (right):

https://codereview.chromium.org/2126713006/diff/200001/components/password_ma...
components/password_manager/core/browser/sql_table_builder.h:59: void
AddColumn(std::string name, std::string type);
Nit: comments for all methods *Column: It would be look nicer to user the same
type for arguments, i.e. std::string or const std::string&. As I see they are
used in the same way, so it seems there is not sense to make them of different
type.
And if std::string is used and move is done inside a method, probably it makes
sense to indicate in comments to the method that string is moved inside of the
method. WDYT?

https://codereview.chromium.org/2126713006/diff/200001/components/password_ma...
components/password_manager/core/browser/sql_table_builder.h:96: std::string
ListAllNames() const;
Nit: isn't it better to add "Column" in the function name i.e.
ListAllColumnNames

https://codereview.chromium.org/2126713006/diff/200001/components/password_ma...
components/password_manager/core/browser/sql_table_builder.h:125:
std::list<Column>::reverse_iterator FindLastByName(const std::string& name);
Nit: what's about making this function to be const?

[vabr (Chromium), 2016-07-13 09:01:15]

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

[commit-bot: I haz the power, 2016-07-13 09:01:43]

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-07-13 09:12:14]

Thanks, Vadym, for your kind review!

I addressed most of your comments, and replied to the rest with an explanation.
Please have a look if that makes sense to you.

Cheers,
Vaclav

https://codereview.chromium.org/2126713006/diff/200001/components/password_ma...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/2126713006/diff/200001/components/password_ma...
components/password_manager/core/browser/login_database.cc:554: // Initialize
the cached strings.
On 2016/07/12 14:48:58, dvadym wrote:
> Nit: it looks like all next strings can be moved inside if.

Done.

https://codereview.chromium.org/2126713006/diff/200001/components/password_ma...
components/password_manager/core/browser/login_database.cc:566: if
(add_statement_.empty()) {
On 2016/07/12 14:48:58, dvadym wrote:
> Nit: I'd prefer to extract initialization *statement to separate method, since
> Init() method is too big. But it's up to you.

Done.

https://codereview.chromium.org/2126713006/diff/200001/components/password_ma...
File components/password_manager/core/browser/sql_table_builder.cc (right):

https://codereview.chromium.org/2126713006/diff/200001/components/password_ma...
components/password_manager/core/browser/sql_table_builder.cc:143:
std::for_each(columns_.begin(), columns_.end(),
On 2016/07/12 14:48:58, dvadym wrote:
> Nit: Isn't it easier to use range based for? 
> for (auto& column: columns_) {
> 
> instead of 
> 
> std::for_each(columns_.begin(), columns_.end(),
> [&names, this](const Column& column) {
> 
> And the same for all others for_each usages.

Sounds reasonable. I guess I got carried away a bit with STL algorithms :). If I
understand this correctly, for_each would be an advantage if I needed to write
template code unaware of the underlying container type and working over various
ranges other than the whole container. None of that is the case here, so I
followed your suggestion.

https://codereview.chromium.org/2126713006/diff/200001/components/password_ma...
components/password_manager/core/browser/sql_table_builder.cc:150: if
(!transaction.Begin() ||
On 2016/07/12 14:48:58, dvadym wrote:
> Nit: it's possible to write
>   return transaction.Begin() && db->Execute( ...
> No negations, no if, only one return.

Done.

https://codereview.chromium.org/2126713006/diff/200001/components/password_ma...
components/password_manager/core/browser/sql_table_builder.cc:190: if
(result.empty())
On 2016/07/12 14:48:58, dvadym wrote:
> Nit: we can avoid duplication by something like:
> if (!result.empty())
>   result += " AND ";
> result += column.name + "=?";

Done.

https://codereview.chromium.org/2126713006/diff/200001/components/password_ma...
components/password_manager/core/browser/sql_table_builder.cc:311: [this](const
Column& column) {
On 2016/07/12 14:48:58, dvadym wrote:
> Nit: better to limit scope, and to add to scope only sealed_version_

This is not possible, lambda capture must be from the enclosing scope. So I can
capture 'this' but not particular data members. See, e.g.,
http://stackoverflow.com/questions/7895879/using-member-variable-in-lambda-ca....

I know that the wide capture [=] is forbidden in the style, but [this] is a
different situation. It appears on a couple of places in Chromium already. We
even had a similar discussion with engedy@ where I did a special hoop to avoid
capturing |this| and engedy@ suggested to just capture |this| to make the code
simpler. So I'd prefer to keep [this] here.

https://codereview.chromium.org/2126713006/diff/200001/components/password_ma...
File components/password_manager/core/browser/sql_table_builder.h (right):

https://codereview.chromium.org/2126713006/diff/200001/components/password_ma...
components/password_manager/core/browser/sql_table_builder.h:59: void
AddColumn(std::string name, std::string type);
On 2016/07/12 14:48:58, dvadym wrote:
> Nit: comments for all methods *Column: It would be look nicer to user the same
> type for arguments, i.e. std::string or const std::string&. As I see they are
> used in the same way, so it seems there is not sense to make them of different
> type.
> And if std::string is used and move is done inside a method, probably it makes
> sense to indicate in comments to the method that string is moved inside of the
> method. WDYT?

There is some logic in whether the string is passed by value or reference:
 (1) Look-up strings are const ref.
 (2) Strings to be parts of the stored structures are passed by value.

I'm not sure if a comment is needed about moving the string inside of the
method. It should not make a difference for the caller, they are not able to use
it again -- either they passed a temporary (hence no way to reference that
later), or a named object, in which case a copy was made.

I'm torn -- on one hand I'd really like to make a habit out of using the move
semantics where appropriate. And here it is appropriate, because all the
callsites use temporary arguments, and its likely to be like that in the future.

On the other hand, it is apparently still raising suspicion. And in this
particular case the methods are called rarely, so the performance difference is
negligible.

Based on the above I'm going with switching all in const references.

https://codereview.chromium.org/2126713006/diff/200001/components/password_ma...
components/password_manager/core/browser/sql_table_builder.h:96: std::string
ListAllNames() const;
On 2016/07/12 14:48:59, dvadym wrote:
> Nit: isn't it better to add "Column" in the function name i.e.
> ListAllColumnNames

Done.

https://codereview.chromium.org/2126713006/diff/200001/components/password_ma...
components/password_manager/core/browser/sql_table_builder.h:125:
std::list<Column>::reverse_iterator FindLastByName(const std::string& name);
On 2016/07/12 14:48:58, dvadym wrote:
> Nit: what's about making this function to be const?

This won't work, the code modifies the list of columns through the returned
iterator.

[dvadym, 2016-07-13 09:45:11]

Thanks Vaclav, LGTM.

https://codereview.chromium.org/2126713006/diff/200001/components/password_ma...
File components/password_manager/core/browser/sql_table_builder.cc (right):

https://codereview.chromium.org/2126713006/diff/200001/components/password_ma...
components/password_manager/core/browser/sql_table_builder.cc:143:
std::for_each(columns_.begin(), columns_.end(),
On 2016/07/13 09:12:13, vabr (Chromium) wrote:
> On 2016/07/12 14:48:58, dvadym wrote:
> > Nit: Isn't it easier to use range based for? 
> > for (auto& column: columns_) {
> > 
> > instead of 
> > 
> > std::for_each(columns_.begin(), columns_.end(),
> > [&names, this](const Column& column) {
> > 
> > And the same for all others for_each usages.
> 
> Sounds reasonable. I guess I got carried away a bit with STL algorithms :). If
I
> understand this correctly, for_each would be an advantage if I needed to write
> template code unaware of the underlying container type and working over
various
> ranges other than the whole container. None of that is the case here, so I
> followed your suggestion.

I believe there are a lot of cases where for_each can be used, for example for
making polymorphic behaviour by choosing different functional objects or
different collection. But definetely here we don't need this. So it seems to be
more or less true rule of thumb, use that construction that it's easier to
understand and to use. For example using find_if in other places look for me
reasonable.

https://codereview.chromium.org/2126713006/diff/200001/components/password_ma...
components/password_manager/core/browser/sql_table_builder.cc:311: [this](const
Column& column) {
On 2016/07/13 09:12:13, vabr (Chromium) wrote:
> On 2016/07/12 14:48:58, dvadym wrote:
> > Nit: better to limit scope, and to add to scope only sealed_version_
> 
> This is not possible, lambda capture must be from the enclosing scope. So I
can
> capture 'this' but not particular data members. See, e.g.,
>
http://stackoverflow.com/questions/7895879/using-member-variable-in-lambda-ca....
> 
> I know that the wide capture [=] is forbidden in the style, but [this] is a
> different situation. It appears on a couple of places in Chromium already. We
> even had a similar discussion with engedy@ where I did a special hoop to avoid
> capturing |this| and engedy@ suggested to just capture |this| to make the code
> simpler. So I'd prefer to keep [this] here.

Acknowledged. Thanks for explaining this, I didn't know that's impossible to
capture fields. Sometimes it's very informative to make code reviews :).

https://codereview.chromium.org/2126713006/diff/200001/components/password_ma...
File components/password_manager/core/browser/sql_table_builder.h (right):

https://codereview.chromium.org/2126713006/diff/200001/components/password_ma...
components/password_manager/core/browser/sql_table_builder.h:125:
std::list<Column>::reverse_iterator FindLastByName(const std::string& name);
On 2016/07/13 09:12:13, vabr (Chromium) wrote:
> On 2016/07/12 14:48:58, dvadym wrote:
> > Nit: what's about making this function to be const?
> 
> This won't work, the code modifies the list of columns through the returned
> iterator.

Acknowledged

[vabr (Chromium), 2016-07-13 10:39:04]

The CQ bit was unchecked by vabr@chromium.org

[vabr (Chromium), 2016-07-13 10:39:12]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2016-07-13 10:39: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, 2016-07-13 10:43:36]

Description was changed from

==========
Refactor LoginDatabase migration

The migration code had some unnecessarily complicated parts which made
https://codereview.chromium.org/2127533003/ harder than needed.

This CL pulls common database tasks related to table scheme migration
and creation into a separate class, SQLTableBuilder. During that the code
is also cleaned up and made safer by using transactions.

The CL also fixes two typos in the test files.

BUG=413020
==========

to

==========
Refactor LoginDatabase migration

The migration code had some unnecessarily complicated parts which made
https://codereview.chromium.org/2127533003/ harder than needed.

This CL pulls common database tasks related to table scheme migration
and creation into a separate class, SQLTableBuilder. During that the code
is also cleaned up and made safer by using transactions.

The CL also fixes two typos in the test files.

BUG=413020
==========

[commit-bot: I haz the power, 2016-07-13 10:43:37]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-07-13 10:43:45]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 10:45:49]

Description was changed from

==========
Refactor LoginDatabase migration

The migration code had some unnecessarily complicated parts which made
https://codereview.chromium.org/2127533003/ harder than needed.

This CL pulls common database tasks related to table scheme migration
and creation into a separate class, SQLTableBuilder. During that the code
is also cleaned up and made safer by using transactions.

The CL also fixes two typos in the test files.

BUG=413020
==========

to

==========
Refactor LoginDatabase migration

The migration code had some unnecessarily complicated parts which made
https://codereview.chromium.org/2127533003/ harder than needed.

This CL pulls common database tasks related to table scheme migration
and creation into a separate class, SQLTableBuilder. During that the code
is also cleaned up and made safer by using transactions.

The CL also fixes two typos in the test files.

BUG=413020

Committed: https://crrev.com/8ff2f69c0743ca2b95fd9eac2d9b250e599eeedb
Cr-Commit-Position: refs/heads/master@{#405116}
==========

[commit-bot: I haz the power, 2016-07-13 10:45:50]

Patchset 13 (id:??) landed as
https://crrev.com/8ff2f69c0743ca2b95fd9eac2d9b250e599eeedb
Cr-Commit-Position: refs/heads/master@{#405116}