Set OEM apps folder name from customization manifest

Set OEM apps folder name from customization manifest

BUG=351567
TEST=unit_tests
R=nkostylev@chromium.org, stevenjb@chromium.org

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

[stevenjb, 2014-03-20 22:12:56]

https://codereview.chromium.org/206673006/diff/1/chrome/browser/chromeos/cust...
File chrome/browser/chromeos/customization_document.cc (right):

https://codereview.chromium.org/206673006/diff/1/chrome/browser/chromeos/cust...
chrome/browser/chromeos/customization_document.cc:188: std::string
CustomizationDocument::GetLocaleSpecificStringImpl(
Is there a reason for this to be a static method instead of just file local?

https://codereview.chromium.org/206673006/diff/1/chrome/browser/chromeos/cust...
File chrome/browser/chromeos/customization_document_unittest.cc (right):

https://codereview.chromium.org/206673006/diff/1/chrome/browser/chromeos/cust...
chrome/browser/chromeos/customization_document_unittest.cc:335:
EXPECT_EQ(doc->GetOemFolderName("ru"), "The OEM Name");
Maybe add a comment that we expect this to return the default name?
Also maybe add 'en' also so that we're testing correct lookup of more than the
first and default items.

https://codereview.chromium.org/206673006/diff/1/chrome/browser/ui/app_list/a...
File chrome/browser/ui/app_list/app_list_syncable_service.h (right):

https://codereview.chromium.org/206673006/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_syncable_service.h:83: std::string
GetOemFolderNameForTest() { return oem_folder_name_; }
nit: return const&

[Dmitry Polukhin, 2014-03-20 22:41:10]

https://codereview.chromium.org/206673006/diff/1/chrome/browser/chromeos/cust...
File chrome/browser/chromeos/customization_document.cc (right):

https://codereview.chromium.org/206673006/diff/1/chrome/browser/chromeos/cust...
chrome/browser/chromeos/customization_document.cc:188: std::string
CustomizationDocument::GetLocaleSpecificStringImpl(
On 2014/03/20 22:12:56, stevenjb wrote:
> Is there a reason for this to be a static method instead of just file local?

Done.

https://codereview.chromium.org/206673006/diff/1/chrome/browser/chromeos/cust...
File chrome/browser/chromeos/customization_document_unittest.cc (right):

https://codereview.chromium.org/206673006/diff/1/chrome/browser/chromeos/cust...
chrome/browser/chromeos/customization_document_unittest.cc:335:
EXPECT_EQ(doc->GetOemFolderName("ru"), "The OEM Name");
On 2014/03/20 22:12:56, stevenjb wrote:
> Maybe add a comment that we expect this to return the default name?
> Also maybe add 'en' also so that we're testing correct lookup of more than the
> first and default items.

Done.

https://codereview.chromium.org/206673006/diff/1/chrome/browser/ui/app_list/a...
File chrome/browser/ui/app_list/app_list_syncable_service.h (right):

https://codereview.chromium.org/206673006/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_syncable_service.h:83: std::string
GetOemFolderNameForTest() { return oem_folder_name_; }
On 2014/03/20 22:12:56, stevenjb wrote:
> nit: return const&

Done.

[stevenjb, 2014-03-20 22:53:40]

lgtm

https://codereview.chromium.org/206673006/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/customization_document.cc (right):

https://codereview.chromium.org/206673006/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document.cc:633: if (!service)
let's log a warning here in case some change ends up with this occurring before
AppListSyncableService is ready.

[Dmitry Polukhin, 2014-03-20 23:08:41]

https://codereview.chromium.org/206673006/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/customization_document.cc (right):

https://codereview.chromium.org/206673006/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document.cc:633: if (!service)
On 2014/03/20 22:53:40, stevenjb wrote:
> let's log a warning here in case some change ends up with this occurring
before
> AppListSyncableService is ready.

Done.

[Nikita (slow), 2014-03-24 04:04:12]

lgtm

https://codereview.chromium.org/206673006/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/customization_document.cc (right):

https://codereview.chromium.org/206673006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document.cc:476:
UpdateCachedManifest((*it)->profile());
It looks like these actions should be extracted into a separate method:

1. (optional) UpdateCacheManifest(profile)
2. loader->SetCurrentApps()
3. SetOemFolderName(profile).

476..479
600..602
609..610

https://codereview.chromium.org/206673006/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/customization_document.h (right):

https://codereview.chromium.org/206673006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document.h:172: // Returns the name of the
folder for OEM apps for given locale.
nit: |locale|

https://codereview.chromium.org/206673006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document.h:173: std::string
GetOemFolderName(const std::string& locale) const;
nit: Rename to GetOemAppsFolderName?

https://codereview.chromium.org/206673006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document.h:230: // Set OEM apps folder
name for AppListSyncableService for the profile.
nit: |profile|

https://codereview.chromium.org/206673006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document.h:233: // Returns the name of the
folder for OEM apps for given locale.
nit: |loclae|

https://codereview.chromium.org/206673006/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/customization_document_unittest.cc (right):

https://codereview.chromium.org/206673006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document_unittest.cc:335:
EXPECT_EQ(doc->GetOemFolderName("en"), "EN OEM Name");
l10n_util.cc will always map "en" to "en-US"
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/l10n/l10n_...

[Dmitry Polukhin, 2014-03-24 04:27:20]

https://codereview.chromium.org/206673006/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/customization_document.cc (right):

https://codereview.chromium.org/206673006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document.cc:476:
UpdateCachedManifest((*it)->profile());
On 2014/03/24 04:04:12, Nikita Kostylev wrote:
> It looks like these actions should be extracted into a separate method:
> 
> 1. (optional) UpdateCacheManifest(profile)
> 2. loader->SetCurrentApps()
> 3. SetOemFolderName(profile).
> 
> 476..479
> 600..602
> 609..610

I'm not sure that it will make code cleaner or smaller - the code use different
Dictionaries and has one optional step. So outlining will only make things
longer.

https://codereview.chromium.org/206673006/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/customization_document.h (right):

https://codereview.chromium.org/206673006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document.h:172: // Returns the name of the
folder for OEM apps for given locale.
On 2014/03/24 04:04:12, Nikita Kostylev wrote:
> nit: |locale|

Done.

https://codereview.chromium.org/206673006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document.h:173: std::string
GetOemFolderName(const std::string& locale) const;
On 2014/03/24 04:04:12, Nikita Kostylev wrote:
> nit: Rename to GetOemAppsFolderName?

Done.

https://codereview.chromium.org/206673006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document.h:230: // Set OEM apps folder
name for AppListSyncableService for the profile.
On 2014/03/24 04:04:12, Nikita Kostylev wrote:
> nit: |profile|

Done.

https://codereview.chromium.org/206673006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document.h:233: // Returns the name of the
folder for OEM apps for given locale.
On 2014/03/24 04:04:12, Nikita Kostylev wrote:
> nit: |loclae|

Done.

https://codereview.chromium.org/206673006/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/customization_document_unittest.cc (right):

https://codereview.chromium.org/206673006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document_unittest.cc:335:
EXPECT_EQ(doc->GetOemFolderName("en"), "EN OEM Name");
On 2014/03/24 04:04:12, Nikita Kostylev wrote:
> l10n_util.cc will always map "en" to "en-US"
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/l10n/l10n_...

Yes, it is just for testing. In tests browser locale is 'en' not 'en-US'.

[Dmitry Polukhin, 2014-03-24 04:30:15]

The CQ bit was checked by dpolukhin@chromium.org

[commit-bot: I haz the power, 2014-03-24 04:30:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dpolukhin@chromium.org/206673006/80001

[Dmitry Polukhin, 2014-03-24 05:37:10]

Committed patchset #5 manually as r258879 (presubmit successful).