Componentize content_settings_types.h

Componentize content_settings_types.h

BUG=384861
TBR=jochen@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285555

[vasilii, 2014-07-24 09:29:32]

Hi guys,

please review the CL.

[vabr (Chromium), 2014-07-24 09:47:26]

LGTM, thanks!

I was thinking, why this is actually in common and not in browser, because it
seems only to be used by the browser code.
But it has been in common before, so this refactoring step should not change it.
If the OWNERS ever decided to move it, they could do it in the component.

Cheers,
Vaclav

https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
File components/content_settings/core/common/content_settings_types.cc (right):

https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
components/content_settings/core/common/content_settings_types.cc:7: // This is
an empty file to make the Windows compilation work. Thus it can
Maybe put a TODO in here, saying that once there is some other .cc file in the
new component, this dummy one can be removed.

Otherwise, the comment will become confusing (it relies on the fact that there
are no other .cc files).

[jochen (gone - plz use gerrit), 2014-07-24 09:57:40]

agree that it should go into browser

any reason for the core/ directory?

components should go into their own namespace

not sure whether moving a single header is a useful step...

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

https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
components/content_settings.gypi:10: 'type': 'static_library',
just make it 'type': 'none', then you don't need a cc file

https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
components/content_settings.gypi:12: '../base/base.gyp:base',
why do you depend on base?

https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
File components/content_settings/core/common/BUILD.gn (right):

https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
components/content_settings/core/common/BUILD.gn:5: static_library("common") {
should be source_set

https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
components/content_settings/core/common/BUILD.gn:7: "content_settings_types.cc",
not required on gn

[vabr (Chromium), 2014-07-24 10:07:34]

On 2014/07/24 09:57:40, jochen wrote:
> agree that it should go into browser
> 
> any reason for the core/ directory?
Yes, the created component will be layered, i.e., it has a separate layer
depending on content, which cannot be shared by the iOS code (whereas the core/
part can).
> 
> components should go into their own namespace
This will happen ultimately, but not before we move all of that stuff, as having
the namespace just somewhere would mess things up. http://crbug.com/389523
> 
> not sure whether moving a single header is a useful step...
Well, it's the first step, there will be soon more. It is already a long CL, so
I'm actually in favour of doing steps one-per-file.
> 
>
https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
> File components/content_settings.gypi (right):
> 
>
https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
> components/content_settings.gypi:10: 'type': 'static_library',
> just make it 'type': 'none', then you don't need a cc file
> 
>
https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
> components/content_settings.gypi:12: '../base/base.gyp:base',
> why do you depend on base?
> 
>
https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
> File components/content_settings/core/common/BUILD.gn (right):
> 
>
https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
> components/content_settings/core/common/BUILD.gn:5: static_library("common") {
> should be source_set
> 
>
https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
> components/content_settings/core/common/BUILD.gn:7:
"content_settings_types.cc",
> not required on gn

[blundell, 2014-07-24 10:08:52]

On 2014/07/24 09:57:40, jochen wrote:
> agree that it should go into browser
> 

yes, it can go into browser/, as it's not used from the renderer

> any reason for the core/ directory?
> 

i assume because the content_settings component is going to be a layered
component...vabr@ can confirm.

> components should go into their own namespace
> 

i typically recommend doing this step when a componentization is finished to
avoid churn while the componentization is in process

> not sure whether moving a single header is a useful step...
> 

i'm ok with this as a first step personally

>
https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
> File components/content_settings.gypi (right):
> 
>
https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
> components/content_settings.gypi:10: 'type': 'static_library',
> just make it 'type': 'none', then you don't need a cc file
> 
>
https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
> components/content_settings.gypi:12: '../base/base.gyp:base',
> why do you depend on base?
> 
>
https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
> File components/content_settings/core/common/BUILD.gn (right):
> 
>
https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
> components/content_settings/core/common/BUILD.gn:5: static_library("common") {
> should be source_set
> 
>
https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
> components/content_settings/core/common/BUILD.gn:7:
"content_settings_types.cc",
> not required on gn

[vasilii, 2014-07-24 10:52:26]

The enum is used in renderer (see chrome/renderer/content_settings_observer.h).
However, the header is included indirectly through content_settings.h.

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

https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
components/content_settings.gypi:10: 'type': 'static_library',
On 2014/07/24 09:57:40, jochen wrote:
> just make it 'type': 'none', then you don't need a cc file

Done.

https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
components/content_settings.gypi:12: '../base/base.gyp:base',
On 2014/07/24 09:57:40, jochen wrote:
> why do you depend on base?

I'll depend on base later. For now I'm removing it.

https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
File components/content_settings/core/common/BUILD.gn (right):

https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
components/content_settings/core/common/BUILD.gn:5: static_library("common") {
On 2014/07/24 09:57:40, jochen wrote:
> should be source_set

Done.

https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
components/content_settings/core/common/BUILD.gn:7: "content_settings_types.cc",
On 2014/07/24 09:57:40, jochen wrote:
> not required on gn

Done.

https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
File components/content_settings/core/common/content_settings_types.cc (right):

https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
components/content_settings/core/common/content_settings_types.cc:7: // This is
an empty file to make the Windows compilation work. Thus it can
On 2014/07/24 09:47:25, vabr (Chromium) wrote:
> Maybe put a TODO in here, saying that once there is some other .cc file in the
> new component, this dummy one can be removed.
> 
> Otherwise, the comment will become confusing (it relies on the fact that there
> are no other .cc files).

Done.

[vabr (Chromium), 2014-07-24 11:04:00]

On 2014/07/24 10:52:26, vasilii wrote:
> The enum is used in renderer (see
chrome/renderer/content_settings_observer.h).
> However, the header is included indirectly through content_settings.h.

Looking at content_setting.h, it does not use content_settings_types.h at all
(apart from the #include), so I suggest removing that #include, and including
content_settings_types.h directly in places where needed. That's a part of a
healthy clean-up to be done during the componentisation.

> 
>
https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
> File components/content_settings.gypi (right):
> 
>
https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
> components/content_settings.gypi:10: 'type': 'static_library',
> On 2014/07/24 09:57:40, jochen wrote:
> > just make it 'type': 'none', then you don't need a cc file
> 
> Done.
> 
>
https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
> components/content_settings.gypi:12: '../base/base.gyp:base',
> On 2014/07/24 09:57:40, jochen wrote:
> > why do you depend on base?
> 
> I'll depend on base later. For now I'm removing it.
> 
>
https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
> File components/content_settings/core/common/BUILD.gn (right):
> 
>
https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
> components/content_settings/core/common/BUILD.gn:5: static_library("common") {
> On 2014/07/24 09:57:40, jochen wrote:
> > should be source_set
> 
> Done.
> 
>
https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
> components/content_settings/core/common/BUILD.gn:7:
"content_settings_types.cc",
> On 2014/07/24 09:57:40, jochen wrote:
> > not required on gn
> 
> Done.
> 
>
https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
> File components/content_settings/core/common/content_settings_types.cc
(right):
> 
>
https://codereview.chromium.org/410993002/diff/40001/components/content_setti...
> components/content_settings/core/common/content_settings_types.cc:7: // This
is
> an empty file to make the Windows compilation work. Thus it can
> On 2014/07/24 09:47:25, vabr (Chromium) wrote:
> > Maybe put a TODO in here, saying that once there is some other .cc file in
the
> > new component, this dummy one can be removed.
> > 
> > Otherwise, the comment will become confusing (it relies on the fact that
there
> > are no other .cc files).
> 
> Done.

[vasilii, 2014-07-24 11:34:47]

I removed this dependency.

[jochen (gone - plz use gerrit), 2014-07-24 11:55:06]

ok, I defer to blundell@

[vabr (Chromium), 2014-07-24 12:32:52]

Thanks, Vasilii, LGTM.

You need a reviewer from the security team for the change in
chrome/common/render_messages.h. You can probably TBR that one though, because
it does not change anything, only makes an indirect include a direct one.

Cheers,
Vaclav

[vabr (Chromium), 2014-07-24 12:38:07]

Hi Nasko,

Could you please review the change in chrome/common/render_messages.h?

It is only replacing an indirect include with a direct include.

Cheers,
Vaclav

[nasko, 2014-07-24 12:40:12]

IPC LGTM

[vasilii, 2014-07-24 13:35:31]

Thanks, Nasko.
Colin?

[blundell, 2014-07-25 07:00:01]

LGTM, thanks

https://codereview.chromium.org/410993002/diff/80001/components/content_setti...
File components/content_settings/DEPS (right):

https://codereview.chromium.org/410993002/diff/80001/components/content_setti...
components/content_settings/DEPS:1: include_rules = [
nit: No need for an empty DEPS file.

[vasilii, 2014-07-25 08:32:56]

https://codereview.chromium.org/410993002/diff/80001/components/content_setti...
File components/content_settings/DEPS (right):

https://codereview.chromium.org/410993002/diff/80001/components/content_setti...
components/content_settings/DEPS:1: include_rules = [
On 2014/07/25 07:00:01, blundell wrote:
> nit: No need for an empty DEPS file.

Done.

[vasilii, 2014-07-25 08:33:42]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-07-25 08:36:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vasilii@chromium.org/410993002/100001

[commit-bot: I haz the power, 2014-07-25 10:28:00]

Change committed as 285555