Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10176005:
Add dom_storage::SessionStorageDatabase. (Closed)
|
Created:
8 years, 8 months ago by marja Modified:
8 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, jochen (gone - plz use gerrit) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd dom_storage::SessionStorageDatabase.
BUG=104292
TEST=SessionStorageDatabaseTest.*
TBR=tony@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=134259
Patch Set 1 #Patch Set 2 : Code review, more tests, fixes. #Patch Set 3 : . #Patch Set 4 : . #Patch Set 5 : . #Patch Set 6 : Error handling, more tests. #Patch Set 7 : More tests. #Patch Set 8 : . #
Total comments: 43
Patch Set 9 : Code review. #Patch Set 10 : . #Patch Set 11 : . #
Total comments: 41
Patch Set 12 : Code review. #
Total comments: 4
Patch Set 13 : Code review. #
Messages
Total messages: 12 (0 generated)
This cl adds SessionStorageDatabase and tests without integrating it to DomStorageArea yet. Michael, could you have a look?
The .cc file is a tough read in general. The test demonstrates that your schema/code works but the code part is challenging to follow. http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... File webkit/dom_storage/session_storage_database.cc (right): http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:40: SessionStorageDatabase::SessionStorageDatabase(const FilePath& file_path) Might be helpful to distinguish between namespace_ids with the offset applied and those w/o in param/var naming. It's easy to lose track of which flavor of id is sitting in any param or local var. Also, how confident are you of this offsetting scheme? Another option is to leave that indirection out in this initial CL. More consistently using int64s for mapids and namespaceids for method params instead of sometimes having the string form and other times having an int form might be helpful. Some of the methods are tricky to follow due to dropping down into leveldb primitives where its all prefixed string keys with prefixed string values. The high level logic easily gets lost in there. I did some noodling with a few of the public methods to see what set of private helpers might with readability. Please see the pseudo code noodling comments as food for thought. GetMapForArea(namespace_id, origin, &mapid); GetOrCreateMapForArea(namespace_id, origin, &map_id); SetMapForArea(namespace_id, origin, mapid); DeleteAreaFromNamespace(namespace_id, origin); CreateNamespaceKey(namespace_id); DeleteNamespaceKey(namespace_id); CreateMap(&new_map_id); GetMapRefCount(map_id, &count); SetMapRefCount(map_id, count); DeleteMap(map_id); std::vector<std::pair<GURL, int64> > areas; GetAreasInNamespace(namespace_id, &areas); AddAreaToNamespace(new_namespace_id, origin, mapid); http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:65: ReadMap(map_id, result, false); pseudo code noodling... if (!LazyOpen(false)) return; int64 map_id; if (!GetMapForArea(namespace_id, origin, &map_id)) return; ReadMap(map_id, result, false); http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:71: const ValuesMap& changes) { pseudo code noodling... if (!LazyOpen(true)) return false; leveldb::WriteBatch batch; int64 map_id; if (!GetOrCreateMapForArea(namespace_id, origin, &map_id, &batch)) return false; DCHECK(1, GetMapRefCount(map_id)) << "shouldn't be mutating a shared map"; if (clear_all_first) { if (!ClearMap(map_id, &batch)) return false; } WriteValuesToMap(map_id, changes, &batch); return db_->Write(leveldb::WriteOptions(), &batch).ok(); http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:81: batch.Put(NamespaceStartKey(namespace_id, namespace_offset_), ""); would be nice to not overwrite existing entries for these key on each call http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:97: // We shouldn't write data to a shallow copy. should we dcheck this condition instead? http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:124: int64 new_namespace_id) { pseudo code noodling... WriteBatch batch; // setup required entries for a namespace ensuring its really new if (!CreateNamespaceKey(new_namespace_id, &batch) return false; std::vector<std::pair<GURL, int64> >* areas; if (!GetAreasInNamespace(namespace_id, &areas)) return false; for_each(area) { SetMapRefCount(mapid, GetMapRefCount(map_id) + 1, &batch); AddAreaToNamespace(new_namespace_id, origin, mapid); } return batch.Write(); http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:190: bool SessionStorageDatabase::DeepCopy(int64 namespace_id, const GURL& origin) { pseudo code noodling... if (!GetMapForArea(namespace_id, origin, &old_map_id)) return false; ReadMap(old_map_id, &values); WriteBatch batch; CreateMap(&new_map_id, &batch); // created with refcount of 1 WriteMap(new_map_id, values); SetMapForArea(namespace_id, origin, new_map_id); int old_refcount = GetMapRefCount(old_map_id); DCHECK(old_refcount > 1); SetMapRefCount(old_map_id, old_refcount - 1, &batch); http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:223: // If this copy is the the only "shallow" copy of the map, no deep copying is Is there any case where the higher level code would call to DeepCopy an unshared map? Would it make sense to DCHECK this instead of handling it? http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:249: const GURL& origin) { ResetArea(id, origin) pseudo code noodling... int64 map_id; GetMapForArea(id, origin, &map_id); DeleteAreaFromNamespace(id, origin); int refcount; GetMapRefCount(map_id, &refcount); if (refcount == 1) DeleteMap(map_id); else SetMapRefCount(map_id, refcount - 1); http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:284: bool SessionStorageDatabase::DeleteNamespace(int64 namespace_id) { pseudo code noodling... std::vector<std::pair<GURL, int64> > areas; if (!GetAreasInNamespace(namespace_id, &areas)) return false; for_each(area) { int refcount = GetMapRefCount(area.mapid); if (refcount == 1) DeleteMap(mapid); else SetMapRefCount(refcount - 1); DeleteAreaFromNamespace(namespace_id, area.origin); } DeleteNamespaceKey(namespace_id); http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:366: leveldb::Status s = db_->Write(leveldb::WriteOptions(), &batch); I think this results in an empty session storage database. In the first loop, all namespace- prefixed keys are deleted and every map ref is tallied for release. In the second loop, all map- prefixed keys are deleted for any map that was referred to by a namespace (which i think should be all maps). Is that right? Is that the desired behavior? As mentioned in the .h file, maybe defer the addition of this method for later once the desired behavior is better understood. Or maybe put a simpler class method in for now... void SessionStorageDatabase::ClearAll() { for_each(key) batch->Delete(key); } http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:449: file_util::Delete(file_path_, true); This doesn't look like a viable error handling strategy. On some systems the delete will fail if because the files are open by db_, also given that one thread can be reading while another is writing, its dicey to delete these files w/o greater coordination. To start with, i think you could just return false here w/o actually doing any handling, and come back to this later. Maybe a TODO for better error handling? ditto for ConsistencyCheck() method http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:486: (*result)[key16] = NullableString16(value, false); is the call to resize needed? i'm not sure if this would prevent a memcpy for the stack allocated instance or not, maybe? size_t len = it->value().size() / sizeof(char16); const char16* data_ptr = reinterpret_cast<const char16*>(it->value().data()); (*result)[key16] = NullableString16(string16(data_ptr, len), false) http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:522: batch->Delete(key); can batch->Delete or batch->Put fail... ah... i see they cannot http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:528: batch->Put(key, std::string(data, size)); i think you could use the Slice(const char* d, size_t n) ctor to avoid a memcpy here... batch->Put(key, Slice(data, size)); http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... File webkit/dom_storage/session_storage_database.h (right): http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.h:22: } // namespace leveldb style nit: generally we forward declare more in a more condensed less verbose way namespace leveldb { class DB; class WriteBatch; } http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.h:39: void ReadAllValues(int64 namespace_id, Since this class works with multiple object types (unlike the simpler single-origin-only DomStorageDatabase), maybe these method names could be modified to more consistently reflect what they're operating on. Maybe... ReadAreaValues CommitAreaChanges CloneNamespace DeepCopyArea ResetArea (ie. DisaccociateMap) DeleteArea (ie. DeleteOrigin) DeleteNamespace DeleteAll (ie. DeleteLeftoverData) Where 'area' refers to stuff for <namespace_id, origin>. http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.h:54: bool ShallowCopyNamespace(int64 namespace_id, int64 new_namespace_id); consider calling this CloneNamespace to be more consistent with naming on other classes... namespaces are cloned, areas are copied (shallow | deep). http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.h:70: bool DeleteLeftoverData(); Feels like it might be premature to add this method. Consider leaving it out for now and revisiting when integrating with the higher level chrome level logic to do session restore. Or maybe simplify the impl and call the method DeleteAll(). http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... File webkit/dom_storage/session_storage_database_unittest.cc (right): http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database_unittest.cc:337: int64 SessionStorageDatabaseTest::MapIdForNamespaceAndOrigin( efficient impls of these three helpers might be useful as private methods of the class proper http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database_unittest.cc:368: int64 ref_count; ref_count? http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database_unittest.cc:557: EXPECT_TRUE(db_->ShallowCopyNamespace(6, 7)); i like this one :) http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database_unittest.cc:726: TEST_F(SessionStorageDatabaseTest, WriteRawBytes) { i like this one too!
Thanks for comments, they were very helpful for restructuring the code! http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... File webkit/dom_storage/session_storage_database.cc (right): http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:40: SessionStorageDatabase::SessionStorageDatabase(const FilePath& file_path) On 2012/04/25 08:32:12, michaeln wrote: > Might be helpful to distinguish between namespace_ids with the offset applied > and those w/o in param/var naming. It's easy to lose track of which flavor of id > is sitting in any param or local var. Also, how confident are you of this > offsetting scheme? Another option is to leave that indirection out in this > initial CL. > > More consistently using int64s for mapids and namespaceids for method params > instead of sometimes having the string form and other times having an int form > might be helpful. > > Some of the methods are tricky to follow due to dropping down into leveldb > primitives where its all prefixed string keys with prefixed string values. The > high level logic easily gets lost in there. I did some noodling with a few of > the public methods to see what set of private helpers might with readability. > Please see the pseudo code noodling comments as food for thought. > > GetMapForArea(namespace_id, origin, &mapid); > GetOrCreateMapForArea(namespace_id, origin, &map_id); > SetMapForArea(namespace_id, origin, mapid); > DeleteAreaFromNamespace(namespace_id, origin); > > CreateNamespaceKey(namespace_id); > DeleteNamespaceKey(namespace_id); > > CreateMap(&new_map_id); > GetMapRefCount(map_id, &count); > SetMapRefCount(map_id, count); > DeleteMap(map_id); > > std::vector<std::pair<GURL, int64> > areas; > GetAreasInNamespace(namespace_id, &areas); > AddAreaToNamespace(new_namespace_id, origin, mapid); I tried to separate the db logic better from the high level logic. I made it so that the namespace_id_str always contains the offset (it didn't in couple of places; those were bugs (added tests)) and namespace_id is int64 and never contains the offset. I added tests which call all the API functions in a situation where the offset transformation matters. DisassociateMap aka. ResetArea turned out to be useless, since DeleteArea does the exact same thing. Removed it. http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:65: ReadMap(map_id, result, false); On 2012/04/25 08:32:12, michaeln wrote: > pseudo code noodling... > > if (!LazyOpen(false)) > return; > int64 map_id; > if (!GetMapForArea(namespace_id, origin, &map_id)) > return; > ReadMap(map_id, result, false); Done. http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:71: const ValuesMap& changes) { On 2012/04/25 08:32:12, michaeln wrote: > pseudo code noodling... > > if (!LazyOpen(true)) > return false; > > leveldb::WriteBatch batch; > > int64 map_id; > if (!GetOrCreateMapForArea(namespace_id, origin, &map_id, &batch)) > return false; > > DCHECK(1, GetMapRefCount(map_id)) << "shouldn't be mutating a shared map"; > > if (clear_all_first) { > if (!ClearMap(map_id, &batch)) > return false; > } > > WriteValuesToMap(map_id, changes, &batch); > return db_->Write(leveldb::WriteOptions(), &batch).ok(); This doesn't work, we cannot go and read the ref count of a map which is just being created. http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:81: batch.Put(NamespaceStartKey(namespace_id, namespace_offset_), ""); On 2012/04/25 08:32:12, michaeln wrote: > would be nice to not overwrite existing entries for these key on each call Done. http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:97: // We shouldn't write data to a shallow copy. On 2012/04/25 08:32:12, michaeln wrote: > should we dcheck this condition instead? ConsistencyCheck DCHECKs. ConsistencyCheck is there so that I can write: if (!ConsistencyCheck(consistent)) return false; instead of: DCHECK(consistent); if (!consistent) { is_inconsistent_ = true; return false; } http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:124: int64 new_namespace_id) { On 2012/04/25 08:32:12, michaeln wrote: > pseudo code noodling... > > WriteBatch batch; > > // setup required entries for a namespace ensuring its really new > if (!CreateNamespaceKey(new_namespace_id, &batch) > return false; > > std::vector<std::pair<GURL, int64> >* areas; > if (!GetAreasInNamespace(namespace_id, &areas)) > return false; > > for_each(area) { > SetMapRefCount(mapid, GetMapRefCount(map_id) + 1, &batch); > AddAreaToNamespace(new_namespace_id, origin, mapid); > } > > return batch.Write(); Done. http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:190: bool SessionStorageDatabase::DeepCopy(int64 namespace_id, const GURL& origin) { On 2012/04/25 08:32:12, michaeln wrote: > pseudo code noodling... > > if (!GetMapForArea(namespace_id, origin, &old_map_id)) > return false; > > ReadMap(old_map_id, &values); > > WriteBatch batch; > CreateMap(&new_map_id, &batch); // created with refcount of 1 > WriteMap(new_map_id, values); > SetMapForArea(namespace_id, origin, new_map_id); > > int old_refcount = GetMapRefCount(old_map_id); > DCHECK(old_refcount > 1); > SetMapRefCount(old_map_id, old_refcount - 1, &batch); Done. http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:223: // If this copy is the the only "shallow" copy of the map, no deep copying is On 2012/04/25 08:32:12, michaeln wrote: > Is there any case where the higher level code would call to DeepCopy an unshared > map? Would it make sense to DCHECK this instead of handling it? Atm this can happen if we do a shallow copy, write data to it (it will be made deep), and then write data to the original map (which was also shallow). But if I change the "is_shallow_" handling in my other CL to use the ref count of the map, this cannot happen any more. So, I ConsistencyChecked this here. http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:249: const GURL& origin) { On 2012/04/25 08:32:12, michaeln wrote: > ResetArea(id, origin) > > pseudo code noodling... > > int64 map_id; > GetMapForArea(id, origin, &map_id); > DeleteAreaFromNamespace(id, origin); > > int refcount; > GetMapRefCount(map_id, &refcount); > if (refcount == 1) > DeleteMap(map_id); > else > SetMapRefCount(map_id, refcount - 1); This turned out to be the same than DeleteOrigin --> removed. http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:284: bool SessionStorageDatabase::DeleteNamespace(int64 namespace_id) { On 2012/04/25 08:32:12, michaeln wrote: > pseudo code noodling... > > std::vector<std::pair<GURL, int64> > areas; > if (!GetAreasInNamespace(namespace_id, &areas)) > return false; > > for_each(area) { > int refcount = GetMapRefCount(area.mapid); > if (refcount == 1) > DeleteMap(mapid); > else > SetMapRefCount(refcount - 1); > DeleteAreaFromNamespace(namespace_id, area.origin); > } > > DeleteNamespaceKey(namespace_id); Done. http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:366: leveldb::Status s = db_->Write(leveldb::WriteOptions(), &batch); On 2012/04/25 08:32:12, michaeln wrote: > I think this results in an empty session storage database. In the first loop, > all namespace- prefixed keys are deleted and every map ref is tallied for > release. In the second loop, all map- prefixed keys are deleted for any map that > was referred to by a namespace (which i think should be all maps). > > Is that right? Is that the desired behavior? As mentioned in the .h file, maybe > defer the addition of this method for later once the desired behavior is better > understood. Or maybe put a simpler class method in for now... > > void SessionStorageDatabase::ClearAll() { > for_each(key) > batch->Delete(key); > } Deleted it for now. http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:449: file_util::Delete(file_path_, true); On 2012/04/25 08:32:12, michaeln wrote: > This doesn't look like a viable error handling strategy. > > On some systems the delete will fail if because the files are open by db_, also > given that one thread can be reading while another is writing, its dicey to > delete these files w/o greater coordination. > > To start with, i think you could just return false here w/o actually doing any > handling, and come back to this later. Maybe a TODO for better error handling? > > ditto for ConsistencyCheck() method Ahh, true, we should at least close the db first. Removed this, added a TODO. http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:486: (*result)[key16] = NullableString16(value, false); On 2012/04/25 08:32:12, michaeln wrote: > is the call to resize needed? i'm not sure if this would prevent a memcpy for > the stack allocated instance or not, maybe? > > size_t len = it->value().size() / sizeof(char16); > const char16* data_ptr = reinterpret_cast<const char16*>(it->value().data()); > (*result)[key16] = NullableString16(string16(data_ptr, len), false) Done. http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:528: batch->Put(key, std::string(data, size)); On 2012/04/25 08:32:12, michaeln wrote: > i think you could use the Slice(const char* d, size_t n) ctor to avoid a memcpy > here... > > batch->Put(key, Slice(data, size)); Done. http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... File webkit/dom_storage/session_storage_database.h (right): http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.h:22: } // namespace leveldb On 2012/04/25 08:32:12, michaeln wrote: > style nit: generally we forward declare more in a more condensed less verbose > way > > namespace leveldb { > class DB; > class WriteBatch; > } Done. http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.h:39: void ReadAllValues(int64 namespace_id, On 2012/04/25 08:32:12, michaeln wrote: > Since this class works with multiple object types (unlike the simpler > single-origin-only DomStorageDatabase), maybe these method names could be > modified to more consistently reflect what they're operating on. Maybe... > > ReadAreaValues > CommitAreaChanges > CloneNamespace > DeepCopyArea > ResetArea (ie. DisaccociateMap) > DeleteArea (ie. DeleteOrigin) > DeleteNamespace > DeleteAll (ie. DeleteLeftoverData) > > Where 'area' refers to stuff for <namespace_id, origin>. > Done. http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.h:54: bool ShallowCopyNamespace(int64 namespace_id, int64 new_namespace_id); On 2012/04/25 08:32:12, michaeln wrote: > consider calling this CloneNamespace to be more consistent with naming on other > classes... namespaces are cloned, areas are copied (shallow | deep). Done. http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.h:70: bool DeleteLeftoverData(); On 2012/04/25 08:32:12, michaeln wrote: > Feels like it might be premature to add this method. Consider leaving it out for > now and revisiting when integrating with the higher level chrome level logic to > do session restore. Or maybe simplify the impl and call the method DeleteAll(). Done (deleted it). http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... File webkit/dom_storage/session_storage_database_unittest.cc (right): http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database_unittest.cc:337: int64 SessionStorageDatabaseTest::MapIdForNamespaceAndOrigin( On 2012/04/25 08:32:12, michaeln wrote: > efficient impls of these three helpers might be useful as private methods of the > class proper Done & I modified these wrappers to EXPECT_TRUE on the return value. http://codereview.chromium.org/10176005/diff/33001/webkit/dom_storage/session... webkit/dom_storage/session_storage_database_unittest.cc:368: int64 ref_count; On 2012/04/25 08:32:12, michaeln wrote: > ref_count? Done.
Hi marja, noctural code review for me again, but not so late as the last one :) This most recent snapshot is so much more readable, thank you thank you, thank you for that! I think I have only one material issue to consider, LazyOpen concurrency. I do wonder about whether all of the consistency checks should be as dire as they all are. Seems like some are of a different nature than the others (programming error vs something aint right in the db), but we can edit to taste going forward. Also I'm not entirely clear (and i dont need to be) on what you have in mind with the 'offset' scheme and how chrome::SessionRestore logic will reestablish old sessions based on that scheme, but seems like you have something in mind and i'm looking forward to see how that plays out. I haven't looked as closely at the tests yet as i could have, but i'd like to take one last look after you mull over LazyOpen and i'll look more closely then. http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... File webkit/dom_storage/session_storage_database.cc (right): http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database.cc:76: if (!CreateNamespace(namespace_id, true, &batch)) webkit style rubbing off on me too much maybe... maybe make a local const bool kOkIfExists = true; variable for use as the param to this method? http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database.cc:88: if (!ConsistencyCheck(ref_count == 1)) This feel like a programming error more than a data integrity issue? If the caller tries to mutate a shallow copy, that's they're bug. Maybe the db is borked, but more likely the caller just screwed up. Instead of turning the lights off for all accesses by flagging a consistency error, should we DCHECK (to yell at the programmer) and return false, but otherwise let further access continue? http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database.cc:131: if (!CreateNamespace(new_namespace_id, false, &batch)) simlarly, const bool kFailIfExists = false local for use as the method param http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database.cc:170: if (!LazyOpen(true)) maybe LazyOpen(false) here, if the db doesnt exist is there anything to copy? http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database.cc:231: const std::string& origin = it->first; thnx for the self-document-code that tells me what 'first' is here http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database.cc:240: bool SessionStorageDatabase::LazyOpen(bool create_if_needed) { I think we may need a mutex locker around this method since it can be called concurrently from two different threads. They'll be racing towards the leveldb Open call and db_ assignment and the loser will likely mess up the winner and such. Please think about concurrent calls to this method and how best to deal with it. http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database.cc:241: if (db_error_ || is_inconsistent_) { If we trip on any 'consistency' error that's really just a programming error... the lights go out here. Not sure but this may be overly aggressive. http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database.cc:336: return ConsistencyCheck(ok_if_exists); Is this really a consistency error? Or possibly a programming error? http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database.cc:478: batch->Put(MapRefCountKey(*map_id), base::Int64ToString(1)); maybe "1"? http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database.cc:617: std::string SessionStorageDatabase::NamespacePrefix() { maybe consider making the truely constant 'prefix' and well-known-key getters const char* return values http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... File webkit/dom_storage/session_storage_database.h (right): http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database.h:10: #include <set> is <set> needed here or maybe just in the .cc file? http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database.h:191: // the offset yet. Tthe namespaces ids which are handled as strings (named type: Tthe http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... File webkit/dom_storage/session_storage_database_unittest.cc (right): http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database_unittest.cc:61: GURL origin1; can these all be const, and named as such, kFoo?
this looks like a pretty solid first cut at it, thanx for working this up modulo the tsan thing mentioned below... lgtm... i just have minor nits beyond that threading issue. if you want me to take another look, lmk, or if you make any significant changes please ping me to take another look http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... File webkit/dom_storage/session_storage_database_unittest.cc (right): http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database_unittest.cc:141: EXPECT_TRUE(conversion_ok); do you also want to pull the origin out of 'key' and assign it to 'origin', or maybe remove the 'origin' out param if not? http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database_unittest.cc:187: void SessionStorageDatabaseTest::CheckConsistency() const { nit: to be more clear about the scope of what is being checked maybe CheckDatabaseConsistency http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database_unittest.cc:215: std::map<int64, int64> map_refcounts; would 'expected_map_refcounts' be a more informative name? http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database_unittest.cc:225: max_namespace_id = namespace_id; might be able to use std::max() here for better readability http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database_unittest.cc:239: max_map_id = map_id; ditto std::max http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database_unittest.cc:284: ASSERT_EQ(0U, map_refcounts.size()); could use map.empty() here http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database_unittest.cc:296: void SessionStorageDatabaseTest::CheckEmpty() const { nit: CheckEmptyDatabase http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database_unittest.cc:329: void SessionStorageDatabaseTest::CheckData(int64 namespace_id, nit: CheckAreaData http://codereview.chromium.org/10176005/diff/32003/webkit/dom_storage/session... File webkit/dom_storage/session_storage_database.cc (right): http://codereview.chromium.org/10176005/diff/32003/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:311: is_inconsistent_ = !ok; i think the tsan bots would eventually get around to rightly complaining about this since the is_bustedinsomeway_ data members can be modified on one thread and racily read from another, also, it seems like you want these values to be sticky such that once they're set true they don't get reset to false maybe like this... if (ok) return true; AutoLock locker(db_lock_); is_<flavor_of_busted>_ = true; return false; http://codereview.chromium.org/10176005/diff/32003/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:507: int prefix_length = std::string(MapPrefix()).length(); would be nice to pull this out of the inner loop and it could be const int kPrefixLength
Oops, I had forgotten to publish my comments from yesterday. I didn't do any other changes than the ones you pointed out. Trying to commit this now.. Thanks for the review(s)! http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... File webkit/dom_storage/session_storage_database.cc (right): http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database.cc:76: if (!CreateNamespace(namespace_id, true, &batch)) On 2012/04/26 05:07:43, michaeln wrote: > webkit style rubbing off on me too much maybe... maybe make a local > const bool kOkIfExists = true; > variable for use as the param to this method? Done. http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database.cc:88: if (!ConsistencyCheck(ref_count == 1)) On 2012/04/26 05:07:43, michaeln wrote: > This feel like a programming error more than a data integrity issue? If the > caller tries to mutate a shallow copy, that's they're bug. Maybe the db is > borked, but more likely the caller just screwed up. > > Instead of turning the lights off for all accesses by flagging a consistency > error, should we DCHECK (to yell at the programmer) and return false, but > otherwise let further access continue? Done, added CallerErrorCheck which will be called in cases like this. http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database.cc:131: if (!CreateNamespace(new_namespace_id, false, &batch)) On 2012/04/26 05:07:43, michaeln wrote: > simlarly, const bool kFailIfExists = false local for use as the method param Done. http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database.cc:170: if (!LazyOpen(true)) On 2012/04/26 05:07:43, michaeln wrote: > maybe LazyOpen(false) here, if the db doesnt exist is there anything to copy? Below we check that the thing to copy must exist, so I would be consistent here. Otherwise, the API usage error of calling this for a non-existing area might go undetected if the database hasn't been opened yet, and it's inconsistent. http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database.cc:240: bool SessionStorageDatabase::LazyOpen(bool create_if_needed) { On 2012/04/26 05:07:43, michaeln wrote: > I think we may need a mutex locker around this method since it can be called > concurrently from two different threads. They'll be racing towards the leveldb > Open call and db_ assignment and the loser will likely mess up the winner and > such. Please think about concurrent calls to this method and how best to deal > with it. Done. http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database.cc:241: if (db_error_ || is_inconsistent_) { On 2012/04/26 05:07:43, michaeln wrote: > If we trip on any 'consistency' error that's really just a programming error... > the lights go out here. Not sure but this may be overly aggressive. Now this should distinguish better between incorrect API usage (e.g., trying to shallow copy into an existing namespace, trying to DeepCopy something that is not a shallow copy) and inconsistency (e.g., ref count is not an int). http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database.cc:336: return ConsistencyCheck(ok_if_exists); On 2012/04/26 05:07:43, michaeln wrote: > Is this really a consistency error? Or possibly a programming error? Done. http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database.cc:478: batch->Put(MapRefCountKey(*map_id), base::Int64ToString(1)); On 2012/04/26 05:07:43, michaeln wrote: > maybe "1"? Done. http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database.cc:617: std::string SessionStorageDatabase::NamespacePrefix() { On 2012/04/26 05:07:43, michaeln wrote: > maybe consider making the truely constant 'prefix' and well-known-key getters > const char* return values Done. http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... File webkit/dom_storage/session_storage_database.h (right): http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database.h:10: #include <set> On 2012/04/26 05:07:43, michaeln wrote: > is <set> needed here or maybe just in the .cc file? Done. (Needed nowhere.) http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database.h:191: // the offset yet. Tthe namespaces ids which are handled as strings (named On 2012/04/26 05:07:43, michaeln wrote: > type: Tthe Done. http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... File webkit/dom_storage/session_storage_database_unittest.cc (right): http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database_unittest.cc:61: GURL origin1; On 2012/04/26 05:07:43, michaeln wrote: > can these all be const, and named as such, kFoo? Done. http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database_unittest.cc:141: EXPECT_TRUE(conversion_ok); On 2012/04/26 22:07:02, michaeln wrote: > do you also want to pull the origin out of 'key' and assign it to 'origin', or > maybe remove the 'origin' out param if not? Done (latter). http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database_unittest.cc:187: void SessionStorageDatabaseTest::CheckConsistency() const { On 2012/04/26 22:07:02, michaeln wrote: > nit: to be more clear about the scope of what is being checked maybe > CheckDatabaseConsistency Done. http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database_unittest.cc:215: std::map<int64, int64> map_refcounts; On 2012/04/26 22:07:02, michaeln wrote: > would 'expected_map_refcounts' be a more informative name? Done. http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database_unittest.cc:225: max_namespace_id = namespace_id; On 2012/04/26 22:07:02, michaeln wrote: > might be able to use std::max() here for better readability Done. http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database_unittest.cc:239: max_map_id = map_id; On 2012/04/26 22:07:02, michaeln wrote: > ditto std::max Done. http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database_unittest.cc:284: ASSERT_EQ(0U, map_refcounts.size()); On 2012/04/26 22:07:02, michaeln wrote: > could use map.empty() here Done. http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database_unittest.cc:296: void SessionStorageDatabaseTest::CheckEmpty() const { On 2012/04/26 22:07:02, michaeln wrote: > nit: CheckEmptyDatabase Done. http://codereview.chromium.org/10176005/diff/4004/webkit/dom_storage/session_... webkit/dom_storage/session_storage_database_unittest.cc:329: void SessionStorageDatabaseTest::CheckData(int64 namespace_id, On 2012/04/26 22:07:02, michaeln wrote: > nit: CheckAreaData Done. http://codereview.chromium.org/10176005/diff/32003/webkit/dom_storage/session... File webkit/dom_storage/session_storage_database.cc (right): http://codereview.chromium.org/10176005/diff/32003/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:311: is_inconsistent_ = !ok; On 2012/04/26 22:07:02, michaeln wrote: > i think the tsan bots would eventually get around to rightly complaining about > this since the is_bustedinsomeway_ data members can be modified on one thread > and racily read from another, also, it seems like you want these values to be > sticky such that once they're set true they don't get reset to false > > maybe like this... > > if (ok) > return true; > > AutoLock locker(db_lock_); > is_<flavor_of_busted>_ = true; > return false; Done. http://codereview.chromium.org/10176005/diff/32003/webkit/dom_storage/session... webkit/dom_storage/session_storage_database.cc:507: int prefix_length = std::string(MapPrefix()).length(); On 2012/04/26 22:07:02, michaeln wrote: > would be nice to pull this out of the inner loop and it could be const int > kPrefixLength Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/marja@chromium.org/10176005/53001
Presubmit check for 10176005-53001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
webkit
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/marja@chromium.org/10176005/53001
Change committed as 134259
Committed this with TBR=tony, since the file causing missing OWNERS check is webkit/tools/test_shell/test_shell.gypi, and it's only adding new unit tests there.
Sorry, I'm back now. LGTM. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
