add the InfoPlist.strings generation based on GRD strings....

Tweak the comment in the lproj fixup script to explain why we need an "en" folder.
Added a string for the products short name, this is current mac only, and is used for the application menu title via the infoplist.strings file.
Added source for a tool to build InfoPlist.strings files based on the values within the GRD files.
Run the InfoPlist.strings generation tool during the build.
Added a script to take a string and list of locale and generate all the versions of the string.
Wired up the string locale tool so GYP knows about all the inputs/outputs from the InfoPlist.strings generation tool.
Stop setting some of the Info.plist keys that are now covered by the InfoPlist.strings files.
Update the mac links script to stop creating a resources link.
Add a shim to nuke the helper's resource link so it can get a real folder.
Helper in in chrome_paths_mac to find the main app's bundle (that the helper lives in).
At startup, if not the browser, set the main bundle to be the parent of this bundle.
Fix up the breakpad init to use the mac_util helper for main bundle.

TEST=when runnining in the supported languages, Finder Get Info should show a localized copyright.  The process names in activity monitor should also be correct and show localized names once the TC work is done.
BUG=19019
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=24104

[TVL, 2009-08-18 18:42:16]

This cl sorta grew, but it appears to be complete now.

[Mark Mentovai, 2009-08-18 21:38:22]

Here are some comments.

http://codereview.chromium.org/171040/diff/3040/3042
File base/mac_util.h (right):

http://codereview.chromium.org/171040/diff/3040/3042#newcode53
Line 53: FilePath GetParentBundlePath();
This does not belong here.  It's specific to the Chrome implementation.  It's
not generally useful in base.

I also think that it should be GetBrowserBundlePath (that name would only make
sense in Chrome), so that it can actually do something useful in the browser
process by returning the outer .app.

http://codereview.chromium.org/171040/diff/3040/3041
File base/mac_util.mm (right):

http://codereview.chromium.org/171040/diff/3040/3041#newcode98
Line 98: FilePath GetParentBundlePath() {
I think that this is hokey.  Let's see if it comes out better as
GetBrowserBundlePath and living in chrome.

http://codereview.chromium.org/171040/diff/3040/3043
File build/branding_value.sh (left):

http://codereview.chromium.org/171040/diff/3040/3043#oldcode1
Line 1: #!/bin/sh
You can't get rid of this script, other things use it.

http://codereview.chromium.org/171040/diff/3040/3045
File chrome/app/app-Info.plist (right):

http://codereview.chromium.org/171040/diff/3040/3045#newcode145
Line 145: <key>LSHasLocalizedDisplayName</key>
The keys in this file are sorted.  Or at least they're supposed to be.  Run this
through "plutil -convert binary1" and then back with "plutil -convert xml1".  Do
the same thing with the other plist.

http://codereview.chromium.org/171040/diff/3040/3046
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/171040/diff/3040/3046#newcode346
Line 346: // they cand set the main bundle to to be the framework so all our
resource
cand

http://codereview.chromium.org/171040/diff/3040/3046#newcode347
Line 347: // live within that (the lines above for the command line can also
move back
This whole TODO isn't really necessarily true, I'd just leave it out.

http://codereview.chromium.org/171040/diff/3040/3046#newcode349
Line 349: if (!process_type.empty()) {
Can we do this after Breakpad init?

http://codereview.chromium.org/171040/diff/3040/3049
File chrome/app/helper-Info.plist (right):

http://codereview.chromium.org/171040/diff/3040/3049#newcode1
Line 1: <?xml version="1.0" encoding="UTF-8"?>
sort comment

http://codereview.chromium.org/171040/diff/3040/3051
File chrome/app/nuke_mac_resources_link (right):

http://codereview.chromium.org/171040/diff/3040/3051#newcode9
Line 9: # The helper's Resource directory used to be a link, but now that we
need
Resources

http://codereview.chromium.org/171040/diff/3040/3051#newcode10
Line 10: # real resource, we have this script delete it as a stop gap for the
resources

it = ?

(the old symbolic link)

http://codereview.chromium.org/171040/diff/3040/3051#newcode17
Line 17: if [ -L "${CONTENTS_PATH}/Resources" ] ;
Why didn't you do RESOURCES_PATH?

http://codereview.chromium.org/171040/diff/3040/3051#newcode18
Line 18: then
then on same line as if, right after the ;.

http://codereview.chromium.org/171040/diff/3040/3051#newcode19
Line 19: rm "${CONTENTS_PATH}/Resources"
reuse RESOURCES_PATH.

Use rm -f.

http://codereview.chromium.org/171040/diff/3040/3052
File chrome/app/tweak_mac_lproj_folders (right):

http://codereview.chromium.org/171040/diff/3040/3052#newcode24
Line 24: # versions, and locale fall back will go remove suffixes (i.e. "en_GB"
falls
remove "go"

http://codereview.chromium.org/171040/diff/3040/3052#newcode25
Line 25: # back to "en"), so Chrome provides "en" so most users wanting English
get it
Delete everything after the comma and write it again.

http://codereview.chromium.org/171040/diff/3040/3055
File chrome/chrome.gyp (right):

http://codereview.chromium.org/171040/diff/3040/3055#newcode3107
Line 3107: #   CFBundleIdentifier of CHROMIUM_BUNDLE_ID
Whoops, mention "CFBundleSignature of CHROMIUM_CREATOR"

http://codereview.chromium.org/171040/diff/3040/3055#newcode3139
Line 3139: 'action_name': 'run_infoplist_strings_tool',
Why did you give it a name like run_infoplist_strings_tool?  It can be "Run
infoplist_strings_tool" or really anything.  You're not bound to
using_underscores_here.

http://codereview.chromium.org/171040/diff/3040/3055#newcode3143
Line 3143: 'output_path': '<(INTERMEDIATE_DIR)/main_infop_str',
What's main_infop_str?  You should just call it InfoPlist.strings, right?  Since
that's what it is?

http://codereview.chromium.org/171040/diff/3040/3055#newcode4721
Line 4721: 'inputs': [],
Doesn't this work if you omit inputs and outputs?

http://codereview.chromium.org/171040/diff/3040/3055#newcode4727
Line 4727: 'action_name': 'run_infoplist_strings_tool',
as above.

http://codereview.chromium.org/171040/diff/3040/3055#newcode4731
Line 4731: 'output_path': '<(INTERMEDIATE_DIR)/helper_infop_str',
Info.plist.  Again.  Because you've got a different INTERMEDIATE_DIR here than
when building the main .app.

http://codereview.chromium.org/171040/diff/3040/3053
File chrome/tools/build/apply_locales.py (right):

http://codereview.chromium.org/171040/diff/3040/3053#newcode11
Line 11: if argv is None:
Get rid of this and the next line.  Get rid of the default value for argv.

http://codereview.chromium.org/171040/diff/3040/3054
File chrome/tools/mac_helpers/infoplist_strings_util.mm (right):

http://codereview.chromium.org/171040/diff/3040/3054#newcode6
Line 6: // the GRD files and generate the infoPlist.strings files needed for a
InfoPlist.strings

http://codereview.chromium.org/171040/diff/3040/3054#newcode7
Line 7: // Mac OS X app bundles.
needed for a Mac OS X app bundles?

http://codereview.chromium.org/171040/diff/3040/3054#newcode9
Line 9: #import "Foundation/Foundation.h"
#import <...>

http://codereview.chromium.org/171040/diff/3040/3054#newcode101
Line 101: int main(int argc, char * const argv[]) {
Space only on one side of the *.

http://codereview.chromium.org/171040/diff/3040/3054#newcode102
Line 102: NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init];
Use base's scoped_nsautorelease_pool?

http://codereview.chromium.org/171040/diff/3040/3054#newcode156
Line 156: char * const * lang_list = argv;
also here, spaces on only one side of the *s.

http://codereview.chromium.org/171040/diff/3040/3054#newcode168
Line 168: for (int lp = 0; lp < lang_list_count; ++lp) {
What's lp stand for?

http://codereview.chromium.org/171040/diff/3040/3054#newcode207
Line 207: NSString *get_info = [NSString stringWithFormat:@"%@ %@, %@",
Is this right for all languages?

http://codereview.chromium.org/171040/diff/3040/3054#newcode211
Line 211: NSString* strings_file_contents_string =
Caution: embedded quotes and backslashes and stuff are honored in
InfoPlist.strings.  If found here, we'll produce unusable strings files.  If
we're not doing proper encoding, we should at least check the strings we've got
to make sure they don't have characters that would hurt the strings file parser.

http://codereview.chromium.org/171040/diff/3040/3054#newcode213
Line 213: @"CFBundleName = \"%@\";\n"
Let's keep this sorted by key too, for reasons of OCD.

http://codereview.chromium.org/171040/diff/3040/3054#newcode219
Line 219: // We setup Xcode projects expecting strings files to be UTF8, so make
set up as a verb is two words.

http://codereview.chromium.org/171040/diff/3040/3054#newcode220
Line 220: // sure we write the data in that form.
.strings files should be UTF-16, though.  In fact, they should specifically be
UTF-16BE with a BOM.

[TVL, 2009-08-19 12:54:29]

http://codereview.chromium.org/171040/diff/3040/3046
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/171040/diff/3040/3046#newcode349
Line 349: if (!process_type.empty()) {
On 2009/08/18 21:38:22, Mark Mentovai wrote:
> Can we do this after Breakpad init?

Nope, because it needs to have the right bundle to fetch the breakpad helper
binaries from.

http://codereview.chromium.org/171040/diff/3040/3051
File chrome/app/nuke_mac_resources_link (right):

http://codereview.chromium.org/171040/diff/3040/3051#newcode17
Line 17: if [ -L "${CONTENTS_PATH}/Resources" ] ;
On 2009/08/18 21:38:22, Mark Mentovai wrote:
> Why didn't you do RESOURCES_PATH?

Just following your lead.  :)

Do you mean UNLOCALIZED_RESOURCES_FOLDER_PATH?  I don't think there is a
RESOURCES_PATH in Xcodes env.

http://codereview.chromium.org/171040/diff/3040/3055
File chrome/chrome.gyp (right):

http://codereview.chromium.org/171040/diff/3040/3055#newcode3139
Line 3139: 'action_name': 'run_infoplist_strings_tool',
On 2009/08/18 21:38:22, Mark Mentovai wrote:
> Why did you give it a name like run_infoplist_strings_tool?  It can be "Run
> infoplist_strings_tool" or really anything.  You're not bound to
> using_underscores_here.

habit.

http://codereview.chromium.org/171040/diff/3040/3055#newcode3143
Line 3143: 'output_path': '<(INTERMEDIATE_DIR)/main_infop_str',
On 2009/08/18 21:38:22, Mark Mentovai wrote:
> What's main_infop_str?  You should just call it InfoPlist.strings, right? 
Since
> that's what it is?

that's just a directory to write the, since both the main app and helper need to
do this, they need their own directories to scribble in and INTERMEDIATE_DIR
seems to be common across all targets in a project, so it needs something so the
two sets of InfoPlist.strings don't step on each other.

http://codereview.chromium.org/171040/diff/3040/3055#newcode4721
Line 4721: 'inputs': [],
On 2009/08/18 21:38:22, Mark Mentovai wrote:
> Doesn't this work if you omit inputs and outputs?

exception from the xcode generator if they aren't there.

http://codereview.chromium.org/171040/diff/3040/3055#newcode4731
Line 4731: 'output_path': '<(INTERMEDIATE_DIR)/helper_infop_str',
On 2009/08/18 21:38:22, Mark Mentovai wrote:
> Info.plist.  Again.  Because you've got a different INTERMEDIATE_DIR here than
> when building the main .app.

Actually you don't, it is: xcodebuild/chrome.build/DerivedSources/Debug

The promise in the docs isn't correct, and the comment at the top of the Xcode
generator is explicit about not making it per target, but per project.

http://codereview.chromium.org/171040/diff/3040/3054
File chrome/tools/mac_helpers/infoplist_strings_util.mm (right):

http://codereview.chromium.org/171040/diff/3040/3054#newcode220
Line 220: // sure we write the data in that form.
On 2009/08/18 21:38:22, Mark Mentovai wrote:
> .strings files should be UTF-16, though.  In fact, they should specifically be
> UTF-16BE with a BOM.

when it copies them it does the reencode for you.

[TVL, 2009-08-19 18:22:35]

New versions up, I need to rerun unittests after your fix for the extension
helper lands.

[Mark Mentovai, 2009-08-19 19:59:27]

LGTM.

Of course, since it's a huge changelist, the volume of comments looks kind of
heavy, but most of them are probably easy to hit.

http://codereview.chromium.org/171040/diff/2085/2087
File chrome/app/app-Info.plist (right):

http://codereview.chromium.org/171040/diff/2085/2087#newcode6
Line 6: <string>English</string>
Should we be using en-US in both Info.plists now?

http://codereview.chromium.org/171040/diff/2085/2089
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/171040/diff/2085/2089#newcode341
Line 341: parsed_command_line.GetSwitchValue(switches::kProcessType);
Continuation line gets a 4-space indent.

http://codereview.chromium.org/171040/diff/2085/2089#newcode345
Line 345: // the main bundle is so we can find out resources.
find our resources.

(but remember how I feel about comments that use "we" in most cases.)

http://codereview.chromium.org/171040/diff/2085/2089#newcode361
Line 361: // where debugging symbols are present.  But it needs the paths to the
The comment you're adding here isn't really a "but".

Also, I don't think it applies to this block here as much as it does to the
InitCrashReporter call a few lines above.

So maybe we can adjust that comment a few lines above to list this as another
reason that we currently need InitCrashReporter to be where it is.

http://codereview.chromium.org/171040/diff/2085/2090
File chrome/app/chromium_strings.grd (right):

http://codereview.chromium.org/171040/diff/2085/2090#newcode347
Line 347: <message name="IDS_SHORT_PRODUCT_NAME" desc="The short Chrome
application name, used for things like the Mac's application menu (16char or
less: Chrome not Google Chrome)">
desc="The application's short name, used for the Mac's application menu.  This
should be less than 16 characters.  Example: Chrome, not Google Chrome."

http://codereview.chromium.org/171040/diff/2085/2090#newcode350
Line 350: <message name="IDS_HELPER_NAME" desc="The Chrome Helper application
name (Mac Chrome uses different apps for the helper processes for vs the one the
user launches)">
desc="The helper application's name.  Should contain IDS_PRODUCT_NAME."

http://codereview.chromium.org/171040/diff/2085/2091
File chrome/app/google_chrome_strings.grd (right):

http://codereview.chromium.org/171040/diff/2085/2091#newcode395
Line 395: <message name="IDS_SHORT_PRODUCT_NAME" desc="The short Chrome
application name, used for things like the Mac's application menu (16char or
less: Chrome not Google Chrome)">
Use the desc strings proposed in the other file.

http://codereview.chromium.org/171040/diff/2085/2092
File chrome/app/helper-Info.plist (right):

http://codereview.chromium.org/171040/diff/2085/2092#newcode6
Line 6: <string>English</string>
Same question as in the other Info.plist.

http://codereview.chromium.org/171040/diff/2085/2094
File chrome/app/nuke_mac_resources_link (right):

http://codereview.chromium.org/171040/diff/2085/2094#newcode15
Line 15: CONTENTS_PATH="${BUILT_PRODUCTS_DIR}/${WRAPPER_NAME}/Contents"
Right, I meant that you should assign RESOURCES_PATH and not CONTENTS_PATH.

My lead was correct in make_mac_app_symlinks initially because we had
CONTENTS_PATH and we wanted to build Resources and Frameworks off of it.

In this file (and actually in make_mac_app_symlinks now), there is no need to
have a variable for Contents.  We want Resources in this file and Frameworks in
the other.

Actually, in make_mac_app_symlinks as it stands now, it's only used once, so we
don't really even need the variable at all.

http://codereview.chromium.org/171040/diff/2085/2095
File chrome/app/tweak_mac_lproj_folders (right):

http://codereview.chromium.org/171040/diff/2085/2095#newcode23
Line 23: # The normal OS language list includes English (not an specific
"flavor", like
Rewrite to:

# The standard OS language list includes English (en) and not a specific
# "flavor" such as U.S. English (en-US). When checking for a language match,
# the OS will remove subtags if no exact match is found, so that en will be
# used if the user's preferred language list only contains British English
# (en-GB) and no specific en-GB translation is available.
#
# To ensure that users with the default language list, which contains English
# without any regional subtag, as well as users with a specific variant of
# English with a regional subtag, will have access to an English-localized
# application, a symbolic link from en.lproj to en-US.lproj is provided.

http://codereview.chromium.org/171040/diff/2085/2095#newcode24
Line 24: #  en-US). When checking the list, the OS fall back will remove
suffixes (i.e.
Also, maybe all of this means that we shouldn't ship en-US.lproj but just move
it to en.lproj?

http://codereview.chromium.org/171040/diff/2085/2097
File chrome/common/chrome_paths_internal.h (right):

http://codereview.chromium.org/171040/diff/2085/2097#newcode33
Line 33: // Fetched the browser bundle path.  It is only valid too call this
from the
// Retrieves the browser bundle path.  It is only valid to call this from a
// helper process, as it makes assumptions about the location of the enclosing
// bundle on disk.

Also, I think that this should return the browser's own bundle when called from
within the browser.  Let's take a look at the code...

http://codereview.chromium.org/171040/diff/2085/2096
File chrome/common/chrome_paths_mac.mm (right):

http://codereview.chromium.org/171040/diff/2085/2096#newcode75
Line 75: bool GetBrowserBundlePath(FilePath* result) {
This ought to be simple.  Check [[[[NSBundle mainBundle] infoDictionary]
objectForKey:@"LSUIElement"] boolValue] or some junk like that.  If it's not an
LSUIElement, just pass back FilePath([[[NSBundle mainBundle] bundlePath]
fileSystmRepresentation]).

http://codereview.chromium.org/171040/diff/2085/2096#newcode76
Line 76: NSString* main_bundle_path = [[NSBundle mainBundle] bundlePath];
main_bundle_path is a little confusing in this function.  How about
this_bundle_path or self_bundle_path?  Depending on the structure of this
function, if you know it's always going to be a helper bundle, then
helper_bundle_path would be preferred.

http://codereview.chromium.org/171040/diff/2085/2096#newcode79
Line 79: // Helper lives as ...app/Contents/Resources/...Helper.app
Lives at, not as.

http://codereview.chromium.org/171040/diff/2085/2096#newcode82
Line 82: << "too few path components for us to be within another bundle";
"for us" not helpful.

http://codereview.chromium.org/171040/diff/2085/2096#newcode86
Line 86: NSString* final_ns_path = [NSString pathWithComponents:components];
final_ns_path is not as specific as it could be, either.  How about either
browser_bundle_path or outer_bundle_path?

http://codereview.chromium.org/171040/diff/2085/2096#newcode93
Line 93: return true;
You've got this function of type bool, but all you ever do is return true.

Make it return a real success value, make it return void, or make it return the
FilePath rather than building it up into a pointer.

http://codereview.chromium.org/171040/diff/2085/2099
File chrome/tools/mac_helpers/infoplist_strings_util.mm (right):

http://codereview.chromium.org/171040/diff/2085/2099#newcode24
Line 24: NSString* BuildVersionString(const char* version_file_path) {
BuildVersionString sounds like it might return something like @"9L30".  Maybe
call this ApplicationVersionString?

http://codereview.chromium.org/171040/diff/2085/2099#newcode26
Line 26: NSString *path_string = [NSString
stringWithUTF8String:version_file_path];
You used |Type* variable| on everything surrounding this line.  This line should
match.

http://codereview.chromium.org/171040/diff/2085/2099#newcode108
Line 108: '\n', '\t', '\"',
I'd add '\r' to the list too, at the very least.

http://codereview.chromium.org/171040/diff/2085/2099#newcode113
Line 113: NSString* replace_str = [NSString stringWithFormat:@"\\%c",
kNeedBackslash[x]];
1. 80

2. You didn't test this, did you?  You need to replace '\n' with '\\n'.  This
looks like it will replace it with '\\\n'.  This replacement is only correct for
'\\'.  You should really test this with a good mix of backslashes and other
funny characters.

http://codereview.chromium.org/171040/diff/2085/2099#newcode227
Line 227: // Helpers don't show their menubar, so we don't need a real short
name.
We should avoid putting CFBundleName in their InfoPlist.strings files
altogether, then, right?

http://codereview.chromium.org/171040/diff/2085/2099#newcode253
Line 253: // sure we write the data in that form (when Xcode copies them it will
No parentheses here.  It should be its own sentence.

[TVL, 2009-08-19 21:15:09]

http://codereview.chromium.org/171040/diff/2085/2095
File chrome/app/tweak_mac_lproj_folders (right):

http://codereview.chromium.org/171040/diff/2085/2095#newcode24
Line 24: #  en-US). When checking the list, the OS fall back will remove
suffixes (i.e.
On 2009/08/19 19:59:27, Mark Mentovai wrote:
> Also, maybe all of this means that we shouldn't ship en-US.lproj but just move
> it to en.lproj?

that gets even worse for a build pov, either we make all the scripts deal with
the remapping and get them into the inputs/outputs in GYP files, or we do a
rename after a build, and then a rebuild will do all the resources work again
since en-US will have gone away.  :(  symlink seems simple/safe in this case.

[Mark Mentovai, 2009-08-19 21:38:23]

http://codereview.chromium.org/171040/diff/2108/2112
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/171040/diff/2108/2112#newcode357
Line 357: // to the helpers, and so it needs the base::MainAppBundle to be set
I don't know what the base::MainAppBundle is.  But you can say that this needs
to follow mac_util::SetOverrideAppBundlePath.

http://codereview.chromium.org/171040/diff/2108/2113
File chrome/app/chromium_strings.grd (right):

http://codereview.chromium.org/171040/diff/2108/2113#newcode347
Line 347: <message name="IDS_SHORT_PRODUCT_NAME" desc="The application's short
name, used for the Mac's application menu, activity monitor, etc. This should be
less than 16 characters.  Example: Chrome, not Google Chrome.">
nit: choose one or two spaces to put between sentences.

http://codereview.chromium.org/171040/diff/2108/2113#newcode350
Line 350: <message name="IDS_HELPER_NAME" desc="The Chrome Helper application
name (Mac Chrome uses different apps for the helper processes for vs the one the
user launches)">
I gave you better text for this, didn't I?

http://codereview.chromium.org/171040/diff/2108/2113#newcode353
Line 353: <message name="IDS_SHORT_HELPER_NAME" desc="The Chrome Helper
application's short name, used for the Mac's application menu, activity monitor,
etc. This should be less than 16 characters.  Example: Chrome Helper, not Google
Chrome Helper.">
There's no way this will be less than 16 characters in all languages, given
Chromium.

http://codereview.chromium.org/171040/diff/2108/2120
File chrome/common/chrome_paths_internal.h (right):

http://codereview.chromium.org/171040/diff/2108/2120#newcode33
Line 33: // Retrieves the browser bundle path.  It is only valid to call this
from a
This comment is no longer valid.

http://codereview.chromium.org/171040/diff/2108/2119
File chrome/common/chrome_paths_mac.mm (right):

http://codereview.chromium.org/171040/diff/2108/2119#newcode75
Line 75: NSBundle* mainBundle = [NSBundle mainBundle];
Don't mix interCapped with underscore_separated names.

http://codereview.chromium.org/171040/diff/2108/2119#newcode76
Line 76: NSString* main_bundle_path = [mainBundle bundlePath];
Didn't I suggest that "main" is too confusing a name for this application and
that "self" or "this" would have been better?

http://codereview.chromium.org/171040/diff/2108/2122
File chrome/tools/mac_helpers/infoplist_strings_util.mm (right):

http://codereview.chromium.org/171040/diff/2108/2122#newcode221
Line 221: product_name =
Instead of having two branches to assign into product_name and
short_product_name, consider having the branch just set the variables that need
to change between the different calls.

[Mark Mentovai, 2009-08-19 21:48:41]

mark@chromium.org wrote:
> http://codereview.chromium.org/171040/diff/2085/2099#newcode227
> Line 227: // Helpers don't show their menubar, so we don't need a real
> short name.
> We should avoid putting CFBundleName in their InfoPlist.strings files
> altogether, then, right?

If we're going to put CFBundleName back into our Info.plists, then I
guess we should leave this in...and maybe do it the right way, with
its own grd file entry?