optimize branding - use branding_path_component variable for defining pranding specific paths

optimize branding - use branding_path_component variable for defining branding specific paths

Please take a look at my post here

https://groups.google.com/a/chromium.org/forum/?fromgroups#!topic/chromium-dev/pIQjhkVl4cE

I am not sure about build/common.gypi
Please correct me if I am wrong.

PS:
This is just to start, there are more places to refactor.

BUG=

Committed: https://crrev.com/c88987ff0e5f9df2218c1bb62e671cbbb50991e0
Cr-Commit-Position: refs/heads/master@{#326765}

[gburanov, 2015-04-17 13:25:05]

gburanov@gmail.com changed reviewers:
+ cjhopman@chromium.org, dpranke@chromium.org

[gburanov, 2015-04-17 13:26:19]

gburanov@gmail.com changed reviewers:
+ jochen@chromium.org, sky@chromium.org

[sdefresne, 2015-04-17 13:41:28]

Just some comments about the syntax.

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

https://codereview.chromium.org/1090213002/diff/1/build/common.gypi#newcode94
build/common.gypi:94: 'branding_path_component%': 'chromium',
You can remove this one.

https://codereview.chromium.org/1090213002/diff/1/build/common.gypi#newcode150
build/common.gypi:150: 'branding_path_component%': '<(branding_path_component)',
Should be: 'branding_path_component%': 'chromium',

https://codereview.chromium.org/1090213002/diff/1/build/common.gypi#newcode303
build/common.gypi:303: 'branding_path_component%': '<(branding_path_component)',
Fix indentation here and everywhere else.

[Dirk Pranke, 2015-04-18 00:38:25]

should there be corresponding changes in the GN files?

I'm not actually sure yet if we have the branding stuff hooked up in the
necessary places in GN, but please check and verify.

[gburanov, 2015-04-20 12:29:51]

Fixed according to comments

We have branding hooked in GN files, but we dont need to change anything in GN
files (at least in this review).

Because all paths are remaining exactly the same

[Michael Moss, 2015-04-21 15:11:52]

mmoss@chromium.org changed reviewers:
+ mmoss@chromium.org

[Michael Moss, 2015-04-21 15:11:53]

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

https://codereview.chromium.org/1090213002/diff/20001/build/common.gypi#newco...
build/common.gypi:192: 'branding_path_component%': '<(branding)',
Default 'branding' is "Chromium", but the usages you're replacing are
"chromium", so if branding is unspecified, this will break on case-sensitive
file systems.

[gburanov, 2015-04-21 15:48:21]

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

https://codereview.chromium.org/1090213002/diff/20001/build/common.gypi#newco...
build/common.gypi:192: 'branding_path_component%': '<(branding)',
On 2015/04/21 15:11:53, Michael Moss wrote:
> Default 'branding' is "Chromium", but the usages you're replacing are
> "chromium", so if branding is unspecified, this will break on case-sensitive
> file systems.

No, just to clarify:

For Chromium:
branding_path_component: 'chromium'
branding: 'Chromium'

For Google Chrome
branding_path_component: 'google_chrome'
branding: 'Chrome'

For other brandings, branding_path_component = branding


Take a look at upper line 
'branding_path_component%': 'chromium',

[Michael Moss, 2015-04-21 17:21:20]

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

https://codereview.chromium.org/1090213002/diff/20001/build/common.gypi#newco...
build/common.gypi:192: 'branding_path_component%': '<(branding)',
> Take a look at upper line 
> 'branding_path_component%': 'chromium',

That just sets a default, which you then universally override here, by setting
it to either 'google_chrome' or '<(branding)'. This effectively makes the
default "branding_path_component" value == the default "branding" value, which
is 'Chromium', not 'chromium'. You can easily see this by running gyp_chromium
with different "branding" values (or no "branding" defined) and see what paths
are used in the output.

[gburanov, 2015-04-21 17:26:59]

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

https://codereview.chromium.org/1090213002/diff/20001/build/common.gypi#newco...
build/common.gypi:192: 'branding_path_component%': '<(branding)',
yep, you are correct. sorry. I will fix it.

[Michael Moss, 2015-04-21 18:00:28]

https://codereview.chromium.org/1090213002/diff/20001/chrome/chrome_installer...
File chrome/chrome_installer.gypi (right):

https://codereview.chromium.org/1090213002/diff/20001/chrome/chrome_installer...
chrome/chrome_installer.gypi:10: 'branding_dir_100':
'app/theme/default_100_percent/<(branding_path_component)',
FYI, I think since this is moved out of the conditionals at the bottom, you can
also remove the workaround re-definitions of these variables in the Mac and
Linux sections below.

[gburanov, 2015-04-22 15:20:46]

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

https://codereview.chromium.org/1090213002/diff/20001/build/common.gypi#newco...
build/common.gypi:192: 'branding_path_component%': '<(branding)',
Do you know how to make double if here?

            ['branding == "Chrome"', {
              'branding_path_component%': 'google_chrome',
            }, {
                 ['branding == "Chromium"', {
                   'branding_pathcomponent%': 'chromium',
                 }, {
                   'branding_path_component%': '<(branding)',
                 }]
            }],


This one is not working...

[Michael Moss, 2015-04-22 16:16:51]

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

https://codereview.chromium.org/1090213002/diff/20001/build/common.gypi#newco...
build/common.gypi:192: 'branding_path_component%': '<(branding)',
You can wrap a 'conditions' inside a 'conditions', which is awkward, but I don't
know if there's any better way to basically do an 'if/else-if/else' in gyp,
like:

# if
['branding == "Chrome"', {
  'branding_path_component%': 'google_chrome',
}, {
  # else-if
  'conditions': [
    ['branding == "Chromium"', {
      'branding_path_component%': 'chromium',
    # else
    }, {
      'branding_path_component%': '<(branding)',
    }],
  ]
}],


Or maybe something like:

# Set the default above, where you currently set it to 'chromium'.
'branding_path_component%': '<(branding)',

# Then handle special cases (with no "else") in the 'conditions' block.
'conditions': [
  ['branding == "Chrome"', {
    'branding_path_component%': 'google_chrome',
  }],

  ['branding == "Chromium"', {
    'branding_path_component%': 'chromium',
  }],

[gburanov, 2015-04-23 09:02:20]

yep, fixed, I like the second much more!

[gburanov, 2015-04-23 11:34:08]

On 2015/04/23 09:02:20, gburanov wrote:
> yep, fixed, I like the second much more!

BTW, I have found one strange definition in remoting_options.gypi

    'branding_path': '../remoting/branding_<(branding)',

this seems dangerous to be (case sensitive issue) and can be also refactored by
me. What do you think?

[gburanov, 2015-04-23 11:46:21]

On 2015/04/23 11:34:08, gburanov wrote:
> On 2015/04/23 09:02:20, gburanov wrote:
> > yep, fixed, I like the second much more!
> 
> BTW, I have found one strange definition in remoting_options.gypi
> 
>     'branding_path': '../remoting/branding_<(branding)',
> 
> this seems dangerous to be (case sensitive issue) and can be also refactored
by
> me. What do you think?

but better to do it in separate review fo course

[gburanov, 2015-04-23 13:47:16]

Just take a look how much optimization it is possible to do after this review
will be closed. And this is just the first part.
https://codereview.chromium.org/1105633003

This will be the next review after this one will be closed and commited

[Michael Moss, 2015-04-23 15:23:34]

On 2015/04/23 11:34:08, gburanov wrote:
> On 2015/04/23 09:02:20, gburanov wrote:
> > yep, fixed, I like the second much more!
> 
> BTW, I have found one strange definition in remoting_options.gypi
> 
>     'branding_path': '../remoting/branding_<(branding)',
> 
> this seems dangerous to be (case sensitive issue) and can be also refactored
by
> me. What do you think?

Yes, that will break for any new "branding" values. It probably shouldn't be
doing that :(

[gburanov, 2015-04-23 15:40:56]

> Yes, that will break for any new "branding" values. It probably shouldn't be
> doing that :(

I means this will break for new branding anyway.

But IMHO it is better to use lower case file names and use

'branding_path': '../remoting/branding_<(branding_path_component)',

[gburanov, 2015-04-23 21:03:41]

Is it possible to close it today?)

[gburanov, 2015-04-23 21:04:06]

Hi. Is it possible to close it today please?)

[cjhopman, 2015-04-23 21:27:38]

lgtm

[Dirk Pranke, 2015-04-23 21:38:36]

lgtm

[gburanov, 2015-04-23 21:39:56]

The CQ bit was checked by gburanov@gmail.com

[commit-bot: I haz the power, 2015-04-23 21:40:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1090213002/40001

[commit-bot: I haz the power, 2015-04-23 21:48:12]

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

[commit-bot: I haz the power, 2015-04-23 21:48:14]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[gburanov, 2015-04-24 09:14:46]

gburanov@gmail.com changed reviewers:
+ jhawkins@chromium.org

[jochen (gone - plz use gerrit), 2015-04-24 09:19:28]

lgtm

[gburanov, 2015-04-24 09:22:27]

The CQ bit was checked by gburanov@gmail.com

[commit-bot: I haz the power, 2015-04-24 09:22:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1090213002/40001

[commit-bot: I haz the power, 2015-04-24 09:26:27]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-24 09:27:17]

Patchset 3 (id:??) landed as
https://crrev.com/c88987ff0e5f9df2218c1bb62e671cbbb50991e0
Cr-Commit-Position: refs/heads/master@{#326765}

[gburanov, 2015-04-24 09:34:54]

On 2015/04/24 09:27:17, I haz the power (commit-bot) wrote:
> Patchset 3 (id:??) landed as
> https://crrev.com/c88987ff0e5f9df2218c1bb62e671cbbb50991e0
> Cr-Commit-Position: refs/heads/master@{#326765}

thank you!

[James Hawkins, 2015-04-24 16:53:30]

lgtm

[sky, 2015-04-24 17:44:08]

LGTM