[Web Intents] Localization of intent titles for CWS suggestions

[Web Intents] Localization of intent titles for CWS suggestions


R=mpcomplete@chromium.org,gbillock@chromium.org
BUG=130432
TEST=CWSIntentsRegistryTest.LocalizeMatchingLocale:ExtensionL10nUtil.GetAllFallbackLocales:ExtensionL10nUtil.LoadMessageCatalogs*


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

[Matt Perry, 2012-07-20 00:10:34]

extensions/* LGTM. (I'm not familiar with the CWS code)

[groby-ooo-7-16, 2012-07-20 00:12:05]

mpcomplete: Sorry, forgot to mention I needed you just for OWNERS on extension
:)

[Greg Billock, 2012-07-20 15:58:30]

http://codereview.chromium.org/10790087/diff/1/chrome/browser/intents/cws_int...
File chrome/browser/intents/cws_intents_registry.cc (right):

http://codereview.chromium.org/10790087/diff/1/chrome/browser/intents/cws_int...
chrome/browser/intents/cws_intents_registry.cc:107: 
Can you add a comment about why we need this block, and what it would take to
get rid of it?

http://codereview.chromium.org/10790087/diff/1/chrome/browser/intents/cws_int...
chrome/browser/intents/cws_intents_registry.cc:119: !locale->GetString("title",
&title))
So this isn't the normal place, so the extension l10n utils won't work?

http://codereview.chromium.org/10790087/diff/1/chrome/browser/intents/cws_int...
chrome/browser/intents/cws_intents_registry.cc:125: std::vector<std::string>
valid_locales;
Can you just use vector<string> here and below? The namespaces are a bit
noisifying.

http://codereview.chromium.org/10790087/diff/1/chrome/browser/intents/cws_int...
chrome/browser/intents/cws_intents_registry.cc:127:
extension_l10n_util::CurrentLocaleOrDefault(),
Shouldn't the util class compose these default/current/"all" ordering and just
give you back the vector?

http://codereview.chromium.org/10790087/diff/1/chrome/common/extensions/exten...
File chrome/common/extensions/extension_l10n_util.cc (right):

http://codereview.chromium.org/10790087/diff/1/chrome/common/extensions/exten...
chrome/common/extensions/extension_l10n_util.cc:245:
l10n_util::GetParentLocales(application_locale, all_fallback_locales);
Should GetParentLocales not append "all" already? I'd think "all" would always
be in the parent hierarchy, so if it is there, it'd be in the list in the right
spot.

[groby-ooo-7-16, 2012-07-20 22:59:54]

http://codereview.chromium.org/10790087/diff/1/chrome/browser/intents/cws_int...
File chrome/browser/intents/cws_intents_registry.cc (right):

http://codereview.chromium.org/10790087/diff/1/chrome/browser/intents/cws_int...
chrome/browser/intents/cws_intents_registry.cc:107: 
On 2012/07/20 15:58:30, Greg Billock wrote:
> Can you add a comment about why we need this block, and what it would take to
> get rid of it?

Done.

http://codereview.chromium.org/10790087/diff/1/chrome/browser/intents/cws_int...
chrome/browser/intents/cws_intents_registry.cc:119: !locale->GetString("title",
&title))
On 2012/07/20 15:58:30, Greg Billock wrote:
> So this isn't the normal place, so the extension l10n utils won't work?

That is correct. There's also the point that the CWS reply always calls it
"title", but the manifest might expect other identifiers. (l10n for manifests is
pretty flexible)

http://codereview.chromium.org/10790087/diff/1/chrome/browser/intents/cws_int...
chrome/browser/intents/cws_intents_registry.cc:125: std::vector<std::string>
valid_locales;
As far as I can tell, we always prefix. 
On 2012/07/20 15:58:30, Greg Billock wrote:
> Can you just use vector<string> here and below? The namespaces are a bit
> noisifying.

http://codereview.chromium.org/10790087/diff/1/chrome/browser/intents/cws_int...
chrome/browser/intents/cws_intents_registry.cc:127:
extension_l10n_util::CurrentLocaleOrDefault(),
No, because the util class is shared with extension l10n, and that supports
different defaults and application locales.
On 2012/07/20 15:58:30, Greg Billock wrote:
> Shouldn't the util class compose these default/current/"all" ordering and just
> give you back the vector?

http://codereview.chromium.org/10790087/diff/1/chrome/common/extensions/exten...
File chrome/common/extensions/extension_l10n_util.cc (right):

http://codereview.chromium.org/10790087/diff/1/chrome/common/extensions/exten...
chrome/common/extensions/extension_l10n_util.cc:245:
l10n_util::GetParentLocales(application_locale, all_fallback_locales);
No - see use in LoadMessageCatalogs.

On 2012/07/20 15:58:30, Greg Billock wrote:
> Should GetParentLocales not append "all" already? I'd think "all" would always
> be in the parent hierarchy, so if it is there, it'd be in the list in the
right
> spot.

[Greg Billock, 2012-07-20 23:08:17]

On 2012/07/20 22:59:54, groby wrote:
>
http://codereview.chromium.org/10790087/diff/1/chrome/browser/intents/cws_int...
> File chrome/browser/intents/cws_intents_registry.cc (right):
> 
>
http://codereview.chromium.org/10790087/diff/1/chrome/browser/intents/cws_int...
> chrome/browser/intents/cws_intents_registry.cc:107: 
> On 2012/07/20 15:58:30, Greg Billock wrote:
> > Can you add a comment about why we need this block, and what it would take
to
> > get rid of it?
> 
> Done.
> 
>
http://codereview.chromium.org/10790087/diff/1/chrome/browser/intents/cws_int...
> chrome/browser/intents/cws_intents_registry.cc:119:
!locale->GetString("title",
> &title))
> On 2012/07/20 15:58:30, Greg Billock wrote:
> > So this isn't the normal place, so the extension l10n utils won't work?
> 
> That is correct. There's also the point that the CWS reply always calls it
> "title", but the manifest might expect other identifiers. (l10n for manifests
is
> pretty flexible)
> 
>
http://codereview.chromium.org/10790087/diff/1/chrome/browser/intents/cws_int...
> chrome/browser/intents/cws_intents_registry.cc:125: std::vector<std::string>
> valid_locales;
> As far as I can tell, we always prefix. 
> On 2012/07/20 15:58:30, Greg Billock wrote:
> > Can you just use vector<string> here and below? The namespaces are a bit
> > noisifying.
> 
>
http://codereview.chromium.org/10790087/diff/1/chrome/browser/intents/cws_int...
> chrome/browser/intents/cws_intents_registry.cc:127:
> extension_l10n_util::CurrentLocaleOrDefault(),
> No, because the util class is shared with extension l10n, and that supports
> different defaults and application locales.
> On 2012/07/20 15:58:30, Greg Billock wrote:
> > Shouldn't the util class compose these default/current/"all" ordering and
just
> > give you back the vector?
> 
>
http://codereview.chromium.org/10790087/diff/1/chrome/common/extensions/exten...
> File chrome/common/extensions/extension_l10n_util.cc (right):
> 
>
http://codereview.chromium.org/10790087/diff/1/chrome/common/extensions/exten...
> chrome/common/extensions/extension_l10n_util.cc:245:
> l10n_util::GetParentLocales(application_locale, all_fallback_locales);
> No - see use in LoadMessageCatalogs.
> 
> On 2012/07/20 15:58:30, Greg Billock wrote:
> > Should GetParentLocales not append "all" already? I'd think "all" would
always
> > be in the parent hierarchy, so if it is there, it'd be in the list in the
> right
> > spot.

lgtm

[commit-bot: I haz the power, 2012-07-20 23:38:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/10790087/8001

[commit-bot: I haz the power, 2012-07-21 00:24:54]

Try job failure for 10790087-8001 (retry) on linux_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[groby-ooo-7-16, 2012-07-26 17:31:00]

FYI: Made sure locale is restored at end of test. We have some tests that rely
on the locale being set to en. (Will submit separate CL to fix that)

[Greg Billock, 2012-07-26 22:48:48]

On 2012/07/26 17:31:00, groby wrote:
> FYI: Made sure locale is restored at end of test. We have some tests that rely
> on the locale being set to en. (Will submit separate CL to fix that)

lgtm

[commit-bot: I haz the power, 2012-07-26 22:50:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/10790087/16001

[commit-bot: I haz the power, 2012-07-26 23:09:16]

Try job failure for 10790087-16001 (retry) on linux_chromeos for step "update"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-07-27 02:54:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/10790087/25004

[commit-bot: I haz the power, 2012-07-27 04:23:20]

Change committed as 148720