Wrapping blocked filesystems into TabSpecificContentSettings

Wrapping blocked filesystems into TabSpecificContentSettings

BUG=63703
TEST=TabSpecificContentSettings* in `unit_tests`


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

[Mike West, 2011-05-24 19:10:58]

In the spirit of reviewing early and often: this is the first piece of pulling
filesystems into TabSpecificContentSettings that's worth looking at in
abstraction.  I still need to do a good bit of of relatively rote work to make
CookiesTreeModel understand filesystems, but I'd like to start the reviews now.
:)  I hope either Jochen or Bernhard will have time to take a look today, and
probably again tomorrow for the rest (Hi Jochen and Bernhard.  :))

This CL depends on http://codereview.chromium.org/7063020/ and can't be
committed until that lands.

[Bernhard Bauer, 2011-05-24 19:42:25]

Can one of you make sure that clearing filesystems is hooked up to
BrowsingDataRemover as well, if necessary?

http://codereview.chromium.org/6966036/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/tab_specific_content_settings.h (right):

http://codereview.chromium.org/6966036/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/tab_specific_content_settings.h:100: // If
access was blocked dur to the user's content settings,
Nit: "due"

[Mike West, 2011-05-25 09:32:45]

http://codereview.chromium.org/6966036/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/tab_specific_content_settings.h (right):

http://codereview.chromium.org/6966036/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/tab_specific_content_settings.h:100: // If
access was blocked dur to the user's content settings,
On 2011/05/24 19:42:25, Bernhard Bauer wrote:
> Nit: "due"

Done.

[Mike West, 2011-05-25 11:33:28]

Uploading the cookies_tree_model changes for review.  Jochen/Bernhard, I think
you're still the right guys for this section.

[jochen (gone - plz use gerrit), 2011-05-25 15:54:09]

LGTM with nits

http://codereview.chromium.org/6966036/diff/9003/chrome/browser/content_setti...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

http://codereview.chromium.org/6966036/diff/9003/chrome/browser/content_setti...
chrome/browser/content_settings/tab_specific_content_settings.cc:39:
databases_->empty() &&
add filesystem_->empty()

http://codereview.chromium.org/6966036/diff/9003/chrome/browser/content_setti...
File chrome/browser/content_settings/tab_specific_content_settings_unittest.cc
(right):

http://codereview.chromium.org/6966036/diff/9003/chrome/browser/content_setti...
chrome/browser/content_settings/tab_specific_content_settings_unittest.cc:83:
net::CookieOptions options;
what do you need the options for?

http://codereview.chromium.org/6966036/diff/9003/chrome/browser/cookies_tree_...
File chrome/browser/cookies_tree_model.h (right):

http://codereview.chromium.org/6966036/diff/9003/chrome/browser/cookies_tree_...
chrome/browser/cookies_tree_model.h:24: #include
"chrome/browser/browsing_data_file_system_helper.h"
duplicate

http://codereview.chromium.org/6966036/diff/9003/chrome/browser/mock_browsing...
File chrome/browser/mock_browsing_data_file_system_helper.cc (right):

http://codereview.chromium.org/6966036/diff/9003/chrome/browser/mock_browsing...
chrome/browser/mock_browsing_data_file_system_helper.cc:38: file_systems_[
origin.spec() ] = true;
no spaces around origin.spec()

http://codereview.chromium.org/6966036/diff/9003/chrome/browser/mock_browsing...
chrome/browser/mock_browsing_data_file_system_helper.cc:62: if (i->second)
{ } required if the body is > 1line

http://codereview.chromium.org/6966036/diff/9003/chrome/browser/renderer_host...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

http://codereview.chromium.org/6966036/diff/9003/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_render_message_filter.cc:374: // If content
was blocked, tell the UI to display the blocked content icon.
that's not what the following lines do. it's more "record access to file system"
or so

[Mike West, 2011-05-25 16:55:16]

Great, thanks Jochen.  I've cleaned up the nits, and I'll just wait on
http://codereview.chromium.org/7063020/ to land before I commit this.

http://codereview.chromium.org/6966036/diff/9003/chrome/browser/content_setti...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

http://codereview.chromium.org/6966036/diff/9003/chrome/browser/content_setti...
chrome/browser/content_settings/tab_specific_content_settings.cc:39:
databases_->empty() &&
On 2011/05/25 15:54:09, jochen wrote:
> add filesystem_->empty()

Good catch.

http://codereview.chromium.org/6966036/diff/9003/chrome/browser/content_setti...
File chrome/browser/content_settings/tab_specific_content_settings_unittest.cc
(right):

http://codereview.chromium.org/6966036/diff/9003/chrome/browser/content_setti...
chrome/browser/content_settings/tab_specific_content_settings_unittest.cc:83:
net::CookieOptions options;
On 2011/05/25 15:54:09, jochen wrote:
> what do you need the options for?

Company?

I copied it down from `BlockedContent` I'm sure; killed it now...

http://codereview.chromium.org/6966036/diff/9003/chrome/browser/cookies_tree_...
File chrome/browser/cookies_tree_model.h (right):

http://codereview.chromium.org/6966036/diff/9003/chrome/browser/cookies_tree_...
chrome/browser/cookies_tree_model.h:24: #include
"chrome/browser/browsing_data_file_system_helper.h"
On 2011/05/25 15:54:09, jochen wrote:
> duplicate

Done.

http://codereview.chromium.org/6966036/diff/9003/chrome/browser/mock_browsing...
File chrome/browser/mock_browsing_data_file_system_helper.cc (right):

http://codereview.chromium.org/6966036/diff/9003/chrome/browser/mock_browsing...
chrome/browser/mock_browsing_data_file_system_helper.cc:38: file_systems_[
origin.spec() ] = true;
On 2011/05/25 15:54:09, jochen wrote:
> no spaces around origin.spec()

Done.

http://codereview.chromium.org/6966036/diff/9003/chrome/browser/mock_browsing...
chrome/browser/mock_browsing_data_file_system_helper.cc:62: if (i->second)
On 2011/05/25 15:54:09, jochen wrote:
> { } required if the body is > 1line

Done.

http://codereview.chromium.org/6966036/diff/9003/chrome/browser/renderer_host...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

http://codereview.chromium.org/6966036/diff/9003/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_render_message_filter.cc:374: // If content
was blocked, tell the UI to display the blocked content icon.
On 2011/05/25 15:54:09, jochen wrote:
> that's not what the following lines do. it's more "record access to file
system"
> or so

Done.

[jochen (gone - plz use gerrit), 2011-05-25 22:53:10]

http://codereview.chromium.org/6966036/diff/11003/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

http://codereview.chromium.org/6966036/diff/11003/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_render_message_filter.cc:374: // Record
access to file system for potential display in UI.
can you fix the same in chrome_worker_process_filter.cc?

[Mike West, 2011-05-27 07:38:58]

Rebased onto trunk.  Will throw into the commit queue once the trybots are
happy.

[commit-bot: I haz the power, 2011-05-27 11:07:47]

Change committed as 87006