Fetch OEM services customization manifest from URL async....

Fetch OEM services customization manifest from URL async.

BUG=chromium-os:5123
TEST=none


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

[Nikita (slow), 2010-08-04 13:03:54]

http://codereview.chromium.org/3026048/diff/1/2
File chrome/browser/chromeos/login/apply_services_customization.cc (right):

http://codereview.chromium.org/3026048/diff/1/2#newcode20
chrome/browser/chromeos/login/apply_services_customization.cc:20: // URL where
to fetch OEM services customization manifest from.
namespace {} for constants

http://codereview.chromium.org/3026048/diff/1/2#newcode25
chrome/browser/chromeos/login/apply_services_customization.cc:25: static const
wchar_t kServicesCustomizationApplied[] =
Comment?

http://codereview.chromium.org/3026048/diff/1/2#newcode30
chrome/browser/chromeos/login/apply_services_customization.cc:30: if
(!prefs->GetBoolean(kServicesCustomizationApplied)) {
Add RegisterPrefs static method and call it from browser_prefs.cc

http://codereview.chromium.org/3026048/diff/1/2#newcode47
chrome/browser/chromeos/login/apply_services_customization.cc:47: return false;
URL is hardcoded. If it is incorrect shouldn't we mark that
ServicesCustomizationApplied pref to stop trying?

http://codereview.chromium.org/3026048/diff/1/2#newcode52
chrome/browser/chromeos/login/apply_services_customization.cc:52: if
(file_util::ReadFileToString(FilePath(url.path()), &manifest))
I suggest that URL with file:/// scheme should be treated as an typical URL and
fetched using URLFetcher.
This way code will only have single path.

http://codereview.chromium.org/3026048/diff/1/3
File chrome/browser/chromeos/login/apply_services_customization.h (right):

http://codereview.chromium.org/3026048/diff/1/3#newcode17
chrome/browser/chromeos/login/apply_services_customization.h:17: // as soon as
the document downloaded.
nit: is downloaded

http://codereview.chromium.org/3026048/diff/1/3#newcode20
chrome/browser/chromeos/login/apply_services_customization.h:20: static void
StartIfNeeded();
Comments?

http://codereview.chromium.org/3026048/diff/1/3#newcode36
chrome/browser/chromeos/login/apply_services_customization.h:36: void
ApplyPartnerServicesCustomizations(const std::string& manifest);
Comment?

http://codereview.chromium.org/3026048/diff/1/6
File chrome/browser/chromeos/login/wizard_controller.cc (right):

http://codereview.chromium.org/3026048/diff/1/6#newcode732
chrome/browser/chromeos/login/wizard_controller.cc:732:
chromeos::ApplyServicesCustomization::StartIfNeeded();
According to design doc this check should happen only after update check.
At this point (start of OOBE) we don't even have network configured.

[Dmitry Polukhin, 2010-08-05 08:41:29]

Added logic to retry if network is no available yet.

http://codereview.chromium.org/3026048/diff/1/2
File chrome/browser/chromeos/login/apply_services_customization.cc (right):

http://codereview.chromium.org/3026048/diff/1/2#newcode20
chrome/browser/chromeos/login/apply_services_customization.cc:20: // URL where
to fetch OEM services customization manifest from.
On 2010/08/04 13:03:54, Nikita Kostylev wrote:
> namespace {} for constants

Done.

http://codereview.chromium.org/3026048/diff/1/2#newcode25
chrome/browser/chromeos/login/apply_services_customization.cc:25: static const
wchar_t kServicesCustomizationApplied[] =
On 2010/08/04 13:03:54, Nikita Kostylev wrote:
> Comment?

Done.

http://codereview.chromium.org/3026048/diff/1/2#newcode30
chrome/browser/chromeos/login/apply_services_customization.cc:30: if
(!prefs->GetBoolean(kServicesCustomizationApplied)) {
On 2010/08/04 13:03:54, Nikita Kostylev wrote:
> Add RegisterPrefs static method and call it from browser_prefs.cc

Done.

http://codereview.chromium.org/3026048/diff/1/2#newcode47
chrome/browser/chromeos/login/apply_services_customization.cc:47: return false;
On 2010/08/04 13:03:54, Nikita Kostylev wrote:
> URL is hardcoded. If it is incorrect shouldn't we mark that
> ServicesCustomizationApplied pref to stop trying?

No we shouldn't because Chrome binaries can be updated and we don't need to
think that customization were applied successfully in local state.

http://codereview.chromium.org/3026048/diff/1/2#newcode52
chrome/browser/chromeos/login/apply_services_customization.cc:52: if
(file_util::ReadFileToString(FilePath(url.path()), &manifest))
On 2010/08/04 13:03:54, Nikita Kostylev wrote:
> I suggest that URL with file:/// scheme should be treated as an typical URL
and
> fetched using URLFetcher.
> This way code will only have single path.

URLFetcher failed to load from file://

http://codereview.chromium.org/3026048/diff/1/3
File chrome/browser/chromeos/login/apply_services_customization.h (right):

http://codereview.chromium.org/3026048/diff/1/3#newcode17
chrome/browser/chromeos/login/apply_services_customization.h:17: // as soon as
the document downloaded.
On 2010/08/04 13:03:54, Nikita Kostylev wrote:
> nit: is downloaded

Done.

http://codereview.chromium.org/3026048/diff/1/3#newcode20
chrome/browser/chromeos/login/apply_services_customization.h:20: static void
StartIfNeeded();
On 2010/08/04 13:03:54, Nikita Kostylev wrote:
> Comments?

Done.

http://codereview.chromium.org/3026048/diff/1/3#newcode36
chrome/browser/chromeos/login/apply_services_customization.h:36: void
ApplyPartnerServicesCustomizations(const std::string& manifest);
On 2010/08/04 13:03:54, Nikita Kostylev wrote:
> Comment?

Done.

[Nikita (slow), 2010-08-05 09:55:05]

LGTM
Please run trybots.

http://codereview.chromium.org/3026048/diff/1/2
File chrome/browser/chromeos/login/apply_services_customization.cc (right):

http://codereview.chromium.org/3026048/diff/1/2#newcode52
chrome/browser/chromeos/login/apply_services_customization.cc:52: if
(file_util::ReadFileToString(FilePath(url.path()), &manifest))
On 2010/08/05 08:41:29, Dmitry Polukhin wrote:
> On 2010/08/04 13:03:54, Nikita Kostylev wrote:
> > I suggest that URL with file:/// scheme should be treated as an typical URL
> and
> > fetched using URLFetcher.
> > This way code will only have single path.
> 
> URLFetcher failed to load from file://

Got it. I was under false impression.

http://codereview.chromium.org/3026048/diff/14002/21001#newcode77
chrome/browser/chromeos/login/apply_services_customization.cc:77: }
As discussed it's OK to continue trying to fetch file even if it's doesn't
exist.

http://codereview.chromium.org/3026048/diff/14002/21002
File chrome/browser/chromeos/login/apply_services_customization.h (right):

http://codereview.chromium.org/3026048/diff/14002/21002#newcode32
chrome/browser/chromeos/login/apply_services_customization.h:32: static bool
IsServicesCustomizationApplied();
nit: might be simplified to IsApplied()

ApplyServicesCustomization::IsApplied();

http://codereview.chromium.org/3026048/diff/14002/21002#newcode74
chrome/browser/chromeos/login/apply_services_customization.h:74: static const
int kMaxFetchRetries = 3;
Could be moved to cc file.

http://codereview.chromium.org/3026048/diff/14002/21005
File chrome/browser/chromeos/login/wizard_controller.cc (right):

http://codereview.chromium.org/3026048/diff/14002/21005#newcode456
chrome/browser/chromeos/login/wizard_controller.cc:456:
chromeos::ApplyServicesCustomization::RegisterPrefs(local_state);
It's better to place this call at browser_prefs

[Dmitry Polukhin, 2010-08-05 10:14:58]

LGTM

http://codereview.chromium.org/3026048/diff/14002/21002
File chrome/browser/chromeos/login/apply_services_customization.h (right):

http://codereview.chromium.org/3026048/diff/14002/21002#newcode32
chrome/browser/chromeos/login/apply_services_customization.h:32: static bool
IsServicesCustomizationApplied();
On 2010/08/05 09:55:05, Nikita Kostylev wrote:
> nit: might be simplified to IsApplied()
> 
> ApplyServicesCustomization::IsApplied();

Done.

http://codereview.chromium.org/3026048/diff/14002/21002#newcode74
chrome/browser/chromeos/login/apply_services_customization.h:74: static const
int kMaxFetchRetries = 3;
On 2010/08/05 09:55:05, Nikita Kostylev wrote:
> Could be moved to cc file.

Done.

http://codereview.chromium.org/3026048/diff/14002/21005
File chrome/browser/chromeos/login/wizard_controller.cc (right):

http://codereview.chromium.org/3026048/diff/14002/21005#newcode456
chrome/browser/chromeos/login/wizard_controller.cc:456:
chromeos::ApplyServicesCustomization::RegisterPrefs(local_state);
On 2010/08/05 09:55:05, Nikita Kostylev wrote:
> It's better to place this call at browser_prefs

Done.

[Dmitry Polukhin, 2010-08-05 10:16:29]

I took wrong tab and gave LGTM to myself :)