Upstream Android-specific strings.

Upstream Android-specific strings.

BUG=136951
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150217

[newt (away), 2012-07-31 22:51:34]

PTAL, thanks!

[sky, 2012-07-31 23:09:11]

http://codereview.chromium.org/10824121/diff/1/chrome/app/chromium_strings.grd
File chrome/app/chromium_strings.grd (right):

http://codereview.chromium.org/10824121/diff/1/chrome/app/chromium_strings.gr...
chrome/app/chromium_strings.grd:447: Do you want Chromium to save your password
for this site?
Did you run these by the ux guys? Seems like we should be consistent.

http://codereview.chromium.org/10824121/diff/1/chrome/app/generated_resources...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10824121/diff/1/chrome/app/generated_resources...
chrome/app/generated_resources.grd:6164: +      <if expr="pp_ifdef('android')
and pp_ifdef('use_titlecase')">
Does android really need both titlecase and not-titlecase?

[newt (away), 2012-08-01 02:05:12]

http://codereview.chromium.org/10824121/diff/1/chrome/app/chromium_strings.grd
File chrome/app/chromium_strings.grd (right):

http://codereview.chromium.org/10824121/diff/1/chrome/app/chromium_strings.gr...
chrome/app/chromium_strings.grd:447: Do you want Chromium to save your password
for this site?
On 2012/07/31 23:09:11, sky wrote:
> Did you run these by the ux guys? Seems like we should be consistent.

This was a PM decision to make the infobar fit on the screen.  We lengthened the
prompt text and shortened the button text.

http://codereview.chromium.org/10824121/diff/1/chrome/app/generated_resources...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10824121/diff/1/chrome/app/generated_resources...
chrome/app/generated_resources.grd:6164: +      <if expr="pp_ifdef('android')
and pp_ifdef('use_titlecase')">
On 2012/07/31 23:09:11, sky wrote:
> Does android really need both titlecase and not-titlecase?

Good point.  No we don't.  In fact, we've incorrectly been using title case for
these few strings... which I've now fixed in common.gypi.

[newt (away), 2012-08-01 22:18:00]

This is ready for re-review.  Thank you!

[sky, 2012-08-02 17:22:50]

LGTM with the following change.

http://codereview.chromium.org/10824121/diff/4001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/10824121/diff/4001/build/common.gypi#newcode439
build/common.gypi:439: ['(toolkit_views==0 and OS!="android") or OS=="mac" or
OS=="ios"', {
Can you change the part you have in () to tookit_uses_gtk==1, eg:

'toolkit_uses_gtk==1 or OS=="mac" or OS=="ios"'

That matches the description on 440 and is easier to grok.

[newt (away), 2012-08-02 21:55:07]

I can't change common.gypi as you suggested :( -- see the comment

http://codereview.chromium.org/10824121/diff/4001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/10824121/diff/4001/build/common.gypi#newcode439
build/common.gypi:439: ['(toolkit_views==0 and OS!="android") or OS=="mac" or
OS=="ios"', {
On 2012/08/02 17:22:50, sky wrote:
> Can you change the part you have in () to tookit_uses_gtk==1, eg:
> 
> 'toolkit_uses_gtk==1 or OS=="mac" or OS=="ios"'
> 
> That matches the description on 440 and is easier to grok.

Hmm... there doesn't seem to be an easy way to use toolkit_uses_gtk since it's
not actually defined in this scope.  If I try the suggestion above, I get an
error: 

NameError: name 'toolkit_uses_gtk' is not defined while evaluating condition
'toolkit_uses_gtk==1 or OS=="mac" or OS=="ios"'

The variable dependency chain is: grit_defines -> use_titlecase_in_grd_files ->
toolkit_uses_gtk -> use_aura, which requires more levels of nested 'variables'
sections than we have.

The easiest solution seems be to leaving the code as is, which appears to be
working so far.  Or maybe there's a gyp trick that could be used here?  Do you
have any suggestions?

[sky, 2012-08-03 16:48:52]

I added Mark to the review in case he has any suggestions.

http://codereview.chromium.org/10824121/diff/4001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/10824121/diff/4001/build/common.gypi#newcode439
build/common.gypi:439: ['(toolkit_views==0 and OS!="android") or OS=="mac" or
OS=="ios"', {
On 2012/08/02 21:55:07, newt wrote:
> On 2012/08/02 17:22:50, sky wrote:
> > Can you change the part you have in () to tookit_uses_gtk==1, eg:
> > 
> > 'toolkit_uses_gtk==1 or OS=="mac" or OS=="ios"'
> > 
> > That matches the description on 440 and is easier to grok.
> 
> Hmm... there doesn't seem to be an easy way to use toolkit_uses_gtk since it's
> not actually defined in this scope.  If I try the suggestion above, I get an
> error: 
> 
> NameError: name 'toolkit_uses_gtk' is not defined while evaluating condition
> 'toolkit_uses_gtk==1 or OS=="mac" or OS=="ios"'
> 
> The variable dependency chain is: grit_defines -> use_titlecase_in_grd_files
->
> toolkit_uses_gtk -> use_aura, which requires more levels of nested 'variables'
> sections than we have.
> 
> The easiest solution seems be to leaving the code as is, which appears to be
> working so far.  Or maybe there's a gyp trick that could be used here?  Do you
> have any suggestions?

I'm surprised that this doesn't work since tookit_uses_gtk is defined above
this. Maybe Mark has some suggestions.

[Mark Mentovai, 2012-08-03 21:14:24]

http://codereview.chromium.org/10824121/diff/4001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/10824121/diff/4001/build/common.gypi#newcode439
build/common.gypi:439: ['(toolkit_views==0 and OS!="android") or OS=="mac" or
OS=="ios"', {
toolkit_uses_gtk is defined in THIS variables block, inside a conditions block.
You can’t use variables defined in the same scope as yourself, you’d need to put
the definition in an inner variables scope. You can then re-export that inner
version back out. That looks like:

'variables': {
  'variables: {
    'conditions': [
      ['your_condition', {
        'toolkit_uses_gtk%': 1,
      }, {  # else
        'toolkit_uses_gtk%': 1,
      }],
    ],
  },
  'toolkit_uses_gtk': '<(toolkit_uses_gtk)',  # export this back out a level
  'conditions': [
    ['toolkit_uses_gtk==1', {  # now this works
    }],
  ],
}

Sorta like how the thing below that says “# Copy conditionally-set variables out
one scope.” works.

[newt (away), 2012-08-03 22:47:57]

On 2012/08/03 21:14:24, Mark Mentovai wrote:
> http://codereview.chromium.org/10824121/diff/4001/build/common.gypi
> File build/common.gypi (right):
> 
> http://codereview.chromium.org/10824121/diff/4001/build/common.gypi#newcode439
> build/common.gypi:439: ['(toolkit_views==0 and OS!="android") or OS=="mac" or
> OS=="ios"', {
> toolkit_uses_gtk is defined in THIS variables block, inside a conditions
block.
> You can’t use variables defined in the same scope as yourself, you’d need to
put
> the definition in an inner variables scope. You can then re-export that inner
> version back out. That looks like:
> 
> 'variables': {
>   'variables: {
>     'conditions': [
>       ['your_condition', {
>         'toolkit_uses_gtk%': 1,
>       }, {  # else
>         'toolkit_uses_gtk%': 1,
>       }],
>     ],
>   },
>   'toolkit_uses_gtk': '<(toolkit_uses_gtk)',  # export this back out a level
>   'conditions': [
>     ['toolkit_uses_gtk==1', {  # now this works
>     }],
>   ],
> }
> 
> Sorta like how the thing below that says “# Copy conditionally-set variables
out
> one scope.” works.

Thanks for the explanation.  I've tried what you suggested, though it's more
complicated due to the variable dependencies. chromeos, use_aura, use_ash, and
toolkit_uses_gtk have to be moved around as well as a few conditionals.  See
patch set #3.  Is this what you're suggesting?

(Honestly, this makes me think gyp should support a type of variables section
where later variables can depend on previous ones without any nesting and
copying required :-/ )

[newt (away), 2012-08-03 22:51:44]

Mark, can you take another look?  Thanks.

(p.s. any changes after line 450 in common.gypi are from rebasing)

[Mark Mentovai, 2012-08-06 13:13:27]

This seems fine.

http://codereview.chromium.org/10824121/diff/11001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/10824121/diff/11001/build/common.gypi#newcode68
build/common.gypi:68: # Chromeos implies ash.
Isn’t ChromeOS spelled with more capital letters than this?

[newt (away), 2012-08-06 18:23:55]

Is this good to go now?

http://codereview.chromium.org/10824121/diff/11001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/10824121/diff/11001/build/common.gypi#newcode68
build/common.gypi:68: # Chromeos implies ash.
On 2012/08/06 13:13:28, Mark Mentovai wrote:
> Isn’t ChromeOS spelled with more capital letters than this?

Done.

[Mark Mentovai, 2012-08-06 18:34:44]

LGTM

[sky, 2012-08-06 20:41:52]

http://codereview.chromium.org/10824121/diff/13001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/10824121/diff/13001/build/common.gypi#newcode74
build/common.gypi:74: # For now, Windows *AND* Linux builds that |use_aura|
should also
Don't you mean windows only here?

[newt (away), 2012-08-06 21:13:16]

http://codereview.chromium.org/10824121/diff/13001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/10824121/diff/13001/build/common.gypi#newcode74
build/common.gypi:74: # For now, Windows *AND* Linux builds that |use_aura|
should also
On 2012/08/06 20:41:52, sky wrote:
> Don't you mean windows only here?

I guess so (though I don't know much about aura and ash).  That comment used to
make sense, then https://chromiumcodereview.appspot.com/10389217 changed things.
 Is it true that linux builds now use aura without ash?

[sky, 2012-08-06 21:47:49]

On Mon, Aug 6, 2012 at 2:13 PM,  <newt@chromium.org> wrote:
>
> http://codereview.chromium.org/10824121/diff/13001/build/common.gypi
> File build/common.gypi (right):
>
>
http://codereview.chromium.org/10824121/diff/13001/build/common.gypi#newcode74
> build/common.gypi:74: # For now, Windows *AND* Linux builds that
> |use_aura| should also
> On 2012/08/06 20:41:52, sky wrote:
>>
>> Don't you mean windows only here?
>
>
> I guess so (though I don't know much about aura and ash).  That comment
> used to make sense, then https://chromiumcodereview.appspot.com/10389217
> changed things.  Is it true that linux builds now use aura without ash?

Yes, that's why the comment was particularly confusing.

  -Scott

[newt (away), 2012-08-06 21:57:53]

http://codereview.chromium.org/10824121/diff/13001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/10824121/diff/13001/build/common.gypi#newcode74
build/common.gypi:74: # For now, Windows *AND* Linux builds that |use_aura|
should also
On 2012/08/06 20:41:52, sky wrote:
> Don't you mean windows only here?

Done.

[sky, 2012-08-06 22:53:29]

LGTM

[commit-bot: I haz the power, 2012-08-06 23:50:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/newt@chromium.org/10824121/13002

[commit-bot: I haz the power, 2012-08-07 01:23:15]

Change committed as 150217