Branding of Chrome.app....

Branding of Chrome.app.
To brand, do a "src/tools/gyp/gyp_dogfood -Dbranding=Chrome src/build/all.gyp"
(Then build, of course.)


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

[Mark Mentovai, 2009-03-03 23:46:04]

I'm glad you were able to figure this out without docs, even if it was
frustrating.

http://codereview.chromium.org/40066/diff/1/2
File build/common.gypi (right):

http://codereview.chromium.org/40066/diff/1/2#newcode11
Line 11: ['BRANDING=="Chromium"', { 'defines': ['CHROMIUM_BUILD'], } ],
This is annoying because now everyone has to do -DBRANDING=somethingorother. 
What we should do, a few lines up:

'variables': {
  'chromium_code%': 0,
  'branding%': 'Chromium',
},

That says that if branding isn't defined, set it to Chromium, otherwise, leave
it alone.

I've named it branding and not BRANDING because UPPERCASE_NAMES are for
GYP-predefined variables.  User variables should be named_like_this.

Moving on...

Rather than writing this as two separate conditionals, it should be an else.  So
in the final form, we'd have:

'conditions': [
  ['branding=="Chrome"', {
    'defines': ['GOOGLE_CHROME_BUILD'],
  }, {  # else: branding!="Chrome"
    'defines': ['CHROMIUM_BUILD'],
  }],
],

http://codereview.chromium.org/40066/diff/1/4
File chrome/chrome.gyp (right):

http://codereview.chromium.org/40066/diff/1/4#newcode1363
Line 1363: # TODO(jrg): Why doesn't this work?
Bug.  Hang on to this, I'll fix it tonight.  I would prefer doing it as you have
it written here in the comment.

http://codereview.chromium.org/40066/diff/1/4#newcode1455
Line 1455: 'target_name': 'package_app',
This target is nonsense (right now) on non-Mac, I think we should put it into a
conditions section to make it exist only on the Mac for the time being.

http://codereview.chromium.org/40066/diff/1/4#newcode1455
Line 1455: 'target_name': 'package_app',
This target is nonsense (right now) on non-Mac, I think we should put it into a
conditions section to make it exist only on the Mac for the time being.

http://codereview.chromium.org/40066/diff/1/4#newcode1455
Line 1455: 'target_name': 'package_app',
This target is nonsense (right now) on non-Mac, I think we should put it into a
conditions section to make it exist only on the Mac for the time being.

[John Grabowski, 2009-03-04 00:00:30]

New diffs uploaded.

http://codereview.chromium.org/40066/diff/1/2
File build/common.gypi (right):

http://codereview.chromium.org/40066/diff/1/2#newcode11
Line 11: ['BRANDING=="Chromium"', { 'defines': ['CHROMIUM_BUILD'], } ],
On 2009/03/03 23:46:06, Mark Mentovai wrote:
> This is annoying because now everyone has to do -DBRANDING=somethingorother. 
> What we should do, a few lines up:
> 
> 'variables': {
>   'chromium_code%': 0,
>   'branding%': 'Chromium',
> },
> 
> That says that if branding isn't defined, set it to Chromium, otherwise, leave
> it alone.
> 
> I've named it branding and not BRANDING because UPPERCASE_NAMES are for
> GYP-predefined variables.  User variables should be named_like_this.
> 
> Moving on...
> 
> Rather than writing this as two separate conditionals, it should be an else. 
So
> in the final form, we'd have:
> 
> 'conditions': [
>   ['branding=="Chrome"', {
>     'defines': ['GOOGLE_CHROME_BUILD'],
>   }, {  # else: branding!="Chrome"
>     'defines': ['CHROMIUM_BUILD'],
>   }],
> ],

Fixed.  (Don't forget to doc the % thing.)
Also downcased branding across this CL.

http://codereview.chromium.org/40066/diff/1/4
File chrome/chrome.gyp (right):

http://codereview.chromium.org/40066/diff/1/4#newcode1363
Line 1363: # TODO(jrg): Why doesn't this work?
On 2009/03/03 23:46:06, Mark Mentovai wrote:
> Bug.  Hang on to this, I'll fix it tonight.  I would prefer doing it as you
have
> it written here in the comment.

I will hang on.  Are you otherwise OK with the CL?  I'll be happy to change once
it's ready.

http://codereview.chromium.org/40066/diff/1/4#newcode1455
Line 1455: 'target_name': 'package_app',
On 2009/03/03 23:46:06, Mark Mentovai wrote:
> This target is nonsense (right now) on non-Mac, I think we should put it into
a
> conditions section to make it exist only on the Mac for the time being.

Added to a condition.

http://codereview.chromium.org/40066/diff/1/4#newcode1455
Line 1455: 'target_name': 'package_app',
On 2009/03/03 23:46:06, Mark Mentovai wrote:
> This target is nonsense (right now) on non-Mac, I think we should put it into
a
> conditions section to make it exist only on the Mac for the time being.

Added to a condition.

[Mark Mentovai, 2009-03-04 02:00:43]

John, do we want to brand as Google Chrome or just Chrome?  No need to get this
right just yet, but it's as good a time as any to think about it.

http://codereview.chromium.org/40066/diff/1003/14
File build/common.gypi (right):

http://codereview.chromium.org/40066/diff/1003/14#newcode12
Line 12: ['branding=="Chromium"', {
I'd put Chrome first because it's "special" and it's "everyone else" that should
get the generic case.

http://codereview.chromium.org/40066/diff/1003/14#newcode14
Line 14: }, { # else
I usually write

}, {  # else: branding!="Chrome"

so that a grep using the same format that you have to use for the conditional
itself will still find this.

http://codereview.chromium.org/40066/diff/1003/15
File chrome/app/app-Info.plist (right):

http://codereview.chromium.org/40066/diff/1003/15#newcode10
Line 10: <string>${PRODUCT_NAME:identifier}</string>
(fyi) In my version of heaven, we'd have app.icns in both branding forms, and we
could just set this to <string>app</string>.  But I appreciate that we're just
borrowing the .ico names Windows uses for our .icns names, and it's probably
more important to be consistent with Windows than for me to be 100% content with
minor details like this.

http://codereview.chromium.org/40066/diff/1003/16
File chrome/chrome.gyp (right):

http://codereview.chromium.org/40066/diff/1003/16#newcode1366
Line 1366: 'product_name': 'Chromium',
Let's do

['OS=="mac"', {
  'product_name': '<(branding)',  # after my thing is in
  'conditions': [
    ['branding=="Chrome"', {
      'mac_bundle_resources': ['app/theme/google_chrome/chrome.icns'],
    }, {  # else: branding!="Chrome"
      'mac_bundle_resources': ['app/theme/chromium/chromium.icns'],
    }],
  ],
}],

Coupla reasons:

1. The branding var is already perfectly suitable for use directly (but I know I
won't get any argument from you since you fought against my bug to do it this
way)

2. The fact that Python " and " works in conditions is a pet peeve of mine.

3. It gives us a regression test case for nested conditions.

http://codereview.chromium.org/40066/diff/1003/16#newcode1454
Line 1454: { 'conditions': [
This seems right, right?  But it's no good, if OS!="mac" then this will wind up
an empty dict (after the "conditions" is ripped out.)  GYP will come across an
empty target dict and promptly throw.

What we need to do is, after the 'targets': [{}, {}, {}] list, put a
'conditions' section that contains its own 'targets'.  When it's evaluated, the
'targets' in the 'conditions' will be appended to the unconditional list of
'targets'.

[John Grabowski, 2009-03-04 02:28:02]

Applied all your suggestions but having trouble with the last one (please
advise).

http://codereview.chromium.org/40066/diff/1003/16
File chrome/chrome.gyp (right):

http://codereview.chromium.org/40066/diff/1003/16#newcode1454
Line 1454: { 'conditions': [
On 2009/03/04 02:00:43, Mark Mentovai wrote:
> This seems right, right?  But it's no good, if OS!="mac" then this will wind
up
> an empty dict (after the "conditions" is ripped out.)  GYP will come across an
> empty target dict and promptly throw.
> 
> What we need to do is, after the 'targets': [{}, {}, {}] list, put a
> 'conditions' section that contains its own 'targets'.  When it's evaluated,
the
> 'targets' in the 'conditions' will be appended to the unconditional list of
> 'targets'.

This is borderline magical (i.e. if I see the error about empty dict it won't be
clear how to fix).  Perhaps gyp could ignore empty dicts?

In any case, I've made the change you proposed, but gyp yells at me.
See new uploaded diffs for my change.
For the added dict, if I place it in a standalone py program it parses fine, but
within the "main" dict gyp chokes with a syntax error.

Can you tell why?

[Mark Mentovai, 2009-03-04 02:47:33]

> This is borderline magical (i.e. if I see the error about empty dict it
> won't be clear how to fix).  Perhaps gyp could ignore empty dicts?

I think that's just as magical though.  What's an empty dict mean when it's the
value of another dict's key?  You can't just ignore it then.  What if you put an
empty dict in the "then" branch of a condition because your conditional makes
more sense to you as an if(x) NO-OP else OP?  You can't just ignore it then
either.

I agree that error handling could be better here but I'm not really sure what
good things can be done with an empty dict.  In the absence of better options,
I'd rather it be handled as a predictable error, even if it's somewhat more
difficult to track down the source of the error, than for it to be handled in
some "graceful" way that doesn't throw an error, and therefore winds up going
into production where it can do all sorts of damage while being even harder to
track down.

http://codereview.chromium.org/40066/diff/1005/23
File chrome/chrome.gyp (right):

http://codereview.chromium.org/40066/diff/1005/23#newcode1765
Line 1765: { 'conditions': [
You have too many dicts here, you need to get rid of the outer {

http://codereview.chromium.org/40066/diff/1005/23#newcode1785
Line 1785: ]},  # 'conditions'
and outer } here.

This is a parse error as soon as Python sees the comma.  You're already in dict
scope and you've just followed a comma (line 1762), so Python takes the entire
offending {} as a dict key.  Expecting a :, it gets pretty pissed when it sees a
, instead.

[Mark Mentovai, 2009-03-04 02:48:32]

Also, lgtm if you fix the extra curlies and don't want to wait for me to fix GYP
to do variable expansions inside conditionals.  I'm going to wait on Brad's
review to commit that, so if you commit this first I'll just send you a
follow-up when mine's in.

[John Grabowski, 2009-03-04 19:05:57]

On 2009/03/04 02:48:32, Mark Mentovai wrote:
> Also, lgtm if you fix the extra curlies and don't want to wait for me to fix
GYP
> to do variable expansions inside conditionals.  I'm going to wait on Brad's
> review to commit that, so if you commit this first I'll just send you a
> follow-up when mine's in.

Thx.  Final diffs uploaded.
Extra curlies fixed but I'll wait for you.
(Tree closed now anyway...)

[Mark Mentovai, 2009-03-04 19:16:09]

Also, I recommend fixing the CL desc so that it says branding instead of
BRANDING.

http://codereview.chromium.org/40066/diff/3001/3004
File chrome/chrome.gyp (right):

http://codereview.chromium.org/40066/diff/3001/3004#newcode1363
Line 1363: # TODO(jrg): Why doesn't this work?
Wait a sec, when you added the above didn't you want to take out the stuff
below?

[John Grabowski, 2009-03-04 19:29:34]

Thanks for catching this.
CL desc updated.

On 2009/03/04 19:16:09, Mark Mentovai wrote:
> Also, I recommend fixing the CL desc so that it says branding instead of
> BRANDING.
> 
> http://codereview.chromium.org/40066/diff/3001/3004
> File chrome/chrome.gyp (right):
> 
> http://codereview.chromium.org/40066/diff/3001/3004#newcode1363
> Line 1363: # TODO(jrg): Why doesn't this work?
> Wait a sec, when you added the above didn't you want to take out the stuff
> below?

[John Grabowski, 2009-03-04 19:30:04]

CL desc updated.

http://codereview.chromium.org/40066/diff/3001/3004
File chrome/chrome.gyp (right):

http://codereview.chromium.org/40066/diff/3001/3004#newcode1363
Line 1363: # TODO(jrg): Why doesn't this work?
On 2009/03/04 19:16:10, Mark Mentovai wrote:
> Wait a sec, when you added the above didn't you want to take out the stuff
> below?

Yes; removed.

[Mark Mentovai, 2009-03-04 19:32:19]

http://codereview.chromium.org/40066/diff/27/1012
File chrome/chrome.gyp (right):

http://codereview.chromium.org/40066/diff/27/1012#newcode1365
Line 1365: ['OS=="mac"', { 'product_name': '<(branding)', }],
This still duplicates the above OS=="mac" section.

I like the comment though, can you move that up when you get rid of this?

[John Grabowski, 2009-03-04 20:31:44]

http://codereview.chromium.org/40066/diff/27/1012
File chrome/chrome.gyp (right):

http://codereview.chromium.org/40066/diff/27/1012#newcode1365
Line 1365: ['OS=="mac"', { 'product_name': '<(branding)', }],
On 2009/03/04 19:32:20, Mark Mentovai wrote:
> This still duplicates the above OS=="mac" section.
> 
> I like the comment though, can you move that up when you get rid of this?

Ahh.. there were actually 3 versions in this file.  
Deleted, and comment moved up.
Version submitted will only have one.

[Mark Mentovai, 2009-03-04 20:35:48]

This version is perfect.

[Mark Larson, 2009-03-11 01:38:22]

On 2009/03/04 02:00:43, Mark Mentovai wrote:
> John, do we want to brand as Google Chrome or just Chrome?  No need to get
this
> right just yet, but it's as good a time as any to think about it.


The product name is Google Chrome.


> 
> http://codereview.chromium.org/40066/diff/1003/14
> File build/common.gypi (right):
> 
> http://codereview.chromium.org/40066/diff/1003/14#newcode12
> Line 12: ['branding=="Chromium"', {
> I'd put Chrome first because it's "special" and it's "everyone else" that
should
> get the generic case.
> 
> http://codereview.chromium.org/40066/diff/1003/14#newcode14
> Line 14: }, { # else
> I usually write
> 
> }, {  # else: branding!="Chrome"
> 
> so that a grep using the same format that you have to use for the conditional
> itself will still find this.
> 
> http://codereview.chromium.org/40066/diff/1003/15
> File chrome/app/app-Info.plist (right):
> 
> http://codereview.chromium.org/40066/diff/1003/15#newcode10
> Line 10: <string>${PRODUCT_NAME:identifier}</string>
> (fyi) In my version of heaven, we'd have app.icns in both branding forms, and
we
> could just set this to <string>app</string>.  But I appreciate that we're just
> borrowing the .ico names Windows uses for our .icns names, and it's probably
> more important to be consistent with Windows than for me to be 100% content
with
> minor details like this.
> 
> http://codereview.chromium.org/40066/diff/1003/16
> File chrome/chrome.gyp (right):
> 
> http://codereview.chromium.org/40066/diff/1003/16#newcode1366
> Line 1366: 'product_name': 'Chromium',
> Let's do
> 
> ['OS=="mac"', {
>   'product_name': '<(branding)',  # after my thing is in
>   'conditions': [
>     ['branding=="Chrome"', {
>       'mac_bundle_resources': ['app/theme/google_chrome/chrome.icns'],
>     }, {  # else: branding!="Chrome"
>       'mac_bundle_resources': ['app/theme/chromium/chromium.icns'],
>     }],
>   ],
> }],
> 
> Coupla reasons:
> 
> 1. The branding var is already perfectly suitable for use directly (but I know
I
> won't get any argument from you since you fought against my bug to do it this
> way)
> 
> 2. The fact that Python " and " works in conditions is a pet peeve of mine.
> 
> 3. It gives us a regression test case for nested conditions.
> 
> http://codereview.chromium.org/40066/diff/1003/16#newcode1454
> Line 1454: { 'conditions': [
> This seems right, right?  But it's no good, if OS!="mac" then this will wind
up
> an empty dict (after the "conditions" is ripped out.)  GYP will come across an
> empty target dict and promptly throw.
> 
> What we need to do is, after the 'targets': [{}, {}, {}] list, put a
> 'conditions' section that contains its own 'targets'.  When it's evaluated,
the
> 'targets' in the 'conditions' will be appended to the unconditional list of
> 'targets'.