Delete indexedDBs from the cookie tree ui.

* Wire up the delete command in the cookie tree UI to actually delete indexedDBs. Also cleaned up some internal structures used by the UI layer.

* Clean up some loose ends around the transition from sqlite backing to leveldb backing. Code had not been updated to reflect that the indexedDBpath is a directory path instead of a file path.

* Fix a problem with reporting inconsistent size value to the quota manager. Now we always compute deltas against a cached size value and don't change the cached size value w/o reporting a delta.

* Fix a problem with erroneously accessing the filesystem for queries about indexed db usage when working with incognito profiles.

BUG=56249, 76641
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98352

[michaeln, 2011-08-23 20:39:47]

I'll update the CL description to say more about what's in the change, but i
hope the comments below explain things.

http://codereview.chromium.org/7692016/diff/17003/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_indexed_db_helper.cc (left):

http://codereview.chromium.org/7692016/diff/17003/chrome/browser/browsing_dat...
chrome/browser/browsing_data_indexed_db_helper.cc:44: Profile* profile_;
Removed the profile ptr since it's not safe to access the profile ptr on the
webkit thread.

http://codereview.chromium.org/7692016/diff/17003/chrome/browser/browsing_dat...
chrome/browser/browsing_data_indexed_db_helper.cc:113:
WebSecurityOrigin::createFromDatabaseIdentifier(
Got rid of WebKit api usage in this chrome code since its so far removed from
actually poking at webkit/webcore.

http://codereview.chromium.org/7692016/diff/17003/chrome/browser/browsing_dat...
chrome/browser/browsing_data_indexed_db_helper.cc:121: bool ret =
file_util::GetFileInfo(file_path, &file_info);
The actual file_path is an internal impl detail of the indexedDB system, removed
dependencies the UI layer had on it.

http://codereview.chromium.org/7692016/diff/17003/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_indexed_db_helper.cc (right):

http://codereview.chromium.org/7692016/diff/17003/chrome/browser/browsing_dat...
chrome/browser/browsing_data_indexed_db_helper.cc:30: virtual void
DeleteIndexedDB(const GURL& origin_url);
Changed this method signature to use origin_url to indicate what to delete
instead of file_path.

http://codereview.chromium.org/7692016/diff/17003/chrome/browser/browsing_dat...
chrome/browser/browsing_data_indexed_db_helper.cc:137:
indexed_db_context_->DeleteIndexedDBForOrigin(origin_url);
This is actually what i set out to to, wire the UI delete gesture up.

http://codereview.chromium.org/7692016/diff/17003/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_indexed_db_helper.h (left):

http://codereview.chromium.org/7692016/diff/17003/chrome/browser/browsing_dat...
chrome/browser/browsing_data_indexed_db_helper.h:54: FilePath file_path;
Simplified this structure getting rid of duplicitous and unused member.

http://codereview.chromium.org/7692016/diff/17003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service_unittest.cc (left):

http://codereview.chromium.org/7692016/diff/17003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service_unittest.cc:2766:
EXPECT_TRUE(file_util::PathExists(idb_path));
This test was never updated when switching from sqlite backed to leveldb backed
stuff. Changed it to reflect that 'idb_path' actually refers to a directory now.

http://codereview.chromium.org/7692016/diff/17003/chrome/browser/ui/cocoa/con...
File chrome/browser/ui/cocoa/content_settings/cookie_details.mm (right):

http://codereview.chromium.org/7692016/diff/17003/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/content_settings/cookie_details.mm:17: @implementation
CocoaCookieDetails
Is this really still used now that we have the "webui"?

http://codereview.chromium.org/7692016/diff/17003/chrome/browser/ui/gtk/gtk_c...
File chrome/browser/ui/gtk/gtk_chrome_cookie_view.cc (right):

http://codereview.chromium.org/7692016/diff/17003/chrome/browser/ui/gtk/gtk_c...
chrome/browser/ui/gtk/gtk_chrome_cookie_view.cc:589: void
gtk_chrome_cookie_view_display_indexed_db(
Is this really still used now that we have the "webui"?

http://codereview.chromium.org/7692016/diff/17003/chrome/browser/ui/views/ind...
File chrome/browser/ui/views/indexed_db_info_view.cc (right):

http://codereview.chromium.org/7692016/diff/17003/chrome/browser/ui/views/ind...
chrome/browser/ui/views/indexed_db_info_view.cc:26:
IndexedDBInfoView::IndexedDBInfoView()
Is this really still used now that we have the "webui"?

http://codereview.chromium.org/7692016/diff/17003/content/browser/in_process_...
File content/browser/in_process_webkit/indexed_db_context.cc (right):

http://codereview.chromium.org/7692016/diff/17003/content/browser/in_process_...
content/browser/in_process_webkit/indexed_db_context.cc:35: void
GetAllOriginsAndPaths(
Consolidated this directory listing stuff in one place instead of having it
replicated.

http://codereview.chromium.org/7692016/diff/17003/content/browser/in_process_...
content/browser/in_process_webkit/indexed_db_context.cc:125: if
(!webkit_context->is_incognito())
Made sure to not look in the filesystem for incognito.

http://codereview.chromium.org/7692016/diff/17003/content/browser/in_process_...
content/browser/in_process_webkit/indexed_db_context.cc:182: void
IndexedDBContext::GetAllOrigins(std::vector<GURL>* origins) {
This can get called frequently, utilize a cache of the origins using indexedDBs
instead of repeatedly accessing the filesystem.

http://codereview.chromium.org/7692016/diff/17003/content/browser/in_process_...
content/browser/in_process_webkit/indexed_db_context.cc:195: return
origin_size_map_[origin_url];
Depend more heavily on the cached size values here so the set of values reported
to the QM are consistent.

http://codereview.chromium.org/7692016/diff/17003/content/browser/in_process_...
File content/browser/in_process_webkit/indexed_db_context.h (right):

http://codereview.chromium.org/7692016/diff/17003/content/browser/in_process_...
content/browser/in_process_webkit/indexed_db_context.h:66: void
GetAllOrigins(std::vector<GURL>* origins);
Switched to consistently using origin_url in the public interface.

http://codereview.chromium.org/7692016/diff/17003/content/browser/in_process_...
content/browser/in_process_webkit/indexed_db_context.h:91: FilePath
GetIndexedDBFilePath(const string16& origin_id) const;
Made this private!

[michaeln, 2011-08-23 21:54:39]

http://codereview.chromium.org/7692016/diff/17003/content/browser/in_process_...
File content/browser/in_process_webkit/indexed_db_context.cc (left):

http://codereview.chromium.org/7692016/diff/17003/content/browser/in_process_...
content/browser/in_process_webkit/indexed_db_context.cc:52:
file_util::Delete(file_path, false);
Looks like this was never updated in the switch from sqlite to leveldb backing
either. Changed to operate on directories now and also had to update the test in
IndexedDBBrowserTest::ClearLocalState.

[michaeln, 2011-08-23 23:26:25]

hi mike, jochen's on vacaction till sept5 can u take a looks at the cookie tree
helper and ui parts of this instead?

[Mike West, 2011-08-24 08:09:00]

browsing_data_*, mock_browsing_data_*, and cookie_tree_* LGTM with nits, and one
question about the mock.

I think you'll likely need OWNERS approval for the webui bits (estade@, I
think?)

http://codereview.chromium.org/7692016/diff/18013/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_indexed_db_helper.h (right):

http://codereview.chromium.org/7692016/diff/18013/chrome/browser/browsing_dat...
chrome/browser/browsing_data_indexed_db_helper.h:35: const GURL& origin_url,
This is great; thanks for removing redundant data.

Nit: Why `origin_url` and not `origin`? Is there a semantic difference? If not,
shorter variables are better. :)

http://codereview.chromium.org/7692016/diff/18013/chrome/browser/browsing_dat...
chrome/browser/browsing_data_indexed_db_helper.h:62: // Requests a single
indexed database file to be deleted in the WEBKIT thread.
Nit: Probably worth dropping the word 'file' from the comment as well, both here
and in the .cc

http://codereview.chromium.org/7692016/diff/18013/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_indexed_db_helper_unittest.cc (right):

http://codereview.chromium.org/7692016/diff/18013/chrome/browser/browsing_dat...
chrome/browser/browsing_data_indexed_db_helper_unittest.cc:17: TestingProfile
profile;
Do you need this anymore? Looks like it's safe to remove...

http://codereview.chromium.org/7692016/diff/18013/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service_unittest.cc (right):

http://codereview.chromium.org/7692016/diff/18013/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service_unittest.cc:2761: // the file on the
disk.
Nit: You're no longer explicitly creating a file on disk, but a directory
instead.

http://codereview.chromium.org/7692016/diff/18013/chrome/browser/mock_browsin...
File chrome/browser/mock_browsing_data_indexed_db_helper.cc (right):

http://codereview.chromium.org/7692016/diff/18013/chrome/browser/mock_browsin...
chrome/browser/mock_browsing_data_indexed_db_helper.cc:14:
origin2_deleted_(false) {
This is certainly much more straightforward than the previous implementation,
and it does save 6 lines.

It's less flexible, though, and though we don't test more than two origins at
the moment, I'm not convinced that we'll never test more than two. Given that
the old code worked, why throw it away?

[dgrogan, 2011-08-24 22:20:37]

http://codereview.chromium.org/7692016/diff/18013/chrome/browser/ui/cocoa/con...
File chrome/browser/ui/cocoa/content_settings/cookie_details.mm (right):

http://codereview.chromium.org/7692016/diff/18013/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/content_settings/cookie_details.mm:247:
indexedDBInfo->origin_url.spec()) retain]);
What is webui?  Content settings used to be shown in a native dialog and webui
is the current web-based display?

http://codereview.chromium.org/7692016/diff/18013/content/browser/in_process_...
File content/browser/in_process_webkit/indexed_db_context.cc (right):

http://codereview.chromium.org/7692016/diff/18013/content/browser/in_process_...
content/browser/in_process_webkit/indexed_db_context.cc:47: WebKit::WebString
origin_id_webstring =
We have to use the intermediate WebString because the decoding routine is in
WebCore/page/SecurityOrigin.cpp?

http://codereview.chromium.org/7692016/diff/18013/content/browser/in_process_...
content/browser/in_process_webkit/indexed_db_context.cc:158: // Note: This is
not called in response to a UI action in Content Settings. Only
I think you can delete this comment now.

http://codereview.chromium.org/7692016/diff/18013/content/browser/in_process_...
content/browser/in_process_webkit/indexed_db_context.cc:252:
FILE_PATH_LITERAL(".indexeddb"));
While you're thinking about indexeddb clean up stuff...

I cringed when I added this stray ".indexeddb" to match webcore.  Adding it to
kIndexedDBExtension would break the file enumerator in GetAllOriginsAndPaths. 
It would start comparing .leveldb to .indexeddb.leveldb and not finding any
leveldb directories.

Maybe we should just add it to kIndexedDBExtension and change
GetAllOriginsAndPaths to check against the end of the dirname rather than the
Extension.

Altnernatively we could take it out of webcore so that the directories would be
named some_url_80.leveldb/ instead of some_url_80.indexeddb.leveldb/  Dealing
with legacy some_url_80.indexeddb.leveldb/ directories would confuse the new
code though.

Any opinions?

http://codereview.chromium.org/7692016/diff/18013/content/browser/in_process_...
File content/browser/in_process_webkit/indexed_db_context.h (right):

http://codereview.chromium.org/7692016/diff/18013/content/browser/in_process_...
content/browser/in_process_webkit/indexed_db_context.h:15: #include
"base/gtest_prod_util.h"
What's needed from here?  FRIEND_TEST?

[michaeln, 2011-08-25 01:20:04]

ptal

http://codereview.chromium.org/7692016/diff/18013/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_indexed_db_helper.h (right):

http://codereview.chromium.org/7692016/diff/18013/chrome/browser/browsing_dat...
chrome/browser/browsing_data_indexed_db_helper.h:35: const GURL& origin_url,
On 2011/08/24 08:09:00, Mike West wrote:
> This is great; thanks for removing redundant data.
> 
> Nit: Why `origin_url` and not `origin`? Is there a semantic difference? If
not,
> shorter variables are better. :)

Done.

Mostly i just wanted the compiler to help me be sure i got rid of all of the
uses of the old 'origin' data member. Now that i have, i'll switch this to just
'origin' because i appreciate brevity too.

http://codereview.chromium.org/7692016/diff/18013/chrome/browser/browsing_dat...
chrome/browser/browsing_data_indexed_db_helper.h:62: // Requests a single
indexed database file to be deleted in the WEBKIT thread.
On 2011/08/24 08:09:00, Mike West wrote:
> Nit: Probably worth dropping the word 'file' from the comment as well, both
here
> and in the .cc

Done.

http://codereview.chromium.org/7692016/diff/18013/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_indexed_db_helper_unittest.cc (right):

http://codereview.chromium.org/7692016/diff/18013/chrome/browser/browsing_dat...
chrome/browser/browsing_data_indexed_db_helper_unittest.cc:17: TestingProfile
profile;
On 2011/08/24 08:09:00, Mike West wrote:
> Do you need this anymore? Looks like it's safe to remove...

Done, thnx for catching this.

http://codereview.chromium.org/7692016/diff/18013/chrome/browser/mock_browsin...
File chrome/browser/mock_browsing_data_indexed_db_helper.cc (right):

http://codereview.chromium.org/7692016/diff/18013/chrome/browser/mock_browsin...
chrome/browser/mock_browsing_data_indexed_db_helper.cc:14:
origin2_deleted_(false) {
On 2011/08/24 08:09:00, Mike West wrote:
> This is certainly much more straightforward than the previous implementation,
> and it does save 6 lines.
> 
> It's less flexible, though, and though we don't test more than two origins at
> the moment, I'm not convinced that we'll never test more than two. Given that
> the old code worked, why throw it away?

Done.

http://codereview.chromium.org/7692016/diff/18013/chrome/browser/ui/cocoa/con...
File chrome/browser/ui/cocoa/content_settings/cookie_details.mm (right):

http://codereview.chromium.org/7692016/diff/18013/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/content_settings/cookie_details.mm:247:
indexedDBInfo->origin_url.spec()) retain]);
On 2011/08/24 22:20:37, dgrogan wrote:
> What is webui?  Content settings used to be shown in a native dialog and webui
> is the current web-based display?

Yup, so what's with the .mm code still being here i wonder?

http://codereview.chromium.org/7692016/diff/18013/content/browser/in_process_...
File content/browser/in_process_webkit/indexed_db_context.cc (right):

http://codereview.chromium.org/7692016/diff/18013/content/browser/in_process_...
content/browser/in_process_webkit/indexed_db_context.cc:47: WebKit::WebString
origin_id_webstring =
On 2011/08/24 22:20:37, dgrogan wrote:
> We have to use the intermediate WebString because the decoding routine is in
> WebCore/page/SecurityOrigin.cpp?

I just moved this enum loop from the old instance method,
GetAllOriginIdentifiers. Converting from FilePaths to strings is made
intentionally obscure because of variance in character encodings used by
different file systems. The  webkit_glue function is the most convenient way i
know of to do that conversion, it just happens to produce a WebString.

http://codereview.chromium.org/7692016/diff/18013/content/browser/in_process_...
content/browser/in_process_webkit/indexed_db_context.cc:158: // Note: This is
not called in response to a UI action in Content Settings. Only
On 2011/08/24 22:20:37, dgrogan wrote:
> I think you can delete this comment now.

Done.

http://codereview.chromium.org/7692016/diff/18013/content/browser/in_process_...
content/browser/in_process_webkit/indexed_db_context.cc:252:
FILE_PATH_LITERAL(".indexeddb"));
On 2011/08/24 22:20:37, dgrogan wrote:
> While you're thinking about indexeddb clean up stuff...
> 
> I cringed when I added this stray ".indexeddb" to match webcore.  Adding it to
> kIndexedDBExtension would break the file enumerator in GetAllOriginsAndPaths. 
> It would start comparing .leveldb to .indexeddb.leveldb and not finding any
> leveldb directories.
> 
> Maybe we should just add it to kIndexedDBExtension and change
> GetAllOriginsAndPaths to check against the end of the dirname rather than the
> Extension.
> 
> Altnernatively we could take it out of webcore so that the directories would
be
> named some_url_80.leveldb/ instead of some_url_80.indexeddb.leveldb/  Dealing
> with legacy some_url_80.indexeddb.leveldb/ directories would confuse the new
> code though.
> 
> Any opinions?

Are you suggesting we change how the files are named? I think we're stuck with
the filenames that are being produced now since we shipped this already,
unless/until we want to migrate from one naming scheme to another. I think
that's out of scope for this CL.

http://codereview.chromium.org/7692016/diff/18013/content/browser/in_process_...
File content/browser/in_process_webkit/indexed_db_context.h (right):

http://codereview.chromium.org/7692016/diff/18013/content/browser/in_process_...
content/browser/in_process_webkit/indexed_db_context.h:15: #include
"base/gtest_prod_util.h"
On 2011/08/24 22:20:37, dgrogan wrote:
> What's needed from here?  FRIEND_TEST?

Yes

[dgrogan, 2011-08-25 01:45:26]

LGTM

http://codereview.chromium.org/7692016/diff/18013/content/browser/in_process_...
File content/browser/in_process_webkit/indexed_db_context.cc (right):

http://codereview.chromium.org/7692016/diff/18013/content/browser/in_process_...
content/browser/in_process_webkit/indexed_db_context.cc:252:
FILE_PATH_LITERAL(".indexeddb"));
On 2011/08/25 01:20:04, michaeln wrote:
> Are you suggesting we change how the files are named?

Just an option.  But you're right, that'd be way overkill.

> I think we're stuck with
> the filenames that are being produced now since we shipped this already,
> unless/until we want to migrate from one naming scheme to another. I think
> that's out of scope for this CL.