Fix localstorage data migration.

Fix localstorage data migration.

https://codereview.chromium.org/2748023008 changed the data format for
the new localstorage backend, but broke migration of data from the old
format in the process. This change adds a end-to-end test for migration
and fixes the migration again.

BUG=640707, 586194
Review-Url: https://codereview.chromium.org/2767103002
Cr-Commit-Position: refs/heads/master@{#461351}
Committed: https://chromium.googlesource.com/chromium/src/+/86396aa4a030f140170a52d24972f90cfd75bce9

[Marijn Kruisselbrink, 2017-03-22 17:27:45]

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

[commit-bot: I haz the power, 2017-03-22 17:28:08]

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

[Marijn Kruisselbrink, 2017-03-22 18:35:34]

mek@chromium.org changed reviewers:
+ michaeln@chromium.org

[Marijn Kruisselbrink, 2017-03-22 18:35:34]

As I committed my CL yesterday I realized that it broke data migration (and none
of the tests really tested the migration end-to-end). So addressed both of those
in this CL.

[commit-bot: I haz the power, 2017-03-22 19:06:15]

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

[commit-bot: I haz the power, 2017-03-22 19:06:16]

Dry run: This issue passed the CQ dry run.

[michaeln, 2017-03-22 23:49:51]

lgtm but please see comments

https://codereview.chromium.org/2767103002/diff/1/content/browser/dom_storage...
File content/browser/dom_storage/local_storage_context_mojo.cc (right):

https://codereview.chromium.org/2767103002/diff/1/content/browser/dom_storage...
content/browser/dom_storage/local_storage_context_mojo.cc:75:
result.push_back(0);
The magic "0" value here is pretty magical, it might be worth sharing a function
or a symbol for the value somewhere in common\dom_storage, or short of that,
even duplicating the enum class here. I see the test would pick up a failure in
a future format change, but some breadcrumbs (or compiler errors) might help
more quickly.

https://codereview.chromium.org/2767103002/diff/1/content/browser/dom_storage...
File content/browser/dom_storage/local_storage_context_mojo_unittest.cc (right):

https://codereview.chromium.org/2767103002/diff/1/content/browser/dom_storage...
content/browser/dom_storage/local_storage_context_mojo_unittest.cc:70:
result.insert(result.end(), data, data + input.size() * sizeof(base::char16));
ditto comment about sharing 'format' enum or encode function in some way, maybe
put StorageFormat enum in common/dom_storage_types.h?

also, i'm just noticing these databases aren't portable in the endianness sense,
do we care about that?

[Marijn Kruisselbrink, 2017-03-31 23:05:34]

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

[Marijn Kruisselbrink, 2017-03-31 23:06:15]

https://codereview.chromium.org/2767103002/diff/1/content/browser/dom_storage...
File content/browser/dom_storage/local_storage_context_mojo.cc (right):

https://codereview.chromium.org/2767103002/diff/1/content/browser/dom_storage...
content/browser/dom_storage/local_storage_context_mojo.cc:75:
result.push_back(0);
On 2017/03/22 at 23:49:51, michaeln wrote:
> The magic "0" value here is pretty magical, it might be worth sharing a
function or a symbol for the value somewhere in common\dom_storage, or short of
that, even duplicating the enum class here. I see the test would pick up a
failure in a future format change, but some breadcrumbs (or compiler errors)
might help more quickly.

Yeah, I agree that this isn't very nice. I changed it to at least give the
number a name. Moving to common/dom_storage is something I'd rather not do, as
ultimately the renderer side code should move into blink, so wouldn't be able to
use content/common anyway.

https://codereview.chromium.org/2767103002/diff/1/content/browser/dom_storage...
File content/browser/dom_storage/local_storage_context_mojo_unittest.cc (right):

https://codereview.chromium.org/2767103002/diff/1/content/browser/dom_storage...
content/browser/dom_storage/local_storage_context_mojo_unittest.cc:70:
result.insert(result.end(), data, data + input.size() * sizeof(base::char16));
On 2017/03/22 at 23:49:51, michaeln wrote:
> ditto comment about sharing 'format' enum or encode function in some way,
maybe put StorageFormat enum in common/dom_storage_types.h?

And I changed it so at least the test here shares the encoding function with
LocalStorageContextMojo by adding it as a static function there.

> also, i'm just noticing these databases aren't portable in the endianness
sense, do we care about that?

I think we're okay not being portable endianness wise? But yeah, that's a good
question. I thought somebody said SerializedScriptValue (i.e. indexeddb) isn't
endian safe either, but I'm not sure about that...

[commit-bot: I haz the power, 2017-03-31 23:06:15]

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-04-01 00:08:58]

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

[commit-bot: I haz the power, 2017-04-01 00:08:59]

Dry run: This issue passed the CQ dry run.

[michaeln, 2017-04-02 01:21:40]

still lgtm, even more so :)

[Marijn Kruisselbrink, 2017-04-02 23:07:30]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2017-04-02 23:07:43]

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-04-03 00:09:36]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491174450857640,
"parent_rev": "3c30e1d302ee8b1f8878630f607adb8954c030f2", "commit_rev":
"86396aa4a030f140170a52d24972f90cfd75bce9"}

[commit-bot: I haz the power, 2017-04-03 00:11:31]

Description was changed from

==========
Fix localstorage data migration.

https://codereview.chromium.org/2748023008 changed the data format for
the new localstorage backend, but broke migration of data from the old
format in the process. This change adds a end-to-end test for migration
and fixes the migration again.

BUG=640707,586194
==========

to

==========
Fix localstorage data migration.

https://codereview.chromium.org/2748023008 changed the data format for
the new localstorage backend, but broke migration of data from the old
format in the process. This change adds a end-to-end test for migration
and fixes the migration again.

BUG=640707,586194

Review-Url: https://codereview.chromium.org/2767103002
Cr-Commit-Position: refs/heads/master@{#461351}
Committed:
https://chromium.googlesource.com/chromium/src/+/86396aa4a030f140170a52d24972...
==========

[commit-bot: I haz the power, 2017-04-03 00:11:32]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/86396aa4a030f140170a52d24972...