Componentize content_settings_provider.h

Componentize content_settings_provider.h

File moved and includes adjusted.

BUG=384865

[vabr (Chromium), 2014-08-01 11:58:04]

Thanks, Lucinka.

Please address the comments below. Once you fix those, we can add markusheintz
for OWNERS review.

Cheers,
Vaclav

https://codereview.chromium.org/430813004/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/content_settings/content_settings_store.h
(right):

https://codereview.chromium.org/430813004/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/content_settings/content_settings_store.h:19:
#include "components/content_settings/core/browser/content_settings_provider.h"
Does not seem to be used. Could you remove?

https://codereview.chromium.org/430813004/diff/1/chrome/browser/notifications...
File chrome/browser/notifications/desktop_notification_profile_util.cc (right):

https://codereview.chromium.org/430813004/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_profile_util.cc:10: #include
"components/content_settings/core/browser/content_settings_provider.h"
It's unfortunate that this needs to be kept #included just because of
NO_RESOURCE_IDENTIFIER, but it's definitely outside the scope of this CL to fix
that. I filed http://crbug.com/399592 for that clean-up.

https://codereview.chromium.org/430813004/diff/1/chrome/browser/notifications...
File chrome/browser/notifications/desktop_notification_service.h (right):

https://codereview.chromium.org/430813004/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.h:23: #include
"components/content_settings/core/browser/content_settings_provider.h"
It does not look needed at all, could you remove it?

https://codereview.chromium.org/430813004/diff/1/chrome/browser/ui/search/ins...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/430813004/diff/1/chrome/browser/ui/search/ins...
chrome/browser/ui/search/instant_controller.cc:24: #include
"components/content_settings/core/browser/content_settings_provider.h"
not used, please remove

https://codereview.chromium.org/430813004/diff/1/chrome/browser/ui/website_se...
File chrome/browser/ui/website_settings/website_settings_unittest.cc (right):

https://codereview.chromium.org/430813004/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/website_settings_unittest.cc:19: #include
"components/content_settings/core/browser/content_settings_provider.h"
ditto

https://codereview.chromium.org/430813004/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/430813004/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_handler.cc:35: #include
"components/content_settings/core/browser/content_settings_provider.h"
ditto

https://codereview.chromium.org/430813004/diff/1/components/content_settings....
File components/content_settings.gypi (right):

https://codereview.chromium.org/430813004/diff/1/components/content_settings....
components/content_settings.gypi:17:
'content_settings/core/browser/content_settings_provider.h',
You need to create a new target (content_settings_core_browser), and put it in
there.

[lucinka.brozkova, 2014-08-01 13:25:09]

Hi Vaclav,

Thank you for your review.

I addressed all your comments.

Cheers,

Lucie

https://codereview.chromium.org/430813004/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/content_settings/content_settings_store.h
(right):

https://codereview.chromium.org/430813004/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/content_settings/content_settings_store.h:19:
#include "components/content_settings/core/browser/content_settings_provider.h"
This is actually used.
On 2014/08/01 11:58:04, vabr (Chromium) wrote:
> Does not seem to be used. Could you remove?

https://codereview.chromium.org/430813004/diff/1/chrome/browser/notifications...
File chrome/browser/notifications/desktop_notification_profile_util.cc (right):

https://codereview.chromium.org/430813004/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_profile_util.cc:10: #include
"components/content_settings/core/browser/content_settings_provider.h"
On 2014/08/01 11:58:04, vabr (Chromium) wrote:
> It's unfortunate that this needs to be kept #included just because of
> NO_RESOURCE_IDENTIFIER, but it's definitely outside the scope of this CL to
fix
> that. I filed http://crbug.com/399592 for that clean-up.

Acknowledged.

https://codereview.chromium.org/430813004/diff/1/chrome/browser/notifications...
File chrome/browser/notifications/desktop_notification_service.h (right):

https://codereview.chromium.org/430813004/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.h:23: #include
"components/content_settings/core/browser/content_settings_provider.h"
Removed, but added to chrome_content_browser_client.cc because of the
'NO_RESOURCE_IDENTIFIER'

On 2014/08/01 11:58:04, vabr (Chromium) wrote:
> It does not look needed at all, could you remove it?

https://codereview.chromium.org/430813004/diff/1/chrome/browser/ui/search/ins...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/430813004/diff/1/chrome/browser/ui/search/ins...
chrome/browser/ui/search/instant_controller.cc:24: #include
"components/content_settings/core/browser/content_settings_provider.h"
On 2014/08/01 11:58:04, vabr (Chromium) wrote:
> not used, please remove

Done.

https://codereview.chromium.org/430813004/diff/1/chrome/browser/ui/website_se...
File chrome/browser/ui/website_settings/website_settings_unittest.cc (right):

https://codereview.chromium.org/430813004/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/website_settings_unittest.cc:19: #include
"components/content_settings/core/browser/content_settings_provider.h"
On 2014/08/01 11:58:04, vabr (Chromium) wrote:
> ditto

Done.

https://codereview.chromium.org/430813004/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/430813004/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_handler.cc:35: #include
"components/content_settings/core/browser/content_settings_provider.h"
On 2014/08/01 11:58:04, vabr (Chromium) wrote:
> ditto

Done.

https://codereview.chromium.org/430813004/diff/1/components/content_settings....
File components/content_settings.gypi (right):

https://codereview.chromium.org/430813004/diff/1/components/content_settings....
components/content_settings.gypi:17:
'content_settings/core/browser/content_settings_provider.h',
On 2014/08/01 11:58:04, vabr (Chromium) wrote:
> You need to create a new target (content_settings_core_browser), and put it in
> there.

Done.

[lucinka.brozkova, 2014-08-01 13:26:55]

Hi Markus,

Could you please do an OWNERS review?

Thank you,

Lucie

[vabr (Chromium), 2014-08-01 13:40:59]

Hi Lucinka,

A comment to the .gypi change below.

You'll need an OWNERS review from jochen@ for most of the files, thankfully it's
mostly trivial name changes (except for the buildfiles).

https://codereview.chromium.org/430813004/diff/20001/components/content_setti...
File components/content_settings.gypi (right):

https://codereview.chromium.org/430813004/diff/20001/components/content_setti...
components/content_settings.gypi:20: # GN version:
//components/content_settings/core/browser
You should also change the BUILD.gn files, see
https://codereview.chromium.org/410993002 for a sample.

[lucinka.brozkova, 2014-08-01 14:00:41]

Hi Vaclav,

Thank you.

Comment addressed.

Cheers,

Lucie

https://codereview.chromium.org/430813004/diff/20001/components/content_setti...
File components/content_settings.gypi (right):

https://codereview.chromium.org/430813004/diff/20001/components/content_setti...
components/content_settings.gypi:20: # GN version:
//components/content_settings/core/browser
On 2014/08/01 13:40:59, vabr (Chromium) wrote:
> You should also change the BUILD.gn files, see
> https://codereview.chromium.org/410993002 for a sample.

Done.

[lucinka.brozkova, 2014-08-01 14:01:31]

Hi Jochen,

Could you please do an OWNERS review?

Thank you,

Lucie

[vabr (Chromium), 2014-08-01 14:32:48]

https://codereview.chromium.org/430813004/diff/10017/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/430813004/diff/10017/chrome/browser/BUILD.gn#...
chrome/browser/BUILD.gn:48: "//components/content_settings/core/browser",
Did you forget to include components/content_settings/core/browser/BUILD.gn in
your patch?

[lucinka.brozkova, 2014-08-01 15:12:20]

Added missing file.

https://codereview.chromium.org/430813004/diff/10017/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/430813004/diff/10017/chrome/browser/BUILD.gn#...
chrome/browser/BUILD.gn:48: "//components/content_settings/core/browser",
Yes, sorry. Fixed now.

On 2014/08/01 14:32:47, vabr (Chromium) wrote:
> Did you forget to include components/content_settings/core/browser/BUILD.gn in
> your patch?

[markusheintz_, 2014-08-01 15:27:20]

LGTM

https://codereview.chromium.org/430813004/diff/50001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.h (right):

https://codereview.chromium.org/430813004/diff/50001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.h:30: class
ContentSettingsPattern;
While you are touching this file anyway. I think this forward declaration is not
used any more. Pls remove.

https://codereview.chromium.org/430813004/diff/50001/chrome/browser/ui/search...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/430813004/diff/50001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_controller.cc:10: #include
"chrome/browser/content_settings/host_content_settings_map.h"
Looks like this is also not used.

[vabr (Chromium), 2014-08-04 10:48:59]

Lucie,

Jochen informed me today that he prefers CLs moving files being done in a batch
for as many files as possible, to spare review time. That's understandable.

However, to allow this CL to land, you should check the files not covered by
Markus' approval, find specific OWNERS (the longer the path before the OWNERS
file, the better, e.g., chrome/browser/extensions/OWNERS), ask them to review
the specific files (do mention them in the review request). For
components/content_settings.gypi you can hopefully ask blundell@, and once all
else is there, maybe Jochen will review the single file chrome/browser/BUILD.gn
(if not, find another chrome/OWNER).

In the next CLs, let's not do the file moving together with dependency breaking,
and only move the files in bigger batches after dependencies are cleared.

Cheers,
Vaclav

[jochen (gone - plz use gerrit), 2014-08-04 11:13:43]

On 2014/08/04 at 10:48:59, vabr wrote:
> Lucie,
> 
> Jochen informed me today that he prefers CLs moving files being done in a
batch for as many files as possible, to spare review time. That's
understandable.
> 
> However, to allow this CL to land, you should check the files not covered by
Markus' approval, find specific OWNERS (the longer the path before the OWNERS
file, the better, e.g., chrome/browser/extensions/OWNERS), ask them to review
the specific files (do mention them in the review request). For
components/content_settings.gypi you can hopefully ask blundell@, and once all
else is there, maybe Jochen will review the single file chrome/browser/BUILD.gn
(if not, find another chrome/OWNER).
> 
> In the next CLs, let's not do the file moving together with dependency
breaking, and only move the files in bigger batches after dependencies are
cleared.
> 
> Cheers,
> Vaclav

I'm happy to cover all parts outside of Markus' approval powers

[lucinka.brozkova, 2014-08-08 10:28:18]

Hi Jochen,

I addressed the comments from Markus.

Could you review these files please?
chrome/browser/BUILD.gn
chrome/browser/extensions/api/content_settings/content_settings_store.h
chrome/browser/media/media_stream_devices_controller.cc
chrome/browser/notifications/desktop_notification_profile_util.cc
chrome/browser/notifications/desktop_notification_service.h
chrome/browser/notifications/desktop_notification_service.cc
chrome/browser/profiles/profile_io_data.cc
chrome/browser/ui/search/instant_controller.cc
chrome/browser/ui/webui/options/content_settings_handler.cc
components/content_settings.gypi

Cheers,

Lucie

https://codereview.chromium.org/430813004/diff/50001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.h (right):

https://codereview.chromium.org/430813004/diff/50001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.h:30: class
ContentSettingsPattern;
On 2014/08/01 15:27:20, markusheintz_ wrote:
> While you are touching this file anyway. I think this forward declaration is
not
> used any more. Pls remove.

Done.

https://codereview.chromium.org/430813004/diff/50001/chrome/browser/ui/search...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/430813004/diff/50001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_controller.cc:10: #include
"chrome/browser/content_settings/host_content_settings_map.h"
On 2014/08/01 15:27:20, markusheintz_ wrote:
> Looks like this is also not used.

Done.