Create and store persistent unique ids for sessionStorage.

Create and store persistent unique ids for sessionStorage.

This helps us to store / restore sessionStorages when doing a session
restore (in a later CL).

BUG=104292
TEST=existing tests+DomStorageContextTest.PersistentIds


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

[marja, 2012-06-14 14:20:43]

This is the first CL towards file-backed sessionStorage. It creates and
maintains persistent IDs to DomStorageNamespaces & -Areas for sessionStorage.

The big picture is here: http://codereview.chromium.org/9963107/

And this CL requires http://codereview.chromium.org/10540003/ to work (it's
getting committed atm).

Could you review the following:
michaeln: all
brettw: webkit/tools/test_shell/simple_dom_storage_system.cc (trivial additions)

[marja, 2012-06-14 14:29:40]

Hmm, sorry, I meant tony instead of brettw.

So:
michaeln: all
tony: webkit/tools/test_shell/simple_dom_storage_system.cc (trivial additions)

[tony, 2012-06-14 18:11:32]

webkit/tools LGTM.

[tony, 2012-06-14 18:11:40]

http://codereview.chromium.org/10546167/diff/6001/webkit/tools/test_shell/sim...
File webkit/tools/test_shell/simple_dom_storage_system.cc (right):

http://codereview.chromium.org/10546167/diff/6001/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_dom_storage_system.cc:110:
Context()->CloneSessionNamespace(namespace_id_, new_id, "");
Nit: std::string()

http://codereview.chromium.org/10546167/diff/6001/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_dom_storage_system.cc:218:
context_->CreateSessionNamespace(id, "");
Nit: std::string()

[michaeln, 2012-06-14 22:53:48]

https://chromiumcodereview.appspot.com/10546167/diff/6001/content/browser/dom...
File content/browser/dom_storage/session_storage_namespace_impl.h (right):

https://chromiumcodereview.appspot.com/10546167/diff/6001/content/browser/dom...
content/browser/dom_storage/session_storage_namespace_impl.h:23: explicit
SessionStorageNamespaceImpl(DOMStorageContextImpl* context);
ditto comments on ctor's might be helpful

https://chromiumcodereview.appspot.com/10546167/diff/6001/content/browser/dom...
content/browser/dom_storage/session_storage_namespace_impl.h:29: virtual
std::string persistent_id() const OVERRIDE;
ditto const ref question

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

https://chromiumcodereview.appspot.com/10546167/diff/6001/content/public/brow...
content/public/browser/session_storage_namespace.h:17: // NOTE: That if we're
shutting down, we don't strictly need to do this, but
while your here, could you remove this non-informative NOTE comment

https://chromiumcodereview.appspot.com/10546167/diff/6001/content/public/brow...
content/public/browser/session_storage_namespace.h:23: virtual std::string
persistent_id() const = 0;
can the return value be of type const std::string&

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

https://chromiumcodereview.appspot.com/10546167/diff/6001/webkit/dom_storage/...
webkit/dom_storage/dom_storage_area.cc:82: map_(new
DomStorageMap(kPerAreaQuota)),
per some more recent changes this should be
(kPerAreaQuota + kPerAreaOverQuotaAllowance)

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

https://chromiumcodereview.appspot.com/10546167/diff/6001/webkit/dom_storage/...
webkit/dom_storage/dom_storage_area.h:35: DomStorageArea(int64 namespace_id,
Can we remove the namespace_id param from this ctor since its only valid to call
with the kLocalStorageNamespaceId value?

https://chromiumcodereview.appspot.com/10546167/diff/6001/webkit/dom_storage/...
File webkit/dom_storage/dom_storage_namespace.h (right):

https://chromiumcodereview.appspot.com/10546167/diff/6001/webkit/dom_storage/...
webkit/dom_storage/dom_storage_namespace.h:39: std::string
persistent_namespace_id() const {
could the return value here be const string&

https://chromiumcodereview.appspot.com/10546167/diff/6001/webkit/dom_storage/...
File webkit/dom_storage/dom_storage_session.h (right):

https://chromiumcodereview.appspot.com/10546167/diff/6001/webkit/dom_storage/...
webkit/dom_storage/dom_storage_session.h:25: explicit
DomStorageSession(DomStorageContext* context);
The constructors for this are not so obvious (here and in the content library).
Might help to add a comments on each ctor about the behavior|intended_use.

https://chromiumcodereview.appspot.com/10546167/diff/6001/webkit/dom_storage/...
webkit/dom_storage/dom_storage_session.h:29: std::string
persistent_namespace_id() const {
ditto const ref question

[marja, 2012-06-15 09:17:42]

Thanks for comments!

http://codereview.chromium.org/10546167/diff/6001/content/browser/dom_storage...
File content/browser/dom_storage/session_storage_namespace_impl.h (right):

http://codereview.chromium.org/10546167/diff/6001/content/browser/dom_storage...
content/browser/dom_storage/session_storage_namespace_impl.h:23: explicit
SessionStorageNamespaceImpl(DOMStorageContextImpl* context);
On 2012/06/14 22:53:48, michaeln wrote:
> ditto comments on ctor's might be helpful

Done.

http://codereview.chromium.org/10546167/diff/6001/content/browser/dom_storage...
content/browser/dom_storage/session_storage_namespace_impl.h:29: virtual
std::string persistent_id() const OVERRIDE;
On 2012/06/14 22:53:48, michaeln wrote:
> ditto const ref question

Done.

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

http://codereview.chromium.org/10546167/diff/6001/content/public/browser/sess...
content/public/browser/session_storage_namespace.h:17: // NOTE: That if we're
shutting down, we don't strictly need to do this, but
On 2012/06/14 22:53:48, michaeln wrote:
> while your here, could you remove this non-informative NOTE comment

Done.

http://codereview.chromium.org/10546167/diff/6001/content/public/browser/sess...
content/public/browser/session_storage_namespace.h:23: virtual std::string
persistent_id() const = 0;
On 2012/06/14 22:53:48, michaeln wrote:
> can the return value be of type const std::string&

Done.

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

http://codereview.chromium.org/10546167/diff/6001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.cc:82: map_(new
DomStorageMap(kPerAreaQuota)),
On 2012/06/14 22:53:48, michaeln wrote:
> per some more recent changes this should be
> (kPerAreaQuota + kPerAreaOverQuotaAllowance)

Done.

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

http://codereview.chromium.org/10546167/diff/6001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.h:35: DomStorageArea(int64 namespace_id,
On 2012/06/14 22:53:48, michaeln wrote:
> Can we remove the namespace_id param from this ctor since its only valid to
call
> with the kLocalStorageNamespaceId value?

Done.

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

http://codereview.chromium.org/10546167/diff/6001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_namespace.h:39: std::string
persistent_namespace_id() const {
On 2012/06/14 22:53:48, michaeln wrote:
> could the return value here be const string&

I think so; it would be invalid if the string was used after the
DomStorageNamespace is deleted, but I don't think that would easily happen.

(const references keep temporaries alive, but in this case it wouldn't keep the
string which is|was a member variable alive any longer... right?)

http://codereview.chromium.org/10546167/diff/6001/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_session.cc (right):

http://codereview.chromium.org/10546167/diff/6001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_session.cc:45: std::string persistent_clone_id =
context->AllocatePersistentSessionId();
(This could also be const std::string& persistent_clone_id = ...; since a const
reference is enough to keep the temporary alive. But some code reviewers dislike
it, so I didn't make it a const ref.)

http://codereview.chromium.org/10546167/diff/6001/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_session.h (right):

http://codereview.chromium.org/10546167/diff/6001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_session.h:25: explicit
DomStorageSession(DomStorageContext* context);
On 2012/06/14 22:53:48, michaeln wrote:
> The constructors for this are not so obvious (here and in the content
library).
> Might help to add a comments on each ctor about the behavior|intended_use.

Done. (In the header.)

http://codereview.chromium.org/10546167/diff/6001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_session.h:29: std::string
persistent_namespace_id() const {
On 2012/06/14 22:53:48, michaeln wrote:
> ditto const ref question

Done.

http://codereview.chromium.org/10546167/diff/6001/webkit/tools/test_shell/sim...
File webkit/tools/test_shell/simple_dom_storage_system.cc (right):

http://codereview.chromium.org/10546167/diff/6001/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_dom_storage_system.cc:110:
Context()->CloneSessionNamespace(namespace_id_, new_id, "");
On 2012/06/14 18:11:40, tony wrote:
> Nit: std::string()

Done.

http://codereview.chromium.org/10546167/diff/6001/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_dom_storage_system.cc:218:
context_->CreateSessionNamespace(id, "");
On 2012/06/14 18:11:40, tony wrote:
> Nit: std::string()

Done.

[jochen (gone - plz use gerrit), 2012-06-15 13:57:21]

drive-by content nits

http://codereview.chromium.org/10546167/diff/2003/content/browser/dom_storage...
File content/browser/dom_storage/session_storage_namespace_impl.cc (right):

http://codereview.chromium.org/10546167/diff/2003/content/browser/dom_storage...
content/browser/dom_storage/session_storage_namespace_impl.cc:27:
persistent_id)) {
fits on previous line?

http://codereview.chromium.org/10546167/diff/2003/content/browser/dom_storage...
File content/browser/dom_storage/session_storage_namespace_impl.h (right):

http://codereview.chromium.org/10546167/diff/2003/content/browser/dom_storage...
content/browser/dom_storage/session_storage_namespace_impl.h:35: virtual int64
id() const OVERRIDE;
can you move the virtual methods in a separate block and prefix them with a
comment like // SessionStorageNamespace implementation?

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

http://codereview.chromium.org/10546167/diff/2003/content/public/browser/sess...
content/public/browser/session_storage_namespace.h:20: virtual int64 id() const
= 0;
can you please document these two getters?

also, currently you're not using them outside of content/. Will that change?
Otherwise, the getters should not be on the public interface.

http://codereview.chromium.org/10546167/diff/2003/content/public/browser/sess...
content/public/browser/session_storage_namespace.h:22: protected:
empty line before protected

http://codereview.chromium.org/10546167/diff/2003/content/public/browser/sess...
content/public/browser/session_storage_namespace.h:25: };
disallow copy/assign

[marja, 2012-06-15 14:31:32]

http://codereview.chromium.org/10546167/diff/2003/content/browser/dom_storage...
File content/browser/dom_storage/session_storage_namespace_impl.cc (right):

http://codereview.chromium.org/10546167/diff/2003/content/browser/dom_storage...
content/browser/dom_storage/session_storage_namespace_impl.cc:27:
persistent_id)) {
On 2012/06/15 13:57:21, jochen wrote:
> fits on previous line?

Done.

http://codereview.chromium.org/10546167/diff/2003/content/browser/dom_storage...
File content/browser/dom_storage/session_storage_namespace_impl.h (right):

http://codereview.chromium.org/10546167/diff/2003/content/browser/dom_storage...
content/browser/dom_storage/session_storage_namespace_impl.h:35: virtual int64
id() const OVERRIDE;
On 2012/06/15 13:57:21, jochen wrote:
> can you move the virtual methods in a separate block and prefix them with a
> comment like // SessionStorageNamespace implementation?

Done.

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

http://codereview.chromium.org/10546167/diff/2003/content/public/browser/sess...
content/public/browser/session_storage_namespace.h:20: virtual int64 id() const
= 0;
On 2012/06/15 13:57:21, jochen wrote:
> can you please document these two getters?
> 
> also, currently you're not using them outside of content/. Will that change?
> Otherwise, the getters should not be on the public interface.

Added docs: 

// Returns the ID of the |SessionStorageNamespace|. The ID is unique among all
// SessionStorageNamespace objects, but not unique across browser runs.

// Returns the persistent ID for the |SessionStorageNamespace|. The ID is
// unique across browser runs.

They'll be used outside content.

http://codereview.chromium.org/10546167/diff/2003/content/public/browser/sess...
content/public/browser/session_storage_namespace.h:22: protected:
On 2012/06/15 13:57:21, jochen wrote:
> empty line before protected

Done.

http://codereview.chromium.org/10546167/diff/2003/content/public/browser/sess...
content/public/browser/session_storage_namespace.h:25: };
On 2012/06/15 13:57:21, jochen wrote:
> disallow copy/assign

Done.

[michaeln, 2012-06-15 18:29:51]

lgtm

https://chromiumcodereview.appspot.com/10546167/diff/17001/content/public/bro...
File content/public/browser/session_storage_namespace.h (right):

https://chromiumcodereview.appspot.com/10546167/diff/17001/content/public/bro...
content/public/browser/session_storage_namespace.h:21: SessionStorageNamespace()
{}
is this ctor necessary?

[marja, 2012-06-18 07:08:30]

Thanks for review!

https://chromiumcodereview.appspot.com/10546167/diff/17001/content/public/bro...
File content/public/browser/session_storage_namespace.h (right):

https://chromiumcodereview.appspot.com/10546167/diff/17001/content/public/bro...
content/public/browser/session_storage_namespace.h:21: SessionStorageNamespace()
{}
On 2012/06/15 18:29:51, michaeln wrote:
> is this ctor necessary?

Yes, because of the DISALLOW_COPY_AND_ASSIGN.

[commit-bot: I haz the power, 2012-06-18 07:08:46]

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

[commit-bot: I haz the power, 2012-06-18 07:08:52]

Presubmit check for 10546167-17001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/public/browser

Presubmit checks took 2.0s to calculate.

[marja, 2012-06-18 07:15:14]

Oops, my set cover algorithm to find the reviewers failed.

avi, could you review the following:
content/public/browser/session_storage_namespace.h

[Avi (use Gerrit), 2012-06-18 14:40:14]

lgtm

https://chromiumcodereview.appspot.com/10546167/diff/17001/content/public/bro...
File content/public/browser/session_storage_namespace.h (right):

https://chromiumcodereview.appspot.com/10546167/diff/17001/content/public/bro...
content/public/browser/session_storage_namespace.h:26: // Returns the persistent
ID for the |SessionStorageNamespace|. The ID is
Blank line above this comment.

[marja, 2012-06-18 15:00:11]

Thanks!

https://chromiumcodereview.appspot.com/10546167/diff/17001/content/public/bro...
File content/public/browser/session_storage_namespace.h (right):

https://chromiumcodereview.appspot.com/10546167/diff/17001/content/public/bro...
content/public/browser/session_storage_namespace.h:26: // Returns the persistent
ID for the |SessionStorageNamespace|. The ID is
On 2012/06/18 14:40:14, Avi wrote:
> Blank line above this comment.

Done.

[commit-bot: I haz the power, 2012-06-18 15:00:22]

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

[commit-bot: I haz the power, 2012-06-18 16:52:47]

Change committed as 142735

[marja, 2012-06-20 16:45:25]

jochen: fyi

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

http://codereview.chromium.org/10546167/diff/2003/content/public/browser/sess...
content/public/browser/session_storage_namespace.h:25: };
On 2012/06/15 14:31:32, marja wrote:
> On 2012/06/15 13:57:21, jochen wrote:
> > disallow copy/assign
> 
> Done.

michaeln says here https://chromiumcodereview.appspot.com/10572015/ that we
shouldn't DISALLOW_.. for abstract classes. This class has no member variables
either, so conceptually copying/assigning it is a no op, and we could just allow
it..