[Mac] Localize app shims with the OS preferred language.

[Mac] Localize app shims with the OS preferred language.

Previously this created an english localization folder and assumed OSX
would use it since it's the only one available. However, when english is
not one of the preferred languages, the metadata for the shim bundle
would report its filename as the display name. The metadata is used by
Spotlight, so the user would be unable to search for the app.
See the bug for more details.

With this CL, we still only create one localization folder, but use the
most preferred language reported by the OS.

This also deletes any existing shims before creating new ones.

BUG=482658
Committed: https://crrev.com/546101a5f05e8356d5e536bb17e12f182f393c4f
Cr-Commit-Position: refs/heads/master@{#330038}

[jackhou1, 2015-05-14 06:54:49]

jackhou@chromium.org changed reviewers:
+ tapted@chromium.org

[jackhou1, 2015-05-14 06:54:50]

tapted, could you take a look?

[tapted, 2015-05-14 07:53:14]

https://codereview.chromium.org/1137373003/diff/1/chrome/browser/web_applicat...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/1137373003/diff/1/chrome/browser/web_applicat...
chrome/browser/web_applications/web_app_mac.mm:904: NSString* language =
[[NSLocale preferredLanguages] objectAtIndex:0];
should it use
l10n_util::NormalizeLocale(l10n_util::GetApplicationLocale(std::string())); ?
This is what UpdateAppShortcutsSubdirLocalizedName is doing.

i.e. the first preferredLanguage might not be supported by Chrome, and so a
different localization might be getting used for info_->title, and we should aim
to ensure that these match.

(Also are there any implications for this bug for
UpdateAppShortcutsSubdirLocalizedName()?)

And, I'm _pretty_ sure we don't need to worry about UpdateDisplayName being
called for an app that already has a locale folder due to an upgrade (i.e.
because the folder is deleted recursively first) - but that might be something
to check.

Also is there a plan to do a batch upgrade, e.g. a once-off thing if "en"
doesn't appear in [NSLocale preferredLanguages]?

[jackhou1, 2015-05-15 00:41:08]

https://codereview.chromium.org/1137373003/diff/1/chrome/browser/web_applicat...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/1137373003/diff/1/chrome/browser/web_applicat...
chrome/browser/web_applications/web_app_mac.mm:904: NSString* language =
[[NSLocale preferredLanguages] objectAtIndex:0];
On 2015/05/14 07:53:14, tapted wrote:
> should it use
> l10n_util::NormalizeLocale(l10n_util::GetApplicationLocale(std::string())); ?
> This is what UpdateAppShortcutsSubdirLocalizedName is doing.
> 
> i.e. the first preferredLanguage might not be supported by Chrome, and so a
> different localization might be getting used for info_->title, and we should
aim
> to ensure that these match.
> 
> (Also are there any implications for this bug for
> UpdateAppShortcutsSubdirLocalizedName()?)

With the app shim, we don't actually care what language the folder says, we
always write strings for the best available language. I.e. the shim pretends to
support the OS preferred language, but always displays Chrome's language. For
the most part they are the same. If Chrome defaults to "en" because none of the
OS preferred languages are supported, we want the shim to show the english app
name and not "Default <extension-id>".

For the Chrome Apps folder, it makes more sense for the localization folder to
actually match the language of the strings. If Chrome's language changes, we end
up writing multiple localization folders, and they'll all be correct. If none of
them are in the OS preferred languages, the display name defaults to "Chrome
Apps", which is fine.

> And, I'm _pretty_ sure we don't need to worry about UpdateDisplayName being
> called for an app that already has a locale folder due to an upgrade (i.e.
> because the folder is deleted recursively first) - but that might be something
> to check.

That's right. But creating shortcuts while one exists will merge the
directories. I don't think it matters much, it'll either overwrite the
InfoPlist.strings or there'll be two localization folders.

> Also is there a plan to do a batch upgrade, e.g. a once-off thing if "en"
> doesn't appear in [NSLocale preferredLanguages]?

If we want to batch upgrade, I think it's better to use the upgrade-all
mechanism. I'd prefer not to add code for once-off behavior.

[tapted, 2015-05-15 00:57:29]

lgtm if the stuff below sounds right.

On 2015/05/15 00:41:08, jackhou1 wrote:
>
https://codereview.chromium.org/1137373003/diff/1/chrome/browser/web_applicat...
> File chrome/browser/web_applications/web_app_mac.mm (right):
> 
>
https://codereview.chromium.org/1137373003/diff/1/chrome/browser/web_applicat...
> chrome/browser/web_applications/web_app_mac.mm:904: NSString* language =
> [[NSLocale preferredLanguages] objectAtIndex:0];
> On 2015/05/14 07:53:14, tapted wrote:
> > should it use
> > l10n_util::NormalizeLocale(l10n_util::GetApplicationLocale(std::string()));
?
> > This is what UpdateAppShortcutsSubdirLocalizedName is doing.
> > 
> > i.e. the first preferredLanguage might not be supported by Chrome, and so a
> > different localization might be getting used for info_->title, and we should
> aim
> > to ensure that these match.
> > 
> > (Also are there any implications for this bug for
> > UpdateAppShortcutsSubdirLocalizedName()?)
> 
> With the app shim, we don't actually care what language the folder says, we
> always write strings for the best available language. I.e. the shim pretends
to
> support the OS preferred language, but always displays Chrome's language. For
> the most part they are the same. If Chrome defaults to "en" because none of
the
> OS preferred languages are supported, we want the shim to show the english app
> name and not "Default <extension-id>".

I guess my concern here would be if the user had some obscure language as their
first preference which they later removed. Or if there was some odd suffix on
the locale string (like _GB). Picking a language Chrome supports might have more
longevity, but I suppose it's pretty corner-casey.

> For the Chrome Apps folder, it makes more sense for the localization folder to
> actually match the language of the strings. If Chrome's language changes, we
end
> up writing multiple localization folders, and they'll all be correct. If none
of
> them are in the OS preferred languages, the display name defaults to "Chrome
> Apps", which is fine.
> 
> > And, I'm _pretty_ sure we don't need to worry about UpdateDisplayName being
> > called for an app that already has a locale folder due to an upgrade (i.e.
> > because the folder is deleted recursively first) - but that might be
something
> > to check.
> 
> That's right. But creating shortcuts while one exists will merge the
> directories.

Is this true? I glanced at the code and got the impression it always first did
`rm -r`. I think it would be bad to have two localization folders, since only
one of them will have an updated app name For example, if the name of the app
changes in the webstore, or gets localized to the users language we wouldn't
want the old name lying around.

> I don't think it matters much, it'll either overwrite the
> InfoPlist.strings or there'll be two localization folders.
> 
> > Also is there a plan to do a batch upgrade, e.g. a once-off thing if "en"
> > doesn't appear in [NSLocale preferredLanguages]?
> 
> If we want to batch upgrade, I think it's better to use the upgrade-all
> mechanism. I'd prefer not to add code for once-off behavior.

sg. I guess we want it soon for bookmark apps anyway.

[jackhou1, 2015-05-15 01:46:45]

On 2015/05/15 00:57:29, tapted wrote:
> lgtm if the stuff below sounds right.
> 
> On 2015/05/15 00:41:08, jackhou1 wrote:
> >
>
https://codereview.chromium.org/1137373003/diff/1/chrome/browser/web_applicat...
> > File chrome/browser/web_applications/web_app_mac.mm (right):
> > 
> >
>
https://codereview.chromium.org/1137373003/diff/1/chrome/browser/web_applicat...
> > chrome/browser/web_applications/web_app_mac.mm:904: NSString* language =
> > [[NSLocale preferredLanguages] objectAtIndex:0];
> > On 2015/05/14 07:53:14, tapted wrote:
> > > should it use
> > >
l10n_util::NormalizeLocale(l10n_util::GetApplicationLocale(std::string()));
> ?
> > > This is what UpdateAppShortcutsSubdirLocalizedName is doing.
> > > 
> > > i.e. the first preferredLanguage might not be supported by Chrome, and so
a
> > > different localization might be getting used for info_->title, and we
should
> > aim
> > > to ensure that these match.
> > > 
> > > (Also are there any implications for this bug for
> > > UpdateAppShortcutsSubdirLocalizedName()?)
> > 
> > With the app shim, we don't actually care what language the folder says, we
> > always write strings for the best available language. I.e. the shim pretends
> to
> > support the OS preferred language, but always displays Chrome's language.
For
> > the most part they are the same. If Chrome defaults to "en" because none of
> the
> > OS preferred languages are supported, we want the shim to show the english
app
> > name and not "Default <extension-id>".
> 
> I guess my concern here would be if the user had some obscure language as
their
> first preference which they later removed. Or if there was some odd suffix on
> the locale string (like _GB). Picking a language Chrome supports might have
more
> longevity, but I suppose it's pretty corner-casey.

Either way if the language is removed from preferred languages, it'll break. My
guess is that most people don't change their first preference, and if they do,
they keep their old language as second preference.

> > For the Chrome Apps folder, it makes more sense for the localization folder
to
> > actually match the language of the strings. If Chrome's language changes, we
> end
> > up writing multiple localization folders, and they'll all be correct. If
none
> of
> > them are in the OS preferred languages, the display name defaults to "Chrome
> > Apps", which is fine.
> > 
> > > And, I'm _pretty_ sure we don't need to worry about UpdateDisplayName
being
> > > called for an app that already has a locale folder due to an upgrade (i.e.
> > > because the folder is deleted recursively first) - but that might be
> something
> > > to check.
> > 
> > That's right. But creating shortcuts while one exists will merge the
> > directories.
> 
> Is this true? I glanced at the code and got the impression it always first did
> `rm -r`. I think it would be bad to have two localization folders, since only
> one of them will have an updated app name For example, if the name of the app
> changes in the webstore, or gets localized to the users language we wouldn't
> want the old name lying around.

UpdateShortcuts() deletes them first, so updated names should be fine.
CreateShortcuts() doesn't, and is only triggered by the user creating shortcuts,
but it wouldn't hurt to delete them anyway. PTAL at Patch Set 2.

> > I don't think it matters much, it'll either overwrite the
> > InfoPlist.strings or there'll be two localization folders.
> > 
> > > Also is there a plan to do a batch upgrade, e.g. a once-off thing if "en"
> > > doesn't appear in [NSLocale preferredLanguages]?
> > 
> > If we want to batch upgrade, I think it's better to use the upgrade-all
> > mechanism. I'd prefer not to add code for once-off behavior.
> 
> sg. I guess we want it soon for bookmark apps anyway.

[tapted, 2015-05-15 02:02:49]

https://codereview.chromium.org/1137373003/diff/20001/chrome/browser/web_appl...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/1137373003/diff/20001/chrome/browser/web_appl...
chrome/browser/web_applications/web_app_mac.mm:703: if
(!base::CopyDirectory(staging_path, dst_path, true)) {
What about before this, something like

  // Ensure the copy does not merge with stale info.
  base::DeleteFile(dst_path.Append(app_name));

[jackhou1, 2015-05-15 02:44:57]

https://codereview.chromium.org/1137373003/diff/20001/chrome/browser/web_appl...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/1137373003/diff/20001/chrome/browser/web_appl...
chrome/browser/web_applications/web_app_mac.mm:703: if
(!base::CopyDirectory(staging_path, dst_path, true)) {
On 2015/05/15 02:02:49, tapted wrote:
> What about before this, something like
> 
>   // Ensure the copy does not merge with stale info.
>   base::DeleteFile(dst_path.Append(app_name));

Done.

[tapted, 2015-05-15 03:22:15]

lgtm

[jackhou1, 2015-05-15 03:27:37]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2015-05-15 03:28:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1137373003/40001

[commit-bot: I haz the power, 2015-05-15 04:12:38]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-15 04:13:27]

Patchset 3 (id:??) landed as
https://crrev.com/546101a5f05e8356d5e536bb17e12f182f393c4f
Cr-Commit-Position: refs/heads/master@{#330038}