Starting mac l10n:...

Starting mac l10n:
  Added a script to process a xib file and generate a localizer out of the resource constants it finds in the xib.
  Update the MainMenu.xib to use a generated localizer.
  Kill off the menu_localizer in favor of a generated one.
  ui_localizer is a helper so each "localizer" is as small as possible.
  Build some menus out of base strings and the product name like windows.
Added the dir generated for the localizers so we can load the header to directly create them (menubar one).
Enable the other 3 languages we were building to help test.
Made the context menu code use the new code for handling window's accelerators and ellipsis.
Added unittest for ui_localizer.
Opened http://crbug.com/17380 to track the problem with the menu titles so I can move on to other parts of the UI for now.

TEST=The main menu will have some items localized now (and more will be localizable in the TC).
BUG=16764
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=21272

[TVL, 2009-07-20 16:24:39]

This basic approach is what I'm gonna go after for the strings with the xib
files.  I'm pretty confident this is right for menus.

I'm still digging into how Windows does the placement/sizing tweaks, but wanted
to get this going first.  Once I have a better handle on the Windows side, I'll
start writing up a doc and float that around.

[Mark Mentovai, 2009-07-20 16:34:53]

http://codereview.chromium.org/155774/diff/1/2
File build/mac/generate_localizer (right):

http://codereview.chromium.org/155774/diff/1/2#newcode18
Line 18: '''// Copyright (c) 2009 The Chromium Authors. All rights reserved.
We don't need lice in generated files.

http://codereview.chromium.org/155774/diff/1/2#newcode23
Line 23: // THIS IS A GENERATED FILE, DO NOT EDIT IT DIRECTLY!
It's nice to say what source went into producing a generated file, so people
know what to edit.

http://codereview.chromium.org/155774/diff/1/2#newcode25
Line 25: #ifndef %s_LOCALIZER_H_
I suggest using %s(keyname) and then formatting with template % dict.  Keeps
things mad flexible.

http://docs.python.org/library/stdtypes.html#string-formatting

http://codereview.chromium.org/155774/diff/1/2#newcode41
Line 41: localizer_template_mm = \
Same comments apply here as to the .h case.  And you can even share the format
dict.

http://codereview.chromium.org/155774/diff/1/2#newcode68
Line 68: // (if need be, use bsearch() instead)
What's "if need be?"  Does it mean TODO?

http://codereview.chromium.org/155774/diff/1/2#newcode93
Line 93: sys.stderr.write('ERROR: ibtool on \'%s\' failed (%d):\n%s\n' %
In Python, you can write 'ERROR: "%s"\n' or "ERROR: '%s'\n" - you've got two
separate but equal quoting characters so you can avoid writing \' and \".

http://codereview.chromium.org/155774/diff/1/2#newcode99
Line 99: """Extracts all the values that start with ^ID from the locializable
localizable

http://codereview.chromium.org/155774/diff/1/2#newcode118
Line 118: mm_file = localizer_template_mm % ( header_name, constant_list_str,
class_name )
80

http://codereview.chromium.org/155774/diff/1/2#newcode128
Line 128: xib_path = argv[1];
xib_path, output_h_path, output_mm_path = argv[1:]

http://codereview.chromium.org/155774/diff/1/2#newcode133
Line 133: plist_string = xib_localizable_strings(argv[1])
Use xib_path

http://codereview.chromium.org/155774/diff/1/2#newcode142
Line 142: sys.stderr.write('WARNING: no resource strings found\n');
One thing I like to see is a tool printing its own name (argv[0]) when it prints
error messages.  This makes it much easier to track down errors.  You can do
this elsewhere in this script too.

http://codereview.chromium.org/155774/diff/1/2#newcode147
Line 147: class_name = class_name[:-10];
-10 is magic.  Make '_localizer' a variable and then do -len(your_var).

http://codereview.chromium.org/155774/diff/1/4
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/155774/diff/1/4#newcode222
Line 222: scoped_nsobject<mainmenuLocalizer> localizer(
Not sure how I feel about the look here.

http://codereview.chromium.org/155774/diff/1/7
File chrome/chrome.gyp (right):

http://codereview.chromium.org/155774/diff/1/7#newcode1947
Line 1947: 'action': ['<(DEPTH)/build/mac/generate_localizer',
The script counts as an input too.  If it changes we want to run the action
again, right?

[TVL, 2009-07-20 17:09:24]

I think this gets everything but the generated class name.

due to how we do class & file names, I wasn't seeing a good way to handle it.  i
could take the class name as another argument to avoid the class name calc?

[TVL, 2009-07-20 19:15:20]

ok, updated:
  added a needed DEPS for checkdeps.
  did a better class name generation.

[Mark Mentovai, 2009-07-20 19:29:55]

http://codereview.chromium.org/155774/diff/29/30
File build/mac/generate_localizer (right):

http://codereview.chromium.org/155774/diff/29/30#newcode30
Line 30: #import <Cocoa/Cocoa.h>
I don't think we need this here.

http://codereview.chromium.org/155774/diff/29/30#newcode40
Line 40: #endif
#endif  // %(class_name)s_LOCALIZER_H_

http://codereview.chromium.org/155774/diff/29/30#newcode68
Line 68: - (NSString *)localizedStringForString:(NSString *)string {
I wonder...

I think this should be defined as a function only once somewhere in a
non-generated file.  This function in each generated file should then just be a
stub that calls it with the correct MacUIResourceMap and size parameter.

MacUIResourceMap (the struct definition) would then need to fall out into a
common header, which is good - the less stuff in the generated files, the
better.  You'd also lose some header #includes - good.  The generated file would
then become basically the map, a sizeof, and the stub.

http://codereview.chromium.org/155774/diff/29/30#newcode70
Line 70: // enought string constants in here, look at using bsearch to speed up
the
enough has no t in it.

[TVL, 2009-07-20 21:31:12]

fyi - i split the whitespace stuff in chrome.gyp into a different cl along with
the xib shuffle so i can keep this open longer as i work on more of it.

[TVL, 2009-07-21 20:10:12]

CL description update, new snapshot up.  I'd like to go ahead and get this
landed since it gets in everything we need to get localization done for the
menubar.  There are a bunch of new strings since Windows doesn't have a menubar,
so those will need localizing before they they show up in a new build.

I'll finish out the work on looking at stuff in a window, and then finish the
docs I started on this.

[pink (ping after 24hrs), 2009-07-21 20:41:38]

LGTM with comments below

http://codereview.chromium.org/155774/diff/51/1056
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/155774/diff/51/1056#newcode4212
Line 4212: </message> <!-- os == 'darwin' -->
the comment should go on the next line, i think.

http://codereview.chromium.org/155774/diff/51/1060
File chrome/browser/cocoa/ui_localizer.h (right):

http://codereview.chromium.org/155774/diff/51/1060#newcode25
Line 25: NSString *LocalizedStringForKeyFromMapList(NSString* key,
* always goes with the type name in chrome style, eg NSString*.

http://codereview.chromium.org/155774/diff/51/1061
File chrome/browser/cocoa/ui_localizer.mm (right):

http://codereview.chromium.org/155774/diff/51/1061#newcode21
Line 21: if ('&' == c) {
variable == constant, the other way impairs readability.

http://codereview.chromium.org/155774/diff/51/1061#newcode48
Line 48: // If we end up with enough string constants in here, look at using
bsearch to
80cols

[Mark Mentovai, 2009-07-21 20:47:53]

Rietveld didn't let me finish reviewing the unit test.

http://codereview.chromium.org/155774/diff/51/1054
File build/mac/generate_localizer (right):

http://codereview.chromium.org/155774/diff/51/1054#newcode39
Line 39: #endif %(class_name)s_LOCALIZER_H_
That doesn't smell like a comment to me.

http://codereview.chromium.org/155774/diff/51/1060
File chrome/browser/cocoa/ui_localizer.h (right):

http://codereview.chromium.org/155774/diff/51/1060#newcode15
Line 15: // Remove the window's style accelerator marker and change "..." into
an
Is this temporary?  Can we ultimately use real ellipses on every platform?

http://codereview.chromium.org/155774/diff/51/1060#newcode25
Line 25: NSString *LocalizedStringForKeyFromMapList(NSString* key,
NSString* Localized...

http://codereview.chromium.org/155774/diff/51/1060#newcode26
Line 26: const ResourceMap* aap_list,
What is an aap list?

http://codereview.chromium.org/155774/diff/51/1061
File chrome/browser/cocoa/ui_localizer.mm (right):

http://codereview.chromium.org/155774/diff/51/1061#newcode5
Line 5: #include "chrome/browser/cocoa/ui_localizer.h"
ObjC header, this can be #import

http://codereview.chromium.org/155774/diff/51/1061#newcode16
Line 16: const char16 elipsisUTF16 = 0x2026;
ellipsisUTF16

Shouldn't this be named in this_style since it's not inside an
@interface/@implementation?

http://codereview.chromium.org/155774/diff/51/1061#newcode18
Line 18: ret.reserve(label.length());
label.length() might be inexpensive but seeing it called this many times makes
me say "cache it in a local variable."

http://codereview.chromium.org/155774/diff/51/1061#newcode26
Line 26: } else if (('.' == c) && (i + 2) < label.length() && ('.' == label[i +
1])
Your use of parentheses in some cases is kind of inconsistent.

You don't really need any extra parens in this entire expression, but if you
want them for readability, you can just wrap them around each &&-separated term.

You don't need to do things like (i + 2) < label.length() (and in fact you don't
do this elsewhere, like on 19 and 22).

http://codereview.chromium.org/155774/diff/51/1061#newcode38
Line 38: NSString *LocalizedStringForKeyFromMapList(NSString* key,
NSString* L...

http://codereview.chromium.org/155774/diff/51/1061#newcode43
Line 43: DCHECK(map_list_len >= 0);
map_list_len is unsigned, this is pointless.

http://codereview.chromium.org/155774/diff/51/1061#newcode48
Line 48: // If we end up with enough string constants in here, look at using
bsearch to
80

http://codereview.chromium.org/155774/diff/51/1061#newcode65
Line 65: // If we've passed where the string would be, give up.
I'd put a blank line before this.

http://codereview.chromium.org/155774/diff/51/1062
File chrome/browser/cocoa/ui_localizer_unittest.mm (right):

http://codereview.chromium.org/155774/diff/51/1062#newcode11
Line 11: TEST(ui_localizer, FixUpWindowsStyleLabel) {
Since this test pulls in Cocoid string manipulation, doesn't it demand an
autorelease pool?  Should it be a PlatformTest to get the pool management?

http://codereview.chromium.org/155774/diff/51/1062#newcode27
Line 27: { @"foo bar...", @"foo bar…" },
I think we should write UTF-8 sequences with \x here instead of using literal
'…'.  See base/string_util_unittest.cc for an example.

http://codereview.chromium.org/155774/diff/51/1062#newcode40
Line 40: << "For idx " << idx << ", expected '" << [data[idx].output UTF8String]
Needs more cowbell.  And indent.

http://codereview.chromium.org/155774/diff/51/1066
File chrome/browser/tab_contents/render_view_context_menu_mac.mm (right):

http://codereview.chromium.org/155774/diff/51/1066#newcode10
Line 10: #include "chrome/browser/cocoa/ui_localizer.h"
#import

[pink (ping after 24hrs), 2009-07-21 20:53:35]

http://codereview.chromium.org/155774/diff/51/1061
File chrome/browser/cocoa/ui_localizer.mm (right):

http://codereview.chromium.org/155774/diff/51/1061#newcode16
Line 16: const char16 elipsisUTF16 = 0x2026;
this should probably be kEllipsisUTF16 since it's a constant.

http://codereview.chromium.org/155774/diff/51/1061#newcode26
Line 26: } else if (('.' == c) && (i + 2) < label.length() && ('.' == label[i +
1])
If he doesn't check i+2 < length, it'll blow up if the string ends with one or
two periods, but not three, no?

[Mark Mentovai, 2009-07-21 21:00:30]

http://codereview.chromium.org/155774/diff/51/1061
File chrome/browser/cocoa/ui_localizer.mm (right):

http://codereview.chromium.org/155774/diff/51/1061#newcode26
Line 26: } else if (('.' == c) && (i + 2) < label.length() && ('.' == label[i +
1])
pink wrote:
> If he doesn't check i+2 < length, it'll blow up if the string ends with one or
> two periods, but not three, no?

He needs to check it, he just doesn't need to wrap |i + 2| in extra parentheses.

That was unclear from my comment, sorry.

[TVL, 2009-07-21 21:12:42]

new diffs up.

http://codereview.chromium.org/155774/diff/51/1060
File chrome/browser/cocoa/ui_localizer.h (right):

http://codereview.chromium.org/155774/diff/51/1060#newcode15
Line 15: // Remove the window's style accelerator marker and change "..." into
an
On 2009/07/21 20:47:53, Mark Mentovai wrote:
> Is this temporary?  Can we ultimately use real ellipses on every platform?

windows appears to not use real ones.

[Mark Mentovai, 2009-07-21 21:23:52]

LGTM

http://codereview.chromium.org/155774/diff/80/1083
File chrome/browser/cocoa/ui_localizer.h (right):

http://codereview.chromium.org/155774/diff/80/1083#newcode15
Line 15: // Remove the window's style accelerator marker and change "..." into
an
Windows-style

http://codereview.chromium.org/155774/diff/80/1083#newcode16
Line 16: // ellipsis and return the result in an autoreleased NSString.
This becomes a run-on at the second "and".

// ellipsis.  Returns the result...

http://codereview.chromium.org/155774/diff/80/1084
File chrome/browser/cocoa/ui_localizer.mm (right):

http://codereview.chromium.org/155774/diff/80/1084#newcode16
Line 16: const char16 ellipsis_utf16 = 0x2026;
pink suggested kEllipsisUTF16 because it's konstant, do you agree?

http://codereview.chromium.org/155774/diff/80/1084#newcode23
Line 23: if (i + 1 < label_len && '&' == label[i + 1]) {
label[i + 1] == '&' to placate pink again.

http://codereview.chromium.org/155774/diff/80/1084#newcode63
Line 63: 
This blank line is not helpful.

http://codereview.chromium.org/155774/diff/80/1085
File chrome/browser/cocoa/ui_localizer_unittest.mm (right):

http://codereview.chromium.org/155774/diff/80/1085#newcode36
Line 36: for (size_t idx = 0; idx < ARRAYSIZE_UNSAFE(data) ; ++idx) {
Nit: consistency with spacing around ;s.