Moved deleting the indexed db context to the WebKitContext destructor.

Moved deleting the indexed db context to the IndexedDBContext destructor.

This gives us the safety that we delete the files only when they are certainly not accessed by the webkit context anymore.

BUG=56249
TEST=IndexedDBBrowserTest.ClearLocalState

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

[pastarmovj, 2010-11-27 18:49:58]

Please review.

[jochen (gone - plz use gerrit), 2010-11-27 19:02:26]

http://codereview.chromium.org/5359005/diff/6001/chrome/browser/browser_proce...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/5359005/diff/6001/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:33: #include
"chrome/browser/in_process_webkit/indexed_db_context.h"
please remove

http://codereview.chromium.org/5359005/diff/6001/chrome/browser/in_process_we...
File chrome/browser/in_process_webkit/webkit_context.cc (right):

http://codereview.chromium.org/5359005/diff/6001/chrome/browser/in_process_we...
chrome/browser/in_process_webkit/webkit_context.cc:43: if (clear_on_exit()) {
since we're explicitly deleting the context, you could push to value of
clear_on_exit to the indexed db context and clear the local state in indexed
db's destructor. then you also won't need the lock

http://codereview.chromium.org/5359005/diff/6001/chrome/browser/in_process_we...
chrome/browser/in_process_webkit/webkit_context.cc:107:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
that shouldn't be required

http://codereview.chromium.org/5359005/diff/6001/chrome/browser/in_process_we...
chrome/browser/in_process_webkit/webkit_context.cc:115: LOG(WARNING) << "Option
changed " << clear_on_exit_;
please remove

http://codereview.chromium.org/5359005/diff/6001/chrome/browser/in_process_we...
File chrome/browser/in_process_webkit/webkit_context.h (right):

http://codereview.chromium.org/5359005/diff/6001/chrome/browser/in_process_we...
chrome/browser/in_process_webkit/webkit_context.h:31: : public
NotificationObserver,
notification observer is a pure virtual class, refcountedthreadsafe is not. the
coding style says that non-pure virtual classes should come first

http://codereview.chromium.org/5359005/diff/6001/chrome/browser/in_process_we...
chrome/browser/in_process_webkit/webkit_context.h:85: bool clear_on_exit_;
I'd name this something like clear_local_state_on_exit_. Also the comment should
be "True if ..."

http://codereview.chromium.org/5359005/diff/6001/chrome/browser/in_process_we...
chrome/browser/in_process_webkit/webkit_context.h:87: Profile* profile_;
what do you need to profile for?

[pastarmovj, 2010-11-27 20:45:15]

Fixed the CL description to reflect the changes. Please check both my comments
on your comments and on the newly uploaded version.

http://codereview.chromium.org/5359005/diff/6001/chrome/browser/browser_proce...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/5359005/diff/6001/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:33: #include
"chrome/browser/in_process_webkit/indexed_db_context.h"
On 2010/11/27 19:02:27, jochen wrote:
> please remove

Done.

http://codereview.chromium.org/5359005/diff/6001/chrome/browser/in_process_we...
File chrome/browser/in_process_webkit/webkit_context.cc (right):

http://codereview.chromium.org/5359005/diff/6001/chrome/browser/in_process_we...
chrome/browser/in_process_webkit/webkit_context.cc:43: if (clear_on_exit()) {
On 2010/11/27 19:02:27, jochen wrote:
> since we're explicitly deleting the context, you could push to value of
> clear_on_exit to the indexed db context and clear the local state in indexed
> db's destructor. then you also won't need the lock

Done. 

As a result I had to push(done upon construction) the data_path value as well.
However what I realized is that as this is the only peace of information needed
from the webkit context we can remove the pointer to it from the indexed db
context.

http://codereview.chromium.org/5359005/diff/6001/chrome/browser/in_process_we...
chrome/browser/in_process_webkit/webkit_context.cc:59: return clear_on_exit_;
Not needed anymore.

http://codereview.chromium.org/5359005/diff/6001/chrome/browser/in_process_we...
chrome/browser/in_process_webkit/webkit_context.cc:107:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2010/11/27 19:02:27, jochen wrote:
> that shouldn't be required

Removed it.

http://codereview.chromium.org/5359005/diff/6001/chrome/browser/in_process_we...
chrome/browser/in_process_webkit/webkit_context.cc:115: LOG(WARNING) << "Option
changed " << clear_on_exit_;
On 2010/11/27 19:02:27, jochen wrote:
> please remove

Done. It was debug output to test if it is getting properly called.

http://codereview.chromium.org/5359005/diff/6001/chrome/browser/in_process_we...
File chrome/browser/in_process_webkit/webkit_context.h (right):

http://codereview.chromium.org/5359005/diff/6001/chrome/browser/in_process_we...
chrome/browser/in_process_webkit/webkit_context.h:31: : public
NotificationObserver,
On 2010/11/27 19:02:27, jochen wrote:
> notification observer is a pure virtual class, refcountedthreadsafe is not.
the
> coding style says that non-pure virtual classes should come first

Done.

http://codereview.chromium.org/5359005/diff/6001/chrome/browser/in_process_we...
chrome/browser/in_process_webkit/webkit_context.h:85: bool clear_on_exit_;
On 2010/11/27 19:02:27, jochen wrote:
> I'd name this something like clear_local_state_on_exit_. Also the comment
should
> be "True if ..."

Done. I felt it would have been way too long and clear_on_exit was conveying
enough meaning but maybe it is a bit misleading as of what is really cleared and
it is certainly not the webkit context itself.

http://codereview.chromium.org/5359005/diff/6001/chrome/browser/in_process_we...
chrome/browser/in_process_webkit/webkit_context.h:87: Profile* profile_;
On 2010/11/27 19:02:27, jochen wrote:
> what do you need to profile for?

For the Observer to be able to extract the new pref value from the profile.
Added comment about that in the code too.

http://codereview.chromium.org/5359005/diff/13001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/indexed_db_context.h (right):

http://codereview.chromium.org/5359005/diff/13001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_context.h:38: static void
ClearLocalState(const FilePath& indexeddb_path,
I hope you will agree with this change. It allows for much cleaner code around
calling this function and moreover it has nothing to search in another places of
the profile anyways.

http://codereview.chromium.org/5359005/diff/13001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_context.h:43: }
Do I got that correctly that really simple setters and getters can be
implemented inline even if they don't fit a single line?

http://codereview.chromium.org/5359005/diff/13001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_context.h:55: FilePath data_path_;
I replaced the webkit context pointer with the piece of data that we really
needed out of it. It is used in the destructor of this object and we can't rely
on having the parent object around that time so we have to cache this and
therefore the other pointer becomes obsolete.

[pastarmovj, 2010-11-29 09:36:59]

Please review. 10x

[jochen (gone - plz use gerrit), 2010-11-29 10:45:24]

http://codereview.chromium.org/5359005/diff/20001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/indexed_db_browsertest.cc (right):

http://codereview.chromium.org/5359005/diff/20001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_browsertest.cc:111:
context.ClearLocalState(indexeddb_dir, "https");
you should delete the webkit context and check that the files are gone.

http://codereview.chromium.org/5359005/diff/20001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/indexed_db_context.cc (right):

http://codereview.chromium.org/5359005/diff/20001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_context.cc:40:
ClearLocalState(data_path_, chrome::kExtensionScheme);
indent only two spaces. also, ClearLocalState doesn't need to be a member of
IndexedDBContext. You could move it into an anonymous namespace at the top of
this file

http://codereview.chromium.org/5359005/diff/20001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_context.cc:53: return data_path_;
this method should return the complete filename, not just the directory

http://codereview.chromium.org/5359005/diff/20001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/indexed_db_context.h (right):

http://codereview.chromium.org/5359005/diff/20001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_context.h:41: void
SetClearLocalStateOnExit(bool clear_local_state) {
should be set_clear_local_state_on_exit

http://codereview.chromium.org/5359005/diff/20001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/webkit_context.cc (right):

http://codereview.chromium.org/5359005/diff/20001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/webkit_context.cc:99:
clear_local_state_on_exit_ = profile_->GetPrefs()->GetBoolean(
the notification source is the pref service, so you don't need the pointer to
the profile

http://codereview.chromium.org/5359005/diff/20001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/webkit_context.h (right):

http://codereview.chromium.org/5359005/diff/20001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/webkit_context.h:74: ~WebKitContext();
please make this virtual

[pastarmovj, 2010-11-29 12:50:37]

http://codereview.chromium.org/5359005/diff/20001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/indexed_db_browsertest.cc (right):

http://codereview.chromium.org/5359005/diff/20001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_browsertest.cc:111:
context.ClearLocalState(indexeddb_dir, "https");
On 2010/11/29 10:45:24, jochen wrote:
> you should delete the webkit context and check that the files are gone.

done. More refactoring was needed to make this test work again though.

http://codereview.chromium.org/5359005/diff/20001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/indexed_db_context.cc (right):

http://codereview.chromium.org/5359005/diff/20001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_context.cc:40:
ClearLocalState(data_path_, chrome::kExtensionScheme);
On 2010/11/29 10:45:24, jochen wrote:
> indent only two spaces. also, ClearLocalState doesn't need to be a member of
> IndexedDBContext. You could move it into an anonymous namespace at the top of
> this file

Done.

http://codereview.chromium.org/5359005/diff/20001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_context.cc:53: return data_path_;
On 2010/11/29 10:45:24, jochen wrote:
> this method should return the complete filename, not just the directory

Done. Sorry this was a mistake while I was cleaning up the code.

http://codereview.chromium.org/5359005/diff/20001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/indexed_db_context.h (right):

http://codereview.chromium.org/5359005/diff/20001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_context.h:41: void
SetClearLocalStateOnExit(bool clear_local_state) {
On 2010/11/29 10:45:24, jochen wrote:
> should be set_clear_local_state_on_exit

Done.

http://codereview.chromium.org/5359005/diff/20001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/webkit_context.cc (right):

http://codereview.chromium.org/5359005/diff/20001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/webkit_context.cc:99:
clear_local_state_on_exit_ = profile_->GetPrefs()->GetBoolean(
On 2010/11/29 10:45:24, jochen wrote:
> the notification source is the pref service, so you don't need the pointer to
> the profile

Done.

http://codereview.chromium.org/5359005/diff/20001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/webkit_context.h (right):

http://codereview.chromium.org/5359005/diff/20001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/webkit_context.h:74: ~WebKitContext();
On 2010/11/29 10:45:24, jochen wrote:
> please make this virtual

Done.

[jochen (gone - plz use gerrit), 2010-11-29 13:01:09]

http://codereview.chromium.org/5359005/diff/27001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/indexed_db_browsertest.cc (right):

http://codereview.chromium.org/5359005/diff/27001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_browsertest.cc:111: // context which
should trigger the clean up.
can you instead explicitly deref the webkit context so it will always be deleted
before the profile is?

http://codereview.chromium.org/5359005/diff/27001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/webkit_context.h (right):

http://codereview.chromium.org/5359005/diff/27001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/webkit_context.h:84: Profile* profile_;
you shouldn't need that

[pastarmovj, 2010-11-29 13:10:44]

http://codereview.chromium.org/5359005/diff/27001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/indexed_db_browsertest.cc (right):

http://codereview.chromium.org/5359005/diff/27001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_browsertest.cc:111: // context which
should trigger the clean up.
On 2010/11/29 13:01:09, jochen wrote:
> can you instead explicitly deref the webkit context so it will always be
deleted
> before the profile is?

Done.

http://codereview.chromium.org/5359005/diff/27001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/webkit_context.h (right):

http://codereview.chromium.org/5359005/diff/27001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/webkit_context.h:84: Profile* profile_;
On 2010/11/29 13:01:09, jochen wrote:
> you shouldn't need that

True. :)

[jochen (gone - plz use gerrit), 2010-11-29 13:29:33]

http://codereview.chromium.org/5359005/diff/35001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/indexed_db_browsertest.cc (right):

http://codereview.chromium.org/5359005/diff/35001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_browsertest.cc:116:
webkit_context->Release();
release

http://codereview.chromium.org/5359005/diff/35001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/webkit_context.cc (right):

http://codereview.chromium.org/5359005/diff/35001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/webkit_context.cc:24: PrefService* prefs =
profile->GetPrefs();
hum. if the profile is off the record, you should just set
clear_local_state_on_exit_ to false and not observe the pref changes

http://codereview.chromium.org/5359005/diff/35001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/webkit_context.cc:100: {
{ on previous line

http://codereview.chromium.org/5359005/diff/35001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/webkit_context.cc:101:
clear_local_state_on_exit_ = prefs->GetBoolean(
i'd wrap around after the =

[pastarmovj, 2010-11-29 13:45:12]

http://codereview.chromium.org/5359005/diff/35001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/indexed_db_browsertest.cc (right):

http://codereview.chromium.org/5359005/diff/35001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_browsertest.cc:116:
webkit_context->Release();
On 2010/11/29 13:29:33, jochen wrote:
> release

Done.

http://codereview.chromium.org/5359005/diff/35001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/webkit_context.cc (right):

http://codereview.chromium.org/5359005/diff/35001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/webkit_context.cc:24: PrefService* prefs =
profile->GetPrefs();
On 2010/11/29 13:29:33, jochen wrote:
> hum. if the profile is off the record, you should just set
> clear_local_state_on_exit_ to false and not observe the pref changes

Done.

http://codereview.chromium.org/5359005/diff/35001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/webkit_context.cc:100: {
On 2010/11/29 13:29:33, jochen wrote:
> { on previous line

Done.

http://codereview.chromium.org/5359005/diff/35001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/webkit_context.cc:101:
clear_local_state_on_exit_ = prefs->GetBoolean(
On 2010/11/29 13:29:33, jochen wrote:
> i'd wrap around after the =

Done.

[jochen (gone - plz use gerrit), 2010-11-29 13:46:47]

LGTM pending trybot happiness

[pastarmovj, 2010-11-30 13:45:24]

Please review. Passes all try servers.

[jochen (gone - plz use gerrit), 2010-11-30 13:46:25]

LGTM with a small nit

http://codereview.chromium.org/5359005/diff/49001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/indexed_db_browsertest.cc (right):

http://codereview.chromium.org/5359005/diff/49001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_browsertest.cc:108: // create the
scope which will ensure we run the destructor of the webkit
Create