Expand VPD initial_locale to a list of locales. Use the expanded VPD initial_locale on the OOBE.

Expand VPD initial_locale to a list of locales. Use the expanded VPD initial_locale on the OOBE.

Let's have device "most relevant" locales to be the first ones in the list
of locales at OOBE. To store "most relevant" list we expand VPD initial_locale
to contain list of locales.

BUG=334495, 334509
TEST=manual

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

[Alexander Alekseev, 2014-01-28 13:15:49]

As a side-effest, Accept-Language header will also be sorted according to VPD.

[Nikita (slow), 2014-01-28 14:30:45]

On 2014/01/28 13:15:49, alemate wrote:
> As a side-effest, Accept-Language header will also be sorted according to VPD.

As discussed offline this change does only affect default ordering of
accept-language header.
User may always open settings and reorder those languages.

[Alexander Alekseev, 2014-01-28 16:47:57]

I've added OptGroup to language Select element.
And fixed unittest.

Please review.

[Dmitry Polukhin, 2014-01-28 19:40:29]

lgtm

https://codereview.chromium.org/144363006/diff/20001/chrome/app/chromeos_stri...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/144363006/diff/20001/chrome/app/chromeos_stri...
chrome/app/chromeos_strings.grdp:344: <message name="IDS_OTHER_LANGUAGES"
desc="Option group name dividing vendor-configured languages from other
available languages in OOBE 'Select language' select control.">
I would call it IDS_OOBE_OTHER_LANGUAGES for consistency.

https://codereview.chromium.org/144363006/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc (right):

https://codereview.chromium.org/144363006/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:258: if
(value.empty())
Please add {} because it if(true) statement has two lines and bellow please do
the same.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...

https://codereview.chromium.org/144363006/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.h
(right):

https://codereview.chromium.org/144363006/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.h:102:
const bool insert_divider);
nit, const here has no sense and we usually omit it.

[Alexander Alekseev, 2014-01-29 15:38:59]

https://codereview.chromium.org/144363006/diff/20001/chrome/app/chromeos_stri...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/144363006/diff/20001/chrome/app/chromeos_stri...
chrome/app/chromeos_strings.grdp:344: <message name="IDS_OTHER_LANGUAGES"
desc="Option group name dividing vendor-configured languages from other
available languages in OOBE 'Select language' select control.">
On 2014/01/28 19:40:30, Dmitry Polukhin wrote:
> I would call it IDS_OOBE_OTHER_LANGUAGES for consistency.

Done.

https://codereview.chromium.org/144363006/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc (right):

https://codereview.chromium.org/144363006/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:258: if
(value.empty())
On 2014/01/28 19:40:30, Dmitry Polukhin wrote:
> Please add {} because it if(true) statement has two lines and bellow please do
> the same.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...

Done.

https://codereview.chromium.org/144363006/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.h
(right):

https://codereview.chromium.org/144363006/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.h:102:
const bool insert_divider);
On 2014/01/28 19:40:30, Dmitry Polukhin wrote:
> nit, const here has no sense and we usually omit it.

Done.

[Dmitry Polukhin, 2014-01-29 16:17:27]

lgtm

[Nikita (slow), 2014-01-30 13:23:27]

Please add tests for new functionality in 

StartupCustomizationDocument
CrosLanguageOptionsHandler::GetLanguageListInternal

https://codereview.chromium.org/144363006/diff/40001/chrome/app/chromeos_stri...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/144363006/diff/40001/chrome/app/chromeos_stri...
chrome/app/chromeos_strings.grdp:344: <message name="IDS_OOBE_OTHER_LANGUAGES"
desc="Option group name dividing vendor-configured languages from other
available languages in OOBE 'Select language' select control.">
nit: OOBE > out-of-box

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

https://codereview.chromium.org/144363006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document.cc:220:
configured_locales_.push_back(std::string());
I think this breaks assumptions.
If initial_locale VPD value is empty then configured_locales() getter should
return empty vector. One could even check its size.

Caller of configured_locales() should not check its content for valid values.

Just make initial_locale_default() return empty string in that case.

Another thing should always happen: if initial_locale_default() is empty string
(i.e. empty VPD values + customization manifest missing), locale will be en-US.
I think this already is the case.

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

https://codereview.chromium.org/144363006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document.h:8: #include <string>
<vector>

https://codereview.chromium.org/144363006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document.h:82: // Vector of individual
locale values.
nit: Insert empty line before comment (same below).

https://codereview.chromium.org/144363006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document.h:84: //
configured_locales().front() - default locale value.
Please rewrite comment and remove configured_locales().front() mention. No need
to expose method implementation.

https://codereview.chromium.org/144363006/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc (right):

https://codereview.chromium.org/144363006/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:258: if
(value.empty()) {
nit: Check for a specific constant instead.

https://codereview.chromium.org/144363006/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc
(right):

https://codereview.chromium.org/144363006/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc:223:
// The auxiliary vector mentioned above. (except vendor locales)
nit: Insert empty line before comment (and below).

https://codereview.chromium.org/144363006/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc:316:
dictionary->SetString("code", "");
nit: As discussed, let's use a specific string constant that will mean a divider
element. This way if someone inserts invalid input_method manager code (empty
string) that will not become a divider.

https://codereview.chromium.org/144363006/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.h
(right):

https://codereview.chromium.org/144363006/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.h:60: // 
 An entry with empty "code" is used as a divider to separate "most
nit: Update comment to reflect that divider constant.

[Alexander Alekseev, 2014-01-31 12:46:53]

Please review.

https://codereview.chromium.org/144363006/diff/40001/chrome/app/chromeos_stri...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/144363006/diff/40001/chrome/app/chromeos_stri...
chrome/app/chromeos_strings.grdp:344: <message name="IDS_OOBE_OTHER_LANGUAGES"
desc="Option group name dividing vendor-configured languages from other
available languages in OOBE 'Select language' select control.">
On 2014/01/30 13:23:28, Nikita Kostylev wrote:
> nit: OOBE > out-of-box

Done.

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

https://codereview.chromium.org/144363006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document.cc:220:
configured_locales_.push_back(std::string());
In offline discussion we've decided to leave this as is.

On 2014/01/30 13:23:28, Nikita Kostylev wrote:
> I think this breaks assumptions.
> If initial_locale VPD value is empty then configured_locales() getter should
> return empty vector. One could even check its size.
> 
> Caller of configured_locales() should not check its content for valid values.
> 
> Just make initial_locale_default() return empty string in that case.
> 
> Another thing should always happen: if initial_locale_default() is empty
string
> (i.e. empty VPD values + customization manifest missing), locale will be
en-US.
> I think this already is the case.

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

https://codereview.chromium.org/144363006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document.h:8: #include <string>
On 2014/01/30 13:23:28, Nikita Kostylev wrote:
> <vector>

Done.

https://codereview.chromium.org/144363006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document.h:82: // Vector of individual
locale values.
On 2014/01/30 13:23:28, Nikita Kostylev wrote:
> nit: Insert empty line before comment (same below).

Done.

https://codereview.chromium.org/144363006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/customization_document.h:84: //
configured_locales().front() - default locale value.
On 2014/01/30 13:23:28, Nikita Kostylev wrote:
> Please rewrite comment and remove configured_locales().front() mention. No
need
> to expose method implementation.

Done.

https://codereview.chromium.org/144363006/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc (right):

https://codereview.chromium.org/144363006/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:258: if
(value.empty()) {
On 2014/01/30 13:23:28, Nikita Kostylev wrote:
> nit: Check for a specific constant instead.

Done.

https://codereview.chromium.org/144363006/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc
(right):

https://codereview.chromium.org/144363006/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc:223:
// The auxiliary vector mentioned above. (except vendor locales)
On 2014/01/30 13:23:28, Nikita Kostylev wrote:
> nit: Insert empty line before comment (and below).

Done.

https://codereview.chromium.org/144363006/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc:316:
dictionary->SetString("code", "");
On 2014/01/30 13:23:28, Nikita Kostylev wrote:
> nit: As discussed, let's use a specific string constant that will mean a
divider
> element. This way if someone inserts invalid input_method manager code (empty
> string) that will not become a divider.

Done.

https://codereview.chromium.org/144363006/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.h
(right):

https://codereview.chromium.org/144363006/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.h:60: // 
 An entry with empty "code" is used as a divider to separate "most
On 2014/01/30 13:23:28, Nikita Kostylev wrote:
> nit: Update comment to reflect that divider constant.

Done.

[Nikita (slow), 2014-01-31 15:47:20]

lgtm with nits

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

https://codereview.chromium.org/144363006/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/customization_document.h:32: class
CrosLanguageOptionsHandlerTest_GetUILanguageListMulti_Test;
As discussed, these should be removed.

You can just add CrosLanguageOptionsHandlerTest base class as a friend class and
access private methods from test cases via a method in that base class.

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

https://codereview.chromium.org/144363006/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/customization_document_unittest.cc:188:
EXPECT_EQ((size_t)4, customization.configured_locales().size());
nit: insert space after (size_t)

https://codereview.chromium.org/144363006/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc
(right):

https://codereview.chromium.org/144363006/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc:60:
"VENDOR_OTHER_LANGUAGES_LIST_DIVIDER";
nit: How about changing to a short constant as "divider".

You'll be comparing this string with each string, right?

https://codereview.chromium.org/144363006/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.h
(right):

https://codereview.chromium.org/144363006/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.h:19:
extern const char kVENDOR_OTHER_LANGUAGES_LIST_DIVIDER[];
nit: Please follow kConstantName naming style.

https://codereview.chromium.org/144363006/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.h:65: // 
 An entry with "code" attribute set to
nit: Drop extra spaces after //

https://codereview.chromium.org/144363006/diff/100001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler_unittest.cc
(right):

https://codereview.chromium.org/144363006/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler_unittest.cc:205:
EXPECT_GT(list->GetSize(), (size_t)5);
nit: Insert space after (size_t)

[Dmitry Polukhin, 2014-01-31 17:04:40]

lgtm

https://codereview.chromium.org/144363006/diff/100001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler_unittest.cc
(right):

https://codereview.chromium.org/144363006/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler_unittest.cc:205:
EXPECT_GT(list->GetSize(), (size_t)5);
On 2014/01/31 15:47:20, Nikita Kostylev wrote:
> nit: Insert space after (size_t)

Please don't use C-style cast. Use static_cast<size_t>(5) or IMHO size_t(5) (not
encouraged in style guide but I would object).

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Casting

[Alexander Alekseev, 2014-01-31 17:20:59]

I'll commit this.

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

https://codereview.chromium.org/144363006/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/customization_document.h:32: class
CrosLanguageOptionsHandlerTest_GetUILanguageListMulti_Test;
On 2014/01/31 15:47:20, Nikita Kostylev wrote:
> As discussed, these should be removed.
> 
> You can just add CrosLanguageOptionsHandlerTest base class as a friend class
and
> access private methods from test cases via a method in that base class.

Done.

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

https://codereview.chromium.org/144363006/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/customization_document_unittest.cc:188:
EXPECT_EQ((size_t)4, customization.configured_locales().size());
On 2014/01/31 15:47:20, Nikita Kostylev wrote:
> nit: insert space after (size_t)

Done.

https://codereview.chromium.org/144363006/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc
(right):

https://codereview.chromium.org/144363006/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc:60:
"VENDOR_OTHER_LANGUAGES_LIST_DIVIDER";
On 2014/01/31 15:47:20, Nikita Kostylev wrote:
> nit: How about changing to a short constant as "divider".
> 
> You'll be comparing this string with each string, right?

Languages are all lower-case, so any string starting with capital letter is very
fast to compare. I've chosen a long one becase it is
self-descripting.

https://codereview.chromium.org/144363006/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.h
(right):

https://codereview.chromium.org/144363006/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.h:19:
extern const char kVENDOR_OTHER_LANGUAGES_LIST_DIVIDER[];
On 2014/01/31 15:47:20, Nikita Kostylev wrote:
> nit: Please follow kConstantName naming style.

Done.

https://codereview.chromium.org/144363006/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.h:65: // 
 An entry with "code" attribute set to
On 2014/01/31 15:47:20, Nikita Kostylev wrote:
> nit: Drop extra spaces after //

Done.

https://codereview.chromium.org/144363006/diff/100001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler_unittest.cc
(right):

https://codereview.chromium.org/144363006/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler_unittest.cc:205:
EXPECT_GT(list->GetSize(), (size_t)5);
On 2014/01/31 15:47:20, Nikita Kostylev wrote:
> nit: Insert space after (size_t)

Done.

https://codereview.chromium.org/144363006/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler_unittest.cc:205:
EXPECT_GT(list->GetSize(), (size_t)5);
On 2014/01/31 17:04:41, Dmitry Polukhin wrote:
> On 2014/01/31 15:47:20, Nikita Kostylev wrote:
> > nit: Insert space after (size_t)
> 
> Please don't use C-style cast. Use static_cast<size_t>(5) or IMHO size_t(5)
(not
> encouraged in style guide but I would object).
> 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Casting

I've changed it to suffix-style "5u".

[Alexander Alekseev, 2014-01-31 17:21:11]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2014-01-31 17:21:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alemate@chromium.org/144363006/270001

[commit-bot: I haz the power, 2014-01-31 19:24:19]

Change committed as 248241

[commit-bot: I haz the power, 2014-01-31 19:24:24]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 19:24:27]

CQ bit was unchecked on CL. Ignoring.