Localized Chromoting Host on Mac and Linux.

Localized Chromoting Host on Mac and Linux.

This CL implements generation of localizable strings from remoting_strings.grd file. Depending on the platform the localized resources are placed to:
  - Mac: localized .string and .pak resources are added to each application bundle under 'Resources/<locale>.lproj'
  - Linux: localized .pak files are placed under 'remoting_locales' directory next to the binary locading them.
  - Windows: .rc resources are generated from .jinja2 templates and embedded into a relevant binary.

Chrome l10n and i18n APIs are used to retrieve the current locale and RTL flag (Mac & Linux). The it2me plugin sets the locale to match the locale of the browser.

Collateral changes:
  - UiString is not used any more.
  - Increased width of disconnect window message on Mac.
  - The host plugin version is correctly reported on Mac.
  - Dialogs use RTL templates in case of RTL languages. No more updating the templates dynamically (Windows).
  - remoting_unittests.ResourcesTest row runs on Mac, LInux and Windows.
  - '@' is used for variable substitutions by remoting_localize.py.
  - HOST_PLUGIN_MIME_TYPE is defined in one place now.
  - Deleted unused commong_resources.grd.

Mac installer and preference panel are not localized yet.

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

[alexeypa (please no reviews), 2013-07-23 21:42:04]

PTAL.

[Sergey Ulanov, 2013-07-23 22:19:47]

How is that supposed to work in It2Me? can we include .pak file in a webapp?

[Sergey Ulanov, 2013-07-23 22:45:39]

https://chromiumcodereview.appspot.com/19803010/diff/1/remoting/base/resource...
File remoting/base/resources_win.cc (right):

https://chromiumcodereview.appspot.com/19803010/diff/1/remoting/base/resource...
remoting/base/resources_win.cc:13: NOTREACHED();
Is that right? ResourceTest still calls this function. I think this function
should be either empty or shouldn't exist on Windows if it's not expected to be
used there.

https://chromiumcodereview.appspot.com/19803010/diff/1/remoting/host/host_mai...
File remoting/host/host_main.cc (right):

https://chromiumcodereview.appspot.com/19803010/diff/1/remoting/host/host_mai...
remoting/host/host_main.cc:238: #if !defined(OS_WIN) && !defined(OS_LINUX)
Can we always call LoadResources() and platform-specific implementation will do
what it needs to do?

https://chromiumcodereview.appspot.com/19803010/diff/1/remoting/host/plugin/h...
File remoting/host/plugin/host_plugin.cc (right):

https://chromiumcodereview.appspot.com/19803010/diff/1/remoting/host/plugin/h...
remoting/host/plugin/host_plugin.cc:378: #if !defined(OS_WIN)
how is g_ui_name supposed to be initialized on Windows?

https://chromiumcodereview.appspot.com/19803010/diff/1/remoting/host/plugin/h...
remoting/host/plugin/host_plugin.cc:462: *reinterpret_cast<const char**>(value)
= g_ui_name->c_str();
can we call l10n_util::GetStringUTF8() here instead of storing the name in
g_ui_name?

https://chromiumcodereview.appspot.com/19803010/diff/1/remoting/host/plugin/h...
remoting/host/plugin/host_plugin.cc:462: *reinterpret_cast<const char**>(value)
= g_ui_name->c_str();
Are you sure we need to localize this name and the description below? 
They are shown only in about:plugins, which is not user facing, so doesn't need
localization.
These strings do not seem to be localized for any other plugins (e.g. Flash, PDF
Viewer).

[alexeypa (please no reviews), 2013-07-23 23:25:56]

On 2013/07/23 22:19:47, Sergey Ulanov wrote:
> How is that supposed to work in It2Me? can we include .pak file in a webapp?

Had a chat with Jamie. He thinks it should not be a problem but I'll verify it
by uploading to the webstore a throw-away app.

[alexeypa (please no reviews), 2013-07-23 23:35:26]

I'll update the code shortly.

https://chromiumcodereview.appspot.com/19803010/diff/1/remoting/base/resource...
File remoting/base/resources_win.cc (right):

https://chromiumcodereview.appspot.com/19803010/diff/1/remoting/base/resource...
remoting/base/resources_win.cc:13: NOTREACHED();
On 2013/07/23 22:45:40, Sergey Ulanov wrote:
> Is that right? ResourceTest still calls this function. I think this function
> should be either empty or shouldn't exist on Windows if it's not expected to
be
> used there.

Yes, you are right I'll update this part.

https://chromiumcodereview.appspot.com/19803010/diff/1/remoting/host/host_mai...
File remoting/host/host_main.cc (right):

https://chromiumcodereview.appspot.com/19803010/diff/1/remoting/host/host_mai...
remoting/host/host_main.cc:238: #if !defined(OS_WIN) && !defined(OS_LINUX)
On 2013/07/23 22:45:40, Sergey Ulanov wrote:
> Can we always call LoadResources() and platform-specific implementation will
do
> what it needs to do?

Will do.

https://chromiumcodereview.appspot.com/19803010/diff/1/remoting/host/plugin/h...
File remoting/host/plugin/host_plugin.cc (right):

https://chromiumcodereview.appspot.com/19803010/diff/1/remoting/host/plugin/h...
remoting/host/plugin/host_plugin.cc:378: #if !defined(OS_WIN)
On 2013/07/23 22:45:40, Sergey Ulanov wrote:
> how is g_ui_name supposed to be initialized on Windows?

g_ui_name and g_ui_description are not used on Windows and Mac. It is not
obvious from the code, so I'll update it.

https://chromiumcodereview.appspot.com/19803010/diff/1/remoting/host/plugin/h...
remoting/host/plugin/host_plugin.cc:462: *reinterpret_cast<const char**>(value)
= g_ui_name->c_str();
On 2013/07/23 22:45:40, Sergey Ulanov wrote:
> Are you sure we need to localize this name and the description below? 

We can live without localizing them but once we got dialogs localized it is much
nicer to localize these as well. Also these strings are stored in a very
localizable way on Mac and Windows implying that they might localized as well.

> These strings do not seem to be localized for any other plugins (e.g. Flash,
PDF
> Viewer).

They are localized by some plugins, QuickTime for instance.

https://chromiumcodereview.appspot.com/19803010/diff/1/remoting/host/plugin/h...
remoting/host/plugin/host_plugin.cc:462: *reinterpret_cast<const char**>(value)
= g_ui_name->c_str();
On 2013/07/23 22:45:40, Sergey Ulanov wrote:
> can we call l10n_util::GetStringUTF8() here instead of storing the name in
> g_ui_name?

No, because the plugin still owns the string and it should be kept alive after
GetValue() returns.

[alexeypa (please no reviews), 2013-07-24 17:44:24]

I updated the code. PTAL.

[alexeypa (please no reviews), 2013-07-25 16:24:58]

ping

[Sergey Ulanov, 2013-07-25 18:40:57]

https://chromiumcodereview.appspot.com/19803010/diff/1/remoting/host/plugin/h...
File remoting/host/plugin/host_plugin.cc (right):

https://chromiumcodereview.appspot.com/19803010/diff/1/remoting/host/plugin/h...
remoting/host/plugin/host_plugin.cc:462: *reinterpret_cast<const char**>(value)
= g_ui_name->c_str();
On 2013/07/23 23:35:26, alexeypa wrote:
> On 2013/07/23 22:45:40, Sergey Ulanov wrote:
> > Are you sure we need to localize this name and the description below? 
> 
> We can live without localizing them but once we got dialogs localized it is
much
> nicer to localize these as well. Also these strings are stored in a very
> localizable way on Mac and Windows implying that they might localized as well.
> 
> > These strings do not seem to be localized for any other plugins (e.g. Flash,
> PDF
> > Viewer).
> 
> They are localized by some plugins, QuickTime for instance.

I still think that name and description shouldn't be localized. Localizing them
adds these ugly globals without giving anything in return. Yes, some plugins
have these strings localized, but I think it makes more sense to be consistent
with default plugins that ship with Chrome.

https://chromiumcodereview.appspot.com/19803010/diff/20001/remoting/base/reso...
File remoting/base/resources_win.cc (right):

https://chromiumcodereview.appspot.com/19803010/diff/20001/remoting/base/reso...
remoting/base/resources_win.cc:11: return false;
I think it would be more logical to return true from here - we can assume that
resources are loaded from the start, but returning false from here indicates
that they are not. 
In either case mac/linux implementation always return true, so I wonder if we
need to return anything here at all (i.e. maybe declare the function result as
void).

[alexeypa (please no reviews), 2013-07-25 18:48:02]

https://chromiumcodereview.appspot.com/19803010/diff/1/remoting/host/plugin/h...
File remoting/host/plugin/host_plugin.cc (right):

https://chromiumcodereview.appspot.com/19803010/diff/1/remoting/host/plugin/h...
remoting/host/plugin/host_plugin.cc:462: *reinterpret_cast<const char**>(value)
= g_ui_name->c_str();
On 2013/07/25 18:40:57, Sergey Ulanov wrote:
> I still think that name and description shouldn't be localized. Localizing
them
> adds these ugly globals without giving anything in return.

It gives something in return. It let's the plugin name and description to be
defined in one place (.grd) instead of having it defined in multiple places like
(branding files, .gyp and .cc).

https://chromiumcodereview.appspot.com/19803010/diff/20001/remoting/base/reso...
File remoting/base/resources_win.cc (right):

https://chromiumcodereview.appspot.com/19803010/diff/20001/remoting/base/reso...
remoting/base/resources_win.cc:11: return false;
On 2013/07/25 18:40:57, Sergey Ulanov wrote:
> I think it would be more logical to return true from here - we can assume that
> resources are loaded from the start, but returning false from here indicates
> that they are not. 

That is exactly why this return value is needed. If it is false, functions like
l10n_util::GetStringUTF8() may not be called. In other words, |false| means that
Chrome-style localization is not available.

[alexeypa (please no reviews), 2013-07-26 15:48:28]

Gary, ping^2

[garykac, 2013-07-26 16:20:15]

lgtm

https://codereview.chromium.org/19803010/diff/20001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://codereview.chromium.org/19803010/diff/20001/remoting/remoting.gyp#new...
remoting/remoting.gyp:50: # These values must match host/plugin/constants.h
The comment about these needing to match the values in constants.h can be
removed now.

https://codereview.chromium.org/19803010/diff/20001/remoting/tools/build/remo...
File remoting/tools/build/remoting_localize.py (right):

https://codereview.chromium.org/19803010/diff/20001/remoting/tools/build/remo...
remoting/tools/build/remoting_localize.py:646: # expansion when passing the
string frmo GYP.
from

[Sergey Ulanov, 2013-07-26 17:58:24]

lgtm

https://chromiumcodereview.appspot.com/19803010/diff/1/remoting/host/plugin/h...
File remoting/host/plugin/host_plugin.cc (right):

https://chromiumcodereview.appspot.com/19803010/diff/1/remoting/host/plugin/h...
remoting/host/plugin/host_plugin.cc:462: *reinterpret_cast<const char**>(value)
= g_ui_name->c_str();
On 2013/07/25 18:48:03, alexeypa wrote:
> On 2013/07/25 18:40:57, Sergey Ulanov wrote:
> > I still think that name and description shouldn't be localized. Localizing
> them
> > adds these ugly globals without giving anything in return.
> 
> It gives something in return. It let's the plugin name and description to be
> defined in one place (.grd) instead of having it defined in multiple places
like
> (branding files, .gyp and .cc). 

Ok, makes sense. Maybe add translateable="false" for these strings in GRD file?

https://chromiumcodereview.appspot.com/19803010/diff/20001/remoting/base/reso...
File remoting/base/resources_win.cc (right):

https://chromiumcodereview.appspot.com/19803010/diff/20001/remoting/base/reso...
remoting/base/resources_win.cc:11: return false;
On 2013/07/25 18:48:03, alexeypa wrote:
> On 2013/07/25 18:40:57, Sergey Ulanov wrote:
> > I think it would be more logical to return true from here - we can assume
that
> > resources are loaded from the start, but returning false from here indicates
> > that they are not. 
> 
> That is exactly why this return value is needed. If it is false, functions
like
> l10n_util::GetStringUTF8() may not be called. In other words, |false| means
that
> Chrome-style localization is not available.

I see. Sorry, I mistakenly assumed that l10n_util will still work (pulling
strings from .rc). If it doesn't then it will break cross-platform code that
needs localized strings - It's probably not a problem right now, but may become
one in the future. Will it be possible to avoid ifdefs in that case?

[alexeypa (please no reviews), 2013-07-26 18:55:01]

https://chromiumcodereview.appspot.com/19803010/diff/1/remoting/host/plugin/h...
File remoting/host/plugin/host_plugin.cc (right):

https://chromiumcodereview.appspot.com/19803010/diff/1/remoting/host/plugin/h...
remoting/host/plugin/host_plugin.cc:462: *reinterpret_cast<const char**>(value)
= g_ui_name->c_str();
On 2013/07/26 17:58:25, Sergey Ulanov wrote:
> Ok, makes sense. Maybe add translateable="false" for these strings in GRD
file?

Why? They are already translated and the translations are nicely shown in the UI
(even if it just about:plugins). It is better to have a translated string if the
user prefers a translated string.

https://chromiumcodereview.appspot.com/19803010/diff/20001/remoting/base/reso...
File remoting/base/resources_win.cc (right):

https://chromiumcodereview.appspot.com/19803010/diff/20001/remoting/base/reso...
remoting/base/resources_win.cc:11: return false;
On 2013/07/26 17:58:25, Sergey Ulanov wrote:
> If it doesn't then it will break cross-platform code that
> needs localized strings - It's probably not a problem right now, but may
become
> one in the future. Will it be possible to avoid ifdefs in that case?

This is the price for not having dependency on icudt.dll. Now there is a good
chance that we may need to add this dependency because of changes in chromium
code outside of remoting. If this ever happens that plan is to add icudt.dll and
.pak files to the installation and make LoadResources to initialize the resourse
bundle just like Linux does.

https://chromiumcodereview.appspot.com/19803010/diff/20001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://chromiumcodereview.appspot.com/19803010/diff/20001/remoting/remoting....
remoting/remoting.gyp:50: # These values must match host/plugin/constants.h
On 2013/07/26 16:20:15, garykac wrote:
> The comment about these needing to match the values in constants.h can be
> removed now.

Done.

https://chromiumcodereview.appspot.com/19803010/diff/20001/remoting/tools/bui...
File remoting/tools/build/remoting_localize.py (right):

https://chromiumcodereview.appspot.com/19803010/diff/20001/remoting/tools/bui...
remoting/tools/build/remoting_localize.py:646: # expansion when passing the
string frmo GYP.
On 2013/07/26 16:20:15, garykac wrote:
> from

Done.

[commit-bot: I haz the power, 2013-07-26 20:48:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/19803010/56001

[commit-bot: I haz the power, 2013-07-26 21:27:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/19803010/56001

[commit-bot: I haz the power, 2013-07-26 22:55:08]

Change committed as 213997