Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7057006:
Add Dump{Quota,LastAccessTime}Table (Closed)
|
Created:
9 years, 7 months ago by tzik Modified:
9 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, Paweł Hajdan Jr. Visibility:
Public. |
DescriptionAdd Dump{Quota,LastAccessTime}Table to QuotaDatabase
BUG=61676
TEST='QuotaDatabaseTest.*'
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86403
Patch Set 1 #
Total comments: 5
Patch Set 2 : 'Move Assign*** and Rename AccessTable' #
Total comments: 3
Patch Set 3 : '' #
Total comments: 18
Patch Set 4 : '' #
Total comments: 2
Patch Set 5 : '' #
Messages
Total messages: 13 (0 generated)
http://codereview.chromium.org/7057006/diff/1/webkit/quota/quota_database.cc File webkit/quota/quota_database.cc (right): http://codereview.chromium.org/7057006/diff/1/webkit/quota/quota_database.cc#... webkit/quota/quota_database.cc:510: bool QuotaDatabase::DumpAccessTable(AccessTableCallback* callback) { Can we rename s/AccessTable/LastAccessTimeTable/ http://codereview.chromium.org/7057006/diff/1/webkit/quota/quota_database.cc#... webkit/quota/quota_database.cc:535: const std::set<QuotaTableEntry>& entries) { As we chatted locally, as long as it is labeled 'for test and debug' I may feel using set here is a bit overkill. http://codereview.chromium.org/7057006/diff/1/webkit/quota/quota_database.h File webkit/quota/quota_database.h (right): http://codereview.chromium.org/7057006/diff/1/webkit/quota/quota_database.h#n... webkit/quota/quota_database.h:90: bool AssignQuotaTable(const std::set<QuotaTableEntry>& entries); I guess we won't need assign methods?
http://codereview.chromium.org/7057006/diff/1/webkit/quota/quota_database.cc File webkit/quota/quota_database.cc (right): http://codereview.chromium.org/7057006/diff/1/webkit/quota/quota_database.cc#... webkit/quota/quota_database.cc:510: bool QuotaDatabase::DumpAccessTable(AccessTableCallback* callback) { On 2011/05/20 13:19:54, kinuko wrote: > Can we rename s/AccessTable/LastAccessTimeTable/ Done. http://codereview.chromium.org/7057006/diff/1/webkit/quota/quota_database.h File webkit/quota/quota_database.h (right): http://codereview.chromium.org/7057006/diff/1/webkit/quota/quota_database.h#n... webkit/quota/quota_database.h:90: bool AssignQuotaTable(const std::set<QuotaTableEntry>& entries); On 2011/05/20 13:19:54, kinuko wrote: > I guess we won't need assign methods? Done. Moved to quota_database_unittest.
A short question http://codereview.chromium.org/7057006/diff/6001/webkit/quota/quota_database.cc File webkit/quota/quota_database.cc (right): http://codereview.chromium.org/7057006/diff/6001/webkit/quota/quota_database.... webkit/quota/quota_database.cc:533: if (!callback->Run(entry)) Is the caller responsible for deleting the callback? http://codereview.chromium.org/7057006/diff/6001/webkit/quota/quota_database.... webkit/quota/quota_database.cc:558: if (!callback->Run(entry)) ditto.
http://codereview.chromium.org/7057006/diff/6001/webkit/quota/quota_database.cc File webkit/quota/quota_database.cc (right): http://codereview.chromium.org/7057006/diff/6001/webkit/quota/quota_database.... webkit/quota/quota_database.cc:533: if (!callback->Run(entry)) On 2011/05/23 06:16:42, Dai Mikurube wrote: > Is the caller responsible for deleting the callback? Done. Now, ownership of the callback shall be passed.
On 2011/05/23 06:57:11, tzik wrote: > http://codereview.chromium.org/7057006/diff/6001/webkit/quota/quota_database.cc > File webkit/quota/quota_database.cc (right): > > http://codereview.chromium.org/7057006/diff/6001/webkit/quota/quota_database.... > webkit/quota/quota_database.cc:533: if (!callback->Run(entry)) > On 2011/05/23 06:16:42, Dai Mikurube wrote: > > Is the caller responsible for deleting the callback? > > Done. Now, ownership of the callback shall be passed. LGTM if Kinuko-san says ok.
some nits; mostly for style issues. http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database.cc File webkit/quota/quota_database.cc (right): http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database.... webkit/quota/quota_database.cc:534: if (!callback->Run(entry)) does this mean 'the callback may return false when it wants to stop reading data'? would you mind adding some comment about that? http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database.h File webkit/quota/quota_database.h (right): http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database.... webkit/quota/quota_database.h:93: }; Please move all the typedefs before the methods (i.e. right after private:). http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database.... webkit/quota/quota_database.h:96: friend bool operator <(const QuotaTableEntry& lhs, Please place this next to (right after) the struct define. http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database_... File webkit/quota/quota_database_unittest.cc (right): http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database_... webkit/quota/quota_database_unittest.cc:42: LastAccessTimeTableCallback; style-nit: There're mixed notions but I may expect 4 spaces here http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database_... webkit/quota/quota_database_unittest.cc:54: for (;itr != end; ++itr) { style-nit: space after ';' http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database_... webkit/quota/quota_database_unittest.cc:72: return transaction.Commit(); QuotaDatabase now always keeps one transaction opened; I guess calling this will just flush the outer transaction? I think you can instead call quota_database->Commit() here and remove line 50-52. http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database_... webkit/quota/quota_database_unittest.cc:85: for (;itr != end; ++itr) { style-nit: space after ';' http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database_... webkit/quota/quota_database_unittest.cc:104: return transaction.Commit(); ditto http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database_... webkit/quota/quota_database_unittest.cc:274: :table(itr, end) {} style-nit: space after ':'
One more nit: please update the issue subject! Thx. On 2011/05/23 08:16:52, kinuko wrote: > some nits; mostly for style issues. > > http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database.cc > File webkit/quota/quota_database.cc (right): > > http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database.... > webkit/quota/quota_database.cc:534: if (!callback->Run(entry)) > does this mean 'the callback may return false when it wants to stop reading > data'? would you mind adding some comment about that? > > http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database.h > File webkit/quota/quota_database.h (right): > > http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database.... > webkit/quota/quota_database.h:93: }; > Please move all the typedefs before the methods (i.e. right after private:). > > http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database.... > webkit/quota/quota_database.h:96: friend bool operator <(const QuotaTableEntry& > lhs, > Please place this next to (right after) the struct define. > > http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database_... > File webkit/quota/quota_database_unittest.cc (right): > > http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database_... > webkit/quota/quota_database_unittest.cc:42: LastAccessTimeTableCallback; > style-nit: There're mixed notions but I may expect 4 spaces here > > http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database_... > webkit/quota/quota_database_unittest.cc:54: for (;itr != end; ++itr) { > style-nit: space after ';' > > http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database_... > webkit/quota/quota_database_unittest.cc:72: return transaction.Commit(); > QuotaDatabase now always keeps one transaction opened; I guess calling this will > just flush the outer transaction? > > I think you can instead call quota_database->Commit() here and remove line > 50-52. > > http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database_... > webkit/quota/quota_database_unittest.cc:85: for (;itr != end; ++itr) { > style-nit: space after ';' > > http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database_... > webkit/quota/quota_database_unittest.cc:104: return transaction.Commit(); > ditto > > http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database_... > webkit/quota/quota_database_unittest.cc:274: :table(itr, end) {} > style-nit: space after ':'
http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database.cc File webkit/quota/quota_database.cc (right): http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database.... webkit/quota/quota_database.cc:534: if (!callback->Run(entry)) On 2011/05/23 08:16:52, kinuko wrote: > does this mean 'the callback may return false when it wants to stop reading > data'? would you mind adding some comment about that? Done. http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database.h File webkit/quota/quota_database.h (right): http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database.... webkit/quota/quota_database.h:93: }; On 2011/05/23 08:16:52, kinuko wrote: > Please move all the typedefs before the methods (i.e. right after private:). Done. http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database.... webkit/quota/quota_database.h:96: friend bool operator <(const QuotaTableEntry& lhs, On 2011/05/23 08:16:52, kinuko wrote: > Please place this next to (right after) the struct define. Done. http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database_... File webkit/quota/quota_database_unittest.cc (right): http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database_... webkit/quota/quota_database_unittest.cc:42: LastAccessTimeTableCallback; On 2011/05/23 08:16:52, kinuko wrote: > style-nit: There're mixed notions but I may expect 4 spaces here Done. http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database_... webkit/quota/quota_database_unittest.cc:54: for (;itr != end; ++itr) { On 2011/05/23 08:16:52, kinuko wrote: > style-nit: space after ';' Done. http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database_... webkit/quota/quota_database_unittest.cc:72: return transaction.Commit(); On 2011/05/23 08:16:52, kinuko wrote: > QuotaDatabase now always keeps one transaction opened; I guess calling this will > just flush the outer transaction? > > I think you can instead call quota_database->Commit() here and remove line > 50-52. Done. http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database_... webkit/quota/quota_database_unittest.cc:85: for (;itr != end; ++itr) { On 2011/05/23 08:16:52, kinuko wrote: > style-nit: space after ';' Done. http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database_... webkit/quota/quota_database_unittest.cc:104: return transaction.Commit(); On 2011/05/23 08:16:52, kinuko wrote: > ditto Done. http://codereview.chromium.org/7057006/diff/8003/webkit/quota/quota_database_... webkit/quota/quota_database_unittest.cc:274: :table(itr, end) {} On 2011/05/23 08:16:52, kinuko wrote: > style-nit: space after ':' Done.
lgtm http://codereview.chromium.org/7057006/diff/13001/webkit/quota/quota_database... File webkit/quota/quota_database_unittest.cc (right): http://codereview.chromium.org/7057006/diff/13001/webkit/quota/quota_database... webkit/quota/quota_database_unittest.cc:44: template<typename Iterator> style-nit: space before '<' (here and elsewhere)
http://codereview.chromium.org/7057006/diff/13001/webkit/quota/quota_database... File webkit/quota/quota_database_unittest.cc (right): http://codereview.chromium.org/7057006/diff/13001/webkit/quota/quota_database... webkit/quota/quota_database_unittest.cc:44: template<typename Iterator> On 2011/05/24 05:22:21, kinuko wrote: > style-nit: space before '<' (here and elsewhere) Done.
lgtm (still) On 2011/05/24 05:42:35, tzik wrote: > http://codereview.chromium.org/7057006/diff/13001/webkit/quota/quota_database... > File webkit/quota/quota_database_unittest.cc (right): > > http://codereview.chromium.org/7057006/diff/13001/webkit/quota/quota_database... > webkit/quota/quota_database_unittest.cc:44: template<typename Iterator> > On 2011/05/24 05:22:21, kinuko wrote: > > style-nit: space before '<' (here and elsewhere) > > Done.
Change committed as 86403 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
