Size reduction: halve npchrome_frame.dll via code motion.

Moved ParamTraits specializations that depend on WebKit out of content/common/common_param_traits and into webkit_param_traits.  Also pulled some specializations out of chrome/common/render_messages and into chrome/common/common_param_traits.  This is done so that the MS toolchain can succeed in discarding more code at link time.  This halves the size of npchrome_frame.dll in ordinary "Release" builds.  I hope for a similar reduction in official builds.

BUG=77445
TEST=Everything should just work(TM).

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

[amit, 2011-04-14 15:48:36]

lgtm

http://codereview.chromium.org/6840044/diff/1016/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

http://codereview.chromium.org/6840044/diff/1016/chrome/chrome_common.gypi#ne...
chrome/chrome_common.gypi:136: 'common/automation_message_types.cc',
How about: common_automation_messages.h/cc, chrome_frame_messages.h/cc?

http://codereview.chromium.org/6840044/diff/1016/chrome_frame/chrome_frame.gyp
File chrome_frame/chrome_frame.gyp (right):

http://codereview.chromium.org/6840044/diff/1016/chrome_frame/chrome_frame.gy...
chrome_frame/chrome_frame.gyp:889: '../chrome/chrome.gyp:chrome',
These (chrome, chrome_dll etc) dependencies are there just as a build
convenience i.e. building chrome_frame automatically builds Chrome. We don't
really import anything from them. Maybe another low hanging fruit?

[jam, 2011-04-14 16:22:34]

I'm very apprehensive about an approach that I agree is not maintainable.
 It's hard for people to know which file to look for things, or where to put
new objects, and it's easy to regress.

If we want something hacky, then the least ugly approach IMO would be just
to have the webkit dependency in content_common be behind a dependency so
that either it's not there for Windows build, or it's only there for the
linux shared build.

On Wed, Apr 13, 2011 at 8:16 PM, <grt@chromium.org> wrote:

> Reviewers: John Abd-El-Malek, amit,
>
> Description:
> Size reduction: split some big .cc files in content\common and
> chrome\common in
> two so that the MS toolchain can succeed in discarding more code at link
> time.
> This halves the size of npchrome_frame.dll in ordinary "Release" builds.  I
> hope
> for a similar reduction in official builds.
>
> I moved code around as follows:
>
> - Types previously declared in automation_messages.h are now in
> automation_message_types.h
> - Types previously defined in automation_messages.cc are now in
> automation_message_types.cc and automation_message_types2.cc.  The former
> contains types used only by tests, whereas the latter is for use by tests
> and
> Chrome Frame and has no dependencies on Webkit or Skia.
> - ParamTraits specializations for ContentSetting and ContentSettingsType,
> which
> were in render_messages.h, are now in content_settings_traits.{cc,h}
> - The subset of types previously defined in
> content/common/common_param_traits.cc that are used by Chrome Frame are now
> in
> common_param_traits2.cc (which has no dependencies on Webkit or Skia).
>
> This change could regress fairly easily, so a good next move would be to
> split
> off the parts of content\common and chrome\common used by Chrome Frame into
> their own libs that simply don't depend on WebKit and Skia.  I haven't done
> so
> in this CL since I want to start small.
>
> BUG=77445
> TEST=Everything should just work(TM).
>
> Please review this at http://codereview.chromium.org/6840044/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src/
>
> Affected files:
>  M     chrome/chrome_common.gypi
>  A     chrome/common/automation_message_types.h
>  A     chrome/common/automation_message_types.cc
>  A     chrome/common/automation_message_types2.cc
>  M     chrome/common/automation_messages.h
>  M     chrome/common/automation_messages.cc
>  A     chrome/common/content_settings_traits.h
>  A     chrome/common/content_settings_traits.cc
>  M     chrome/common/render_messages.h
>  M     chrome_frame/chrome_frame.gyp
>  M     content/common/common_param_traits.cc
>  A     content/common/common_param_traits2.cc
>  M     content/content_common.gypi
>
>
>

[amit, 2011-04-14 17:56:47]

Well, I don't think there's anything hacky about refactoring
automation_messages.h et al and this is a step in the right direction.
Immediate next step to eliminate the breakage window is to include chrome
frame relevant portions of automation and content common into a separate lib
instead of linking chrome frame with automation lib.

On Thu, Apr 14, 2011 at 12:22 PM, John Abd-El-Malek <jam@chromium.org>wrote:

> I'm very apprehensive about an approach that I agree is not maintainable.
>  It's hard for people to know which file to look for things, or where to put
> new objects, and it's easy to regress.
>
> If we want something hacky, then the least ugly approach IMO would be just
> to have the webkit dependency in content_common be behind a dependency so
> that either it's not there for Windows build, or it's only there for the
> linux shared build.
>
>
> On Wed, Apr 13, 2011 at 8:16 PM, <grt@chromium.org> wrote:
>
>> Reviewers: John Abd-El-Malek, amit,
>>
>> Description:
>> Size reduction: split some big .cc files in content\common and
>> chrome\common in
>> two so that the MS toolchain can succeed in discarding more code at link
>> time.
>> This halves the size of npchrome_frame.dll in ordinary "Release" builds.
>>  I hope
>> for a similar reduction in official builds.
>>
>> I moved code around as follows:
>>
>> - Types previously declared in automation_messages.h are now in
>> automation_message_types.h
>> - Types previously defined in automation_messages.cc are now in
>> automation_message_types.cc and automation_message_types2.cc.  The former
>> contains types used only by tests, whereas the latter is for use by tests
>> and
>> Chrome Frame and has no dependencies on Webkit or Skia.
>> - ParamTraits specializations for ContentSetting and ContentSettingsType,
>> which
>> were in render_messages.h, are now in content_settings_traits.{cc,h}
>> - The subset of types previously defined in
>> content/common/common_param_traits.cc that are used by Chrome Frame are
>> now in
>> common_param_traits2.cc (which has no dependencies on Webkit or Skia).
>>
>> This change could regress fairly easily, so a good next move would be to
>> split
>> off the parts of content\common and chrome\common used by Chrome Frame
>> into
>> their own libs that simply don't depend on WebKit and Skia.  I haven't
>> done so
>> in this CL since I want to start small.
>>
>> BUG=77445
>> TEST=Everything should just work(TM).
>>
>> Please review this at http://codereview.chromium.org/6840044/
>>
>> SVN Base: svn://svn.chromium.org/chrome/trunk/src/
>>
>> Affected files:
>>  M     chrome/chrome_common.gypi
>>  A     chrome/common/automation_message_types.h
>>  A     chrome/common/automation_message_types.cc
>>  A     chrome/common/automation_message_types2.cc
>>  M     chrome/common/automation_messages.h
>>  M     chrome/common/automation_messages.cc
>>  A     chrome/common/content_settings_traits.h
>>  A     chrome/common/content_settings_traits.cc
>>  M     chrome/common/render_messages.h
>>  M     chrome_frame/chrome_frame.gyp
>>  M     content/common/common_param_traits.cc
>>  A     content/common/common_param_traits2.cc
>>  M     content/content_common.gypi
>>
>>
>>
>

[grt (UTC plus 2), 2011-04-14 17:59:05]

On Thu, Apr 14, 2011 at 12:22 PM, John Abd-El-Malek <jam@chromium.org>wrote:

> I'm very apprehensive about an approach that I agree is not maintainable.
>  It's hard for people to know which file to look for things, or where to put
> new objects, and it's easy to regress.
>
>
Yeah, the file split makes me feel a bit icky, but I figured that it's a
small change that could easily make it into M12.


> If we want something hacky, then the least ugly approach IMO would be just
> to have the webkit dependency in content_common be behind a dependency so
> that either it's not there for Windows build, or it's only there for the
> linux shared build.
>
>
Another approach, which I think would work equally well, would be to replace
common_param_traits with individual FOO_param_traits files, one per
ParamTraits specialization.  Personally, I like this much better than the
giant bag-of-types approach, since it makes it even easier to figure out
where to put a new type (in a new file).  How do you feel about this idea?

[jam, 2011-04-14 18:01:33]

On Thu, Apr 14, 2011 at 10:56 AM, Amit Joshi <amit@chromium.org> wrote:

> Well, I don't think there's anything hacky about refactoring
> automation_messages.h et al and this is a step in the right direction.
> Immediate next step to eliminate the breakage window is to include chrome
> frame relevant portions of automation and content common into a separate lib
> instead of linking chrome frame with automation lib.


I'm fine with the next step, but I'd rather that be done first, instead of
having the same filenames with "2" suffixes.  If something quick is needed,
then removing that explicit dependency can fill the need in the mean time
until a separate "common" can be carved out.

>
>
> On Thu, Apr 14, 2011 at 12:22 PM, John Abd-El-Malek <jam@chromium.org>wrote:
>
>> I'm very apprehensive about an approach that I agree is not maintainable.
>>  It's hard for people to know which file to look for things, or where to put
>> new objects, and it's easy to regress.
>>
>> If we want something hacky, then the least ugly approach IMO would be just
>> to have the webkit dependency in content_common be behind a dependency so
>> that either it's not there for Windows build, or it's only there for the
>> linux shared build.
>>
>>
>> On Wed, Apr 13, 2011 at 8:16 PM, <grt@chromium.org> wrote:
>>
>>> Reviewers: John Abd-El-Malek, amit,
>>>
>>> Description:
>>> Size reduction: split some big .cc files in content\common and
>>> chrome\common in
>>> two so that the MS toolchain can succeed in discarding more code at link
>>> time.
>>> This halves the size of npchrome_frame.dll in ordinary "Release" builds.
>>>  I hope
>>> for a similar reduction in official builds.
>>>
>>> I moved code around as follows:
>>>
>>> - Types previously declared in automation_messages.h are now in
>>> automation_message_types.h
>>> - Types previously defined in automation_messages.cc are now in
>>> automation_message_types.cc and automation_message_types2.cc.  The former
>>> contains types used only by tests, whereas the latter is for use by tests
>>> and
>>> Chrome Frame and has no dependencies on Webkit or Skia.
>>> - ParamTraits specializations for ContentSetting and ContentSettingsType,
>>> which
>>> were in render_messages.h, are now in content_settings_traits.{cc,h}
>>> - The subset of types previously defined in
>>> content/common/common_param_traits.cc that are used by Chrome Frame are
>>> now in
>>> common_param_traits2.cc (which has no dependencies on Webkit or Skia).
>>>
>>> This change could regress fairly easily, so a good next move would be to
>>> split
>>> off the parts of content\common and chrome\common used by Chrome Frame
>>> into
>>> their own libs that simply don't depend on WebKit and Skia.  I haven't
>>> done so
>>> in this CL since I want to start small.
>>>
>>> BUG=77445
>>> TEST=Everything should just work(TM).
>>>
>>> Please review this at http://codereview.chromium.org/6840044/
>>>
>>> SVN Base: svn://svn.chromium.org/chrome/trunk/src/
>>>
>>> Affected files:
>>>  M     chrome/chrome_common.gypi
>>>  A     chrome/common/automation_message_types.h
>>>  A     chrome/common/automation_message_types.cc
>>>  A     chrome/common/automation_message_types2.cc
>>>  M     chrome/common/automation_messages.h
>>>  M     chrome/common/automation_messages.cc
>>>  A     chrome/common/content_settings_traits.h
>>>  A     chrome/common/content_settings_traits.cc
>>>  M     chrome/common/render_messages.h
>>>  M     chrome_frame/chrome_frame.gyp
>>>  M     content/common/common_param_traits.cc
>>>  A     content/common/common_param_traits2.cc
>>>  M     content/content_common.gypi
>>>
>>>
>>>
>>
>

[jam, 2011-04-14 18:03:36]

On Thu, Apr 14, 2011 at 10:58 AM, Greg Thompson <grt@chromium.org> wrote:

> On Thu, Apr 14, 2011 at 12:22 PM, John Abd-El-Malek <jam@chromium.org>wrote:
>
>> I'm very apprehensive about an approach that I agree is not maintainable.
>>  It's hard for people to know which file to look for things, or where to put
>> new objects, and it's easy to regress.
>>
>>
> Yeah, the file split makes me feel a bit icky, but I figured that it's a
> small change that could easily make it into M12.
>

If you just want something quick for M12, why not take out the webkit
dependency for non-linux-shared-library builds?

>
>
>> If we want something hacky, then the least ugly approach IMO would be just
>> to have the webkit dependency in content_common be behind a dependency so
>> that either it's not there for Windows build, or it's only there for the
>> linux shared build.
>>
>>
> Another approach, which I think would work equally well, would be to
> replace common_param_traits with individual FOO_param_traits files, one per
> ParamTraits specialization.  Personally, I like this much better than the
> giant bag-of-types approach, since it makes it even easier to figure out
> where to put a new type (in a new file).  How do you feel about this idea?


I would actually prefer not like having to create a separate file for each
param trait..   That file shouldn't be growing anyways.  There are few
structs that need to be used in more than one message file.

[amit, 2011-04-14 18:04:25]

There are other good reasons to separate test automation messages from
chrome frame messages.  Historically kept in the same file, requirements for
automation messages vs chrome frame messages are diverse.  Automation IPC
messages used for test automation are encouraged to be sync messages to
reduce UI test flakiness. Whereas chrome frame messages are always routed
and can never be sent as sync due to deadlock considerations.

On Thu, Apr 14, 2011 at 1:58 PM, Greg Thompson <grt@chromium.org> wrote:

> On Thu, Apr 14, 2011 at 12:22 PM, John Abd-El-Malek <jam@chromium.org>wrote:
>
>> I'm very apprehensive about an approach that I agree is not maintainable.
>>  It's hard for people to know which file to look for things, or where to put
>> new objects, and it's easy to regress.
>>
>>
> Yeah, the file split makes me feel a bit icky, but I figured that it's a
> small change that could easily make it into M12.
>
>
>> If we want something hacky, then the least ugly approach IMO would be just
>> to have the webkit dependency in content_common be behind a dependency so
>> that either it's not there for Windows build, or it's only there for the
>> linux shared build.
>>
>>
> Another approach, which I think would work equally well, would be to
> replace common_param_traits with individual FOO_param_traits files, one per
> ParamTraits specialization.  Personally, I like this much better than the
> giant bag-of-types approach, since it makes it even easier to figure out
> where to put a new type (in a new file).  How do you feel about this idea?

[grt (UTC plus 2), 2011-04-14 18:10:16]

On Thu, Apr 14, 2011 at 2:03 PM, John Abd-El-Malek <jam@chromium.org> wrote:

>
>
> On Thu, Apr 14, 2011 at 10:58 AM, Greg Thompson <grt@chromium.org> wrote:
>
>> On Thu, Apr 14, 2011 at 12:22 PM, John Abd-El-Malek <jam@chromium.org>wrote:
>>
>>> I'm very apprehensive about an approach that I agree is not maintainable.
>>>  It's hard for people to know which file to look for things, or where to put
>>> new objects, and it's easy to regress.
>>>
>>>
>> Yeah, the file split makes me feel a bit icky, but I figured that it's a
>> small change that could easily make it into M12.
>>
>
> If you just want something quick for M12, why not take out the webkit
> dependency for non-linux-shared-library builds?
>


Can you elaborate?  content_common.lib needs WebKit and Skia to build (and
the rest of Chrome needs those parts).  I'm certainly not a GYP expert, but
I don't see how we can remove that dependency.



> If we want something hacky, then the least ugly approach IMO would be just
>>> to have the webkit dependency in content_common be behind a dependency so
>>> that either it's not there for Windows build, or it's only there for the
>>> linux shared build.
>>>
>>>
>> Another approach, which I think would work equally well, would be to
>> replace common_param_traits with individual FOO_param_traits files, one per
>> ParamTraits specialization.  Personally, I like this much better than the
>> giant bag-of-types approach, since it makes it even easier to figure out
>> where to put a new type (in a new file).  How do you feel about this idea?
>
>
> I would actually prefer not like having to create a separate file for each
> param trait..   That file shouldn't be growing anyways.  There are few
> structs that need to be used in more than one message file.
>
>
You're making my point for me.  :-)  ParamTraits<SkBitmap> seems to be
needed for drag_messages and view_messages.  It's not needed for
automation_messages.  Since it's compiled into the same .obj file as other
types needed by automation_messages, it and Skia get pulled into
npchrome_frame.dll even though they're never used.

[jam, 2011-04-14 18:11:00]

On Thu, Apr 14, 2011 at 11:04 AM, Amit Joshi <amit@chromium.org> wrote:

> There are other good reasons to separate test automation messages from
> chrome frame messages.  Historically kept in the same file, requirements for
> automation messages vs chrome frame messages are diverse.  Automation IPC
> messages used for test automation are encouraged to be sync messages to
> reduce UI test flakiness. Whereas chrome frame messages are always routed
> and can never be sent as sync due to deadlock considerations.


I agree with the above (I actually would prefer if automation didn't use IPC
at all, but that's a different story).  And as I've said before, we do need
a new common that is shared between CF and chrome.  But I don't think that
this change gives us this, so I prefer we don't move all this stuff around
until we have a new library.


>
> On Thu, Apr 14, 2011 at 1:58 PM, Greg Thompson <grt@chromium.org> wrote:
>
>> On Thu, Apr 14, 2011 at 12:22 PM, John Abd-El-Malek <jam@chromium.org>wrote:
>>
>>> I'm very apprehensive about an approach that I agree is not maintainable.
>>>  It's hard for people to know which file to look for things, or where to put
>>> new objects, and it's easy to regress.
>>>
>>>
>> Yeah, the file split makes me feel a bit icky, but I figured that it's a
>> small change that could easily make it into M12.
>>
>>
>>> If we want something hacky, then the least ugly approach IMO would be
>>> just to have the webkit dependency in content_common be behind a dependency
>>> so that either it's not there for Windows build, or it's only there for the
>>> linux shared build.
>>>
>>>
>> Another approach, which I think would work equally well, would be to
>> replace common_param_traits with individual FOO_param_traits files, one per
>> ParamTraits specialization.  Personally, I like this much better than the
>> giant bag-of-types approach, since it makes it even easier to figure out
>> where to put a new type (in a new file).  How do you feel about this idea?
>
>
>

[jam, 2011-04-14 18:25:27]

On Thu, Apr 14, 2011 at 11:10 AM, Greg Thompson <grt@chromium.org> wrote:

>
> On Thu, Apr 14, 2011 at 2:03 PM, John Abd-El-Malek <jam@chromium.org>wrote:
>
>>
>>
>> On Thu, Apr 14, 2011 at 10:58 AM, Greg Thompson <grt@chromium.org> wrote:
>>
>>> On Thu, Apr 14, 2011 at 12:22 PM, John Abd-El-Malek <jam@chromium.org>wrote:
>>>
>>>> I'm very apprehensive about an approach that I agree is not
>>>> maintainable.  It's hard for people to know which file to look for things,
>>>> or where to put new objects, and it's easy to regress.
>>>>
>>>>
>>> Yeah, the file split makes me feel a bit icky, but I figured that it's a
>>> small change that could easily make it into M12.
>>>
>>
>> If you just want something quick for M12, why not take out the webkit
>> dependency for non-linux-shared-library builds?
>>
>
>
> Can you elaborate?  content_common.lib needs WebKit and Skia to build (and
> the rest of Chrome needs those parts).  I'm certainly not a GYP expert, but
> I don't see how we can remove that dependency.
>

The WebKit dependency got added in
http://src.chromium.org/viewvc/chrome?view=rev&revision=77976 to fix the
Linux shared library build.

Per the thread on the chrome frame mailing team, I had to add this when I
moved some code from chrome\common to content\common, because the linux
shared library build broke.  I don't know how why we didn't need
this explicit dependency in chrome\common.  I suspect that when that library
is generated in the linux shared library build, it links with other stuff
that bring in WebKit.  Note that this dependency is ONLY needed for the
linux shared library build. It's technically correct for all other builds,
but not necessary, since they don't build content_common into a separate
target.

Here's the change that I mean: http://codereview.chromium.org/6854027


>
>
>>  If we want something hacky, then the least ugly approach IMO would be
>>>> just to have the webkit dependency in content_common be behind a dependency
>>>> so that either it's not there for Windows build, or it's only there for the
>>>> linux shared build.
>>>>
>>>>
>>> Another approach, which I think would work equally well, would be to
>>> replace common_param_traits with individual FOO_param_traits files, one per
>>> ParamTraits specialization.  Personally, I like this much better than the
>>> giant bag-of-types approach, since it makes it even easier to figure out
>>> where to put a new type (in a new file).  How do you feel about this idea?
>>
>>
>> I would actually prefer not like having to create a separate file for each
>> param trait..   That file shouldn't be growing anyways.  There are few
>> structs that need to be used in more than one message file.
>>
>>
> You're making my point for me.  :-)  ParamTraits<SkBitmap> seems to be
> needed for drag_messages and view_messages.  It's not needed for
> automation_messages.  Since it's compiled into the same .obj file as other
> types needed by automation_messages, it and Skia get pulled into
> npchrome_frame.dll even though they're never used.
>

[jam, 2011-04-14 19:43:01]

On Thu, Apr 14, 2011 at 11:25 AM, John Abd-El-Malek <jam@chromium.org>wrote:

>
>
> On Thu, Apr 14, 2011 at 11:10 AM, Greg Thompson <grt@chromium.org> wrote:
>
>>
>> On Thu, Apr 14, 2011 at 2:03 PM, John Abd-El-Malek <jam@chromium.org>wrote:
>>
>>>
>>>
>>> On Thu, Apr 14, 2011 at 10:58 AM, Greg Thompson <grt@chromium.org>wrote:
>>>
>>>> On Thu, Apr 14, 2011 at 12:22 PM, John Abd-El-Malek
<jam@chromium.org>wrote:
>>>>
>>>>> I'm very apprehensive about an approach that I agree is not
>>>>> maintainable.  It's hard for people to know which file to look for things,
>>>>> or where to put new objects, and it's easy to regress.
>>>>>
>>>>>
>>>> Yeah, the file split makes me feel a bit icky, but I figured that it's a
>>>> small change that could easily make it into M12.
>>>>
>>>
>>> If you just want something quick for M12, why not take out the webkit
>>> dependency for non-linux-shared-library builds?
>>>
>>
>>
>> Can you elaborate?  content_common.lib needs WebKit and Skia to build (and
>> the rest of Chrome needs those parts).  I'm certainly not a GYP expert, but
>> I don't see how we can remove that dependency.
>>
>
> The WebKit dependency got added in
> http://src.chromium.org/viewvc/chrome?view=rev&revision=77976 to fix the
> Linux shared library build.
>
> Per the thread on the chrome frame mailing team, I had to add this when I
> moved some code from chrome\common to content\common, because the linux
> shared library build broke.  I don't know how why we didn't need
> this explicit dependency in chrome\common.  I suspect that when that library
> is generated in the linux shared library build, it links with other stuff
> that bring in WebKit.  Note that this dependency is ONLY needed for the
> linux shared library build. It's technically correct for all other builds,
> but not necessary, since they don't build content_common into a separate
> target.
>
> Here's the change that I mean: http://codereview.chromium.org/6854027
>

ok, actually I tried this through the trybot, and now I see that this won't
work anymore (it would have worked at that point, but now more files in
content\common include webkit code that get linked in).

Now that I think about it, I think this size increase might have happened
because there used to be a separate file for webkit_param_traits in
chrome\common, and when I moved these to content, I put them together with
the other param traits.  So what if we go back to that model, and have a
webkit_param_traits in content\common?  That way it would be easy for people
to figure out where to put/find these, and it would be a logical separation.
 The message files can also be split into automation_messages and
chrome_frame_messages like Amit said.

>
>
>>
>>
>>>  If we want something hacky, then the least ugly approach IMO would be
>>>>> just to have the webkit dependency in content_common be behind a
dependency
>>>>> so that either it's not there for Windows build, or it's only there for
the
>>>>> linux shared build.
>>>>>
>>>>>
>>>> Another approach, which I think would work equally well, would be to
>>>> replace common_param_traits with individual FOO_param_traits files, one per
>>>> ParamTraits specialization.  Personally, I like this much better than the
>>>> giant bag-of-types approach, since it makes it even easier to figure out
>>>> where to put a new type (in a new file).  How do you feel about this idea?
>>>
>>>
>>> I would actually prefer not like having to create a separate file for
>>> each param trait..   That file shouldn't be growing anyways.  There are few
>>> structs that need to be used in more than one message file.
>>>
>>>
>> You're making my point for me.  :-)  ParamTraits<SkBitmap> seems to be
>> needed for drag_messages and view_messages.  It's not needed for
>> automation_messages.  Since it's compiled into the same .obj file as other
>> types needed by automation_messages, it and Skia get pulled into
>> npchrome_frame.dll even though they're never used.
>>
>
>

[grt (UTC plus 2), 2011-04-15 18:42:32]

Hi John,

PTAL.  I've made the changes we discussed yesterday.  Thanks.

[jam, 2011-04-15 19:01:21]

http://codereview.chromium.org/6840044/diff/11005/chrome/common/common_param_...
File chrome/common/common_param_traits.cc (right):

http://codereview.chromium.org/6840044/diff/11005/chrome/common/common_param_...
chrome/common/common_param_traits.cc:6: #include "ipc/ipc_message.h"
nit: need blank line before this

http://codereview.chromium.org/6840044/diff/11005/content/common/common_param...
File content/common/common_param_traits.cc (right):

http://codereview.chromium.org/6840044/diff/11005/content/common/common_param...
content/common/common_param_traits.cc:13: #include "ui/gfx/rect.h"
rect.h is enough, since it brings in the other two

http://codereview.chromium.org/6840044/diff/11005/content/common/content_mess...
File content/common/content_message_generator.cc (right):

http://codereview.chromium.org/6840044/diff/11005/content/common/content_mess...
content/common/content_message_generator.cc:8: #include
"content/common/webkit_param_traits.h"
why is this needed, since you include it in the headers?

[grt (UTC plus 2), 2011-04-15 19:39:53]

PTAL.

http://codereview.chromium.org/6840044/diff/11005/chrome/common/common_param_...
File chrome/common/common_param_traits.cc (right):

http://codereview.chromium.org/6840044/diff/11005/chrome/common/common_param_...
chrome/common/common_param_traits.cc:6: #include "ipc/ipc_message.h"
On 2011/04/15 19:01:21, John Abd-El-Malek wrote:
> nit: need blank line before this

Done.

http://codereview.chromium.org/6840044/diff/11005/content/common/common_param...
File content/common/common_param_traits.cc (right):

http://codereview.chromium.org/6840044/diff/11005/content/common/common_param...
content/common/common_param_traits.cc:13: #include "ui/gfx/rect.h"
On 2011/04/15 19:01:21, John Abd-El-Malek wrote:
> rect.h is enough, since it brings in the other two

Done.

http://codereview.chromium.org/6840044/diff/11005/content/common/content_mess...
File content/common/content_message_generator.cc (right):

http://codereview.chromium.org/6840044/diff/11005/content/common/content_mess...
content/common/content_message_generator.cc:8: #include
"content/common/webkit_param_traits.h"
On 2011/04/15 19:01:21, John Abd-El-Malek wrote:
> why is this needed, since you include it in the headers?

As it turns out, neither of these are needed, but ui/gfx/point.h is.  Thanks.

[jam, 2011-04-15 19:42:19]

http://codereview.chromium.org/6840044/diff/11005/content/common/content_mess...
File content/common/content_message_generator.cc (right):

http://codereview.chromium.org/6840044/diff/11005/content/common/content_mess...
content/common/content_message_generator.cc:8: #include
"content/common/webkit_param_traits.h"
On 2011/04/15 19:39:54, grt wrote:
> On 2011/04/15 19:01:21, John Abd-El-Malek wrote:
> > why is this needed, since you include it in the headers?
> 
> As it turns out, neither of these are needed, but ui/gfx/point.h is.  Thanks.

oh, if point.h is needed, then we need to add it to whatever header needs it but
didn't use it, instead of in here..

[grt (UTC plus 2), 2011-04-15 19:45:30]

On 2011/04/15 19:42:19, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/6840044/diff/11005/content/common/content_mess...
> File content/common/content_message_generator.cc (right):
> 
>
http://codereview.chromium.org/6840044/diff/11005/content/common/content_mess...
> content/common/content_message_generator.cc:8: #include
> "content/common/webkit_param_traits.h"
> On 2011/04/15 19:39:54, grt wrote:
> > On 2011/04/15 19:01:21, John Abd-El-Malek wrote:
> > > why is this needed, since you include it in the headers?
> > 
> > As it turns out, neither of these are needed, but ui/gfx/point.h is. 
Thanks.
> 
> oh, if point.h is needed, then we need to add it to whatever header needs it
but
> didn't use it, instead of in here..

gfx::Point is (and has been) forward decl'd in common_param_traits.h.  This file
needs the full definition.  Including point.h in the .h seems to go against
coding guidelines.

[jam, 2011-04-15 19:49:43]

On Fri, Apr 15, 2011 at 12:45 PM, <grt@chromium.org> wrote:

> On 2011/04/15 19:42:19, John Abd-El-Malek wrote:
>
>
>
http://codereview.chromium.org/6840044/diff/11005/content/common/content_mess...
>
>> File content/common/content_message_generator.cc (right):
>>
>
>
>
>
http://codereview.chromium.org/6840044/diff/11005/content/common/content_mess...
>
>> content/common/content_message_generator.cc:8: #include
>> "content/common/webkit_param_traits.h"
>> On 2011/04/15 19:39:54, grt wrote:
>> > On 2011/04/15 19:01:21, John Abd-El-Malek wrote:
>> > > why is this needed, since you include it in the headers?
>> >
>> > As it turns out, neither of these are needed, but ui/gfx/point.h is.
>>
> Thanks.
>
>  oh, if point.h is needed, then we need to add it to whatever header needs
>> it
>>
> but
>
>> didn't use it, instead of in here..
>>
>
> gfx::Point is (and has been) forward decl'd in common_param_traits.h.  This
> file
> needs the full definition.  Including point.h in the .h seems to go against
> coding guidelines.


Non-IPC code has the guideline of trying to forward declare as much as
possible.  But IPC message files require that any header needed is listed,
otherwise you end up with the compile error that you saw.  Note that IPC
message files are only ever included from cc files, so that's fine.


>
>
> http://codereview.chromium.org/6840044/
>

[grt (UTC plus 2), 2011-04-15 19:55:13]

On 2011/04/15 19:49:43, John Abd-El-Malek wrote:
> Non-IPC code has the guideline of trying to forward declare as much as
> possible.  But IPC message files require that any header needed is listed,
> otherwise you end up with the compile error that you saw.  Note that IPC
> message files are only ever included from cc files, so that's fine.

Aaah, gotcha.  It was missing from drag_messages.h.  I've moved it there. 
Thanks.

[jam, 2011-04-15 20:00:49]

lgtm, thanks for tracking this down!

[commit-bot: I haz the power, 2011-04-15 21:07:05]

Change committed as 81803