Persist sessionStorage on disk.

Better session restore: Persist sessionStorage on disk.

Write sessionStorage on disk, and restore it after
relaunching, when recovering from a crash, and when the
startup option "continue where I left off" is selected.

BUG=104292
TEST=Manual; SessionStorageDatabaseTest.ReadOriginsInNamespace


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150314

[marja, 2012-04-03 15:32:22]

Hi michaeln, wdyt about this approach of leveldb-based sessionStorage?

Still missing from this CL:
- For keeping track of which maps are needed, I'd add a key
refcount-<mapid>.
- Shallow copies: they'd just create an association:
"area-<newnamespaceid>-origin = <mapid>"
- Deep copies: they'd just read all the keys for a mapid and copy them under a
different mapid.
- Plugging it in: all DomStorageAreas would share the same
SessionStorageDatabase (refcounted).

[michaeln, 2012-04-03 22:56:45]

I think the class that immediately wraps leveldb is a real good place to start.
Your plan for areas sharing a reference to the same refcount instance of that
class sgtm. If it were me, I'd defer sweating the details of how it gets hooked
up to the existing classes until that core class that wraps leveldb is in good
shape.

On 2012/04/03 15:32:22, marja wrote:
> Hi michaeln, wdyt about this approach of leveldb-based sessionStorage?
>
> Still missing from this CL:
> - For keeping track of which maps are needed, I'd add a key
> refcount-<mapid>.

Not clear you'd need the seperate refcount entries given you have 'association'
entries, provided the 'association' entries were prefixed with <mapid>? Or vice
versa, maybe you wouldn't need 'association' entries if you have 'refcount'
entries.

> - Shallow copies: they'd just create an association:
> "area-<newnamespaceid>-origin = <mapid>"
> - Deep copies: they'd just read all the keys for a mapid and copy them under a
> different mapid.
> - Plugging it in: all DomStorageAreas would s

https://chromiumcodereview.appspot.com/9963107/diff/1/webkit/dom_storage/sess...
File webkit/dom_storage/session_storage_database.cc (right):

https://chromiumcodereview.appspot.com/9963107/diff/1/webkit/dom_storage/sess...
webkit/dom_storage/session_storage_database.cc:17: // Layout of the database:
This seems like the best place to start hammering out the details on, the
'schema'.

https://chromiumcodereview.appspot.com/9963107/diff/1/webkit/dom_storage/sess...
webkit/dom_storage/session_storage_database.cc:20: // | area-1 (1 = namespace
id) | dummy               | start of area-1-* keys
what's the benefit of the dummy entries?

https://chromiumcodereview.appspot.com/9963107/diff/1/webkit/dom_storage/sess...
webkit/dom_storage/session_storage_database.cc:36: std::ostringstream stream;
do we use sstream elswhere in the project? if not, maybe we should be using our
base library functions to form these strings (like base::Int64ToString() and
base::StringPringf())

https://chromiumcodereview.appspot.com/9963107/diff/1/webkit/dom_storage/sess...
webkit/dom_storage/session_storage_database.cc:83: bool
SessionStorageDatabase::CommitChanges(int64 namespace_id,
Another detail to hammer out is around the read/write access patterns where the
class may be used from multiple threads. Not sure if we'll need to use the
'snapshot' facility, but i'm pretty sure we'll need to use the 'writebatch' for
a coherent set of changes in this sort of CommitChanges method.

https://chromiumcodereview.appspot.com/9963107/diff/1/webkit/dom_storage/sess...
webkit/dom_storage/session_storage_database.cc:112: ReadAllValuesInMap(map_id,
&values);
is there a way to clear values w/o having to read them?

https://chromiumcodereview.appspot.com/9963107/diff/1/webkit/dom_storage/sess...
webkit/dom_storage/session_storage_database.cc:129: s =
db_->Put(leveldb::WriteOptions(), key, UTF16ToUTF8(value.string()));
i think we need to treat the 'value' as binary data and not attempt to perform
any character type conversions on it. lets assume there can be embedded nulls
and invalid unicode chars in these values.

https://chromiumcodereview.appspot.com/9963107/diff/1/webkit/dom_storage/sess...
webkit/dom_storage/session_storage_database.cc:136: bool
SessionStorageDatabase::LazyOpen(bool create_if_needed) {
This stuff about lazily creating the directory on disk (a leveldb instance on
disk is a directory) seems secondary to the schema + r/w access pattern stuff.

https://chromiumcodereview.appspot.com/9963107/diff/1/webkit/dom_storage/sess...
File webkit/dom_storage/session_storage_database.h (right):

https://chromiumcodereview.appspot.com/9963107/diff/1/webkit/dom_storage/sess...
webkit/dom_storage/session_storage_database.h:29: virtual void
ReadAllValues(int64 namespace_id,
This pre-existing interface borrowed from localstorage isn't expressive enough
to handle shallowclone / deepcopy type of semantics we're talking about for
sessionstorage. 

Rather than be constrained by that pre-existing interface, can you take a stab
at authoring a storage interface for sessionstorage specifically. I don't think
this class necessarily should derive from the same class currently used for
per-origin sqlite database files. We can come back later and reconcile how
'areas' talk to the storage interface, maybe some adapter classes that smooth
out the differences will come in handy at that time.

[marja, 2012-04-10 15:34:45]

Hi & many thanks for comments!

I didn't manage to think of this SessionStorageDatabase as a separate entity
without worrying about how it integrates with DomStorageArea, so, in this CL
it's plugged in in the DomStorageArea. But I can later split up this CL in
smaller, logical parts.

Next I'll work on deleting the data from the session storage backing whenever
needed (atm it's never deleted).

https://chromiumcodereview.appspot.com/9963107/diff/1/webkit/dom_storage/sess...
File webkit/dom_storage/session_storage_database.cc (right):

https://chromiumcodereview.appspot.com/9963107/diff/1/webkit/dom_storage/sess...
webkit/dom_storage/session_storage_database.cc:20: // | area-1 (1 = namespace
id) | dummy               | start of area-1-* keys
On 2012/04/03 22:56:45, michaeln wrote:
> what's the benefit of the dummy entries?

To be able to iterate "all origins that belong to area-1" or "all keys that
belong to map-1". Otherwise we wouldn't know where to start. Now the first key
of those things to iterate is a known string, and the keys are stored in order.
So, we can start there, and iterate forward until we hit something that we're
not interested in any more.

Actually, the value of the entries "map-1" etc. can be used for storing the
refcount.

https://chromiumcodereview.appspot.com/9963107/diff/1/webkit/dom_storage/sess...
webkit/dom_storage/session_storage_database.cc:36: std::ostringstream stream;
On 2012/04/03 22:56:45, michaeln wrote:
> do we use sstream elswhere in the project? if not, maybe we should be using
our
> base library functions to form these strings (like base::Int64ToString() and
> base::StringPringf())

Done.

https://chromiumcodereview.appspot.com/9963107/diff/1/webkit/dom_storage/sess...
webkit/dom_storage/session_storage_database.cc:83: bool
SessionStorageDatabase::CommitChanges(int64 namespace_id,
On 2012/04/03 22:56:45, michaeln wrote:
> Another detail to hammer out is around the read/write access patterns where
the
> class may be used from multiple threads. Not sure if we'll need to use the
> 'snapshot' facility, but i'm pretty sure we'll need to use the 'writebatch'
for
> a coherent set of changes in this sort of CommitChanges method.

Added WriteBatch. If we allow writing from 2 threads, we'll also need snapshots.
I'll add them later.

https://chromiumcodereview.appspot.com/9963107/diff/1/webkit/dom_storage/sess...
webkit/dom_storage/session_storage_database.cc:112: ReadAllValuesInMap(map_id,
&values);
On 2012/04/03 22:56:45, michaeln wrote:
> is there a way to clear values w/o having to read them?

No, afaik. The interface is just Get(), Put(), Delete(), and we'd need to know
which keys to delete. (Otoh, DOMStorageArea has them in memory. Should I use
that information when deleting the keys?)

https://chromiumcodereview.appspot.com/9963107/diff/1/webkit/dom_storage/sess...
webkit/dom_storage/session_storage_database.cc:129: s =
db_->Put(leveldb::WriteOptions(), key, UTF16ToUTF8(value.string()));
On 2012/04/03 22:56:45, michaeln wrote:
> i think we need to treat the 'value' as binary data and not attempt to perform
> any character type conversions on it. lets assume there can be embedded nulls
> and invalid unicode chars in these values.

Ok, UTF16ToUTF8 was definitely wrong. Otoh, we cannot store arbitrary blobs if
the raw byte count is not even (lots of APIs seem to handle string16's, and I
have no idea how to represent a odd-numbered amount of raw bytes as string16).
But I'll change this so that null bytes and invalid UTF16 char sequences don't
break it.

https://chromiumcodereview.appspot.com/9963107/diff/1/webkit/dom_storage/sess...
File webkit/dom_storage/session_storage_database.h (right):

https://chromiumcodereview.appspot.com/9963107/diff/1/webkit/dom_storage/sess...
webkit/dom_storage/session_storage_database.h:29: virtual void
ReadAllValues(int64 namespace_id,
On 2012/04/03 22:56:45, michaeln wrote:
> This pre-existing interface borrowed from localstorage isn't expressive enough
> to handle shallowclone / deepcopy type of semantics we're talking about for
> sessionstorage. 
> 
> Rather than be constrained by that pre-existing interface, can you take a stab
> at authoring a storage interface for sessionstorage specifically. I don't
think
> this class necessarily should derive from the same class currently used for
> per-origin sqlite database files. We can come back later and reconcile how
> 'areas' talk to the storage interface, maybe some adapter classes that smooth
> out the differences will come in handy at that time. 

Yes, I added ShallowCopy() and DeepCopy() here. But it would be good to be able
to utilize the "db backing for in-memory data" logic of DomStorageArea, so that
I wouldn't need to rewrite that. DomStorageArea::ShallowCopy() and
Set|RemoveItem would then call these functions.

[marja, 2012-04-11 13:34:31]

Hi again,

Michael, could you have another look at this CL draft?

I added code to delete the data:
- ~DomStorageNamespace deletes the corresponding namespace.
- On startup, we'd need to delete the data which is not needed for session
restore, or which was leftover because of an unclean exit. Now DomStorageContext
initiates the data deletion, and I'm planning to integrate the "protect the data
needed for the session restore" in this mechanism (but not in this CL).

How does this approach sound? (And in general the approach in
SessionStorageDatabase.)

[michaeln, 2012-04-12 01:48:46]

I think this is a reasonable start at it. Plenty of details to flesh out still.
The class that wraps leveldb directly doesn't need to be gated on fleshing out
all of the details. I want to encourage you again to focus on that class first,
with r/w access patterns in mind, and after that's done, sweat how to plug it
in.

https://chromiumcodereview.appspot.com/9963107/diff/19001/webkit/dom_storage/...
File webkit/dom_storage/dom_storage_area.cc (right):

https://chromiumcodereview.appspot.com/9963107/diff/19001/webkit/dom_storage/...
webkit/dom_storage/dom_storage_area.cc:130:
session_storage_backing_->DeepCopy(namespace_id_, origin_);
see comments below about serializing deepcopy with other changes

https://chromiumcodereview.appspot.com/9963107/diff/19001/webkit/dom_storage/...
webkit/dom_storage/dom_storage_area.cc:149:
session_storage_backing_->DeepCopy(namespace_id_, origin_);
The changes to the backing store made by DeepCopy have to be serialized with
other changes to the backing store, like made for SetItem and Clear. Otherwise,
you may be copying stale data here which is scheduled to be modified by an
in_flight_commit_batch_.

I think these calls to DeepCopy (actually all mutations to the backing store)
should happen on the COMMIT_SEQUENCE.

Maybe we can add a make_deep_copy_first flag to CommitBatch and have
CommitChanges() make the call to DeepCopy when needed.

https://chromiumcodereview.appspot.com/9963107/diff/19001/webkit/dom_storage/...
webkit/dom_storage/dom_storage_area.cc:168: map_ = new
DomStorageMap(kPerAreaQuota);
if this area is a 'shallow copy', we'll need to coin a new mapid for it in the
session database at this point, no point in actually copying in this case, but
we do need to break the association between this area and the old populated map
(if its shared with another area).

Flags for CommitChanges() could inform it of what do to...

if (commit_batch->make_deep_copy_first) {
  if (commit_batch->clear_all_first)
    // create a new mapid for the area w/o bothering to copy
  else
    DeepCopy();
}

https://chromiumcodereview.appspot.com/9963107/diff/19001/webkit/dom_storage/...
webkit/dom_storage/dom_storage_area.cc:184:
session_storage_backing_->ShallowCopy(namespace_id_, origin_,
see earlier comments about serializing changes in the backing store, this
probably needs to happen on the COMMIT_SEQUENCE too

https://chromiumcodereview.appspot.com/9963107/diff/19001/webkit/dom_storage/...
webkit/dom_storage/dom_storage_area.cc:220:
session_storage_backing_->DeleteOrigin(namespace_id_, origin_);
probably needs to be serialized on the COMMIT_SEQUENCE

https://chromiumcodereview.appspot.com/9963107/diff/19001/webkit/dom_storage/...
File webkit/dom_storage/dom_storage_context.cc (right):

https://chromiumcodereview.appspot.com/9963107/diff/19001/webkit/dom_storage/...
webkit/dom_storage/dom_storage_context.cc:48:
session_storage_database_->DeleteLeftoverData();
This constructor is invoked on the UI thread in chrome early on during profile
creation. We definitely don't want to do a lot of IO at this early time. The
DeleteLeftoverData method results in lots of IO.

https://chromiumcodereview.appspot.com/9963107/diff/19001/webkit/dom_storage/...
webkit/dom_storage/dom_storage_context.cc:98: // FIXME(marja): Get usage infos
for sessionStorage.
The content settings ui doesn't show anything about session storage data
currently and the DeleteOrigin and DeleteDataModifiedSince methods only apply to
localstorage atm. Eventually blending in info about 'sessionstorage' usage is a
possibility.

Maybe open a bug for this instead of having a TODO buried in this class since it
also involves changes to the UI layer and programming interfaces used between
here and there?

https://chromiumcodereview.appspot.com/9963107/diff/19001/webkit/dom_storage/...
webkit/dom_storage/dom_storage_context.cc:217: namespaces_.erase(namespace_id);
This might be a better place to trigger deletion from the
session_storage_database than the namespace dtor? For two reasons...

1) The namespace dtor will get invoked eventually for all of them when
DomStorageContext is destructed. This will include namespaces that you
presumably want to keep around in the database. As coded the namespace dtor will
remove them from the database too.

2) There's no guarantee about what thread ~DomStorageContext (and consequently
~DomStorageNamespace) happens on. The DeleteSessionNamespace() and Shutdown()
methods are very determistic about that (they're invoked on the PRIMARY_SEQUENCE
defined by the DomStorageTaskRunner).

In general, things in this library are written to not depend on destruction as
much as explicit method calls for book keeping.

https://chromiumcodereview.appspot.com/9963107/diff/19001/webkit/dom_storage/...
File webkit/dom_storage/dom_storage_database.h (right):

https://chromiumcodereview.appspot.com/9963107/diff/19001/webkit/dom_storage/...
webkit/dom_storage/dom_storage_database.h:32: virtual void ReadAllValues(int64
namespace_id,
I think this existing class for localstorage backed by a per-origin sqlitedb is
fine as it is. The namespace_id and origin parameters are meaningless to this
class. The problem is that this interface is not sufficient for session storage,
and not sufficient for localstorage that where multiple origins are stored in
the same db.

Instead of forcing the existing class to have the multiplexing interface. Why
dont we do something like this...

// Interface used by DomStorageArea
class DomStorageDatabaseAdapter {
  virtual void ReadAllValues(ValuesMap* result) = 0;
  virtual bool CommitChanges(
      bool clear_all_first, const ValuesMap& changes) = 0;
};

// Impl that utilizes old school per-origin DomStorageDatabase
class LocalStorageDatabaseAdapter {
  LocalStorageDatabaseAdapter(DomStorageDatabase* db);
  virtual void ReadAllValues(ValuesMap* result) = 0;
  virtual bool CommitChanges(
      bool clear_all_first, const ValuesMap& changes) = 0;
 private:
  scoped_ptr<DomStorageDatabase> db_;
};


// Impl that utilizes shiny new multiplexing SessionStorageDatabase
class SessionStorageDatabaseAdapter {
  SessionStorageDatabaseAdapter(SessionStorageDatabase* db,
                                int64 namespace_id,
                                GURL origin);
  virtual void ReadAllValues(ValuesMap* result) = 0;
  virtual bool CommitChanges(
      bool clear_all_first, const ValuesMap& changes) = 0;
 private:
  scoped_refptr<SessionStorageDatabase> db_;
};

Where the namespace creates the appropriate adapter. This might make unit tests
easier to compose (and faster to run) too since areas could be given a mock
adapter to work with.

There are odd difference around 'DeleteOrigin' that could be abstracted out into
the adapter too.

It'd be great if the next change to DomStorageDatabase.cc or .h was an svn
delete.

https://chromiumcodereview.appspot.com/9963107/diff/19001/webkit/dom_storage/...
File webkit/dom_storage/session_storage_database.cc (right):

https://chromiumcodereview.appspot.com/9963107/diff/19001/webkit/dom_storage/...
webkit/dom_storage/session_storage_database.cc:30: // | next-map-id             
 | 4                   |
Do we need to ensure that session namespace_ids in this database are unique
across browser runs? As coded now, they start at 1 and go up from there on every
restart (see DomStorageContext), and those values are passed into this class.

https://chromiumcodereview.appspot.com/9963107/diff/19001/webkit/dom_storage/...
webkit/dom_storage/session_storage_database.cc:35: std::string
AreaStartKey(const std::string& namespace_id) {
Maybe NamespaceStartKey() for clarity?

https://chromiumcodereview.appspot.com/9963107/diff/19001/webkit/dom_storage/...
webkit/dom_storage/session_storage_database.cc:39: std::string
AreaStartKey(int64 namespace_id) {
ditto

https://chromiumcodereview.appspot.com/9963107/diff/19001/webkit/dom_storage/...
webkit/dom_storage/session_storage_database.cc:52: std::string MapStartKey(const
std::string& map_id) {
Maybe MapRefcountKey for clarity?

https://chromiumcodereview.appspot.com/9963107/diff/19001/webkit/dom_storage/...
webkit/dom_storage/session_storage_database.cc:83: leveldb::Status s =
db_->Get(leveldb::ReadOptions(), area_key, &map_id);
How should we handle genuine leveldb errors in this class, in the methods that
write data out in particular? You have DCHECKs that assert no genuine errors can
happen right now... but they can.

https://chromiumcodereview.appspot.com/9963107/diff/19001/webkit/dom_storage/...
webkit/dom_storage/session_storage_database.cc:108: if (s.IsNotFound()) {
style nit: dont need {}'s for one-liners

https://chromiumcodereview.appspot.com/9963107/diff/19001/webkit/dom_storage/...
webkit/dom_storage/session_storage_database.cc:125: // |namespace_id| and its
ref count is increased.
When we a Clone() a session namespace, we're going to do create a shallow copy
for each origin in the src namespace. Should we arrange to have all of those
mutations be performed in a single WriteBatch?

Maybe the method we really want on this class is CloneNamespace(srcid, dstid)?

https://chromiumcodereview.appspot.com/9963107/diff/19001/webkit/dom_storage/...
webkit/dom_storage/session_storage_database.cc:239: void
SessionStorageDatabase::DeleteLeftoverData() {
As coded, this method appears to delete everything. I see this method being
called in the ctor of the context class. So everything that was saved will
promptly be deleted upon constructing the new context on restart. Can you
describe what you have in mind here?

https://chromiumcodereview.appspot.com/9963107/diff/19001/webkit/dom_storage/...
webkit/dom_storage/session_storage_database.cc:287: // situation gracefully by
creating the database now.
tzik recently added logic to handle this sort of situation with
filesystemdirectory leveldb's in the webkit/fileapi world, please take a look at
how this has been handled there and consider if the handling here should be
modified in anyway.

https://chromiumcodereview.appspot.com/9963107/diff/19001/webkit/dom_storage/...
webkit/dom_storage/session_storage_database.cc:317: for
(it->Seek(map_start_key), it->Next(); it->Valid(); it->Next()) {
is it valid to call Next() on an invalid iterator, idk one way or the other,
just asking since it looks like Seek() could return an invalid 'it' initially

https://chromiumcodereview.appspot.com/9963107/diff/19001/webkit/dom_storage/...
webkit/dom_storage/session_storage_database.cc:393: DeleteValuesInMap(map_id,
batch);
This method name is a little decieving since it also may actually delete all the
values in the map. A better name might help?

https://chromiumcodereview.appspot.com/9963107/diff/19001/webkit/dom_storage/...
webkit/dom_storage/session_storage_database.cc:401: ReadValuesInMap(map_id,
&values);
Might be nice to read the keys only since that's all that's needed to delete.

But would be really nice to have a batch->RangeDelete("map-1", "map-1~")
function in leveldb :)

https://chromiumcodereview.appspot.com/9963107/diff/19001/webkit/dom_storage/...
webkit/dom_storage/session_storage_database.cc:422: // Skip the dummy entry,
then start iterating the origins in the area.
The "origins in the area" is a confusing comment since areas are a per-origin
thing. This is iterating the areas in a namespace.

Maybe the method should be NamespaceStartKey() instead?

https://chromiumcodereview.appspot.com/9963107/diff/19001/webkit/dom_storage/...
File webkit/dom_storage/session_storage_database.h (right):

https://chromiumcodereview.appspot.com/9963107/diff/19001/webkit/dom_storage/...
webkit/dom_storage/session_storage_database.h:21: // SessionStorageDatabase.
It'd be great if you carved this class out of the larger CL and put together
some unittests for it.

https://chromiumcodereview.appspot.com/9963107/diff/19001/webkit/dom_storage/...
webkit/dom_storage/session_storage_database.h:49: bool ShallowCopy(int64
namespace_id,
See comment elsewhere about having CloneNamespace to do all areas in a single
batch

[marja, 2012-04-19 10:20:50]

Thanks for comments again,

Q: Where should the "origin -> local storage file name" conversion live? Now
it's in DomStorageArea, but that doesn't seem right. DomStorageDatabase doesn't
seem right either.

Next I'll create a CL for just adding the SessionStorageDatabase and write tests
for it. Does the interface between SessionStorageDatabase and the other classes
look reasonably good?

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_area.cc (right):

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.cc:130:
session_storage_backing_->DeepCopy(namespace_id_, origin_);
On 2012/04/12 01:48:46, michaeln wrote:
> see comments below about serializing deepcopy with other changes

Done.

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.cc:149:
session_storage_backing_->DeepCopy(namespace_id_, origin_);
On 2012/04/12 01:48:46, michaeln wrote:
> The changes to the backing store made by DeepCopy have to be serialized with
> other changes to the backing store, like made for SetItem and Clear.
Otherwise,
> you may be copying stale data here which is scheduled to be modified by an
> in_flight_commit_batch_.
> 
> I think these calls to DeepCopy (actually all mutations to the backing store)
> should happen on the COMMIT_SEQUENCE.
> 
> Maybe we can add a make_deep_copy_first flag to CommitBatch and have
> CommitChanges() make the call to DeepCopy when needed.

Done. deep_copy_first doens't work, since the same commit batch can contain
changes that need to happen before the deep copy, and changes that need to
happen after it.

E.g., if we do: SetItem, fork a new namespace, SetItem, then the first SetItem
needs to happen before the deep copy, so that the value is available in the
forked namespace, and the second SetItem needs to happen after the deep copy.

I moved the deep copying to happen in the COMMIT_SEQUENCE, and separated
changes-before-deep-copy from changes-after-deep-copy.

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.cc:168: map_ = new
DomStorageMap(kPerAreaQuota);
On 2012/04/12 01:48:46, michaeln wrote:
> if this area is a 'shallow copy', we'll need to coin a new mapid for it in the
> session database at this point, no point in actually copying in this case, but
> we do need to break the association between this area and the old populated
map
> (if its shared with another area).
> 
> Flags for CommitChanges() could inform it of what do to...
> 
> if (commit_batch->make_deep_copy_first) {
>   if (commit_batch->clear_all_first)
>     // create a new mapid for the area w/o bothering to copy
>   else
>     DeepCopy();
> }

Done.

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.cc:184:
session_storage_backing_->ShallowCopy(namespace_id_, origin_,
On 2012/04/12 01:48:46, michaeln wrote:
> see earlier comments about serializing changes in the backing store, this
> probably needs to happen on the COMMIT_SEQUENCE too

Done.

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.cc:220:
session_storage_backing_->DeleteOrigin(namespace_id_, origin_);
On 2012/04/12 01:48:46, michaeln wrote:
> probably needs to be serialized on the COMMIT_SEQUENCE

Done.

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_context.cc (right):

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_context.cc:48:
session_storage_database_->DeleteLeftoverData();
On 2012/04/12 01:48:46, michaeln wrote:
> This constructor is invoked on the UI thread in chrome early on during profile
> creation. We definitely don't want to do a lot of IO at this early time. The
> DeleteLeftoverData method results in lots of IO.

Yes, this was wrong.

If we crash, DeleteSessionNamespace()s won't be called, and we have leftover
data. When should such data be deleted? On the next startup? Or on the next
clean exit? (Atm, I put it to Shutdown().)

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_context.cc:98: // FIXME(marja): Get usage infos
for sessionStorage.
On 2012/04/12 01:48:46, michaeln wrote:
> The content settings ui doesn't show anything about session storage data
> currently and the DeleteOrigin and DeleteDataModifiedSince methods only apply
to
> localstorage atm. Eventually blending in info about 'sessionstorage' usage is
a
> possibility.
> 
> Maybe open a bug for this instead of having a TODO buried in this class since
it
> also involves changes to the UI layer and programming interfaces used between
> here and there?

Done. crbug.com/123599

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_context.cc:217: namespaces_.erase(namespace_id);
On 2012/04/12 01:48:46, michaeln wrote:
> This might be a better place to trigger deletion from the
> session_storage_database than the namespace dtor? For two reasons...
> 
> 1) The namespace dtor will get invoked eventually for all of them when
> DomStorageContext is destructed. This will include namespaces that you
> presumably want to keep around in the database. As coded the namespace dtor
will
> remove them from the database too.
> 
> 2) There's no guarantee about what thread ~DomStorageContext (and consequently
> ~DomStorageNamespace) happens on. The DeleteSessionNamespace() and Shutdown()
> methods are very determistic about that (they're invoked on the
PRIMARY_SEQUENCE
> defined by the DomStorageTaskRunner).
> 
> In general, things in this library are written to not depend on destruction as
> much as explicit method calls for book keeping.

Done.

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_database.h (right):

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_database.h:32: virtual void ReadAllValues(int64
namespace_id,
On 2012/04/12 01:48:46, michaeln wrote:
> I think this existing class for localstorage backed by a per-origin sqlitedb
is
> fine as it is. The namespace_id and origin parameters are meaningless to this
> class. The problem is that this interface is not sufficient for session
storage,
> and not sufficient for localstorage that where multiple origins are stored in
> the same db.
> 
> Instead of forcing the existing class to have the multiplexing interface. Why
> dont we do something like this...
> 
> // Interface used by DomStorageArea
> class DomStorageDatabaseAdapter {
>   virtual void ReadAllValues(ValuesMap* result) = 0;
>   virtual bool CommitChanges(
>       bool clear_all_first, const ValuesMap& changes) = 0;
> };
> 
> // Impl that utilizes old school per-origin DomStorageDatabase
> class LocalStorageDatabaseAdapter {
>   LocalStorageDatabaseAdapter(DomStorageDatabase* db);
>   virtual void ReadAllValues(ValuesMap* result) = 0;
>   virtual bool CommitChanges(
>       bool clear_all_first, const ValuesMap& changes) = 0;
>  private:
>   scoped_ptr<DomStorageDatabase> db_;
> };
> 
> 
> // Impl that utilizes shiny new multiplexing SessionStorageDatabase
> class SessionStorageDatabaseAdapter {
>   SessionStorageDatabaseAdapter(SessionStorageDatabase* db,
>                                 int64 namespace_id,
>                                 GURL origin);
>   virtual void ReadAllValues(ValuesMap* result) = 0;
>   virtual bool CommitChanges(
>       bool clear_all_first, const ValuesMap& changes) = 0;
>  private:
>   scoped_refptr<SessionStorageDatabase> db_;
> };
> 
> Where the namespace creates the appropriate adapter. This might make unit
tests
> easier to compose (and faster to run) too since areas could be given a mock
> adapter to work with.
> 
> There are odd difference around 'DeleteOrigin' that could be abstracted out
into
> the adapter too.
> 
> It'd be great if the next change to DomStorageDatabase.cc or .h was an svn
> delete.

Done. Though, the DomStorageArea still has 2 pointers to adapters, one generic
one which can be either session storage or local storage, and one which is
always session storage. This is because it needs to do different operations
(shallow copy, the shortcut for "deep copy + clear", etc.) for session storage.
Should I also put some parts of this logic into the adapter?

I also added DeleteFiles() and Reset() to the adapter, to be able to do
DeleteOrigin and PurgeMemory. Atm only LocalStorageDatabaseAdapter implements
them.

I also tried out having the Namespace create the Adapter, but it's somewhat
messy. DomStorageArea still needs the raw SessionStorageDatabase so that it can
call the special functions, so, it would make sense to have it create the
Adapter around it, too. Wdyt?

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/session_...
File webkit/dom_storage/session_storage_database.cc (right):

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/session_...
webkit/dom_storage/session_storage_database.cc:30: // | next-map-id             
 | 4                   |
On 2012/04/12 01:48:46, michaeln wrote:
> Do we need to ensure that session namespace_ids in this database are unique
> across browser runs? As coded now, they start at 1 and go up from there on
every
> restart (see DomStorageContext), and those values are passed into this class.

Done. In the latest patch, I made it so that SessionStorageNamespace keeps an
offset which is stored in the database, and the offset is added to the
namespace_ids which get passed via the API. This way the namespaces created on
different sessions don't overlap. And, when namespaces from the last session are
referred to (not in this CL), the namespace ids of them will be negative
(avoiding kLocalStorageNamespaceId which is 0). I'd just need to carefully fetch
the real namespace ids from the data base when associating the namespace ids
with tabs in session restore... Does this sound ok?

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/session_...
webkit/dom_storage/session_storage_database.cc:35: std::string
AreaStartKey(const std::string& namespace_id) {
On 2012/04/12 01:48:46, michaeln wrote:
> Maybe NamespaceStartKey() for clarity?

Done.

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/session_...
webkit/dom_storage/session_storage_database.cc:39: std::string
AreaStartKey(int64 namespace_id) {
On 2012/04/12 01:48:46, michaeln wrote:
> ditto

Done.

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/session_...
webkit/dom_storage/session_storage_database.cc:52: std::string MapStartKey(const
std::string& map_id) {
On 2012/04/12 01:48:46, michaeln wrote:
> Maybe MapRefcountKey for clarity?

Done.

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/session_...
webkit/dom_storage/session_storage_database.cc:83: leveldb::Status s =
db_->Get(leveldb::ReadOptions(), area_key, &map_id);
On 2012/04/12 01:48:46, michaeln wrote:
> How should we handle genuine leveldb errors in this class, in the methods that
> write data out in particular? You have DCHECKs that assert no genuine errors
can
> happen right now... but they can.

Yes, I'll add proper error handling when I'm more confident that there are no
bugs. Atm, it seems that sometimes on of these DCHECKs fails, when doing a deep
copy, and I'm not sure why.

In the proper error handling, what should we do if we detect that the database
is in a corrupted state? We could just delete and recreate it, but to be able to
do deep and shallow copies, the things-to-copy would need to exist and I'm not
sure how it would work. Or clear the database and go to a "no more writes during
this session" mode? (We could also do that if there is a genuine db error.)

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/session_...
webkit/dom_storage/session_storage_database.cc:108: if (s.IsNotFound()) {
On 2012/04/12 01:48:46, michaeln wrote:
> style nit: dont need {}'s for one-liners

Done.

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/session_...
webkit/dom_storage/session_storage_database.cc:125: // |namespace_id| and its
ref count is increased.
On 2012/04/12 01:48:46, michaeln wrote:
> When we a Clone() a session namespace, we're going to do create a shallow copy
> for each origin in the src namespace. Should we arrange to have all of those
> mutations be performed in a single WriteBatch?
> 
> Maybe the method we really want on this class is CloneNamespace(srcid, dstid)?

Done. For this, I needed an uglyish hack for committing the data in the database
before the shallow copy is made. But it's unavoidable, I guess.

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/session_...
webkit/dom_storage/session_storage_database.cc:239: void
SessionStorageDatabase::DeleteLeftoverData() {
On 2012/04/12 01:48:46, michaeln wrote:
> As coded, this method appears to delete everything. I see this method being
> called in the ctor of the context class. So everything that was saved will
> promptly be deleted upon constructing the new context on restart. Can you
> describe what you have in mind here?

I was planning to do the "protect some origins from deletion to be able to do a
session restore" in a separate CL. So, the first CL would be the
SessionStorageDatabase and tests, the second CL would integrate it to
DomStorageArea, but never keep any origins, and the third CL would associate the
origins in the database with the session restore system, and protect them from
deletion. Does that sound ok?

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/session_...
webkit/dom_storage/session_storage_database.cc:287: // situation gracefully by
creating the database now.
On 2012/04/12 01:48:46, michaeln wrote:
> tzik recently added logic to handle this sort of situation with
> filesystemdirectory leveldb's in the webkit/fileapi world, please take a look
at
> how this has been handled there and consider if the handling here should be
> modified in anyway.

Do you mean the recovery options in FileSystemDirectoryDatabase::Init? They look
independent of this "we cannot from the outside detect if the db exists or not"
problem.

I'm not sure if this "Avoid creating the database if it doesn't exist" logic is
worth it in the session storage. If we only read from the db, we would avoid
creating and opening it, but for the (deep & shallow) copying to work, we'd need
to write empty placeholders more often... And how do we detect that "the
database exists", is it enough that there is at least one file in the
directory...

But, I added "if we fail to open the database, clear the directory and try
again" code here, it seems like a reasonable thing to do.

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/session_...
webkit/dom_storage/session_storage_database.cc:317: for
(it->Seek(map_start_key), it->Next(); it->Valid(); it->Next()) {
On 2012/04/12 01:48:46, michaeln wrote:
> is it valid to call Next() on an invalid iterator, idk one way or the other,
> just asking since it looks like Seek() could return an invalid 'it' initially

Done. (It's not ok to call Next() on an invalid iterator; restructured this.)

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/session_...
webkit/dom_storage/session_storage_database.cc:393: DeleteValuesInMap(map_id,
batch);
On 2012/04/12 01:48:46, michaeln wrote:
> This method name is a little decieving since it also may actually delete all
the
> values in the map. A better name might help?

Done. DeleteValuesInMap -> ClearMap.

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/session_...
webkit/dom_storage/session_storage_database.cc:401: ReadValuesInMap(map_id,
&values);
On 2012/04/12 01:48:46, michaeln wrote:
> Might be nice to read the keys only since that's all that's needed to delete.
> 
> But would be really nice to have a batch->RangeDelete("map-1", "map-1~")
> function in leveldb :)

Done. (Only reading the keys, not the RangeDelete.:)

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/session_...
webkit/dom_storage/session_storage_database.cc:422: // Skip the dummy entry,
then start iterating the origins in the area.
On 2012/04/12 01:48:46, michaeln wrote:
> The "origins in the area" is a confusing comment since areas are a per-origin
> thing. This is iterating the areas in a namespace.
> 
> Maybe the method should be NamespaceStartKey() instead?

Done. (Fixed the comment, AreaStartKey is now NamespaceStartKey.)

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/session_...
File webkit/dom_storage/session_storage_database.h (right):

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/session_...
webkit/dom_storage/session_storage_database.h:21: // SessionStorageDatabase.
On 2012/04/12 01:48:46, michaeln wrote:
> It'd be great if you carved this class out of the larger CL and put together
> some unittests for it.

I'll do that next...

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/session_...
webkit/dom_storage/session_storage_database.h:49: bool ShallowCopy(int64
namespace_id,
On 2012/04/12 01:48:46, michaeln wrote:
> See comment elsewhere about having CloneNamespace to do all areas in a single
> batch

Done.

[michaeln, 2012-04-22 19:51:05]

> Thanks for comments again,

Thanx for putting up with my comments :)

> Q: Where should the "origin -> local storage file name" conversion live? Now
> it's in DomStorageArea, but that doesn't seem right. DomStorageDatabase
doesn't
> seem right either.

I'm not too hung up on that honestly. It's not bad where they are but i'd agree
that someplace else could work better. Reasonable candidates...
DomStorageDatabase or LocalStorageDatabaseAdapter. Might be an improvement if
these halpers had the term 'Sql' in their names because if/when we get to
obsoleting the sql backed storage these two helpers we'd want to delete these
two helpers.

> Next I'll create a CL for just adding the SessionStorageDatabase and write
tests
> for it. Does the interface between SessionStorageDatabase and the other
classes
> look reasonably good?

sounds like a plan... i'll more closely at the snapshot and put comments inline.

[michaeln, 2012-04-22 21:43:34]

i haven't looked closely at the session_storage_database.cc in the latest
snapshot, but i took a look at the others. i'm wondering if the integration with
DomStorageArea could be simplified by 'flushing' pending changes at certain
points. the commitBatch stuff is looking complicated in the current snapshot.

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/session_...
File webkit/dom_storage/session_storage_database.cc (right):

http://codereview.chromium.org/9963107/diff/19001/webkit/dom_storage/session_...
webkit/dom_storage/session_storage_database.cc:30: // | next-map-id             
 | 4                   |
> Done. In the latest patch, I made it so that SessionStorageNamespace keeps an
> offset which is stored in the database, and the offset is added to the
> namespace_ids which get passed via the API. This way the namespaces created on
> different sessions don't overlap. And, when namespaces from the last session
are
> referred to (not in this CL), the namespace ids of them will be negative
> (avoiding kLocalStorageNamespaceId which is 0). I'd just need to carefully
fetch
> the real namespace ids from the data base when associating the namespace ids
> with tabs in session restore... Does this sound ok?

Sounds like a plausible approach.

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_area.cc (right):

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.cc:136: // anything.
i'd rather depend on HasOneRef() for this and nix the new data member

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.cc:148: // once.
It might be a simplification to flush pending changes prior to deep copying?
Could keep the commit batch structure simpler and the logic to set them up
simpler. I think we'd need a queue of in_flight_commit_batches_ instead of just
one.

bool DomStorageArea::MutatorOfSomeKind() {
  if (is_shutdown_)
    return false;
  InitialImportIfNeeded();

  bool needs_deep_copy = !map_->HasOneRef();
  if (needs_deep_copy ) {
    map_ = map_->DeepCopy();
    if (HasUncommittedChanges())
      OnCommitTimer();  // flushes pending changes
    DCHECK(!commit_batch_);
  }

  // mutate map_ as needed

  if ((success || needs_deep_copy) && backing_get()) {
    CommitBatch* commit_batch = CreateCommitBatchIfNeeded();
    commit_batch->needs_deep_copy = needs_deep_copy;
    if (success) {
      // mutate batch->changed_values as needed
    }
  }
}

wdyt?

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.cc:236: // the database. An empty commit
batch achieves this.
Does the empty placeholder really need to be there? Could this be conditional...

    if (HasUncommittedChanges())
      OnCommitTimer();  // flushes pending changes

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_area.h (right):

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.h:97: // and commit
changed_values_after_deep_copy.
see comments in the .cc file about simplifying this

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.h:132: bool is_shallow_copy_;
not sure the is_shallow_copy_ data member is good addition, when the other
breaks instance breaks the link, this data member isn't updated...
map_->HasOneRef() works in all cases and we end up relying on that even with
this data member

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_context.cc (right):

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_context.cc:164:
base::Bind(&DomStorageContext::DeleteLeftoverDataInCommitSequence, this));
it might be premature to have this here, feels like how/when to identify which
areas to retain and which to nix is not entirely worked out

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_context.cc:227: // deletion.
maybe the way the session handling logic in chrome keeps sessions alive across
restarts is by setting a 'restorable' attribute on those
SessionStorageNamespaces, and this lower level logic would avoid deletion of
them.

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_namespace.cc (right):

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_namespace.cc:46:
session_storage_database_->DeleteNamespace(namespace_id_);
should this be here given the logic in DomStorageContext?

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_namespace.cc:88: // the data.
not sure this elaborate comment is needed here
// Clone the in-memory structures.

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_namespace.cc:93: //
CloneNamespaceInCommitSequence will be done after writing data.
// And clone the on-disk structures too.

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_namespace.cc:94: bool success =
task_runner_->PostShutdownBlockingTask(
in mem-only cases (chromiumDRT and some tests) its valid for task_runner to be
null. in any cases where there is backing on disk, it cant be null.

i think you'll need an test here for if (session_db_) and only post the task to
clone in the db if non-null.

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_namespace.cc:97:
base::Bind(&DomStorageNamespace::CloneNamespaceInCommitSequence, this,
instead of binding to a helper method of this object, i think you could make a
closure directly around the SessionStorageDatabase method to avoid the
CloneNamespaceInCommitSequence helper in this class... one less helper method
would be nice

base::Bind(&SessionStorageDatabase::Clone, db_, orig_id, new_id)

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_namespace.h (right):

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_namespace.h:39: // on disk via
|session_storage_database|.
i see in the .cc file this ctor is being called with a NULL db param, maybe we
dont need the middle ctor at all... have one for session and another for local,
each with optionally empty or null 'backing' related param.

// Constructor for LocalStorage.
DomStorageNamespace(const FilePath& directory,  // may be empty
                    DomStorageTaskRunner* task_runner);

// Constructor for SessionStorage.
DomStorageNamespace(int64 namespace_id,
                    SessionStorageDatabase* db,  // may be NULL
                    DomStorageTaskRunner* task_runner);

[michaeln, 2012-04-22 22:50:00]

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/session_...
File webkit/dom_storage/session_storage_database.cc (right):

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/session_...
webkit/dom_storage/session_storage_database.cc:61: return
std::string("namespace-");
nit: indent

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/session_...
webkit/dom_storage/session_storage_database.cc:281: s =
db_->Get(leveldb::ReadOptions(), namespace_key, &old_map_id);
why is it ok to not update the value is namespace_key such that it doesn't refer
to old_map_id anymore? seems like this writebatch would leave the db in an
inconsistent state with more namespaces associated to a map than refcounts on
that map

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/session_...
webkit/dom_storage/session_storage_database.cc:319:
scoped_ptr<leveldb::Iterator> it(db_->NewIterator(leveldb::ReadOptions()));
should we start the iteration at the 'namespace-' entries and stop when we run
off the end

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/session_...
webkit/dom_storage/session_storage_database.cc:553: DecreaseRefCount(map_id,
batch);
not sure this will do what you want it to do, a leveldb WriteBatch is not a
transaction, mutations made in a write batch will not be reflected in db->Get()
calls until after the batch is committed.

i think this refcounting logic wont function as coded because if two origins
referred to the same map at the start of the 'batch'... each time thru this
method the value of '2' will be seen... and at commit time it will be '1'...
whereas it should have dropped to zero and deleted all the values in that map

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/session_...
File webkit/dom_storage/session_storage_database.h (right):

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/session_...
webkit/dom_storage/session_storage_database.h:64: void DeleteOrigin(int64
namespace_id, const GURL& origin);
Might be useful to have this method invoked as a consequence of
DomStorageArea::DeleteOrigin() being called?

[michaeln, 2012-04-22 23:48:02]

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_area.cc (right):

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.cc:148: // once.
> wdyt?

or maybe a pattern like this that factors out more of the boiler plate

bool DomStorageArea::MutatorOfSomeKind() {
  if (is_shutdown_)
    return false;
  InitialImportIfNeeded();
  DeepCopyIfNeeded();

  // mutate map_ as needed, code stays the same as before

  if (success && backing_get()) {
    CommitBatch* commit_batch = CreateCommitBatchIfNeeded();
    // code stays the same as before
  }
}

void DomStorageArea::DeepCopyIfNeeded() {
  if (!map_->HasOneRef()) {
    map_ = map_->DeepCopy();
    if (HasUncommittedChanges())
      OnCommitTimer();  // flushes pending changes
    DCHECK(!commit_batch_);
    CommitBatch* commit_batch = CreateCommitBatchIfNeeded();
    commit_batch->needs_deep_copy = true;
  }
}

[marja, 2012-04-23 14:38:25]

Hi & thanks for comments,

I created http://codereview.chromium.org/10176005/ which contains only the
SessionStorageDatabase + tests.

The comments to session_storage_database.{cc,h} are addressed there.

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/session_...
File webkit/dom_storage/session_storage_database.cc (right):

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/session_...
webkit/dom_storage/session_storage_database.cc:61: return
std::string("namespace-");
On 2012/04/22 22:50:01, michaeln wrote:
> nit: indent

Done (in codereview.chromium.org/10176005 ).

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/session_...
webkit/dom_storage/session_storage_database.cc:319:
scoped_ptr<leveldb::Iterator> it(db_->NewIterator(leveldb::ReadOptions()));
On 2012/04/22 22:50:01, michaeln wrote:
> should we start the iteration at the 'namespace-' entries and stop when we run
> off the end

Done (in codereview.chromium.org/10176005 ).

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/session_...
webkit/dom_storage/session_storage_database.cc:553: DecreaseRefCount(map_id,
batch);
On 2012/04/22 22:50:01, michaeln wrote:
> not sure this will do what you want it to do, a leveldb WriteBatch is not a
> transaction, mutations made in a write batch will not be reflected in
db->Get()
> calls until after the batch is committed.
> 
> i think this refcounting logic wont function as coded because if two origins
> referred to the same map at the start of the 'batch'... each time thru this
> method the value of '2' will be seen... and at commit time it will be '1'...
> whereas it should have dropped to zero and deleted all the values in that map

Ahh, I see, thanks for catching this. Things go wrong when this function is
called indirectly by DeleteLeftoverData, and the db contains a shallow copy.

Done in codereview.chromium.org/10176005 & added a test for this.

[marja, 2012-04-23 15:11:36]

Oops, forgot this one:

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/session_...
File webkit/dom_storage/session_storage_database.cc (right):

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/session_...
webkit/dom_storage/session_storage_database.cc:281: s =
db_->Get(leveldb::ReadOptions(), namespace_key, &old_map_id);
On 2012/04/22 22:50:01, michaeln wrote:
> why is it ok to not update the value is namespace_key such that it doesn't
refer
> to old_map_id anymore? seems like this writebatch would leave the db in an
> inconsistent state with more namespaces associated to a map than refcounts on
> that map

This was wrong, fixed in codereview.chromium.org/10176005/ .

[marja, 2012-05-11 12:18:32]

Hi again, I updated this CL to integrate the SessionStorageNamespace into
DomStorage(Area|Context|Namespace).

Should I put the on-disk backing of sessionStorage behind the
--enable-restore-session-state command line flag (which I'd need to revive), or
only the "associate namespace id's with tabs and store/restore them"
functionality?

Should I split off some parts of this CL, e.g., the DomStorageDatabaseAdaptor
indirection?

(I still haven't updated the tests, I'll do that when we agree that the approach
looks ok.)

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_area.cc (right):

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.cc:136: // anything.
On 2012/04/22 21:43:35, michaeln wrote:
> i'd rather depend on HasOneRef() for this and nix the new data member

Done.

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.cc:148: // once.
On 2012/04/22 21:43:35, michaeln wrote:
> It might be a simplification to flush pending changes prior to deep copying?
> Could keep the commit batch structure simpler and the logic to set them up
> simpler. I think we'd need a queue of in_flight_commit_batches_ instead of
just
> one.
> 
> bool DomStorageArea::MutatorOfSomeKind() {
>   if (is_shutdown_)
>     return false;
>   InitialImportIfNeeded();
> 
>   bool needs_deep_copy = !map_->HasOneRef();
>   if (needs_deep_copy ) {
>     map_ = map_->DeepCopy();
>     if (HasUncommittedChanges())
>       OnCommitTimer();  // flushes pending changes
>     DCHECK(!commit_batch_);
>   }
> 
>   // mutate map_ as needed
> 
>   if ((success || needs_deep_copy) && backing_get()) {
>     CommitBatch* commit_batch = CreateCommitBatchIfNeeded();
>     commit_batch->needs_deep_copy = needs_deep_copy;
>     if (success) {
>       // mutate batch->changed_values as needed
>     }
>   }
> }
> 
> wdyt?

- The deep copy problem was solved by http://codereview.chromium.org/10378066/
- The "multiple commits in flight at the same time" possibility was added by
http://codereview.chromium.org/10389061/
- And I modified this CL to queue the things in the right order: When
ShallowCopy() is called, we queue committing whatever is in commit_batch_, then
the upper layer (Namespace) schedules making the clone, and if changes happen
after these, they'll be committed after the clone.

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.cc:236: // the database. An empty commit
batch achieves this.
On 2012/04/22 21:43:35, michaeln wrote:
> Does the empty placeholder really need to be there? Could this be
conditional...
> 
>     if (HasUncommittedChanges())
>       OnCommitTimer();  // flushes pending changes

Done. Empty placeholder is not needed any more.

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_area.h (right):

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.h:97: // and commit
changed_values_after_deep_copy.
On 2012/04/22 21:43:35, michaeln wrote:
> see comments in the .cc file about simplifying this

Done.

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.h:132: bool is_shallow_copy_;
On 2012/04/22 21:43:35, michaeln wrote:
> not sure the is_shallow_copy_ data member is good addition, when the other
> breaks instance breaks the link, this data member isn't updated...
> map_->HasOneRef() works in all cases and we end up relying on that even with
> this data member

Gone.

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_context.cc (right):

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_context.cc:164:
base::Bind(&DomStorageContext::DeleteLeftoverDataInCommitSequence, this));
On 2012/04/22 21:43:35, michaeln wrote:
> it might be premature to have this here, feels like how/when to identify which
> areas to retain and which to nix is not entirely worked out

Should I make it so that this CL just deletes all data on clean exit (as it's
not going to associate the data back to the Namespaces after restoring the
session)?

How I think this could work:
- Normally, DeleteSessionNamespace is called for each namespace on exit.
- At that time, we know if we're going to restore the session or not.
- If we're not going to restore, then just delete the data as it is now.
- Also, we need some way to delete the data after a crash: there will be data in
the database but no DomStorageSession or SessionStorageNamespaceImpl classes
associated with it. When should we delete such data? On a clean exit?
- If we're going to restore, the session restore system would tell
DomStorageContext a list of namespace IDs that should be protected from
deletion. DeleteSessionNamespace wouldn't tell delete those. (This functionality
would be in a separate CL.)

So, in this CL I would do this:
- DeleteSessionNamespace always deletes the data.
- We also (somehow) delete the data which is left over after a crash.

In the next CL, I would do this:
- On exit, when we're going to restore, protect parts of the data from deletion.
I'd imagine this works so that the session restore system keeps track of what
namespace id's are associated with the tabs it knows about, and on exit, gives
that list to DomStorageContext.
- When restoring a session, associate the data in the database with the restored
tabs.
- We don't always restore the session after a crash, so that leftover data still
needs to be deleted somewhere.

Wdyt?

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_context.cc:227: // deletion.
On 2012/04/22 21:43:35, michaeln wrote:
> maybe the way the session handling logic in chrome keeps sessions alive across
> restarts is by setting a 'restorable' attribute on those
> SessionStorageNamespaces, and this lower level logic would avoid deletion of
> them.

Hmm, not sure yet how this would work.

Could the session restore system tell DomStorageContext a list of namespace ids
not to delete, and they would then be protected from deletion in this function?
(The "which namespace ids are in the session restore data" would be somewhat
easy to maintain. I'm atm slightly confused about which tabs would be restorable
(by the session restore, only the open ones, but by the tab restorer, some
others, and how could SessionStorageNamespaceImpl get to know that...)

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_namespace.cc (right):

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_namespace.cc:46:
session_storage_database_->DeleteNamespace(namespace_id_);
On 2012/04/22 21:43:35, michaeln wrote:
> should this be here given the logic in DomStorageContext?

Done.

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_namespace.cc:88: // the data.
On 2012/04/22 21:43:35, michaeln wrote:
> not sure this elaborate comment is needed here
> // Clone the in-memory structures.

Done. DomStorageArea::ShallowCopy will also put the pending changes to the task
queue so they'll be written on disk... but maybe this layer doesn't need to
care.

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_namespace.cc:93: //
CloneNamespaceInCommitSequence will be done after writing data.
On 2012/04/22 21:43:35, michaeln wrote:
> // And clone the on-disk structures too.

Done.

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_namespace.cc:94: bool success =
task_runner_->PostShutdownBlockingTask(
On 2012/04/22 21:43:35, michaeln wrote:
> in mem-only cases (chromiumDRT and some tests) its valid for task_runner to be
> null. in any cases where there is backing on disk, it cant be null.
> 
> i think you'll need an test here for if (session_db_) and only post the task
to
> clone in the db if non-null.

Done.

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_namespace.cc:97:
base::Bind(&DomStorageNamespace::CloneNamespaceInCommitSequence, this,
On 2012/04/22 21:43:35, michaeln wrote:
> instead of binding to a helper method of this object, i think you could make a
> closure directly around the SessionStorageDatabase method to avoid the
> CloneNamespaceInCommitSequence helper in this class... one less helper method
> would be nice
> 
> base::Bind(&SessionStorageDatabase::Clone, db_, orig_id, new_id)

Done. For that to work, I needed to change the return value of
SessionStorageDatabase::CloneNamespace from bool to void, so that it can be
ignored here. (Not sure if there was a better way to do it, so that I could keep
the bool value and e.g., check it in tests.

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_namespace.h (right):

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_namespace.h:39: // on disk via
|session_storage_database|.
On 2012/04/22 21:43:35, michaeln wrote:
> i see in the .cc file this ctor is being called with a NULL db param, maybe we
> dont need the middle ctor at all... have one for session and another for
local,
> each with optionally empty or null 'backing' related param.
> 
> // Constructor for LocalStorage.
> DomStorageNamespace(const FilePath& directory,  // may be empty
>                     DomStorageTaskRunner* task_runner);
> 
> // Constructor for SessionStorage.
> DomStorageNamespace(int64 namespace_id,
>                     SessionStorageDatabase* db,  // may be NULL
>                     DomStorageTaskRunner* task_runner);

Done.

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/session_...
File webkit/dom_storage/session_storage_database.h (right):

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/session_...
webkit/dom_storage/session_storage_database.h:64: void DeleteOrigin(int64
namespace_id, const GURL& origin);
On 2012/04/22 22:50:01, michaeln wrote:
> Might be useful to have this method invoked as a consequence of
> DomStorageArea::DeleteOrigin() being called?

Done.

[michaeln, 2012-05-16 08:10:55]

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_area.cc (right):

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.cc:148: // once.
> - The deep copy problem was solved by http://codereview.chromium.org/10378066/
> - The "multiple commits in flight at the same time" possibility was added by
> http://codereview.chromium.org/10389061/
> - And I modified this CL to queue the things in the right order: When
> ShallowCopy() is called, we queue committing whatever is in commit_batch_,
then
> the upper layer (Namespace) schedules making the clone, and if changes happen
> after these, they'll be committed after the clone.

sgtm!

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_context.cc (right):

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_context.cc:164:
base::Bind(&DomStorageContext::DeleteLeftoverDataInCommitSequence, this));
On 2012/05/11 12:18:32, marja wrote:
> On 2012/04/22 21:43:35, michaeln wrote:
> > it might be premature to have this here, feels like how/when to identify
which
> > areas to retain and which to nix is not entirely worked out
> 
> Should I make it so that this CL just deletes all data on clean exit (as it's
> not going to associate the data back to the Namespaces after restoring the
> session)?

Maybe don't try to answer this question just yet, and instead make progress on
parts we are certain about (DomStorageArea + session db backing). As coded this
method will delete everything provided there's a clean shutdown (so what why
bother having written it to disk). Let's have chromium built from trunk not
write to disk until its ready to restore from disk.

> 
> How I think this could work:
> - Normally, DeleteSessionNamespace is called for each namespace on exit.
> - At that time, we know if we're going to restore the session or not.
> - If we're not going to restore, then just delete the data as it is now.
> - Also, we need some way to delete the data after a crash: there will be data
in
> the database but no DomStorageSession or SessionStorageNamespaceImpl classes
> associated with it. When should we delete such data? On a clean exit?

about when to delete dead stuff, incrementally while up and running is probably
the best answer, at startup isn't an option at all, at shutdown doesn't happen
sometimes and when it does happen a million other things happen too (so it takes
quite a while already)

> - If we're going to restore, the session restore system would tell
> DomStorageContext a list of namespace IDs that should be protected from
> deletion. DeleteSessionNamespace wouldn't tell delete those. (This
functionality
> would be in a separate CL.)
> 
> So, in this CL I would do this:
> - DeleteSessionNamespace always deletes the data.
> - We also (somehow) delete the data which is left over after a crash.
> 
> In the next CL, I would do this:
> - On exit, when we're going to restore, protect parts of the data from
deletion.
> I'd imagine this works so that the session restore system keeps track of what
> namespace id's are associated with the tabs it knows about, and on exit, gives
> that list to DomStorageContext.

another way to think about it maybe... DomStorageContext assumes by default that
all new and existing session are to be kept forever... until session restore
says otherwise, DomStorageContext then remembers which its been told can go away
will eventually make those go away.

So as session restore figures out something is no longer needed, it call
DSC.RevokeSessionStorage(id) or some such method. And when doing 'restore', the
session restore can query DSC for a list of unrevoked session ids and call
revoke on those not needing to be restored.

It's probably worth taking a stab at that session restore logic prior to coming
to any final decisions about this aspect of things.

> - When restoring a session, associate the data in the database with the
restored
> tabs.
> - We don't always restore the session after a crash, so that leftover data
still
> needs to be deleted somewhere.
> 
> Wdyt?

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_namespace.cc (right):

http://codereview.chromium.org/9963107/diff/48001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_namespace.cc:97:
base::Bind(&DomStorageNamespace::CloneNamespaceInCommitSequence, this,
> Done. For that to work, I needed to change the return value of
> SessionStorageDatabase::CloneNamespace from bool to void, so that it can be
> ignored here. (Not sure if there was a better way to do it, so that I could
keep
> the bool value and e.g., check it in tests.

The base::IgnoreResult template could probably work for that.

http://codereview.chromium.org/9963107/diff/64033/content/browser/dom_storage...
File content/browser/dom_storage/dom_storage_context_impl.cc (right):

http://codereview.chromium.org/9963107/diff/64033/content/browser/dom_storage...
content/browser/dom_storage/dom_storage_context_impl.cc:91: data_path :
data_path.AppendASCII(kSessionStorageDirectory),
until we work out how the higher level session restore logic interfaces with
this lower level logic, and it's ready to utilize that interface, it'd probably
be good to keep this as an empty path... we can turn this on when things are
ready from end to end.

http://codereview.chromium.org/9963107/diff/64033/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_area.cc (right):

http://codereview.chromium.org/9963107/diff/64033/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.cc:183: // All the uncommitted changes to
this area need to happen before the actual
really nice simplifications to this file compared to prior snapshots!

http://codereview.chromium.org/9963107/diff/64033/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.cc:214: } else if
(session_storage_backing_.get()) {
should is_initial_import_done_ = true be set here?

http://codereview.chromium.org/9963107/diff/64033/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_namespace.cc (right):

http://codereview.chromium.org/9963107/diff/64033/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_namespace.cc:98: } else if
(session_storage_database_.get()) {
i think the DomStorageNamespace::DeleteOrigin method is unreachable for session
storage namespaces right now. also i think there's a race condition with this
handling of that function for session storage.

suppose immediately after this block executes, there's a call to
DomStorageNamespace::OpenStorageArea for this 'origin' followed by something
that causes the area to be read from the database. those calls to open+read on
the primary sequence will be racing with deletion scheduled on the commit
sequence by the area->DeleteOrigin() method call below.

since this is currently unreachable (its only reached via the
DomStorageContext::DeleteOrigin method which only calls thru for the local
storage namespace)... consider putting a
DCHECK(!session_storage_database_.get()) here for now

[marja, 2012-05-25 13:04:06]

Hi michaeln,

I uploaded a new version which drafts how restoring the namespaces would work.

The current state only deals with storing the namespace ID along the session
data and associating it back when restoring the session. The "delete stale data"
and "protect data from deletion" features are still unimplemented, but the
namespace ID association part should work.

The diff between patch sets 29 and 30 is the interesting part, and the new code
is in these files:
session_restore.cc
session_service.{cc,h}
session_types.{cc,h}

For transmitting the association back and forth:
dom_storage_context_impl.{cc,h}
content/public/browser/dom_storage_context.h
notification_types.h
session_storage_namespace.h
dom_storage_context.{cc,h}
dom_storage_session.{cc,h}

Do these parts look ok?

I realized the simplistic mapping of "real namespace id in the db = upper layer
namespace id + offset" doesn't work. The real id gets saved to the session
information, but it's tricky to figure out what the upper layer namespace id
should be when restoring the session. At that point, the session storage db is
not yet open, so we don't know what the offset will be...

So, I changed it so that the SessionStorageDatabase maintains a mapping between
upper layer namespace ids and the real namespace ids. The session restore system
can then tell it to add an entry to this mapping, when restoring a tab.

This didn't change anything in DomStorageArea. DomStorageNamespace &
DomStorageContext need to change, since they need to protect some namespaces
from deletion.

I could put the ID handling changes of SessionStorageDatabase into a separate
CL, wdyt?

Next I'll try to come up with a solution for telling DomStorageContext which
namespaces can be deleted and which must be saved for the session restore... And
I didn't yet do all your comments.

http://codereview.chromium.org/9963107/diff/64033/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_area.cc (right):

http://codereview.chromium.org/9963107/diff/64033/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.cc:214: } else if
(session_storage_backing_.get()) {
On 2012/05/16 08:10:55, michaeln wrote:
> should is_initial_import_done_ = true be set here?

Do you mean is_initial_import_done_ = false, like above?

[michaeln, 2012-05-30 01:29:18]

Thanx for looking into the session restore layer of things on this and pointing
where the relevant sources are in the CL.

> I realized the simplistic mapping of "real namespace id in the db = upper
layer
> namespace id + offset" doesn't work.

Ok, i was wondering about how this part was going to work out. Sounds like there
are some kinks in that. The 'real' vs other integer id mappings and translations
are somewhat confusing and the big racey notification loop from session storage
db back up to session restore stuff in chrome seems awkward.

I'm wondering if we should coin a unique persistent guid from the git go and use
that value in the db and in the 'content' api provided to session restore
instead of trying to get the sequence ids to work out for this?

https://chromiumcodereview.appspot.com/9963107/diff/64033/webkit/dom_storage/...
File webkit/dom_storage/dom_storage_area.cc (right):

https://chromiumcodereview.appspot.com/9963107/diff/64033/webkit/dom_storage/...
webkit/dom_storage/dom_storage_area.cc:214: } else if
(session_storage_backing_.get()) {
On 2012/05/25 13:04:06, marja wrote:
> On 2012/05/16 08:10:55, michaeln wrote:
> > should is_initial_import_done_ = true be set here?
> 
> Do you mean is_initial_import_done_ = false, like above?

I actually meant 'true'. Suppose RemoveItem() is called prior to the DeleteArea
tasks having been run on the commit sequence. If that flag is 'false', we'll
read from the db values that have been logically deleted.

https://chromiumcodereview.appspot.com/9963107/diff/82002/chrome/browser/sess...
File chrome/browser/sessions/session_restore.cc (right):

https://chromiumcodereview.appspot.com/9963107/diff/82002/chrome/browser/sess...
chrome/browser/sessions/session_restore.cc:879:
browser->AddRestoredTab(tab.navigations,
The last argument to the AddRestoredTab method is a
content::SessionStorageNamespace ptr. Could that be setup to refer to the
restored namespace from the git go?

It looks like the AddRestoredTab() and ResplaceRestoredTab() are used in
situations other than 'full session restore', which I imagine is to support
reopening rececently closed tabs via the popup menu on the bottom right of the
NTP. In that case, looks like the a reference to namespace being held by the
'Tab' struct is passed as a parameter to the restore methods.

When restoring a tab via full session restore, do we end up with a new or
modified 'Tab' struct? And does that struct end up referring to the correct
SessionStorageNamespace?

https://chromiumcodereview.appspot.com/9963107/diff/82002/chrome/browser/sess...
File chrome/browser/sessions/session_service.cc (right):

https://chromiumcodereview.appspot.com/9963107/diff/82002/chrome/browser/sess...
chrome/browser/sessions/session_service.cc:21: #include
"content/browser/dom_storage/session_storage_namespace_impl.h"
we shouldn't be using interfaces that aren't in the content/public api here

https://chromiumcodereview.appspot.com/9963107/diff/82002/content/public/brow...
File content/public/browser/session_storage_namespace.h (right):

https://chromiumcodereview.appspot.com/9963107/diff/82002/content/public/brow...
content/public/browser/session_storage_namespace.h:17: class
SessionStorageNamespace
What if there were two different methods to manufacture an instance of this
class. One for use when creating completely new empty sessions and another for
use when re-animating a previously saved session given its persistent_id?

Would it make things more clear if this class had an id() and a persistent_id()
accessor?

[marja, 2012-05-30 11:46:18]

Marking some old comments done, thanks for new comments!

Using GUIDs instead of int IDs for sessionStorage sounds good; it will solve a
lot of problems. I'll create another CL for this change only.

http://codereview.chromium.org/9963107/diff/64033/content/browser/dom_storage...
File content/browser/dom_storage/dom_storage_context_impl.cc (right):

http://codereview.chromium.org/9963107/diff/64033/content/browser/dom_storage...
content/browser/dom_storage/dom_storage_context_impl.cc:91: data_path :
data_path.AppendASCII(kSessionStorageDirectory),
On 2012/05/16 08:10:55, michaeln wrote:
> until we work out how the higher level session restore logic interfaces with
> this lower level logic, and it's ready to utilize that interface, it'd
probably
> be good to keep this as an empty path... we can turn this on when things are
> ready from end to end.

I would have liked to put this "save session state" behind a command line flag,
but the information about command line flags seems to be so far away here...

I added BrowserContext::ShouldSaveSessionStorageOnDisk here, and the version
overwritten by ProfileImpl reads the command line flag. Wdyt?

http://codereview.chromium.org/9963107/diff/64033/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_area.cc (right):

http://codereview.chromium.org/9963107/diff/64033/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.cc:214: } else if
(session_storage_backing_.get()) {
On 2012/05/25 13:04:06, marja wrote:
> On 2012/05/16 08:10:55, michaeln wrote:
> > should is_initial_import_done_ = true be set here?
> 
> Do you mean is_initial_import_done_ = false, like above?

Done. I was confused, = true makes sense. But why does the above have = false?
The db is empty, so there is nothing to import..

http://codereview.chromium.org/9963107/diff/64033/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_namespace.cc (right):

http://codereview.chromium.org/9963107/diff/64033/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_namespace.cc:98: } else if
(session_storage_database_.get()) {
On 2012/05/16 08:10:55, michaeln wrote:
> i think the DomStorageNamespace::DeleteOrigin method is unreachable for
session
> storage namespaces right now. also i think there's a race condition with this
> handling of that function for session storage.
> 
> suppose immediately after this block executes, there's a call to
> DomStorageNamespace::OpenStorageArea for this 'origin' followed by something
> that causes the area to be read from the database. those calls to open+read on
> the primary sequence will be racing with deletion scheduled on the commit
> sequence by the area->DeleteOrigin() method call below.
> 
> since this is currently unreachable (its only reached via the
> DomStorageContext::DeleteOrigin method which only calls thru for the local
> storage namespace)... consider putting a
> DCHECK(!session_storage_database_.get()) here for now

Done.

Ah, yes, there was a race. What if I set is_initial_import_done to true in
DomStorageArea::DeleteOrigin, when it posts the actual deletion task... That
should remove the race, no?

But, anyhow, I don't have a clear idea on how this origin-based deletion should
work.. Should SessionStorageDatabase have a function to delete an origin from
all namespaces?

(I guess it's plugged into this "modification times for origins" logic, so
should it instead have a function to delete all origins which have been modified
after a certain time?)

[marja, 2012-05-31 14:41:33]

Thanks for comments again!


I uploaded an updated version which creates GUIDs along with the integer
identifiers (which were not unique across) sessions. First I thought it could
work to have only one ID and that would be the GUID, but the integer IDs are
used in so many places that changing tha would be tedious... (so, I just added
the GUID to be used along the integer ID).

Lots of details are still unfinalized:
- RenderMessageFilter::OnMsgCreateWindow goes on and clones the session storage
(DOMStorageContextImpl::LeakyCloneSessionStorage) without creating a
SessionStorageNamespace; idk how to deal with that. There is a TODO to fix that,
I think this CL won't fully work until it's fixed. (I could fix it in a separate
CL.) (*)
- The leftover data (data in the db which doesn't have an associated
DomStorageNamespace) is now deleted at shutdown.
- I'm not perfectly sure if the "sessionStorage can be deleted now" logic is
correct. Now SessionService marks the sessionStorage as deletable whenever a tab
is closed...
- The guid generation code is in chrome/common, now webkit/dom_storage is just
violating the include rules by including it...
- The threading is not yet correct, some funcs are called on the wrong thread.

(*) At that point, there is a SessionStorageNamespace created for the tab,
should that code just go and create a new one and replace the existing one with
it? Or is there something more complicated involved?

http://codereview.chromium.org/9963107/diff/82002/chrome/browser/sessions/ses...
File chrome/browser/sessions/session_restore.cc (right):

http://codereview.chromium.org/9963107/diff/82002/chrome/browser/sessions/ses...
chrome/browser/sessions/session_restore.cc:879:
browser->AddRestoredTab(tab.navigations,
On 2012/05/30 01:29:19, michaeln wrote:
> The last argument to the AddRestoredTab method is a
> content::SessionStorageNamespace ptr. Could that be setup to refer to the
> restored namespace from the git go?
> 
> It looks like the AddRestoredTab() and ResplaceRestoredTab() are used in
> situations other than 'full session restore', which I imagine is to support
> reopening rececently closed tabs via the popup menu on the bottom right of the
> NTP. In that case, looks like the a reference to namespace being held by the
> 'Tab' struct is passed as a parameter to the restore methods.
> 
> When restoring a tab via full session restore, do we end up with a new or
> modified 'Tab' struct? And does that struct end up referring to the correct
> SessionStorageNamespace?

1. Constructed a SessionStorageNamespace here and used it as the last parameter.

2. Yes.

3. If done like you suggested, the SessionService will write the correct
association between the new tab and the sessionStorage into its data when it
receives the "tab parented" notification. Not sure if this was what you meant?
New or modified Tab struct?

http://codereview.chromium.org/9963107/diff/82002/chrome/browser/sessions/ses...
File chrome/browser/sessions/session_service.cc (right):

http://codereview.chromium.org/9963107/diff/82002/chrome/browser/sessions/ses...
chrome/browser/sessions/session_service.cc:21: #include
"content/browser/dom_storage/session_storage_namespace_impl.h"
On 2012/05/30 01:29:19, michaeln wrote:
> we shouldn't be using interfaces that aren't in the content/public api here

Removed this, but now session_restore needs to include it so that it's able to
create a SessionStorageNamespaceImpl.

http://codereview.chromium.org/9963107/diff/82002/content/public/browser/sess...
File content/public/browser/session_storage_namespace.h (right):

http://codereview.chromium.org/9963107/diff/82002/content/public/browser/sess...
content/public/browser/session_storage_namespace.h:17: class
SessionStorageNamespace
On 2012/05/30 01:29:19, michaeln wrote:
> What if there were two different methods to manufacture an instance of this
> class. One for use when creating completely new empty sessions and another for
> use when re-animating a previously saved session given its persistent_id?
> 
> Would it make things more clear if this class had an id() and a
persistent_id()
> accessor?

Done.

[marja, 2012-05-31 17:13:11]

I had a closer look on the LeakyCloneSessionStorage fix, it could work if I..
- Transmit the cloned_session_storage_namespace_id as a param to CreateNewWindow
all the way to WebContentsImpl
- Add a ctor to SessionStorageNamespace which takes an existing ID (and in this
CL, also the persistent ID)
- WebContentsImpl::CreateNewWindow then constructs the SessionStorageNamespace
based on the ID, and passes it to WebContentsImpl ctor, instead of the NULL it
passes now.

Does this sound ok?

[michaeln, 2012-06-01 03:21:23]

> I had a closer look on the LeakyCloneSessionStorage fix...

I think it would be great if you came up with a fix for that bug. I haven't
looked that too closely, mostly just noticed the leak. One thing i wonder about
is raciness, if after the clone is made, the target process is terminated prior
to the new window claiming ownership of it? If something like that can happen
and we end up cloning w/o creating a SessionStorageNamespace instance with an
owner, then we'll still have a leak (albeit with a smaller window of
opportunity).

[michaeln, 2012-06-01 07:41:51]

The patch is quite large. I'll look more closely and make more detailed comments
tomorrow, but this is looking pretty good to me.

> I uploaded an updated version which creates GUIDs along with the integer
> identifiers (which were not unique across) sessions. First I thought it could
> work to have only one ID and that would be the GUID, but the integer IDs are
> used in so many places that changing tha would be tedious... (so, I just added
> the GUID to be used along the integer ID).

Might be nice to consistently refer to one as id and the other as persistent_id
throughout.

> 
> Lots of details are still unfinalized:
> - RenderMessageFilter::OnMsgCreateWindow goes on and clones the session
storage
> (DOMStorageContextImpl::LeakyCloneSessionStorage) without creating a
> SessionStorageNamespace; idk how to deal with that. There is a TODO to fix
that,
> I think this CL won't fully work until it's fixed. (I could fix it in a
separate
> CL.) (*)
> - The leftover data (data in the db which doesn't have an associated
> DomStorageNamespace) is now deleted at shutdown.
> - I'm not perfectly sure if the "sessionStorage can be deleted now" logic is
> correct. Now SessionService marks the sessionStorage as deletable whenever a
tab
> is closed...

The DSC method name looks like a getter more than a mutator. We use the verb
'Doom' in other places of a similar nature, maybe
DoomSessionStorageNamespace(persistent_namespace_id).

> - The guid generation code is in chrome/common, now webkit/dom_storage is just
> violating the include rules by including it...

Right, i'm not sure what the story is with that function not being in the base
library? If need be we can roll our own 128 bits worth of random value
generation in the dom_storage library using base random stuff (or shudder, call
out to webcores guid generator). It would probably be good if the string values
we use do not contain '-' characters for this values since those are used to
delineate key prefixes and such.

> - The threading is not yet correct, some funcs are called on the wrong thread.
> 
> (*) At that point, there is a SessionStorageNamespace created for the tab,
> should that code just go and create a new one and replace the existing one
with
> it? Or is there something more complicated involved?
> 
>
http://codereview.chromium.org/9963107/diff/82002/chrome/browser/sessions/ses...
> File chrome/browser/sessions/session_restore.cc (right):
> 
>
http://codereview.chromium.org/9963107/diff/82002/chrome/browser/sessions/ses...
> chrome/browser/sessions/session_restore.cc:879:
> browser->AddRestoredTab(tab.navigations,
> On 2012/05/30 01:29:19, michaeln wrote:
> > The last argument to the AddRestoredTab method is a
> > content::SessionStorageNamespace ptr. Could that be setup to refer to the
> > restored namespace from the git go?
> > 
> > It looks like the AddRestoredTab() and ResplaceRestoredTab() are used in
> > situations other than 'full session restore', which I imagine is to support
> > reopening rececently closed tabs via the popup menu on the bottom right of
the
> > NTP. In that case, looks like the a reference to namespace being held by the
> > 'Tab' struct is passed as a parameter to the restore methods.
> > 
> > When restoring a tab via full session restore, do we end up with a new or
> > modified 'Tab' struct? And does that struct end up referring to the correct
> > SessionStorageNamespace?
> 
> 1. Constructed a SessionStorageNamespace here and used it as the last
parameter.

I'm glad that works out.

> 
> 2. Yes.
> 
> 3. If done like you suggested, the SessionService will write the correct
> association between the new tab and the sessionStorage into its data when it
> receives the "tab parented" notification. Not sure if this was what you meant?
> New or modified Tab struct?
> 
>
http://codereview.chromium.org/9963107/diff/82002/chrome/browser/sessions/ses...
> File chrome/browser/sessions/session_service.cc (right):
> 
>
http://codereview.chromium.org/9963107/diff/82002/chrome/browser/sessions/ses...
> chrome/browser/sessions/session_service.cc:21: #include
> "content/browser/dom_storage/session_storage_namespace_impl.h"
> On 2012/05/30 01:29:19, michaeln wrote:
> > we shouldn't be using interfaces that aren't in the content/public api here
> 
> Removed this, but now session_restore needs to include it so that it's able to
> create a SessionStorageNamespaceImpl.

i think we'll need to put static factory methods in the content api so chrome
can construct them
static SessionStorageNamespace* SessionStorageNamespace::Create(...)

> 
>
http://codereview.chromium.org/9963107/diff/82002/content/public/browser/sess...
> File content/public/browser/session_storage_namespace.h (right):
> 
>
http://codereview.chromium.org/9963107/diff/82002/content/public/browser/sess...
> content/public/browser/session_storage_namespace.h:17: class
> SessionStorageNamespace
> On 2012/05/30 01:29:19, michaeln wrote:
> > What if there were two different methods to manufacture an instance of this
> > class. One for use when creating completely new empty sessions and another
for
> > use when re-animating a previously saved session given its persistent_id?
> > 
> > Would it make things more clear if this class had an id() and a
> persistent_id()
> > accessor?
> 
> Done.

[marja, 2012-06-04 13:32:24]

Hi again,

I uploaded a new patch set:
- Unified "id" vs "persistent id" naming
- CanDelete -> Doom
- added DOMStorageContext::CreateSessionStorage (if the creation func was a
static in SessionStorageNamespace, it couldn't be overwritten by
SessionStorageNamespaceImpl)

[michaeln, 2012-06-05 01:41:07]

Please (please) think about how to split this up into manageable pieces. There's
a bunch of different things going on in this very large CL...

- support for a cmd-line flag
- mods to existing SessionStorageDB class in light of newer thinking
- some amount of SessionSave/Restore logic to connect the dots
- some amount of dom_storage logic to support the above
- some amount of content lib plumbing to let one talk to the other

... with different buckets of stuff at different levels of doneness.

It looks like one of the remaining pieces is how/when the higher level parts
tell dom_storage to doom things. But there's lots of stuff that i think could be
carved off and committed.

Question about things at the SessionRestore level? Can it distinguish between
the previous session ended with a crash vs the previous session ended nicely? I
wonder if the doom'ed handling is different in those cases?

https://chromiumcodereview.appspot.com/9963107/diff/100003/chrome/browser/ses...
File chrome/browser/sessions/session_restore.cc (right):

https://chromiumcodereview.appspot.com/9963107/diff/100003/chrome/browser/ses...
chrome/browser/sessions/session_restore.cc:884:
content::SessionStorageNamespace* session_storage_namespace = NULL;
probably s/b a scoped_refptr<> here

https://chromiumcodereview.appspot.com/9963107/diff/100003/content/public/bro...
File content/public/browser/browser_context.h (right):

https://chromiumcodereview.appspot.com/9963107/diff/100003/content/public/bro...
content/public/browser/browser_context.h:127: virtual bool
ShouldSaveSessionStorageOnDisk() = 0;
There's a bunch of indirection with 'should save' (in profile and here) just
because the cmd line switch is defined up in the chrome library. Maybe we should
define that string constant in the in the content library instead? It'd be nice
to avoid these changes to Profile and BrowserContext may to temporarily put this
behind a flag.

https://chromiumcodereview.appspot.com/9963107/diff/100003/content/public/bro...
File content/public/browser/dom_storage_context.h (right):

https://chromiumcodereview.appspot.com/9963107/diff/100003/content/public/bro...
content/public/browser/dom_storage_context.h:52: virtual void
DoomSessionStorage(int64 namespace_id) = 0;
Do you know where the callsite for this method should be? When does it get
called by whom? A little odd that one method works with the shorter lived ids
and the other works with longer lived persistent_ids.

https://chromiumcodereview.appspot.com/9963107/diff/100003/content/public/bro...
content/public/browser/dom_storage_context.h:54: virtual
SessionStorageNamespace* CreateSessionStorage(
Probably should have the return value be of type scoped_refptr<T> here since
this is returning a newly created refcounted object. Would
RecreateSessionStorage(persistent_id) or even RestoreSessionStorage(...) be a
better name for this particular method since the intent is to restore an old one
from disk?

https://chromiumcodereview.appspot.com/9963107/diff/100003/webkit/dom_storage...
File webkit/dom_storage/dom_storage_context.h (right):

https://chromiumcodereview.appspot.com/9963107/diff/100003/webkit/dom_storage...
webkit/dom_storage/dom_storage_context.h:211: std::set<int64>
deletable_session_storages_;
would doomed_session_ids_ work as a name? i think it'd be nice to haved 'doomed'
in there to make it clear it was related to that method with that name in it
too.

[marja, 2012-06-06 13:37:10]

Hi again & thanks for comments,

This CL still contains the following parts:

(1) adapter indirection between DomStorageArea and DomStorageDatabase
(2) creating and maintaining persistent IDs
(3) saving & restoring persistent ids
(4) integrating SessionStorageDatabase into DomStorageArea

I could split these into separate CLs, wdyt?

https://chromiumcodereview.appspot.com/9963107/diff/100003/chrome/browser/ses...
File chrome/browser/sessions/session_restore.cc (right):

https://chromiumcodereview.appspot.com/9963107/diff/100003/chrome/browser/ses...
chrome/browser/sessions/session_restore.cc:884:
content::SessionStorageNamespace* session_storage_namespace = NULL;
On 2012/06/05 01:41:07, michaeln wrote:
> probably s/b a scoped_refptr<> here

Done.

https://chromiumcodereview.appspot.com/9963107/diff/100003/content/public/bro...
File content/public/browser/browser_context.h (right):

https://chromiumcodereview.appspot.com/9963107/diff/100003/content/public/bro...
content/public/browser/browser_context.h:127: virtual bool
ShouldSaveSessionStorageOnDisk() = 0;
On 2012/06/05 01:41:07, michaeln wrote:
> There's a bunch of indirection with 'should save' (in profile and here) just
> because the cmd line switch is defined up in the chrome library. Maybe we
should
> define that string constant in the in the content library instead? It'd be
nice
> to avoid these changes to Profile and BrowserContext may to temporarily put
this
> behind a flag.

Done. The flag is in DOMStorageContextImpl ctor.

https://chromiumcodereview.appspot.com/9963107/diff/100003/content/public/bro...
File content/public/browser/dom_storage_context.h (right):

https://chromiumcodereview.appspot.com/9963107/diff/100003/content/public/bro...
content/public/browser/dom_storage_context.h:52: virtual void
DoomSessionStorage(int64 namespace_id) = 0;
On 2012/06/05 01:41:07, michaeln wrote:
> Do you know where the callsite for this method should be? When does it get
> called by whom? A little odd that one method works with the shorter lived ids
> and the other works with longer lived persistent_ids.

Atm the DoomSessionStorage is called by SessionService when it gets the "tab
destroyed" notification. Such a tab won't be restored in the next session
restore.

It's possible to know if the last exit was clean or unclean, but if it was
unclean, it's not (atm) possible to know if the "restore your session" infobar
is still visible or not (so there is no well-defined point in time where we can
be sure that the session restore won't happen).

Probably it wouldn't make it any easier to read through the session restore data
we're not going to use, and doom namespaces based on it. If we're didn't
restore, DomStorageContext::DeleteLeftoverDataInCommitSequence will delete all
the data in the db unless we have a good reason for not deleting something (an
existing SessionStorageDatabase, or we know the id is in the current session
restore data).

It doesn't really matter if this takes the int64 id or the persistent id (the
callsite knows both). Changed it to take a persistent id.

https://chromiumcodereview.appspot.com/9963107/diff/100003/content/public/bro...
content/public/browser/dom_storage_context.h:54: virtual
SessionStorageNamespace* CreateSessionStorage(
On 2012/06/05 01:41:07, michaeln wrote:
> Probably should have the return value be of type scoped_refptr<T> here since
> this is returning a newly created refcounted object. Would
> RecreateSessionStorage(persistent_id) or even RestoreSessionStorage(...) be a
> better name for this particular method since the intent is to restore an old
one
> from disk?

Done. Changed the name to RecreateSessionStorage and made it to return
scoped_refptr.

https://chromiumcodereview.appspot.com/9963107/diff/100003/webkit/dom_storage...
File webkit/dom_storage/dom_storage_context.h (right):

https://chromiumcodereview.appspot.com/9963107/diff/100003/webkit/dom_storage...
webkit/dom_storage/dom_storage_context.h:211: std::set<int64>
deletable_session_storages_;
On 2012/06/05 01:41:07, michaeln wrote:
> would doomed_session_ids_ work as a name? i think it'd be nice to haved
'doomed'
> in there to make it clear it was related to that method with that name in it
> too.

Changed to "doomed_persistent_session_ids_".

[michaeln, 2012-06-08 01:04:20]

split plans sounds reasonable, think about how to split so you can get good
reviews from appropriate folks w/o them having to look at too much other stuff
they may not be so best as a reviewer on... there's content stuff and chrome
stuff and sessionRestore stuff and dom_storage stuff... lots of stuff :)

https://chromiumcodereview.appspot.com/9963107/diff/100003/content/public/bro...
File content/public/browser/browser_context.h (right):

https://chromiumcodereview.appspot.com/9963107/diff/100003/content/public/bro...
content/public/browser/browser_context.h:127: virtual bool
ShouldSaveSessionStorageOnDisk() = 0;
> Done. The flag is in DOMStorageContextImpl ctor.

Drat, I think i may have given you a bum steer around this one :( The flag in
DOMStorageContextImpl ctor definitely works to start with, but...

I wasn't thinking about the possibility of other 'content' library embedders
besides chrome. On second thought, it might be nice to have a big master on/off
switch for enabling persistent session storage stuff in the content library...
for embedders with no intention of ever doing anything like 'session restore'...
could make their lives a little easier.

Having that master on/off switch on the BrowserContext could work. Another
option might be to have it on the ContentBrowserClient interface. Where ever it
goes, a name that includes 'persistent' might be helpful.

virtual bool ShouldPersistSessionStorage();

https://chromiumcodereview.appspot.com/9963107/diff/111002/chrome/browser/ses...
File chrome/browser/sessions/session_service.cc (right):

https://chromiumcodereview.appspot.com/9963107/diff/111002/chrome/browser/ses...
chrome/browser/sessions/session_service.cc:623: // in the session restore.
I see that NOTIFICATION_TAB_CONTENTS_DESTROYED happens when closing a tab. But i
also see that NOTIFICATION_TAB_PARENTED happens to wake it up if the user
reopens a recently closed page via the  drop down menu at the bottom-right of
the new-tab-page. In this reopen case, the same SessionStorageNamespace instance
is there ready to go. But since the data was doomed in
NOTIFICATION_TAB_CONTENTS_DESTROYED, it won't be avaialble for later session
restoration. Do we need an UnDoom() method too for use in the PARENTED case? Or
maybe there's another way to manage which to keep|discard?

What if content::SessionStorageNamespace had another property (getter/setter) to
control keep vs discard behavior?

class SessionStorageNamespace {
  int64 id();
  std::string persistent_id();
  bool should_persist();
  void SetShouldPersist(bool persist);
}

Upon object creation, should persist is false. 
In PARENTED, chrome calls SetShouldPersist(true).
In DESTROYED, chrome calls SetShouldPersist(false).

While the namespace exists, data is put in the db regardless of how the value is
set at. It can toggle back and forth during its lifetime w/o any immediate side
effects. But at SessionStorageNamespace final release time or DomStorageContext
shutdown time (whichever comes first), the attribute value is used to determine
whether to delete or not from the db.

We still have the issue of deleting 'left over' stuff at some point. After
restarting and either restoring tabs or having determined that we wont be
restoring old tabs (when the infobar is dismissed maybe), that might be a good
time to start deleting left over stuff. I think any session in the db for which
there is no live SessionStorageNamespace instance is probably eligible for
deletion at this time. The lower-level DomStorageContext could retrieve a list
of those in the db, remove live ids from that list, and lazily call delete on
the remaining items... delete one every 60 seconds till there gone type of
thing. Something in the public 'content' api to kick that off...
   DOMStorageContext::StartScavengingUnusedSessionStorage().

wdyt?

[marja, 2012-06-19 13:17:17]

Fixed restoring tabs which were opened via the "recently closed" menu.

Next I'll split off the parts which associate the persistent IDs to the session
restore system, with dummy implementation for the "recreate" functionality
(which just creates a new storage).

https://chromiumcodereview.appspot.com/9963107/diff/111002/chrome/browser/ses...
File chrome/browser/sessions/session_service.cc (right):

https://chromiumcodereview.appspot.com/9963107/diff/111002/chrome/browser/ses...
chrome/browser/sessions/session_service.cc:623: // in the session restore.
On 2012/06/08 01:04:21, michaeln wrote:
> I see that NOTIFICATION_TAB_CONTENTS_DESTROYED happens when closing a tab. But
i
> also see that NOTIFICATION_TAB_PARENTED happens to wake it up if the user
> reopens a recently closed page via the  drop down menu at the bottom-right of
> the new-tab-page. In this reopen case, the same SessionStorageNamespace
instance
> is there ready to go. But since the data was doomed in
> NOTIFICATION_TAB_CONTENTS_DESTROYED, it won't be avaialble for later session
> restoration. Do we need an UnDoom() method too for use in the PARENTED case?
Or
> maybe there's another way to manage which to keep|discard?
> 
> What if content::SessionStorageNamespace had another property (getter/setter)
to
> control keep vs discard behavior?
> 
> class SessionStorageNamespace {
>   int64 id();
>   std::string persistent_id();
>   bool should_persist();
>   void SetShouldPersist(bool persist);
> }
> 
> Upon object creation, should persist is false. 
> In PARENTED, chrome calls SetShouldPersist(true).
> In DESTROYED, chrome calls SetShouldPersist(false).
> 
> While the namespace exists, data is put in the db regardless of how the value
is
> set at. It can toggle back and forth during its lifetime w/o any immediate
side
> effects. But at SessionStorageNamespace final release time or
DomStorageContext
> shutdown time (whichever comes first), the attribute value is used to
determine
> whether to delete or not from the db.
> 
> We still have the issue of deleting 'left over' stuff at some point. After
> restarting and either restoring tabs or having determined that we wont be
> restoring old tabs (when the infobar is dismissed maybe), that might be a good
> time to start deleting left over stuff. I think any session in the db for
which
> there is no live SessionStorageNamespace instance is probably eligible for
> deletion at this time. The lower-level DomStorageContext could retrieve a list
> of those in the db, remove live ids from that list, and lazily call delete on
> the remaining items... delete one every 60 seconds till there gone type of
> thing. Something in the public 'content' api to kick that off...
>    DOMStorageContext::StartScavengingUnusedSessionStorage().
> 
> wdyt?

Did the SetShouldPersist change; it's cleaner now.

For the DOMStorageContext::StartScavengingUnusedSessionStorage() change I'd need
to add code to send a signal when the infobar is dismissed, and it's harder than
it sounds to add that (at least the last time I tried I bumped into some unclear
ownership of infobars and other complications). If you think it's important that
it's done that way, I'll start working on it.

[michaeln, 2012-06-20 19:33:49]

> Did the SetShouldPersist change; it's cleaner now.

Nice!

> StartScavengingUnusedSessionStorage

Lets see what jochen thinks? I do think it would be nicer to spread the cost out
over time instead of having it happen all during shutdown, kind of cleans up
after itself as it goes incrementally. Consider a user that never really quits
the browser.

But I guess even w/o scavenging remnants from previous browser sessions, storage
utilized in the current browser session does get cleaned up incrementally as the
associated tabs become unrestorable, so its just remnants from previous sessions
that get bulk deleted at the end.

If getting that signal out of the infobar is problematic (why is that though?),
starting with 'do it all on shutdown' and revisit later might be reasonable.

>>

Hey... remember where this started...
https://chromiumcodereview.appspot.com/8929007/ :)

[jochen (gone - plz use gerrit), 2012-06-20 22:31:04]

I agree with Michael :)

On Wed, Jun 20, 2012 at 9:33 PM, <michaeln@chromium.org> wrote:

> Did the SetShouldPersist change; it's cleaner now.
>>
>
> Nice!
>
>  StartScavengingUnusedSessionSt**orage
>>
>
> Lets see what jochen thinks? I do think it would be nicer to spread the
> cost out
> over time instead of having it happen all during shutdown, kind of cleans
> up
> after itself as it goes incrementally. Consider a user that never really
> quits
> the browser.
>
> But I guess even w/o scavenging remnants from previous browser sessions,
> storage
> utilized in the current browser session does get cleaned up incrementally
> as the
> associated tabs become unrestorable, so its just remnants from previous
> sessions
> that get bulk deleted at the end.
>
> If getting that signal out of the infobar is problematic (why is that
> though?),
> starting with 'do it all on shutdown' and revisit later might be
> reasonable.
>
>
>
> Hey... remember where this started...
>
https://chromiumcodereview.**appspot.com/8929007/<https://chromiumcodereview....)
>
>
https://chromiumcodereview.**appspot.com/9963107/<https://chromiumcodereview....
>

[marja, 2012-06-25 16:05:26]

Hi michaeln,

No more spin-offs, but I rebased this and uploaded a new version which scavenges
the sessionStorages. pkasting said the way to notice the "Your browsing session
has crashed" infobar has expired is by listening to this
NOTIFICATION_TAB_CONTENTS_INFOBAR_REMOVED notification.

The "one switch in content:: for switching the file-backed sessionStorage on" is
not implemented yet, I'll do that tomorrow, but to parallelize, could you have a
look at this version?

I was also wondering how to integrate sessionStorage into the "Usage data" (&
BrowsingDataRemover). For each namespace-origin pair, should I save a "last
access" timestamp? (Afaics leveldb doesn't support timestamps as such, so I'd
just stringify a base::Time or sth.)

[michaeln, 2012-06-26 06:15:12]

http://codereview.chromium.org/9963107/diff/141002/chrome/browser/sessions/se...
File chrome/browser/sessions/session_restore.cc (right):

http://codereview.chromium.org/9963107/diff/141002/chrome/browser/sessions/se...
chrome/browser/sessions/session_restore.cc:737: return
FinishedTabCreation(false, false);
would it make sense to also kick of scavenging prior to this early return?

http://codereview.chromium.org/9963107/diff/141002/content/public/browser/dom...
File content/public/browser/dom_storage_context.h (right):

http://codereview.chromium.org/9963107/diff/141002/content/public/browser/dom...
content/public/browser/dom_storage_context.h:42: virtual void
StartScavengingUnusedSessionStorage() = 0;
a doc comment about this method would be good

http://codereview.chromium.org/9963107/diff/141002/webkit/dom_storage/dom_sto...
File webkit/dom_storage/dom_storage_area.cc (right):

http://codereview.chromium.org/9963107/diff/141002/webkit/dom_storage/dom_sto...
webkit/dom_storage/dom_storage_area.cc:185: copy->is_shutdown_ = is_shutdown_;
since map_ is populated, should we set is_initial_import_done_ to true here?

http://codereview.chromium.org/9963107/diff/141002/webkit/dom_storage/dom_sto...
File webkit/dom_storage/dom_storage_context.cc (right):

http://codereview.chromium.org/9963107/diff/141002/webkit/dom_storage/dom_sto...
webkit/dom_storage/dom_storage_context.cc:261: if
(session_storage_database_.get()) {
this is so weird to be clearing 'session only' stuff from the set of 'session
only' storage namespace :)

http://codereview.chromium.org/9963107/diff/141002/webkit/dom_storage/dom_sto...
webkit/dom_storage/dom_storage_context.cc:298: for
(StorageNamespaceMap::const_iterator it = namespaces_.begin();
The namespaces_ collection shouldn't be accessed on the COMMIT sequence. Maybe
make StartScavenging only callable on the READ sequence (the content layer class
can PostTask to it as is done for all of the other methods), and put this set<>
together in Start and pass it as an OwnPtr'd param to this method.

Also once we start scavenging, should we clear out the
protected_persistent_session_ids_ and arrange to no longer add to it?

http://codereview.chromium.org/9963107/diff/141002/webkit/dom_storage/dom_sto...
webkit/dom_storage/dom_storage_context.cc:314: task_runner_->PostDelayedTask(
This will post a task to the PRIMARYsequence which i think is not what you
intend? Looks like you intend to call db->DeleteNamespace(id) on the COMMIT
sequence, which i think is a good idea.

http://codereview.chromium.org/9963107/diff/141002/webkit/dom_storage/dom_sto...
webkit/dom_storage/dom_storage_context.cc:328: task_runner_->PostDelayedTask(
Ditto about this will post something for execution on the PRIMARY sequence.

[michaeln, 2012-06-26 06:28:39]

Also, there's another thing thats not clear to me. Do we need to be using
leveldb snapshotting in some way to allow reads from the PRIMARY sequence to not
pick up confusing results where some data was read prior to a write batch commit
on the COMMIT sequence, and other data is read after? Do any of our read tasks
involve multiple leveldb read operations?

[marja, 2012-06-26 14:18:05]

Thanks for comments!

Trying to get the threading right...

DomStorageArea does ReadAllValues (the initial import) in the thread it lives
in, and DeleteArea and CommitChanges in the COMMIT_SEQUENCE. The initial import
cannot interfere with CommitChanges, since the initial import is forced before
anything gets committed, but it can interfere with DeleteArea.

DomStorageNamespace does only CloneNamespace in the COMMIT_SEQUENCE.

DomStorageContext does ReadNamespaceIds, ReadOriginsInNamespace and DeleteArea,
all in COMMIT_SEQUENCE (after the fix). The scavenging shouldn't interfere with
ReadAllValues, since the scavenged data shouldn't have the corresponding
DomStorageNamespace or DomStorageArea alive.

The SessionStorageNamespace data reading operations all do multiple leveldb
reading operations.

So, the only potential problem I see is that the DeleteArea posted by
DomStorageArea::DeleteOrigin interferes with
DomStorageArea::InitialImportIfNeeded. Should we solve that on DomStorageArea
side, e.g., setting a flag "deletion in progress", and if that is true,
InitialImportIfNeeded figures the data will be empty anyway, and doesn't try to
read it. Does that sound ok? (That is also implemented in the latest patch set.)

http://codereview.chromium.org/9963107/diff/141002/chrome/browser/sessions/se...
File chrome/browser/sessions/session_restore.cc (right):

http://codereview.chromium.org/9963107/diff/141002/chrome/browser/sessions/se...
chrome/browser/sessions/session_restore.cc:737: return
FinishedTabCreation(false, false);
On 2012/06/26 06:15:12, michaeln wrote:
> would it make sense to also kick of scavenging prior to this early return?

Done.

http://codereview.chromium.org/9963107/diff/141002/content/public/browser/dom...
File content/public/browser/dom_storage_context.h (right):

http://codereview.chromium.org/9963107/diff/141002/content/public/browser/dom...
content/public/browser/dom_storage_context.h:42: virtual void
StartScavengingUnusedSessionStorage() = 0;
On 2012/06/26 06:15:12, michaeln wrote:
> a doc comment about this method would be good

Done.

http://codereview.chromium.org/9963107/diff/141002/webkit/dom_storage/dom_sto...
File webkit/dom_storage/dom_storage_area.cc (right):

http://codereview.chromium.org/9963107/diff/141002/webkit/dom_storage/dom_sto...
webkit/dom_storage/dom_storage_area.cc:185: copy->is_shutdown_ = is_shutdown_;
On 2012/06/26 06:15:12, michaeln wrote:
> since map_ is populated, should we set is_initial_import_done_ to true here?

Done.

http://codereview.chromium.org/9963107/diff/141002/webkit/dom_storage/dom_sto...
File webkit/dom_storage/dom_storage_context.cc (right):

http://codereview.chromium.org/9963107/diff/141002/webkit/dom_storage/dom_sto...
webkit/dom_storage/dom_storage_context.cc:261: if
(session_storage_database_.get()) {
On 2012/06/26 06:15:12, michaeln wrote:
> this is so weird to be clearing 'session only' stuff from the set of 'session
> only' storage namespace :)

Ack :)

http://codereview.chromium.org/9963107/diff/141002/webkit/dom_storage/dom_sto...
webkit/dom_storage/dom_storage_context.cc:298: for
(StorageNamespaceMap::const_iterator it = namespaces_.begin();
On 2012/06/26 06:15:12, michaeln wrote:
> The namespaces_ collection shouldn't be accessed on the COMMIT sequence. Maybe
> make StartScavenging only callable on the READ sequence (the content layer
class
> can PostTask to it as is done for all of the other methods), and put this
set<>
> together in Start and pass it as an OwnPtr'd param to this method.
> 
> Also once we start scavenging, should we clear out the
> protected_persistent_session_ids_ and arrange to no longer add to it?

How about this version which passes const std::set<std::string>
namespace_ids_in_use? Do we need the OwnedPtr, shouldn't this just work as
expected?

Also, cleared protected_persistent_session_ids.

http://codereview.chromium.org/9963107/diff/141002/webkit/dom_storage/dom_sto...
webkit/dom_storage/dom_storage_context.cc:314: task_runner_->PostDelayedTask(
On 2012/06/26 06:15:12, michaeln wrote:
> This will post a task to the PRIMARYsequence which i think is not what you
> intend? Looks like you intend to call db->DeleteNamespace(id) on the COMMIT
> sequence, which i think is a good idea.

Done, yes, this was a bug, I meant COMMIT_SEQUENCE here.

There is no PostDelayedTask which would take a sequence id as a param; I first
PostDelayedTask into the primary sequence, from there PostShutdownBlockingTask
into the COMMIT_SEQUENCE.

I also added a kSessionStorageScavengingSeconds delay before finding the unused
session storages; it's probably better to wait a bit before doing it in case we
have just restored a session and all the tabs try to load...

http://codereview.chromium.org/9963107/diff/141002/webkit/dom_storage/dom_sto...
webkit/dom_storage/dom_storage_context.cc:328: task_runner_->PostDelayedTask(
On 2012/06/26 06:15:12, michaeln wrote:
> Ditto about this will post something for execution on the PRIMARY sequence.

Done.

[michaeln, 2012-07-10 01:16:57]

On 2012/06/26 14:18:05, marja wrote:
> Thanks for comments!
> 
> Trying to get the threading right...
> 
> DomStorageArea does ReadAllValues (the initial import) in the thread it lives
> in, and DeleteArea and CommitChanges in the COMMIT_SEQUENCE. The initial
import
> cannot interfere with CommitChanges, since the initial import is forced before
> anything gets committed, but it can interfere with DeleteArea.
> 
> DomStorageNamespace does only CloneNamespace in the COMMIT_SEQUENCE.
> 
> DomStorageContext does ReadNamespaceIds, ReadOriginsInNamespace and
DeleteArea,
> all in COMMIT_SEQUENCE (after the fix). The scavenging shouldn't interfere
with
> ReadAllValues, since the scavenged data shouldn't have the corresponding
> DomStorageNamespace or DomStorageArea alive.
> 
> The SessionStorageNamespace data reading operations all do multiple leveldb
> reading operations.
> 
> So, the only potential problem I see is that the DeleteArea posted by
> DomStorageArea::DeleteOrigin interferes with
> DomStorageArea::InitialImportIfNeeded. Should we solve that on DomStorageArea
> side, e.g., setting a flag "deletion in progress", and if that is true,
> InitialImportIfNeeded figures the data will be empty anyway, and doesn't try
to
> read it. Does that sound ok? (That is also implemented in the latest patch
set.)

Sounds like its only ReadAllValues that happens on the primary sequence (great).

But concurrent with one area being committed, a different area may be getting
initially imported. Can either iterators used by ReadAllValues become invalid
midstread after a commit, or is it possible for some values read on behalf of
ReadAllValues to be inconsistent with later values read in that method call?

https://chromiumcodereview.appspot.com/9963107/diff/157002/chrome/browser/ui/...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://chromiumcodereview.appspot.com/9963107/diff/157002/chrome/browser/ui/...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:591:
dom_storage_context->StartScavengingUnusedSessionStorage();
It's unfortunate there are so many places where this gets kicked off. Not sure
if there's a better answer or not, just sayin...

https://chromiumcodereview.appspot.com/9963107/diff/157002/content/browser/br...
File content/browser/browser_context.cc (right):

https://chromiumcodereview.appspot.com/9963107/diff/157002/content/browser/br...
content/browser/browser_context.cc:258: // Too late.
Is this case ever to be expected, if not should we DCHECK here instead? Also is
there a reason to use noop UserData instance instead of a simple bool data
member for this setting?

https://chromiumcodereview.appspot.com/9963107/diff/157002/webkit/dom_storage...
File webkit/dom_storage/dom_storage_area.cc (right):

https://chromiumcodereview.appspot.com/9963107/diff/157002/webkit/dom_storage...
webkit/dom_storage/dom_storage_area.cc:227: base::AutoLock
lock(deletion_in_progress_lock_);
What does the flag+lock accomplish given that is_initial_import_done_ is true?
Looks like it's to avoid importing while deleting, but i think the
is_import_done flag guards against that already.

Also, i think this method may actually be unreachable in the case of session
storage atm? Namespaces get deleted wholesale, but there's no codepath or
usecase for deleting an individual origin out of a session namespace just yet
(see DomStorageNamespace for the DeleteOrigin callsite). If that's right, you
may want to just DCHECK that assumption here instead.

https://chromiumcodereview.appspot.com/9963107/diff/157002/webkit/dom_storage...
File webkit/dom_storage/dom_storage_context.cc (right):

https://chromiumcodereview.appspot.com/9963107/diff/157002/webkit/dom_storage...
webkit/dom_storage/dom_storage_context.cc:282: void
DomStorageContext::StartScavengingUnusedSessionStorage() {
any chance this can be called more than once? maybe early return if
scavenging_started_

https://chromiumcodereview.appspot.com/9963107/diff/157002/webkit/dom_storage...
webkit/dom_storage/dom_storage_context.cc:293: this, namespace_ids_in_use,
protected_persistent_session_ids_),
you'll need to nix the _ on the protected_persistent_session_ids_ param in this
method call

https://chromiumcodereview.appspot.com/9963107/diff/157002/webkit/dom_storage...
webkit/dom_storage/dom_storage_context.cc:307:
&DomStorageContext::FindUnusedSessionStorageInCommitSequence,
Is there a window of time between StartScavenging and this call where other
namespaces can be created/deleted and not get added to the protected collection?

https://chromiumcodereview.appspot.com/9963107/diff/157002/webkit/dom_storage...
File webkit/dom_storage/dom_storage_context.h (right):

https://chromiumcodereview.appspot.com/9963107/diff/157002/webkit/dom_storage...
webkit/dom_storage/dom_storage_context.h:181: void FindUnusedSessionStorage(
naming nit: maybe name all of the Find and Delete methods as
<Verb>UnusedNamespace<Adverb> for consistency with one another

https://chromiumcodereview.appspot.com/9963107/diff/157002/webkit/dom_storage...
webkit/dom_storage/dom_storage_context.h:187: void DeleteNextUnunsedNamespace();
typo Ununsed -> Unused

[marja, 2012-07-10 13:40:51]

Hi & thanks for comments again.

Here's another version, but I didn't have time to read it through, so it might
have some unclean or weird changes in it...

What I can imagine that can happen is that CommitAreaChanges updates the ref
count of a map while ReadAreaValues is iterating over the map refcount key. Not
sure if that would cause problems, but I added a snapshot to ReadAreaValues, and
I added a comment to the header that ReadAreaValues can be called by any thread,
but only one thread should call the other functions.

StartScavenging should be called only once, but now that I moved setting the
bool out, it can't any more DCHECK it...

http://codereview.chromium.org/9963107/diff/157002/chrome/browser/ui/startup/...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

http://codereview.chromium.org/9963107/diff/157002/chrome/browser/ui/startup/...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:591:
dom_storage_context->StartScavengingUnusedSessionStorage();
On 2012/07/10 01:16:58, michaeln wrote:
> It's unfortunate there are so many places where this gets kicked off. Not sure
> if there's a better answer or not, just sayin...

Ack. The knowledge on "now we're sure that the session restore won't occur" or
"now we're sure that it occurred and the needed Namespaces are created" seems to
be scattered. :/

http://codereview.chromium.org/9963107/diff/157002/content/browser/browser_co...
File content/browser/browser_context.cc (right):

http://codereview.chromium.org/9963107/diff/157002/content/browser/browser_co...
content/browser/browser_context.cc:258: // Too late.
On 2012/07/10 01:16:58, michaeln wrote:
> Is this case ever to be expected, if not should we DCHECK here instead? Also
is
> there a reason to use noop UserData instance instead of a simple bool data
> member for this setting?

Done. DCHECK added. I wasn't sure if it's ok to add a bool data member to
BrowserContext. Added it now.

http://codereview.chromium.org/9963107/diff/157002/webkit/dom_storage/dom_sto...
File webkit/dom_storage/dom_storage_area.cc (right):

http://codereview.chromium.org/9963107/diff/157002/webkit/dom_storage/dom_sto...
webkit/dom_storage/dom_storage_area.cc:227: base::AutoLock
lock(deletion_in_progress_lock_);
On 2012/07/10 01:16:58, michaeln wrote:
> What does the flag+lock accomplish given that is_initial_import_done_ is true?
> Looks like it's to avoid importing while deleting, but i think the
> is_import_done flag guards against that already.
> 
> Also, i think this method may actually be unreachable in the case of session
> storage atm? Namespaces get deleted wholesale, but there's no codepath or
> usecase for deleting an individual origin out of a session namespace just yet
> (see DomStorageNamespace for the DeleteOrigin callsite). If that's right, you
> may want to just DCHECK that assumption here instead.

The lock was guarding the bool (I didn't want to assume that assigning a bool is
atomic).

This is indeed not wired in -> removed this code (and the deletion_in_progress_,
deletion_in_progress_lock_) and added a DCHECK here.

http://codereview.chromium.org/9963107/diff/157002/webkit/dom_storage/dom_sto...
File webkit/dom_storage/dom_storage_context.cc (right):

http://codereview.chromium.org/9963107/diff/157002/webkit/dom_storage/dom_sto...
webkit/dom_storage/dom_storage_context.cc:282: void
DomStorageContext::StartScavengingUnusedSessionStorage() {
On 2012/07/10 01:16:58, michaeln wrote:
> any chance this can be called more than once? maybe early return if
> scavenging_started_

I moved reading the atm-in-use namespaces forward in time (FindUnusedNamespace),
since that's the latest point in this thread that we can read the info. Also,
scavenging_started_ is set there, and it posts a task in the COMMIT_SEQUENCE to
read the DB.

Now, if a namespace is created, data is written to it, and it's deleted, the
data won't be written into the DB before we read the DB for finding unused
namespaces (since the next thing that will happen in the COMMIT_SEQUENCE) is
FindUnusedNamespaceInCommitSequence.

So this version cannot delete data by accident, afaics...

http://codereview.chromium.org/9963107/diff/157002/webkit/dom_storage/dom_sto...
webkit/dom_storage/dom_storage_context.cc:293: this, namespace_ids_in_use,
protected_persistent_session_ids_),
On 2012/07/10 01:16:58, michaeln wrote:
> you'll need to nix the _ on the protected_persistent_session_ids_ param in
this
> method call

Done.

http://codereview.chromium.org/9963107/diff/157002/webkit/dom_storage/dom_sto...
webkit/dom_storage/dom_storage_context.cc:307:
&DomStorageContext::FindUnusedSessionStorageInCommitSequence,
On 2012/07/10 01:16:58, michaeln wrote:
> Is there a window of time between StartScavenging and this call where other
> namespaces can be created/deleted and not get added to the protected
collection?

This problem should be solved now, pls see above.

http://codereview.chromium.org/9963107/diff/157002/webkit/dom_storage/dom_sto...
File webkit/dom_storage/dom_storage_context.h (right):

http://codereview.chromium.org/9963107/diff/157002/webkit/dom_storage/dom_sto...
webkit/dom_storage/dom_storage_context.h:181: void FindUnusedSessionStorage(
On 2012/07/10 01:16:58, michaeln wrote:
> naming nit: maybe name all of the Find and Delete methods as
> <Verb>UnusedNamespace<Adverb> for consistency with one another

Done.

http://codereview.chromium.org/9963107/diff/157002/webkit/dom_storage/dom_sto...
webkit/dom_storage/dom_storage_context.h:187: void DeleteNextUnunsedNamespace();
On 2012/07/10 01:16:58, michaeln wrote:
> typo Ununsed -> Unused

Done.

[michaeln, 2012-07-10 19:04:13]

dom_storage parts lgtm, you'll need to get others to look at the details of the
content layer and profile changes and such

https://chromiumcodereview.appspot.com/9963107/diff/149028/content/browser/br...
File content/browser/browser_context.cc (right):

https://chromiumcodereview.appspot.com/9963107/diff/149028/content/browser/br...
content/browser/browser_context.cc:86: context->save_session_storage_on_disk());
All of the other methods in the content::BrowsingContext class are virtual. This
getter maybe should be as well for consistency with all else? Maybe the content
interface for this should be virtual getter with a default impl that returns
'false', and a chrome specific derivation can override the method and check the
cmdline flag.

You're going to need 'content' owners approval for these changes to the
BrowsingContext class and interface. Maybe jam can give us some direction about
how he'd like to see this exposed in that interface?

https://chromiumcodereview.appspot.com/9963107/diff/149028/webkit/dom_storage...
File webkit/dom_storage/dom_storage_area.h (right):

https://chromiumcodereview.appspot.com/9963107/diff/149028/webkit/dom_storage...
webkit/dom_storage/dom_storage_area.h:15: #include "base/synchronization/lock.h"
is this still needed?

https://chromiumcodereview.appspot.com/9963107/diff/149028/webkit/dom_storage...
webkit/dom_storage/dom_storage_area.h:117: void DeleteOriginInCommitSequence();
and this?

https://chromiumcodereview.appspot.com/9963107/diff/149028/webkit/dom_storage...
File webkit/dom_storage/dom_storage_context.cc (right):

https://chromiumcodereview.appspot.com/9963107/diff/149028/webkit/dom_storage...
webkit/dom_storage/dom_storage_context.cc:292:
DCHECK(session_storage_database_.get());
Not quite as effective as in the Start method, but we could
DCHECK(!scavenging_started_) here.

[marja, 2012-07-11 12:41:30]

michaeln, thank you so much for review!

Also, I noticed, that it's possible that a leveldb iterator has status "ok", but
still Valid() returns false (somewhat unintuitively). So, I added validity
checks and asked on leveldb mailing list if this is normal...

Adding OWNERS reviewers, could you review the following parts:

pkasting:
chrome/browser/ui/*

sail:
chrome/browser/profiles/*

jam:
content/*

(sky: FYI, this changes chrome/browser/sessions/*, too.)

http://codereview.chromium.org/9963107/diff/149028/content/browser/browser_co...
File content/browser/browser_context.cc (right):

http://codereview.chromium.org/9963107/diff/149028/content/browser/browser_co...
content/browser/browser_context.cc:86: context->save_session_storage_on_disk());
On 2012/07/10 19:04:13, michaeln wrote:
> All of the other methods in the content::BrowsingContext class are virtual.
This
> getter maybe should be as well for consistency with all else? Maybe the
content
> interface for this should be virtual getter with a default impl that returns
> 'false', and a chrome specific derivation can override the method and check
the
> cmdline flag.
> 
> You're going to need 'content' owners approval for these changes to the
> BrowsingContext class and interface. Maybe jam can give us some direction
about
> how he'd like to see this exposed in that interface?

Added virtual ShouldSaveSessionStorageOnDisk which ProfileImpl overrides.

http://codereview.chromium.org/9963107/diff/149028/webkit/dom_storage/dom_sto...
File webkit/dom_storage/dom_storage_area.h (right):

http://codereview.chromium.org/9963107/diff/149028/webkit/dom_storage/dom_sto...
webkit/dom_storage/dom_storage_area.h:15: #include "base/synchronization/lock.h"
On 2012/07/10 19:04:13, michaeln wrote:
> is this still needed?

Done.

http://codereview.chromium.org/9963107/diff/149028/webkit/dom_storage/dom_sto...
webkit/dom_storage/dom_storage_area.h:117: void DeleteOriginInCommitSequence();
On 2012/07/10 19:04:13, michaeln wrote:
> and this?

Done.

http://codereview.chromium.org/9963107/diff/149028/webkit/dom_storage/dom_sto...
File webkit/dom_storage/dom_storage_context.cc (right):

http://codereview.chromium.org/9963107/diff/149028/webkit/dom_storage/dom_sto...
webkit/dom_storage/dom_storage_context.cc:292:
DCHECK(session_storage_database_.get());
On 2012/07/10 19:04:13, michaeln wrote:
> Not quite as effective as in the Start method, but we could
> DCHECK(!scavenging_started_) here.

Done.

[jochen (gone - plz use gerrit), 2012-07-11 12:44:25]

content/ looks good :)

[jam, 2012-07-11 15:51:35]

http://codereview.chromium.org/9963107/diff/158010/chrome/browser/ui/startup/...
File chrome/browser/ui/startup/session_crashed_prompt.cc (right):

http://codereview.chromium.org/9963107/diff/158010/chrome/browser/ui/startup/...
chrome/browser/ui/startup/session_crashed_prompt.cc:17: #include
"content/public/browser/browser_context.h"
nit: profile derives from browsercontext, and you're including the header for
the former, so no need for this

http://codereview.chromium.org/9963107/diff/158010/chrome/browser/ui/startup/...
chrome/browser/ui/startup/session_crashed_prompt.cc:19: #include
"content/public/browser/dom_storage_context.h"
nit: order

http://codereview.chromium.org/9963107/diff/158010/chrome/browser/ui/startup/...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

http://codereview.chromium.org/9963107/diff/158010/chrome/browser/ui/startup/...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:74: #include
"content/public/browser/browser_context.h"
nit: ditto

http://codereview.chromium.org/9963107/diff/158010/content/browser/browser_co...
File content/browser/browser_context.cc (right):

http://codereview.chromium.org/9963107/diff/158010/content/browser/browser_co...
content/browser/browser_context.cc:90:
context->ShouldSaveSessionStorageOnDisk());
how about instead of adding a method to the BrowserContext API (which I've tried
to keep minimal), you add a setter for this on content::DOMStorageContext? that
interface already has a method related to session restore.

so kind of like how it's done for cookies: 
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/profiles/pr...

http://codereview.chromium.org/9963107/diff/158010/content/browser/dom_storag...
File content/browser/dom_storage/dom_storage_context_impl.cc (right):

http://codereview.chromium.org/9963107/diff/158010/content/browser/dom_storag...
content/browser/dom_storage/dom_storage_context_impl.cc:97: DCHECK(context_);
nit: dcheck is unnecessary, if it's NULL, the next line will be just as good of
a signal

[sail, 2012-07-11 16:56:11]

http://codereview.chromium.org/9963107/diff/158010/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/9963107/diff/158010/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.h:70: virtual bool
ShouldSaveSessionStorageOnDisk() const OVERRIDE;
I'm not sure what session storage is. Does this really need to be a per profile
flag? Currently the implementation is not profile specific. Should there be a
TODO somewhere?

[sail, 2012-07-11 16:58:20]

Just a general comment about this types of CLs. Since they touch a lot of files
it would be nice to have a more descriptive CL description. Something along the
lines of what session storage is, what you're trying to do, and why you're doing
it. The same applies for the bug.

[jochen (gone - plz use gerrit), 2012-07-11 17:52:15]

http://codereview.chromium.org/9963107/diff/158010/content/browser/browser_co...
File content/browser/browser_context.cc (right):

http://codereview.chromium.org/9963107/diff/158010/content/browser/browser_co...
content/browser/browser_context.cc:90:
context->ShouldSaveSessionStorageOnDisk());
Cookies are a bad role model here: in contrast to all other storage APIs, the
embedder has to provide a persistent store for cookies.

Everything else persists stuff to disk iff the browser context has a path
defined.

The situation here is that an embedder might want to provide a path to have
regular data stored on disk, but would prefer not to persist sessionStorage.
This is different from the flag that is passed to the cookie monster (or the
other storage systems) that just controls how shutdown/startup is handled, but
doesn't change whether a file backend is created or not.

The problem with setting this flag on the DOM storage context directly is that
it needs to be passed to the constructor. The DOM storage context, however, is
created lazily. The embedder would need to invoke this method early enough,
requiring detailed information on how the browser context is implemented.

[jam, 2012-07-11 21:40:23]

http://codereview.chromium.org/9963107/diff/158010/content/browser/browser_co...
File content/browser/browser_context.cc (right):

http://codereview.chromium.org/9963107/diff/158010/content/browser/browser_co...
content/browser/browser_context.cc:90:
context->ShouldSaveSessionStorageOnDisk());
On 2012/07/11 17:52:16, jochen wrote:
> Cookies are a bad role model here: in contrast to all other storage APIs, the
> embedder has to provide a persistent store for cookies.
> 
> Everything else persists stuff to disk iff the browser context has a path
> defined.
> 
> The situation here is that an embedder might want to provide a path to have
> regular data stored on disk, but would prefer not to persist sessionStorage.
> This is different from the flag that is passed to the cookie monster (or the
> other storage systems) that just controls how shutdown/startup is handled, but
> doesn't change whether a file backend is created or not.
> 
> The problem with setting this flag on the DOM storage context directly is that
> it needs to be passed to the constructor. The DOM storage context, however, is
> created lazily. The embedder would need to invoke this method early enough,
> requiring detailed information on how the browser context is implemented.

I see. It seems that this can be changed so that it's not required at
construction, but just has to be set before it's used? i.e. it's the embedder
who creates a BrowserContext, so if they want to set this, they can do so soon
after before they use it.

[marja, 2012-07-12 09:22:02]

Thanks for comments!

michaeln, can you have another look at the DOMStorageContext API change?

Also, there is a leveldb gotcha, an iterator will never have status IsNotFound,
but if the key is not found, the iterator's Valid() will return false. I
simplified the code to check this.

http://codereview.chromium.org/9963107/diff/158010/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/9963107/diff/158010/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.h:70: virtual bool
ShouldSaveSessionStorageOnDisk() const OVERRIDE;
On 2012/07/11 16:56:11, sail wrote:
> I'm not sure what session storage is. Does this really need to be a per
profile
> flag? Currently the implementation is not profile specific. Should there be a
> TODO somewhere?

sessionStorage is part of the HTML5 DOM storage specification, see e.g.,
http://www.whatwg.org/specs/web-apps/current-work/#storage

I changed the Profile-related parts (according to jam's comments), so this
function doesn't exist any more, but instead ProfileImpl ctor retrieves a
DOMStorageContext and calls SetSaveSessionStorageOnDisk on it. This also makes
the changes to Profile minimal.

The DOM storage is per-profile (the data is stored under the profile directory),
and for incognito profiles both localStorage and sessionStorage are stored only
in memory.

http://codereview.chromium.org/9963107/diff/158010/chrome/browser/ui/startup/...
File chrome/browser/ui/startup/session_crashed_prompt.cc (right):

http://codereview.chromium.org/9963107/diff/158010/chrome/browser/ui/startup/...
chrome/browser/ui/startup/session_crashed_prompt.cc:17: #include
"content/public/browser/browser_context.h"
On 2012/07/11 15:51:35, John Abd-El-Malek wrote:
> nit: profile derives from browsercontext, and you're including the header for
> the former, so no need for this

Done.

http://codereview.chromium.org/9963107/diff/158010/chrome/browser/ui/startup/...
chrome/browser/ui/startup/session_crashed_prompt.cc:19: #include
"content/public/browser/dom_storage_context.h"
On 2012/07/11 15:51:35, John Abd-El-Malek wrote:
> nit: order

Done.

http://codereview.chromium.org/9963107/diff/158010/chrome/browser/ui/startup/...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

http://codereview.chromium.org/9963107/diff/158010/chrome/browser/ui/startup/...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:74: #include
"content/public/browser/browser_context.h"
On 2012/07/11 15:51:35, John Abd-El-Malek wrote:
> nit: ditto

Done.

http://codereview.chromium.org/9963107/diff/158010/content/browser/browser_co...
File content/browser/browser_context.cc (right):

http://codereview.chromium.org/9963107/diff/158010/content/browser/browser_co...
content/browser/browser_context.cc:90:
context->ShouldSaveSessionStorageOnDisk());
On 2012/07/11 21:40:28, John Abd-El-Malek wrote:
> On 2012/07/11 17:52:16, jochen wrote:
> > Cookies are a bad role model here: in contrast to all other storage APIs,
the
> > embedder has to provide a persistent store for cookies.
> > 
> > Everything else persists stuff to disk iff the browser context has a path
> > defined.
> > 
> > The situation here is that an embedder might want to provide a path to have
> > regular data stored on disk, but would prefer not to persist sessionStorage.
> > This is different from the flag that is passed to the cookie monster (or the
> > other storage systems) that just controls how shutdown/startup is handled,
but
> > doesn't change whether a file backend is created or not.
> > 
> > The problem with setting this flag on the DOM storage context directly is
that
> > it needs to be passed to the constructor. The DOM storage context, however,
is
> > created lazily. The embedder would need to invoke this method early enough,
> > requiring detailed information on how the browser context is implemented.
> 
> I see. It seems that this can be changed so that it's not required at
> construction, but just has to be set before it's used? i.e. it's the embedder
> who creates a BrowserContext, so if they want to set this, they can do so soon
> after before they use it.

Done.

http://codereview.chromium.org/9963107/diff/158010/content/browser/dom_storag...
File content/browser/dom_storage/dom_storage_context_impl.cc (right):

http://codereview.chromium.org/9963107/diff/158010/content/browser/dom_storag...
content/browser/dom_storage/dom_storage_context_impl.cc:97: DCHECK(context_);
On 2012/07/11 15:51:35, John Abd-El-Malek wrote:
> nit: dcheck is unnecessary, if it's NULL, the next line will be just as good
of
> a signal

In all other funcs in this file there is DCHECK(context_) -> kept this.

[sail, 2012-07-12 17:20:04]

LGTM

http://codereview.chromium.org/9963107/diff/168029/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/9963107/diff/168029/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:306:
content::BrowserContext::GetDOMStorageContext(this)->
I guess this is ok. I still think this doesn't have anything to do with profiles
and shouldn't be in this class.

Couldn't you just add something to the store code? Something like:
  if (!my_profile->is_incognito())
    SetSaveSessionStorageOnDisk();

[jam, 2012-07-12 18:27:38]

lgtm with nits

http://codereview.chromium.org/9963107/diff/168029/content/browser/dom_storag...
File content/browser/dom_storage/dom_storage_context_impl.cc (right):

http://codereview.chromium.org/9963107/diff/168029/content/browser/dom_storag...
content/browser/dom_storage/dom_storage_context_impl.cc:58: data_path :
data_path.AppendASCII(kSessionStorageDirectory),
nit: indent

http://codereview.chromium.org/9963107/diff/168029/webkit/dom_storage/dom_sto...
File webkit/dom_storage/dom_storage_context.cc (right):

http://codereview.chromium.org/9963107/diff/168029/webkit/dom_storage/dom_sto...
webkit/dom_storage/dom_storage_context.cc:291: DCHECK(success);
nit: i would remove all the DCHECK's on the result of Post*Task. this is an
anti-pattern since even if this function returns true, there's no guarantee that
the task will execute

[michaeln, 2012-07-12 21:09:15]

still lgtm, even with the minor content api'isms

https://chromiumcodereview.appspot.com/9963107/diff/168029/content/public/bro...
File content/public/browser/dom_storage_context.h (right):

https://chromiumcodereview.appspot.com/9963107/diff/168029/content/public/bro...
content/public/browser/dom_storage_context.h:38: virtual void
SetSaveSessionStorageOnDisk() = 0;
This works but its an awkward method with that must be called after creating and
before using and such. An alternative could be to hang a new method on either
ContentClient or ContentBrowserClient which can be overriden by chrome to return
true (and false by say ContentShell), and have DomStorageContextImpl.cc query
that interface during construction/initialzation. That would take it out of the
'profile' world (but we'd also loose the ability to toggle this on/off
per-profile).

[marja, 2012-07-13 09:05:17]

Thanks for review!

pkasting, a friendly ping, your OWNERS review is the only one missing...

http://codereview.chromium.org/9963107/diff/168029/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/9963107/diff/168029/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:306:
content::BrowserContext::GetDOMStorageContext(this)->
On 2012/07/12 17:20:04, sail wrote:
> I guess this is ok. I still think this doesn't have anything to do with
profiles
> and shouldn't be in this class.
> 
> Couldn't you just add something to the store code? Something like:
>   if (!my_profile->is_incognito())
>     SetSaveSessionStorageOnDisk();

The dom storage layer doesn't know about profiles, the code is in content/ and
in webkit/.

Also, we wanted to make it so that some content/ embedders can say "session
storage should be saved on disk" (e.g., chrome, which does session restore) and
some can say "session storage shouldn't be saved on disk".

(However, see the comment where michaeln suggests putting the decision to
ContentClient or ContentBrowserClient.)

http://codereview.chromium.org/9963107/diff/168029/content/browser/dom_storag...
File content/browser/dom_storage/dom_storage_context_impl.cc (right):

http://codereview.chromium.org/9963107/diff/168029/content/browser/dom_storag...
content/browser/dom_storage/dom_storage_context_impl.cc:58: data_path :
data_path.AppendASCII(kSessionStorageDirectory),
On 2012/07/12 18:27:38, John Abd-El-Malek wrote:
> nit: indent

Done.

http://codereview.chromium.org/9963107/diff/168029/content/public/browser/dom...
File content/public/browser/dom_storage_context.h (right):

http://codereview.chromium.org/9963107/diff/168029/content/public/browser/dom...
content/public/browser/dom_storage_context.h:38: virtual void
SetSaveSessionStorageOnDisk() = 0;
On 2012/07/12 21:09:15, michaeln wrote:
> This works but its an awkward method with that must be called after creating
and
> before using and such. An alternative could be to hang a new method on either
> ContentClient or ContentBrowserClient which can be overriden by chrome to
return
> true (and false by say ContentShell), and have DomStorageContextImpl.cc query
> that interface during construction/initialzation. That would take it out of
the
> 'profile' world (but we'd also loose the ability to toggle this on/off
> per-profile).

Yes, okay, that would work. I can do that in a separate CL if we agree that
that's the right way to do this. jam, wdyt?

http://codereview.chromium.org/9963107/diff/168029/webkit/dom_storage/dom_sto...
File webkit/dom_storage/dom_storage_context.cc (right):

http://codereview.chromium.org/9963107/diff/168029/webkit/dom_storage/dom_sto...
webkit/dom_storage/dom_storage_context.cc:291: DCHECK(success);
On 2012/07/12 18:27:38, John Abd-El-Malek wrote:
> nit: i would remove all the DCHECK's on the result of Post*Task. this is an
> anti-pattern since even if this function returns true, there's no guarantee
that
> the task will execute

Deleted the ones added by this CL, didn't touch the ones already there.

[jam, 2012-07-13 17:42:56]

http://codereview.chromium.org/9963107/diff/168029/content/public/browser/dom...
File content/public/browser/dom_storage_context.h (right):

http://codereview.chromium.org/9963107/diff/168029/content/public/browser/dom...
content/public/browser/dom_storage_context.h:38: virtual void
SetSaveSessionStorageOnDisk() = 0;
On 2012/07/12 21:09:15, michaeln wrote:
> This works but its an awkward method with that must be called after creating
and
> before using and such. An alternative could be to hang a new method on either
> ContentClient or ContentBrowserClient which can be overriden by chrome to
return
> true (and false by say ContentShell), and have DomStorageContextImpl.cc query
> that interface during construction/initialzation. That would take it out of
the
> 'profile' world (but we'd also loose the ability to toggle this on/off
> per-profile).

ContentBrowserClient is a last-resort interface, where there's no context and so
content calls the embedder's "static" interface.

Given that the embedder is the one that creates the BrowserContext, I don't
think there's an issue if they have to set advanced flags on it that almost no
other embedder of content will care about. This keeps the interfaces that every
embedder has to implement simpler (i.e. ContentBrowserClient or BrowserContext).

[michaeln, 2012-07-16 17:19:25]

ok, sgtm

> ContentBrowserClient is a last-resort interface, where there's no context and
so
> content calls the embedder's "static" interface.
> 
> Given that the embedder is the one that creates the BrowserContext, I don't
> think there's an issue if they have to set advanced flags on it that almost no
> other embedder of content will care about. This keeps the interfaces that
every
> embedder has to implement simpler (i.e. ContentBrowserClient or
BrowserContext).

[Peter Kasting, 2012-07-16 17:41:02]

https://chromiumcodereview.appspot.com/9963107/diff/150016/chrome/browser/ui/...
File chrome/browser/ui/startup/session_crashed_prompt.cc (right):

https://chromiumcodereview.appspot.com/9963107/diff/150016/chrome/browser/ui/...
chrome/browser/ui/startup/session_crashed_prompt.cc:107: switch (type) {
Nit: Do a DCHECK_EQ(chrome::NOTIFICATION_TAB_CONTENTS_INFOBAR_REMOVED, type);
instead of this switch

https://chromiumcodereview.appspot.com/9963107/diff/150016/chrome/browser/ui/...
chrome/browser/ui/startup/session_crashed_prompt.cc:111: if (!accepted_) {
You're running this code when any infobar is closed.  You need to check that the
notification is for the infobar you care about.

Also, since you're adding this observer in the delegate itself, you need to
handle both the case where you get this notification and the case where you're
destroyed before you get it, since either one might happen.  Also add a TODO
comment that you can safely eliminate the listener and rely on reaching your
destructor once InfoBars own their delegates.

https://chromiumcodereview.appspot.com/9963107/diff/150016/chrome/browser/ui/...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://chromiumcodereview.appspot.com/9963107/diff/150016/chrome/browser/ui/...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:629:
content::DOMStorageContext* dom_storage_context =
Nit: Could also inline into next line and break after '->'

[marja, 2012-08-06 11:53:32]

pkasting, thanks for comments!

https://chromiumcodereview.appspot.com/9963107/diff/150016/chrome/browser/ui/...
File chrome/browser/ui/startup/session_crashed_prompt.cc (right):

https://chromiumcodereview.appspot.com/9963107/diff/150016/chrome/browser/ui/...
chrome/browser/ui/startup/session_crashed_prompt.cc:107: switch (type) {
On 2012/07/16 17:41:02, Peter Kasting wrote:
> Nit: Do a DCHECK_EQ(chrome::NOTIFICATION_TAB_CONTENTS_INFOBAR_REMOVED, type);
> instead of this switch

Done.

https://chromiumcodereview.appspot.com/9963107/diff/150016/chrome/browser/ui/...
chrome/browser/ui/startup/session_crashed_prompt.cc:111: if (!accepted_) {
On 2012/07/16 17:41:02, Peter Kasting wrote:
> You're running this code when any infobar is closed.  You need to check that
the
> notification is for the infobar you care about.
> 
> Also, since you're adding this observer in the delegate itself, you need to
> handle both the case where you get this notification and the case where you're
> destroyed before you get it, since either one might happen.  Also add a TODO
> comment that you can safely eliminate the listener and rely on reaching your
> destructor once InfoBars own their delegates.

Ahh, thanks for pointing that out.

Done.

https://chromiumcodereview.appspot.com/9963107/diff/150016/chrome/browser/ui/...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://chromiumcodereview.appspot.com/9963107/diff/150016/chrome/browser/ui/...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:629:
content::DOMStorageContext* dom_storage_context =
On 2012/07/16 17:41:02, Peter Kasting wrote:
> Nit: Could also inline into next line and break after '->'

Done.

[Peter Kasting, 2012-08-07 01:31:54]

LGTM

https://chromiumcodereview.appspot.com/9963107/diff/179002/chrome/browser/ses...
File chrome/browser/sessions/session_restore.cc (right):

https://chromiumcodereview.appspot.com/9963107/diff/179002/chrome/browser/ses...
chrome/browser/sessions/session_restore.cc:738: content::DOMStorageContext*
dom_storage_context =
Nit: I'd probably just do:

  content::BrowserContext::GetDefaultDOMStorageContext(profile_)->
      StartScavengingUnusedSessionStorage();

(2 places)

https://chromiumcodereview.appspot.com/9963107/diff/179002/chrome/browser/ui/...
File chrome/browser/ui/startup/session_crashed_prompt.cc (right):

https://chromiumcodereview.appspot.com/9963107/diff/179002/chrome/browser/ui/...
chrome/browser/ui/startup/session_crashed_prompt.cc:44: //
content::NotificationObserver implementation.
Nit: s/ implementation./:/ for consistency

https://chromiumcodereview.appspot.com/9963107/diff/179002/chrome/browser/ui/...
chrome/browser/ui/startup/session_crashed_prompt.cc:75:
content::DOMStorageContext* dom_storage_context =
Nit: I would do:

    content::BrowserContext::GetDefaultDOMStorageContext(browser_->profile())->
        StartScavengingUnusedSessionStorage();

(2 places)

https://chromiumcodereview.appspot.com/9963107/diff/179002/chrome/browser/ui/...
chrome/browser/ui/startup/session_crashed_prompt.cc:121:
content::Details<std::pair<InfoBarDelegate*, bool> > det(details);
Nit: |det| is not a good variable name, and you should be using the typedef for
this:

  if (content::Details<InfoBarRemovedDetails>(details)->first != this)

https://chromiumcodereview.appspot.com/9963107/diff/179002/chrome/browser/ui/...
chrome/browser/ui/startup/session_crashed_prompt.cc:124: // See
~SessionCrashedInfoBarDelegate and the TODO in
Nit: No need for this comment, the TODO in the constructor is sufficient on its
own

https://chromiumcodereview.appspot.com/9963107/diff/179002/chrome/common/chro...
File chrome/common/chrome_switches.cc (right):

https://chromiumcodereview.appspot.com/9963107/diff/179002/chrome/common/chro...
chrome/common/chrome_switches.cc:612: // Enables experimental features of better
session restore. See also
Nit: This comment is vague ("experimental features"?).

Can we get by without a flag at all?  Seems like you could just check this in
enabled, and go back and revisit if there are problems.

[marja, 2012-08-07 07:31:22]

Thanks for reviewing!

https://chromiumcodereview.appspot.com/9963107/diff/179002/chrome/browser/ses...
File chrome/browser/sessions/session_restore.cc (right):

https://chromiumcodereview.appspot.com/9963107/diff/179002/chrome/browser/ses...
chrome/browser/sessions/session_restore.cc:738: content::DOMStorageContext*
dom_storage_context =
On 2012/08/07 01:31:54, Peter Kasting wrote:
> Nit: I'd probably just do:
> 
>   content::BrowserContext::GetDefaultDOMStorageContext(profile_)->
>       StartScavengingUnusedSessionStorage();
> 
> (2 places)

Done.

https://chromiumcodereview.appspot.com/9963107/diff/179002/chrome/browser/ui/...
File chrome/browser/ui/startup/session_crashed_prompt.cc (right):

https://chromiumcodereview.appspot.com/9963107/diff/179002/chrome/browser/ui/...
chrome/browser/ui/startup/session_crashed_prompt.cc:44: //
content::NotificationObserver implementation.
On 2012/08/07 01:31:54, Peter Kasting wrote:
> Nit: s/ implementation./:/ for consistency

Done.

https://chromiumcodereview.appspot.com/9963107/diff/179002/chrome/browser/ui/...
chrome/browser/ui/startup/session_crashed_prompt.cc:75:
content::DOMStorageContext* dom_storage_context =
On 2012/08/07 01:31:54, Peter Kasting wrote:
> Nit: I would do:
> 
>    
content::BrowserContext::GetDefaultDOMStorageContext(browser_->profile())->
>         StartScavengingUnusedSessionStorage();
> 
> (2 places)

Done.

https://chromiumcodereview.appspot.com/9963107/diff/179002/chrome/browser/ui/...
chrome/browser/ui/startup/session_crashed_prompt.cc:121:
content::Details<std::pair<InfoBarDelegate*, bool> > det(details);
On 2012/08/07 01:31:54, Peter Kasting wrote:
> Nit: |det| is not a good variable name, and you should be using the typedef
for
> this:
> 
>   if (content::Details<InfoBarRemovedDetails>(details)->first != this)

Done.

https://chromiumcodereview.appspot.com/9963107/diff/179002/chrome/browser/ui/...
chrome/browser/ui/startup/session_crashed_prompt.cc:124: // See
~SessionCrashedInfoBarDelegate and the TODO in
On 2012/08/07 01:31:54, Peter Kasting wrote:
> Nit: No need for this comment, the TODO in the constructor is sufficient on
its
> own

Done.

https://chromiumcodereview.appspot.com/9963107/diff/179002/chrome/common/chro...
File chrome/common/chrome_switches.cc (right):

https://chromiumcodereview.appspot.com/9963107/diff/179002/chrome/common/chro...
chrome/common/chrome_switches.cc:612: // Enables experimental features of better
session restore. See also
On 2012/08/07 01:31:54, Peter Kasting wrote:
> Nit: This comment is vague ("experimental features"?).
> 
> Can we get by without a flag at all?  Seems like you could just check this in
> enabled, and go back and revisit if there are problems.

We agreed to put the features first behind the flag so it's easy to disable the
features with a small change (e.g., reverting the CL which turns the feature on
by default). This sessionStorage feature is the experimentalest of them all.

I changed the comment though to mention sessionStorage since it's the only (and
the last) experimental feature this flag will affect.

[commit-bot: I haz the power, 2012-08-07 07:33:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/9963107/174003

[commit-bot: I haz the power, 2012-08-07 09:23:54]

Change committed as 150314

[michaeln, 2012-08-08 22:16:02]

On 2012/08/07 09:23:54, I haz the power (commit-bot) wrote:
> Change committed as 150314

Great to see this get committed!