Implement the renderer side of the mojo based local storage implementation.

Implement the renderer side of the mojo based local storage implementation.

This is based on the existing chrome IPC based implementation.

BUG=586194
Committed: https://crrev.com/490e88eee68dd6907195dfeb0922d9e7cf37a693
Cr-Commit-Position: refs/heads/master@{#382138}

[jam, 2016-03-17 22:31:41]

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

[jam, 2016-03-17 22:31:42]

dcheng: content/common/leveldb_wrapper.mojom
michaeln: the rest

[michaeln, 2016-03-18 01:05:17]

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

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.cc:66: }
do we care about retaining this behavior?

  // Sudden termination is disabled when there are callbacks pending
  // to more reliably commit changes during shutdown.

unless theres reason not to, i'd like to.

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.cc:127:
areas_.erase(area->id());
Where is the LocalStorageCachedArea deleted?

I think adhoc manual refcounting is more error prone then using primitives
custom built for it (case in point:). A LocalStorageCachedArea is shared by
multiple areas, refcounting is a good fit for this class imho. Please consider
retaining that.

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.cc:147: // already
changed it and are waiting for the confirmation callback.
i'm not sure this holds true if the originating was closed after sent but prior
to completion?

Here's the old code...

  WebStorageAreaImpl* originating_area = NULL;
  if (params.connection_id) {
    originating_area = WebStorageAreaImpl::FromConnectionId(
        params.connection_id);
  } else {
    DOMStorageCachedArea* cached_area = proxy_->LookupCachedArea(
        params.namespace_id, params.origin);
    if (cached_area)
      cached_area->ApplyMutation(params.key, params.new_value);
  }


In that world, the browser sends 0 for a connection_id to all but the
originating process. The criteria for same process is not an area with a
matching id.

Oh... we could PackSource(pageurl, area_id, cache_area_id) so we could detect of
the change originated in |this| object.

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.cc:239:
map_->SwapValues(&values);
wdyt about creating parallel TimeToPrime uma stats so we can compare apples to
apples?

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.cc:248:
std::map<base::string16, int>::iterator found =
maybe use auto here for readability?

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.cc:255: void
LocalStorageCachedArea::OnRemoveItemComplete(
looks like we could share code for OnSet and OnRemove complete?

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.cc:265: void
LocalStorageCachedArea::OnClearComplete(leveldb::DatabaseError result) {
maybe we don't need this callback, i think recreating the binding solves the
problem this callback was for

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
File content/renderer/dom_storage/local_storage_cached_area.h (right):

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.h:88:
LocalStorageCachedAreas* cached_areas_;
have this one be weak

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
File content/renderer/dom_storage/local_storage_cached_areas.h (right):

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_areas.h:30:
LocalStorageCachedArea* GetLocalStorageCachedArea(const url::Origin& origin);
naming nit: since these are in a class named LocalStorageCachedAreas, you could
use shorter method names... maybe OpenCachedArea/CacheAreaClosed()

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_areas.h:36:
StoragePartitionService* const storage_partition_service_;
I'm a wary about the assumption that Blink is well behaved and will actually
delete all of the WebStorageAreas prior to Shutdown() and/or at least not invoke
or delete any after Shutdown() has been called.

Having been burned by shutdown bugs too many time, i'd rather defend against the
unexpected in that regard. I have little confidence that  ~WebStorageArea()
cant/wont happen after ~LocalStorageCachedAreas() in some crazy cases.

Maybe have the backptrs be WeakPtrs?

[dcheng, 2016-03-18 06:35:30]

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

https://codereview.chromium.org/1814003002/diff/20001/content/common/leveldb_...
content/common/leveldb_wrapper.mojom:14: // In the methods below, |source| is a
user-defined string which was passed to
Why do we need key and source? It's not clear to me what source is ultimately
used for.

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
File content/renderer/dom_storage/local_storage_area.h (right):

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_area.h:41: std::string id_;
Maybe const this so it can only be bound in the constructor.

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

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.cc:24: return
page_url.spec() + "\n" + storage_area_id;
Can GURL::spec() contain raw newlines?

https://codereview.chromium.org/1814003002/diff/20001/mojo/common/common_type...
File mojo/common/common_type_converters.cc (right):

https://codereview.chromium.org/1814003002/diff/20001/mojo/common/common_type...
mojo/common/common_type_converters.cc:59: base::string16
TypeConverter<base::string16, Array<uint8_t> >::Convert(
Nit: >>

https://codereview.chromium.org/1814003002/diff/20001/mojo/common/common_type...
mojo/common/common_type_converters.cc:61: if (input.is_null() || input.size() ==
0u)
Nit: input.empty()

https://codereview.chromium.org/1814003002/diff/20001/mojo/common/common_type...
mojo/common/common_type_converters.cc:71: if (input.size() > 0)
Nit: input.empty()

https://codereview.chromium.org/1814003002/diff/20001/mojo/common/common_type...
File mojo/common/common_type_converters.h (right):

https://codereview.chromium.org/1814003002/diff/20001/mojo/common/common_type...
mojo/common/common_type_converters.h:50: struct MOJO_COMMON_EXPORT
TypeConverter<base::string16, Array<uint8_t> > {
Nit: >>

[jam, 2016-03-18 16:48:32]

jam@chromium.org changed reviewers:
+ isherman@chromium.org

[jam, 2016-03-18 16:48:34]

+isherman for histograms.xml

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

https://codereview.chromium.org/1814003002/diff/20001/content/common/leveldb_...
content/common/leveldb_wrapper.mojom:14: // In the methods below, |source| is a
user-defined string which was passed to
On 2016/03/18 06:35:29, dcheng wrote:
> Why do we need key and source? It's not clear to me what source is ultimately
> used for.

"key" is for the key-value pair

"source" is arbitrary user data that is sent in the LevelDBWrapper calls, like
Put/Delete, and the same string is passed back in the observer interface. this
is needed for mutation events, so that when a renderer's observer is called it
can figure out if its process or another was responsible for the change

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
File content/renderer/dom_storage/local_storage_area.h (right):

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_area.h:41: std::string id_;
On 2016/03/18 06:35:29, dcheng wrote:
> Maybe const this so it can only be bound in the constructor.

i believe this is personal preference (i.e. nothing in the style guide about
this). i don't normally do this (just don't think it's that useful)

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

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.cc:24: return
page_url.spec() + "\n" + storage_area_id;
On 2016/03/18 06:35:29, dcheng wrote:
> Can GURL::spec() contain raw newlines?

no (that's why i used it. it's one of the characters that aren't allowed in a
valid url, just like space, tab etc)

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.cc:66: }
On 2016/03/18 01:05:16, michaeln wrote:
> do we care about retaining this behavior?
> 
>   // Sudden termination is disabled when there are callbacks pending
>   // to more reliably commit changes during shutdown.
> 
> unless theres reason not to, i'd like to.

good question. i should have called it out that i didn't port that part
explicitly. 

the patchset that added it is
https://chromiumcodereview.appspot.com/10383123/#ps45003, and the comment was
"Darin brought up a good point about having to ensure that changes get flushed
during shutdown, so I'll be poking at the DomStorageDispatcher some more to see
to that."

It wasn't clear to me why we need to keep the renderer alive, since it's kept
alive just until it gets confirmation from the leveldb service. but the calls to
it have already been sent out, which means they're already being applied to the
database. i.e. i don't see how keeping the renderer would help ensure they're
flushed.

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.cc:127:
areas_.erase(area->id());
On 2016/03/18 01:05:17, michaeln wrote:
> Where is the LocalStorageCachedArea deleted?

ah I forgot to add that (hard to remember this stuff since I can't test this
code now until the other pieces exist). fixed.

> 
> I think adhoc manual refcounting is more error prone then using primitives
> custom built for it (case in point:).

in this case i removed the automatic refcounting because i needed explicit calls
from the LocalStorageArea to LocalStorageCachedArea to tell it about
construction/destruction. i felt it was a bit redundant to have both these calls
expliclity, and implicit references using scoped_refptr. but looking back at the
changes i dont feel strongly either way.

> A LocalStorageCachedArea is shared by
> multiple areas, refcounting is a good fit for this class imho. Please consider
> retaining that.

i don't feel strongly about this. seems like you prefer it, so i added it back

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.cc:147: // already
changed it and are waiting for the confirmation callback.
On 2016/03/18 01:05:17, michaeln wrote:
> i'm not sure this holds true if the originating was closed after sent but
prior
> to completion?
> 
> Here's the old code...
> 
>   WebStorageAreaImpl* originating_area = NULL;
>   if (params.connection_id) {
>     originating_area = WebStorageAreaImpl::FromConnectionId(
>         params.connection_id);
>   } else {
>     DOMStorageCachedArea* cached_area = proxy_->LookupCachedArea(
>         params.namespace_id, params.origin);
>     if (cached_area)
>       cached_area->ApplyMutation(params.key, params.new_value);
>   }
> 
> 
> In that world, the browser sends 0 for a connection_id to all but the
> originating process. The criteria for same process is not an area with a
> matching id.
> 
> Oh... we could PackSource(pageurl, area_id, cache_area_id) so we could detect
of
> the change originated in |this| object.

this is covered by the fact that observers are called before the callback (see
comments i added in leveldb_wrapper_impl.cc). so ignore_key_mutations_ will
still have the key and the mutation won't be applied.

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.cc:239:
map_->SwapValues(&values);
On 2016/03/18 01:05:17, michaeln wrote:
> wdyt about creating parallel TimeToPrime uma stats so we can compare apples to
> apples?

good idea, done

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.cc:248:
std::map<base::string16, int>::iterator found =
On 2016/03/18 01:05:17, michaeln wrote:
> maybe use auto here for readability?

Done.

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.cc:255: void
LocalStorageCachedArea::OnRemoveItemComplete(
On 2016/03/18 01:05:17, michaeln wrote:
> looks like we could share code for OnSet and OnRemove complete?

it's 3 lines ignoring the dcheck; i didn't think it was worth it to put that
into a function?

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.cc:265: void
LocalStorageCachedArea::OnClearComplete(leveldb::DatabaseError result) {
On 2016/03/18 01:05:17, michaeln wrote:
> maybe we don't need this callback, i think recreating the binding solves the
> problem this callback was for

i kept it just so we can have a DCHECK during testing..

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
File content/renderer/dom_storage/local_storage_cached_area.h (right):

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.h:88:
LocalStorageCachedAreas* cached_areas_;
On 2016/03/18 01:05:17, michaeln wrote:
> have this one be weak

the way the code is structured is that LocalStorageCachedAreas is renderer wide
and outlives all these objects (since it keeps track of them). so i dont think a
weak pointer is needed. if we ever see crashes, we would know that something is
very wrong.

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
File content/renderer/dom_storage/local_storage_cached_areas.h (right):

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_areas.h:30:
LocalStorageCachedArea* GetLocalStorageCachedArea(const url::Origin& origin);
On 2016/03/18 01:05:17, michaeln wrote:
> naming nit: since these are in a class named LocalStorageCachedAreas, you
could
> use shorter method names... maybe OpenCachedArea/CacheAreaClosed()

Done.

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_areas.h:36:
StoragePartitionService* const storage_partition_service_;
On 2016/03/18 01:05:17, michaeln wrote:
> I'm a wary about the assumption that Blink is well behaved and will actually
> delete all of the WebStorageAreas prior to Shutdown() and/or at least not
invoke
> or delete any after Shutdown() has been called.
> 
> Having been burned by shutdown bugs too many time, i'd rather defend against
the
> unexpected in that regard. I have little confidence that  ~WebStorageArea()
> cant/wont happen after ~LocalStorageCachedAreas() in some crazy cases.
> 
> Maybe have the backptrs be WeakPtrs?

in this specific case, I prefer to not do this unless we see crashes. the reason
is that LocalStorageCachedAreas is owned by the blink platform impl. that is
alive for the lifetime of the renderer process (owned by RenderThread). so i
don't see how that could happen.

note that this code will all move to blink once it's working, so hopefully that
decreases the chance that there's different assumptions.

https://codereview.chromium.org/1814003002/diff/20001/mojo/common/common_type...
File mojo/common/common_type_converters.cc (right):

https://codereview.chromium.org/1814003002/diff/20001/mojo/common/common_type...
mojo/common/common_type_converters.cc:59: base::string16
TypeConverter<base::string16, Array<uint8_t> >::Convert(
On 2016/03/18 06:35:29, dcheng wrote:
> Nit: >>

Done.

https://codereview.chromium.org/1814003002/diff/20001/mojo/common/common_type...
mojo/common/common_type_converters.cc:61: if (input.is_null() || input.size() ==
0u)
On 2016/03/18 06:35:29, dcheng wrote:
> Nit: input.empty()

Done.

https://codereview.chromium.org/1814003002/diff/20001/mojo/common/common_type...
mojo/common/common_type_converters.cc:71: if (input.size() > 0)
On 2016/03/18 06:35:29, dcheng wrote:
> Nit: input.empty()

Done.

https://codereview.chromium.org/1814003002/diff/20001/mojo/common/common_type...
File mojo/common/common_type_converters.h (right):

https://codereview.chromium.org/1814003002/diff/20001/mojo/common/common_type...
mojo/common/common_type_converters.h:50: struct MOJO_COMMON_EXPORT
TypeConverter<base::string16, Array<uint8_t> > {
On 2016/03/18 06:35:29, dcheng wrote:
> Nit: >>

Done.

[Ilya Sherman, 2016-03-18 21:46:14]

metrics lgtm % nits:

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

https://codereview.chromium.org/1814003002/diff/40001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.cc:8: #include
"base/metrics/histogram.h"
nit: Please #include histogram_macros instead.

https://codereview.chromium.org/1814003002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1814003002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:18801: +<histogram
name="LocalStorage.MojoTimeToPrime">
Please specify the units.

https://codereview.chromium.org/1814003002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:18809: +<histogram
name="LocalStorage.MojoTimeToPrimeFor">
Please specify the units.

[michaeln, 2016-03-18 22:01:14]

lgtm

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

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.cc:66: }
per our hallway discussion... nevermind :)

mojo has a very nice characteristic i this regard, upon return from a method
call the data may not yet have been read by the remote side, but it will
eventaully be delivered even if the sender crashes on the next line

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.cc:147: // already
changed it and are waiting for the confirmation callback.
On 2016/03/18 16:48:33, jam wrote:
> On 2016/03/18 01:05:17, michaeln wrote:
> > i'm not sure this holds true if the originating was closed after sent but
> prior
> > to completion?
> > 
> > Here's the old code...
> > 
> >   WebStorageAreaImpl* originating_area = NULL;
> >   if (params.connection_id) {
> >     originating_area = WebStorageAreaImpl::FromConnectionId(
> >         params.connection_id);
> >   } else {
> >     DOMStorageCachedArea* cached_area = proxy_->LookupCachedArea(
> >         params.namespace_id, params.origin);
> >     if (cached_area)
> >       cached_area->ApplyMutation(params.key, params.new_value);
> >   }
> > 
> > 
> > In that world, the browser sends 0 for a connection_id to all but the
> > originating process. The criteria for same process is not an area with a
> > matching id.
> > 
> > Oh... we could PackSource(pageurl, area_id, cache_area_id) so we could
detect
> of
> > the change originated in |this| object.
> 
> this is covered by the fact that observers are called before the callback (see
> comments i added in leveldb_wrapper_impl.cc). so ignore_key_mutations_ will
> still have the key and the mutation won't be applied.

ah, yes, i see!

ok, please update the comment to not suggest the change must have come from a
different renderer

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
File content/renderer/dom_storage/local_storage_cached_area.h (right):

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.h:88:
LocalStorageCachedAreas* cached_areas_;
On 2016/03/18 16:48:33, jam wrote:
> On 2016/03/18 01:05:17, michaeln wrote:
> > have this one be weak
> 
> the way the code is structured is that LocalStorageCachedAreas is renderer
wide
> and outlives all these objects (since it keeps track of them). so i dont think
a
> weak pointer is needed. if we ever see crashes, we would know that something
is
> very wrong.

ok, but dont be surprised when your surprised by what happens out in the wild or
in test harnesses :)

when a shutdown bug like this occurs, it's generally not that something has gone
"very wrong", its more the case that something rather unexpected but unimportant
has happened, and then the P1 release blocking status of such bugs is really not
a good use of various people's time

https://codereview.chromium.org/1814003002/diff/40001/content/renderer/dom_st...
File content/renderer/dom_storage/local_storage_area.h (right):

https://codereview.chromium.org/1814003002/diff/40001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_area.h:21: explicit
LocalStorageArea(scoped_refptr<LocalStorageCachedArea> cached_area);
thnx

https://codereview.chromium.org/1814003002/diff/40001/content/renderer/dom_st...
File content/renderer/dom_storage/local_storage_cached_areas.h (right):

https://codereview.chromium.org/1814003002/diff/40001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_areas.h:18: // Owns all the
LocalStorageCachedArea objects in a renderer. This is needed
nit: owns is a little misleading, "keeps a map of"?

[jam, 2016-03-18 22:15:00]

jam@chromium.org changed reviewers:
+ tsepez@chromium.org

[jam, 2016-03-18 22:15:02]

+tsepez instead of dcheng who's ooo

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

https://codereview.chromium.org/1814003002/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.cc:147: // already
changed it and are waiting for the confirmation callback.
On 2016/03/18 22:01:14, michaeln wrote:
> On 2016/03/18 16:48:33, jam wrote:
> > On 2016/03/18 01:05:17, michaeln wrote:
> > > i'm not sure this holds true if the originating was closed after sent but
> > prior
> > > to completion?
> > > 
> > > Here's the old code...
> > > 
> > >   WebStorageAreaImpl* originating_area = NULL;
> > >   if (params.connection_id) {
> > >     originating_area = WebStorageAreaImpl::FromConnectionId(
> > >         params.connection_id);
> > >   } else {
> > >     DOMStorageCachedArea* cached_area = proxy_->LookupCachedArea(
> > >         params.namespace_id, params.origin);
> > >     if (cached_area)
> > >       cached_area->ApplyMutation(params.key, params.new_value);
> > >   }
> > > 
> > > 
> > > In that world, the browser sends 0 for a connection_id to all but the
> > > originating process. The criteria for same process is not an area with a
> > > matching id.
> > > 
> > > Oh... we could PackSource(pageurl, area_id, cache_area_id) so we could
> detect
> > of
> > > the change originated in |this| object.
> > 
> > this is covered by the fact that observers are called before the callback
(see
> > comments i added in leveldb_wrapper_impl.cc). so ignore_key_mutations_ will
> > still have the key and the mutation won't be applied.
> 
> ah, yes, i see!
> 
> ok, please update the comment to not suggest the change must have come from a
> different renderer

Done.

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

https://codereview.chromium.org/1814003002/diff/40001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.cc:8: #include
"base/metrics/histogram.h"
On 2016/03/18 21:46:14, Ilya Sherman (Away Mar 19-27) wrote:
> nit: Please #include histogram_macros instead.

Done.

https://codereview.chromium.org/1814003002/diff/40001/content/renderer/dom_st...
File content/renderer/dom_storage/local_storage_cached_areas.h (right):

https://codereview.chromium.org/1814003002/diff/40001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_areas.h:18: // Owns all the
LocalStorageCachedArea objects in a renderer. This is needed
On 2016/03/18 22:01:14, michaeln wrote:
> nit: owns is a little misleading, "keeps a map of"?

Done.

https://codereview.chromium.org/1814003002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1814003002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:18801: +<histogram
name="LocalStorage.MojoTimeToPrime">
On 2016/03/18 21:46:14, Ilya Sherman (Away Mar 19-27) wrote:
> Please specify the units.

Done.

https://codereview.chromium.org/1814003002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:18809: +<histogram
name="LocalStorage.MojoTimeToPrimeFor">
On 2016/03/18 21:46:14, Ilya Sherman (Away Mar 19-27) wrote:
> Please specify the units.

Done.

[jam, 2016-03-18 22:16:44]

Description was changed from

==========
Implement the renderer side of the mojo based local storage implementation.

This is based off the existing chrome IPC based implementation.

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

to

==========
Implement the renderer side of the mojo based local storage implementation.

This is based on the existing chrome IPC based implementation.

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

[Tom Sepez, 2016-03-18 22:36:09]

mojom LGTM.

[jam, 2016-03-18 22:57:46]

The CQ bit was checked by jam@chromium.org

[jam, 2016-03-18 22:57:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1814003002/#ps60001
(title: "review comments")

[commit-bot: I haz the power, 2016-03-18 22:58:33]

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

[commit-bot: I haz the power, 2016-03-19 00:29:41]

Description was changed from

==========
Implement the renderer side of the mojo based local storage implementation.

This is based on the existing chrome IPC based implementation.

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

to

==========
Implement the renderer side of the mojo based local storage implementation.

This is based on the existing chrome IPC based implementation.

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

[commit-bot: I haz the power, 2016-03-19 00:29:43]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-19 00:31:46]

Description was changed from

==========
Implement the renderer side of the mojo based local storage implementation.

This is based on the existing chrome IPC based implementation.

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

to

==========
Implement the renderer side of the mojo based local storage implementation.

This is based on the existing chrome IPC based implementation.

BUG=586194

Committed: https://crrev.com/490e88eee68dd6907195dfeb0922d9e7cf37a693
Cr-Commit-Position: refs/heads/master@{#382138}
==========

[commit-bot: I haz the power, 2016-03-19 00:31:48]

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