Session-only appcache.

Session-only appcache.


BUG=47049
TEST=ChromeAppCacheServiceTest.*


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

[marja, 2011-08-26 12:35:57]

Hi Michael,

I was wondering what would be a right approach for cleaning session-only
appcaches on exit. Do you want something simple like this CL, or something more
generic that would handle all quota-managed storage types in a uniform way?

[michaeln, 2011-08-26 21:16:05]

On 2011/08/26 12:35:57, marja wrote:
> Hi Michael,
> 
> I was wondering what would be a right approach for cleaning session-only
> appcaches on exit. Do you want something simple like this CL, or something
more
> generic that would handle all quota-managed storage types in a uniform way?

I think i prefer this approach for its simplicity.

Since this is so similar to the existing "clear all on exit" functionality, i
think handling it in a way that similar to that is most prudent... so not in an
aggregated quota-manager way.

[michaeln, 2011-08-26 21:31:01]

http://codereview.chromium.org/7741040/diff/1/webkit/appcache/appcache_storag...
File webkit/appcache/appcache_storage_impl.cc (right):

http://codereview.chromium.org/7741040/diff/1/webkit/appcache/appcache_storag...
webkit/appcache/appcache_storage_impl.cc:75: bool delete_permanent) {
naming nit: Thoughout the project, this flag goes as "clear_on_exit" (or
something close to that with "clear" being the first term). It be nice to retain
that convention here. I see that conflicts with the existing function name since
now the function is used for multiple purposes... see comments in
~AppCacheStorageImpl about this...

http://codereview.chromium.org/7741040/diff/1/webkit/appcache/appcache_storag...
webkit/appcache/appcache_storage_impl.cc:1167: ClearOnExit,
Since this is now done unconditionally, maybe rename the function to
CleanUpOnDatabaseThread (or something along those lines), and have it also be
responsible for calling delete on the database instance, in addition to being
responsible for doing clear_local_state and clear_session_only_state stuff.

http://codereview.chromium.org/7741040/diff/1/webkit/appcache/appcache_storag...
webkit/appcache/appcache_storage_impl.cc:1171:
AppCacheThread::DeleteSoon(AppCacheThread::db(), FROM_HERE, database_);
if you buy that, this line could be deleted

[marja, 2011-08-29 09:41:42]

http://codereview.chromium.org/7741040/diff/1/webkit/appcache/appcache_storag...
File webkit/appcache/appcache_storage_impl.cc (right):

http://codereview.chromium.org/7741040/diff/1/webkit/appcache/appcache_storag...
webkit/appcache/appcache_storage_impl.cc:75: bool delete_permanent) {
On 2011/08/26 21:31:01, michaeln wrote:
> naming nit: Thoughout the project, this flag goes as "clear_on_exit" (or
> something close to that with "clear" being the first term). It be nice to
retain
> that convention here. I see that conflicts with the existing function name
since
> now the function is used for multiple purposes... see comments in
> ~AppCacheStorageImpl about this...

I renamed the flag to clear_all_appcaches (as opposed to clearing only
session-only appcaches), is that ok?

http://codereview.chromium.org/7741040/diff/1/webkit/appcache/appcache_storag...
webkit/appcache/appcache_storage_impl.cc:1167: ClearOnExit,
On 2011/08/26 21:31:01, michaeln wrote:
> Since this is now done unconditionally, maybe rename the function to
> CleanUpOnDatabaseThread (or something along those lines), and have it also be
> responsible for calling delete on the database instance, in addition to being
> responsible for doing clear_local_state and clear_session_only_state stuff.

Renamed to CleanUpOnDatabaseThread.

http://codereview.chromium.org/7741040/diff/1/webkit/appcache/appcache_storag...
webkit/appcache/appcache_storage_impl.cc:1171:
AppCacheThread::DeleteSoon(AppCacheThread::db(), FROM_HERE, database_);
On 2011/08/26 21:31:01, michaeln wrote:
> if you buy that, this line could be deleted

Done.

[michaeln, 2011-08-29 17:43:15]

lgtm modulo a couple of minor things

http://codereview.chromium.org/7741040/diff/5001/webkit/appcache/appcache_sto...
File webkit/appcache/appcache_storage_impl.cc (right):

http://codereview.chromium.org/7741040/diff/5001/webkit/appcache/appcache_sto...
webkit/appcache/appcache_storage_impl.cc:72: // appcache data. Also, schedules
the database to be destroyed.
This doc comment is pretty clear, consider removing the additional code comments
in the method body.

http://codereview.chromium.org/7741040/diff/5001/webkit/appcache/appcache_sto...
webkit/appcache/appcache_storage_impl.cc:79:
AppCacheThread::DeleteSoon(AppCacheThread::db(), FROM_HERE, database);
Please use a local scoped_ptr<> here since it's lighter weight and doesn't
involve a second trip thru the message loop.

http://codereview.chromium.org/7741040/diff/5001/webkit/appcache/appcache_sto...
webkit/appcache/appcache_storage_impl.cc:94: if (!clear_all_appcaches &&
!special_storage_policy)
Looks like this could be moved up above the database->Find call to avoid the db
read in this case.

[marja, 2011-08-30 09:45:09]

Thanks for the review.

http://codereview.chromium.org/7741040/diff/5001/webkit/appcache/appcache_sto...
File webkit/appcache/appcache_storage_impl.cc (right):

http://codereview.chromium.org/7741040/diff/5001/webkit/appcache/appcache_sto...
webkit/appcache/appcache_storage_impl.cc:72: // appcache data. Also, schedules
the database to be destroyed.
On 2011/08/29 17:43:15, michaeln wrote:
> This doc comment is pretty clear, consider removing the additional code
comments
> in the method body.

Done.

http://codereview.chromium.org/7741040/diff/5001/webkit/appcache/appcache_sto...
webkit/appcache/appcache_storage_impl.cc:79:
AppCacheThread::DeleteSoon(AppCacheThread::db(), FROM_HERE, database);
On 2011/08/29 17:43:15, michaeln wrote:
> Please use a local scoped_ptr<> here since it's lighter weight and doesn't
> involve a second trip thru the message loop.

Done.

http://codereview.chromium.org/7741040/diff/5001/webkit/appcache/appcache_sto...
webkit/appcache/appcache_storage_impl.cc:94: if (!clear_all_appcaches &&
!special_storage_policy)
On 2011/08/29 17:43:15, michaeln wrote:
> Looks like this could be moved up above the database->Find call to avoid the
db
> read in this case.

Done.

[commit-bot: I haz the power, 2011-08-30 12:47:02]

Change committed as 98804