Add chromecast_build gyp variable and add rules when the variable is set to 1.

Add chromecast_build gyp variable and add rules when the variable is set to 1.

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

[damienv1, 2014-04-03 21:45:32]

https://codereview.chromium.org/223143004/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/223143004/diff/1/build/common.gypi#newcode2506
build/common.gypi:2506: 'ENABLE_MPEG2TS_STREAM_PARSER',
There is now a gyp parameter the mpeg2 TS parser: enable_mpeg2ts_stream_parser.
See https://codereview.chromium.org/216423008/

[jam, 2014-04-03 21:51:39]

Adding Darin for his thoughts. Generally adding more build configs makes it easy
to add too many ifdefs all over the code for each config, but I don't see an
alternative here, especially with the other cls that are using this like

https://codereview.chromium.org/223593002/
https://codereview.chromium.org/223583003/
https://codereview.chromium.org/223253006/
https://codereview.chromium.org/223143006/
https://codereview.chromium.org/222753004/

[darin (slow to review), 2014-04-14 04:50:34]

https://codereview.chromium.org/223143004/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/223143004/diff/1/build/common.gypi#newcode2504
build/common.gypi:2504: 'CHROMECAST_BUILD',
I think it would be nice to see if we could solve for Chromecast without
introducing an #ifdef. I realize that we have #ifdefs for other ports, but
adding an #ifdef for every port just doesn't scale. Each has its own recipe of
things to enable / disable. We have likewise resisted an OPERA_BUILD #ifdef.

Looking over the various patches I have seen it seems like it should be possible
to extract code into separate files, eliminating #ifdefs in favor of selecting
the right .cc files at build time.

[lcwu1, 2014-04-15 06:04:11]

On 2014/04/14 04:50:34, darin wrote:
> https://codereview.chromium.org/223143004/diff/1/build/common.gypi
> File build/common.gypi (right):
> 
> https://codereview.chromium.org/223143004/diff/1/build/common.gypi#newcode2504
> build/common.gypi:2504: 'CHROMECAST_BUILD',
> I think it would be nice to see if we could solve for Chromecast without
> introducing an #ifdef. I realize that we have #ifdefs for other ports, but
> adding an #ifdef for every port just doesn't scale. Each has its own recipe of
> things to enable / disable. We have likewise resisted an OPERA_BUILD #ifdef.
> 
> Looking over the various patches I have seen it seems like it should be
possible
> to extract code into separate files, eliminating #ifdefs in favor of selecting
> the right .cc files at build time.

Darin,

Yes, we understand that introducing a new #ifdef is discouraged, and have been
trying to avoid it as much as possible. However, currently there are still uses
of the #ifdefs that appears inevitable (in the near term). For example, we have
a CL (which would be sent out for review later) that sets a macro to a different
value (inside ffmeg) due to our use of gcc-4.5.3 for the ARM platform (as shown
below):

--- a/chromium/config/Chrome/linux/arm-neon/config.h
+++ b/chromium/config/Chrome/linux/arm-neon/config.h
@@ -244,7 +244,13 @@
 #define HAVE_POD2MAN 1
 #define HAVE_POLL_H 1
 #define HAVE_POSIX_MEMALIGN 1
+// TODO(lcwu): gcc-4.5 does not support "#pragma diagnostics" inside
+// a function. We should remove this workaround after the compiler is upgraded.
+#if defined(CHROMECAST_BUILD)
+#define HAVE_PRAGMA_DEPRECATED 0
+#else
 #define HAVE_PRAGMA_DEPRECATED 1
+#endif
 #define HAVE_PTHREAD_CANCEL 1


This workaround is needed with our current compiler, but can be removed once we
work with our SoC provider to upgrade our compiler to gcc-4.7 (or 4.8).

There are also uses of #ifdefs that address Chromium components that don't yet
have a good injection mechanism for different ports. Some of the cases can
probably be addressed using a build-time file selection (as you mentioned) or
re-factored to provide an injection mechanism relatively easily (and in
relatively short time), and we will surely work with the component owners to do
just that. But there are also cases where it could take a much longer time to
device a good injection mechanism (e.g. we have been working with the media team
on how to provide/inject an external audio/video renderer/provider) and do need
a way to hook up our specific implementation in the near term.

In order for us to be able to quickly set up upstream Chromecast trybots and
move the team to develop directly on upstream code base, do you think if it
makes sense to allow the use of the #ifdefs in the short term? We do intend for
the #ifdefs to be temporary and will only use it as a last resort (after
discussing with the project/component owners on other possible solutions). Also
we will file a CrBug to track the task of removing these #ifdefs eventually, and
add a TODO in the places where #ifdef is used (similar to the example CL shown
above). Does that sound reasonable to you?

Thanks,

Le-chun

[darin (slow to review), 2014-04-15 06:37:30]

On Mon, Apr 14, 2014 at 11:04 PM, <lcwu@chromium.org> wrote:

> On 2014/04/14 04:50:34, darin wrote:
>
>> https://codereview.chromium.org/223143004/diff/1/build/common.gypi
>> File build/common.gypi (right):
>>
>
>  https://codereview.chromium.org/223143004/diff/1/build/
>> common.gypi#newcode2504
>> build/common.gypi:2504: 'CHROMECAST_BUILD',
>> I think it would be nice to see if we could solve for Chromecast without
>> introducing an #ifdef. I realize that we have #ifdefs for other ports, but
>> adding an #ifdef for every port just doesn't scale. Each has its own
>> recipe of
>> things to enable / disable. We have likewise resisted an OPERA_BUILD
>> #ifdef.
>>
>
>  Looking over the various patches I have seen it seems like it should be
>>
> possible
>
>> to extract code into separate files, eliminating #ifdefs in favor of
>> selecting
>> the right .cc files at build time.
>>
>
> Darin,
>
> Yes, we understand that introducing a new #ifdef is discouraged, and have
> been
> trying to avoid it as much as possible. However, currently there are still
> uses
> of the #ifdefs that appears inevitable (in the near term). For example, we
> have
> a CL (which would be sent out for review later) that sets a macro to a
> different
> value (inside ffmeg) due to our use of gcc-4.5.3 for the ARM platform (as
> shown
> below):
>
> --- a/chromium/config/Chrome/linux/arm-neon/config.h
> +++ b/chromium/config/Chrome/linux/arm-neon/config.h
> @@ -244,7 +244,13 @@
>  #define HAVE_POD2MAN 1
>  #define HAVE_POLL_H 1
>  #define HAVE_POSIX_MEMALIGN 1
> +// TODO(lcwu): gcc-4.5 does not support "#pragma diagnostics" inside
> +// a function. We should remove this workaround after the compiler is
> upgraded.
> +#if defined(CHROMECAST_BUILD)
> +#define HAVE_PRAGMA_DEPRECATED 0
> +#else
>  #define HAVE_PRAGMA_DEPRECATED 1
> +#endif
>  #define HAVE_PTHREAD_CANCEL 1
>
>
> This workaround is needed with our current compiler, but can be removed
> once we
> work with our SoC provider to upgrade our compiler to gcc-4.7 (or 4.8).
>

It sounds like this should be described using a compiler specific #ifdef,
no? Can you use some combination of these macros:

__GNUC__
__GNUC_MINOR__
__GNUC_PATCHLEVEL__




>
> There are also uses of #ifdefs that address Chromium components that don't
> yet
> have a good injection mechanism for different ports. Some of the cases can
> probably be addressed using a build-time file selection (as you mentioned)
> or
> re-factored to provide an injection mechanism relatively easily (and in
> relatively short time), and we will surely work with the component owners
> to do
> just that. But there are also cases where it could take a much longer time
> to
> device a good injection mechanism (e.g. we have been working with the
> media team
> on how to provide/inject an external audio/video renderer/provider) and do
> need
> a way to hook up our specific implementation in the near term.
>
> In order for us to be able to quickly set up upstream Chromecast trybots
> and
> move the team to develop directly on upstream code base, do you think if it
> makes sense to allow the use of the #ifdefs in the short term? We do
> intend for
> the #ifdefs to be temporary and will only use it as a last resort (after
> discussing with the project/component owners on other possible solutions).
> Also
> we will file a CrBug to track the task of removing these #ifdefs
> eventually, and
> add a TODO in the places where #ifdef is used (similar to the example CL
> shown
> above). Does that sound reasonable to you?
>

It is a slippery slope, and in my experience it can take a long time to
clean up. I know you have the best of intentions...

Can we start by examining cases where it is needed / where the cost of
developing an alternative solution is too high? The CLs I've seen thus far
do not seem to warrant the catch-all #ifdef.

Thanks,
-Darin



>
> Thanks,
>
> Le-chun
>
> https://codereview.chromium.org/223143004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[lcwu1, 2014-04-15 07:17:01]

On 2014/04/15 06:37:30, darin wrote:
> On Mon, Apr 14, 2014 at 11:04 PM, <mailto:lcwu@chromium.org> wrote:
> 
> > On 2014/04/14 04:50:34, darin wrote:
> >
> >> https://codereview.chromium.org/223143004/diff/1/build/common.gypi
> >> File build/common.gypi (right):
> >>
> >
> >  https://codereview.chromium.org/223143004/diff/1/build/
> >> common.gypi#newcode2504
> >> build/common.gypi:2504: 'CHROMECAST_BUILD',
> >> I think it would be nice to see if we could solve for Chromecast without
> >> introducing an #ifdef. I realize that we have #ifdefs for other ports, but
> >> adding an #ifdef for every port just doesn't scale. Each has its own
> >> recipe of
> >> things to enable / disable. We have likewise resisted an OPERA_BUILD
> >> #ifdef.
> >>
> >
> >  Looking over the various patches I have seen it seems like it should be
> >>
> > possible
> >
> >> to extract code into separate files, eliminating #ifdefs in favor of
> >> selecting
> >> the right .cc files at build time.
> >>
> >
> > Darin,
> >
> > Yes, we understand that introducing a new #ifdef is discouraged, and have
> > been
> > trying to avoid it as much as possible. However, currently there are still
> > uses
> > of the #ifdefs that appears inevitable (in the near term). For example, we
> > have
> > a CL (which would be sent out for review later) that sets a macro to a
> > different
> > value (inside ffmeg) due to our use of gcc-4.5.3 for the ARM platform (as
> > shown
> > below):
> >
> > --- a/chromium/config/Chrome/linux/arm-neon/config.h
> > +++ b/chromium/config/Chrome/linux/arm-neon/config.h
> > @@ -244,7 +244,13 @@
> >  #define HAVE_POD2MAN 1
> >  #define HAVE_POLL_H 1
> >  #define HAVE_POSIX_MEMALIGN 1
> > +// TODO(lcwu): gcc-4.5 does not support "#pragma diagnostics" inside
> > +// a function. We should remove this workaround after the compiler is
> > upgraded.
> > +#if defined(CHROMECAST_BUILD)
> > +#define HAVE_PRAGMA_DEPRECATED 0
> > +#else
> >  #define HAVE_PRAGMA_DEPRECATED 1
> > +#endif
> >  #define HAVE_PTHREAD_CANCEL 1
> >
> >
> > This workaround is needed with our current compiler, but can be removed
> > once we
> > work with our SoC provider to upgrade our compiler to gcc-4.7 (or 4.8).
> >
> 
> It sounds like this should be described using a compiler specific #ifdef,
> no? Can you use some combination of these macros:
> 
> __GNUC__
> __GNUC_MINOR__
> __GNUC_PATCHLEVEL__
> 

Yes, agreed. I think I used a bad example in my arguments. :-)

> 
> 
> 
> >
> > There are also uses of #ifdefs that address Chromium components that don't
> > yet
> > have a good injection mechanism for different ports. Some of the cases can
> > probably be addressed using a build-time file selection (as you mentioned)
> > or
> > re-factored to provide an injection mechanism relatively easily (and in
> > relatively short time), and we will surely work with the component owners
> > to do
> > just that. But there are also cases where it could take a much longer time
> > to
> > device a good injection mechanism (e.g. we have been working with the
> > media team
> > on how to provide/inject an external audio/video renderer/provider) and do
> > need
> > a way to hook up our specific implementation in the near term.
> >
> > In order for us to be able to quickly set up upstream Chromecast trybots
> > and
> > move the team to develop directly on upstream code base, do you think if it
> > makes sense to allow the use of the #ifdefs in the short term? We do
> > intend for
> > the #ifdefs to be temporary and will only use it as a last resort (after
> > discussing with the project/component owners on other possible solutions).
> > Also
> > we will file a CrBug to track the task of removing these #ifdefs
> > eventually, and
> > add a TODO in the places where #ifdef is used (similar to the example CL
> > shown
> > above). Does that sound reasonable to you?
> >
> 
> It is a slippery slope, and in my experience it can take a long time to
> clean up. I know you have the best of intentions...
> 
> Can we start by examining cases where it is needed / where the cost of
> developing an alternative solution is too high? The CLs I've seen thus far
> do not seem to warrant the catch-all #ifdef.

Sure. We will re-examine the CLs that use #ifdefs and see if we can develop an
alternative solution in reasonable time and effort for each case. In terms of
this particular CL, I will upload a new patch set without defining
CHROMECAT_BUILD when chromecast=1.

Thanks,

Le-chun

> 
> Thanks,
> -Darin
> 
> 
> 
> >
> > Thanks,
> >
> > Le-chun
> >
> > https://codereview.chromium.org/223143004/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[jam, 2014-04-15 16:55:11]

On 2014/04/15 06:04:11, lcwu1 wrote:
> On 2014/04/14 04:50:34, darin wrote:
> > https://codereview.chromium.org/223143004/diff/1/build/common.gypi
> > File build/common.gypi (right):
> > 
> >
https://codereview.chromium.org/223143004/diff/1/build/common.gypi#newcode2504
> > build/common.gypi:2504: 'CHROMECAST_BUILD',
> > I think it would be nice to see if we could solve for Chromecast without
> > introducing an #ifdef. I realize that we have #ifdefs for other ports, but
> > adding an #ifdef for every port just doesn't scale. Each has its own recipe
of
> > things to enable / disable. We have likewise resisted an OPERA_BUILD #ifdef.
> > 
> > Looking over the various patches I have seen it seems like it should be
> possible
> > to extract code into separate files, eliminating #ifdefs in favor of
selecting
> > the right .cc files at build time.
> 
> Darin,
> 
> Yes, we understand that introducing a new #ifdef is discouraged, and have been
> trying to avoid it as much as possible. However, currently there are still
uses
> of the #ifdefs that appears inevitable (in the near term). For example, we
have
> a CL (which would be sent out for review later) that sets a macro to a
different
> value (inside ffmeg) due to our use of gcc-4.5.3 for the ARM platform (as
shown
> below):
> 
> --- a/chromium/config/Chrome/linux/arm-neon/config.h
> +++ b/chromium/config/Chrome/linux/arm-neon/config.h
> @@ -244,7 +244,13 @@
>  #define HAVE_POD2MAN 1
>  #define HAVE_POLL_H 1
>  #define HAVE_POSIX_MEMALIGN 1
> +// TODO(lcwu): gcc-4.5 does not support "#pragma diagnostics" inside
> +// a function. We should remove this workaround after the compiler is
upgraded.
> +#if defined(CHROMECAST_BUILD)
> +#define HAVE_PRAGMA_DEPRECATED 0
> +#else
>  #define HAVE_PRAGMA_DEPRECATED 1
> +#endif
>  #define HAVE_PTHREAD_CANCEL 1
> 
> 
> This workaround is needed with our current compiler, but can be removed once
we
> work with our SoC provider to upgrade our compiler to gcc-4.7 (or 4.8).
> 
> There are also uses of #ifdefs that address Chromium components that don't yet
> have a good injection mechanism for different ports. Some of the cases can
> probably be addressed using a build-time file selection (as you mentioned) or
> re-factored to provide an injection mechanism relatively easily (and in
> relatively short time), and we will surely work with the component owners to
do
> just that. But there are also cases where it could take a much longer time to
> device a good injection mechanism (e.g. we have been working with the media
team
> on how to provide/inject an external audio/video renderer/provider) and do
need
> a way to hook up our specific implementation in the near term.
> 
> In order for us to be able to quickly set up upstream Chromecast trybots and
> move the team to develop directly on upstream code base, do you think if it
> makes sense to allow the use of the #ifdefs in the short term? We do intend
for
> the #ifdefs to be temporary and will only use it as a last resort (after
> discussing with the project/component owners on other possible solutions).
Also
> we will file a CrBug to track the task of removing these #ifdefs eventually,
and
> add a TODO in the places where #ifdef is used (similar to the example CL shown
> above). Does that sound reasonable to you?

In response to this last paragraph, chrome code that is forked has to land
without temporary hacks. We have years-old TODOs in the code in cases where we
didn't do this because the product teams always have higher priority tasks to
do.

[lcwu1, 2014-04-22 01:15:16]

In patch set #2 we added and modified gyp rules to set relevant gyp variables
when chromecast==1, so that developers/users can simply do "build/gyp_chromium
-Dchromecast=1" followed by "ninja -C out/Debug cast_shell" to build cast shell
(on x86 linux) without special custom build scripts. We also removed the
definition of CHROMECAST_BUILD macro. Please take another look. Thanks.

[darin (slow to review), 2014-04-22 03:15:54]

LGTM

[jam, 2014-04-22 16:21:14]

looks much better!

https://codereview.chromium.org/223143004/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/223143004/diff/20001/build/common.gypi#newcod...
build/common.gypi:1647: 'disable_nacl%': 1,
I'm curious why this is needed? You only bring in code from content layer and
below, and content doesn't know about NaCl.

https://codereview.chromium.org/223143004/diff/20001/build/common.gypi#newcod...
build/common.gypi:2537: ['chromecast==1', {
i'm curious why this one and the previous section couldn't be combined with the
first chromecast==1 section?

[alexst (slow to review), 2014-04-22 17:28:12]

https://codereview.chromium.org/223143004/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/223143004/diff/20001/build/common.gypi#newcod...
build/common.gypi:104: # ToT Linux should be aura (except for chromecast
builds).
Does that mean all the rendering messages that now go through aura will be
duplicated for chromecast?

[lcwu1, 2014-04-22 18:24:41]

Please take a look at patch set #3. Thanks.

https://codereview.chromium.org/223143004/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/223143004/diff/20001/build/common.gypi#newcod...
build/common.gypi:104: # ToT Linux should be aura (except for chromecast
builds).
On 2014/04/22 17:28:12, alexst wrote:
> Does that mean all the rendering messages that now go through aura will be
> duplicated for chromecast?

Alex, I would think that when aura is disabled, the messages that go through
aura will either go through another channel or be irrelevant. Could you
elaborate a bit on the duplicating messages concerns? Thanks.

https://codereview.chromium.org/223143004/diff/20001/build/common.gypi#newcod...
build/common.gypi:1647: 'disable_nacl%': 1,
On 2014/04/22 16:21:15, jam wrote:
> I'm curious why this is needed? You only bring in code from content layer and
> below, and content doesn't know about NaCl.

Yes, this is indeed not needed. Removed.

https://codereview.chromium.org/223143004/diff/20001/build/common.gypi#newcod...
build/common.gypi:2537: ['chromecast==1', {
On 2014/04/22 16:21:15, jam wrote:
> i'm curious why this one and the previous section couldn't be combined with
the
> first chromecast==1 section?

I was trying to follow the convention here in this file. The convention looks to
me that the settings of C macros are done in the outer-most scope if possible.
And the definition/initialization of a gyp variable should only be introduced in
a scope when its immediate outer scope uses it.

[jam, 2014-04-22 18:31:57]

https://codereview.chromium.org/223143004/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/223143004/diff/20001/build/common.gypi#newcod...
build/common.gypi:2537: ['chromecast==1', {
On 2014/04/22 18:24:42, lcwu1 wrote:
> On 2014/04/22 16:21:15, jam wrote:
> > i'm curious why this one and the previous section couldn't be combined with
> the
> > first chromecast==1 section?
> 
> I was trying to follow the convention here in this file. The convention looks
to
> me that the settings of C macros are done in the outer-most scope if possible.
> And the definition/initialization of a gyp variable should only be introduced
in
> a scope when its immediate outer scope uses it.
> 

I'm not sure I understand what you mean. you're not defining a C macro anymore.
I'm not sure what is the difference between the new blocks on line 566, 1646,
2083, and 2537

[alexst (slow to review), 2014-04-22 18:49:12]

> Alex, I would think that when aura is disabled, the messages that go through
> aura will either go through another channel or be irrelevant. Could you
> elaborate a bit on the duplicating messages concerns? Thanks.
> 

Le-Chun, I was mostly talking about things like OnSwapCompositorFrame that
manage compositor frame and ACK flow.
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

This code changes when compositor behaviour is modified. From personal
experience working on <webview>, which had to handle some of these messages in a
different place, it was prone to instability. People modifying rendering
behavior may not know to update your copy of this code outside of aura or fully
understand your assumptions. I think your number of breakages and flake over
time would go down substantially if you shared this code instead of maintaining
your own version. It would reduce the burden on both you and the compositor
team.

[rjkroege, 2014-04-22 21:15:49]

https://codereview.chromium.org/223143004/diff/40001/build/linux/system.gyp
File build/linux/system.gyp (right):

https://codereview.chromium.org/223143004/diff/40001/build/linux/system.gyp#n...
build/linux/system.gyp:1053: 'target_name': 'directfb',
Does this imply that you will eventually need a RenderWidgetHost*Directfb and
equivalent plumbing?

Shouldn't an addition of a target with an additional library and build time
dependencies require either updating install-build-deps.sh or adding directfb to
third_party?

[lcwu1, 2014-04-22 22:12:39]

On 2014/04/22 18:31:57, jam wrote:
> https://codereview.chromium.org/223143004/diff/20001/build/common.gypi
> File build/common.gypi (right):
> 
>
https://codereview.chromium.org/223143004/diff/20001/build/common.gypi#newcod...
> build/common.gypi:2537: ['chromecast==1', {
> On 2014/04/22 18:24:42, lcwu1 wrote:
> > On 2014/04/22 16:21:15, jam wrote:
> > > i'm curious why this one and the previous section couldn't be combined
with
> > the
> > > first chromecast==1 section?
> > 
> > I was trying to follow the convention here in this file. The convention
looks
> to
> > me that the settings of C macros are done in the outer-most scope if
possible.
> > And the definition/initialization of a gyp variable should only be
introduced
> in
> > a scope when its immediate outer scope uses it.
> > 
> 
> I'm not sure I understand what you mean. you're not defining a C macro
anymore.
> I'm not sure what is the difference between the new blocks on line 566, 1646,
> 2083, and 2537

I combined the rules in line 566, 1646, and 2083 into one block (in the
outer-most 'variables' scope). The block in line 2537 is defining a couple of
macros (LOG_DISABLED=0 and __SOFTFP) that does not belong to the 'variables'
section.

Please take another look at patch set #4.

[lcwu1, 2014-04-22 22:26:27]

Robert,

On 2014/04/22 21:15:49, rjkroege wrote:
> https://codereview.chromium.org/223143004/diff/40001/build/linux/system.gyp
> File build/linux/system.gyp (right):
> 
>
https://codereview.chromium.org/223143004/diff/40001/build/linux/system.gyp#n...
> build/linux/system.gyp:1053: 'target_name': 'directfb',
> Does this imply that you will eventually need a RenderWidgetHost*Directfb and
> equivalent plumbing?

Hooking up with directfb is done through our gfx_plane and surface interface. So
we don't need additional plumbing from the content's (RenderWidgeHost*) point of
view.

> 
> Shouldn't an addition of a target with an additional library and build time
> dependencies require either updating install-build-deps.sh or adding directfb
to
> third_party?

Yes, that's a good point. I will modify install-build-deps.sh to add directfb as
a dependence. Thanks.

[lcwu1, 2014-04-22 23:19:56]

On 2014/04/22 18:49:12, alexst wrote:
> > Alex, I would think that when aura is disabled, the messages that go through
> > aura will either go through another channel or be irrelevant. Could you
> > elaborate a bit on the duplicating messages concerns? Thanks.
> > 
> 
> Le-Chun, I was mostly talking about things like OnSwapCompositorFrame that
> manage compositor frame and ACK flow.
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
> 
> This code changes when compositor behaviour is modified. From personal
> experience working on <webview>, which had to handle some of these messages in
a
> different place, it was prone to instability. People modifying rendering
> behavior may not know to update your copy of this code outside of aura or
fully
> understand your assumptions. I think your number of breakages and flake over
> time would go down substantially if you shared this code instead of
maintaining
> your own version. It would reduce the burden on both you and the compositor
> team.

I think (in theory) as long as the compositor doesn't make any implicit
assumption of what RenderWidgtHostView APIs should be implemented, and the ports
implement those APIs faithfully, the ports shouldn't be broken too often. (And
in fact, we haven't encountered any problem in this part of the code since two
years ago.) But I agree this is in theory. :-) Having a trybot set up is
definitely one thing we like to do as soon as possible. Also moving to aura is
surely something on our road map.

[lcwu1, 2014-04-22 23:21:49]

Patch set #5 include the changes to add directfb in install-build-deps.sh.
Please take a look. Thanks.

[jam, 2014-04-24 15:38:16]

https://codereview.chromium.org/223143004/diff/80001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/223143004/diff/80001/build/common.gypi#newcod...
build/common.gypi:3545: # Some components in Chormium (e.g. v8) define their own
nit: spelling

https://codereview.chromium.org/223143004/diff/80001/build/common.gypi#newcod...
build/common.gypi:3546: # cflags that are not desirable. Remove them explicitly
hmm, it seems that the proper solution here is to update v8's gyp flags to not
add this stuff for chromecast

https://codereview.chromium.org/223143004/diff/80001/build/filename_rules.gypi
File build/filename_rules.gypi (right):

https://codereview.chromium.org/223143004/diff/80001/build/filename_rules.gyp...
build/filename_rules.gypi:58: ['exclude',
'_(cast|chromecast)(_unittest)?\\.(h|cc)$'],
since after the last meeting we're going to try to not add chromecast files as a
port, but instead use existing or new feature-specific flags, it seems this
isn't necessary anymore?

[rjkroege, 2014-04-24 18:45:07]

On 2014/04/22 22:26:27, lcwu1 wrote:
> Robert,
> 
> On 2014/04/22 21:15:49, rjkroege wrote:
> > https://codereview.chromium.org/223143004/diff/40001/build/linux/system.gyp
> > File build/linux/system.gyp (right):
> > 
> >
>
https://codereview.chromium.org/223143004/diff/40001/build/linux/system.gyp#n...
> > build/linux/system.gyp:1053: 'target_name': 'directfb',
> > Does this imply that you will eventually need a RenderWidgetHost*Directfb
and
> > equivalent plumbing?
> 
> Hooking up with directfb is done through our gfx_plane and surface interface.
So
> we don't need additional plumbing from the content's (RenderWidgeHost*) point
of
> view.

But you are adding a new RenderWidgetHostView in another CL yes?

> 
> > 
> > Shouldn't an addition of a target with an additional library and build time
> > dependencies require either updating install-build-deps.sh or adding
directfb
> to
> > third_party?
> 
> Yes, that's a good point. I will modify install-build-deps.sh to add directfb
as
> a dependence. Thanks.

[rjkroege, 2014-04-24 18:49:35]

https://codereview.chromium.org/223143004/diff/80001/build/install-build-deps.sh
File build/install-build-deps.sh (right):

https://codereview.chromium.org/223143004/diff/80001/build/install-build-deps...
build/install-build-deps.sh:168: chromecast_list="libdirectfb-dev"
Will making this not installed by default make setting up builders more tedious?
And are there any license issues with this package?

[lcwu1, 2014-04-26 00:14:20]

On 2014/04/24 18:45:07, rjkroege wrote:
> On 2014/04/22 22:26:27, lcwu1 wrote:
> > Robert,
> > 
> > On 2014/04/22 21:15:49, rjkroege wrote:
> > >
https://codereview.chromium.org/223143004/diff/40001/build/linux/system.gyp
> > > File build/linux/system.gyp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/223143004/diff/40001/build/linux/system.gyp#n...
> > > build/linux/system.gyp:1053: 'target_name': 'directfb',
> > > Does this imply that you will eventually need a RenderWidgetHost*Directfb
> and
> > > equivalent plumbing?
> > 
> > Hooking up with directfb is done through our gfx_plane and surface
interface.
> So
> > we don't need additional plumbing from the content's (RenderWidgeHost*)
point
> of
> > view.
> 
> But you are adding a new RenderWidgetHostView in another CL yes?

Yes, we were. But we are now looking into enabling aura,
RenderWidgetHostViewCast should no longer be needed.

> 
> > 
> > > 
> > > Shouldn't an addition of a target with an additional library and build
time
> > > dependencies require either updating install-build-deps.sh or adding
> directfb
> > to
> > > third_party?
> > 
> > Yes, that's a good point. I will modify install-build-deps.sh to add
directfb
> as
> > a dependence. Thanks.

[rjkroege, 2014-04-29 19:48:53]

On 2014/04/26 00:14:20, lcwu1 wrote:
> On 2014/04/24 18:45:07, rjkroege wrote:
> > On 2014/04/22 22:26:27, lcwu1 wrote:
> > > Robert,
> > > 
> > > On 2014/04/22 21:15:49, rjkroege wrote:
> > > >
> https://codereview.chromium.org/223143004/diff/40001/build/linux/system.gyp
> > > > File build/linux/system.gyp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/223143004/diff/40001/build/linux/system.gyp#n...
> > > > build/linux/system.gyp:1053: 'target_name': 'directfb',
> > > > Does this imply that you will eventually need a
RenderWidgetHost*Directfb
> > and
> > > > equivalent plumbing?
> > > 
> > > Hooking up with directfb is done through our gfx_plane and surface
> interface.
> > So
> > > we don't need additional plumbing from the content's (RenderWidgeHost*)
> point
> > of
> > > view.
> > 
> > But you are adding a new RenderWidgetHostView in another CL yes?
> 
> Yes, we were. But we are now looking into enabling aura,
> RenderWidgetHostViewCast should no longer be needed.

Sounds excellent to me.


> 
> > 
> > > 
> > > > 
> > > > Shouldn't an addition of a target with an additional library and build
> time
> > > > dependencies require either updating install-build-deps.sh or adding
> > directfb
> > > to
> > > > third_party?
> > > 
> > > Yes, that's a good point. I will modify install-build-deps.sh to add
> directfb
> > as
> > > a dependence. Thanks.

[lcwu1, 2014-06-24 22:34:39]

Patch set #6 enables aura/ozone/embedded when chromecast==1. Also added a new
build dependency (libgles2-mesa-dev) for chromecast build as it is required for
using ozone's egl x11 backend/platform on x86.

Please take another look. Thanks.

[rjkroege, 2014-06-25 16:44:16]

https://codereview.chromium.org/223143004/diff/100001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/223143004/diff/100001/build/common.gypi#newco...
build/common.gypi:1762: 'ozone_platform_ozonex%': 1,
where is this platform?

https://codereview.chromium.org/223143004/diff/100001/build/common.gypi#newco...
build/common.gypi:1900: ['os_posix==1 and chromeos==0 and OS!="android" and
OS!="ios" and chromecast==0', {
cups is for printing yes? I think you get this turned off for free with
embedded=1. If not, embedded should be updated to turn off  cups.

https://codereview.chromium.org/223143004/diff/100001/build/common.gypi#newco...
build/common.gypi:2691: 'LOG_DISABLED=0',
why?

https://codereview.chromium.org/223143004/diff/100001/build/common.gypi#newco...
build/common.gypi:2696: # TODO(lcwu): Work around an error when building
Chromium
it would be a good idea to have a bug that goes with this TODO

https://codereview.chromium.org/223143004/diff/100001/build/common.gypi#newco...
build/common.gypi:3745: 'cflags': [
this block would seem also be temporary? i.e. it could have a TODO and have a
tracking bug?

https://codereview.chromium.org/223143004/diff/100001/build/install-build-dep...
File build/install-build-deps.sh (right):

https://codereview.chromium.org/223143004/diff/100001/build/install-build-dep...
build/install-build-deps.sh:173: chromecast_list="libgles2-mesa-dev"
i think dnicoara landed a change already to arrange for this?

https://codereview.chromium.org/223143004/diff/100001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/223143004/diff/100001/media/media.gyp#newcode25
media/media.gyp:25: ['(OS=="linux" or OS=="freebsd" or OS=="solaris") and
(embedded!=1 or (chromecast==1 and target_arch!="arm"))', {
this seems a trifle odd to me. Why?

[spang, 2014-06-25 16:53:28]

https://codereview.chromium.org/223143004/diff/100001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/223143004/diff/100001/build/common.gypi#newco...
build/common.gypi:1762: 'ozone_platform_ozonex%': 1,
On 2014/06/25 16:44:16, rjkroege wrote:
> where is this platform?

This is a gyp hack to allow X dependencies in ozone builds. See
build/linux/system.gyp for why this is here.

[spang, 2014-06-25 19:55:46]

https://codereview.chromium.org/223143004/diff/100001/build/install-build-dep...
File build/install-build-deps.sh (right):

https://codereview.chromium.org/223143004/diff/100001/build/install-build-dep...
build/install-build-deps.sh:173: chromecast_list="libgles2-mesa-dev"
On 2014/06/25 16:44:16, rjkroege wrote:
> i think dnicoara landed a change already to arrange for this? 

No that was gbm.

I think this can go in the main list though. It is needed for Ozone on X11 too.

Also, I guess it needs the same logic as gbm to match mesa versioning.

[spang, 2014-06-25 20:01:07]

On 2014/06/25 19:55:46, spang wrote:
>
https://codereview.chromium.org/223143004/diff/100001/build/install-build-dep...
> File build/install-build-deps.sh (right):
> 
>
https://codereview.chromium.org/223143004/diff/100001/build/install-build-dep...
> build/install-build-deps.sh:173: chromecast_list="libgles2-mesa-dev"
> On 2014/06/25 16:44:16, rjkroege wrote:
> > i think dnicoara landed a change already to arrange for this? 
> 
> No that was gbm.
> 
> I think this can go in the main list though. It is needed for Ozone on X11
too.
> 
> Also, I guess it needs the same logic as gbm to match mesa versioning.

This missing dep is my fault and I can fix it.

Split out into https://codereview.chromium.org/332823004/

[lcwu1, 2014-06-25 22:41:02]

https://codereview.chromium.org/223143004/diff/100001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/223143004/diff/100001/build/common.gypi#newco...
build/common.gypi:1900: ['os_posix==1 and chromeos==0 and OS!="android" and
OS!="ios" and chromecast==0', {
On 2014/06/25 16:44:16, rjkroege wrote:
> cups is for printing yes? I think you get this turned off for free with
> embedded=1. If not, embedded should be updated to turn off  cups. 

I don't think it is turned off when embedded==1. I can change chromecast==0 to
embedded==0 here.

https://codereview.chromium.org/223143004/diff/100001/build/common.gypi#newco...
build/common.gypi:2691: 'LOG_DISABLED=0',
On 2014/06/25 16:44:16, rjkroege wrote:
> why?

Because we don't want to disable all the logs from WebKit in release/non-debug
mode. (Note that this macro alone won't enable all the logs. It just allows us
to enable logging for individual WebKit log channel when needed/desired.)

https://codereview.chromium.org/223143004/diff/100001/build/common.gypi#newco...
build/common.gypi:2696: # TODO(lcwu): Work around an error when building
Chromium
On 2014/06/25 16:44:16, rjkroege wrote:
> it would be a good idea to have a bug that goes with this TODO

Will create one.

https://codereview.chromium.org/223143004/diff/100001/build/common.gypi#newco...
build/common.gypi:3745: 'cflags': [
On 2014/06/25 16:44:16, rjkroege wrote:
> this block would seem also be temporary? i.e. it could have a TODO and have a
> tracking bug?

This block is not temporary. I only added this big chunk of comments to explain
why we don't have an "-march" flag here (which is unnecessary anyway given that
we have specified a more detailed cpu implementation, cortex-a9).

https://codereview.chromium.org/223143004/diff/100001/build/install-build-dep...
File build/install-build-deps.sh (right):

https://codereview.chromium.org/223143004/diff/100001/build/install-build-dep...
build/install-build-deps.sh:173: chromecast_list="libgles2-mesa-dev"
On 2014/06/25 19:55:46, spang wrote:
> On 2014/06/25 16:44:16, rjkroege wrote:
> > i think dnicoara landed a change already to arrange for this? 
> 
> No that was gbm.
> 
> I think this can go in the main list though. It is needed for Ozone on X11
too.
> 
> Also, I guess it needs the same logic as gbm to match mesa versioning.

Michael has a CL to add libgles2-mesa-dev, I will remove the changes in this
file from the CL.

https://codereview.chromium.org/223143004/diff/100001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/223143004/diff/100001/media/media.gyp#newcode25
media/media.gyp:25: ['(OS=="linux" or OS=="freebsd" or OS=="solaris") and
(embedded!=1 or (chromecast==1 and target_arch!="arm"))', {
On 2014/06/25 16:44:16, rjkroege wrote:
> this seems a trifle odd to me. Why?

Unlike our arm platform where we have our own audio/video codec, when running
cast shell on x86 linux workstation, we do need an audio support like ALSA.

[spang, 2014-06-26 23:06:15]

lgtm

[lcwu1, 2014-07-03 02:04:40]

The CQ bit was checked by lcwu@chromium.org

[commit-bot: I haz the power, 2014-07-03 02:06:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/lcwu@chromium.org/223143004/140001

[commit-bot: I haz the power, 2014-07-03 03:16:42]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/24435)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/28839)

[commit-bot: I haz the power, 2014-07-03 03:18:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-03 03:18:28]

Try jobs failed on following builders:
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/24452)

[lcwu1, 2014-07-03 03:57:09]

@spang, besides moving the chromecast rule that enables embedded (and ozone) out
one scope level to fix the trybot failure, I have also moved the
use_system_fontconfig rule to the outer-most scope because it doesn't need to
happen in the inner scope and get propagated out several times.

Please take another look at patch set #9. Thanks.

[spang, 2014-07-03 14:04:24]

lgtm

[lcwu1, 2014-07-03 14:59:14]

The CQ bit was checked by lcwu@chromium.org

[commit-bot: I haz the power, 2014-07-03 15:00:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/lcwu@chromium.org/223143004/160001

[commit-bot: I haz the power, 2014-07-03 16:49:53]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)

[commit-bot: I haz the power, 2014-07-03 17:20:20]

Change committed as 281285