Move content_client.h into content/public.

Move content_client.h into content/public.

TBR=jam@chromium.org
BUG=98716
TEST=it builds

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

[Jói, 2011-10-18 00:55:37]

After many meetings with people, I finally had today's meeting with git grep.

Cheers,
Jói

[Dirk Pranke, 2011-10-18 01:00:38]

http://codereview.chromium.org/8336007/diff/1/content/public/common/content_c...
File content/public/common/content_client.cc (right):

http://codereview.chromium.org/8336007/diff/1/content/public/common/content_c...
content/public/common/content_client.cc:5: #include
"content/public/public/common/content_client.h"
content/public/public?

Also, since there's real logic in this file, I think we want it to stay in
content/common ...

[Jói, 2011-10-18 01:12:15]

Heh, you caught that like 30 seconds before my compiler did :)  I
changed the path manually first, then as part of the global search and
replace.

John and I had discussed before the weekend about whether to allow .cc
files in public and decided this would make our lives easier and not
place an undue burden on embedders.  We can discuss further if you
want.

Cheers,
Jói


On Mon, Oct 17, 2011 at 6:00 PM,  <dpranke@chromium.org> wrote:
>
>
http://codereview.chromium.org/8336007/diff/1/content/public/common/content_c...
> File content/public/common/content_client.cc (right):
>
>
http://codereview.chromium.org/8336007/diff/1/content/public/common/content_c...
> content/public/common/content_client.cc:5: #include
> "content/public/public/common/content_client.h"
> content/public/public?
>
> Also, since there's real logic in this file, I think we want it to stay
> in content/common ...
>
> http://codereview.chromium.org/8336007/
>

[Jói, 2011-10-18 01:38:49]

Uploaded a new version that should compile cleanly (it does on Linux,
and I fixed the one error the Mac trybot spotted).

Cheers,
Jói


2011/10/17 Jói Sigurðsson <joi@chromium.org>:
> Heh, you caught that like 30 seconds before my compiler did :)  I
> changed the path manually first, then as part of the global search and
> replace.
>
> John and I had discussed before the weekend about whether to allow .cc
> files in public and decided this would make our lives easier and not
> place an undue burden on embedders.  We can discuss further if you
> want.
>
> Cheers,
> Jói
>
>
> On Mon, Oct 17, 2011 at 6:00 PM,  <dpranke@chromium.org> wrote:
>>
>>
http://codereview.chromium.org/8336007/diff/1/content/public/common/content_c...
>> File content/public/common/content_client.cc (right):
>>
>>
http://codereview.chromium.org/8336007/diff/1/content/public/common/content_c...
>> content/public/common/content_client.cc:5: #include
>> "content/public/public/common/content_client.h"
>> content/public/public?
>>
>> Also, since there's real logic in this file, I think we want it to stay
>> in content/common ...
>>
>> http://codereview.chromium.org/8336007/
>>
>

[jam, 2011-10-18 01:39:49]

the convention I've been trying to follow in my changes and code that I review
is to not include content_client.h when including any of other process specific
content_X_client.h files. that's why all of the content_X_client.h files include
content_client.h. So you caught all the files that weren't following the
convention. can you just remove the include from there?

[Jói, 2011-10-18 01:41:35]

Sure, will report back soon.

On Mon, Oct 17, 2011 at 6:39 PM,  <jam@chromium.org> wrote:
> the convention I've been trying to follow in my changes and code that I
> review
> is to not include content_client.h when including any of other process
> specific
> content_X_client.h files. that's why all of the content_X_client.h files
> include
> content_client.h. So you caught all the files that weren't following the
> convention. can you just remove the include from there?
>
> http://codereview.chromium.org/8336007/
>

[Jói, 2011-10-18 01:44:00]

Wait, what about IWYU?  If the header file actually uses
ContentClient, we should include it, no?

2011/10/17 Jói Sigurðsson <joi@chromium.org>:
> Sure, will report back soon.
>
> On Mon, Oct 17, 2011 at 6:39 PM,  <jam@chromium.org> wrote:
>> the convention I've been trying to follow in my changes and code that I
>> review
>> is to not include content_client.h when including any of other process
>> specific
>> content_X_client.h files. that's why all of the content_X_client.h files
>> include
>> content_client.h. So you caught all the files that weren't following the
>> convention. can you just remove the include from there?
>>
>> http://codereview.chromium.org/8336007/
>>
>

[jam, 2011-10-18 01:51:43]

If a file only uses content_client.h, then in that case of course we have to
include it.

In almost all cases though, content code does
content::GetContentCllient()->browser()->X(). GetContentClient is from
content_client.h. Since all users of
ContentBrowserClient/ContentRendererClient etc can only get to those
interfaces through content_client.h, it seems redundant to include both
files each time.

On Mon, Oct 17, 2011 at 6:43 PM, Jói Sigurðsson <joi@chromium.org> wrote:

> Wait, what about IWYU?  If the header file actually uses
> ContentClient, we should include it, no?
>
> 2011/10/17 Jói Sigurðsson <joi@chromium.org>:
> > Sure, will report back soon.
> >
> > On Mon, Oct 17, 2011 at 6:39 PM,  <jam@chromium.org> wrote:
> >> the convention I've been trying to follow in my changes and code that I
> >> review
> >> is to not include content_client.h when including any of other process
> >> specific
> >> content_X_client.h files. that's why all of the content_X_client.h files
> >> include
> >> content_client.h. So you caught all the files that weren't following the
> >> convention. can you just remove the include from there?
> >>
> >> http://codereview.chromium.org/8336007/
> >>
> >
>

[jam, 2011-10-18 01:54:21]

and to be even more verbose :) this is not like the scenario where header X
currently includes header Y, so even if you use Y, you can get away by not
including it because you include X. This is fragile because X can change of
course. But in this case, we will always make X include Y because using Y
will always need X, and it seems redundant to always include both.

On Mon, Oct 17, 2011 at 6:51 PM, John Abd-El-Malek <jam@chromium.org> wrote:

> If a file only uses content_client.h, then in that case of course we have
> to include it.
>
> In almost all cases though, content code does
> content::GetContentCllient()->browser()->X(). GetContentClient is from
> content_client.h. Since all users of
> ContentBrowserClient/ContentRendererClient etc can only get to those
> interfaces through content_client.h, it seems redundant to include both
> files each time.
>
>
> On Mon, Oct 17, 2011 at 6:43 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>
>> Wait, what about IWYU?  If the header file actually uses
>> ContentClient, we should include it, no?
>>
>> 2011/10/17 Jói Sigurðsson <joi@chromium.org>:
>> > Sure, will report back soon.
>> >
>> > On Mon, Oct 17, 2011 at 6:39 PM,  <jam@chromium.org> wrote:
>> >> the convention I've been trying to follow in my changes and code that I
>> >> review
>> >> is to not include content_client.h when including any of other process
>> >> specific
>> >> content_X_client.h files. that's why all of the content_X_client.h
>> files
>> >> include
>> >> content_client.h. So you caught all the files that weren't following
>> the
>> >> convention. can you just remove the include from there?
>> >>
>> >> http://codereview.chromium.org/8336007/
>> >>
>> >
>>
>
>

[Dirk Pranke, 2011-10-18 02:27:03]

On Mon, Oct 17, 2011 at 6:33 PM, John Abd-El-Malek <jam@chromium.org>
wrote:>>> On Mon, Oct 17, 2011 at 6:16 PM, Dirk Pranke
<dpranke@chromium.org> wrote:>>
>> The last few times I've talked to John about this I thought we were>> only
going to put .cc files alongside .h files if the .cc files only>> contained
things like constructors that we would've inlined into the>> header except that
we're not allowed to by the style guide.>>>> Did we change our minds on this?
For the record, I don't think we>> should be putting any .cc files under public/
(only headers), but I>> don't really care what the rule is as long as we're
consistent and I>> know what the rule is :).>> I think we have to separate  a
few different scenarios:> -interface.h, interface_impl.h, interface_impl.cc. in
this case, it's> obvious that interface.h should be in public> -functions.h,
functions.cc (i.e. child_process_sandbox_support_linux) where> the cc files has
iimplementations. this is kind of like the above, except> that we don't have a
C++ interface and just have a bunch of functions in a> namespace. i think this
is also obviously like the first case.> The above two cases should cover most of
the files. Then we're left with> things like RenderViewObserver, where the cc
file might do automatic> registration/unregistration. in cases like this, Joi
and I chatted about> whether it makes things better for developers to have them
be separate or> together (we had this while in sf, otherwise would have looped
you in). we> came to the conclusion that putting them side by side will make
things> easier to follow for developers, since they can quickly switch between
files> in their IDE/editor. putting the cc file there also causes no harm. i>
believe this case also applies for ContentClient.
I think there is a small amount of harm in that it makes the number of
files in content/public greater and it means that there are files in
content/public that we don't actually want clients to look at (i.e.,
they're not really public). In my view, this outweighs the benefits of
having the .cc file next to the .h file (and I'm not sure which
editors really need this to be the case, although I know there are
simple vi macros that will make use of this in lieu of tag files to
help you out).

But, I've been outvoted before, and I don't wish to keep beating a
dead horse, so that's fine.
On Mon, Oct 17, 2011 at 6:54 PM, John Abd-El-Malek <jam@chromium.org> wrote:
> and to be even more verbose :) this is not like the scenario where header X
> currently includes header Y, so even if you use Y, you can get away by not
> including it because you include X. This is fragile because X can change of
> course. But in this case, we will always make X include Y because using Y
> will always need X, and it seems redundant to always include both.
>
> On Mon, Oct 17, 2011 at 6:51 PM, John Abd-El-Malek <jam@chromium.org> wrote:
>>
>> If a file only uses content_client.h, then in that case of course we have
>> to include it.
>> In almost all cases though, content code does
>> content::GetContentCllient()->browser()->X(). GetContentClient is from
>> content_client.h. Since all users of
>> ContentBrowserClient/ContentRendererClient etc can only get to those
>> interfaces through content_client.h, it seems redundant to include both
>> files each time.
>>

Perhaps it makes sense to define a content::GetContentBrowserClient()
wrapper in content_browser_client.h (and so on)? Then callers wouldn't
even need to include content_client.h at all. (I am also mildly
allergic to pointer chains like this having to be repeated all the
time).

-- Dirk

[Jói, 2011-10-18 02:37:47]

I don't feel strongly about whether to allow .cc files under public or
not.  It seems easier to allow them for now, and trivial to move them
out of public later, but I'm not opposed to a different approach.

I think the content::GetContentBrowserClient() idea is a good one.

Until we implement that, I'm still not sure if it's right to not
include content_client.h when in fact the file including it uses it;
where do we stop if we do this?  Case in point,
chrome/app/chrome_main_delegate.cc includes only content_client.h out
of the content client headers, but then it includes
chrome_content_browser_client.h which I happen to know includes
content_browser_client.h which we plan to guarantee includes
content_client.h.

I normally wouldn't mind the idea of making it standard practice to
skip content_client.h in the face of e.g. content_browser_client.h,
but I'm concerned that we will have to revert this decision if/when we
start running the IWYU Clang plug-in (which I believe somebody is
working on making compatible with Chrome).

Cheers,
Jói


On Mon, Oct 17, 2011 at 7:26 PM, Dirk Pranke <dpranke@chromium.org> wrote:
> On Mon, Oct 17, 2011 at 6:33 PM, John Abd-El-Malek <jam@chromium.org>
> wrote:>>> On Mon, Oct 17, 2011 at 6:16 PM, Dirk Pranke
> <dpranke@chromium.org> wrote:>>
>>> The last few times I've talked to John about this I thought we were>> only
going to put .cc files alongside .h files if the .cc files only>> contained
things like constructors that we would've inlined into the>> header except that
we're not allowed to by the style guide.>>>> Did we change our minds on this?
For the record, I don't think we>> should be putting any .cc files under public/
(only headers), but I>> don't really care what the rule is as long as we're
consistent and I>> know what the rule is :).>> I think we have to separate  a
few different scenarios:> -interface.h, interface_impl.h, interface_impl.cc. in
this case, it's> obvious that interface.h should be in public> -functions.h,
functions.cc (i.e. child_process_sandbox_support_linux) where> the cc files has
iimplementations. this is kind of like the above, except> that we don't have a
C++ interface and just have a bunch of functions in a> namespace. i think this
is also obviously like the first case.> The above two cases should cover most of
the files. Then we're left with> things like RenderViewObserver, where the cc
file might do automatic> registration/unregistration. in cases like this, Joi
and I chatted about> whether it makes things better for developers to have them
be separate or> together (we had this while in sf, otherwise would have looped
you in). we> came to the conclusion that putting them side by side will make
things> easier to follow for developers, since they can quickly switch between
files> in their IDE/editor. putting the cc file there also causes no harm. i>
believe this case also applies for ContentClient.
> I think there is a small amount of harm in that it makes the number of
> files in content/public greater and it means that there are files in
> content/public that we don't actually want clients to look at (i.e.,
> they're not really public). In my view, this outweighs the benefits of
> having the .cc file next to the .h file (and I'm not sure which
> editors really need this to be the case, although I know there are
> simple vi macros that will make use of this in lieu of tag files to
> help you out).
>
> But, I've been outvoted before, and I don't wish to keep beating a
> dead horse, so that's fine.
> On Mon, Oct 17, 2011 at 6:54 PM, John Abd-El-Malek <jam@chromium.org> wrote:
>> and to be even more verbose :) this is not like the scenario where header X
>> currently includes header Y, so even if you use Y, you can get away by not
>> including it because you include X. This is fragile because X can change of
>> course. But in this case, we will always make X include Y because using Y
>> will always need X, and it seems redundant to always include both.
>>
>> On Mon, Oct 17, 2011 at 6:51 PM, John Abd-El-Malek <jam@chromium.org> wrote:
>>>
>>> If a file only uses content_client.h, then in that case of course we have
>>> to include it.
>>> In almost all cases though, content code does
>>> content::GetContentCllient()->browser()->X(). GetContentClient is from
>>> content_client.h. Since all users of
>>> ContentBrowserClient/ContentRendererClient etc can only get to those
>>> interfaces through content_client.h, it seems redundant to include both
>>> files each time.
>>>
>
> Perhaps it makes sense to define a content::GetContentBrowserClient()
> wrapper in content_browser_client.h (and so on)? Then callers wouldn't
> even need to include content_client.h at all. (I am also mildly
> allergic to pointer chains like this having to be repeated all the
> time).
>
> -- Dirk
>

[Dirk Pranke, 2011-10-18 02:43:17]

On Mon, Oct 17, 2011 at 7:37 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> I don't feel strongly about whether to allow .cc files under public or
> not.  It seems easier to allow them for now, and trivial to move them
> out of public later, but I'm not opposed to a different approach.
>
> I think the content::GetContentBrowserClient() idea is a good one.
>
> Until we implement that, I'm still not sure if it's right to not
> include content_client.h when in fact the file including it uses it;
> where do we stop if we do this?  Case in point,
> chrome/app/chrome_main_delegate.cc includes only content_client.h out
> of the content client headers, but then it includes
> chrome_content_browser_client.h which I happen to know includes
> content_browser_client.h which we plan to guarantee includes
> content_client.h.
>
> I normally wouldn't mind the idea of making it standard practice to
> skip content_client.h in the face of e.g. content_browser_client.h,
> but I'm concerned that we will have to revert this decision if/when we
> start running the IWYU Clang plug-in (which I believe somebody is
> working on making compatible with Chrome).
>

Personally I prefer the IWYU/redundancy, so no argument from me.

-- Dirk

[jam, 2011-10-18 15:55:29]

I'm fine with having getters in content. I think maybe
content::ContentBrowserClient::Get() to match other static getters. We can
put the implementation in content_client.cc so that we don't have to add cc
files for each of the process specific contentclient files.

I'm still not a fan of having everyone include both files. I think if/when
the IWYU clang plugin comes, we can figure out another solution if we don't
do the above.

On Mon, Oct 17, 2011 at 7:42 PM, Dirk Pranke <dpranke@chromium.org> wrote:

> On Mon, Oct 17, 2011 at 7:37 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> > I don't feel strongly about whether to allow .cc files under public or
> > not.  It seems easier to allow them for now, and trivial to move them
> > out of public later, but I'm not opposed to a different approach.
> >
> > I think the content::GetContentBrowserClient() idea is a good one.
> >
> > Until we implement that, I'm still not sure if it's right to not
> > include content_client.h when in fact the file including it uses it;
> > where do we stop if we do this?  Case in point,
> > chrome/app/chrome_main_delegate.cc includes only content_client.h out
> > of the content client headers, but then it includes
> > chrome_content_browser_client.h which I happen to know includes
> > content_browser_client.h which we plan to guarantee includes
> > content_client.h.
> >
> > I normally wouldn't mind the idea of making it standard practice to
> > skip content_client.h in the face of e.g. content_browser_client.h,
> > but I'm concerned that we will have to revert this decision if/when we
> > start running the IWYU Clang plug-in (which I believe somebody is
> > working on making compatible with Chrome).
> >
>
> Personally I prefer the IWYU/redundancy, so no argument from me.
>
> -- Dirk
>

[Jói, 2011-10-18 18:33:43]

OK, will remove the redundant content_client.h includes for now.

On Tue, Oct 18, 2011 at 8:55 AM, John Abd-El-Malek <jam@chromium.org> wrote:
> I'm fine with having getters in content. I think maybe
> content::ContentBrowserClient::Get() to match other static getters. We can
> put the implementation in content_client.cc so that we don't have to add cc
> files for each of the process specific contentclient files.
> I'm still not a fan of having everyone include both files. I think if/when
> the IWYU clang plugin comes, we can figure out another solution if we don't
> do the above.
>
> On Mon, Oct 17, 2011 at 7:42 PM, Dirk Pranke <dpranke@chromium.org> wrote:
>>
>> On Mon, Oct 17, 2011 at 7:37 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>> > I don't feel strongly about whether to allow .cc files under public or
>> > not.  It seems easier to allow them for now, and trivial to move them
>> > out of public later, but I'm not opposed to a different approach.
>> >
>> > I think the content::GetContentBrowserClient() idea is a good one.
>> >
>> > Until we implement that, I'm still not sure if it's right to not
>> > include content_client.h when in fact the file including it uses it;
>> > where do we stop if we do this?  Case in point,
>> > chrome/app/chrome_main_delegate.cc includes only content_client.h out
>> > of the content client headers, but then it includes
>> > chrome_content_browser_client.h which I happen to know includes
>> > content_browser_client.h which we plan to guarantee includes
>> > content_client.h.
>> >
>> > I normally wouldn't mind the idea of making it standard practice to
>> > skip content_client.h in the face of e.g. content_browser_client.h,
>> > but I'm concerned that we will have to revert this decision if/when we
>> > start running the IWYU Clang plug-in (which I believe somebody is
>> > working on making compatible with Chrome).
>> >
>>
>> Personally I prefer the IWYU/redundancy, so no argument from me.
>>
>> -- Dirk
>
>

[Jói, 2011-10-18 22:23:03]

PTAL

[Jói, 2011-10-18 23:36:22]

On 2011/10/18 22:23:03, Jói wrote:
> PTAL

I discussed with John on IM and TBRed with his approval.

[jam, 2011-10-19 00:05:02]

lgtm