Don't replace $1 with an empty string when localizing string for host plugin.

Don't replace $1 with an empty string when localizing string for host plugin.

Previously chrome.l18n.getString() wouldn't touch $1 within the resource
when substitutions are not specified. This was fixed in crrev.com/141280, and
now it replaced $1 with an empty strings, which broke chromoting host 
localization that relied on the previous incorrect behaviour. Fixed
the webapp to correctly handle strings substitutions.

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

[Jamie, 2012-06-12 03:00:00]

Are we in any rush to release this? If not, then I think we'd be better off
fixing it in the plug-in. Firstly, it shouldn't crash just because a "$1" is
missing. Secondly, it would be nice to be able to make use of getMessage's
substitution feature if we ever need it, which we can easily do by passing the
default (no-op) substitutions from the plugin, rather than hard-coding them at
the JS level.

On the other hand, if we do need to push this quickly, then this is fine a quick
hack.

[Sergey Ulanov, 2012-06-12 04:01:20]

On 2012/06/12 03:00:00, Jamie wrote:
> Are we in any rush to release this? If not, then I think we'd be better off
> fixing it in the plug-in. Firstly, it shouldn't crash just because a "$1" is
> missing.

The plugin doesn't crash, it DCHECKs, and the DCHECK is useful - it helped to
detect an actual bug.

> Secondly, it would be nice to be able to make use of getMessage's
> substitution feature if we ever need it, which we can easily do by passing the
> default (no-op) substitutions from the plugin, rather than hard-coding them at
> the JS level.

I'm not sure that this would be the right thing to do. The localization API for
chrome extensions is designed to be used from JavaScript. We use it for
something it wasn't really designed for - localizing NPAPI plugins. I don't
think that calling JavaScript to get localized string would give us much or make
the code simpler in any way.  

> On the other hand, if we do need to push this quickly, then this is fine a
quick
> hack.

It's not necessary to push this fix quickly, but it might be better to release
it before M21 goes to stable, otherwise It2Me UI will be broken.

[Jamie, 2012-06-12 20:50:38]

On 2012/06/12 04:01:20, sergeyu wrote:
> On 2012/06/12 03:00:00, Jamie wrote:
> > Are we in any rush to release this? If not, then I think we'd be better off
> > fixing it in the plug-in. Firstly, it shouldn't crash just because a "$1" is
> > missing.
> 
> The plugin doesn't crash, it DCHECKs, and the DCHECK is useful - it helped to
> detect an actual bug.

Fair enough. I didn't know what the specifics were.

> 
> > Secondly, it would be nice to be able to make use of getMessage's
> > substitution feature if we ever need it, which we can easily do by passing
the
> > default (no-op) substitutions from the plugin, rather than hard-coding them
at
> > the JS level.
> 
> I'm not sure that this would be the right thing to do. The localization API
for
> chrome extensions is designed to be used from JavaScript. We use it for
> something it wasn't really designed for - localizing NPAPI plugins. I don't
> think that calling JavaScript to get localized string would give us much or
make
> the code simpler in any way.

I disagree. NPAPI provides a bridge between native code and JS, so calling the
getMessage API in this way is fine, and if we're doing that, I'd prefer that it
has the semantics it would normally have. 

Think of it this way: the only reason we rely on the previous behaviour (whereby
"$1" is not substituted) is because we don't have the correct substitution to
hand when we're doing the l10n and we don't want to have to do the thread
redirect dance later on when we do have it. That's an implementation detail of
the plugin, and shouldn't affect the design of the Javascript code.

You're right that it's going to take more code to pass "$1" explicitly for this
string, but I think it will be clearer--we could even substitute a more
sensibly-named placeholder for "$1".

[Sergey Ulanov, 2012-06-12 23:48:57]

On 2012/06/12 20:50:38, Jamie wrote:
> On 2012/06/12 04:01:20, sergeyu wrote:
> > On 2012/06/12 03:00:00, Jamie wrote:
> > > Are we in any rush to release this? If not, then I think we'd be better
off
> > > fixing it in the plug-in. Firstly, it shouldn't crash just because a "$1"
is
> > > missing.
> > 
> > The plugin doesn't crash, it DCHECKs, and the DCHECK is useful - it helped
to
> > detect an actual bug.
> 
> Fair enough. I didn't know what the specifics were.
> 
> > 
> > > Secondly, it would be nice to be able to make use of getMessage's
> > > substitution feature if we ever need it, which we can easily do by passing
> the
> > > default (no-op) substitutions from the plugin, rather than hard-coding
them
> at
> > > the JS level.
> > 
> > I'm not sure that this would be the right thing to do. The localization API
> for
> > chrome extensions is designed to be used from JavaScript. We use it for
> > something it wasn't really designed for - localizing NPAPI plugins. I don't
> > think that calling JavaScript to get localized string would give us much or
> make
> > the code simpler in any way.
> 
> I disagree. NPAPI provides a bridge between native code and JS, so calling the
> getMessage API in this way is fine, and if we're doing that, I'd prefer that
it
> has the semantics it would normally have. 
> 
> Think of it this way: the only reason we rely on the previous behaviour
(whereby
> "$1" is not substituted) is because we don't have the correct substitution to
> hand when we're doing the l10n and we don't want to have to do the thread
> redirect dance later on when we do have it. That's an implementation detail of
> the plugin, and shouldn't affect the design of the Javascript code.
> 
> You're right that it's going to take more code to pass "$1" explicitly for
this
> string, but I think it will be clearer--we could even substitute a more
> sensibly-named placeholder for "$1".

Yes, calling JavaScript from NPAPI every time to get localized strings might be
cleaner, if we were writing this code from scratch, but I don't think it's
really something that needs to be fixed now. Can we land this change as is to
fix Debug builds? We can always refactor this code later if it turns necessary.

[Jamie, 2012-06-12 23:51:32]

On 2012/06/12 23:48:57, sergeyu wrote:
> On 2012/06/12 20:50:38, Jamie wrote:
> > On 2012/06/12 04:01:20, sergeyu wrote:
> > > On 2012/06/12 03:00:00, Jamie wrote:
> > > > Are we in any rush to release this? If not, then I think we'd be better
> off
> > > > fixing it in the plug-in. Firstly, it shouldn't crash just because a
"$1"
> is
> > > > missing.
> > > 
> > > The plugin doesn't crash, it DCHECKs, and the DCHECK is useful - it helped
> to
> > > detect an actual bug.
> > 
> > Fair enough. I didn't know what the specifics were.
> > 
> > > 
> > > > Secondly, it would be nice to be able to make use of getMessage's
> > > > substitution feature if we ever need it, which we can easily do by
passing
> > the
> > > > default (no-op) substitutions from the plugin, rather than hard-coding
> them
> > at
> > > > the JS level.
> > > 
> > > I'm not sure that this would be the right thing to do. The localization
API
> > for
> > > chrome extensions is designed to be used from JavaScript. We use it for
> > > something it wasn't really designed for - localizing NPAPI plugins. I
don't
> > > think that calling JavaScript to get localized string would give us much
or
> > make
> > > the code simpler in any way.
> > 
> > I disagree. NPAPI provides a bridge between native code and JS, so calling
the
> > getMessage API in this way is fine, and if we're doing that, I'd prefer that
> it
> > has the semantics it would normally have. 
> > 
> > Think of it this way: the only reason we rely on the previous behaviour
> (whereby
> > "$1" is not substituted) is because we don't have the correct substitution
to
> > hand when we're doing the l10n and we don't want to have to do the thread
> > redirect dance later on when we do have it. That's an implementation detail
of
> > the plugin, and shouldn't affect the design of the Javascript code.
> > 
> > You're right that it's going to take more code to pass "$1" explicitly for
> this
> > string, but I think it will be clearer--we could even substitute a more
> > sensibly-named placeholder for "$1".
> 
> Yes, calling JavaScript from NPAPI every time to get localized strings might
be
> cleaner, if we were writing this code from scratch, but I don't think it's
> really something that needs to be fixed now. Can we land this change as is to
> fix Debug builds? We can always refactor this code later if it turns
necessary.

Okay, since it's blocking testing, lgtm (but please run it through jscompiler if
you haven't done so already).

[Sergey Ulanov, 2012-06-13 00:14:46]

> Okay, since it's blocking testing, lgtm (but please run it through jscompiler
if
> you haven't done so already).

Done. Also opened crbug.com/132370

[commit-bot: I haz the power, 2012-06-13 00:15:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/10533101/6002