Some fleshing out the mojo based localstorage implementation.

Some fleshing out the mojo based localstorage implementation.

Data is not yet stored on disk, but its functional from a pages point of view. The mojo based plumbing between the caching layers in the renderer and and browser process is working.

The browser side now enforces a size constraint.

The observer on the renderer side now longer lived so storage events are not dropped. Events have to be raised even if the local cache is not loaded.

BUG=586194
Committed: https://crrev.com/7337bd98d54955a98a1e339367ea21d2c98e49cc
Cr-Commit-Position: refs/heads/master@{#385533}

[michaeln, 2016-03-29 22:18:30]

Description was changed from

==========
working

BUG=
==========

to

==========
Some fleshing out the mojo based localstorage implementation.

Data is not yet stored on disk, but its functional from a pages point of view.
The mojo based plumbing between the caching layers in the renderer and and
browser process is working.

BUG=586194
==========

[michaeln, 2016-03-29 22:19:52]

michaeln@chromium.org changed reviewers:
+ erg@chromium.org, jam@chromium.org

[michaeln, 2016-03-29 22:20:46]

need to rebase, but i think this much is working now

https://codereview.chromium.org/1837883003/diff/20001/content/browser/leveldb...
File content/browser/leveldb_wrapper_impl.cc (left):

https://codereview.chromium.org/1837883003/diff/20001/content/browser/leveldb...
content/browser/leveldb_wrapper_impl.cc:51: void
LevelDBWrapperImpl::Get(mojo::Array<uint8_t> key,
we don't have a use for this method in the localstorage impl, so i've taken this
out for now at least

https://codereview.chromium.org/1837883003/diff/20001/content/browser/leveldb...
File content/browser/leveldb_wrapper_impl.cc (right):

https://codereview.chromium.org/1837883003/diff/20001/content/browser/leveldb...
content/browser/leveldb_wrapper_impl.cc:65: observer->KeyAdded(key.Clone(),
value.Clone(), source);
like we've talked about, the repeated cloning is not so great

[michaeln, 2016-03-29 22:31:28]

Description was changed from

==========
Some fleshing out the mojo based localstorage implementation.

Data is not yet stored on disk, but its functional from a pages point of view.
The mojo based plumbing between the caching layers in the renderer and and
browser process is working.

BUG=586194
==========

to

==========
Some fleshing out the mojo based localstorage implementation.

Data is not yet stored on disk, but its functional from a pages point of view.
The mojo based plumbing between the caching layers in the renderer and and
browser process is working.

The observer on the renderer side now longer lived so no storage events will
dropped. Events have to be raised even if the local cache is not loaded. 

BUG=586194
==========

[michaeln, 2016-03-29 22:33:26]

Description was changed from

==========
Some fleshing out the mojo based localstorage implementation.

Data is not yet stored on disk, but its functional from a pages point of view.
The mojo based plumbing between the caching layers in the renderer and and
browser process is working.

The observer on the renderer side now longer lived so no storage events will
dropped. Events have to be raised even if the local cache is not loaded. 

BUG=586194
==========

to

==========
Some fleshing out the mojo based localstorage implementation.

Data is not yet stored on disk, but its functional from a pages point of view.
The mojo based plumbing between the caching layers in the renderer and and
browser process is working.

The observer on the renderer side now longer lived so storage events are not
dropped. Events have to be raised even if the local cache is not loaded. 

BUG=586194
==========

[michaeln, 2016-03-29 22:34:26]

Description was changed from

==========
Some fleshing out the mojo based localstorage implementation.

Data is not yet stored on disk, but its functional from a pages point of view.
The mojo based plumbing between the caching layers in the renderer and and
browser process is working.

The observer on the renderer side now longer lived so storage events are not
dropped. Events have to be raised even if the local cache is not loaded. 

BUG=586194
==========

to

==========
Some fleshing out the mojo based localstorage implementation.

Data is not yet stored on disk, but its functional from a pages point of view.
The mojo based plumbing between the caching layers in the renderer and and
browser process is working.

The browser side now enforces a size constraint.

The observer on the renderer side now longer lived so storage events are not
dropped. Events have to be raised even if the local cache is not loaded. 

BUG=586194
==========

[jam, 2016-03-30 17:15:16]

https://codereview.chromium.org/1837883003/diff/20001/content/browser/leveldb...
File content/browser/leveldb_wrapper_impl.cc (left):

https://codereview.chromium.org/1837883003/diff/20001/content/browser/leveldb...
content/browser/leveldb_wrapper_impl.cc:51: void
LevelDBWrapperImpl::Get(mojo::Array<uint8_t> key,
On 2016/03/29 22:20:46, michaeln wrote:
> we don't have a use for this method in the localstorage impl, so i've taken
this
> out for now at least

it doesn't harm to keep it there, so let's not remove it. leveldbwrapper is
supposed to be a reusable wrapper around leveldb, and this is a core method of
leveldb's APIs

https://codereview.chromium.org/1837883003/diff/40001/content/common/leveldb_...
File content/common/leveldb_wrapper.mojom (right):

https://codereview.chromium.org/1837883003/diff/40001/content/common/leveldb_...
content/common/leveldb_wrapper.mojom:24: GetAllComplete(uint64 request_id);
for consistency with the other methods, GetAll should take a "string source" and
this method should spit it back out

https://codereview.chromium.org/1837883003/diff/40001/content/common/leveldb_...
content/common/leveldb_wrapper.mojom:35: AddObserver(LevelDBObserver observer);
how about combining this with StoragePartitionService::OpenLocalStorage? i.e. we
make it in lockstep that when one opens a database they specify an optional
observer.

the benefit would be to ensure that notifications aren't dropped.

https://codereview.chromium.org/1837883003/diff/40001/content/renderer/dom_st...
File content/renderer/dom_storage/local_storage_cached_area.cc (right):

https://codereview.chromium.org/1837883003/diff/40001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.cc:210: void
LocalStorageCachedArea::GetAllComplete(uint64_t request_id) {
this is subtle, per your offline explanation, so can you add a comment on why
it's needed?

i.e. something along the line that since the GetAll call is synchronous, we need
to not apply mutations that are received in the meantime but are dispatched
after the message loop unwinds.

[michaeln, 2016-03-30 21:57:01]

https://codereview.chromium.org/1837883003/diff/20001/content/browser/leveldb...
File content/browser/leveldb_wrapper_impl.cc (left):

https://codereview.chromium.org/1837883003/diff/20001/content/browser/leveldb...
content/browser/leveldb_wrapper_impl.cc:51: void
LevelDBWrapperImpl::Get(mojo::Array<uint8_t> key,
On 2016/03/30 17:15:16, jam wrote:
> On 2016/03/29 22:20:46, michaeln wrote:
> > we don't have a use for this method in the localstorage impl, so i've taken
> this
> > out for now at least
> 
> it doesn't harm to keep it there, so let's not remove it. leveldbwrapper is
> supposed to be a reusable wrapper around leveldb, and this is a core method of
> leveldb's APIs

Done.

The comment about general purpose'ness is a good point.

I feel like the in memory map based impl in this cl limits the general
purpose'ness somewhat. It puts practical datasize constraints on its use. That's
not a problem with localstorage, but it could be overly constraining for other
applications.

I guess we can retain more general purposes'ness behind the same api.

If max_size is <size_t>::max() and the observer is null, the wrapper can
function as more of a straight passthru with no change-eventing or
size-constraining. Without the need to raise events, we don't need to provide
the old values when changing or removing them. And without the need for size
constraints, we don't need to know how much is in there. It could still provide
a write buffer and some amount of read buffering.

Some impl details to deal with in a later cl.

https://codereview.chromium.org/1837883003/diff/40001/content/common/leveldb_...
File content/common/leveldb_wrapper.mojom (right):

https://codereview.chromium.org/1837883003/diff/40001/content/common/leveldb_...
content/common/leveldb_wrapper.mojom:24: GetAllComplete(uint64 request_id);
On 2016/03/30 17:15:16, jam wrote:
> for consistency with the other methods, GetAll should take a "string source"
and
> this method should spit it back out

Done

The magic cookie value for the localstorage usecase  is different for this
method compared to the others, but thats ok.

https://codereview.chromium.org/1837883003/diff/40001/content/common/leveldb_...
content/common/leveldb_wrapper.mojom:35: AddObserver(LevelDBObserver observer);
On 2016/03/30 17:15:16, jam wrote:
> how about combining this with StoragePartitionService::OpenLocalStorage? i.e.
we
> make it in lockstep that when one opens a database they specify an optional
> observer.
> 
> the benefit would be to ensure that notifications aren't dropped.

Done.

https://codereview.chromium.org/1837883003/diff/40001/content/renderer/dom_st...
File content/renderer/dom_storage/local_storage_cached_area.cc (right):

https://codereview.chromium.org/1837883003/diff/40001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.cc:210: void
LocalStorageCachedArea::GetAllComplete(uint64_t request_id) {
On 2016/03/30 17:15:16, jam wrote:
> this is subtle, per your offline explanation, so can you add a comment on why
> it's needed?
> 
> i.e. something along the line that since the GetAll call is synchronous, we
need
> to not apply mutations that are received in the meantime but are dispatched
> after the message loop unwinds.

Done, added a comment in the this to the mojom file too

[jam, 2016-03-31 19:48:30]

lgtm

[Elliot Glaysher, 2016-03-31 21:21:09]

lgtm

[michaeln, 2016-04-05 18:57:52]

The CQ bit was checked by michaeln@chromium.org

[michaeln, 2016-04-05 18:57:53]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
erg@chromium.org
Link to the patchset: https://codereview.chromium.org/1837883003/#ps100001
(title: " ")

[commit-bot: I haz the power, 2016-04-05 18:58:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1837883003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1837883003/100001

[commit-bot: I haz the power, 2016-04-05 19:10:24]

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

[commit-bot: I haz the power, 2016-04-05 19:10:25]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[michaeln, 2016-04-05 19:53:49]

+dcheng for mojom changes, fyi it's still behind a flag

[michaeln, 2016-04-05 19:54:21]

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

[michaeln, 2016-04-05 19:54:22]

+dcheng for mojom changes
(helps to actually add him to the reviewers line)

[dcheng, 2016-04-06 00:17:39]

https://codereview.chromium.org/1837883003/diff/100001/content/browser/leveld...
File content/browser/leveldb_wrapper_impl.cc (right):

https://codereview.chromium.org/1837883003/diff/100001/content/browser/leveld...
content/browser/leveldb_wrapper_impl.cc:93: key.Clone(),  old_value.Clone(),
source);
Nit: one space after ,

https://codereview.chromium.org/1837883003/diff/100001/content/renderer/dom_s...
File content/renderer/dom_storage/local_storage_cached_area.cc (right):

https://codereview.chromium.org/1837883003/diff/100001/content/renderer/dom_s...
content/renderer/dom_storage/local_storage_cached_area.cc:144:
base::NullableString16 old_value_str(old_value.To<base::string16>(), false);
Seems a bit weird to use mojo::Array<uint8_t> if they're just strings under the
hood. And if they're not strings, then it feels a bit weird to stick it into a
NullableString16.

https://codereview.chromium.org/1837883003/diff/100001/content/renderer/dom_s...
content/renderer/dom_storage/local_storage_cached_area.cc:162: } else if
(map_.get() && !ignore_all_mutations_) {
No .get() here and elsewhere

https://codereview.chromium.org/1837883003/diff/100001/content/renderer/dom_s...
content/renderer/dom_storage/local_storage_cached_area.cc:225:
base::NullableString16& old_value,
const ref

[michaeln, 2016-04-06 01:00:04]

https://codereview.chromium.org/1837883003/diff/100001/content/browser/leveld...
File content/browser/leveldb_wrapper_impl.cc (right):

https://codereview.chromium.org/1837883003/diff/100001/content/browser/leveld...
content/browser/leveldb_wrapper_impl.cc:93: key.Clone(),  old_value.Clone(),
source);
On 2016/04/06 00:17:39, dcheng wrote:
> Nit: one space after ,

Done.

https://codereview.chromium.org/1837883003/diff/100001/content/renderer/dom_s...
File content/renderer/dom_storage/local_storage_cached_area.cc (right):

https://codereview.chromium.org/1837883003/diff/100001/content/renderer/dom_s...
content/renderer/dom_storage/local_storage_cached_area.cc:144:
base::NullableString16 old_value_str(old_value.To<base::string16>(), false);
On 2016/04/06 00:17:39, dcheng wrote:
> Seems a bit weird to use mojo::Array<uint8_t> if they're just strings under
the
> hood. And if they're not strings, then it feels a bit weird to stick it into a
> NullableString16.

It is effectively binary data which blink gives to us as WebString. WebString
makes a distinction between empty and null. The existing domstorage code impl
uses NullableString16 for the internal representation. We're reusing the map_
class for that internal storage here.

The mojo::Array<uint8_t> datatype is what we've chosen as the to transfer this
binary data to the leveldb wrapper class. We might switch to using a
mojo::String data type in later CLs due to how the array types force callers to
create more copies then are really needed, but we're not sure yet and i think we
could do that in a different CL.

https://codereview.chromium.org/1837883003/diff/100001/content/renderer/dom_s...
content/renderer/dom_storage/local_storage_cached_area.cc:162: } else if
(map_.get() && !ignore_all_mutations_) {
On 2016/04/06 00:17:39, dcheng wrote:
> No .get() here and elsewhere

Done.

https://codereview.chromium.org/1837883003/diff/100001/content/renderer/dom_s...
content/renderer/dom_storage/local_storage_cached_area.cc:225:
base::NullableString16& old_value,
On 2016/04/06 00:17:39, dcheng wrote:
> const ref

Done.

[dcheng, 2016-04-06 01:03:12]

lgtm

https://codereview.chromium.org/1837883003/diff/100001/content/renderer/dom_s...
File content/renderer/dom_storage/local_storage_cached_area.cc (right):

https://codereview.chromium.org/1837883003/diff/100001/content/renderer/dom_s...
content/renderer/dom_storage/local_storage_cached_area.cc:144:
base::NullableString16 old_value_str(old_value.To<base::string16>(), false);
On 2016/04/06 at 01:00:04, michaeln wrote:
> On 2016/04/06 00:17:39, dcheng wrote:
> > Seems a bit weird to use mojo::Array<uint8_t> if they're just strings under
the
> > hood. And if they're not strings, then it feels a bit weird to stick it into
a
> > NullableString16.
> 
> It is effectively binary data which blink gives to us as WebString. WebString
makes a distinction between empty and null. The existing domstorage code impl
uses NullableString16 for the internal representation. We're reusing the map_
class for that internal storage here.
> 
> The mojo::Array<uint8_t> datatype is what we've chosen as the to transfer this
binary data to the leveldb wrapper class. We might switch to using a
mojo::String data type in later CLs due to how the array types force callers to
create more copies then are really needed, but we're not sure yet and i think we
could do that in a different CL.

Not part of this CL obviously, but it seems like putting binary data in a
WebString isn't really the right thing to do: it should be WebData. I guess
that's an artifact of the LocalStorage API using DOMStrings though?

[michaeln, 2016-04-06 18:50:17]

Thnx

> Not part of this CL obviously, but it seems like putting binary data in a
> WebString isn't really the right thing to do: it should be WebData. I guess
> that's an artifact of the LocalStorage API using DOMStrings though?

This will definitely get sorted out when the implementation of the renderer-side
caching logic is hoisted all the way into blink, if not sooner.

[michaeln, 2016-04-06 18:53:12]

The CQ bit was checked by michaeln@chromium.org

[michaeln, 2016-04-06 18:53:12]

The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1837883003/#ps120001
(title: " ")

[commit-bot: I haz the power, 2016-04-06 18:53:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1837883003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1837883003/120001

[commit-bot: I haz the power, 2016-04-06 20:03:22]

Description was changed from

==========
Some fleshing out the mojo based localstorage implementation.

Data is not yet stored on disk, but its functional from a pages point of view.
The mojo based plumbing between the caching layers in the renderer and and
browser process is working.

The browser side now enforces a size constraint.

The observer on the renderer side now longer lived so storage events are not
dropped. Events have to be raised even if the local cache is not loaded. 

BUG=586194
==========

to

==========
Some fleshing out the mojo based localstorage implementation.

Data is not yet stored on disk, but its functional from a pages point of view.
The mojo based plumbing between the caching layers in the renderer and and
browser process is working.

The browser side now enforces a size constraint.

The observer on the renderer side now longer lived so storage events are not
dropped. Events have to be raised even if the local cache is not loaded. 

BUG=586194
==========

[commit-bot: I haz the power, 2016-04-06 20:03:23]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-04-06 20:05:38]

Description was changed from

==========
Some fleshing out the mojo based localstorage implementation.

Data is not yet stored on disk, but its functional from a pages point of view.
The mojo based plumbing between the caching layers in the renderer and and
browser process is working.

The browser side now enforces a size constraint.

The observer on the renderer side now longer lived so storage events are not
dropped. Events have to be raised even if the local cache is not loaded. 

BUG=586194
==========

to

==========
Some fleshing out the mojo based localstorage implementation.

Data is not yet stored on disk, but its functional from a pages point of view.
The mojo based plumbing between the caching layers in the renderer and and
browser process is working.

The browser side now enforces a size constraint.

The observer on the renderer side now longer lived so storage events are not
dropped. Events have to be raised even if the local cache is not loaded.

BUG=586194

Committed: https://crrev.com/7337bd98d54955a98a1e339367ea21d2c98e49cc
Cr-Commit-Position: refs/heads/master@{#385533}
==========

[commit-bot: I haz the power, 2016-04-06 20:05:39]

Patchset 7 (id:??) landed as
https://crrev.com/7337bd98d54955a98a1e339367ea21d2c98e49cc
Cr-Commit-Position: refs/heads/master@{#385533}