Added strings for notification when user presses backspace.

Added strings for notification when user presses backspace.

These will be used in a future CL
(https://codereview.chromium.org/1983803002). The strings are being
checked in early to ensure they are in before branch point.

Notry rationale: Tree is broken due to https://crbug.com/613258. All
other tests have passed on trybots.

BUG=610039
NOTRY=true

Committed: https://crrev.com/8386be414ec1537ad2c033889996f2419b330c42
Cr-Commit-Position: refs/heads/master@{#394968}

[Matt Giuca, 2016-05-19 01:20:06]

mgiuca@chromium.org changed reviewers:
+ pkasting@chromium.org

[Matt Giuca, 2016-05-19 04:23:38]

If you're happy with this, please CQ it to make sure it lands.

[Peter Kasting, 2016-05-19 09:19:00]

https://codereview.chromium.org/1995893002/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1995893002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:12664: +        Press |<ph
name="ACCELERATOR1">$1<ex>Alt</ex></ph>|+|<ph
name="ACCELERATOR2">$2<ex>←</ex></ph>| to go back
Putting pipes around the accelerators doesn't seem good -- if we want to do
anything to delineate them, we should be drawing it entirely in the code.  Pipes
will just look odd and make the localizers' job more confusing, I think.

https://codereview.chromium.org/1995893002/diff/1/ui/strings/ui_strings.grd
File ui/strings/ui_strings.grd (right):

https://codereview.chromium.org/1995893002/diff/1/ui/strings/ui_strings.grd#n...
ui/strings/ui_strings.grd:540: <!-- Shortcut Keys -->
Nit: Why does this get its own heading?  Shouldn't it just be another one in the
"Key names" list above?

[Peter Kasting, 2016-05-19 09:47:27]

https://codereview.chromium.org/1995893002/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1995893002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:12664: +        Press |<ph
name="ACCELERATOR1">$1<ex>Alt</ex></ph>|+|<ph
name="ACCELERATOR2">$2<ex>←</ex></ph>| to go back
On 2016/05/19 09:19:00, Peter Kasting wrote:
> Putting pipes around the accelerators doesn't seem good -- if we want to do
> anything to delineate them, we should be drawing it entirely in the code. 
Pipes
> will just look odd and make the localizers' job more confusing, I think.

Ah, now reading your other CL I understand why you did this.

Don't do things this way :).  Splitting the string on pipe chars is dangerous if
the localized string contains those characters.  It's also harder than just
getting the substitution locations directly: just call the variant of
GetStringFUTF16() that returns a vector of offsets.  You can pass empty strings
for the substitutions if you don't want to sub in anything.

[Matt Giuca, 2016-05-19 10:25:30]

https://codereview.chromium.org/1995893002/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1995893002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:12664: +        Press |<ph
name="ACCELERATOR1">$1<ex>Alt</ex></ph>|+|<ph
name="ACCELERATOR2">$2<ex>←</ex></ph>| to go back
On 2016/05/19 09:47:27, Peter Kasting wrote:
> On 2016/05/19 09:19:00, Peter Kasting wrote:
> > Putting pipes around the accelerators doesn't seem good -- if we want to do
> > anything to delineate them, we should be drawing it entirely in the code. 
> Pipes
> > will just look odd and make the localizers' job more confusing, I think.
> 
> Ah, now reading your other CL I understand why you did this.
> 
> Don't do things this way :).  Splitting the string on pipe chars is dangerous
if
> the localized string contains those characters.  It's also harder than just
> getting the substitution locations directly: just call the variant of
> GetStringFUTF16() that returns a vector of offsets.  You can pass empty
strings
> for the substitutions if you don't want to sub in anything.

This is the way we've always done it and it's not a good time to change it now:
see IDS_FULLSCREEN_PRESS_ESC_TO_EXIT_FULLSCREEN in generated_resources.grd. I
would have to edit all of those other strings and the existing code; arguably I
should do that but given that we're trying to land these strings for branch
point, it isn't a good time to be reworking how the existing feature works.

The reason for doing it this way is a little shaky (I don't have a strong
opinion one way or another). Here is the original discussion:
https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/exclusive...

1. estade said not to use substitutions but directly embed the name of the key
in the string so it can be localized better.
2. In doing that, I had to add pipes to delimit the button border.
3. Then for code reasons, it became difficult to avoid substitutions so I went
back to using substitutions, but kept the pipes.
4. msw (in that above comment) suggested using substitutions directly to draw
the box (as you are suggesting now). At that time I didn't change it.
5. We have had pipes in all such strings for the past 6 months.

I think having pipes is slightly more general as it allows us to migrate to
having the key names directly in the translated strings (which both Evan and the
l10n team have told me is best practice). But it carries the burden of
additional explanation for the translators. As I said, I'm not too fussed, but I
don't want to change it right now.

[Matt Giuca, 2016-05-19 10:27:01]

https://codereview.chromium.org/1995893002/diff/1/ui/strings/ui_strings.grd
File ui/strings/ui_strings.grd (right):

https://codereview.chromium.org/1995893002/diff/1/ui/strings/ui_strings.grd#n...
ui/strings/ui_strings.grd:540: <!-- Shortcut Keys -->
On 2016/05/19 09:19:00, Peter Kasting wrote:
> Nit: Why does this get its own heading?  Shouldn't it just be another one in
the
> "Key names" list above?

Oh yes, you're right. I don't have access to my git repo at the moment. Can you
leave me an LG and I'll fix this nit before submitting.

[Peter Kasting, 2016-05-19 19:44:20]

LGTM

https://codereview.chromium.org/1995893002/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1995893002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:12664: +        Press |<ph
name="ACCELERATOR1">$1<ex>Alt</ex></ph>|+|<ph
name="ACCELERATOR2">$2<ex>←</ex></ph>| to go back
On 2016/05/19 10:25:29, Matt Giuca wrote:
> On 2016/05/19 09:47:27, Peter Kasting wrote:
> > On 2016/05/19 09:19:00, Peter Kasting wrote:
> > > Putting pipes around the accelerators doesn't seem good -- if we want to
do
> > > anything to delineate them, we should be drawing it entirely in the code. 
> > Pipes
> > > will just look odd and make the localizers' job more confusing, I think.
> > 
> > Ah, now reading your other CL I understand why you did this.
> > 
> > Don't do things this way :).  Splitting the string on pipe chars is
dangerous
> if
> > the localized string contains those characters.  It's also harder than just
> > getting the substitution locations directly: just call the variant of
> > GetStringFUTF16() that returns a vector of offsets.  You can pass empty
> strings
> > for the substitutions if you don't want to sub in anything.
> 
> This is the way we've always done it and it's not a good time to change it
now:
> see IDS_FULLSCREEN_PRESS_ESC_TO_EXIT_FULLSCREEN in generated_resources.grd. I
> would have to edit all of those other strings and the existing code; arguably
I
> should do that but given that we're trying to land these strings for branch
> point, it isn't a good time to be reworking how the existing feature works.
> 
> The reason for doing it this way is a little shaky (I don't have a strong
> opinion one way or another). Here is the original discussion:
>
https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/exclusive...
> 
> 1. estade said not to use substitutions but directly embed the name of the key
> in the string so it can be localized better.
> 2. In doing that, I had to add pipes to delimit the button border.
> 3. Then for code reasons, it became difficult to avoid substitutions so I went
> back to using substitutions, but kept the pipes.
> 4. msw (in that above comment) suggested using substitutions directly to draw
> the box (as you are suggesting now). At that time I didn't change it.
> 5. We have had pipes in all such strings for the past 6 months.
> 
> I think having pipes is slightly more general as it allows us to migrate to
> having the key names directly in the translated strings (which both Evan and
the
> l10n team have told me is best practice). But it carries the burden of
> additional explanation for the translators. As I said, I'm not too fussed, but
I
> don't want to change it right now.

That's fine.  And thanks for the history.

I agree that embedding the key names directly is better from an l10n
perspective, so if we can resolve this by getting to that state that would be
fine.

[Matt Giuca, 2016-05-19 22:08:12]

https://codereview.chromium.org/1995893002/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1995893002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:12664: +        Press |<ph
name="ACCELERATOR1">$1<ex>Alt</ex></ph>|+|<ph
name="ACCELERATOR2">$2<ex>←</ex></ph>| to go back
On 2016/05/19 19:44:20, Peter Kasting wrote:
> On 2016/05/19 10:25:29, Matt Giuca wrote:
> > On 2016/05/19 09:47:27, Peter Kasting wrote:
> > > On 2016/05/19 09:19:00, Peter Kasting wrote:
> > > > Putting pipes around the accelerators doesn't seem good -- if we want to
> do
> > > > anything to delineate them, we should be drawing it entirely in the
code. 
> > > Pipes
> > > > will just look odd and make the localizers' job more confusing, I think.
> > > 
> > > Ah, now reading your other CL I understand why you did this.
> > > 
> > > Don't do things this way :).  Splitting the string on pipe chars is
> dangerous
> > if
> > > the localized string contains those characters.  It's also harder than
just
> > > getting the substitution locations directly: just call the variant of
> > > GetStringFUTF16() that returns a vector of offsets.  You can pass empty
> > strings
> > > for the substitutions if you don't want to sub in anything.
> > 
> > This is the way we've always done it and it's not a good time to change it
> now:
> > see IDS_FULLSCREEN_PRESS_ESC_TO_EXIT_FULLSCREEN in generated_resources.grd.
I
> > would have to edit all of those other strings and the existing code;
arguably
> I
> > should do that but given that we're trying to land these strings for branch
> > point, it isn't a good time to be reworking how the existing feature works.
> > 
> > The reason for doing it this way is a little shaky (I don't have a strong
> > opinion one way or another). Here is the original discussion:
> >
>
https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/exclusive...
> > 
> > 1. estade said not to use substitutions but directly embed the name of the
key
> > in the string so it can be localized better.
> > 2. In doing that, I had to add pipes to delimit the button border.
> > 3. Then for code reasons, it became difficult to avoid substitutions so I
went
> > back to using substitutions, but kept the pipes.
> > 4. msw (in that above comment) suggested using substitutions directly to
draw
> > the box (as you are suggesting now). At that time I didn't change it.
> > 5. We have had pipes in all such strings for the past 6 months.
> > 
> > I think having pipes is slightly more general as it allows us to migrate to
> > having the key names directly in the translated strings (which both Evan and
> the
> > l10n team have told me is best practice). But it carries the burden of
> > additional explanation for the translators. As I said, I'm not too fussed,
but
> I
> > don't want to change it right now.
> 
> That's fine.  And thanks for the history.
> 
> I agree that embedding the key names directly is better from an l10n
> perspective, so if we can resolve this by getting to that state that would be
> fine.

OK thanks. I've filed a discussion bug about this. It's still not clear to me
that we want to go down that path but at least there is a place we can have the
discussion:
https://crbug.com/613338

[Matt Giuca, 2016-05-19 22:08:16]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2016-05-19 22:09:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1995893002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1995893002/1

[Matt Giuca, 2016-05-19 22:32:55]

The CQ bit was unchecked by mgiuca@chromium.org

[Matt Giuca, 2016-05-19 22:33:25]

Oops, I forgot I promised to do something in #7 before landing.

[Matt Giuca, 2016-05-19 23:11:35]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2016-05-19 23:11:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1995893002/#ps20001
(title: "Move IDS_APP_ALT_KEY into the Key names section.")

[Matt Giuca, 2016-05-19 23:11:53]

https://codereview.chromium.org/1995893002/diff/1/ui/strings/ui_strings.grd
File ui/strings/ui_strings.grd (right):

https://codereview.chromium.org/1995893002/diff/1/ui/strings/ui_strings.grd#n...
ui/strings/ui_strings.grd:540: <!-- Shortcut Keys -->
On 2016/05/19 10:27:01, Matt Giuca wrote:
> On 2016/05/19 09:19:00, Peter Kasting wrote:
> > Nit: Why does this get its own heading?  Shouldn't it just be another one in
> the
> > "Key names" list above?
> 
> Oh yes, you're right. I don't have access to my git repo at the moment. Can
you
> leave me an LG and I'll fix this nit before submitting.

Done.

[commit-bot: I haz the power, 2016-05-19 23:12:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1995893002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1995893002/20001

[commit-bot: I haz the power, 2016-05-20 00:48:03]

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

[commit-bot: I haz the power, 2016-05-20 00:48:04]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peter Kasting, 2016-05-20 01:05:10]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2016-05-20 01:12:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1995893002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1995893002/20001

[Matt Giuca, 2016-05-20 02:45:43]

linux_chromium_rel_ng is failing interactive_ui_tests due to
https://crbug.com/613258
(WebViewContextMenuInteractiveTest.ContextMenuParamsAfterCSSTransforms is
flaky).

This is clearly nothing to do with my CL so I am going to force land this to
ensure it gets in before branch point. I have no idea why it's managed to fail 3
times with patch and pass 3 times without patch, but it is a known issue
unrelated to my CL.

[Matt Giuca, 2016-05-20 02:45:53]

Description was changed from

==========
Added strings for notification when user presses backspace.

These will be used in a future CL
(https://codereview.chromium.org/1983803002). The strings are being
checked in early to ensure they are in before branch point.

BUG=610039
==========

to

==========
Added strings for notification when user presses backspace.

These will be used in a future CL
(https://codereview.chromium.org/1983803002). The strings are being
checked in early to ensure they are in before branch point.

BUG=610039
NOTRY=true
==========

[Matt Giuca, 2016-05-20 02:45:59]

The CQ bit was unchecked by mgiuca@chromium.org

[Matt Giuca, 2016-05-20 02:46:36]

Description was changed from

==========
Added strings for notification when user presses backspace.

These will be used in a future CL
(https://codereview.chromium.org/1983803002). The strings are being
checked in early to ensure they are in before branch point.

BUG=610039
NOTRY=true
==========

to

==========
Added strings for notification when user presses backspace.

These will be used in a future CL
(https://codereview.chromium.org/1983803002). The strings are being
checked in early to ensure they are in before branch point.

Notry rationale: Tree is broken due to https://crbug.com/613258. All other tests
have passed on trybots.

BUG=610039
NOTRY=true
==========

[Matt Giuca, 2016-05-20 02:46:51]

Description was changed from

==========
Added strings for notification when user presses backspace.

These will be used in a future CL
(https://codereview.chromium.org/1983803002). The strings are being
checked in early to ensure they are in before branch point.

Notry rationale: Tree is broken due to https://crbug.com/613258. All other tests
have passed on trybots.

BUG=610039
NOTRY=true
==========

to

==========
Added strings for notification when user presses backspace.

These will be used in a future CL
(https://codereview.chromium.org/1983803002). The strings are being
checked in early to ensure they are in before branch point.

Notry rationale: Tree is broken due to https://crbug.com/613258. All
other tests have passed on trybots.

BUG=610039
NOTRY=true
==========

[Matt Giuca, 2016-05-20 02:46:57]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2016-05-20 02:47:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1995893002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1995893002/20001

[commit-bot: I haz the power, 2016-05-20 02:51:21]

Description was changed from

==========
Added strings for notification when user presses backspace.

These will be used in a future CL
(https://codereview.chromium.org/1983803002). The strings are being
checked in early to ensure they are in before branch point.

Notry rationale: Tree is broken due to https://crbug.com/613258. All
other tests have passed on trybots.

BUG=610039
NOTRY=true
==========

to

==========
Added strings for notification when user presses backspace.

These will be used in a future CL
(https://codereview.chromium.org/1983803002). The strings are being
checked in early to ensure they are in before branch point.

Notry rationale: Tree is broken due to https://crbug.com/613258. All
other tests have passed on trybots.

BUG=610039
NOTRY=true
==========

[commit-bot: I haz the power, 2016-05-20 02:51:22]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-20 02:52:28]

Description was changed from

==========
Added strings for notification when user presses backspace.

These will be used in a future CL
(https://codereview.chromium.org/1983803002). The strings are being
checked in early to ensure they are in before branch point.

Notry rationale: Tree is broken due to https://crbug.com/613258. All
other tests have passed on trybots.

BUG=610039
NOTRY=true
==========

to

==========
Added strings for notification when user presses backspace.

These will be used in a future CL
(https://codereview.chromium.org/1983803002). The strings are being
checked in early to ensure they are in before branch point.

Notry rationale: Tree is broken due to https://crbug.com/613258. All
other tests have passed on trybots.

BUG=610039
NOTRY=true

Committed: https://crrev.com/8386be414ec1537ad2c033889996f2419b330c42
Cr-Commit-Position: refs/heads/master@{#394968}
==========

[commit-bot: I haz the power, 2016-05-20 02:52:30]

Patchset 2 (id:??) landed as
https://crrev.com/8386be414ec1537ad2c033889996f2419b330c42
Cr-Commit-Position: refs/heads/master@{#394968}