Adding initial implementation of the PartnerCustomization classes....

Adding initial implementation of the PartnerCustomization classes.

BUG=chromiumos:3176
TEST=Run out/Debug/unit_tests. Run out/Debug/chrome --login-manager --startup-manifest=./chrome/browser/chromeos/testdata/startup_manifest.json. There should be no asserts.


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

[whywhat, 2010-05-25 16:26:23]

http://codereview.chromium.org/2101021/diff/7001/8001
File chrome/browser/chromeos/login/partner_customization.cc (right):

http://codereview.chromium.org/2101021/diff/7001/8001#newcode9
chrome/browser/chromeos/login/partner_customization.cc:9: #include
"base/json/json_reader.h"
Sort headers alphabetically.

http://codereview.chromium.org/2101021/diff/7001/8001#newcode16
chrome/browser/chromeos/login/partner_customization.cc:16: static const wchar_t
kVersionAttr[] = L"version";
Better put it into unnamed namespace within namespace chromeos and remove static
keyword

http://codereview.chromium.org/2101021/diff/7001/8001#newcode50
chrome/browser/chromeos/login/partner_customization.cc:50: DCHECK(root.get() !=
NULL);
Is DCHECK enough here? What if partner ship bad file and JSON parser won't parse
it?

http://codereview.chromium.org/2101021/diff/7001/8001#newcode51
chrome/browser/chromeos/login/partner_customization.cc:51:
DCHECK(root->GetType() == Value::TYPE_DICTIONARY);
I'd avoid making cast before we check the type. You could store |root| in
scoped_ptr<Value> and cast when passing it to ParseValue.

http://codereview.chromium.org/2101021/diff/7001/8001#newcode69
chrome/browser/chromeos/login/partner_customization.cc:69:
root->GetString(kInitialLocaleAttr, &initial_locale_);
Why don't you return false if these fields are missing? Are these optional?

http://codereview.chromium.org/2101021/diff/7001/8001#newcode73
chrome/browser/chromeos/login/partner_customization.cc:73:
root->GetList(kSetupContentAttr, &setup_content_value);
I'd init setup_content_value with NULL and check for the result of GetList call.

http://codereview.chromium.org/2101021/diff/7001/8001#newcode75
chrome/browser/chromeos/login/partner_customization.cc:75: std::string
content_locale;
Maybe declare setup_content here too? Or I would declare all these variables
within the loop upon their initialization.

http://codereview.chromium.org/2101021/diff/7001/8002
File chrome/browser/chromeos/login/partner_customization.h (right):

http://codereview.chromium.org/2101021/diff/7001/8002#newcode12
chrome/browser/chromeos/login/partner_customization.h:12: #include
"base/file_path.h"
Guess you could just forward declare FilePath class.

http://codereview.chromium.org/2101021/diff/7001/8002#newcode19
chrome/browser/chromeos/login/partner_customization.h:19: class
PartnerCustomization {
You should add some comments about the classes and its members. I.e. what is
sku?

http://codereview.chromium.org/2101021/diff/7001/8002#newcode30
chrome/browser/chromeos/login/partner_customization.h:30: virtual bool
ParseValue(const DictionaryValue* root);
Could be more meaningful name like ParseJSONConfig or ParseJSON. Value is too
generic word.

http://codereview.chromium.org/2101021/diff/7001/8002#newcode50
chrome/browser/chromeos/login/partner_customization.h:50: virtual bool
ParseValue(const DictionaryValue* root);
Should this become public method?

http://codereview.chromium.org/2101021/diff/7001/8002#newcode64
chrome/browser/chromeos/login/partner_customization.h:64: };
DISALLLOW_COPY_AND_ASSIGN?

http://codereview.chromium.org/2101021/diff/7001/8003
File chrome/browser/chromeos/login/wizard_controller.cc (right):

http://codereview.chromium.org/2101021/diff/7001/8003#newcode481
chrome/browser/chromeos/login/wizard_controller.cc:481: customization = new
chromeos::PartnerStartupCustomization();
Who manages the life scope of the object here?
Maybe we should pass it to wizard controller.

http://codereview.chromium.org/2101021/diff/7001/8003#newcode485
chrome/browser/chromeos/login/wizard_controller.cc:485: DCHECK(manifest_loaded);
Could you print filepath with this DCHECK?

[dr, 2010-05-26 13:27:09]

http://codereview.chromium.org/2101021/diff/7001/8001
File chrome/browser/chromeos/login/partner_customization.cc (right):

http://codereview.chromium.org/2101021/diff/7001/8001#newcode9
chrome/browser/chromeos/login/partner_customization.cc:9: #include
"base/json/json_reader.h"
On 2010/05/25 16:26:23, whywhat wrote:
> Sort headers alphabetically.

Done.

http://codereview.chromium.org/2101021/diff/7001/8001#newcode16
chrome/browser/chromeos/login/partner_customization.cc:16: static const wchar_t
kVersionAttr[] = L"version";
On 2010/05/25 16:26:23, whywhat wrote:
> Better put it into unnamed namespace within namespace chromeos and remove
static
> keyword

Done.

http://codereview.chromium.org/2101021/diff/7001/8001#newcode50
chrome/browser/chromeos/login/partner_customization.cc:50: DCHECK(root.get() !=
NULL);
On 2010/05/25 16:26:23, whywhat wrote:
> Is DCHECK enough here? What if partner ship bad file and JSON parser won't
parse
> it?

Done.

http://codereview.chromium.org/2101021/diff/7001/8001#newcode51
chrome/browser/chromeos/login/partner_customization.cc:51:
DCHECK(root->GetType() == Value::TYPE_DICTIONARY);
On 2010/05/25 16:26:23, whywhat wrote:
> I'd avoid making cast before we check the type. You could store |root| in
> scoped_ptr<Value> and cast when passing it to ParseValue.

Done.

http://codereview.chromium.org/2101021/diff/7001/8001#newcode69
chrome/browser/chromeos/login/partner_customization.cc:69:
root->GetString(kInitialLocaleAttr, &initial_locale_);
On 2010/05/25 16:26:23, whywhat wrote:
> Why don't you return false if these fields are missing? Are these optional?
Yes. Added comment.

http://codereview.chromium.org/2101021/diff/7001/8001#newcode73
chrome/browser/chromeos/login/partner_customization.cc:73:
root->GetList(kSetupContentAttr, &setup_content_value);
On 2010/05/25 16:26:23, whywhat wrote:
> I'd init setup_content_value with NULL and check for the result of GetList
call.

Done.

http://codereview.chromium.org/2101021/diff/7001/8001#newcode75
chrome/browser/chromeos/login/partner_customization.cc:75: std::string
content_locale;
On 2010/05/25 16:26:23, whywhat wrote:
> Maybe declare setup_content here too? Or I would declare all these variables
> within the loop upon their initialization.

Done.

http://codereview.chromium.org/2101021/diff/7001/8002
File chrome/browser/chromeos/login/partner_customization.h (right):

http://codereview.chromium.org/2101021/diff/7001/8002#newcode12
chrome/browser/chromeos/login/partner_customization.h:12: #include
"base/file_path.h"
On 2010/05/25 16:26:23, whywhat wrote:
> Guess you could just forward declare FilePath class.

No, I use const reference to it.

http://codereview.chromium.org/2101021/diff/7001/8002#newcode19
chrome/browser/chromeos/login/partner_customization.h:19: class
PartnerCustomization {
On 2010/05/25 16:26:23, whywhat wrote:
> You should add some comments about the classes and its members. I.e. what is
> sku?

Done.

http://codereview.chromium.org/2101021/diff/7001/8002#newcode30
chrome/browser/chromeos/login/partner_customization.h:30: virtual bool
ParseValue(const DictionaryValue* root);
On 2010/05/25 16:26:23, whywhat wrote:
> Could be more meaningful name like ParseJSONConfig or ParseJSON. Value is too
> generic word.

Done.

http://codereview.chromium.org/2101021/diff/7001/8002#newcode50
chrome/browser/chromeos/login/partner_customization.h:50: virtual bool
ParseValue(const DictionaryValue* root);
On 2010/05/25 16:26:23, whywhat wrote:
> Should this become public method?

Definitely not.

http://codereview.chromium.org/2101021/diff/7001/8003
File chrome/browser/chromeos/login/wizard_controller.cc (right):

http://codereview.chromium.org/2101021/diff/7001/8003#newcode481
chrome/browser/chromeos/login/wizard_controller.cc:481: customization = new
chromeos::PartnerStartupCustomization();
On 2010/05/25 16:26:23, whywhat wrote:
> Who manages the life scope of the object here?
> Maybe we should pass it to wizard controller.

Done.

http://codereview.chromium.org/2101021/diff/7001/8003#newcode485
chrome/browser/chromeos/login/wizard_controller.cc:485: DCHECK(manifest_loaded);
On 2010/05/25 16:26:23, whywhat wrote:
> Could you print filepath with this DCHECK?

Done.

[whywhat, 2010-05-26 14:37:40]

LGTM overall.

http://codereview.chromium.org/2101021/diff/7001/8002
File chrome/browser/chromeos/login/partner_customization.h (right):

http://codereview.chromium.org/2101021/diff/7001/8002#newcode12
chrome/browser/chromeos/login/partner_customization.h:12: #include
"base/file_path.h"
On 2010/05/26 13:27:09, denisromanov wrote:
> On 2010/05/25 16:26:23, whywhat wrote:
> > Guess you could just forward declare FilePath class.
> 
> No, I use const reference to it.

The problem that occurs when you not include the header base/file_path.h is that
base/basictypes.h is not included. You may include basictypes.h and forward
declare FilePath.

http://codereview.chromium.org/2101021/diff/24001/25001
File chrome/browser/chromeos/login/partner_customization.cc (right):

http://codereview.chromium.org/2101021/diff/24001/25001#newcode85
chrome/browser/chromeos/login/partner_customization.cc:85: NOTREACHED();
I'd rather do:
if (something wrong) {
  NOTREACHED(); // Debug check
  return false;
}
background_color = ...

http://codereview.chromium.org/2101021/diff/24001/25001#newcode111
chrome/browser/chromeos/login/partner_customization.cc:111: const
PartnerStartupCustomization::SetupContent*
This long line scares me!

http://codereview.chromium.org/2101021/diff/24001/25002
File chrome/browser/chromeos/login/partner_customization.h (right):

http://codereview.chromium.org/2101021/diff/24001/25002#newcode48
chrome/browser/chromeos/login/partner_customization.h:48: // Partner's help page
for specific locale.
Could you put a blank line before the comment?

http://codereview.chromium.org/2101021/diff/24001/25002#newcode60
chrome/browser/chromeos/login/partner_customization.h:60: int
get_background_color() const { return background_color_; }
Can't we return SkColor here?

http://codereview.chromium.org/2101021/diff/24001/25002#newcode62
chrome/browser/chromeos/login/partner_customization.h:62: const SetupContent*
get_setup_content(const std::string& locale) const;
If this method is not a trivial return, it should be named GetSetupContent and
maybe have a comment.

http://codereview.chromium.org/2101021/diff/24001/25002#newcode67
chrome/browser/chromeos/login/partner_customization.h:67: // Product SKU.
What is SKU?

http://codereview.chromium.org/2101021/diff/24001/25003
File chrome/browser/chromeos/login/partner_customization_unittest.cc (right):

http://codereview.chromium.org/2101021/diff/24001/25003#newcode9
chrome/browser/chromeos/login/partner_customization_unittest.cc:9: static const
char kGoodStartupManifest[] =
unnamed namespace instead of static?

http://codereview.chromium.org/2101021/diff/24001/25003#newcode38
chrome/browser/chromeos/login/partner_customization_unittest.cc:38:
TEST_F(PartnerStartupCustomizationTest, LoadBadStartupManifestFromString) {
It would be nice if test covered all if's/lines in the code :)

http://codereview.chromium.org/2101021/diff/24001/25004
File chrome/browser/chromeos/login/wizard_controller.cc (right):

http://codereview.chromium.org/2101021/diff/24001/25004#newcode494
chrome/browser/chromeos/login/wizard_controller.cc:494:
chromeos::PartnerStartupCustomization* customization = NULL;
Could you use scoped_ptr here too?

http://codereview.chromium.org/2101021/diff/24001/25005
File chrome/browser/chromeos/login/wizard_controller.h (right):

http://codereview.chromium.org/2101021/diff/24001/25005#newcode41
chrome/browser/chromeos/login/wizard_controller.h:41: explicit WizardController(
I think it's better to have set_customization() method, it could be useful for
testing. Comment about ownership would be good too.

http://codereview.chromium.org/2101021/diff/24001/25005#newcode160
chrome/browser/chromeos/login/wizard_controller.h:160: // Partnre startup
customizations.
Typo: partnre -> partner

http://codereview.chromium.org/2101021/diff/24001/25008
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/2101021/diff/24001/25008#newcode1458
chrome/chrome_tests.gypi:1458:
'browser/chromeos/login/partner_customization_unittest.cc',
I guess this section is for browsertests only. Unittest should be added to some
other place.

[Nikita (slow), 2010-05-26 15:04:04]

http://codereview.chromium.org/2101021/diff/24001/25002
File chrome/browser/chromeos/login/partner_customization.h (right):

http://codereview.chromium.org/2101021/diff/24001/25002#newcode45
chrome/browser/chromeos/login/partner_customization.h:45: const std::string&
eula_page_url)
Help page and EULA are HTML documents on a file system.
Please rename all variables to reflect that.

http://codereview.chromium.org/2101021/diff/24001/25002#newcode69
chrome/browser/chromeos/login/partner_customization.h:69: // Initial locale for
the OOBE wizard.
Please add empty line after each declaration.
This way it's more readable.

http://codereview.chromium.org/2101021/diff/24001/25002#newcode86
chrome/browser/chromeos/login/partner_customization.h:86: // Partner's welcome
page that is opened right after the OOBE.
Please add empty lines.

[zel, 2010-05-26 17:27:01]

http://codereview.chromium.org/2101021/diff/24001/25002
File chrome/browser/chromeos/login/partner_customization.h (right):

http://codereview.chromium.org/2101021/diff/24001/25002#newcode40
chrome/browser/chromeos/login/partner_customization.h:40: class
PartnerStartupCustomization : public PartnerCustomization {
The class name implies action as 'customization' - while it is only a model. I
would rather call it StartupCustomizationDocument (and base class
CustomizationDocument)

http://codereview.chromium.org/2101021/diff/24001/25006
File chrome/browser/chromeos/testdata/startup_manifest.json (right):

http://codereview.chromium.org/2101021/diff/24001/25006#newcode13
chrome/browser/chromeos/testdata/startup_manifest.json:13: "help_page" :
"/etc/chromeos/partner/content/en_US/help.html",
We should make file paths here relative to where this manifest file resides. All
these files will live on a partner partition, grouped somewhere in the same
(sub)directory

http://codereview.chromium.org/2101021/diff/24001/25006#newcode19
chrome/browser/chromeos/testdata/startup_manifest.json:19: "eula_page" :
"/etc/chromeos/partner/content/ru_RU/eula.html",
do you need full locale? isn't google's hl enough for chromeos?

http://codereview.chromium.org/2101021/diff/24001/25009
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/2101021/diff/24001/25009#newcode905
chrome/common/chrome_switches.cc:905: // TODO(denisromanov): delate this once
it's not needed for testing anymore.
I hope this is just a temporary mechanism for passing manifest info. The correct
implementation should mount partner partition and read this file from a known
location.
 
Few other issues:
- typo: delete
- include bug number for TODO items

[dr, 2010-05-26 17:33:38]

http://codereview.chromium.org/2101021/diff/24001/25001
File chrome/browser/chromeos/login/partner_customization.cc (right):

http://codereview.chromium.org/2101021/diff/24001/25001#newcode85
chrome/browser/chromeos/login/partner_customization.cc:85: NOTREACHED();
On 2010/05/26 14:37:40, whywhat wrote:
> I'd rather do:
> if (something wrong) {
>   NOTREACHED(); // Debug check
>   return false;
> }
> background_color = ...
This was for possible literal color constant representation. Removed.

http://codereview.chromium.org/2101021/diff/24001/25001#newcode111
chrome/browser/chromeos/login/partner_customization.cc:111: const
PartnerStartupCustomization::SetupContent*
On 2010/05/26 14:37:40, whywhat wrote:
> This long line scares me!
Me too!

http://codereview.chromium.org/2101021/diff/24001/25002
File chrome/browser/chromeos/login/partner_customization.h (right):

http://codereview.chromium.org/2101021/diff/24001/25002#newcode48
chrome/browser/chromeos/login/partner_customization.h:48: // Partner's help page
for specific locale.
On 2010/05/26 14:37:40, whywhat wrote:
> Could you put a blank line before the comment?

Done.

http://codereview.chromium.org/2101021/diff/24001/25002#newcode60
chrome/browser/chromeos/login/partner_customization.h:60: int
get_background_color() const { return background_color_; }
On 2010/05/26 14:37:40, whywhat wrote:
> Can't we return SkColor here?

Done.

http://codereview.chromium.org/2101021/diff/24001/25002#newcode62
chrome/browser/chromeos/login/partner_customization.h:62: const SetupContent*
get_setup_content(const std::string& locale) const;
On 2010/05/26 14:37:40, whywhat wrote:
> If this method is not a trivial return, it should be named GetSetupContent and
> maybe have a comment.

Done.

http://codereview.chromium.org/2101021/diff/24001/25002#newcode67
chrome/browser/chromeos/login/partner_customization.h:67: // Product SKU.
On 2010/05/26 14:37:40, whywhat wrote:
> What is SKU?
Stock Keeping Unit.

http://codereview.chromium.org/2101021/diff/24001/25003
File chrome/browser/chromeos/login/partner_customization_unittest.cc (right):

http://codereview.chromium.org/2101021/diff/24001/25003#newcode38
chrome/browser/chromeos/login/partner_customization_unittest.cc:38:
TEST_F(PartnerStartupCustomizationTest, LoadBadStartupManifestFromString) {
On 2010/05/26 14:37:40, whywhat wrote:
> It would be nice if test covered all if's/lines in the code :)

Added some more cases.

[dr, 2010-05-27 12:36:25]

http://codereview.chromium.org/2101021/diff/24001/25002
File chrome/browser/chromeos/login/partner_customization.h (right):

http://codereview.chromium.org/2101021/diff/24001/25002#newcode40
chrome/browser/chromeos/login/partner_customization.h:40: class
PartnerStartupCustomization : public PartnerCustomization {
On 2010/05/26 17:27:02, zel wrote:
> The class name implies action as 'customization' - while it is only a model. I
> would rather call it StartupCustomizationDocument (and base class
> CustomizationDocument)

Done.

http://codereview.chromium.org/2101021/diff/24001/25002#newcode45
chrome/browser/chromeos/login/partner_customization.h:45: const std::string&
eula_page_url)
On 2010/05/26 15:04:05, Nikita Kostylev wrote:
> Help page and EULA are HTML documents on a file system.
> Please rename all variables to reflect that.

Done.

http://codereview.chromium.org/2101021/diff/24001/25002#newcode69
chrome/browser/chromeos/login/partner_customization.h:69: // Initial locale for
the OOBE wizard.
On 2010/05/26 15:04:05, Nikita Kostylev wrote:
> Please add empty line after each declaration.
> This way it's more readable.

Done.

http://codereview.chromium.org/2101021/diff/24001/25002#newcode86
chrome/browser/chromeos/login/partner_customization.h:86: // Partner's welcome
page that is opened right after the OOBE.
On 2010/05/26 15:04:05, Nikita Kostylev wrote:
> Please add empty lines.

Done.

http://codereview.chromium.org/2101021/diff/24001/25003
File chrome/browser/chromeos/login/partner_customization_unittest.cc (right):

http://codereview.chromium.org/2101021/diff/24001/25003#newcode9
chrome/browser/chromeos/login/partner_customization_unittest.cc:9: static const
char kGoodStartupManifest[] =
On 2010/05/26 14:37:40, whywhat wrote:
> unnamed namespace instead of static?

Done.

http://codereview.chromium.org/2101021/diff/24001/25004
File chrome/browser/chromeos/login/wizard_controller.cc (right):

http://codereview.chromium.org/2101021/diff/24001/25004#newcode494
chrome/browser/chromeos/login/wizard_controller.cc:494:
chromeos::PartnerStartupCustomization* customization = NULL;
On 2010/05/26 14:37:40, whywhat wrote:
> Could you use scoped_ptr here too?

Done.

http://codereview.chromium.org/2101021/diff/24001/25005
File chrome/browser/chromeos/login/wizard_controller.h (right):

http://codereview.chromium.org/2101021/diff/24001/25005#newcode41
chrome/browser/chromeos/login/wizard_controller.h:41: explicit WizardController(
On 2010/05/26 14:37:40, whywhat wrote:
> I think it's better to have set_customization() method, it could be useful for
> testing. Comment about ownership would be good too.

Done.

http://codereview.chromium.org/2101021/diff/24001/25005#newcode160
chrome/browser/chromeos/login/wizard_controller.h:160: // Partnre startup
customizations.
On 2010/05/26 14:37:40, whywhat wrote:
> Typo: partnre -> partner

Done.

http://codereview.chromium.org/2101021/diff/24001/25006
File chrome/browser/chromeos/testdata/startup_manifest.json (right):

http://codereview.chromium.org/2101021/diff/24001/25006#newcode13
chrome/browser/chromeos/testdata/startup_manifest.json:13: "help_page" :
"/etc/chromeos/partner/content/en_US/help.html",
On 2010/05/26 17:27:02, zel wrote:
> We should make file paths here relative to where this manifest file resides.
All
> these files will live on a partner partition, grouped somewhere in the same
> (sub)directory

Done.

http://codereview.chromium.org/2101021/diff/24001/25006#newcode19
chrome/browser/chromeos/testdata/startup_manifest.json:19: "eula_page" :
"/etc/chromeos/partner/content/ru_RU/eula.html",
On 2010/05/26 17:27:02, zel wrote:
> do you need full locale? isn't google's hl enough for chromeos?
Could be. Changed ru_Ru to just ru. Anyway we will be loading all locales and
content paths available in the manifest, and then OEM customization logic will
decide which one to show.

http://codereview.chromium.org/2101021/diff/24001/25008
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/2101021/diff/24001/25008#newcode1458
chrome/chrome_tests.gypi:1458:
'browser/chromeos/login/partner_customization_unittest.cc',
On 2010/05/26 14:37:40, whywhat wrote:
> I guess this section is for browsertests only. Unittest should be added to
some
> other place.

Done.

http://codereview.chromium.org/2101021/diff/24001/25009
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/2101021/diff/24001/25009#newcode905
chrome/common/chrome_switches.cc:905: // TODO(denisromanov): delate this once
it's not needed for testing anymore.
On 2010/05/26 17:27:02, zel wrote:
> I hope this is just a temporary mechanism for passing manifest info. The
correct
> implementation should mount partner partition and read this file from a known
> location.
>  
> Few other issues:
> - typo: delete
> - include bug number for TODO items
> 

Yes, this is temporary until we have valid filesystem and static data for
partner partition in debug builds.

[tfarina, 2010-05-27 12:56:22]

http://codereview.chromium.org/2101021/diff/43002/44002
File chrome/browser/chromeos/login/partner_customization.h (right):

http://codereview.chromium.org/2101021/diff/43002/44002#newcode29
chrome/browser/chromeos/login/partner_customization.h:29: const std::string&
get_version() const { return version_; }
this should be just: version()

http://codereview.chromium.org/2101021/diff/43002/44002#newcode61
chrome/browser/chromeos/login/partner_customization.h:61: const std::string&
get_product_sku() const { return product_sku_; }
foo() const { return foo_; }

[Nikita (slow), 2010-05-27 12:59:17]

http://codereview.chromium.org/2101021/diff/43002/44002
File chrome/browser/chromeos/login/partner_customization.h (right):

http://codereview.chromium.org/2101021/diff/43002/44002#newcode1
chrome/browser/chromeos/login/partner_customization.h:1: // Copyright (c) 2010
The Chromium Authors. All rights reserved.
I believe partner_customization* files should be renamed too.

[dr, 2010-05-27 17:25:12]

http://codereview.chromium.org/2101021/diff/43002/44002
File chrome/browser/chromeos/login/partner_customization.h (right):

http://codereview.chromium.org/2101021/diff/43002/44002#newcode1
chrome/browser/chromeos/login/partner_customization.h:1: // Copyright (c) 2010
The Chromium Authors. All rights reserved.
On 2010/05/27 12:59:17, Nikita Kostylev wrote:
> I believe partner_customization* files should be renamed too.

Done.

http://codereview.chromium.org/2101021/diff/43002/44002#newcode29
chrome/browser/chromeos/login/partner_customization.h:29: const std::string&
get_version() const { return version_; }
On 2010/05/27 12:56:22, tfarina wrote:
> this should be just: version()

Done.

http://codereview.chromium.org/2101021/diff/43002/44002#newcode61
chrome/browser/chromeos/login/partner_customization.h:61: const std::string&
get_product_sku() const { return product_sku_; }
On 2010/05/27 12:56:22, tfarina wrote:
> foo() const { return foo_; }

Done.

[tfarina, 2010-05-27 17:32:44]

http://codereview.chromium.org/2101021/diff/52001/53002
File chrome/browser/chromeos/customization_document.h (right):

http://codereview.chromium.org/2101021/diff/52001/53002#newcode85
chrome/browser/chromeos/customization_document.h:85: };
DISALLOW_COPY_AND_ASSIGN, below too?

http://codereview.chromium.org/2101021/diff/52001/53004
File chrome/browser/chromeos/login/wizard_controller.cc (right):

http://codereview.chromium.org/2101021/diff/52001/53004#newcode17
chrome/browser/chromeos/login/wizard_controller.cc:17: #include
"chrome/browser/chromeos/customization_document.h"
sort this alphabetical.

http://codereview.chromium.org/2101021/diff/52001/53004#newcode168
chrome/browser/chromeos/login/wizard_controller.cc:168: : widget_(NULL),
the old indentation is correct.

[zel, 2010-05-27 17:55:08]

LGTM

http://codereview.chromium.org/2101021/diff/24001/25006
File chrome/browser/chromeos/testdata/startup_manifest.json (right):

http://codereview.chromium.org/2101021/diff/24001/25006#newcode19
chrome/browser/chromeos/testdata/startup_manifest.json:19: "eula_page" :
"/etc/chromeos/partner/content/ru_RU/eula.html",
On 2010/05/27 12:36:25, denisromanov wrote:
> On 2010/05/26 17:27:02, zel wrote:
> > do you need full locale? isn't google's hl enough for chromeos?
> Could be. Changed ru_Ru to just ru. Anyway we will be loading all locales and
> content paths available in the manifest, and then OEM customization logic will
> decide which one to show.
> 

OEM customization logic will be able to show only locales that exactly match
language code in chrome, so we should enforce that content_locale matches one of
those values and if not, then just reject the entry.

[dr, 2010-05-27 18:11:21]

http://codereview.chromium.org/2101021/diff/52001/53002
File chrome/browser/chromeos/customization_document.h (right):

http://codereview.chromium.org/2101021/diff/52001/53002#newcode85
chrome/browser/chromeos/customization_document.h:85: };
On 2010/05/27 17:32:45, tfarina wrote:
> DISALLOW_COPY_AND_ASSIGN, below too?

OK

http://codereview.chromium.org/2101021/diff/52001/53004
File chrome/browser/chromeos/login/wizard_controller.cc (right):

http://codereview.chromium.org/2101021/diff/52001/53004#newcode17
chrome/browser/chromeos/login/wizard_controller.cc:17: #include
"chrome/browser/chromeos/customization_document.h"
On 2010/05/27 17:32:45, tfarina wrote:
> sort this alphabetical.

Done.

http://codereview.chromium.org/2101021/diff/52001/53004#newcode168
chrome/browser/chromeos/login/wizard_controller.cc:168: : widget_(NULL),
On 2010/05/27 17:32:45, tfarina wrote:
> the old indentation is correct.

Done.