libaddressinput string translations

libaddressinput string translations

- make string translations work
- break libaddressinput standalone build

BUG=333405
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245290

[Evan Stade, 2014-01-11 01:31:39]

Hey Mark,

this is a WIP and will need some cleanup before full review, but I'd like to
solicit your feedback on the general structure of the grd resources. Here's the
brief overview:

- libaddressinput is a third party library we're developing on Chrome trunk
before upstreaming and pulling via DEPS
- it has a grd file with some strings in it, which we'd like to pipe through the
chrome translation console
- to do this I must either create a new grd file or tack the strings in the
.grdp onto an existing grd file
- We've chosen the former. The new grd file is unique to libaddressinput.

[Mark Mentovai, 2014-01-13 16:43:36]

The way you’ve structured this seems fine to me.

https://codereview.chromium.org/134903003/diff/1/chrome/browser/ui/autofill/a...
File chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc (right):

https://codereview.chromium.org/134903003/diff/1/chrome/browser/ui/autofill/a...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:17: #include
"grit/libaddressinput_strings.h"
Why not in-line with the #includes above?

https://codereview.chromium.org/134903003/diff/1/third_party/libaddressinput/...
File
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_ui.h
(right):

https://codereview.chromium.org/134903003/diff/1/third_party/libaddressinput/...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_ui.h:29:
// Returns the UI components for the CLDR |region_code|. Returns an empty vector
on error.
Watch the 80 column limit.

[please use gerrit instead, 2014-01-13 18:50:12]

Drive by!

https://codereview.chromium.org/134903003/diff/1/chrome/browser/ui/autofill/a...
File chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc (right):

https://codereview.chromium.org/134903003/diff/1/chrome/browser/ui/autofill/a...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:70: NOTIMPLEMENTED() <<
" SOME STRING CALLED IDS_LIBADDRESSINPUT_I18N_DEPENDENT_LOCALITY_LABEL is ID "
<< IDS_LIBADDRESSINPUT_I18N_DEPENDENT_LOCALITY_LABEL << " and string is " <<
l10n_util::GetStringUTF16(IDS_LIBADDRESSINPUT_I18N_DEPENDENT_LOCALITY_LABEL);
Is this a debug log?

https://codereview.chromium.org/134903003/diff/1/chrome/chrome_browser_ui.gypi
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/134903003/diff/1/chrome/chrome_browser_ui.gyp...
chrome/chrome_browser_ui.gypi:3366:
'../third_party/libaddressinput/libaddressinput.gyp:libaddressinput_strings',
The dependency on libaddressinput_strings should not be necessary, because the
libaddressinput target already depends on it.

https://codereview.chromium.org/134903003/diff/1/third_party/libaddressinput/...
File
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_problem.h
(right):

https://codereview.chromium.org/134903003/diff/1/third_party/libaddressinput/...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_problem.h:21:
#include <string>
string include is not longer necessary.

https://codereview.chromium.org/134903003/diff/1/third_party/libaddressinput/...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_problem.h:21:
#include <string>
string include is no longer necessary.

https://codereview.chromium.org/134903003/diff/1/third_party/libaddressinput/...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_problem.h:71:
// The ID for the string that is the human readable description of the
Let's keep the ID vs id capitalization consistent.

https://codereview.chromium.org/134903003/diff/1/third_party/libaddressinput/...
File
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_ui_component.h
(right):

https://codereview.chromium.org/134903003/diff/1/third_party/libaddressinput/...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_ui_component.h:20:
#include <string>
string include is no longer necessary.

https://codereview.chromium.org/134903003/diff/1/third_party/libaddressinput/...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_ui_component.h:38:
// The id of the string for the name of the field, for example "City".
Let's keep the ID vs id capitalization consistent.

https://codereview.chromium.org/134903003/diff/1/third_party/libaddressinput/...
File third_party/libaddressinput/chromium/cpp/res/messages.grd (left):

https://codereview.chromium.org/134903003/diff/1/third_party/libaddressinput/...
third_party/libaddressinput/chromium/cpp/res/messages.grd:24: Library (WTL). -->
Let's keep this comment.

https://codereview.chromium.org/134903003/diff/1/third_party/libaddressinput/...
File third_party/libaddressinput/chromium/cpp/test/address_ui_test.cc (right):

https://codereview.chromium.org/134903003/diff/1/third_party/libaddressinput/...
third_party/libaddressinput/chromium/cpp/test/address_ui_test.cc:53: if
(component_it->name_id == 0) {
Use INVALID_MESSAGE_ID instead of 0. It's defined in "grit.h"

[Evan Stade, 2014-01-13 23:36:06]

https://codereview.chromium.org/134903003/diff/1/chrome/browser/ui/autofill/a...
File chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc (right):

https://codereview.chromium.org/134903003/diff/1/chrome/browser/ui/autofill/a...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:17: #include
"grit/libaddressinput_strings.h"
On 2014/01/13 16:43:36, Mark Mentovai wrote:
> Why not in-line with the #includes above?

This include is not necessary at all, it was only here to support debugging code
(this is part of the aforementioned cleanup that was necessary before full
review).

https://codereview.chromium.org/134903003/diff/1/chrome/browser/ui/autofill/a...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:70: NOTIMPLEMENTED() <<
" SOME STRING CALLED IDS_LIBADDRESSINPUT_I18N_DEPENDENT_LOCALITY_LABEL is ID "
<< IDS_LIBADDRESSINPUT_I18N_DEPENDENT_LOCALITY_LABEL << " and string is " <<
l10n_util::GetStringUTF16(IDS_LIBADDRESSINPUT_I18N_DEPENDENT_LOCALITY_LABEL);
On 2014/01/13 18:50:13, Rouslan Solomakhin wrote:
> Is this a debug log?

yes. This is a concrete example of this CL not being ready for review.

https://codereview.chromium.org/134903003/diff/1/chrome/chrome_browser_ui.gypi
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/134903003/diff/1/chrome/chrome_browser_ui.gyp...
chrome/chrome_browser_ui.gypi:3366:
'../third_party/libaddressinput/libaddressinput.gyp:libaddressinput_strings',
On 2014/01/13 18:50:13, Rouslan Solomakhin wrote:
> The dependency on libaddressinput_strings should not be necessary, because the
> libaddressinput target already depends on it.

I don't think we're supposed to rely on transitive dependencies. This is here
because code in chrome/browser/ui/autofill/ directly relies on this target.

https://codereview.chromium.org/134903003/diff/1/third_party/libaddressinput/...
File
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_problem.h
(right):

https://codereview.chromium.org/134903003/diff/1/third_party/libaddressinput/...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_problem.h:21:
#include <string>
On 2014/01/13 18:50:13, Rouslan Solomakhin wrote:
> string include is no longer necessary.

Done.

https://codereview.chromium.org/134903003/diff/1/third_party/libaddressinput/...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_problem.h:71:
// The ID for the string that is the human readable description of the
On 2014/01/13 18:50:13, Rouslan Solomakhin wrote:
> Let's keep the ID vs id capitalization consistent.

Done.

https://codereview.chromium.org/134903003/diff/1/third_party/libaddressinput/...
File
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_ui.h
(right):

https://codereview.chromium.org/134903003/diff/1/third_party/libaddressinput/...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_ui.h:29:
// Returns the UI components for the CLDR |region_code|. Returns an empty vector
on error.
On 2014/01/13 16:43:36, Mark Mentovai wrote:
> Watch the 80 column limit.

Done.

https://codereview.chromium.org/134903003/diff/1/third_party/libaddressinput/...
File
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_ui_component.h
(right):

https://codereview.chromium.org/134903003/diff/1/third_party/libaddressinput/...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_ui_component.h:20:
#include <string>
On 2014/01/13 18:50:13, Rouslan Solomakhin wrote:
> string include is no longer necessary.

Done.

https://codereview.chromium.org/134903003/diff/1/third_party/libaddressinput/...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_ui_component.h:38:
// The id of the string for the name of the field, for example "City".
On 2014/01/13 18:50:13, Rouslan Solomakhin wrote:
> Let's keep the ID vs id capitalization consistent.

Done.

https://codereview.chromium.org/134903003/diff/1/third_party/libaddressinput/...
File third_party/libaddressinput/chromium/cpp/res/messages.grd (left):

https://codereview.chromium.org/134903003/diff/1/third_party/libaddressinput/...
third_party/libaddressinput/chromium/cpp/res/messages.grd:24: Library (WTL). -->
On 2014/01/13 18:50:13, Rouslan Solomakhin wrote:
> Let's keep this comment.

I'm copying the format that all chrome .grds use. They don't have this comment
and they do have emit_type='prepend'.

Do you think that this comment should be copied into every .grd file? I don't
think comments on how grit works should be replicated in every place grit is
used.

https://codereview.chromium.org/134903003/diff/1/third_party/libaddressinput/...
File third_party/libaddressinput/chromium/cpp/test/address_ui_test.cc (right):

https://codereview.chromium.org/134903003/diff/1/third_party/libaddressinput/...
third_party/libaddressinput/chromium/cpp/test/address_ui_test.cc:53: if
(component_it->name_id == 0) {
On 2014/01/13 18:50:13, Rouslan Solomakhin wrote:
> Use INVALID_MESSAGE_ID instead of 0. It's defined in "grit.h"

This is a concrete example of this CL not being ready for review.

[Evan Stade, 2014-01-13 23:36:12]

Ready for full review now, please.

+mmoss for chrome/tools/build/repack_locales.py
+thestig for chrome/chrome_resources.gyp

[please use gerrit instead, 2014-01-14 00:00:41]

https://codereview.chromium.org/134903003/diff/1/third_party/libaddressinput/...
File third_party/libaddressinput/chromium/cpp/res/messages.grd (left):

https://codereview.chromium.org/134903003/diff/1/third_party/libaddressinput/...
third_party/libaddressinput/chromium/cpp/res/messages.grd:24: Library (WTL). -->
On 2014/01/13 23:36:07, Evan Stade wrote:
> On 2014/01/13 18:50:13, Rouslan Solomakhin wrote:
> > Let's keep this comment.
> 
> I'm copying the format that all chrome .grds use. They don't have this comment
> and they do have emit_type='prepend'.
> 
> Do you think that this comment should be copied into every .grd file? I don't
> think comments on how grit works should be replicated in every place grit is
> used.

I think that this comment should not be copied to every .grd file in Chromium,
because Chromium developers are familiar with how GRD works.

However, the future maintainer of this stand-alone library will feel a
persistent urge to remove the empty <emit> tag without this comment to remind
them why they shouldn't.

https://codereview.chromium.org/134903003/diff/130001/third_party/libaddressi...
File
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_ui_component.h
(right):

https://codereview.chromium.org/134903003/diff/130001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_ui_component.h:36:
// The ID of the string for the name of the field, for example "City".
The example should be IDS_LIBADDRESSINPUT_I18N_LOCALITY_LABEL.

[Evan Stade, 2014-01-14 00:42:03]

https://codereview.chromium.org/134903003/diff/1/third_party/libaddressinput/...
File third_party/libaddressinput/chromium/cpp/res/messages.grd (left):

https://codereview.chromium.org/134903003/diff/1/third_party/libaddressinput/...
third_party/libaddressinput/chromium/cpp/res/messages.grd:24: Library (WTL). -->
On 2014/01/14 00:00:43, Rouslan Solomakhin wrote:
> On 2014/01/13 23:36:07, Evan Stade wrote:
> > On 2014/01/13 18:50:13, Rouslan Solomakhin wrote:
> > > Let's keep this comment.
> > 
> > I'm copying the format that all chrome .grds use. They don't have this
comment
> > and they do have emit_type='prepend'.
> > 
> > Do you think that this comment should be copied into every .grd file? I
don't
> > think comments on how grit works should be replicated in every place grit is
> > used.
> 
> I think that this comment should not be copied to every .grd file in Chromium,
> because Chromium developers are familiar with how GRD works.
> 
> However, the future maintainer of this stand-alone library will feel a
> persistent urge to remove the empty <emit> tag without this comment to remind
> them why they shouldn't.

Done.

https://codereview.chromium.org/134903003/diff/130001/third_party/libaddressi...
File
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_ui_component.h
(right):

https://codereview.chromium.org/134903003/diff/130001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_ui_component.h:36:
// The ID of the string for the name of the field, for example "City".
On 2014/01/14 00:00:43, Rouslan Solomakhin wrote:
> The example should be IDS_LIBADDRESSINPUT_I18N_LOCALITY_LABEL.

Done.

[Evan Stade, 2014-01-14 00:52:30]

+thestig for real this time. Lei, please have a look at
chrome/chrome_resources.gyp

[Lei Zhang, 2014-01-14 00:58:19]

https://codereview.chromium.org/134903003/diff/190001/chrome/chrome_resources...
File chrome/chrome_resources.gyp (right):

https://codereview.chromium.org/134903003/diff/190001/chrome/chrome_resources...
chrome/chrome_resources.gyp:359:
'<(DEPTH)/third_party/libaddressinput/libaddressinput.gyp:libaddressinput_strings',
stuart: Should this be in the "OS != ios" section below?

[please use gerrit instead, 2014-01-14 01:01:18]

lgtm

[stuartmorgan, 2014-01-14 02:02:29]

https://codereview.chromium.org/134903003/diff/190001/chrome/chrome_resources...
File chrome/chrome_resources.gyp (right):

https://codereview.chromium.org/134903003/diff/190001/chrome/chrome_resources...
chrome/chrome_resources.gyp:359:
'<(DEPTH)/third_party/libaddressinput/libaddressinput.gyp:libaddressinput_strings',
On 2014/01/14 00:58:19, Lei Zhang wrote:
> stuart: Should this be in the "OS != ios" section below?

If the only include of the library is in chrome_browser_ui.gyp, then yes, it
should go below. (Also seems the library is excluded on Android, and protected
by a variable, so I'm wondering why this doesn't match.)

[Mark Mentovai, 2014-01-14 15:21:07]

LGTM

https://codereview.chromium.org/134903003/diff/190001/third_party/libaddressi...
File
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_ui.h
(right):

https://codereview.chromium.org/134903003/diff/190001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_ui.h:32:
const std::string& region_code);
This actually would all fit on one line now.

[Evan Stade, 2014-01-14 23:55:49]

https://codereview.chromium.org/134903003/diff/190001/chrome/chrome_resources...
File chrome/chrome_resources.gyp (right):

https://codereview.chromium.org/134903003/diff/190001/chrome/chrome_resources...
chrome/chrome_resources.gyp:359:
'<(DEPTH)/third_party/libaddressinput/libaddressinput.gyp:libaddressinput_strings',
On 2014/01/14 02:02:30, stuartmorgan wrote:
> On 2014/01/14 00:58:19, Lei Zhang wrote:
> > stuart: Should this be in the "OS != ios" section below?
> 
> If the only include of the library is in chrome_browser_ui.gyp, then yes, it
> should go below. (Also seems the library is excluded on Android, and protected
> by a variable, so I'm wondering why this doesn't match.)

moved

https://codereview.chromium.org/134903003/diff/190001/third_party/libaddressi...
File
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_ui.h
(right):

https://codereview.chromium.org/134903003/diff/190001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_ui.h:32:
const std::string& region_code);
On 2014/01/14 15:21:07, Mark Mentovai wrote:
> This actually would all fit on one line now.

Done.

[Lei Zhang, 2014-01-15 00:03:18]

chrome/*.gyp* lgtm

[commit-bot: I haz the power, 2014-01-15 23:50:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/134903003/530001

[commit-bot: I haz the power, 2014-01-16 01:25:03]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2014-01-16 02:08:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/134903003/790001

[commit-bot: I haz the power, 2014-01-16 02:48:34]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-16 04:02:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/134903003/790001

[commit-bot: I haz the power, 2014-01-16 04:25:53]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2014-01-16 17:56:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/134903003/1310001

[commit-bot: I haz the power, 2014-01-16 19:45:25]

Change committed as 245290