Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7063020:
Adding `browsing_data_filesystem_helper*` as the first step towards content settings UI. (Closed)
|
Created:
9 years, 7 months ago by Mike West Modified:
9 years, 7 months ago Reviewers:
michaeln, jochen (gone - plz use gerrit), ericu, kinuko, commit-bot: I haz the power, Eric U., Paweł Hajdan Jr. CC:
chromium-reviews, ian fette, Dai Mikurube (NOT FULLTIME), tzik Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdding `browsing_data_filesystem_helper*` as the first step towards content settings UI.
gypi files.
BUG=63703
TEST=BrowsingDataFilesystemHelperTest* in `browser_tests` and `unit_tests`
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86904
Patch Set 1 #Patch Set 2 : Adding webkit/quota to chrome/DEPS #
Total comments: 26
Patch Set 3 : Cleaning up tests thanks to Paweł #
Total comments: 16
Patch Set 4 : Cleaning up based on Jochen and Eric's feedback #
Total comments: 16
Patch Set 5 : Working in Kinuko's feedback. #
Total comments: 5
Patch Set 6 : Dropping QuotaManager from TestingProfile." #Patch Set 7 : ValidateFileSystemRootAndGetPathOnFileThread #Patch Set 8 : Failing test. #
Total comments: 4
Patch Set 9 : Rebasing onto trunk, fixing broken test. #
Total comments: 2
Patch Set 10 : Stubbing out file system sizes in FileSystemInfo. #Patch Set 11 : Unstubbing filesystem sizes. #Patch Set 12 : Wrong include. #
Total comments: 2
Patch Set 13 : Dropping explicit use of UI thread. #
Messages
Total messages: 26 (0 generated)
Here's step one of the filesystem UI work, which took a good deal longer than I expected. Apologies for setting poor expectations. I'm splitting it up into a few CLs, as it's otherwise way too much to review. This piece adds the browsing_data_filestem_helper framework bits and pieces that cookie_tree_model will end up using to pull in data and render it to the content setting page. I have another CL that I'll upload in parallel later today for those bits. I'm not particularly happy with some pieces of the code here, I want to do some cleanup work to get rid of artifacts borrowed from browsing_data_*_helper (particularly the callback browsertests from database and the `impl` cruft from indexed_db), but this is a fine start for review. :)
Drive-by with testing nits. http://codereview.chromium.org/7063020/diff/2001/chrome/browser/browsing_data... File chrome/browser/browsing_data_filesystem_helper_browsertest.cc (right): http://codereview.chromium.org/7063020/diff/2001/chrome/browser/browsing_data... chrome/browser/browsing_data_filesystem_helper_browsertest.cc:20: #include "content/browser/browser_thread.h" nit: What's going on with those two separate groups of headers? That seems to be against the style guide. http://codereview.chromium.org/7063020/diff/2001/chrome/browser/browsing_data... chrome/browser/browsing_data_filesystem_helper_browsertest.cc:26: namespace { nit: Add an empty line below. http://codereview.chromium.org/7063020/diff/2001/chrome/browser/browsing_data... chrome/browser/browsing_data_filesystem_helper_browsertest.cc:30: nit: Remove redundant empty line. http://codereview.chromium.org/7063020/diff/2001/chrome/browser/browsing_data... chrome/browser/browsing_data_filesystem_helper_browsertest.cc:54: } nit: Add empty line below. http://codereview.chromium.org/7063020/diff/2001/chrome/browser/browsing_data... chrome/browser/browsing_data_filesystem_helper_browsertest.cc:60: file_util::CreateDirectory(target); nit: Just check return value here instead of calling DirectoryExists. http://codereview.chromium.org/7063020/diff/2001/chrome/browser/browsing_data... chrome/browser/browsing_data_filesystem_helper_browsertest.cc:101: NOTREACHED() << info.origin.spec() << " isn't an origin we added."; Avoid NOTREACHED() in tests: only works in Debug mode, and causes a "crash". Did you mean ADD_FAILURE? http://codereview.chromium.org/7063020/diff/2001/chrome/browser/browsing_data... File chrome/browser/browsing_data_filesystem_helper_unittest.cc (right): http://codereview.chromium.org/7063020/diff/2001/chrome/browser/browsing_data... chrome/browser/browsing_data_filesystem_helper_unittest.cc:31: } // namespace nit: Two spaces between "}" and "// namespace" http://codereview.chromium.org/7063020/diff/2001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/7063020/diff/2001/chrome/chrome_tests.gypi#new... chrome/chrome_tests.gypi:1274: 'browser/browsing_data_filesystem_helper_unittest.cc', nit: Sort please. http://codereview.chromium.org/7063020/diff/2001/chrome/chrome_tests.gypi#new... chrome/chrome_tests.gypi:2255: 'browser/browsing_data_filesystem_helper_browsertest.cc', nit: Same here. http://codereview.chromium.org/7063020/diff/2001/chrome/test/testing_profile.cc File chrome/test/testing_profile.cc (right): http://codereview.chromium.org/7063020/diff/2001/chrome/test/testing_profile.... chrome/test/testing_profile.cc:518: if (!file_system_context_) nit: Multi-line "if" body should have braces {}. http://codereview.chromium.org/7063020/diff/2001/chrome/test/testing_profile.... chrome/test/testing_profile.cc:529: DCHECK(file_system_context_.get()); nit: Get rid of this strange DCHECK in testing code. An additional point against it is that checking whether variable assignment and conditional statements work properly is rather pointless. http://codereview.chromium.org/7063020/diff/2001/chrome/test/testing_profile.... chrome/test/testing_profile.cc:534: if (!quota_manager_) nit: Multi-line "if" body should have braces {}. http://codereview.chromium.org/7063020/diff/2001/chrome/test/testing_profile.... chrome/test/testing_profile.cc:540: DCHECK(quota_manager_.get()); nit: Get rid of this strange DCHECK in testing code. An additional point against it is that checking whether variable assignment and conditional statements work properly is rather pointless.
Thanks Paweł. One question regarding DCHECKs in `test_profile.cc`, the rest were quite clear. http://codereview.chromium.org/7063020/diff/2001/chrome/browser/browsing_data... File chrome/browser/browsing_data_filesystem_helper_browsertest.cc (right): http://codereview.chromium.org/7063020/diff/2001/chrome/browser/browsing_data... chrome/browser/browsing_data_filesystem_helper_browsertest.cc:20: #include "content/browser/browser_thread.h" On 2011/05/24 16:30:31, Paweł Hajdan Jr. wrote: > nit: What's going on with those two separate groups of headers? That seems to be > against the style guide. Correct. It's an artifact of how I built the file, and I'm killing it now. http://codereview.chromium.org/7063020/diff/2001/chrome/browser/browsing_data... chrome/browser/browsing_data_filesystem_helper_browsertest.cc:26: namespace { On 2011/05/24 16:30:31, Paweł Hajdan Jr. wrote: > nit: Add an empty line below. Done. http://codereview.chromium.org/7063020/diff/2001/chrome/browser/browsing_data... chrome/browser/browsing_data_filesystem_helper_browsertest.cc:30: On 2011/05/24 16:30:31, Paweł Hajdan Jr. wrote: > nit: Remove redundant empty line. Done. http://codereview.chromium.org/7063020/diff/2001/chrome/browser/browsing_data... chrome/browser/browsing_data_filesystem_helper_browsertest.cc:54: } On 2011/05/24 16:30:31, Paweł Hajdan Jr. wrote: > nit: Add empty line below. Done. http://codereview.chromium.org/7063020/diff/2001/chrome/browser/browsing_data... chrome/browser/browsing_data_filesystem_helper_browsertest.cc:60: file_util::CreateDirectory(target); On 2011/05/24 16:30:31, Paweł Hajdan Jr. wrote: > nit: Just check return value here instead of calling DirectoryExists. Done. http://codereview.chromium.org/7063020/diff/2001/chrome/browser/browsing_data... chrome/browser/browsing_data_filesystem_helper_browsertest.cc:101: NOTREACHED() << info.origin.spec() << " isn't an origin we added."; On 2011/05/24 16:30:31, Paweł Hajdan Jr. wrote: > Avoid NOTREACHED() in tests: only works in Debug mode, and causes a "crash". Did > you mean ADD_FAILURE? I apparently did. http://codereview.chromium.org/7063020/diff/2001/chrome/browser/browsing_data... File chrome/browser/browsing_data_filesystem_helper_unittest.cc (right): http://codereview.chromium.org/7063020/diff/2001/chrome/browser/browsing_data... chrome/browser/browsing_data_filesystem_helper_unittest.cc:31: } // namespace On 2011/05/24 16:30:31, Paweł Hajdan Jr. wrote: > nit: Two spaces between "}" and "// namespace" Done. http://codereview.chromium.org/7063020/diff/2001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/7063020/diff/2001/chrome/chrome_tests.gypi#new... chrome/chrome_tests.gypi:1274: 'browser/browsing_data_filesystem_helper_unittest.cc', On 2011/05/24 16:30:31, Paweł Hajdan Jr. wrote: > nit: Sort please. Done. http://codereview.chromium.org/7063020/diff/2001/chrome/chrome_tests.gypi#new... chrome/chrome_tests.gypi:2255: 'browser/browsing_data_filesystem_helper_browsertest.cc', On 2011/05/24 16:30:31, Paweł Hajdan Jr. wrote: > nit: Same here. Done. http://codereview.chromium.org/7063020/diff/2001/chrome/test/testing_profile.cc File chrome/test/testing_profile.cc (right): http://codereview.chromium.org/7063020/diff/2001/chrome/test/testing_profile.... chrome/test/testing_profile.cc:518: if (!file_system_context_) On 2011/05/24 16:30:31, Paweł Hajdan Jr. wrote: > nit: Multi-line "if" body should have braces {}. Done. http://codereview.chromium.org/7063020/diff/2001/chrome/test/testing_profile.... chrome/test/testing_profile.cc:529: DCHECK(file_system_context_.get()); On 2011/05/24 16:30:31, Paweł Hajdan Jr. wrote: > nit: Get rid of this strange DCHECK in testing code. > > An additional point against it is that checking whether variable assignment and > conditional statements work properly is rather pointless. There are 8 other DCHECKs in this file. I'm happy to remove them as well, but I think there's some value in documenting the expectations that certain functions have. Would replacing them with expectations/asserts be reasonable? http://codereview.chromium.org/7063020/diff/2001/chrome/test/testing_profile.... chrome/test/testing_profile.cc:534: if (!quota_manager_) On 2011/05/24 16:30:31, Paweł Hajdan Jr. wrote: > nit: Multi-line "if" body should have braces {}. Done.
Code I commented in the drive-by LGTM, thanks! http://codereview.chromium.org/7063020/diff/2001/chrome/test/testing_profile.cc File chrome/test/testing_profile.cc (right): http://codereview.chromium.org/7063020/diff/2001/chrome/test/testing_profile.... chrome/test/testing_profile.cc:529: DCHECK(file_system_context_.get()); On 2011/05/24 16:53:51, Mike West wrote: > On 2011/05/24 16:30:31, Paweł Hajdan Jr. wrote: > > nit: Get rid of this strange DCHECK in testing code. > > > > An additional point against it is that checking whether variable assignment > and > > conditional statements work properly is rather pointless. > > There are 8 other DCHECKs in this file. I'm happy to remove them as well, but I > think there's some value in documenting the expectations that certain functions > have. Would replacing them with expectations/asserts be reasonable? No, let's not touch those other DCHECKs for now. I'm not a fan of DCHECKs in test code, but it's not always worth it to remove all of them or replace them with something else (sometimes the failure leads to a crash or hang anyway). http://codereview.chromium.org/7063020/diff/1018/chrome/browser/browsing_data... File chrome/browser/browsing_data_filesystem_helper_browsertest.cc (right): http://codereview.chromium.org/7063020/diff/1018/chrome/browser/browsing_data... chrome/browser/browsing_data_filesystem_helper_browsertest.cc:60: } nit: Add an empty line below.
http://codereview.chromium.org/7063020/diff/1018/chrome/browser/browsing_data... File chrome/browser/browsing_data_filesystem_helper.cc (right): http://codereview.chromium.org/7063020/diff/1018/chrome/browser/browsing_data... chrome/browser/browsing_data_filesystem_helper.cc:240: BrowserThread::PostTask(BrowserThread::WEBKIT, FROM_HERE, NewRunnableMethod( as you're not accessing webkit methods (such as security origin) or actually anything else that would be bound to a specific thread, there's no reason to do this on another thread http://codereview.chromium.org/7063020/diff/1018/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (left): http://codereview.chromium.org/7063020/diff/1018/chrome/chrome_browser.gypi#o... chrome/chrome_browser.gypi:1779: 'browser/safe_browsing/malware_details_history.cc', doesn't belong in this cl?
I don't know the UI stuff, so I mainly just looked over your calls into the filesystem code. It looks fine; I found a few nits and a missing scoped_ptr, but that's all. Thanks for doing this UI! http://codereview.chromium.org/7063020/diff/1018/chrome/browser/browsing_data... File chrome/browser/browsing_data_filesystem_helper.cc (right): http://codereview.chromium.org/7063020/diff/1018/chrome/browser/browsing_data... chrome/browser/browsing_data_filesystem_helper.cc:75: void BrowsingDataFilesystemHelperImpl::StartFetching( We generally capitalize the S in FileSystem, and break there in file names [file_system] as well. http://codereview.chromium.org/7063020/diff/1018/chrome/browser/browsing_data... chrome/browser/browsing_data_filesystem_helper.cc:109: fileapi::SandboxMountPointProvider::OriginEnumerator* origin_enumerator = You're leaking the origin_enumerator. http://codereview.chromium.org/7063020/diff/1018/chrome/browser/browsing_data... chrome/browser/browsing_data_filesystem_helper.cc:120: FilesystemInfo( Why bother storing scheme, host, and port, if you're going to store the whole GURL anyway? http://codereview.chromium.org/7063020/diff/1018/chrome/browser/browsing_data... File chrome/browser/browsing_data_filesystem_helper.h (right): http://codereview.chromium.org/7063020/diff/1018/chrome/browser/browsing_data... chrome/browser/browsing_data_filesystem_helper.h:26: // client of this class need to call StartFetching from the UI thread to typo: needs http://codereview.chromium.org/7063020/diff/1018/chrome/browser/browsing_data... chrome/browser/browsing_data_filesystem_helper.h:79: // not fetch its information from the filesystem tracker, but gets them s/them/it/
Rebased onto trunk, and took care of the points Eric and Jochen raised, and I've asked Kinuko to take a closer look at the code as well. Anything else pop out at you? http://codereview.chromium.org/7063020/diff/1018/chrome/browser/browsing_data... File chrome/browser/browsing_data_filesystem_helper.cc (right): http://codereview.chromium.org/7063020/diff/1018/chrome/browser/browsing_data... chrome/browser/browsing_data_filesystem_helper.cc:75: void BrowsingDataFilesystemHelperImpl::StartFetching( On 2011/05/24 23:08:15, ericu wrote: > We generally capitalize the S in FileSystem, and break there in file names > [file_system] as well. Done. http://codereview.chromium.org/7063020/diff/1018/chrome/browser/browsing_data... chrome/browser/browsing_data_filesystem_helper.cc:109: fileapi::SandboxMountPointProvider::OriginEnumerator* origin_enumerator = On 2011/05/24 23:08:15, ericu wrote: > You're leaking the origin_enumerator. Wrapped in a scoped_ptr, thanks. http://codereview.chromium.org/7063020/diff/1018/chrome/browser/browsing_data... chrome/browser/browsing_data_filesystem_helper.cc:120: FilesystemInfo( On 2011/05/24 23:08:15, ericu wrote: > Why bother storing scheme, host, and port, if you're going to store the whole > GURL anyway? Fair question. I'm sure it made sense at the time. :) http://codereview.chromium.org/7063020/diff/1018/chrome/browser/browsing_data... chrome/browser/browsing_data_filesystem_helper.cc:240: BrowserThread::PostTask(BrowserThread::WEBKIT, FROM_HERE, NewRunnableMethod( On 2011/05/24 19:41:46, jochen wrote: > as you're not accessing webkit methods (such as security origin) or actually > anything else that would be bound to a specific thread, there's no reason to do > this on another thread Ok. I'll drop the hop over to the WEBKIT thread. I thought I'd be able to drop the PostTask mess entirely, but I run into errors calling the callback. The current status works, but it's a bit idiotic to post tasks back to the same thread. Suggestions welcome. :) http://codereview.chromium.org/7063020/diff/1018/chrome/browser/browsing_data... File chrome/browser/browsing_data_filesystem_helper.h (right): http://codereview.chromium.org/7063020/diff/1018/chrome/browser/browsing_data... chrome/browser/browsing_data_filesystem_helper.h:26: // client of this class need to call StartFetching from the UI thread to On 2011/05/24 23:08:15, ericu wrote: > typo: needs Done. http://codereview.chromium.org/7063020/diff/1018/chrome/browser/browsing_data... chrome/browser/browsing_data_filesystem_helper.h:79: // not fetch its information from the filesystem tracker, but gets them On 2011/05/24 23:08:15, ericu wrote: > s/them/it/ Done. http://codereview.chromium.org/7063020/diff/1018/chrome/browser/browsing_data... File chrome/browser/browsing_data_filesystem_helper_browsertest.cc (right): http://codereview.chromium.org/7063020/diff/1018/chrome/browser/browsing_data... chrome/browser/browsing_data_filesystem_helper_browsertest.cc:60: } On 2011/05/24 17:04:19, Paweł Hajdan Jr. wrote: > nit: Add an empty line below. Done. http://codereview.chromium.org/7063020/diff/1018/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (left): http://codereview.chromium.org/7063020/diff/1018/chrome/chrome_browser.gypi#o... chrome/chrome_browser.gypi:1779: 'browser/safe_browsing/malware_details_history.cc', On 2011/05/24 19:41:46, jochen wrote: > doesn't belong in this cl? Done.
Some more comments. (I'm not very familiar with how BrowsingXxx should work either, so please kindly ignore if I'm saying something wrong) http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... File chrome/browser/browsing_data_file_system_helper.cc (right): http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... chrome/browser/browsing_data_file_system_helper.cc:67: completion_callback_(NULL), nit: no need to explicitly initialize it to NULL http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... chrome/browser/browsing_data_file_system_helper.cc:113: sandbox_provider()->CreateOriginEnumerator()); nit: probably this can be done when you initialize the enumerator (in line 110) http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... chrome/browser/browsing_data_file_system_helper.cc:151: fileapi::FileSystemContext* context = profile_->GetFileSystemContext(); scoped_refptr<FileSystemContext> ? (Looks like it's safe but in general) http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... File chrome/browser/browsing_data_file_system_helper.h (right): http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... chrome/browser/browsing_data_file_system_helper.h:66: virtual ~BrowsingDataFileSystemHelper() {} does this destructor need to be public? http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... chrome/browser/browsing_data_file_system_helper.h:120: mutable base::Lock lock_; After all do we access pending_file_system_info_ other than on UI thread? (I might be missing something) http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... File chrome/browser/browsing_data_file_system_helper_browsertest.cc (right): http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... chrome/browser/browsing_data_file_system_helper_browsertest.cc:59: ASSERT_TRUE(file_util::CreateDirectory(target)); I think you'll need to call sandbox_->ValidateFileSystemRootAndGetPathOnFileThread() with create=true to get the path and ensure there's the directory in a proper way. (The above way may stop working when we switch the internal code of the filesystem) http://codereview.chromium.org/7063020/diff/8001/chrome/test/testing_profile.cc File chrome/test/testing_profile.cc (right): http://codereview.chromium.org/7063020/diff/8001/chrome/test/testing_profile.... chrome/test/testing_profile.cc:523: GetQuotaManager()->proxy(), Will we be using the quota manager in the tests using TestingProfile? If we start heavily testing it I guess we'll need to make sure all the storage client modules are created before quota manager is used (like what we do in ProfileImpl::CreateQuotaManagerAndClients()). Otherwise for now I think we can just pass NULL as quota manager proxy to the FileSystemContext constructor.
Btw overall the code looks really nice thanks for working on this, really. On 2011/05/25 10:24:40, kinuko wrote: > Some more comments. (I'm not very familiar with how BrowsingXxx should work > either, so please kindly ignore if I'm saying something wrong) > > http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... > File chrome/browser/browsing_data_file_system_helper.cc (right): > > http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... > chrome/browser/browsing_data_file_system_helper.cc:67: > completion_callback_(NULL), > nit: no need to explicitly initialize it to NULL > > http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... > chrome/browser/browsing_data_file_system_helper.cc:113: > sandbox_provider()->CreateOriginEnumerator()); > nit: probably this can be done when you initialize the enumerator (in line 110) > > http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... > chrome/browser/browsing_data_file_system_helper.cc:151: > fileapi::FileSystemContext* context = profile_->GetFileSystemContext(); > scoped_refptr<FileSystemContext> ? (Looks like it's safe but in general) > > http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... > File chrome/browser/browsing_data_file_system_helper.h (right): > > http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... > chrome/browser/browsing_data_file_system_helper.h:66: virtual > ~BrowsingDataFileSystemHelper() {} > does this destructor need to be public? > > http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... > chrome/browser/browsing_data_file_system_helper.h:120: mutable base::Lock lock_; > After all do we access pending_file_system_info_ other than on UI thread? (I > might be missing something) > > http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... > File chrome/browser/browsing_data_file_system_helper_browsertest.cc (right): > > http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... > chrome/browser/browsing_data_file_system_helper_browsertest.cc:59: > ASSERT_TRUE(file_util::CreateDirectory(target)); > I think you'll need to call > sandbox_->ValidateFileSystemRootAndGetPathOnFileThread() with create=true to get > the path and ensure there's the directory in a proper way. (The above way may > stop working when we switch the internal code of the filesystem) > > http://codereview.chromium.org/7063020/diff/8001/chrome/test/testing_profile.cc > File chrome/test/testing_profile.cc (right): > > http://codereview.chromium.org/7063020/diff/8001/chrome/test/testing_profile.... > chrome/test/testing_profile.cc:523: GetQuotaManager()->proxy(), > Will we be using the quota manager in the tests using TestingProfile? > If we start heavily testing it I guess we'll need to make sure all the storage > client modules are created before quota manager is used (like what we do in > ProfileImpl::CreateQuotaManagerAndClients()). Otherwise for now I think we can > just pass NULL as quota manager proxy to the FileSystemContext constructor.
Thanks Kinuko! Just one remaining question regarding ValidateFileSystemRootAndGetPathOnFileThread. :) http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... File chrome/browser/browsing_data_file_system_helper.cc (right): http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... chrome/browser/browsing_data_file_system_helper.cc:67: completion_callback_(NULL), On 2011/05/25 10:24:41, kinuko wrote: > nit: no need to explicitly initialize it to NULL Done. http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... chrome/browser/browsing_data_file_system_helper.cc:113: sandbox_provider()->CreateOriginEnumerator()); On 2011/05/25 10:24:41, kinuko wrote: > nit: probably this can be done when you initialize the enumerator (in line 110) Done. http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... chrome/browser/browsing_data_file_system_helper.cc:151: fileapi::FileSystemContext* context = profile_->GetFileSystemContext(); On 2011/05/25 10:24:41, kinuko wrote: > scoped_refptr<FileSystemContext> ? (Looks like it's safe but in general) Done. http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... File chrome/browser/browsing_data_file_system_helper.h (right): http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... chrome/browser/browsing_data_file_system_helper.h:66: virtual ~BrowsingDataFileSystemHelper() {} On 2011/05/25 10:24:41, kinuko wrote: > does this destructor need to be public? Probably not. http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... chrome/browser/browsing_data_file_system_helper.h:120: mutable base::Lock lock_; On 2011/05/25 10:24:41, kinuko wrote: > After all do we access pending_file_system_info_ other than on UI thread? (I > might be missing something) Done. http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... File chrome/browser/browsing_data_file_system_helper_browsertest.cc (right): http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... chrome/browser/browsing_data_file_system_helper_browsertest.cc:59: ASSERT_TRUE(file_util::CreateDirectory(target)); On 2011/05/25 10:24:41, kinuko wrote: > I think you'll need to call > sandbox_->ValidateFileSystemRootAndGetPathOnFileThread() with create=true to get > the path and ensure there's the directory in a proper way. (The above way may > stop working when we switch the internal code of the filesystem) Done, but I have no idea what the third argument is supposed to be. It's unused (and called `unused`), so... is it going away, or will it have meaning in the future? http://codereview.chromium.org/7063020/diff/8001/chrome/test/testing_profile.cc File chrome/test/testing_profile.cc (right): http://codereview.chromium.org/7063020/diff/8001/chrome/test/testing_profile.... chrome/test/testing_profile.cc:523: GetQuotaManager()->proxy(), On 2011/05/25 10:24:41, kinuko wrote: > Will we be using the quota manager in the tests using TestingProfile? > If we start heavily testing it I guess we'll need to make sure all the storage > client modules are created before quota manager is used (like what we do in > ProfileImpl::CreateQuotaManagerAndClients()). Otherwise for now I think we can > just pass NULL as quota manager proxy to the FileSystemContext constructor. I think we will, but not in this CL. I'll drop it for the moment, and bring it back to life with the changes you suggest when I get to that point in a future CL. :)
I think it's looking good, to the best of my superficial look. http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... File chrome/browser/browsing_data_file_system_helper_browsertest.cc (right): http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... chrome/browser/browsing_data_file_system_helper_browsertest.cc:59: ASSERT_TRUE(file_util::CreateDirectory(target)); On 2011/05/25 12:00:01, Mike West wrote: > On 2011/05/25 10:24:41, kinuko wrote: > > I think you'll need to call > > sandbox_->ValidateFileSystemRootAndGetPathOnFileThread() with create=true to > get > > the path and ensure there's the directory in a proper way. (The above way may > > stop working when we switch the internal code of the filesystem) > > Done, but I have no idea what the third argument is supposed to be. It's unused > (and called `unused`), so... is it going away, or will it have meaning in the > future? It's not used and unlikely to be used even in the future for the sandbox Web filesystem. It's for other filesystem types (namely for chromeos's external fs) that need to return different root path depending on the given target virtual path. I believe you can just pass FilePath() here (I do so too in other fs tests code), but you may want to wait eric's another glance. http://codereview.chromium.org/7063020/diff/1040/chrome/browser/browsing_data... File chrome/browser/browsing_data_file_system_helper.cc (right): http://codereview.chromium.org/7063020/diff/1040/chrome/browser/browsing_data... chrome/browser/browsing_data_file_system_helper.cc:230: &CannedBrowsingDataFileSystemHelper::ConvertPendingInfo)); So what's blocking to drop this PostTask? If the callback needs to be called asynchronously maybe we can directly call ConvertPendingInfo here but do post task for Notify? (Not understanding the problem though) http://codereview.chromium.org/7063020/diff/1040/chrome/test/testing_profile.cc File chrome/test/testing_profile.cc (right): http://codereview.chromium.org/7063020/diff/1040/chrome/test/testing_profile.... chrome/test/testing_profile.cc:541: return quota_manager_.get(); Maybe we can just return NULL here if we don't use it in GetFileSystemContext?
http://codereview.chromium.org/7063020/diff/1040/chrome/browser/browsing_data... File chrome/browser/browsing_data_file_system_helper.cc (right): http://codereview.chromium.org/7063020/diff/1040/chrome/browser/browsing_data... chrome/browser/browsing_data_file_system_helper.cc:230: &CannedBrowsingDataFileSystemHelper::ConvertPendingInfo)); On 2011/05/25 12:34:58, kinuko wrote: > So what's blocking to drop this PostTask? If the callback needs to be called > asynchronously maybe we can directly call ConvertPendingInfo here but do post > task for Notify? (Not understanding the problem though) Nothing blocks dropping this PostTask. I kept it because I was running into problems getting rid of the second PostTask (for Notify), and I figured that if I had to have one, I might as well replicate the BrowsingDataFileSystemHelper flow entirely. That said, I'm hoping Jochen/Bernhard can simply point out what I'm doing wrong so that I can fold these three methods into one. http://codereview.chromium.org/7063020/diff/1040/chrome/test/testing_profile.cc File chrome/test/testing_profile.cc (right): http://codereview.chromium.org/7063020/diff/1040/chrome/test/testing_profile.... chrome/test/testing_profile.cc:541: return quota_manager_.get(); On 2011/05/25 12:34:58, kinuko wrote: > Maybe we can just return NULL here if we don't use it in GetFileSystemContext? Right, makes sense.
On 2011/05/25 12:55:10, Mike West wrote: > http://codereview.chromium.org/7063020/diff/1040/chrome/browser/browsing_data... > File chrome/browser/browsing_data_file_system_helper.cc (right): > > http://codereview.chromium.org/7063020/diff/1040/chrome/browser/browsing_data... > chrome/browser/browsing_data_file_system_helper.cc:230: > &CannedBrowsingDataFileSystemHelper::ConvertPendingInfo)); > On 2011/05/25 12:34:58, kinuko wrote: > > So what's blocking to drop this PostTask? If the callback needs to be called > > asynchronously maybe we can directly call ConvertPendingInfo here but do post > > task for Notify? (Not understanding the problem though) > > Nothing blocks dropping this PostTask. I kept it because I was running into > problems getting rid of the second PostTask (for Notify), and I figured that if > I had to have one, I might as well replicate the BrowsingDataFileSystemHelper > flow entirely. > > That said, I'm hoping Jochen/Bernhard can simply point out what I'm doing wrong > so that I can fold these three methods into one. well, just do it :) either it works, or I can then see what you're doing incorrectly > > http://codereview.chromium.org/7063020/diff/1040/chrome/test/testing_profile.cc > File chrome/test/testing_profile.cc (right): > > http://codereview.chromium.org/7063020/diff/1040/chrome/test/testing_profile.... > chrome/test/testing_profile.cc:541: return quota_manager_.get(); > On 2011/05/25 12:34:58, kinuko wrote: > > Maybe we can just return NULL here if we don't use it in GetFileSystemContext? > > Right, makes sense.
Helo me Jochen! You're my only hope! http://codereview.chromium.org/7063020/diff/1040/chrome/browser/browsing_data... File chrome/browser/browsing_data_file_system_helper.cc (right): http://codereview.chromium.org/7063020/diff/1040/chrome/browser/browsing_data... chrome/browser/browsing_data_file_system_helper.cc:230: &CannedBrowsingDataFileSystemHelper::ConvertPendingInfo)); On 2011/05/25 12:55:10, Mike West wrote: > On 2011/05/25 12:34:58, kinuko wrote: > > So what's blocking to drop this PostTask? If the callback needs to be called > > asynchronously maybe we can directly call ConvertPendingInfo here but do post > > task for Notify? (Not understanding the problem though) > > Nothing blocks dropping this PostTask. I kept it because I was running into > problems getting rid of the second PostTask (for Notify), and I figured that if > I had to have one, I might as well replicate the BrowsingDataFileSystemHelper > flow entirely. > > That said, I'm hoping Jochen/Bernhard can simply point out what I'm doing wrong > so that I can fold these three methods into one. So, there you go. Broken browser test if I do things this way: [28868:28868:0525/180417:3210019272700:FATAL:message_loop.cc(360)] Check failed: false. Must be inside Run to call Quit Backtrace: base::debug::StackTrace::StackTrace() [0x15ede69] logging::LogMessage::~LogMessage() [0x161459a] MessageLoop::Quit() [0x1618e6a] BrowsingDataHelperCallback<>::callback() [0x47e1a8] DispatchToMethod<>() [0x47fa97] CallbackImpl<>::RunWithParams() [0x47fa05] CallbackRunner<>::Run<>() [0x88fe35] CannedBrowsingDataFileSystemHelper::StartFetching() [0x88de05] (anonymous namespace)::BrowsingDataFileSystemHelperTest_CannedAddFileSystem_Test::RunTestOnMainThread() [0x47b32d] InProcessBrowserTest::RunTestOnMainThreadLoop() [0x156efb3] DispatchToMethod<>() [0x156fd70] RunnableMethod<>::Run() [0x156fc43] BrowserMain() [0x3d3be95] InProcessBrowserTest::SetUp() [0x156ea20] testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x17b11d0] testing::internal::HandleExceptionsInMethodIfSupported<>() [0x17a809d] testing::Test::Run() [0x179ec95] testing::TestInfo::Run() [0x179f53d] testing::TestCase::Run() [0x179fab7] testing::internal::UnitTestImpl::RunAllTests() [0x17a4649] testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x17ae7e0] testing::internal::HandleExceptionsInMethodIfSupported<>() [0x17a95ad] testing::UnitTest::Run() [0x17a433c] base::TestSuite::Run() [0x16a3798] main [0x79a975] 0x7f59ae7cbc4d 0x434f39
http://codereview.chromium.org/7063020/diff/10004/chrome/browser/browsing_dat... File chrome/browser/browsing_data_file_system_helper.cc (right): http://codereview.chromium.org/7063020/diff/10004/chrome/browser/browsing_dat... chrome/browser/browsing_data_file_system_helper.cc:255: callback->Run(file_system_info_); Do you also need to delete the callback as BrowsingDataFileSystemHelperImp does? If the caller doesn't anticipate the callback being called prior to return from StartFetching, you could post a task to call the callback here instead. It might be good to have it behave asyncly regardless since that's how the actual impl behaves. That way classes using this 'mock' will see the same async behavior as the real thing.
On 2011/05/25 18:26:33, michaeln wrote: > http://codereview.chromium.org/7063020/diff/10004/chrome/browser/browsing_dat... > File chrome/browser/browsing_data_file_system_helper.cc (right): > > http://codereview.chromium.org/7063020/diff/10004/chrome/browser/browsing_dat... > chrome/browser/browsing_data_file_system_helper.cc:255: > callback->Run(file_system_info_); > Do you also need to delete the callback as BrowsingDataFileSystemHelperImp does? > > If the caller doesn't anticipate the callback being called prior to return from > StartFetching, you could post a task to call the callback here instead. It might > be good to have it behave asyncly regardless since that's how the actual impl > behaves. That way classes using this 'mock' will see the same async behavior as > the real thing. right, you'll need to manually delete the callback
http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... File chrome/browser/browsing_data_file_system_helper_browsertest.cc (right): http://codereview.chromium.org/7063020/diff/8001/chrome/browser/browsing_data... chrome/browser/browsing_data_file_system_helper_browsertest.cc:59: ASSERT_TRUE(file_util::CreateDirectory(target)); On 2011/05/25 12:34:58, kinuko wrote: > On 2011/05/25 12:00:01, Mike West wrote: > > On 2011/05/25 10:24:41, kinuko wrote: > > > I think you'll need to call > > > sandbox_->ValidateFileSystemRootAndGetPathOnFileThread() with create=true to > > get > > > the path and ensure there's the directory in a proper way. (The above way > may > > > stop working when we switch the internal code of the filesystem) > > > > Done, but I have no idea what the third argument is supposed to be. It's > unused > > (and called `unused`), so... is it going away, or will it have meaning in the > > future? > > It's not used and unlikely to be used even in the future for the sandbox Web > filesystem. It's for other filesystem types (namely for chromeos's external > fs) that need to return different root path depending on the given target > virtual path. > > I believe you can just pass FilePath() here (I do so too in other fs tests > code), but you may want to wait eric's another glance. Confirmed: as long as you know you're calling into SandboxMountPointProvider, it's not used and won't be checked. If you're calling into the superclass, or into the method of the same name on FileSystemPathManager, you need to fill it in with the virtual path just in case. Your code is fine. http://codereview.chromium.org/7063020/diff/10004/chrome/browser/browsing_dat... File chrome/browser/browsing_data_file_system_helper.cc (right): http://codereview.chromium.org/7063020/diff/10004/chrome/browser/browsing_dat... chrome/browser/browsing_data_file_system_helper.cc:117: } If you got back an origin here, but both HasFileSystemType calls returned false, would that cause problems? I'm not sure it's ever supposed to happen, but it certainly could if the user messed around in their filesystem directory, and could also happen if our cleanups haven't been perfect, or if Chrome had crashed at just the right time in the past.
http://codereview.chromium.org/7063020/diff/10004/chrome/browser/browsing_dat... File chrome/browser/browsing_data_file_system_helper.cc (right): http://codereview.chromium.org/7063020/diff/10004/chrome/browser/browsing_dat... chrome/browser/browsing_data_file_system_helper.cc:117: } On 2011/05/25 19:36:30, ericu wrote: > If you got back an origin here, but both HasFileSystemType calls returned false, > would that cause problems? I'm not sure it's ever supposed to happen, but it > certainly could if the user messed around in their filesystem directory, and > could also happen if our cleanups haven't been perfect, or if Chrome had crashed > at just the right time in the past. As currently implemented, it would show up as a filesystem in the content settings UI, but would be empty (both temporary and persistent bits would be false, so they wouldn't show up). This more or less maps to the actual situation on disk (there's a filesystem, but it's borked). I guess that's ok? Changing it so that the filesystem didn't show up at all would be relatively straightforward, I'm happy to do that if you like.
On Wed, May 25, 2011 at 12:50 PM, <mkwst@chromium.org> wrote: > > http://codereview.chromium.org/7063020/diff/10004/chrome/browser/browsing_dat... > File chrome/browser/browsing_data_file_system_helper.cc (right): > > http://codereview.chromium.org/7063020/diff/10004/chrome/browser/browsing_dat... > chrome/browser/browsing_data_file_system_helper.cc:117: } > On 2011/05/25 19:36:30, ericu wrote: >> >> If you got back an origin here, but both HasFileSystemType calls > > returned false, >> >> would that cause problems? I'm not sure it's ever supposed to happen, > > but it >> >> certainly could if the user messed around in their filesystem > > directory, and >> >> could also happen if our cleanups haven't been perfect, or if Chrome > > had crashed >> >> at just the right time in the past. > > As currently implemented, it would show up as a filesystem in the > content settings UI, but would be empty (both temporary and persistent > bits would be false, so they wouldn't show up). This more or less maps > to the actual situation on disk (there's a filesystem, but it's borked). > I guess that's ok? That sounds fine. > Changing it so that the filesystem didn't show up at all would be > relatively straightforward, I'm happy to do that if you like. > > http://codereview.chromium.org/7063020/ >
Ok, tests are working again. I'll let this run through the trybots, and then I think we're good. http://codereview.chromium.org/7063020/diff/10004/chrome/browser/browsing_dat... File chrome/browser/browsing_data_file_system_helper.cc (right): http://codereview.chromium.org/7063020/diff/10004/chrome/browser/browsing_dat... chrome/browser/browsing_data_file_system_helper.cc:255: callback->Run(file_system_info_); On 2011/05/25 18:26:34, michaeln wrote: > Do you also need to delete the callback as BrowsingDataFileSystemHelperImp does? > > If the caller doesn't anticipate the callback being called prior to return from > StartFetching, you could post a task to call the callback here instead. It might > be good to have it behave asyncly regardless since that's how the actual impl > behaves. That way classes using this 'mock' will see the same async behavior as > the real thing. I'm running with this as an async callback; I think simulating that implementation makes sense, even if we're staying on the same thread. Thanks for the feedback!
FileSystem part LGTM (unless eric objects) http://codereview.chromium.org/7063020/diff/15001/chrome/test/testing_profile.h File chrome/test/testing_profile.h (right): http://codereview.chromium.org/7063020/diff/15001/chrome/test/testing_profile... chrome/test/testing_profile.h:389: scoped_refptr<quota::QuotaManager> quota_manager_; We could remove these two lines for now
Added queries for filesystem size via FileSystemQuotaUtil::GetOriginUsageOnFileThread. Since I'm already on the FILE thread inside FetchFileSystemInfoInFileThread, this seems like a reasonable solution. I had a quick chat with Kinuko, who thought it might be alright. How about you, Eric? This should be the last change for this CL. http://codereview.chromium.org/7063020/diff/15001/chrome/test/testing_profile.h File chrome/test/testing_profile.h (right): http://codereview.chromium.org/7063020/diff/15001/chrome/test/testing_profile... chrome/test/testing_profile.h:389: scoped_refptr<quota::QuotaManager> quota_manager_; On 2011/05/26 07:51:36, kinuko wrote: > We could remove these two lines for now Done.
On 2011/05/26 11:36:34, Mike West wrote: > Added queries for filesystem size via > FileSystemQuotaUtil::GetOriginUsageOnFileThread. Since I'm already on the FILE > thread inside FetchFileSystemInfoInFileThread, this seems like a reasonable > solution. > > I had a quick chat with Kinuko, who thought it might be alright. How about you, > Eric? This should be the last change for this CL. Yup, that's fine. LGTM. http://codereview.chromium.org/7063020/diff/15001/chrome/test/testing_profile.h > File chrome/test/testing_profile.h (right): > > http://codereview.chromium.org/7063020/diff/15001/chrome/test/testing_profile... > chrome/test/testing_profile.h:389: scoped_refptr<quota::QuotaManager> > quota_manager_; > On 2011/05/26 07:51:36, kinuko wrote: > > We could remove these two lines for now > > Done.
lgtm2 http://codereview.chromium.org/7063020/diff/25002/chrome/browser/browsing_dat... File chrome/browser/browsing_data_file_system_helper.cc (right): http://codereview.chromium.org/7063020/diff/25002/chrome/browser/browsing_dat... chrome/browser/browsing_data_file_system_helper.cc:279: BrowserThread::UI, FROM_HERE, maybe use MessageLoop::current or MessageLoopProxy::CreateForCurrentThread here instead of the posting to the well known thread id here
Thanks! http://codereview.chromium.org/7063020/diff/25002/chrome/browser/browsing_dat... File chrome/browser/browsing_data_file_system_helper.cc (right): http://codereview.chromium.org/7063020/diff/25002/chrome/browser/browsing_dat... chrome/browser/browsing_data_file_system_helper.cc:279: BrowserThread::UI, FROM_HERE, On 2011/05/26 17:39:53, michaeln wrote: > maybe use MessageLoop::current or MessageLoopProxy::CreateForCurrentThread here > instead of the posting to the well known thread id here Uploading this now. Thanks. I'll commit as soon as the trybot's happy.
Change committed as 86904 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
