Include upstream libaddressinput strings in Chrome build.

Include upstream libaddressinput strings in Chrome build.

This patch adds the upstream libaddressinput strings into the Chrome
build inside of chrome/app/address_input.grd[p] to work well with the
translation tools.

This patch should precede using upstream libaddressinput to ensure that
the upstream libaddressinput has the correct strings well ahead of using
it in Chrome.

Although the strings include postal code examples and links for error
messages, these features are optional and can be turned off if
undesired.

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

[please use gerrit instead, 2014-05-31 01:30:12]

Evan: PTAL Patch Set 1.

[please use gerrit instead, 2014-05-31 01:31:44]

Lei: OWNERS PTAL Patch Set 1 (chrome/app/address_input_strings.grd[p]).

[Lei Zhang, 2014-05-31 01:39:47]

lgtm with estade's approval.

https://codereview.chromium.org/304223010/diff/20001/chrome/app/address_input...
File chrome/app/address_input_strings.grd (right):

https://codereview.chromium.org/304223010/diff/20001/chrome/app/address_input...
chrome/app/address_input_strings.grd:3: Copyright (C) 2013 Google Inc.
It's 2014, and no (C)

https://codereview.chromium.org/304223010/diff/20001/chrome/app/address_input...
File chrome/app/address_input_strings.grdp (right):

https://codereview.chromium.org/304223010/diff/20001/chrome/app/address_input...
chrome/app/address_input_strings.grdp:23: desc="A country or a political region
(Countries like the United States or
80 column rules don't seem to apply in grd files

https://codereview.chromium.org/304223010/diff/20001/chrome/app/address_input...
chrome/app/address_input_strings.grdp:194: This ZIP code format is not
recognized. Example of a valid ZIP code: <ph
name="EXAMPLE">$1<ex>90291</ex></ph>. Don't know your ZIP code? Find it out <ph
name="BEGIN_LINK">$2</ph>here<ph name="END_LINK">$3</ph>.
"find it out" -> "find out" ?

It's up to you and Evan to agree on the messages here.

[please use gerrit instead, 2014-06-01 14:27:26]

Thanks for the review, Lei!

https://codereview.chromium.org/304223010/diff/20001/chrome/app/address_input...
File chrome/app/address_input_strings.grd (right):

https://codereview.chromium.org/304223010/diff/20001/chrome/app/address_input...
chrome/app/address_input_strings.grd:3: Copyright (C) 2013 Google Inc.
On 2014/05/31 01:39:48, Lei Zhang wrote:
> It's 2014, and no (C)

This is a copy of third_party/libaddressinput/src/cpp/res/messages.grd that's
here to work better with our translation tools. I don't think I should be
changing the original copyright statement, should I?

https://codereview.chromium.org/304223010/diff/20001/chrome/app/address_input...
File chrome/app/address_input_strings.grdp (right):

https://codereview.chromium.org/304223010/diff/20001/chrome/app/address_input...
chrome/app/address_input_strings.grdp:23: desc="A country or a political region
(Countries like the United States or
On 2014/05/31 01:39:48, Lei Zhang wrote:
> 80 column rules don't seem to apply in grd files

Good observation :-). That's because GRD text messages are parsed as literal
strings where each newline or space (except trailing and leading) are present in
the resulting string. Thus we're forced to have super long lines for the text
messages. However, nothing (as far as I know) prevents us from having nicely
readable description messages. I find these to be easier to parse for a human...
Would you agree?

On possibility is that the translation tools might be sensitive to the newlines
and whitespace in the description. If that's the case, then I would have to undo
my hard work to make the descriptions readable. :-\

https://codereview.chromium.org/304223010/diff/20001/chrome/app/address_input...
chrome/app/address_input_strings.grdp:194: This ZIP code format is not
recognized. Example of a valid ZIP code: <ph
name="EXAMPLE">$1<ex>90291</ex></ph>. Don't know your ZIP code? Find it out <ph
name="BEGIN_LINK">$2</ph>here<ph name="END_LINK">$3</ph>.
On 2014/05/31 01:39:48, Lei Zhang wrote:
> "find it out" -> "find out" ?
> 
> It's up to you and Evan to agree on the messages here.

Nope, it's not up to me and Evan, because we have translations for these strings
;-). Changing these strings will cause a translation request, which must be
curated by the i18n team, because these strings are tricky to get right in most
languages. For example, a novice translator might translate CEDEX, postal code,
and ZIP code into a single phrase "postal code", but that would not be correct.
So we're trying to avoid any additional translation effort by re-using these
existing strings with translations.

[Lei Zhang, 2014-06-02 20:47:43]

https://codereview.chromium.org/304223010/diff/20001/chrome/app/address_input...
File chrome/app/address_input_strings.grd (right):

https://codereview.chromium.org/304223010/diff/20001/chrome/app/address_input...
chrome/app/address_input_strings.grd:3: Copyright (C) 2013 Google Inc.
On 2014/06/01 14:27:27, Rouslan Solomakhin wrote:
> On 2014/05/31 01:39:48, Lei Zhang wrote:
> > It's 2014, and no (C)
> 
> This is a copy of third_party/libaddressinput/src/cpp/res/messages.grd that's
> here to work better with our translation tools. I don't think I should be
> changing the original copyright statement, should I?

Since this is living in the chromium repo, it's better to have its copyright
header match the rest of Chromium. IANAL, but I believe Google can relicense its
own code to itself.

https://codereview.chromium.org/304223010/diff/20001/chrome/app/address_input...
File chrome/app/address_input_strings.grdp (right):

https://codereview.chromium.org/304223010/diff/20001/chrome/app/address_input...
chrome/app/address_input_strings.grdp:23: desc="A country or a political region
(Countries like the United States or
On 2014/06/01 14:27:27, Rouslan Solomakhin wrote:
> On 2014/05/31 01:39:48, Lei Zhang wrote:
> > 80 column rules don't seem to apply in grd files
> 
> Good observation :-). That's because GRD text messages are parsed as literal
> strings where each newline or space (except trailing and leading) are present
in
> the resulting string. Thus we're forced to have super long lines for the text
> messages. However, nothing (as far as I know) prevents us from having nicely
> readable description messages. I find these to be easier to parse for a
human...
> Would you agree?
> 
> On possibility is that the translation tools might be sensitive to the
newlines
> and whitespace in the description. If that's the case, then I would have to
undo
> my hard work to make the descriptions readable. :-\

All the other .grd files in this directory do not wrap at 80 chars. So it's
better to match the existing convention.

https://codereview.chromium.org/304223010/diff/20001/chrome/app/address_input...
chrome/app/address_input_strings.grdp:194: This ZIP code format is not
recognized. Example of a valid ZIP code: <ph
name="EXAMPLE">$1<ex>90291</ex></ph>. Don't know your ZIP code? Find it out <ph
name="BEGIN_LINK">$2</ph>here<ph name="END_LINK">$3</ph>.
On 2014/06/01 14:27:27, Rouslan Solomakhin wrote:
> On 2014/05/31 01:39:48, Lei Zhang wrote:
> > "find it out" -> "find out" ?
> > 
> > It's up to you and Evan to agree on the messages here.
> 
> Nope, it's not up to me and Evan, because we have translations for these
strings
> ;-). Changing these strings will cause a translation request, which must be
> curated by the i18n team, because these strings are tricky to get right in
most
> languages. For example, a novice translator might translate CEDEX, postal
code,
> and ZIP code into a single phrase "postal code", but that would not be
correct.
> So we're trying to avoid any additional translation effort by re-using these
> existing strings with translations.

SGTM.

[please use gerrit instead, 2014-06-02 21:07:21]

Thank you for the comments, Lei.

https://codereview.chromium.org/304223010/diff/20001/chrome/app/address_input...
File chrome/app/address_input_strings.grd (right):

https://codereview.chromium.org/304223010/diff/20001/chrome/app/address_input...
chrome/app/address_input_strings.grd:3: Copyright (C) 2013 Google Inc.
On 2014/06/02 20:47:43, Lei Zhang wrote:
> On 2014/06/01 14:27:27, Rouslan Solomakhin wrote:
> > On 2014/05/31 01:39:48, Lei Zhang wrote:
> > > It's 2014, and no (C)
> > 
> > This is a copy of third_party/libaddressinput/src/cpp/res/messages.grd
that's
> > here to work better with our translation tools. I don't think I should be
> > changing the original copyright statement, should I?
> 
> Since this is living in the chromium repo, it's better to have its copyright
> header match the rest of Chromium. IANAL, but I believe Google can relicense
its
> own code to itself.

Done.

https://codereview.chromium.org/304223010/diff/20001/chrome/app/address_input...
File chrome/app/address_input_strings.grdp (right):

https://codereview.chromium.org/304223010/diff/20001/chrome/app/address_input...
chrome/app/address_input_strings.grdp:23: desc="A country or a political region
(Countries like the United States or
On 2014/06/02 20:47:43, Lei Zhang wrote:
> On 2014/06/01 14:27:27, Rouslan Solomakhin wrote:
> > On 2014/05/31 01:39:48, Lei Zhang wrote:
> > > 80 column rules don't seem to apply in grd files
> > 
> > Good observation :-). That's because GRD text messages are parsed as literal
> > strings where each newline or space (except trailing and leading) are
present
> in
> > the resulting string. Thus we're forced to have super long lines for the
text
> > messages. However, nothing (as far as I know) prevents us from having nicely
> > readable description messages. I find these to be easier to parse for a
> human...
> > Would you agree?
> > 
> > On possibility is that the translation tools might be sensitive to the
> newlines
> > and whitespace in the description. If that's the case, then I would have to
> undo
> > my hard work to make the descriptions readable. :-\
> 
> All the other .grd files in this directory do not wrap at 80 chars. So it's
> better to match the existing convention.

Done.

[Lei Zhang, 2014-06-02 21:15:13]

Are the .xtb files all suppose to be only 4 lines long?

[please use gerrit instead, 2014-06-02 22:04:43]

On 2014/06/02 21:15:13, Lei Zhang wrote:
> Are the .xtb files all suppose to be only 4 lines long?

Yes, after this change goes in, we need to run our translation tools to pull in
the existing translations and update the .xtb files.

[Lei Zhang, 2014-06-02 22:05:34]

lgtm++

[Evan Stade, 2014-06-03 01:08:05]

how do you plan to roll changes in the future?

[please use gerrit instead, 2014-06-03 01:55:30]

On 2014/06/03 01:08:05, Evan Stade wrote:
> how do you plan to roll changes in the future?

If we roll DEPS and their strings have changed, we will need to copy-over the
messages.grd[p] files again. We should communicate with i18n team to make sure
they let us know ahead of time of string changes.

[Evan Stade, 2014-06-03 02:15:47]

On 2014/06/03 01:55:30, Rouslan Solomakhin wrote:
> On 2014/06/03 01:08:05, Evan Stade wrote:
> > how do you plan to roll changes in the future?
> 
> If we roll DEPS and their strings have changed, we will need to copy-over the
> messages.grd[p] files again. We should communicate with i18n team to make sure
> they let us know ahead of time of string changes.

that seems error prone...

[please use gerrit instead, 2014-06-03 02:33:16]

On 2014/06/03 02:15:47, Evan Stade wrote:
> On 2014/06/03 01:55:30, Rouslan Solomakhin wrote:
> > On 2014/06/03 01:08:05, Evan Stade wrote:
> > > how do you plan to roll changes in the future?
> > 
> > If we roll DEPS and their strings have changed, we will need to copy-over
the
> > messages.grd[p] files again. We should communicate with i18n team to make
sure
> > they let us know ahead of time of string changes.
> 
> that seems error prone...

A missing string ID would is caught compile time. Did you want a script that
copies over the file?

[Evan Stade, 2014-06-03 17:33:42]

Is it possible to have gclient pull it in there (so that it effectively
pulls it into two locations)?
On Jun 2, 2014 7:33 PM, <rouslan@chromium.org> wrote:

> On 2014/06/03 02:15:47, Evan Stade wrote:
>
>> On 2014/06/03 01:55:30, Rouslan Solomakhin wrote:
>> > On 2014/06/03 01:08:05, Evan Stade wrote:
>> > > how do you plan to roll changes in the future?
>> >
>> > If we roll DEPS and their strings have changed, we will need to
>> copy-over
>>
> the
>
>> > messages.grd[p] files again. We should communicate with i18n team to
>> make
>>
> sure
>
>> > they let us know ahead of time of string changes.
>>
>
>  that seems error prone...
>>
>
> A missing string ID would is caught compile time. Did you want a script
> that
> copies over the file?
>
> https://codereview.chromium.org/304223010/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[please use gerrit instead, 2014-06-03 17:49:37]

Ah, that sounds interesting. Let me try.


On Tue, Jun 3, 2014 at 10:33 AM, Evan Stade <estade@chromium.org> wrote:

> Is it possible to have gclient pull it in there (so that it effectively
> pulls it into two locations)?
> On Jun 2, 2014 7:33 PM, <rouslan@chromium.org> wrote:
>
>> On 2014/06/03 02:15:47, Evan Stade wrote:
>>
>>> On 2014/06/03 01:55:30, Rouslan Solomakhin wrote:
>>> > On 2014/06/03 01:08:05, Evan Stade wrote:
>>> > > how do you plan to roll changes in the future?
>>> >
>>> > If we roll DEPS and their strings have changed, we will need to
>>> copy-over
>>>
>> the
>>
>>> > messages.grd[p] files again. We should communicate with i18n team to
>>> make
>>>
>> sure
>>
>>> > they let us know ahead of time of string changes.
>>>
>>
>>  that seems error prone...
>>>
>>
>> A missing string ID would is caught compile time. Did you want a script
>> that
>> copies over the file?
>>
>> https://codereview.chromium.org/304223010/
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[please use gerrit instead, 2014-06-03 19:30:11]

Evan: PTAL Patch Set 6.

I've added a DEPS hook to copy over the file after sync.

Git does not allow checking out a single file from a repository. Only SVN can do
that.

Many of the bots are failing because they do not re-run deps2git.py after
applying the patch. If we decide to go with this approach, I will need to split
the patch into 2 parts.
1st: Update the DEPS and add the update-strings.py script.
2nd: The rest of the patch.

[Evan Stade, 2014-06-03 20:03:14]

Doesn't this script mean you don't need to check the translation files into git?

[please use gerrit instead, 2014-06-03 20:37:59]

Upstream libaddressinput does not have translation files.

[Evan Stade, 2014-06-03 21:41:09]

could you make the script add a comment to the .grdp that says this file is
automatically copied

[Evan Stade, 2014-06-03 21:41:13]

could you make the script add a comment to the .grdp that says this file is
automatically copied

[please use gerrit instead, 2014-06-03 22:11:48]

Evan: PTAL Patch Set 7.

The update-strings.py script adds the following message to the header of
address_input_strings.grdp:

DO NOT MODIFY.

This file will be overwritten by "gclient runhooks". It is a copy of
src/third_party/libaddressinput/src/cpp/res/messages.grdp. All modifications
must be done at https://libaddressinput.googlecode.com/.

[Evan Stade, 2014-06-03 22:16:09]

https://codereview.chromium.org/304223010/diff/120001/third_party/libaddressi...
File third_party/libaddressinput/chromium/tools/update-strings.py (right):

https://codereview.chromium.org/304223010/diff/120001/third_party/libaddressi...
third_party/libaddressinput/chromium/tools/update-strings.py:15: This file will
be overwritten by "gclient runhooks". It is a copy of
nit:

"""
DO NOT MODIFY.

This file is generated by "gclient runhooks". [...] Submit modifications to the
upstream library at <url>.
"""

https://codereview.chromium.org/304223010/diff/120001/third_party/libaddressi...
third_party/libaddressinput/chromium/tools/update-strings.py:31:
destination.write(source.readline())
uhhh, I would do:

destination.write(source.readline());
destination.write(HEADER);
destination.write(source.read());

And add XML comment markup to HEADER. This seems more robust than trying to
piggy-back on the XML comment markup that already exists in source.

[please use gerrit instead, 2014-06-03 22:39:02]

Evan: PTAL Patch Set 8.

https://codereview.chromium.org/304223010/diff/120001/third_party/libaddressi...
File third_party/libaddressinput/chromium/tools/update-strings.py (right):

https://codereview.chromium.org/304223010/diff/120001/third_party/libaddressi...
third_party/libaddressinput/chromium/tools/update-strings.py:15: This file will
be overwritten by "gclient runhooks". It is a copy of
On 2014/06/03 22:16:10, Evan Stade wrote:
> nit:
> 
> """
> DO NOT MODIFY.
> 
> This file is generated by "gclient runhooks". [...] Submit modifications to
the
> upstream library at <url>.
> """

Done.

https://codereview.chromium.org/304223010/diff/120001/third_party/libaddressi...
third_party/libaddressinput/chromium/tools/update-strings.py:31:
destination.write(source.readline())
On 2014/06/03 22:16:10, Evan Stade wrote:
> uhhh, I would do:
> 
> destination.write(source.readline());
> destination.write(HEADER);
> destination.write(source.read());
> 
> And add XML comment markup to HEADER. This seems more robust than trying to
> piggy-back on the XML comment markup that already exists in source.

Done.

[Evan Stade, 2014-06-03 23:21:06]

lgtm

[please use gerrit instead, 2014-06-03 23:30:13]

Sent the first patch into CQ from https://codereview.chromium.org/312883005/.

[please use gerrit instead, 2014-06-06 05:40:57]

The CQ bit was checked by rouslan@chromium.org

[commit-bot: I haz the power, 2014-06-06 05:42:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rouslan@chromium.org/304223010/150001

[commit-bot: I haz the power, 2014-06-06 09:40:13]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[commit-bot: I haz the power, 2014-06-06 09:46:25]

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

[commit-bot: I haz the power, 2014-06-06 09:46:26]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[please use gerrit instead, 2014-06-07 01:40:16]

The CQ bit was checked by rouslan@chromium.org

[commit-bot: I haz the power, 2014-06-07 01:42:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rouslan@chromium.org/304223010/330001

[commit-bot: I haz the power, 2014-06-07 07:06:05]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)

[commit-bot: I haz the power, 2014-06-07 07:36:04]

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

[commit-bot: I haz the power, 2014-06-07 07:36:05]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)

[please use gerrit instead, 2014-06-07 21:12:03]

The CQ bit was checked by rouslan@chromium.org

[commit-bot: I haz the power, 2014-06-07 21:12:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rouslan@chromium.org/304223010/350001

[commit-bot: I haz the power, 2014-06-07 23:02:22]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)

[commit-bot: I haz the power, 2014-06-07 23:23:55]

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

[commit-bot: I haz the power, 2014-06-07 23:23:56]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)

[please use gerrit instead, 2014-06-11 15:27:08]

The CQ bit was checked by rouslan@chromium.org

[commit-bot: I haz the power, 2014-06-11 15:30:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rouslan@chromium.org/304223010/390001

[commit-bot: I haz the power, 2014-06-11 21:03:38]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[please use gerrit instead, 2014-06-11 21:19:49]

The CQ bit was unchecked by rouslan@chromium.org

[please use gerrit instead, 2014-06-12 03:40:18]

The CQ bit was checked by rouslan@chromium.org

[commit-bot: I haz the power, 2014-06-12 03:43:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rouslan@chromium.org/304223010/390001

[commit-bot: I haz the power, 2014-06-12 07:21:18]

Change committed as 276559

[Torne, 2014-06-13 13:48:09]

Why does this require a hook to copy the file from one place to another? Can't
we just refer to the file in its original location? It seems excessive to
execute a custom python script just to do this.

This broke the Android WebView build since we don't execute the hooks. I *can*
fix up our scripts to run this specific script while it does the merge, but it
seems like it'd be easier to just get rid of the hook. Am I missing some reason
why it must be this way?

[Evan Stade, 2014-06-13 16:30:07]

On 2014/06/13 13:48:09, Torne wrote:
> Why does this require a hook to copy the file from one place to another? Can't
> we just refer to the file in its original location? It seems excessive to
> execute a custom python script just to do this.
> 
> This broke the Android WebView build since we don't execute the hooks. I *can*
> fix up our scripts to run this specific script while it does the merge, but it
> seems like it'd be easier to just get rid of the hook. Am I missing some
reason
> why it must be this way?

The TPMs have certain requirements about where in the tree strings may live.