[Chromecast] Change chromecast_branding to public

[Chromecast] Change chromecast_branding to public

Change chromecast_branding from Chrome/Chromium to
public/internal/google in gyp file. "public" is the same as "Chromium"
and the other two are considered as "Chrome".

BUG=

Committed: https://crrev.com/0f46bd94566380a99ecda0753f659b7e114e3d3d
Cr-Commit-Position: refs/heads/master@{#341016}

[yucliu1, 2015-07-28 23:34:06]

yucliu@chromium.org changed reviewers:
+ byungchul@chromium.org, gunsch@chromium.org, lcwu@chromium.org

[byungchul, 2015-07-28 23:43:34]

https://codereview.chromium.org/1262673004/diff/1/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/1262673004/diff/1/chromecast/chromecast.gyp#n...
chromecast/chromecast.gyp:225: ['chromecast_branding_as_chrome==1 and
use_chromecast_webui==1', {
why not chromecast_branding!='public' as lcwu suggested?

https://codereview.chromium.org/1262673004/diff/1/chromecast/chromecast_local...
File chromecast/chromecast_locales.gyp (right):

https://codereview.chromium.org/1262673004/diff/1/chromecast/chromecast_local...
chromecast/chromecast_locales.gyp:60:
'<!@pymod_do_main(chromecast_repack_locales -i -b <(chromecast_branding_name) -g
<(grit_out_dir) -x <(repack_output_dir) <(locales))'
Can we just use chromecast_branding here?

[yucliu1, 2015-07-29 00:02:00]

https://codereview.chromium.org/1262673004/diff/1/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/1262673004/diff/1/chromecast/chromecast.gyp#n...
chromecast/chromecast.gyp:225: ['chromecast_branding_as_chrome==1 and
use_chromecast_webui==1', {
On 2015/07/28 23:43:34, byungchul wrote:
> why not chromecast_branding!='public' as lcwu suggested?

It's maybe easier to change the code if we change the branding policy. I'm also
OK with lcwu's sugg.

[yucliu1, 2015-07-29 00:19:21]

https://codereview.chromium.org/1262673004/diff/1/chromecast/chromecast_local...
File chromecast/chromecast_locales.gyp (right):

https://codereview.chromium.org/1262673004/diff/1/chromecast/chromecast_local...
chromecast/chromecast_locales.gyp:60:
'<!@pymod_do_main(chromecast_repack_locales -i -b <(chromecast_branding_name) -g
<(grit_out_dir) -x <(repack_output_dir) <(locales))'
On 2015/07/28 23:43:34, byungchul wrote:
> Can we just use chromecast_branding here?

Yes. This needs to change chromecast_repack_locales.py.

[gunsch, 2015-07-29 00:21:10]

gunsch@chromium.org changed reviewers:
+ slan@chromium.org

[gunsch, 2015-07-29 00:22:15]

Please consider also changing the GN files.

+slan to note the GN files. I think they just use a 1/0 flag, which we should
change to using the same strings as GYP now.

https://codereview.chromium.org/1262673004/diff/1/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/1262673004/diff/1/chromecast/chromecast.gyp#n...
chromecast/chromecast.gyp:225: ['chromecast_branding_as_chrome==1 and
use_chromecast_webui==1', {
On 2015/07/29 00:01:59, yucliu1 wrote:
> On 2015/07/28 23:43:34, byungchul wrote:
> > why not chromecast_branding!='public' as lcwu suggested?
> 
> It's maybe easier to change the code if we change the branding policy. I'm
also
> OK with lcwu's sugg.

Where does chromecast_branding_as_chrome come from?

[yucliu1, 2015-07-29 00:23:56]

https://codereview.chromium.org/1262673004/diff/1/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/1262673004/diff/1/chromecast/chromecast.gyp#n...
chromecast/chromecast.gyp:225: ['chromecast_branding_as_chrome==1 and
use_chromecast_webui==1', {
On 2015/07/29 00:22:15, gunsch wrote:
> On 2015/07/29 00:01:59, yucliu1 wrote:
> > On 2015/07/28 23:43:34, byungchul wrote:
> > > why not chromecast_branding!='public' as lcwu suggested?
> > 
> > It's maybe easier to change the code if we change the branding policy. I'm
> also
> > OK with lcwu's sugg.
> 
> Where does chromecast_branding_as_chrome come from?

At the end of this file. There's a big "conditions" block.

[gunsch, 2015-07-29 00:31:04]

https://codereview.chromium.org/1262673004/diff/1/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/1262673004/diff/1/chromecast/chromecast.gyp#n...
chromecast/chromecast.gyp:225: ['chromecast_branding_as_chrome==1 and
use_chromecast_webui==1', {
On 2015/07/29 00:23:56, yucliu1 wrote:
> On 2015/07/29 00:22:15, gunsch wrote:
> > On 2015/07/29 00:01:59, yucliu1 wrote:
> > > On 2015/07/28 23:43:34, byungchul wrote:
> > > > why not chromecast_branding!='public' as lcwu suggested?
> > > 
> > > It's maybe easier to change the code if we change the branding policy. I'm
> > also
> > > OK with lcwu's sugg.
> > 
> > Where does chromecast_branding_as_chrome come from?
> 
> At the end of this file. There's a big "conditions" block.

Ah I see.

I would second preferring "chromecast_branding != 'public'". It's not very
idiomatic to have GYP variables depend on one another like this.

[yucliu1, 2015-07-29 00:56:33]

On 2015/07/29 00:22:15, gunsch wrote:
> Please consider also changing the GN files.
> 
> +slan to note the GN files. I think they just use a 1/0 flag, which we should
> change to using the same strings as GYP now.
> 
> https://codereview.chromium.org/1262673004/diff/1/chromecast/chromecast.gyp
> File chromecast/chromecast.gyp (right):
> 
>
https://codereview.chromium.org/1262673004/diff/1/chromecast/chromecast.gyp#n...
> chromecast/chromecast.gyp:225: ['chromecast_branding_as_chrome==1 and
> use_chromecast_webui==1', {
> On 2015/07/29 00:01:59, yucliu1 wrote:
> > On 2015/07/28 23:43:34, byungchul wrote:
> > > why not chromecast_branding!='public' as lcwu suggested?
> > 
> > It's maybe easier to change the code if we change the branding policy. I'm
> also
> > OK with lcwu's sugg.
> 
> Where does chromecast_branding_as_chrome come from?

I don't see chromecast_branding in GN files. What's the variable name they are
using?

[slan, 2015-07-29 01:00:47]

On 2015/07/29 00:56:33, yucliu1 wrote:
> On 2015/07/29 00:22:15, gunsch wrote:
> > Please consider also changing the GN files.
> > 
> > +slan to note the GN files. I think they just use a 1/0 flag, which we
should
> > change to using the same strings as GYP now.
> > 
> > https://codereview.chromium.org/1262673004/diff/1/chromecast/chromecast.gyp
> > File chromecast/chromecast.gyp (right):
> > 
> >
>
https://codereview.chromium.org/1262673004/diff/1/chromecast/chromecast.gyp#n...
> > chromecast/chromecast.gyp:225: ['chromecast_branding_as_chrome==1 and
> > use_chromecast_webui==1', {
> > On 2015/07/29 00:01:59, yucliu1 wrote:
> > > On 2015/07/28 23:43:34, byungchul wrote:
> > > > why not chromecast_branding!='public' as lcwu suggested?
> > > 
> > > It's maybe easier to change the code if we change the branding policy. I'm
> > also
> > > OK with lcwu's sugg.
> > 
> > Where does chromecast_branding_as_chrome come from?
> 
> I don't see chromecast_branding in GN files. What's the variable name they are
> using?

GN build uses the boolean is_chromecast_chrome_branded.
https://code.google.com/p/chromium/codesearch#chromium/src/chromecast/chromec...

[yucliu1, 2015-07-29 04:22:13]

On 2015/07/29 01:00:47, slan wrote:
> On 2015/07/29 00:56:33, yucliu1 wrote:
> > On 2015/07/29 00:22:15, gunsch wrote:
> > > Please consider also changing the GN files.
> > > 
> > > +slan to note the GN files. I think they just use a 1/0 flag, which we
> should
> > > change to using the same strings as GYP now.
> > > 
> > >
https://codereview.chromium.org/1262673004/diff/1/chromecast/chromecast.gyp
> > > File chromecast/chromecast.gyp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1262673004/diff/1/chromecast/chromecast.gyp#n...
> > > chromecast/chromecast.gyp:225: ['chromecast_branding_as_chrome==1 and
> > > use_chromecast_webui==1', {
> > > On 2015/07/29 00:01:59, yucliu1 wrote:
> > > > On 2015/07/28 23:43:34, byungchul wrote:
> > > > > why not chromecast_branding!='public' as lcwu suggested?
> > > > 
> > > > It's maybe easier to change the code if we change the branding policy.
I'm
> > > also
> > > > OK with lcwu's sugg.
> > > 
> > > Where does chromecast_branding_as_chrome come from?
> > 
> > I don't see chromecast_branding in GN files. What's the variable name they
are
> > using?
> 
> GN build uses the boolean is_chromecast_chrome_branded.
>
https://code.google.com/p/chromium/codesearch#chromium/src/chromecast/chromec...

Is the GN build system ready to use? For upstream? For internal?

[slan, 2015-07-29 04:39:58]

> Is the GN build system ready to use? For upstream? For internal?

Not yet. We are building public cast_shell, but these changes have not been
uploaded yet (neither publicly nor internally). My goal is to have all the
necessary CL's uploaded by the end of next week, then start on internal builds.

In the meantime, I understand that it is hard to test your GN changes. Please
insert the following comment in the line I linked to you:
# TODO(slan): Make this variable consistent with the GYP build.

I will take care of it, linking back to this CL.

Thanks.

[yucliu1, 2015-07-29 18:08:08]

Update GN files.

[gunsch, 2015-07-29 18:15:08]

lgtm % nit

https://codereview.chromium.org/1262673004/diff/40001/chromecast/chromecast.gni
File chromecast/chromecast.gni (right):

https://codereview.chromium.org/1262673004/diff/40001/chromecast/chromecast.g...
chromecast/chromecast.gni:6: # chromecast_branding is used to link different
binaries in different build.
"used to link different binaries" isn't the most helpful. all the flags do that.

[byungchul, 2015-07-29 18:39:20]

What happens for env name? Still setup_*chrome*_env_...?

https://codereview.chromium.org/1262673004/diff/40001/chromecast/tools/build/...
File chromecast/tools/build/chromecast_repack_locales.py (right):

https://codereview.chromium.org/1262673004/diff/40001/chromecast/tools/build/...
chromecast/tools/build/chromecast_repack_locales.py:114: if CHROMECAST_BRANDING
!= "public" and\
use () instead of \

[yucliu1, 2015-07-29 18:42:01]

On 2015/07/29 18:39:20, byungchul wrote:
> What happens for env name? Still setup_*chrome*_env_...?
> 
>
https://codereview.chromium.org/1262673004/diff/40001/chromecast/tools/build/...
> File chromecast/tools/build/chromecast_repack_locales.py (right):
> 
>
https://codereview.chromium.org/1262673004/diff/40001/chromecast/tools/build/...
> chromecast/tools/build/chromecast_repack_locales.py:114: if
CHROMECAST_BRANDING
> != "public" and\
> use () instead of \

Those are used internally, right? It's still chrome/chromium in internal repo.

[slan, 2015-07-29 19:11:26]

GN files lgtm

[yucliu1, 2015-07-29 20:11:17]

More nit.

[byungchul, 2015-07-29 20:20:58]

lgtm

nits

https://codereview.chromium.org/1262673004/diff/60001/chromecast/tools/build/...
File chromecast/tools/build/chromecast_repack_locales.py (right):

https://codereview.chromium.org/1262673004/diff/60001/chromecast/tools/build/...
chromecast/tools/build/chromecast_repack_locales.py:116: CHROMECAST_BRANDING !=
"google"):
prefer ' to " for string constants.

[yucliu1, 2015-07-29 20:34:02]

Wrap long lines

https://codereview.chromium.org/1262673004/diff/60001/chromecast/tools/build/...
File chromecast/tools/build/chromecast_repack_locales.py (right):

https://codereview.chromium.org/1262673004/diff/60001/chromecast/tools/build/...
chromecast/tools/build/chromecast_repack_locales.py:116: CHROMECAST_BRANDING !=
"google"):
On 2015/07/29 20:20:58, byungchul wrote:
> prefer ' to " for string constants.

Done.

[yucliu1, 2015-07-29 20:51:13]

The CQ bit was checked by yucliu@chromium.org

[yucliu1, 2015-07-29 20:51:13]

The patchset sent to the CQ was uploaded after l-g-t-m from slan@chromium.org,
gunsch@chromium.org, byungchul@chromium.org
Link to the patchset: https://codereview.chromium.org/1262673004/#ps80001
(title: "wrap lines")

[commit-bot: I haz the power, 2015-07-29 20:51:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1262673004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1262673004/80001

[byungchul, 2015-07-29 20:53:16]

https://codereview.chromium.org/1262673004/diff/80001/chromecast/tools/build/...
File chromecast/tools/build/chromecast_repack_locales.py (right):

https://codereview.chromium.org/1262673004/diff/80001/chromecast/tools/build/...
chromecast/tools/build/chromecast_repack_locales.py:119: "'public', 'internal'
or 'google'.\n")
Revert the change here. Outer quotation should be ' and inner " as others.

[yucliu1, 2015-07-29 20:53:58]

The CQ bit was unchecked by yucliu@chromium.org

[yucliu1, 2015-07-29 20:56:12]

https://codereview.chromium.org/1262673004/diff/80001/chromecast/tools/build/...
File chromecast/tools/build/chromecast_repack_locales.py (right):

https://codereview.chromium.org/1262673004/diff/80001/chromecast/tools/build/...
chromecast/tools/build/chromecast_repack_locales.py:119: "'public', 'internal'
or 'google'.\n")
On 2015/07/29 20:53:16, byungchul wrote:
> Revert the change here. Outer quotation should be ' and inner " as others.

Done.

[yucliu1, 2015-07-29 21:49:42]

The CQ bit was checked by yucliu@chromium.org

[yucliu1, 2015-07-29 21:49:42]

The patchset sent to the CQ was uploaded after l-g-t-m from slan@chromium.org,
byungchul@chromium.org, gunsch@chromium.org
Link to the patchset: https://codereview.chromium.org/1262673004/#ps100001
(title: "quote")

[commit-bot: I haz the power, 2015-07-29 21:50:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1262673004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1262673004/100001

[commit-bot: I haz the power, 2015-07-29 23:26:59]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-07-29 23:27:50]

Patchset 6 (id:??) landed as
https://crrev.com/0f46bd94566380a99ecda0753f659b7e114e3d3d
Cr-Commit-Position: refs/heads/master@{#341016}

[yucliu1, 2015-07-30 00:33:57]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1266723002/ by yucliu@chromium.org.

The reason for reverting is: Revert the CL temporary for TOT build. Checked in
again once the internal CLs are approved..